From ea72b037796a892aa00ef23902186d8f6b5d2ae9 Mon Sep 17 00:00:00 2001
From: Tom Jackson <tjackson@fb.com>
Date: Tue, 2 Jul 2013 12:29:10 -0700
Subject: [PATCH] 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
---
 folly/json.cpp          |  6 ++++
 folly/json.h            |  4 +++
 folly/test/JsonTest.cpp | 76 ++++++++++++++++++++---------------------
 3 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/folly/json.cpp b/folly/json.cpp
index 445b4b0a..7711a203 100644
--- 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 84a70661..40f23f2a 100644
--- 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 ef66acee..e19ff21c 100644
--- 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) {
-- 
2.34.1