Great discussion! I’ve got two questions: 1) What do you think of Bytes.java (:geode-core:com.gemstone.gemfire.internal.util)? Is it a reasonable use of the Helper / Utility class pattern? If not, how would you change it? Creating a new instance for each use seems inefficient and using a Singleton is just a different way to do statics.
2) Is GeodeSecurityUtil of the same category as Bytes.java or is it something
more?
Anthony
> On Jul 20, 2016, at 10:41 AM, Jinmei Liao <jil...@pivotal.io> wrote:
>
> 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
signature.asc
Description: Message signed with OpenPGP using GPGMail
