I'm specifically advocating wrapping the class with an interface to
facilitate mocking for unit testing classes that depend on it. This way we
can use Mockito ArgumentCaptor to assert that the correct permission
strings are being passed to our security component. We can also provide
fake returns from security for unit testing.
For example, mocking allows us to write Unit Tests that isolate and verify
the behavior of a Client Command object when the user is not authorized as
well as verifying behavior when the user is authorized. The alternative is
an Integration Test. Even if an Integration Test is fast, it's still an
Integration Test and not a Unit Test. If the Geode security component uses
Shiro in a test then that's an Integration Test. Yes, we do want to have
some Integration Tests like this, but most tests should be Unit Tests and
they should not involve Shiro.
With Geode, we have the perfect example of what happens when most of the
tests are Integration Tests. The result is a build that takes 11 hours. We
want to move from Integration Tests to Unit Tests at least for the primary
build and feedback cycle for development/refactoring/commit. The
Integration Tests then become nightly tests.
Usage of static singletons is a different issue. In my opinion, Geode
should only have one Singleton (if any) and that should be the Cache -- the
Cache can own other "singleton" components like security. There are plenty
of resources that describe how to get away from using Singletons and why
they are problematic.
-Kirk
On Mon, Jul 25, 2016 at 5:32 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
>
