get rid of reload() in AuthorizationFramework constructor

fixes #1925

Author: Vladimir Kotal

plugins/LdapPlugin/test/opengrok/auth/plugin/LdapAttrPluginTest.java

@@ -80,7 +80,8 @@ public void setUp() {
 
         plugin.load(parameters);
 
-        framework = new AuthorizationFramework(null);
+        framework = new AuthorizationFramework();
+        framework.reload();
     }
 
     private void prepareRequest(String username, String mail, String... ous) {

src/org/opensolaris/opengrok/authorization/AuthorizationFramework.java

@@ -74,6 +74,11 @@ public final class AuthorizationFramework {
      * Stack of available plugins/stacks in the order of the execution.
      */
     AuthorizationStack stack;
+    /**
+     * New stack. This is set by {@code setStack()} and used for delayed
+     * reconfiguration in {@code reload()}.
+     */
+    AuthorizationStack newStack;
 
     /**
      * Lock for safe reloads.
@@ -89,6 +94,20 @@ public final class AuthorizationFramework {
      */
     private long pluginVersion = 0;
 
+    /**
+     * Whether to load plugins from class files and jar files.
+     */
+    private boolean loadClasses = true;
+    private boolean loadJars = true;
+    
+    /**
+     * Create a new instance of authorization framework with no plugin
+     * directory and the default plugin stack.
+     */
+    public AuthorizationFramework() {
+        this(null);
+    }
+    
     /**
      * Create a new instance of authorization framework with the plugin
      * directory and the default plugin stack.
@@ -109,7 +128,6 @@ public AuthorizationFramework(String path) {
     public AuthorizationFramework(String path, AuthorizationStack stack) {
         this.stack = stack;
         setPluginDirectory(path);
-        reload();
     }
 
     /**
@@ -138,6 +156,38 @@ public void setPluginDirectory(String directory) {
         setPluginDirectory(directory != null ? new File(directory) : null);
     }
 
+    /**
+     * Make {@code reload()} search for plugins in class files.
+     * @param flag true or false
+     */
+    public void setLoadClasses(boolean flag) {
+        loadClasses = flag;
+    }
+    
+    /**
+     * Whether to search for plugins in class files.
+     * @return true if enabled, false otherwise
+     */
+    public boolean isLoadClassesEnabled() {
+        return loadClasses;
+    }
+    
+    /**
+     * Make {@code reload()} search for plugins in jar files.
+     * @param flag true or false
+     */
+    public void setLoadJars(boolean flag) {
+        loadJars = flag;
+    }
+    
+    /**
+     * Whether to search for plugins in class files.
+     * @return true if enabled, false otherwise
+     */
+    public boolean isLoadJarsEnabled() {
+        return loadJars;
+    }
+
     /**
      * Checks if the request should have an access to project. See
      * {@link #checkAll} for more information about invocation order.
@@ -248,17 +298,13 @@ public AuthorizationStack getStack() {
     }
 
     /**
-     * Set the internal stack to this new value.
+     * Set new value of the authorization stack. This will come into effect
+     * only after {@code reload()} is called.
      *
      * @param s new stack to be used
      */
     public void setStack(AuthorizationStack s) {
-        lock.writeLock().lock();
-        try {
-            this.stack = s;
-        } finally {
-            lock.writeLock().unlock();
-        }
+        this.newStack = s;
     }
 
     /**
@@ -438,20 +484,19 @@ protected List<Class> getInterfaces(Class c) {
     }
 
     /**
-     * Traverse list of files which possibly contain a java class and then
-     * traverse a list of jar files to load all classes which are contained
-     * within them into the given stack. Each class is loaded with
-     * {@link #handleLoadClass(String)} which delegates the loading to the
-     * custom class loader {@link #loadClass(String)}.
+     * Traverse list of files which possibly contain a java class 
+     * to load all classes which are contained within them into the given stack.
+     * Each class is loaded with {@link #handleLoadClass(String)} which
+     * delegates the loading to the custom class loader
+     * {@link #loadClass(String)}.
      *
      * @param stack the stack where to add the loaded classes
      * @param classfiles list of files which possibly contain a java class
-     * @param jarfiles list of jar files containing java classes
      *
      * @see #handleLoadClass(String)
      * @see #loadClass(String)
      */
-    private void loadClasses(AuthorizationStack stack, List<File> classfiles, List<File> jarfiles) {
+    private void loadClassFiles(AuthorizationStack stack, List<File> classfiles) {
         IAuthorizationPlugin pf;
 
         for (File file : classfiles) {
@@ -467,6 +512,23 @@ private void loadClasses(AuthorizationStack stack, List<File> classfiles, List<F
                 }
             }
         }
+    }
+
+    /**
+     * Traverse list of jar files to load all classes which are contained within
+     * them into the given stack.
+     * Each class is loaded with {@link #handleLoadClass(String)} which
+     * delegates the loading to the custom class loader
+     * {@link #loadClass(String)}.
+     * 
+     * @param stack the stack where to add the loaded classes
+     * @param jarfiles list of jar files containing java classes
+     * 
+     * @see #handleLoadClass(String)
+     * @see #loadClass(String)
+     */
+    private void loadJarFiles(AuthorizationStack stack, List<File> jarfiles) {
+        IAuthorizationPlugin pf;
 
         for (File file : jarfiles) {
             try (JarFile jar = new JarFile(file)) {
@@ -544,16 +606,24 @@ public Object run() {
             }
         });
 
-        // clone a new stack not interfering with the current stack
-        AuthorizationStack newStack = getStack().clone();
-
-        // load all other possible plugin classes
-        loadClasses(newStack,
-                IOUtils.listFilesRec(pluginDirectory, ".class"),
-                IOUtils.listFiles(pluginDirectory, ".jar"));
+        AuthorizationStack newLocalStack;
+        if (this.newStack == null) {
+            // Clone a new stack not interfering with the current stack.
+            newLocalStack = getStack().clone();
+        } else {
+            newLocalStack = this.newStack.clone();
+        }
+        
+        // Load all other possible plugin classes.
+        if (isLoadClassesEnabled()) {
+            loadClassFiles(newLocalStack, IOUtils.listFilesRec(pluginDirectory, ".class"));
+        }
+        if (isLoadJarsEnabled()) {
+            loadJarFiles(newLocalStack, IOUtils.listFiles(pluginDirectory, ".jar"));
+        }
 
         // fire load events
-        loadAllPlugins(newStack);
+        loadAllPlugins(newLocalStack);
 
         AuthorizationStack oldStack;
         /**
@@ -566,7 +636,7 @@ public Object run() {
         lock.writeLock().lock();
         try {
             oldStack = stack;
-            stack = newStack;
+            stack = newLocalStack;
 
             // increase the current plugin version tracked by the framework
             increasePluginVersion();

src/org/opensolaris/opengrok/web/WebappListener.java

@@ -81,6 +81,7 @@ public void contextInitialized(final ServletContextEvent servletContextEvent) {
          * possibly {@code null} causing the framework to allow every request.
          */
         env.setAuthorizationFramework(new AuthorizationFramework(env.getPluginDirectory(), env.getPluginStack()));
+        env.getAuthorizationFramework().reload();
 
         String address = context.getInitParameter("ConfigAddress");
         if (address != null && address.length() > 0) {

test/org/opensolaris/opengrok/authorization/AuthorizationFrameworkReloadTest.java

@@ -39,11 +39,6 @@
 /**
  * Test behavior of AuthorizationFramework {@code reload()} w.r.t. HTTP sessions.
  * 
- * Note that the tests are accompanied with noise when reloading since
- * the plugin directory contains classes of other tests from this package
- * which the authorization framework is trying (unsuccessfully, since they
- * do not implement the plugin interface) to load.
- * 
  * @author Vladimir Kotal
  */
 public class AuthorizationFrameworkReloadTest {
@@ -66,6 +61,8 @@ public AuthorizationFrameworkReloadTest() {
     public void testReloadSimple() {
         DummyHttpServletRequest req = new DummyHttpServletRequest();
         AuthorizationFramework framework = new AuthorizationFramework(pluginDirectory.getPath());
+        framework.setLoadClasses(false); // to avoid noise when loading classes of other tests
+        framework.reload();
         Statistics stats = RuntimeEnvironment.getInstance().getStatistics();
 
         // Ensure the framework was setup correctly.
@@ -109,6 +106,8 @@ public void testReloadCycle() {
         stack.setForProjects(projectName);
         AuthorizationFramework framework = 
                 new AuthorizationFramework(pluginDirectory.getPath(), stack);
+        framework.setLoadClasses(false); // to avoid noise when loading classes of other tests
+        framework.reload();
 
         // Perform simple sanity check before long run is entered. If this fails,
         // it will be waste of time to continue with the test.
@@ -158,4 +157,20 @@ public void run() {
         System.out.println("number of reloads: " + reloads);
         assertTrue(reloads > 0);
     }
+    
+    @Test
+    public void testSetLoadClasses() {
+        AuthorizationFramework framework = new AuthorizationFramework();
+        assertTrue(framework.isLoadClassesEnabled());
+        framework.setLoadClasses(false);
+        assertFalse(framework.isLoadClassesEnabled());
+    }
+    
+    @Test
+    public void testSetLoadJars() {
+        AuthorizationFramework framework = new AuthorizationFramework();
+        assertTrue(framework.isLoadJarsEnabled());
+        framework.setLoadJars(false);
+        assertFalse(framework.isLoadJarsEnabled());
+    }
 }

test/org/opensolaris/opengrok/authorization/AuthorizationFrameworkTest.java

@@ -30,7 +30,6 @@
 import java.util.stream.Collectors;
 import javax.servlet.http.HttpServletRequest;
 import org.junit.Assert;
-import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -45,7 +44,6 @@ public class AuthorizationFrameworkTest {
 
     private static final Random RANDOM = new Random();
 
-    private AuthorizationFramework framework;
     private final StackSetup setup;
 
     public AuthorizationFrameworkTest(StackSetup setup) {
@@ -651,7 +649,7 @@ public static StackSetup[][] params() {
 
     @Test
     public void testPluginsGeneric() {
-        framework.setStack(setup.stack);
+        AuthorizationFramework framework = new AuthorizationFramework(null, setup.stack);
         framework.loadAllPlugins(setup.stack);
 
         boolean actual;
@@ -672,11 +670,6 @@ public void testPluginsGeneric() {
         }
     }
 
-    @Before
-    public void setUp() {
-        framework = new AuthorizationFramework(null);
-    }
-
     static private Project createAllowedProject() {
         Project p = new Project("allowed" + "_" + "project" + Math.random());
         return p;
@@ -782,7 +775,6 @@ public String toString() {
             }
 
         };
-
     }
 
     static public class TestCase {

test/org/opensolaris/opengrok/index/IndexerRepoTest.java

@@ -18,19 +18,16 @@
  */
 
 /*
- * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2017, Oracle and/or its affiliates. All rights reserved.
  */
 package org.opensolaris.opengrok.index;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
 
 import org.junit.After;
-import org.junit.AfterClass;
 import org.junit.Before;
-import org.junit.BeforeClass;
 import org.junit.Test;
 import org.opensolaris.opengrok.configuration.RuntimeEnvironment;
 import org.opensolaris.opengrok.history.HistoryGuru;

test/org/opensolaris/opengrok/web/PageConfigTest.java

@@ -17,7 +17,7 @@
  * CDDL HEADER END
  */
 
- /*
+/*
  * Copyright (c) 2011, 2017, Oracle and/or its affiliates. All rights reserved.
  */
 package org.opensolaris.opengrok.web;
@@ -187,7 +187,8 @@ public void testGetResourceFileList() {
          *  - disabling "mercurial"
          * </pre>
          */
-        env.setAuthorizationFramework(new AuthorizationFramework(null));
+        env.setAuthorizationFramework(new AuthorizationFramework());
+        env.getAuthorizationFramework().reload();
         env.getAuthorizationFramework().getStack()
                 .add(new AuthorizationPlugin(AuthControlFlag.REQUIRED, new TestPlugin() {
                     @Override

test/org/opensolaris/opengrok/web/ProjectHelperTestBase.java

@@ -266,7 +266,8 @@ public void setUp() {
         Assert.assertNotNull("Repository map should not be null", env.getProjectRepositoriesMap());
         Assert.assertEquals("Repository map should contain 20 project", 20, env.getProjectRepositoriesMap().size());
 
-        env.setAuthorizationFramework(new AuthorizationFramework(null));
+        env.setAuthorizationFramework(new AuthorizationFramework());
+        env.getAuthorizationFramework().reload();
 
         IAuthorizationPlugin plugin = new TestPlugin() {
             @Override