RM-2027 Make class logger static final.

Previously the logger wasn't final to allow it to be set for unit testing.
We're only testing it in the one test, so instead use a mock log4j appender
to check that the message is received.

+review RM-11

git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/modules/recordsmanagement/HEAD@100717 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

rm-server/source/java/org/alfresco/module/org_alfresco_module_rm/classification/ClassificationServiceImpl.java

@@ -42,7 +42,7 @@ public class ClassificationServiceImpl extends ServiceBaseImpl
     private static final String[] REASONS_KEY = new String[] { "org.alfresco",
                                                                "module.org_alfresco_module_rm",
                                                                "classification.reasons" };
-    private static Logger logger = LoggerFactory.getLogger(ClassificationServiceImpl.class); 
+    private static final Logger LOGGER = LoggerFactory.getLogger(ClassificationServiceImpl.class); 
 
     static final String DEFAULT_CONFIG_DIRECTORY = "/alfresco/module/org_alfresco_module_rm/classification/";
     static final String DEFAULT_LEVELS_FILE = DEFAULT_CONFIG_DIRECTORY + "rm-classification-levels.json";
@@ -66,12 +66,10 @@ public class ClassificationServiceImpl extends ServiceBaseImpl
      * Package protected constructor, primarily for unit testing purposes.
      * 
      * @param classificationServiceDao The object from which configuration options will be read.
-     * @param logger The class logger (note - this will be set statically).
      */
-    ClassificationServiceImpl(ClassificationServiceDAO classificationServiceDao, Logger logger)
+    ClassificationServiceImpl(ClassificationServiceDAO classificationServiceDao)
     {
         this.classificationServiceDao = classificationServiceDao;
-        ClassificationServiceImpl.logger = logger;
     }
 
     public void setAttributeService(AttributeService service) { this.attributeService = service; }
@@ -82,8 +80,8 @@ public class ClassificationServiceImpl extends ServiceBaseImpl
         final List<ClassificationLevel> configurationLevels = getConfigurationLevels();
 
         // Note! We cannot log the level names or even the size of these lists for security reasons.
-        logger.debug("Persisted classification levels: {}", loggableStatusOf(allPersistedLevels));
+        LOGGER.debug("Persisted classification levels: {}", loggableStatusOf(allPersistedLevels));
-        logger.debug("Classpath classification levels: {}", loggableStatusOf(configurationLevels));
+        LOGGER.debug("Classpath classification levels: {}", loggableStatusOf(configurationLevels));
 
         if (configurationLevels == null || configurationLevels.isEmpty())
         {
@@ -106,8 +104,8 @@ public class ClassificationServiceImpl extends ServiceBaseImpl
         final List<ClassificationReason> classpathReasons = getConfigurationReasons();
 
         // Note! We cannot log the reasons or even the size of these lists for security reasons.
-        logger.debug("Persisted classification reasons: {}", loggableStatusOf(persistedReasons));
+        LOGGER.debug("Persisted classification reasons: {}", loggableStatusOf(persistedReasons));
-        logger.debug("Classpath classification reasons: {}", loggableStatusOf(classpathReasons));
+        LOGGER.debug("Classpath classification reasons: {}", loggableStatusOf(classpathReasons));
 
         if (isEmpty(persistedReasons))
         {
@@ -122,7 +120,7 @@ public class ClassificationServiceImpl extends ServiceBaseImpl
         {
             if (isEmpty(classpathReasons) || !classpathReasons.equals(persistedReasons))
             {
-                logger.warn("Classification reasons configured in classpath do not match those stored in Alfresco."
+                LOGGER.warn("Classification reasons configured in classpath do not match those stored in Alfresco. "
                             + "Alfresco will use the unchanged values stored in the database.");
                 // RM-2073 says that we should log a warning and proceed normally.
             }

rm-server/unit-test/java/org/alfresco/module/org_alfresco_module_rm/classification/ClassificationServiceImplUnitTest.java

@@ -19,6 +19,7 @@
 package org.alfresco.module.org_alfresco_module_rm.classification;
 
 import static java.util.Arrays.asList;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.eq;
@@ -31,14 +32,18 @@ import static org.mockito.Mockito.when;
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.stream.Stream;
 
 import org.alfresco.module.org_alfresco_module_rm.classification.ClassificationServiceException.MissingConfiguration;
 import org.alfresco.module.org_alfresco_module_rm.test.util.MockAuthenticationUtilHelper;
 import org.alfresco.module.org_alfresco_module_rm.util.AuthenticationUtil;
 import org.alfresco.service.cmr.attributes.AttributeService;
+import org.apache.log4j.Appender;
+import org.apache.log4j.Level;
+import org.apache.log4j.spi.LoggingEvent;
 import org.junit.Before;
 import org.junit.Test;
-import org.slf4j.Logger;
+import org.mockito.ArgumentCaptor;
 
 /**
  * Unit tests for {@link ClassificationServiceImpl}.
@@ -85,22 +90,22 @@ public class ClassificationServiceImplUnitTest
         return levels;
     }
 
-    private ClassificationServiceImpl classificationService;
+    private ClassificationServiceImpl classificationServiceImpl;
 
     private AttributeService         mockedAttributeService       = mock(AttributeService.class);
     private AuthenticationUtil       mockedAuthenticationUtil;
     private ClassificationServiceDAO mockClassificationServiceDAO = mock(ClassificationServiceDAO.class);
-    /** Using a mock logger in the class so that we can verify some of the logging requirements. */
+    /** Using a mock appender in the class logger so that we can verify some of the logging requirements. */
-    private Logger                   mockLogger                   = mock(Logger.class);
+    private Appender                 mockAppender                 = mock(Appender.class);
 
     @Before public void setUp()
     {
-        reset(mockClassificationServiceDAO, mockedAttributeService, mockLogger);
+        reset(mockClassificationServiceDAO, mockedAttributeService, mockAppender);
         mockedAuthenticationUtil = MockAuthenticationUtilHelper.create();
 
-        classificationService = new ClassificationServiceImpl(mockClassificationServiceDAO, mockLogger);
+        classificationServiceImpl = new ClassificationServiceImpl(mockClassificationServiceDAO);
-        classificationService.setAttributeService(mockedAttributeService);
+        classificationServiceImpl.setAttributeService(mockedAttributeService);
-        classificationService.setAuthenticationUtil(mockedAuthenticationUtil);
+        classificationServiceImpl.setAuthenticationUtil(mockedAuthenticationUtil);
     }
 
     @Test public void defaultLevelsConfigurationVanillaSystem()
@@ -108,7 +113,7 @@ public class ClassificationServiceImplUnitTest
         when(mockClassificationServiceDAO.getConfiguredLevels()).thenReturn(DEFAULT_CLASSIFICATION_LEVELS);
         when(mockedAttributeService.getAttribute(anyString(), anyString(), anyString())).thenReturn(null);
 
-        classificationService.initConfiguredClassificationLevels();
+        classificationServiceImpl.initConfiguredClassificationLevels();
 
         verify(mockedAttributeService).setAttribute(eq((Serializable) DEFAULT_CLASSIFICATION_LEVELS),
                 anyString(), anyString(), anyString());
@@ -120,7 +125,7 @@ public class ClassificationServiceImplUnitTest
         when(mockedAttributeService.getAttribute(anyString(), anyString(), anyString()))
                                    .thenReturn((Serializable) DEFAULT_CLASSIFICATION_LEVELS);
 
-        classificationService.initConfiguredClassificationLevels();
+        classificationServiceImpl.initConfiguredClassificationLevels();
 
         verify(mockedAttributeService).setAttribute(eq((Serializable) ALT_CLASSIFICATION_LEVELS),
                 anyString(), anyString(), anyString());
@@ -131,7 +136,7 @@ public class ClassificationServiceImplUnitTest
     {
         when(mockedAttributeService.getAttribute(anyString(), anyString(), anyString())).thenReturn(null);
 
-        classificationService.initConfiguredClassificationLevels();
+        classificationServiceImpl.initConfiguredClassificationLevels();
     }
 
     @Test public void pristineSystemShouldBootstrapReasonsConfiguration()
@@ -142,7 +147,7 @@ public class ClassificationServiceImplUnitTest
         // We'll use a small set of placeholder classification reasons.
         when(mockClassificationServiceDAO.getConfiguredReasons()).thenReturn(PLACEHOLDER_CLASSIFICATION_REASONS);
 
-        classificationService.initConfiguredClassificationReasons();
+        classificationServiceImpl.initConfiguredClassificationReasons();
 
         verify(mockedAttributeService).setAttribute(eq((Serializable)PLACEHOLDER_CLASSIFICATION_REASONS),
                 anyString(), anyString(), anyString());
@@ -154,7 +159,7 @@ public class ClassificationServiceImplUnitTest
         when(mockedAttributeService.getAttribute(anyString(), anyString(), anyString())).thenReturn((Serializable)PLACEHOLDER_CLASSIFICATION_REASONS);
         when(mockClassificationServiceDAO.getConfiguredReasons()).thenReturn(PLACEHOLDER_CLASSIFICATION_REASONS);
 
-        classificationService.initConfiguredClassificationReasons();
+        classificationServiceImpl.initConfiguredClassificationReasons();
 
         verify(mockedAttributeService, never()).setAttribute(any(Serializable.class),
                 anyString(), anyString(), anyString());
@@ -163,6 +168,10 @@ public class ClassificationServiceImplUnitTest
     /**
      * Check that if the reasons supplied on the classpath differ from those already persisted then a warning is logged
      * and no change is made to the persisted reasons.
+     * <p>
+     * This test uses the underlying log4j implementation to insert a mock Appender, and tests this for the warning
+     * message. If the underlying logging framework is changed then this unit test will fail, and it may not be
+     * possible to/worth fixing.
      */
     @Test public void previouslyStartedSystemShouldWarnIfConfiguredReasonsHaveChanged()
     {
@@ -171,12 +180,29 @@ public class ClassificationServiceImplUnitTest
                     (Serializable) PLACEHOLDER_CLASSIFICATION_REASONS);
         when(mockClassificationServiceDAO.getConfiguredReasons()).thenReturn(ALTERNATIVE_CLASSIFICATION_REASONS);
 
-        classificationService.initConfiguredClassificationReasons();
+        // Put the mock Appender into the log4j logger and allow warning messages to be received.
+        org.apache.log4j.Logger log4jLogger = org.apache.log4j.Logger.getLogger(ClassificationServiceImpl.class);
+        log4jLogger.addAppender(mockAppender);
+        Level normalLevel = log4jLogger.getLevel();
+        log4jLogger.setLevel(Level.WARN);
 
-        verify(mockLogger).warn("Classification reasons configured in classpath do not match those stored in Alfresco."
+        // Call the method under test.
-                    + "Alfresco will use the unchanged values stored in the database.");
+        classificationServiceImpl.initConfiguredClassificationReasons();
+
+        // Reset the logging level for other tests.
+        log4jLogger.setLevel(normalLevel);
+
+        // Check the persisted values weren't changed.
         verify(mockedAttributeService, never()).setAttribute(any(Serializable.class),
                 anyString(), anyString(), anyString());
+
+        // Check that the warning message was logged.
+        ArgumentCaptor<LoggingEvent> argumentCaptor = ArgumentCaptor.forClass(LoggingEvent.class);
+        verify(mockAppender).doAppend(argumentCaptor.capture());
+        List<LoggingEvent> loggingEvents = argumentCaptor.getAllValues();
+        Stream<String> messages = loggingEvents.stream().map(event -> event.getRenderedMessage());
+        String expectedMessage = "Classification reasons configured in classpath do not match those stored in Alfresco. Alfresco will use the unchanged values stored in the database.";
+        assertTrue("Warning message not found in log.", messages.anyMatch(message -> message == expectedMessage));
     }
     
     @Test(expected = MissingConfiguration.class)
@@ -186,6 +212,6 @@ public class ClassificationServiceImplUnitTest
                     .thenReturn((Serializable) null);
         when(mockClassificationServiceDAO.getConfiguredReasons()).thenReturn(null);
 
-        classificationService.initConfiguredClassificationReasons();
+        classificationServiceImpl.initConfiguredClassificationReasons();
     }
 }