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
