From aae70ecb8e7e264bf06e5c7a41349722c4a2fa6a Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Tue, 15 Nov 2011 18:38:32 -0500 Subject: [PATCH] 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. --- .../config/impl/PropertiesParser.java | 129 ++++++++++-------- .../typesafe/config/impl/PropertiesTest.scala | 82 +++++++++++ 2 files changed, 151 insertions(+), 60 deletions(-) create mode 100644 src/test/scala/com/typesafe/config/impl/PropertiesTest.scala diff --git a/src/main/java/com/typesafe/config/impl/PropertiesParser.java b/src/main/java/com/typesafe/config/impl/PropertiesParser.java index 8afab336..30ccc46f 100644 --- a/src/main/java/com/typesafe/config/impl/PropertiesParser.java +++ b/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> scopes = new HashMap>(); + /* + * First, build a list of paths that will have values, either strings or + * objects. + */ + Set scopePaths = new HashSet(); + Set valuePaths = new HashSet(); Enumeration i = props.propertyNames(); while (i.hasMoreElements()) { Object o = i.nextElement(); if (o instanceof String) { - try { - String path = (String) o; - String last = lastElement(path); - String exceptLast = exceptLastElement(path); - if (exceptLast == null) - exceptLast = ""; - Map scope = scopes - .get(exceptLast); - if (scope == null) { - scope = new HashMap(); - 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?) + // add value's path + String path = (String) o; + valuePaths.add(path); + + // all parent paths are objects + String next = exceptLastElement(path); + while (next != null) { + scopePaths.add(next); + next = exceptLastElement(next); } } } - // pull out the list of objects that go inside other objects - List childPaths = new ArrayList(); - for (String path : scopes.keySet()) { - if (path != "") - childPaths.add(path); + /* + * If any string values are also objects containing other values, drop + * those string values - objects "win". + */ + valuePaths.removeAll(scopePaths); + + /* + * Create maps for the object-valued values. + */ + Map root = new HashMap(); + Map> scopes = new HashMap>(); + + for (String path : scopePaths) { + Map scope = new HashMap(); + scopes.put(path, scope); } - // put everything in its parent, ensuring all parents exist - for (String path : childPaths) { + /* Store string values in the associated scope maps */ + for (String path : valuePaths) { String parentPath = exceptLastElement(path); - if (parentPath == null) - parentPath = ""; + Map parent = parentPath != null ? scopes + .get(parentPath) : root; - Map parent = scopes.get(parentPath); - if (parent == null) { - parent = new HashMap(); - 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); + String last = lastElement(path); + String value = props.getProperty(path); + parent.put(last, new ConfigString(origin, value)); } - Map root = scopes.get(""); - if (root == null) { - // this would happen only if you had no properties at all - // in "props" - root = Collections. emptyMap(); + /* + * Make a list of scope paths from longest to shortest, so children go + * before parents. + */ + List sortedScopePaths = new ArrayList(); + sortedScopePaths.addAll(scopePaths); + // sort descending by length + Collections.sort(sortedScopePaths, new Comparator() { + @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 scope = scopes.get(scopePath); + + String parentPath = exceptLastElement(scopePath); + Map parent = parentPath != null ? scopes + .get(parentPath) : root; + + AbstractConfigObject o = new SimpleConfigObject(origin, scope, + ResolveStatus.RESOLVED); + parent.put(lastElement(scopePath), o); } // return root config object diff --git a/src/test/scala/com/typesafe/config/impl/PropertiesTest.scala b/src/test/scala/com/typesafe/config/impl/PropertiesTest.scala new file mode 100644 index 00000000..69b86e4c --- /dev/null +++ b/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")) + } +}