Allowing trailing comma in folly::parseJson

Summary:
Introduced a new serialization option, `allow_trailing_comma`, which allows trailing commas to be included in strings when they are parsed. This isn't strictly allowed by RFC 4627, but we're allowing more than that anyway.

I've run into this dozens of times, especially while using SMC.

Test Plan: Unit tests

Reviewed By: delong.j@fb.com

FB internal diff: D872581

diff --git a/folly/json.cpp b/folly/json.cpp
index 445b4b0ae5196c939f244697acb1e5b1a951fd64..7711a20332c85d05978554bd7c11919e29d69833 100644 (file)
--- a/folly/json.cpp
+++ b/folly/json.cpp
@@ -385,6 +385,9 @@ dynamic parseObject(Input& in) {
   }
 
   for (;;) {
+    if (in.getOpts().allow_trailing_comma && *in == '}') {
+      break;
+    }
     if (*in == '\"') { // string
       auto key = parseString(in);
       in.skipWhitespace();
@@ -426,6 +429,9 @@ dynamic parseArray(Input& in) {
   }
 
   for (;;) {
+    if (in.getOpts().allow_trailing_comma && *in == ']') {
+      break;
+    }
     ret.push_back(parseValue(in));
     in.skipWhitespace();
     if (*in != ',') {

diff --git a/folly/json.h b/folly/json.h
index 84a70661c8fd790cfb4f4aa314f5cc4421b257c0..40f23f2a94402f825867358ee46dfc819d8ee1b9 100644 (file)
--- a/folly/json.h
+++ b/folly/json.h
@@ -58,6 +58,7 @@ namespace json {
       , pretty_formatting(false)
       , encode_non_ascii(false)
       , validate_utf8(false)
+      , allow_trailing_comma(false)
     {}
 
     // If true, keys in an object can be non-strings.  (In strict
@@ -81,6 +82,9 @@ namespace json {
 
     // Check that strings are valid utf8
     bool validate_utf8;
+
+    // Allow trailing comma in lists of values / items
+    bool allow_trailing_comma;
   };
 
   /*

diff --git a/folly/test/JsonTest.cpp b/folly/test/JsonTest.cpp
index ef66acee7e60a74f71cbd0002cbd954d50df7218..e19ff21ce5925f57d01264a760fe9290f45f62dc 100644 (file)
--- a/folly/test/JsonTest.cpp
+++ b/folly/test/JsonTest.cpp
@@ -75,6 +75,8 @@ TEST(Json, Parse) {
   EXPECT_EQ(-std::numeric_limits<double>::infinity(),
             parseJson("-Infinity").asDouble());
   EXPECT_TRUE(std::isnan(parseJson("NaN").asDouble()));
+
+  // case matters
   EXPECT_THROW(parseJson("infinity"), std::runtime_error);
   EXPECT_THROW(parseJson("inf"), std::runtime_error);
   EXPECT_THROW(parseJson("nan"), std::runtime_error);
@@ -86,32 +88,16 @@ TEST(Json, Parse) {
     EXPECT_EQ(boost::prior(array.end())->size(), 4);
   }
 
-  bool caught = false;
-  try {
-    parseJson("\n[12,\n\nnotvalidjson");
-  } catch (const std::exception& e) {
-    caught = true;
-  }
-  EXPECT_TRUE(caught);
+  EXPECT_THROW(parseJson("\n[12,\n\nnotvalidjson"),
+               std::runtime_error);
 
-  caught = false;
-  try {
-    parseJson("12e2e2");
-  } catch (const std::exception& e) {
-    caught = true;
-  }
-  EXPECT_TRUE(caught);
-
-  caught = false;
-  try {
-    parseJson("{\"foo\":12,\"bar\":42} \"something\"");
-  } catch (const std::exception& e) {
-    // incomplete parse
-    caught = true;
-  }
-  EXPECT_TRUE(caught);
+  EXPECT_THROW(parseJson("12e2e2"),
+               std::runtime_error);
+
+  EXPECT_THROW(parseJson("{\"foo\":12,\"bar\":42} \"something\""),
+               std::runtime_error);
 
-  dynamic anotherVal = dynamic::object
+  dynamic value = dynamic::object
     ("foo", "bar")
     ("junk", 12)
     ("another", 32.2)
@@ -128,11 +114,11 @@ TEST(Json, Parse) {
     ;
 
   // Print then parse and get the same thing, hopefully.
-  auto value = parseJson(toJson(anotherVal));
-  EXPECT_EQ(value, anotherVal);
+  EXPECT_EQ(value, parseJson(toJson(value)));
+
 
   // Test an object with non-string values.
-  dynamic something = folly::parseJson(
+  dynamic something = parseJson(
     "{\"old_value\":40,\"changed\":true,\"opened\":false}");
   dynamic expected = dynamic::object
     ("old_value", 40)
@@ -141,6 +127,28 @@ TEST(Json, Parse) {
   EXPECT_EQ(something, expected);
 }
 
+TEST(Json, ParseTrailingComma) {
+  folly::json::serialization_opts on, off;
+  on.allow_trailing_comma = true;
+  off.allow_trailing_comma = false;
+
+  dynamic arr { 1, 2 };
+  EXPECT_EQ(arr, parseJson("[1, 2]", on));
+  EXPECT_EQ(arr, parseJson("[1, 2,]", on));
+  EXPECT_EQ(arr, parseJson("[1, 2, ]", on));
+  EXPECT_EQ(arr, parseJson("[1, 2 , ]", on));
+  EXPECT_EQ(arr, parseJson("[1, 2 ,]", on));
+  EXPECT_THROW(parseJson("[1, 2,]", off), std::runtime_error);
+
+  dynamic obj = dynamic::object("a", 1);
+  EXPECT_EQ(obj, parseJson("{\"a\": 1}", on));
+  EXPECT_EQ(obj, parseJson("{\"a\": 1,}", on));
+  EXPECT_EQ(obj, parseJson("{\"a\": 1, }", on));
+  EXPECT_EQ(obj, parseJson("{\"a\": 1 , }", on));
+  EXPECT_EQ(obj, parseJson("{\"a\": 1 ,}", on));
+  EXPECT_THROW(parseJson("{\"a\":1,}", off), std::runtime_error);
+}
+
 TEST(Json, JavascriptSafe) {
   auto badDouble = (1ll << 63ll) + 1;
   dynamic badDyn = badDouble;
@@ -160,17 +168,9 @@ TEST(Json, Produce) {
   value = parseJson("\"Control code: \001 \002 \x1f\"");
   EXPECT_EQ(toJson(value), R"("Control code: \u0001 \u0002 \u001f")");
 
-  bool caught = false;
-  try {
-    dynamic d = dynamic::object;
-    d["abc"] = "xyz";
-    d[42.33] = "asd";
-    auto str = toJson(d);
-  } catch (std::exception const& e) {
-    // We're not allowed to have non-string keys in json.
-    caught = true;
-  }
-  EXPECT_TRUE(caught);
+  // We're not allowed to have non-string keys in json.
+  EXPECT_THROW(toJson(dynamic::object("abc", "xyz")(42.33, "asd")),
+               std::runtime_error);
 }
 
 TEST(Json, JsonEscape) {