I don't believe there's a problem with Shiro using ThreadLocals.
My original intention of this discussion thread was to promote Unit Testing
and use of Mockito for any feature that's more complex than
StringUtils.isEmpty(String value). Just because the security class is
currently called GeodeSecurityUtil does not mean it's similar to
StringUtils. Security is a complex feature and we care about how are
classes interact with it such that it deserves to be mocked for Unit
Testing.
-Kirk
On Mon, Jul 25, 2016 at 6:09 PM, Jacob Barrett <jbarr...@pivotal.io> wrote:
> Security is one of those exceptions to the rules about ThreadLocal. Almost
> every implementation uses ThreadLocal to stash the current executing
> context. Generally then there is a static class that gets the current
> context. To test you should be able to just push your own mocked context
> into the context. Not familiar with Shiro but I have done this with a few
> other JAAS implementations.
>
> -Jake
>
>
> On Mon, Jul 25, 2016 at 5:33 PM Jinmei Liao <jil...@pivotal.io> wrote:
>
> > Shiro relies heavily on ThreadLocal. GeodeSecurityUtils is a wrapper
> around
> > Shiro. That's where all the static usage stems from. There is a trade-off
> > if we pass some sort session object around: calling code would need to
> keep
> > a reference of it and knows when/where/which to pass it down.
> >
> > On Mon, Jul 25, 2016 at 5:21 PM, Dan Smith <dsm...@pivotal.io> wrote:
> >
> > > IMHO static helper methods like those in Bytes are fine. We would never
> > > want to substitute a mock implementation of one of those methods and
> they
> > > don't have any collaborators.
> > >
> > > Just glancing at GeodeSecurityUtils, what I don't like is that it's
> > relying
> > > heavily on mutable static singletons (securityManager, postProcessor)
> and
> > > thread locals. I think we should avoid using singletons and thread
> locals
> > > in geode code, because it's too easy to leave junk lying around and it
> > also
> > > makes code hard to test. In this case maybe it would make more sense to
> > > pass some sort of session object around?
> > >
> > > -Dan
> > >
> > > On Mon, Jul 25, 2016 at 4:26 PM, Kirk Lund <kl...@pivotal.io> wrote:
> > >
> > > > I was trying to describe any Java class in Geode which provides
> feature
> > > > functionality that should be unit tested and integration tested, but
> is
> > > > either currently implemented as a static class or which collaborates
> > > with a
> > > > non-trivial component currently implemented as a static class.
> > > >
> > > > I wasn't describing the simple static classes like Byte, StringUtils,
> > > > IOUtils. These are trivial and not generally considered to be an
> > > important
> > > > collaborator requiring a mock.
> > > >
> > > > Using Mockito allows us to test the interaction between a class under
> > > test
> > > > and its collaborators. To facilitate this, we need the collaborator
> to
> > be
> > > > defined as an interface or abstract class.
> > > >
> > > > This specifically came about on my team because we have been unit
> > testing
> > > > GMSAuthenticator and lots of Client Commands that interact with
> > > Integrated
> > > > Security. We want to include unit tests to verify that the class
> under
> > > test
> > > > correctly interacts with its collaborators including Integrated
> > Security
> > > > (GEODE-17). For this reason, we want to transform GeodeSecurityUtil
> > into
> > > a
> > > > non-static class that implements an interface that can be mocked.
> > > >
> > > > Hopefully everyone agrees that we need to make a concerted effort to
> > > write
> > > > lots of new fast-running unit tests that use mocks. This includes old
> > and
> > > > new code.
> > > >
> > > > In general, actual features implemented in static classes is common
> in
> > > > Geode but it inhibits unit testing and thus should change as we
> > refactor
> > > > the code towards testability.
> > > >
> > > > This thread started as me describing a simple pattern to facilitate
> > unit
> > > > testing against static classes that for whatever reason, we want to
> > hold
> > > > off refactoring them but we don't want to hold off writing unit tests
> > for
> > > > them as an important collaborator class.
> > > >
> > > > Does this make sense? Is everyone on the same page regarding the
> value
> > of
> > > > unit testing and refactoring to testability & maintainability?
> > > >
> > > > -Kirk
> > > >
> > > > On Wednesday, July 20, 2016, Anthony Baker <aba...@pivotal.io>
> wrote:
> > > >
> > > > > 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
> > > > > <javascript:;>> 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
> > > > > <javascript:;>> 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
> > > > > <javascript:;>> 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
> > > > > <javascript:;>> 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
> > > > >
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> > Cheers
> >
> > Jinmei
> >
>
