PropertyFile bugfix: when loading from Properties, read values directly rather than re-parsing (since this caused decode errors on special chars due to a difference between Properties and PropertyFile escaping)

Author: petergeneric

guice/common/src/test/java/com/peterphi/std/guice/common/serviceprops/composite/GuiceConfigTest.java

@@ -2,6 +2,8 @@
 
 import org.junit.Test;
 
+import java.util.Properties;
+
 import static org.junit.Assert.assertEquals;
 
 public class GuiceConfigTest
@@ -49,4 +51,18 @@ public void testVariableRef()
 		assertEquals("getRaw", "a-${var}-b", props.getRaw("name", null));
 		assertEquals("get", "a-123-b", props.get("name", null));
 	}
+
+
+	@Test
+	public void testSetAllPropertiesPreservesLoggingPattern()
+	{
+		Properties properties = new Properties();
+		String name = "log4j.appender.logconsole.layout.ConversionPattern";
+		String pattern = "%-5p %d: café = #! %m%n";
+		properties.setProperty(name, pattern);
+
+		props.setAll(properties);
+
+		assertEquals(pattern, props.get(name, null));
+	}
 }

stdlib/src/main/java/com/peterphi/std/io/PropertyFile.java

@@ -9,15 +9,13 @@
 import java.io.File;
 import java.io.FileReader;
 import java.io.FileWriter;
-import java.io.IOError;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
 import java.io.Reader;
 import java.io.StringReader;
-import java.io.StringWriter;
 import java.io.Writer;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -82,7 +80,6 @@ public static PropertyFile find(final String fileName)
 	 *
 	 * @param classloader
 	 * @param fileNames
-	 *
 	 * @return
 	 */
 	public static PropertyFile find(final ClassLoader classloader, final String... fileNames)
@@ -201,7 +198,6 @@ public static PropertyFile openResource(final ClassLoader classloader, final URL
 	 * Find all property files by the name <code>name</code> by searching the classloader that owns PropertyFile
 	 *
 	 * @param name
-	 *
 	 * @return
 	 */
 	public static PropertyFile[] findAll(final String name)
@@ -215,7 +211,6 @@ public static PropertyFile[] findAll(final String name)
 	 *
 	 * @param loader
 	 * @param name
-	 *
 	 * @return
 	 */
 	public static PropertyFile[] findAll(ClassLoader loader, final String name)
@@ -423,21 +418,18 @@ public void load(Map<String, String> map)
 
 	public void load(Properties p)
 	{
-		try
-		{
-			StringWriter sw = new StringWriter();
-			p.store(sw, null);
-
-			StringBuffer sb = sw.getBuffer();
-
-			StringReader sr = new StringReader(sb.toString());
+		entries.clear();
+		vars.clear();
 
-			load(sr);
-		}
-		catch (IOException e)
+		// Properties already contains decoded strings
+		for (String name : p.stringPropertyNames())
 		{
-			throw new IOError(e);
+			NameValuePair nvp = new NameValuePair(name, p.getProperty(name));
+			entries.add(nvp);
+			vars.put(caseSensitive ? name : name.toLowerCase(), nvp);
 		}
+
+		hook_loaded();
 	}
 
 
@@ -498,6 +490,7 @@ public void save(String comment, OutputStream os) throws IOException
 	// MANIPULATION
 	// ////////////////////////////
 
+
 	/**
 	 * Make this PropertyFile object read-only
 	 */
@@ -896,13 +889,9 @@ protected void _rm(String name)
 	 * Creates a read-only version of a property file<br />
 	 * Fails if the file does not exist.
 	 *
-	 * @param filename
-	 * 		the filename to load
-	 *
+	 * @param filename the filename to load
 	 * @return
-	 *
-	 * @throws IOException
-	 * 		if the file cannot be loaded
+	 * @throws IOException if the file cannot be loaded
 	 */
 	public static PropertyFile readOnly(final File filename) throws IOException
 	{
@@ -916,7 +905,8 @@ public static PropertyFile readOnly(final File filename) throws IOException
 
 	////////////////////////////////////
 	// Property File entry types
-	////////////////////////////////////
+
+	/// /////////////////////////////////
 
 
 	protected abstract class Entry

stdlib/src/test/java/com/peterphi/std/io/PropertyFileTest.java

@@ -6,6 +6,7 @@
 import java.util.Properties;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 
 public class PropertyFileTest
 {
@@ -26,8 +27,109 @@ public void testToProperties()
 		f.set("a", "b");
 
 		Properties properties = new Properties();
-		properties.put("a","b");
+		properties.put("a", "b");
 
 		assertEquals(properties, f.toProperties());
 	}
+
+
+	@Test
+	public void testPropertiesPreserveDecodedCharacters()
+	{
+		Properties properties = new Properties();
+		properties.setProperty("pattern", "%-5p %d: café = #! %m%n");
+		properties.setProperty("special:key=#!\\path", " leading space\ttab\nnewline\rcarriage return\\literal\\n");
+
+		PropertyFile file = new PropertyFile(properties, true);
+
+		assertEquals(properties, file.toProperties());
+	}
+
+
+	@Test
+	public void testPropertiesIncludeDefaultsAndOverrides()
+	{
+		Properties defaults = new Properties();
+		defaults.setProperty("inherited", "default: value");
+		defaults.setProperty("overridden", "default");
+		Properties properties = new Properties(defaults);
+		properties.setProperty("overridden", "explicit");
+
+		PropertyFile file = new PropertyFile(properties);
+
+		assertEquals("default: value", file.get("inherited", null));
+		assertEquals("explicit", file.get("overridden", null));
+		assertEquals(2, file.getVariableCount());
+	}
+
+
+	@Test
+	public void testLoadPropertiesReplacesExistingContentsAndMarksClean() throws Exception
+	{
+		PropertyFile file = new PropertyFile(new java.io.StringReader("# old comment\nold=value\n"));
+		file.set("changed", "value");
+		Properties properties = new Properties();
+		properties.setProperty("new", "value: with = punctuation");
+
+		file.load(properties);
+
+		assertEquals(properties, file.toProperties());
+		assertEquals(1, file.getLineCount());
+		assertFalse(file.dirty);
+
+		file.load(new Properties());
+
+		assertEquals(0, file.getVariableCount());
+		assertEquals(0, file.getLineCount());
+		assertFalse(file.dirty);
+	}
+
+
+	@Test
+	public void testPropertiesRespectCaseSensitivity()
+	{
+		Properties properties = new Properties();
+		properties.setProperty("MixedCase", "value");
+
+		assertEquals("value", new PropertyFile(properties).get("mixedcase", null));
+		PropertyFile sensitive = new PropertyFile(properties, true);
+		assertEquals("value", sensitive.get("MixedCase", null));
+		assertFalse(sensitive.keySet().contains("mixedcase"));
+	}
+
+
+	@Test
+	public void testLoadPropertiesOnlyCallsLoadedHook()
+	{
+		class ObservedPropertyFile extends PropertyFile
+		{
+			int loads;
+			int additions;
+
+
+			@Override
+			protected void hook_loaded()
+			{
+				loads++;
+				super.hook_loaded();
+			}
+
+
+			@Override
+			protected void hook_added(String name, String value)
+			{
+				additions++;
+				super.hook_added(name, value);
+			}
+		}
+		ObservedPropertyFile file = new ObservedPropertyFile();
+		Properties properties = new Properties();
+		properties.setProperty("name", "value");
+
+		file.load(properties);
+
+		assertEquals(1, file.loads);
+		assertEquals(0, file.additions);
+		assertFalse(file.dirty);
+	}
 }