Unify behavior of deprecated_legacy_json_field_conflicts across upb and syntax.

This disables all checks of json_name for upb and protoc under both proto2 and proto3.  This option is deprecated that will be removed in future versions, and is only meant as a temporary solution.  This also fixes a latent bug in the calculation of camelcase name in Python/upb.

Fixes #12525

PiperOrigin-RevId: 603158026
pull/15650/head
Mike Kruskal 2024-01-31 14:20:03 -08:00 committed by Copybara-Service
parent 25c6d34d4e
commit 9a020c4a7b
6 changed files with 51 additions and 30 deletions

View File

@ -983,10 +983,23 @@ static PyObject* PyUpb_FieldDescriptor_GetName(PyUpb_DescriptorBase* self,
return PyUnicode_FromString(upb_FieldDef_Name(self->def));
}
static char PyUpb_AsciiIsUpper(char ch) { return ch >= 'A' && ch <= 'Z'; }
static char PyUpb_AsciiToLower(char ch) {
assert(PyUpb_AsciiIsUpper(ch));
return ch + ('a' - 'A');
}
static PyObject* PyUpb_FieldDescriptor_GetCamelCaseName(
PyUpb_DescriptorBase* self, void* closure) {
// TODO: Ok to use jsonname here?
return PyUnicode_FromString(upb_FieldDef_JsonName(self->def));
// Camelcase is equivalent to JSON name except for potentially the first
// character.
const char* name = upb_FieldDef_JsonName(self->def);
size_t size = strlen(name);
return size > 0 && PyUpb_AsciiIsUpper(name[0])
? PyUnicode_FromFormat("%c%s", PyUpb_AsciiToLower(name[0]),
name + 1)
: PyUnicode_FromStringAndSize(name, size);
}
static PyObject* PyUpb_FieldDescriptor_GetJsonName(PyUpb_DescriptorBase* self,

View File

@ -31,11 +31,6 @@
from google.protobuf.internal.descriptor_test import *
import unittest
# These fail because they attempt to add fields with conflicting JSON names.
# We don't want to support this going forward.
MakeDescriptorTest.testCamelcaseName.__unittest_expecting_failure__ = True
MakeDescriptorTest.testJsonName.__unittest_expecting_failure__ = True
# We pass this test, but the error message is slightly different.
# Our error message is better.
NewDescriptorTest.testImmutableCppDescriptor.__unittest_expecting_failure__ = True

View File

@ -2307,15 +2307,32 @@ TEST_F(ParserValidationErrorTest, Proto2CustomJsonConflictError) {
}
TEST_F(ParserValidationErrorTest, Proto3JsonConflictLegacy) {
ExpectHasValidationErrors(
ExpectParsesTo(
"syntax = 'proto3';\n"
"message TestMessage {\n"
" option deprecated_legacy_json_field_conflicts = true;\n"
" uint32 fooBar = 1;\n"
" uint32 foo_bar = 2;\n"
"}\n",
"4:9: The default JSON name of field \"foo_bar\" (\"fooBar\") conflicts "
"with the default JSON name of field \"fooBar\".\n");
"syntax: 'proto3'\n"
"message_type {\n"
" name: 'TestMessage'\n"
" field {\n"
" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'fooBar' number: 1\n"
" }\n"
" field {\n"
" label: LABEL_OPTIONAL type: TYPE_UINT32 name: 'foo_bar' number: 2\n"
" }\n"
" options {\n"
" uninterpreted_option {\n"
" name {\n"
" name_part: 'deprecated_legacy_json_field_conflicts'\n"
" is_extension: false\n"
" }\n"
" identifier_value: 'true'\n"
" }\n"
" }\n"
"}\n");
}
TEST_F(ParserValidationErrorTest, Proto2JsonConflictLegacy) {

View File

@ -6186,14 +6186,8 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto,
void DescriptorBuilder::CheckFieldJsonNameUniqueness(
const DescriptorProto& proto, const Descriptor* result) {
std::string message_name = result->full_name();
if (pool_->deprecated_legacy_json_field_conflicts_ ||
IsLegacyJsonFieldConflictEnabled(result->options())) {
if (result->file()->edition() == Edition::EDITION_PROTO3) {
// Only check default JSON names for conflicts in proto3. This is legacy
// behavior that will be removed in a later version.
CheckFieldJsonNameUniqueness(message_name, proto, result, false);
}
} else {
if (!pool_->deprecated_legacy_json_field_conflicts_ &&
!IsLegacyJsonFieldConflictEnabled(result->options())) {
// Check both with and without taking json_name into consideration. This is
// needed for field masks, which don't use json_name.
CheckFieldJsonNameUniqueness(message_name, proto, result, false);

View File

@ -7267,7 +7267,7 @@ TEST_F(ValidationErrorTest, ValidateJsonNameConflictProto2) {
// Test that field names that may conflict in JSON is not allowed by protoc.
TEST_F(ValidationErrorTest, ValidateJsonNameConflictProto3Legacy) {
BuildFileWithErrors(
BuildFile(
"name: 'foo.proto' "
"syntax: 'proto3' "
"message_type {"
@ -7275,10 +7275,7 @@ TEST_F(ValidationErrorTest, ValidateJsonNameConflictProto3Legacy) {
" options { deprecated_legacy_json_field_conflicts: true }"
" field { name:'AB' number:1 label:LABEL_OPTIONAL type:TYPE_INT32 }"
" field { name:'_a__b_' number:2 label:LABEL_OPTIONAL type:TYPE_INT32 }"
"}",
"foo.proto: Foo: NAME: The default JSON name of field \"_a__b_\" "
"(\"AB\") "
"conflicts with the default JSON name of field \"AB\".\n");
"}");
}
TEST_F(ValidationErrorTest, ValidateJsonNameConflictProto2Legacy) {

View File

@ -415,7 +415,10 @@ 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 &&
bool skip_json_conflicts =
UPB_DESC(MessageOptions_deprecated_legacy_json_field_conflicts)(
upb_MessageDef_Options(m));
if (!skip_json_conflicts && strcmp(shortname, json_name) != 0 &&
UPB_DESC(FeatureSet_json_format)(m->resolved_features) ==
UPB_DESC(FeatureSet_ALLOW) &&
upb_strtable_lookup(&m->ntof, json_name, &v)) {
@ -425,14 +428,16 @@ void _upb_MessageDef_InsertField(upb_DefBuilder* ctx, upb_MessageDef* m,
}
if (upb_strtable_lookup(&m->jtof, json_name, &v)) {
_upb_DefBuilder_Errf(ctx, "duplicate json_name (%s)", json_name);
if (!skip_json_conflicts) {
_upb_DefBuilder_Errf(ctx, "duplicate json_name (%s)", json_name);
}
} else {
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);
}
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);
}