Wouldn't the change be too intrusive? Now any class that uses security
would need to have this additional constructor, and if a no-arg constructor
is available, it's error prone that the class will be used without
security, and if we delete the no-arg constructor, that would cause a lot
of chain reaction.
The usage of shiro itself is static (all the state information is in the
thread), which, in my opinion is one of the selling points of Shiro since
it greatly simplifies the usage of it. That's why GeodeSecurityUtil follows
this path. If no security is configured, calls into any of the method in
GeodeSecurityUtil would be a no-op. And each VM would need only one
instance of GeodeSecurityUtil so static makes sense here IMHO.
For testing purposes, if you are not testing security in your test, the
calls into GeodeSecurityUtil/Shiro would act like non-existent. If you need
special behavior of GeodeSecurityUtil, just init it with a property object
that would give you the behavior you want in your test like this, but then
we've had tests that already covers the security aspect of all the calling
code, so your test would not need to.
@Test
public void testInitialSecurityFlags() {
// initial state of GeodeSecurityUtil
assertFalse(GeodeSecurityUtil.isClientSecurityRequired());
assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
assertFalse(GeodeSecurityUtil.isPeerSecurityRequired());
}
@Test
public void testInitWithSecurityManager() {
properties.setProperty(SECURITY_MANAGER,
"org.apache.geode.security.templates.SampleSecurityManager");
GeodeSecurityUtil.initSecurity(properties);
assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
assertTrue(GeodeSecurityUtil.isIntegratedSecurity());
assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
}
@Test
public void testInitWithClientAuthenticator()
{
properties.setProperty(SECURITY_CLIENT_AUTHENTICATOR, "org.abc.test");
GeodeSecurityUtil.initSecurity(properties);
assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
assertFalse(GeodeSecurityUtil.isPeerSecurityRequired());
}
@Test
public void testInitWithPeerAuthenticator()
{
properties.setProperty(SECURITY_PEER_AUTHENTICATOR, "org.abc.test");
GeodeSecurityUtil.initSecurity(properties);
assertFalse(GeodeSecurityUtil.isClientSecurityRequired());
assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
}
@Test
public void testInitWithShiroAuthenticator()
{
properties.setProperty(SECURITY_SHIRO_INIT, "shiro.ini");
GeodeSecurityUtil.initSecurity(properties);
assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
assertTrue(GeodeSecurityUtil.isIntegratedSecurity());
assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
}
On Wed, Jul 20, 2016 at 10:16 AM, Kirk Lund <kl...@pivotal.io> wrote:
> To avoid adding boilerplate code, we could just avoid writing any new
> static methods or classes of static methods. Remember static is bad in OO
> and is one of the main things that prevents true, good unit testing because
> you can't replace the class or the method with a fake of any sort (mock,
> spy, or dummy).
>
> I would vote to eliminate all statics and adopt a policy of no new statics
> and follow:
>
> C.1) change all methods in GeodeUtils to be non-static and pass in an
> instance of GeodeUtils to the class constructor of any class that will use
> it
>
> -Kirk
>
>
> On Wed, Jul 20, 2016 at 9:57 AM, Jinmei Liao <jil...@pivotal.io> wrote:
>
> > One other to bear in mind is to avoid adding too much boiler code into
> the
> > production solely for the purpose of testing. I am typically a OO person,
> > but I wouldn't sacrifice code clarity and readability for it.
> >
> > On Fri, Jul 15, 2016 at 12:14 PM, Kirk Lund <kl...@pivotal.io> wrote:
> >
> > > We recently introduced GeodeSecurityUtil for GEODE-17, and I'm looking
> > into
> > > ways to refactor this class or the classes using it to better
> facilitate
> > > unit testing of those classes. I'm hoping that others on this list will
> > > chime in with ideas and opinions. GeodeSecurityUtil itself is a
> > collection
> > > of public static methods that use Apache Shiro and callback APIs (such
> as
> > > SecurityManager) in Geode.
> > >
> > > First, a few general pre-assertions:
> > > A.1) we want to embrace clean coding and solid OOP coding practices,
> > > A.2) we want to write fast running unit tests that fully isolate the
> > class
> > > under test (this typically uses mocking),
> > > A.3) we want to be able to unit test any code in a class even if it's
> in
> > a
> > > method that invokes public static methods in another class,
> > > A.4) simple mocking with Mockito is much preferred over using
> Powermock,
> > > A.5) we want to also write integration tests but the emphasis is on
> fast
> > > unit tests.
> > >
> > > There are quite a lot of procedurally static classes already in the
> Geode
> > > codebase, so I'm hoping some of these ideas will be applicable to
> helping
> > > out others working on other areas of the code as well.
> > >
> > > Any class that has calls to a collaborator with public static methods
> > makes
> > > the unit testing of that class much more difficult:
> > >
> > > B.1) you cannot spy the static util class to verify that the class
> under
> > > test invoked it with proper parameters
> > >
> > > B.2) you cannot mock the static util class to return expected values to
> > the
> > > class under test
> > >
> > > If the following is a util type class:
> > >
> > > public class GeodeUtils {
> > >
> > > public static void persist(String key) {
> > > ...
> > > }
> > >
> > > public static String getValue() {
> > > ...
> > > }
> > > }
> > >
> > > ...then there are two potential ways to facilitate unit testing of
> > classes
> > > that depend on GeodeUtils:
> > >
> > > C.1) change all methods in GeodeUtils to be non-static and pass in an
> > > instance of GeodeUtils to the class constructor of any class that will
> > use
> > > it
> > >
> > > C.2) wrap these statics with an interface and default implementation
> that
> > > delegates to the static calls:
> > >
> > > public interface GeodeService {
> > >
> > > void persist(String key);
> > >
> > > String getValue();
> > > }
> > >
> > > public class DefaultGeodeService implements GeodeService {
> > >
> > > public void persist(String key) {
> > > GeodeUtils.persist(key);
> > > }
> > >
> > > public String getValue() {
> > > return GeodeUtils.getValue();
> > > }
> > > }
> > >
> > > public class DurableOperation {
> > >
> > > private final Properties config;
> > > private final GeodeService geodeService;
> > >
> > > public DurableOperation(Properties config) {
> > > this(config , new DefaultGeodeService());
> > > }
> > >
> > > DurableOperation(Properties config, GeodeService geodeService) {
> > > this.config = config;
> > > this.geodeService = geodeService;
> > > }
> > >
> > > public void perform(String key) throws IOException {
> > > ...
> > > this.geodeService.persist(key);
> > > ...
> > > }
> > > }
> > >
> > > public class DurableOperationTest {
> > >
> > > @Test
> > > public void shouldPersistKey() throws Exception {
> > > // given
> > > Properties config = generateFancyOperationConfig();
> > > GeodeService spyGeodeService = spy(GeodeService.class);
> > > when(spyGeodeService.getValue()).thenReturn("bar");
> > > DurableOperation durableOperation = new DurableOperation(config,
> > > spyGeodeService);
> > >
> > > // when
> > > durableOperation.perform("foo");
> > >
> > > // then
> > > verify(spyGeodeService, times(1)).persist("foo");
> > > }
> > > }
> > >
> > > C.3) do C.2 until C.1 can occur (what prevents us from just doing C.1?)
> > >
> > > I believe that in general we want to use the solution in C.1 where
> > > possible.
> > >
> > > To foster some discussion:
> > >
> > > D.1) do we agree on the pre-assertions I listed in A.1-5?
> > > D.2) do we have procedurally static classes that cannot or should not
> be
> > > changed from static to non-static as described by C.1?
> > > D.3) are there any other approaches that will better facilitate unit
> > > testing of classes that depend on static collaborators?
> > >
> > > I'd like to ultimately encourage the Geode dev community to avoid
> > creating
> > > new classes that consist of static methods and state. I believe we can
> do
> > > this and that it's for the best.
> > >
> > > -Kirk
> > >
> >
> >
> >
> > --
> > Cheers
> >
> > Jinmei
> >
>
--
Cheers
Jinmei
