JSON parser: always accept both name variants, flag controls which we generate.

* JSON parser: always accept both name variants, flag controls which we generate.

My previous commit was based on wrong information about the
proto3 JSON spec.  It turns out we need to accept both field
name formats all the time, and a runtime flag should control
which we generate.

* Documented the preserve_proto_fieldnames option.

* Fix bugs in PR.

4 files changed, 39 insertions, 27 deletions

diff --git a/upb/json/parser.c b/upb/json/parser.c
index e1a334d..1662f1a 100644
--- a/upb/json/parser.c
+++ b/upb/json/parser.c
@@ -1527,7 +1527,7 @@ _again:
 #line 1270 "upb/json/parser.rl"
 
   if (p != pe) {
-    upb_status_seterrf(&parser->status, "Parse error at %s\n", p);
+    upb_status_seterrf(&parser->status, "Parse error at '%.*s'\n", p, pe - p);
     upb_env_reporterror(parser->env, &parser->status);
   } else {
     capture_suspend(parser, &p);
@@ -1628,6 +1628,7 @@ static void add_jsonname_table(upb_json_parsermethod *m, const upb_msgdef* md) {
       upb_msg_field_next(&i)) {
     const upb_fielddef *f = upb_msg_iter_field(&i);
 
+    /* Add an entry for the JSON name. */
     size_t field_len = upb_fielddef_getjsonname(f, buf, len);
     if (field_len > len) {
       size_t len2;
@@ -1638,10 +1639,10 @@ static void add_jsonname_table(upb_json_parsermethod *m, const upb_msgdef* md) {
     }
     upb_strtable_insert(t, buf, upb_value_constptr(f));
 
-    if (getenv("UPB_JSON_ACCEPT_LEGACY_FIELD_NAMES")) {
-      /* Temporary code to help people migrate if they were depending on the
-       * old, non-proto3-json-compliant field names.  In this case we
-       * recognize both old names and new names. */
+    if (strcmp(buf, upb_fielddef_name(f)) != 0) {
+      /* Since the JSON name is different from the regular field name, add an
+       * entry for the raw name (compliant proto3 JSON parsers must accept
+       * both). */
       upb_strtable_insert(t, upb_fielddef_name(f), upb_value_constptr(f));
     }
 
diff --git a/upb/json/parser.rl b/upb/json/parser.rl
index eecb3b2..943a490 100644
--- a/upb/json/parser.rl
+++ b/upb/json/parser.rl
@@ -1269,7 +1269,7 @@ size_t parse(void *closure, const void *hd, const char *buf, size_t size,
   %% write exec;
 
   if (p != pe) {
-    upb_status_seterrf(&parser->status, "Parse error at %s\n", p);
+    upb_status_seterrf(&parser->status, "Parse error at '%.*s'\n", p, pe - p);
     upb_env_reporterror(parser->env, &parser->status);
   } else {
     capture_suspend(parser, &p);
@@ -1363,6 +1363,7 @@ static void add_jsonname_table(upb_json_parsermethod *m, const upb_msgdef* md) {
       upb_msg_field_next(&i)) {
     const upb_fielddef *f = upb_msg_iter_field(&i);
 
+    /* Add an entry for the JSON name. */
     size_t field_len = upb_fielddef_getjsonname(f, buf, len);
     if (field_len > len) {
       size_t len2;
@@ -1373,10 +1374,10 @@ static void add_jsonname_table(upb_json_parsermethod *m, const upb_msgdef* md) {
     }
     upb_strtable_insert(t, buf, upb_value_constptr(f));
 
-    if (getenv("UPB_JSON_ACCEPT_LEGACY_FIELD_NAMES")) {
-      /* Temporary code to help people migrate if they were depending on the
-       * old, non-proto3-json-compliant field names.  In this case we
-       * recognize both old names and new names. */
+    if (strcmp(buf, upb_fielddef_name(f)) != 0) {
+      /* Since the JSON name is different from the regular field name, add an
+       * entry for the raw name (compliant proto3 JSON parsers must accept
+       * both). */
       upb_strtable_insert(t, upb_fielddef_name(f), upb_value_constptr(f));
     }
 
diff --git a/upb/json/printer.c b/upb/json/printer.c
index e6135bb..230aa0d 100644
--- a/upb/json/printer.c
+++ b/upb/json/printer.c
@@ -44,12 +44,11 @@ void freestrpc(void *ptr) {
 }
 
 /* Convert fielddef name to JSON name and return as a string piece. */
-strpc *newstrpc(upb_handlers *h, const upb_fielddef *f) {
+strpc *newstrpc(upb_handlers *h, const upb_fielddef *f,
+                bool preserve_fieldnames) {
   /* TODO(haberman): handle malloc failure. */
   strpc *ret = malloc(sizeof(*ret));
-  if (getenv("UPB_JSON_WRITE_LEGACY_FIELD_NAMES")) {
-    /* Temporary code to help people migrate if they were depending on the
-     * old, non-proto3-json-compliant field names. */
+  if (preserve_fieldnames) {
     ret->ptr = upb_strdup(upb_fielddef_name(f));
     ret->len = strlen(ret->ptr);
   } else {
@@ -565,10 +564,11 @@ static size_t mapkey_bytes(void *closure, const void *handler_data,
 
 static void set_enum_hd(upb_handlers *h,
                         const upb_fielddef *f,
+                        bool preserve_fieldnames,
                         upb_handlerattr *attr) {
   EnumHandlerData *hd = malloc(sizeof(EnumHandlerData));
   hd->enumdef = (const upb_enumdef *)upb_fielddef_subdef(f);
-  hd->keyname = newstrpc(h, f);
+  hd->keyname = newstrpc(h, f, preserve_fieldnames);
   upb_handlers_addcleanup(h, hd, free);
   upb_handlerattr_sethandlerdata(attr, hd);
 }
@@ -585,7 +585,8 @@ static void set_enum_hd(upb_handlers *h,
  * our sources that emit mapentry messages do so canonically (with one key
  * field, and then one value field), so this is not a pressing concern at the
  * moment. */
-void printer_sethandlers_mapentry(const void *closure, upb_handlers *h) {
+void printer_sethandlers_mapentry(const void *closure, bool preserve_fieldnames,
+                                  upb_handlers *h) {
   const upb_msgdef *md = upb_handlers_msgdef(h);
 
   /* A mapentry message is printed simply as '"key": value'. Rather than
@@ -659,7 +660,7 @@ void printer_sethandlers_mapentry(const void *closure, upb_handlers *h) {
       break;
     case UPB_TYPE_ENUM: {
       upb_handlerattr enum_attr = UPB_HANDLERATTR_INITIALIZER;
-      set_enum_hd(h, value_field, &enum_attr);
+      set_enum_hd(h, value_field, preserve_fieldnames, &enum_attr);
       upb_handlers_setint32(h, value_field, mapvalue_enum, &enum_attr);
       upb_handlerattr_uninit(&enum_attr);
       break;
@@ -678,13 +679,13 @@ void printer_sethandlers(const void *closure, upb_handlers *h) {
   bool is_mapentry = upb_msgdef_mapentry(md);
   upb_handlerattr empty_attr = UPB_HANDLERATTR_INITIALIZER;
   upb_msg_field_iter i;
-
-  UPB_UNUSED(closure);
+  const bool *preserve_fieldnames_ptr = closure;
+  const bool preserve_fieldnames = *preserve_fieldnames_ptr;
 
   if (is_mapentry) {
     /* mapentry messages are sufficiently different that we handle them
      * separately. */
-    printer_sethandlers_mapentry(closure, h);
+    printer_sethandlers_mapentry(closure, preserve_fieldnames, h);
     return;
   }
 
@@ -705,7 +706,8 @@ void printer_sethandlers(const void *closure, upb_handlers *h) {
     const upb_fielddef *f = upb_msg_iter_field(&i);
 
     upb_handlerattr name_attr = UPB_HANDLERATTR_INITIALIZER;
-    upb_handlerattr_sethandlerdata(&name_attr, newstrpc(h, f));
+    upb_handlerattr_sethandlerdata(&name_attr,
+                                   newstrpc(h, f, preserve_fieldnames));
 
     if (upb_fielddef_ismap(f)) {
       upb_handlers_setstartseq(h, f, startmap, &name_attr);
@@ -728,7 +730,7 @@ void printer_sethandlers(const void *closure, upb_handlers *h) {
          * option later to control this behavior, but we will wait for a real
          * need first. */
         upb_handlerattr enum_attr = UPB_HANDLERATTR_INITIALIZER;
-        set_enum_hd(h, f, &enum_attr);
+        set_enum_hd(h, f, preserve_fieldnames, &enum_attr);
 
         if (upb_fielddef_isseq(f)) {
           upb_handlers_setint32(h, f, repeated_enum, &enum_attr);
@@ -805,6 +807,8 @@ upb_sink *upb_json_printer_input(upb_json_printer *p) {
 }
 
 const upb_handlers *upb_json_printer_newhandlers(const upb_msgdef *md,
+                                                 bool preserve_fieldnames,
                                                  const void *owner) {
-  return upb_handlers_newfrozen(md, owner, printer_sethandlers, NULL);
+  return upb_handlers_newfrozen(
+      md, owner, printer_sethandlers, &preserve_fieldnames);
 }
diff --git a/upb/json/printer.h b/upb/json/printer.h
index 84d814c..36a24e9 100644
--- a/upb/json/printer.h
+++ b/upb/json/printer.h
@@ -36,8 +36,12 @@ class upb::json::Printer {
   /* The input to the printer. */
   Sink* input();
 
-  /* Returns handlers for printing according to the specified schema. */
-  static reffed_ptr<const Handlers> NewHandlers(const upb::MessageDef* md);
+  /* Returns handlers for printing according to the specified schema.
+   * If preserve_proto_fieldnames is true, the output JSON will use the
+   * original .proto field names (ie. {"my_field":3}) instead of using
+   * camelCased names, which is the default: (eg. {"myField":3}). */
+  static reffed_ptr<const Handlers> NewHandlers(const upb::MessageDef* md,
+                                                bool preserve_proto_fieldnames);
 
   static const size_t kSize = UPB_JSON_PRINTER_SIZE;
 
@@ -54,6 +58,7 @@ upb_json_printer *upb_json_printer_create(upb_env *e, const upb_handlers *h,
                                           upb_bytessink *output);
 upb_sink *upb_json_printer_input(upb_json_printer *p);
 const upb_handlers *upb_json_printer_newhandlers(const upb_msgdef *md,
+                                                 bool preserve_fieldnames,
                                                  const void *owner);
 
 UPB_END_EXTERN_C
@@ -68,8 +73,9 @@ inline Printer* Printer::Create(Environment* env, const upb::Handlers* handlers,
 }
 inline Sink* Printer::input() { return upb_json_printer_input(this); }
 inline reffed_ptr<const Handlers> Printer::NewHandlers(
-    const upb::MessageDef *md) {
-  const Handlers* h = upb_json_printer_newhandlers(md, &h);
+    const upb::MessageDef *md, bool preserve_proto_fieldnames) {
+  const Handlers* h = upb_json_printer_newhandlers(
+      md, preserve_proto_fieldnames, &h);
   return reffed_ptr<const Handlers>(h, &h);
 }
 }  /* namespace json */