extensively fix fromProperties() and add tests

fromProperties() now parses paths with a strict "split on period"

It now handles more than one level of nested object.

It's now defined that if a properties file defines both an
object and a string at the same key, the object wins.

src/main/java/com/typesafe/config/impl/PropertiesParser.java

@@ -4,13 +4,15 @@ import java.io.IOException;
 import java.io.Reader;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.Enumeration;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
+import java.util.Set;
 
-import com.typesafe.config.ConfigException;
 import com.typesafe.config.ConfigOrigin;
 
 final class PropertiesParser {
@@ -21,18 +23,7 @@ final class PropertiesParser {
         return fromProperties(origin, props);
     }
 
-    static void verifyPath(String path) {
-        if (path.startsWith("."))
-            throw new ConfigException.BadPath(path, "Path starts with '.'");
-        if (path.endsWith("."))
-            throw new ConfigException.BadPath(path, "Path ends with '.'");
-        if (path.contains(".."))
-            throw new ConfigException.BadPath(path,
-                    "Path contains '..' (empty element)");
-    }
-
     static String lastElement(String path) {
-        verifyPath(path);
         int i = path.lastIndexOf('.');
         if (i < 0)
             return path;
@@ -41,7 +32,6 @@ final class PropertiesParser {
     }
 
     static String exceptLastElement(String path) {
-        verifyPath(path);
         int i = path.lastIndexOf('.');
         if (i < 0)
             return null;
@@ -51,67 +41,86 @@ final class PropertiesParser {
 
     static AbstractConfigObject fromProperties(ConfigOrigin origin,
             Properties props) {
-        Map<String, Map<String, AbstractConfigValue>> scopes = new HashMap<String, Map<String, AbstractConfigValue>>();
+        /*
+         * First, build a list of paths that will have values, either strings or
+         * objects.
+         */
+        Set<String> scopePaths = new HashSet<String>();
+        Set<String> valuePaths = new HashSet<String>();
         Enumeration<?> i = props.propertyNames();
         while (i.hasMoreElements()) {
             Object o = i.nextElement();
             if (o instanceof String) {
-                try {
+                // add value's path
                 String path = (String) o;
-                    String last = lastElement(path);
+                valuePaths.add(path);
-                    String exceptLast = exceptLastElement(path);
+
-                    if (exceptLast == null)
+                // all parent paths are objects
-                        exceptLast = "";
+                String next = exceptLastElement(path);
-                    Map<String, AbstractConfigValue> scope = scopes
+                while (next != null) {
-                            .get(exceptLast);
+                    scopePaths.add(next);
-                    if (scope == null) {
+                    next = exceptLastElement(next);
-                        scope = new HashMap<String, AbstractConfigValue>();
-                        scopes.put(exceptLast, scope);
-                    }
-                    String value = props.getProperty(path);
-                    scope.put(last, new ConfigString(origin, value));
-                } catch (ConfigException.BadPath e) {
-                    // just skip this one (log it?)
                 }
             }
         }
 
-        // pull out the list of objects that go inside other objects
+        /*
-        List<String> childPaths = new ArrayList<String>();
+         * If any string values are also objects containing other values, drop
-        for (String path : scopes.keySet()) {
+         * those string values - objects "win".
-            if (path != "")
+         */
-                childPaths.add(path);
+        valuePaths.removeAll(scopePaths);
+
+        /*
+         * Create maps for the object-valued values.
+         */
+        Map<String, AbstractConfigValue> root = new HashMap<String, AbstractConfigValue>();
+        Map<String, Map<String, AbstractConfigValue>> scopes = new HashMap<String, Map<String, AbstractConfigValue>>();
+
+        for (String path : scopePaths) {
+            Map<String, AbstractConfigValue> scope = new HashMap<String, AbstractConfigValue>();
+            scopes.put(path, scope);
         }
 
-        // put everything in its parent, ensuring all parents exist
+        /* Store string values in the associated scope maps */
-        for (String path : childPaths) {
+        for (String path : valuePaths) {
             String parentPath = exceptLastElement(path);
-            if (parentPath == null)
+            Map<String, AbstractConfigValue> parent = parentPath != null ? scopes
-                parentPath = "";
+                    .get(parentPath) : root;
 
-            Map<String, AbstractConfigValue> parent = scopes.get(parentPath);
+            String last = lastElement(path);
-            if (parent == null) {
+            String value = props.getProperty(path);
-                parent = new HashMap<String, AbstractConfigValue>();
+            parent.put(last, new ConfigString(origin, value));
-                scopes.put(parentPath, parent);
-            }
-            // NOTE: we are evil and cheating, we mutate the map we
-            // provide to SimpleConfigObject, which is not allowed by
-            // its contract, but since we know nobody has a ref to this
-            // SimpleConfigObject yet we can get away with it.
-            // Also we assume here that any info based on the map that
-            // SimpleConfigObject computes and caches in its constructor
-            // will not change. Basically this is a bad hack.
-            AbstractConfigObject o = new SimpleConfigObject(origin,
-                    scopes.get(path), ResolveStatus.RESOLVED);
-            String basename = lastElement(path);
-            parent.put(basename, o);
         }
 
-        Map<String, AbstractConfigValue> root = scopes.get("");
+        /*
-        if (root == null) {
+         * Make a list of scope paths from longest to shortest, so children go
-            // this would happen only if you had no properties at all
+         * before parents.
-            // in "props"
+         */
-            root = Collections.<String, AbstractConfigValue> emptyMap();
+        List<String> sortedScopePaths = new ArrayList<String>();
+        sortedScopePaths.addAll(scopePaths);
+        // sort descending by length
+        Collections.sort(sortedScopePaths, new Comparator<String>() {
+            @Override
+            public int compare(String a, String b) {
+                return b.length() - a.length();
+            }
+        });
+
+        /*
+         * Create ConfigObject for each scope map, working from children to
+         * parents to avoid modifying any already-created ConfigObject. This is
+         * where we need the sorted list.
+         */
+        for (String scopePath : sortedScopePaths) {
+            Map<String, AbstractConfigValue> scope = scopes.get(scopePath);
+
+            String parentPath = exceptLastElement(scopePath);
+            Map<String, AbstractConfigValue> parent = parentPath != null ? scopes
+                    .get(parentPath) : root;
+
+            AbstractConfigObject o = new SimpleConfigObject(origin, scope,
+                    ResolveStatus.RESOLVED);
+            parent.put(lastElement(scopePath), o);
         }
 
         // return root config object

src/test/scala/com/typesafe/config/impl/PropertiesTest.scala

@@ -0,0 +1,82 @@
+package com.typesafe.config.impl
+
+import org.junit.Assert._
+import org.junit._
+import java.util.Properties
+import com.typesafe.config.Config
+import com.typesafe.config.ConfigParseOptions
+
+class PropertiesTest extends TestUtils {
+    @Test
+    def pathSplitting() {
+        def last(s: String) = PropertiesParser.lastElement(s)
+        def exceptLast(s: String) = PropertiesParser.exceptLastElement(s)
+
+        assertEquals("a", last("a"))
+        assertNull(exceptLast("a"))
+
+        assertEquals("b", last("a.b"))
+        assertEquals("a", exceptLast("a.b"))
+
+        assertEquals("c", last("a.b.c"))
+        assertEquals("a.b", exceptLast("a.b.c"))
+
+        assertEquals("", last(""))
+        assertNull(null, exceptLast(""))
+
+        assertEquals("", last("."))
+        assertEquals("", exceptLast("."))
+
+        assertEquals("", last(".."))
+        assertEquals(".", exceptLast(".."))
+
+        assertEquals("", last("..."))
+        assertEquals("..", exceptLast("..."))
+    }
+
+    @Test
+    def funkyPathsInProperties() {
+        def testPath(propsPath: String, confPath: String) {
+            val props = new Properties()
+
+            props.setProperty(propsPath, propsPath)
+
+            val conf = Config.parse(props, ConfigParseOptions.defaults())
+
+            assertEquals(propsPath, conf.getString(confPath))
+        }
+
+        // the easy ones
+        testPath("x", "x")
+        testPath("y.z", "y.z")
+        testPath("q.r.s", "q.r.s")
+
+        // weird empty path element stuff
+        testPath("", "\"\"")
+        testPath(".", "\"\".\"\"")
+        testPath("..", "\"\".\"\".\"\"")
+        testPath("a.", "a.\"\"")
+        testPath(".b", "\"\".b")
+
+        // quotes in .properties
+        testPath("\"", "\"\\\"\"")
+    }
+
+    @Test
+    def objectsWinOverStrings() {
+        val props = new Properties()
+
+        props.setProperty("a.b", "foo")
+        props.setProperty("a", "bar")
+
+        props.setProperty("x", "baz")
+        props.setProperty("x.y", "bar")
+        props.setProperty("x.y.z", "foo")
+
+        val conf = Config.parse(props, ConfigParseOptions.defaults())
+
+        assertEquals(2, conf.size())
+        assertEquals("foo", conf.getString("a.b"))
+        assertEquals("foo", conf.getString("x.y.z"))
+    }
+}