Conversation
A .proto file could already declare `[json_name = "..."]` on a field, and the parser accepted it, but every JSON code generator then dropped it and used the camelCased field name anyway. The generated codec therefore disagreed on the wire, in both directions and with no diagnostic, with any other protobuf implementation reading the same descriptor. Resolve a field's JSON name once in Pb_codegen_util.json_label_of_label and json_label_of_constructor, and thread it through the yojson and bs backends where they previously called camel_case_of_label inline. rf_options and vc_options were already carried on the IR, so no new plumbing was needed. Per ProtoJSON, json_name replaces the camelCase key rather than adding to it: the encoders emit the json_name, and the decoders accept the json_name and the original proto field name but not the superseded camelCase spelling. Fields with no json_name generate byte-identical code to before, since field_name_pattern already collapses to a single pattern when the two names coincide. Also give the bs decoder the dual-name pattern that 5318252 gave the yojson decoder, so it accepts the original proto field name as ProtoJSON requires of parsers. It looks values up with Js.Dict.unsafeGet, so the matched key is bound when the two spellings differ. Behavior was cross-checked against protoc 23.2 and the protobuf 7.35.1 Python runtime, including the decisive case that an explicit json_name makes the auto-derived camelCase name a rejected unknown field. The new test was confirmed by mutation: seven mutants, one per changed decision, were each seen to fail the suite. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #158.
Summary
ocaml-protoccurrently ignores thejson_namefield option. A.protofilecan declare one, the parser accepts it, and it is then silently dropped, so the
generated JSON codec uses the camelCased field name anyway. The JSON that comes
out is not the JSON any other protobuf implementation produces for that
descriptor.
This makes both JSON backends honor
json_name, and closes a related gap in thebsbackend.Problem
Given this
.proto:ocaml-protoc --yojsonon master generates:custom_JSON_nameappears nowhere. Anything exchanging JSON with a peer thatuses the same
.prototherefore disagrees with it on the wire, in bothdirections, with no diagnostic.
Separately, 5318252 ("more compliant decoding for json") taught the yojson
decoder to accept the original proto field name alongside the camelCase one, as
ProtoJSON requires of parsers. The bs decoder never got that fix and still
accepts the camelCase spelling only.
Fix
Pb_codegen_util.json_label_of_label/json_label_of_constructorresolve afield's JSON name once: the
json_nameoption when the.protosets one to astring, and
camel_case_of_labelotherwise. The four codegen files thread thatvalue where they previously called
camel_case_of_labelinline.rf_optionsand
vc_optionswere already carried on the IR, so nothing new had to beplumbed through the compiler.
Per ProtoJSON,
json_namereplaces the camelCase key rather than adding toit. So for a renamed field the encoders emit the
json_name, and the decodersaccept exactly two spellings, the
json_nameand the original proto field name.The camelCase name the field would otherwise have had is deliberately not
accepted. The existing
field_name_patternhelper already collapses to a singlepattern when the two coincide, so unrenamed fields generate byte-identical code
to before.
The
bsdecoder gets the same dual-name treatment. Because it looks the valueup with
Js.Dict.unsafeGet json "<literal>", the matched key has to be boundwhen the two spellings differ:
The binding is only introduced when the two names differ, so the generated code
does not pick up an unused variable warning in the common case.
Enum values are untouched: they already encode via
cvc_string_value, which isthe name as written in the
.proto, which is what ProtoJSON specifies.Testing
dune runtestis green, including the pre-existing yojson suite and theGoogle unittestconformance run.New
JsonNamemessage insrc/tests/yojson/yojson_unittest.protoplus a blockin
yojson_unittest_ml.mlcovering, for a plain field and for aoneofcase:encoding uses the
json_name; decoding accepts thejson_name; decodingaccepts the proto field name; decoding does not accept the superseded
camelCase name; and fields with no
json_namebehave exactly as before.The expected behavior was not taken from memory. It was measured against
protoc23.2 and the protobuf 7.35.1 Python runtime: every accepted andrejected spelling in the test mirrors a row of that oracle, including the
decisive one, that
{"renamedField": ...}is rejected oncejson_nameis set.The oracle harness carries negative cases and reports whether they were actually
rejected, so a harness that silently accepted everything would not have been
mistaken for agreement.
The new test was confirmed by mutation, not just by passing. Seven mutants, one
per changed decision (ignore
json_namein each of the four codegen sites, makethe option reader always return
None, and drop each half of the decoder'stwo-name pattern) were each applied, seen to fail the suite, and restored.
Known gap, disclosed rather than papered over
An eighth mutant, dropping the proto-name half of the bs decoder's pattern,
survives the suite. That is not a weakness of this change specifically: the
repository has no automated coverage of the bs backend at all, since exercising
it needs a JavaScript toolchain. The bs half of this change was verified by
reading the generated output, quoted above, rather than by a test. Happy to add
a bs codegen fixture with an
.expectedfile if you would like that coverage.Notes for the reviewer
json_namewhose value is not a string constant is ignored and the fieldfalls back to camelCase, rather than being rejected. That matches how the rest
of the compiler treats options it does not recognise.
protocerrors on it,so tightening this is an easy follow-up if you would prefer it.
protocalso rejects ajson_namethat collides with another field's JSONname. This change does not add that check, so a collision produces two
identical match arms and an unused-match-case warning in the generated code.
That hazard already exists on master for two fields that camelCase to the same
key, so it is not new here, but it is easier to trigger now. Happy to add the
check.
exposes as a printer option (
preserve_proto_field_namesin C++,UseProtoNamesin Go). That is a separate feature and I left it out: codegenplugins do not currently receive the command line settings or the file
options, so a global flag needs a new channel into the backends, which felt
like it deserved its own PR and your opinion on the plumbing.
json_nameisthe per-field, spec-defined way to get the same result and needs no new
surface.