From 6c011b5e4a3719ab272c7e4402f0f47d4f8bf1d3 Mon Sep 17 00:00:00 2001 From: Preben Ingvaldsen Date: Mon, 6 Apr 2015 15:29:59 -0700 Subject: [PATCH 1/2] Cleanup of ConfigDocumentParser and tests Clean up ConfigDocumentParser tests by using the `intercept` method to handle exception testing, moving some tests, and removing a test that was re-testing already tested functionality. Fix bug wherein parsing of a single value would throw an exception when the value is a map with a key that has an object value and no separator if the default ConfParserOptions where used. Add a test to validate this fix. --- .../config/impl/ConfigDocumentParser.java | 11 +-- .../impl/ConfigDocumentParserTest.scala | 80 ++++++------------- 2 files changed, 30 insertions(+), 61 deletions(-) diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigDocumentParser.java b/config/src/main/java/com/typesafe/config/impl/ConfigDocumentParser.java index aa0b25ae..cb392a7e 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDocumentParser.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDocumentParser.java @@ -15,7 +15,8 @@ final class ConfigDocumentParser { } static AbstractConfigNodeValue parseValue(Iterator tokens, ConfigOrigin origin, ConfigParseOptions options) { - ParseContext context = new ParseContext(options.getSyntax(), origin, tokens); + ConfigSyntax syntax = options.getSyntax() == null ? ConfigSyntax.CONF : options.getSyntax(); + ParseContext context = new ParseContext(syntax, origin, tokens); return context.parseSingleValue(); } @@ -171,7 +172,7 @@ final class ConfigDocumentParser { for (AbstractConfigNode node : values) { if (node instanceof AbstractConfigNodeValue) value = (AbstractConfigNodeValue)node; - else if (node == null) + else if (value == null) nodes.add(node); else putBack((new ArrayList(node.tokens())).get(0)); @@ -183,7 +184,7 @@ final class ConfigDocumentParser { // any leading/trailing whitespace for (int i = values.size() - 1; i >= 0; i--) { if (values.get(i) instanceof ConfigNodeSingleToken) { - putBack((new ArrayList(values.get(i).tokens())).get(0)); + putBack(((ConfigNodeSingleToken) values.get(i)).token()); values.remove(i); } else { break; @@ -497,7 +498,6 @@ final class ConfigDocumentParser { AbstractConfigNodeValue nextValue = consolidateValues(children); if (nextValue != null) { children.add(nextValue); - nextValue = null; } else { t = nextTokenCollectingWhitespace(children); @@ -510,7 +510,6 @@ final class ConfigDocumentParser { || Tokens.isSubstitution(t)) { nextValue = parseValue(t); children.add(nextValue); - nextValue = null; } else { throw parseError("List should have ] or a first element after the open [, instead had token: " + t @@ -543,7 +542,6 @@ final class ConfigDocumentParser { nextValue = consolidateValues(children); if (nextValue != null) { children.add(nextValue); - nextValue = null; } else { t = nextTokenCollectingWhitespace(children); if (Tokens.isValue(t) || t == Tokens.OPEN_CURLY @@ -551,7 +549,6 @@ final class ConfigDocumentParser { || Tokens.isSubstitution(t)) { nextValue = parseValue(t); children.add(nextValue); - nextValue = null; } else if (flavor != ConfigSyntax.JSON && t == Tokens.CLOSE_SQUARE) { // we allow one trailing comma putBack(t); diff --git a/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentParserTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentParserTest.scala index a0e6bf9f..ae02c8f5 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentParserTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentParserTest.scala @@ -13,65 +13,48 @@ class ConfigDocumentParserTest extends TestUtils { private def parseJSONFailuresTest(origText: String, containsMessage: String) { var exceptionThrown = false - try { - ConfigDocumentParser.parse(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) - } catch { - case e: Exception => - exceptionThrown = true - assertTrue(e.isInstanceOf[ConfigException]) - assertTrue(e.getMessage.contains(containsMessage)) + val e = intercept[ConfigException] { + ConfigDocumentParser.parse(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) } - assertTrue(exceptionThrown) + assertTrue(e.getMessage.contains(containsMessage)) } private def parseSimpleValueTest(origText: String, finalText: String = null) { val expectedRenderedText = if (finalText == null) origText else finalText val node = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults()) assertEquals(expectedRenderedText, node.render()) - assertTrue(node.isInstanceOf[AbstractConfigNodeValue]) + assertTrue(node.isInstanceOf[ConfigNodeSimpleValue]) val nodeJSON = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) assertEquals(expectedRenderedText, nodeJSON.render()) - assertTrue(nodeJSON.isInstanceOf[AbstractConfigNodeValue]) + assertTrue(nodeJSON.isInstanceOf[ConfigNodeSimpleValue]) } private def parseComplexValueTest(origText: String) { val node = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults()) assertEquals(origText, node.render()) - assertTrue(node.isInstanceOf[AbstractConfigNodeValue]) + assertTrue(node.isInstanceOf[ConfigNodeComplexValue]) val nodeJSON = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) assertEquals(origText, nodeJSON.render()) - assertTrue(nodeJSON.isInstanceOf[AbstractConfigNodeValue]) + assertTrue(nodeJSON.isInstanceOf[ConfigNodeComplexValue]) } private def parseSingleValueInvalidJSONTest(origText: String, containsMessage: String) { val node = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults()) assertEquals(origText, node.render()) - var exceptionThrown = false - try { - ConfigDocumentParser.parse(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) - } catch { - case e: Exception => - exceptionThrown = true - assertTrue(e.isInstanceOf[ConfigException]) - assertTrue(e.getMessage.contains(containsMessage)) + val e = intercept[ConfigException] { + ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) } - assertTrue(exceptionThrown) + assertTrue(e.getMessage.contains(containsMessage)) } private def parseLeadingTrailingFailure(toReplace: String) { - var exceptionThrown = false - try { + val e = intercept[ConfigException] { ConfigDocumentParser.parseValue(tokenize(toReplace), fakeOrigin(), ConfigParseOptions.defaults()) - } catch { - case e: Exception => - exceptionThrown = true - assertTrue(e.isInstanceOf[ConfigException]) - assertTrue(e.getMessage.contains("The value from setValue cannot have leading or trailing newlines, whitespace, or comments")) } - assertTrue(exceptionThrown) + assertTrue(e.getMessage.contains("The value from setValue cannot have leading or trailing newlines, whitespace, or comments")) } @Test @@ -212,7 +195,7 @@ class ConfigDocumentParserTest extends TestUtils { parseJSONFailuresTest("""{ "foo": 123 456 789 } """, "Expecting close brace } or a comma") // JSON must begin with { or [ - parseJSONFailuresTest(""""a": 123, "b": 456"""", "Document must have an object or array at root") + parseJSONFailuresTest(""""a": 123, "b": 456""", "Document must have an object or array at root") // JSON does not support unquoted text parseJSONFailuresTest("""{"foo": unquotedtext}""", "Token not allowed in valid JSON") @@ -255,22 +238,14 @@ class ConfigDocumentParserTest extends TestUtils { parseComplexValueTest("""["a","b","c"]""") // Check that concatenations are handled by CONF parsing - var origText = "123 456 unquotedtext abc" + var origText = "123 456 \"abc\"" var node = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults()) assertEquals(origText, node.render()) - // Check that concatenations in JSON will throw an error - origText = "123 456 789" - var exceptionThrown = false - try { - node = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) - } catch { - case e: Exception => - exceptionThrown = true - assertTrue(e.isInstanceOf[ConfigException]) - assertTrue(e.getMessage.contains("Parsing JSON and the value set in setValue was either a concatenation or had trailing whitespace, newlines, or comments")) - } - assertTrue(exceptionThrown) + // Check that keys with no separators and object values are handled by CONF parsing + origText = """{"foo" { "bar" : 12 } }""" + node = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults()) + assertEquals(origText, node.render()) } @Test @@ -291,17 +266,14 @@ class ConfigDocumentParserTest extends TestUtils { parseSingleValueInvalidJSONTest("${a.b}", "Substitutions (${} syntax) not allowed in JSON") // Check that concatenations in JSON will throw an error - val origText = "123 456 789" - var exceptionThrown = false - try { - val node = ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) - } catch { - case e: Exception => - exceptionThrown = true - assertTrue(e.isInstanceOf[ConfigException]) - assertTrue(e.getMessage.contains("Parsing JSON and the value set in setValue was either a concatenation or had trailing whitespace, newlines, or comments")) - } - assertTrue(exceptionThrown) + var origText = "123 456 \"abc\"" + var e = intercept[ConfigException] { ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) } + assertTrue(e.getMessage.contains("Parsing JSON and the value set in setValue was either a concatenation or had trailing whitespace, newlines, or comments")) + + // Check that keys with no separators and object values in JSON will throw an error + origText = """{"foo" { "bar" : 12 } }""" + e = intercept[ConfigException] { ConfigDocumentParser.parseValue(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax((ConfigSyntax.JSON))) } + assertTrue(e.getMessage.contains("""Key '"foo"' may not be followed by token: '{'""")) } @Test From c2b2c10d38a23dbedcc9d87d956156014f5b2a42 Mon Sep 17 00:00:00 2001 From: Preben Ingvaldsen Date: Tue, 7 Apr 2015 14:09:21 -0700 Subject: [PATCH 2/2] Clean up ConfigDocument tests Clean up the ConfigDocument tests by renaming certain tests and modifying all exception testing to use the intercept method. --- .../impl/ConfigDocumentParserTest.scala | 2 +- .../config/impl/ConfigDocumentTest.scala | 32 ++++++------------- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentParserTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentParserTest.scala index ae02c8f5..e26ab7f9 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentParserTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentParserTest.scala @@ -14,7 +14,7 @@ class ConfigDocumentParserTest extends TestUtils { private def parseJSONFailuresTest(origText: String, containsMessage: String) { var exceptionThrown = false val e = intercept[ConfigException] { - ConfigDocumentParser.parse(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) + ConfigDocumentParser.parse(tokenize(origText), fakeOrigin(), ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) } assertTrue(e.getMessage.contains(containsMessage)) } diff --git a/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentTest.scala index e34582aa..bfeca198 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigDocumentTest.scala @@ -252,7 +252,7 @@ class ConfigDocumentTest extends TestUtils { @Test def configDocumentArrayFailures { - // Attempting a replace on a ConfigDocument parsed from an array throws an error + // Attempting certain methods on a ConfigDocument parsed from an array throws an error val origText = "[1, 2, 3, 4, 5]" val document = ConfigDocumentFactory.parseString(origText) @@ -272,16 +272,9 @@ class ConfigDocumentTest extends TestUtils { // will fail val origText = "{\"foo\": \"bar\", \"baz\": \"qux\"}" val document = ConfigDocumentFactory.parseString(origText, ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) - var exceptionThrown = false - try { - document.setValue("foo", "unquoted") - } catch { - case e: Exception => - exceptionThrown = true - assertTrue(e.isInstanceOf[ConfigException]) - assertTrue(e.getMessage.contains("Token not allowed in valid JSON")) - } - assertTrue(exceptionThrown) + + val e = intercept[ConfigException] { document.setValue("foo", "unquoted") } + assertTrue(e.getMessage.contains("Token not allowed in valid JSON")) } @Test @@ -290,16 +283,9 @@ class ConfigDocumentTest extends TestUtils { // fail val origText = "{\"foo\": \"bar\", \"baz\": \"qux\"}" val document = ConfigDocumentFactory.parseString(origText, ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON)) - var exceptionThrown = false - try { - document.setValue("foo", "1 2 3 concatenation") - } catch { - case e: Exception => - exceptionThrown = true - assertTrue(e.isInstanceOf[ConfigException]) - assertTrue(e.getMessage.contains("Parsing JSON and the value set in setValue was either a concatenation or had trailing whitespace, newlines, or comments")) - } - assertTrue(exceptionThrown) + + val e = intercept[ConfigException] { document.setValue("foo", "1 2 3 concatenation") } + assertTrue(e.getMessage.contains("Parsing JSON and the value set in setValue was either a concatenation or had trailing whitespace, newlines, or comments")) } @Test @@ -427,7 +413,7 @@ class ConfigDocumentTest extends TestUtils { } @Test - def configDocumentIndentationValueWithIncludeTest { + def configDocumentIndentationValueWithInclude { val origText = "a {\n b {\n c : 22\n }\n}" val configDocument = ConfigDocumentFactory.parseString(origText) @@ -436,7 +422,7 @@ class ConfigDocumentTest extends TestUtils { } @Test - def configDocumentIndentationBadedOnIncludeNode { + def configDocumentIndentationBasedOnIncludeNode { val origText = "a : b\n include \"foo\"\n" val configDocument = ConfigDocumentFactory.parseString(origText)