NIHVIVO-123 Rewrite ConfigurationProperties to use lazy initialization instead of a static initializer -- the exceptions are much more descriptive that way. Also write messages to the log on a variety of error conditions. Add unit tests.

webapp/src/edu/cornell/mannlib/vitro/webapp/ConfigurationProperties.java

@@ -43,82 +43,30 @@ public class ConfigurationProperties {
 	private static final Logger LOG = Logger
 			.getLogger(ConfigurationProperties.class);
 
+	/**
+	 * The JNDI naming context where Tomcat stores environment attributes.
+	 */
+	static final String JNDI_BASE = "java:comp/env";
+
 	/**
 	 * The name of the JNDI environment mapping for the path to the
 	 * configuration file (or resource).
 	 */
-	private static final String PATH_CONFIGURATION = "path.configuration";
+	static final String PATH_CONFIGURATION = "path.configuration";
+
+	/**
+	 * The map of the configuration properties.
+	 * 
+	 * This should not be accessed directly, but only through the synchronized
+	 * method {@link #getTheMap() (and {@link #reset()} for unit tests).
+	 */
 	private static volatile Map<String, String> theMap;
 
-	static {
-		try {
-			// Obtain our environment naming context
-			Context initCtx = new InitialContext();
-			Context envCtx = (Context) initCtx.lookup("java:comp/env");
-
-			// Get the name of the configuration properties file.
-			String configPath = (String) envCtx.lookup(PATH_CONFIGURATION);
-			if (configPath == null) {
-				throw new IllegalStateException(
-						"Could not find a JNDI Environment naming for '"
-								+ PATH_CONFIGURATION + "'.");
-			}
-
-			InputStream inStream = null;
-			// Try to find this as a file.
-			File file = new File(configPath);
-			try {
-				inStream = new FileInputStream(file);
-			} catch (FileNotFoundException e) {
-				inStream = null;
-			}
-			
-			// If no file, try to find it as a resource.
-			if (inStream == null) {
-				inStream = ConfigurationProperties.class.getClassLoader()
-						.getResourceAsStream(configPath);
-			}
-			
-			// If neither file nor resource, give up.
-			if (inStream == null) {
-				throw new IllegalArgumentException(
-						"Failed to find a configuration properties file at '"
-								+ file.getAbsolutePath()
-								+ "', or a resource at '" + configPath + "'");
-			}
-
-			// Load a properties object - it will handle the syntax of the file.
-			Properties props = new Properties();
-			try {
-				props.load(inStream);
-			} catch (IOException e) {
-				throw new IllegalStateException("Problem while reading the "
-						+ "configuration properties file at '" + configPath
-						+ "'", e);
-			}
-
-			// It's awkward to copy from Properties to a Map.
-			Map<String, String> newMap = new HashMap<String, String>();
-			for (Enumeration<?> keys = props.keys(); keys.hasMoreElements();) {
-				String key = (String) keys.nextElement();
-				newMap.put(key, props.getProperty(key));
-			}
-			
-			LOG.info("Configuration properties are: " + newMap);
-
-			// Save an unmodifiable version of the Map
-			theMap = Collections.unmodifiableMap(newMap);
-		} catch (NamingException e) {
-			throw new IllegalStateException(e);
-		}
-
-	}
-
 	/**
 	 * Get an unmodifiable copy of the map of configuration properties.
 	 */
 	public static Map<String, String> getMap() {
-		return theMap;
+		return getTheMap();
 	}
 
 	/**
@@ -126,7 +74,7 @@ public class ConfigurationProperties {
 	 * property has not been assigned a value.
 	 */
 	public static String getProperty(String key) {
-		return theMap.get(key);
+		return getTheMap().get(key);
 	}
 
 	/**
@@ -134,11 +82,144 @@ public class ConfigurationProperties {
 	 * property has not been assigned a value.
 	 */
 	public static String getProperty(String key, String defaultValue) {
-		String value = theMap.get(key);
+		String value = getTheMap().get(key);
 		if (value == null) {
 			return defaultValue;
 		} else {
 			return value;
 		}
 	}
+
+	/**
+	 * Force the map to be reloaded on the next attempt to access it.
+	 * 
+	 * This and {@link #getTheMap()} should be the only access to
+	 * {@link ConfigurationProperties#theMap}.
+	 * 
+	 * NOTE: This should only be used in Unit Tests.
+	 */
+	static synchronized void reset() {
+		theMap = null;
+	}
+
+	/**
+	 * This and {@link #reset()} should be the only access to {@link #theMap}.
+	 */
+	private static synchronized Map<String, String> getTheMap() {
+		if (theMap == null) {
+			theMap = loadTheMap();
+		}
+		return theMap;
+	}
+
+	/**
+	 * The map is null, so find the properties file and load the map.
+	 */
+	private static synchronized Map<String, String> loadTheMap() {
+		String configPath = getConfigurationFilePath();
+
+		InputStream inStream = getConfigurationInputStream(configPath);
+
+		// Load a properties object - it will parse the file easily.
+		Properties props = new Properties();
+		try {
+			props.load(inStream);
+		} catch (IOException e) {
+			throw new IllegalStateException("Problem while reading the "
+					+ "configuration properties file at '" + configPath + "'",
+					e);
+		} finally {
+			try {
+				inStream.close();
+			} catch (IOException e) {
+				LOG.error("Failed to close input stream", e);
+			}
+		}
+
+		// It's awkward to copy from Properties to a Map.
+		Map<String, String> newMap = new HashMap<String, String>();
+		for (Enumeration<?> keys = props.keys(); keys.hasMoreElements();) {
+			String key = (String) keys.nextElement();
+			newMap.put(key, props.getProperty(key));
+		}
+
+		LOG.info("Configuration properties are: " + newMap);
+
+		// Save an unmodifiable version of the Map
+		return Collections.unmodifiableMap(newMap);
+
+	}
+
+	/**
+	 * Find the path to the Configuration properties file.
+	 * 
+	 * @throws IllegalStateException
+	 *             If we can't find the path.
+	 */
+	private static String getConfigurationFilePath() {
+		String message = "";
+		try {
+			message = "JNDI Lookup on \"" + JNDI_BASE
+					+ "\" failed. Is the context file missing?";
+			Context envCtx = (Context) new InitialContext().lookup(JNDI_BASE);
+			if (envCtx == null) {
+				LOG.error(message);
+				throw new IllegalStateException(message);
+			}
+
+			// Get the name of the configuration properties file.
+			message = "Could not find a JNDI Environment naming for '"
+					+ PATH_CONFIGURATION
+					+ "'. Is the context file set up correctly?";
+			String configPath = (String) envCtx.lookup(PATH_CONFIGURATION);
+			if (configPath == null) {
+				LOG.error(message);
+				throw new IllegalStateException(message);
+			}
+
+			return configPath;
+		} catch (NamingException e) {
+			throw new IllegalStateException(message, e);
+		}
+	}
+
+	/**
+	 * Find the Configuration properties file.
+	 * 
+	 * First try to interpret the path as a file path (like
+	 * /usr/local/config.props).
+	 * 
+	 * If that doesn't work, try it as a resource path (relative to
+	 * WEB-INF/classes).
+	 * 
+	 * @throws IllegalArgumentException
+	 *             If the path fails to locate a file or a resource.
+	 */
+	private static InputStream getConfigurationInputStream(String configPath) {
+		InputStream inStream = null;
+
+		// Try to find this as a file.
+		File file = new File(configPath);
+		try {
+			inStream = new FileInputStream(file);
+		} catch (FileNotFoundException e) {
+			inStream = null;
+		}
+
+		// If no file, try to find it as a resource.
+		if (inStream == null) {
+			inStream = ConfigurationProperties.class.getClassLoader()
+					.getResourceAsStream(configPath);
+		}
+
+		// If neither file nor resource, give up.
+		if (inStream == null) {
+			throw new IllegalArgumentException(
+					"Failed to find a configuration properties file at '"
+							+ file.getAbsolutePath() + "', or a resource at '"
+							+ configPath + "'");
+		}
+		return inStream;
+	}
+
 }

webapp/test/edu/cornell/mannlib/vitro/webapp/ConfigurationPropertiesTest.java

@@ -0,0 +1,164 @@
+/* $This file is distributed under the terms of the license in /doc/license.txt$ */
+
+package edu.cornell.mannlib.vitro.webapp;
+
+import static org.junit.Assert.assertEquals;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.naming.InitialContext;
+import javax.naming.NamingException;
+
+import org.apache.log4j.Level;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import stubs.javax.naming.InitialContextStub;
+import stubs.javax.naming.spi.InitialContextFactoryStub;
+import edu.cornell.mannlib.vitro.testing.AbstractTestClass;
+
+/**
+ * 
+ * @author jeb228
+ */
+public class ConfigurationPropertiesTest extends AbstractTestClass {
+
+	/**
+	 * The JNDI Mapping for the configuration properties path.
+	 */
+	private static final String CONFIGURATION_PROPERTIES_MAPPING = ConfigurationProperties.JNDI_BASE
+			+ "/" + ConfigurationProperties.PATH_CONFIGURATION;
+
+	private static final String NOT_THE_DESIRED_MAPPING = ConfigurationProperties.JNDI_BASE
+			+ "/notTheDesiredMapping";
+
+	private static File tempDir;
+	private static File testFile;
+	private static File invalidFile;
+
+	private InitialContext initial;
+
+	/**
+	 * Create a good test file and a bad test file.
+	 * 
+	 * (a class-path-based resource should already exist.)
+	 */
+	@BeforeClass
+	public static void createTestFiles() throws IOException {
+		tempDir = createTempDirectory(ConfigurationPropertiesTest.class
+				.getSimpleName());
+		testFile = createFile(tempDir, "testFile", "source = file\n");
+		invalidFile = createFile(tempDir, "invalidFile",
+				"source = bad Unicode constant \\uu1045");
+	}
+
+	/**
+	 * Clean up.
+	 */
+	@AfterClass
+	public static void removeTestFiles() {
+		purgeDirectoryRecursively(tempDir);
+	}
+
+	/**
+	 * I don't want to see the INFO messages while running the tests.
+	 */
+	@Before
+	public void setLogging() {
+		setLoggerLevel(ConfigurationProperties.class, Level.WARN);
+	}
+
+	/**
+	 * Use the context stub, and be sure that it is clean for each test.
+	 */
+	@Before
+	public void initializeContextStubs() throws NamingException {
+		System.setProperty(InitialContext.INITIAL_CONTEXT_FACTORY,
+				InitialContextFactoryStub.class.getName());
+		InitialContextStub.reset();
+		initial = new InitialContext();
+	}
+
+	/**
+	 * {@link ConfigurationProperties} is a singleton, so we need to clean it
+	 * before each test.
+	 */
+	@Before
+	public void resetProperties() {
+		ConfigurationProperties.reset();
+	}
+
+	// ----------------------------------------------------------------------
+	// the tests
+	// ----------------------------------------------------------------------
+
+	@Test(expected = IllegalStateException.class)
+	public void topLevelContextIsMissing() {
+		ConfigurationProperties.getMap();
+	}
+
+	@Test(expected = IllegalStateException.class)
+	public void noEnvironmentMapping() throws NamingException {
+		// We map something in the same JNDI environment,
+		// but not the mapping we will be looking for.
+		initial.bind(NOT_THE_DESIRED_MAPPING, "doesn't matter");
+		ConfigurationProperties.getMap();
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void fileNotFound() throws NamingException {
+		initial.bind(CONFIGURATION_PROPERTIES_MAPPING, "noSuchFileOrResource");
+		ConfigurationProperties.getMap();
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void invalidFileFormat() throws NamingException {
+		initial.bind(CONFIGURATION_PROPERTIES_MAPPING, invalidFile.getPath());
+		ConfigurationProperties.getMap();
+	}
+
+	@Test
+	public void readFromResource() throws NamingException {
+		initial.bind(CONFIGURATION_PROPERTIES_MAPPING,
+				"edu/cornell/mannlib/vitro/webapp/test_config.properties");
+		assertExpectedMap(new String[][] { { "source", "resource" } });
+	}
+
+	@Test
+	public void readFromFile() throws NamingException {
+		initial.bind(CONFIGURATION_PROPERTIES_MAPPING, testFile.getPath());
+		assertExpectedMap(new String[][] { { "source", "file" } });
+	}
+
+	@Test
+	public void checkOtherMethods() throws NamingException {
+		initial.bind(CONFIGURATION_PROPERTIES_MAPPING, testFile.getPath());
+		assertEquals("file", ConfigurationProperties.getProperty("source"));
+		assertEquals(null, ConfigurationProperties.getProperty("notThere"));
+		assertEquals("default", ConfigurationProperties.getProperty("notThere",
+				"default"));
+	}
+
+	// ----------------------------------------------------------------------
+	// helper methods
+	// ----------------------------------------------------------------------
+
+	/**
+	 * Does the configuration properties map look like this group of key/value
+	 * pairs?
+	 */
+	private void assertExpectedMap(String[][] strings) {
+		Map<String, String> expected = new HashMap<String, String>();
+		for (String[] pair : strings) {
+			expected.put(pair[0], pair[1]);
+		}
+		assertEquals("properties map", expected, ConfigurationProperties
+				.getMap());
+	}
+
+}

webapp/test/edu/cornell/mannlib/vitro/webapp/test_config.properties

@@ -0,0 +1,4 @@
+#
+# This is a data file for ConfigurationPropertiesTest.
+#
+source = resource