loosen upb for json name conflict check in proto2 between json name and field

name. Once editions is supported this check should turn into a check on LEGACY_BEST_EFFORT

PiperOrigin-RevId: 572041162

Author: anandolee

python/google/protobuf/internal/json_format_test.py

@@ -12,21 +12,22 @@
 import json
 import math
 import struct
-
 import unittest
 
+from google.protobuf import descriptor_pool
+from google.protobuf import json_format
+from google.protobuf.internal import more_messages_pb2
+from google.protobuf.internal import test_proto3_optional_pb2
+
 from google.protobuf import any_pb2
 from google.protobuf import duration_pb2
 from google.protobuf import field_mask_pb2
 from google.protobuf import struct_pb2
 from google.protobuf import timestamp_pb2
 from google.protobuf import wrappers_pb2
-from google.protobuf.internal import test_proto3_optional_pb2
-from google.protobuf import descriptor_pool
-from google.protobuf import json_format
 from google.protobuf import any_test_pb2
-from google.protobuf import unittest_pb2
 from google.protobuf import unittest_mset_pb2
+from google.protobuf import unittest_pb2
 from google.protobuf.util import json_format_pb2
 from google.protobuf.util import json_format_proto3_pb2
 
@@ -1275,5 +1276,38 @@ def testNestedRecursiveLimit(self):
     json_format.Parse('{"payload": {}, "child": {"child":{}}}',
                       message, max_recursion_depth=3)
 
+  def testJsonNameConflictSerilize(self):
+    message = more_messages_pb2.ConflictJsonName(value=2)
+    self.assertEqual(
+        json.loads('{"old_value": 2}'),
+        json.loads(json_format.MessageToJson(message))
+    )
+
+    new_message = more_messages_pb2.ConflictJsonName(new_value=2)
+    self.assertEqual(
+        json.loads('{"value": 2}'),
+        json.loads(json_format.MessageToJson(new_message))
+    )
+
+  def testJsonNameConflictParse(self):
+    message = more_messages_pb2.ConflictJsonName()
+    json_format.Parse('{"value": 2}', message)
+    self.assertEqual(2, message.new_value)
+    self.assertEqual(0, message.value)
+
+  def testJsonNameConflictRoundTrip(self):
+    message = more_messages_pb2.ConflictJsonName(value=2)
+    parsed_message = more_messages_pb2.ConflictJsonName()
+    json_string = json_format.MessageToJson(message)
+    json_format.Parse(json_string, parsed_message)
+    self.assertEqual(message, parsed_message)
+
+    new_message = more_messages_pb2.ConflictJsonName(new_value=2)
+    new_parsed_message = more_messages_pb2.ConflictJsonName()
+    json_string = json_format.MessageToJson(new_message)
+    json_format.Parse(json_string, new_parsed_message)
+    self.assertEqual(new_message, new_parsed_message)
+
+
 if __name__ == '__main__':
   unittest.main()

python/google/protobuf/internal/more_messages.proto

@@ -343,3 +343,8 @@ message RequiredField {
 message RequiredWrapper {
   optional RequiredField request = 1;
 }
+
+message ConflictJsonName {
+  optional int32 value = 1 [json_name = "old_value"];
+  optional int32 new_value = 2 [json_name = "value"];
+}

upb/json/decode_test.cc

@@ -92,3 +92,11 @@ TEST(JsonTest, DecodeFloats) {
     EXPECT_EQ(box, nullptr);
   }
 }
+
+TEST(JsonTest, DecodeConflictJsonName) {
+  upb::Arena a;
+  std::string json_string = R"({"value": 2})";
+  upb_test_Box* box = JsonDecode(json_string.c_str(), a.ptr());
+  EXPECT_EQ(2, upb_test_Box_new_value(box));
+  EXPECT_EQ(0, upb_test_Box_value(box));
+}

upb/json/encode_test.cc

@@ -103,3 +103,14 @@ TEST(JsonTest, EncodeNullEnum) {
   EXPECT_EQ(R"({"val":null})",
             JsonEncode(foo, upb_JsonEncode_FormatEnumsAsIntegers));
 }
+
+TEST(JsonTest, EncodeConflictJsonName) {
+  upb::Arena a;
+  upb_test_Box* box = upb_test_Box_new(a.ptr());
+  upb_test_Box_set_value(box, 2);
+  EXPECT_EQ(R"({"old_value":2})", JsonEncode(box, 0));
+
+  upb_test_Box* new_box = upb_test_Box_new(a.ptr());
+  upb_test_Box_set_new_value(new_box, 2);
+  EXPECT_EQ(R"({"value":2})", JsonEncode(new_box, 0));
+}

upb/json/test.proto

@@ -49,4 +49,6 @@ message Box {
   optional google.protobuf.Value val = 6;
   optional float f = 7;
   optional double d = 8;
+  optional int32 value = 9 [json_name = "old_value"];
+  optional int32 new_value = 10 [json_name = "value"];
 }

upb/reflection/def_type.h

@@ -27,7 +27,6 @@ typedef enum {
   // Only inside message table.
   UPB_DEFTYPE_FIELD = 0,
   UPB_DEFTYPE_ONEOF = 1,
-  UPB_DEFTYPE_FIELD_JSONNAME = 2,
 } upb_deftype_t;
 
 #ifdef __cplusplus

upb/reflection/message_def.c

@@ -38,6 +38,9 @@ struct upb_MessageDef {
   upb_inttable itof;
   upb_strtable ntof;
 
+  // Looking up fields by json name.
+  upb_strtable jtof;
+
   /* All nested defs.
    * MEM: We could save some space here by putting nested defs in a contiguous
    * region and calculating counts from offsets or vice-versa. */
@@ -63,9 +66,6 @@ struct upb_MessageDef {
   bool in_message_set;
   bool is_sorted;
   upb_WellKnown well_known_type;
-#if UINTPTR_MAX == 0xffffffff
-  uint32_t padding;  // Increase size to a multiple of 8.
-#endif
 };
 
 static void assign_msg_wellknowntype(upb_MessageDef* m) {
@@ -208,16 +208,16 @@ bool upb_MessageDef_FindByNameWithSize(const upb_MessageDef* m,
 const upb_FieldDef* upb_MessageDef_FindByJsonNameWithSize(
     const upb_MessageDef* m, const char* name, size_t size) {
   upb_value val;
-  const upb_FieldDef* f;
+
+  if (upb_strtable_lookup2(&m->jtof, name, size, &val)) {
+    return upb_value_getconstptr(val);
+  }
 
   if (!upb_strtable_lookup2(&m->ntof, name, size, &val)) {
     return NULL;
   }
 
-  f = _upb_DefType_Unpack(val, UPB_DEFTYPE_FIELD);
-  if (!f) f = _upb_DefType_Unpack(val, UPB_DEFTYPE_FIELD_JSONNAME);
-
-  return f;
+  return _upb_DefType_Unpack(val, UPB_DEFTYPE_FIELD);
 }
 
 int upb_MessageDef_ExtensionRangeCount(const upb_MessageDef* m) {
@@ -397,17 +397,25 @@ void _upb_MessageDef_InsertField(upb_DefBuilder* ctx, upb_MessageDef* m,
       _upb_MessageDef_Insert(m, shortname, shortnamelen, field_v, ctx->arena);
   if (!ok) _upb_DefBuilder_OomErr(ctx);
 
-  if (strcmp(shortname, json_name) != 0) {
-    if (upb_strtable_lookup(&m->ntof, json_name, &v)) {
-      _upb_DefBuilder_Errf(ctx, "duplicate json_name (%s)", json_name);
-    }
+  // TODO: Once editions is supported this should turn into a
+  // check on LEGACY_BEST_EFFORT
+  if (strcmp(shortname, json_name) != 0 &&
+      upb_FileDef_Syntax(m->file) == kUpb_Syntax_Proto3 &&
+      upb_strtable_lookup(&m->ntof, json_name, &v)) {
+    _upb_DefBuilder_Errf(
+        ctx, "duplicate json_name for (%s) with original field name (%s)",
+        shortname, json_name);
+  }
 
-    const size_t json_size = strlen(json_name);
-    const upb_value json_v = _upb_DefType_Pack(f, UPB_DEFTYPE_FIELD_JSONNAME);
-    ok = _upb_MessageDef_Insert(m, json_name, json_size, json_v, ctx->arena);
-    if (!ok) _upb_DefBuilder_OomErr(ctx);
+  if (upb_strtable_lookup(&m->jtof, json_name, &v)) {
+    _upb_DefBuilder_Errf(ctx, "duplicate json_name (%s)", json_name);
   }
 
+  const size_t json_size = strlen(json_name);
+  ok = upb_strtable_insert(&m->jtof, json_name, json_size,
+                           upb_value_constptr(f), ctx->arena);
+  if (!ok) _upb_DefBuilder_OomErr(ctx);
+
   if (upb_inttable_lookup(&m->itof, field_number, NULL)) {
     _upb_DefBuilder_Errf(ctx, "duplicate field number (%u)", field_number);
   }
@@ -660,6 +668,9 @@ static void create_msgdef(upb_DefBuilder* ctx, const char* prefix,
   ok = upb_strtable_init(&m->ntof, n_oneof + n_field, ctx->arena);
   if (!ok) _upb_DefBuilder_OomErr(ctx);
 
+  ok = upb_strtable_init(&m->jtof, n_field, ctx->arena);
+  if (!ok) _upb_DefBuilder_OomErr(ctx);
+
   UPB_DEF_SET_OPTIONS(m->opts, DescriptorProto, MessageOptions, msg_proto);
 
   m->oneof_count = n_oneof;