Skip to content

json: honor the json_name field option (#158) - #271

Open
MavenRain wants to merge 1 commit into
mransan:masterfrom
MavenRain:fix/json-snake-case-field-names-158
Open

MavenRain wants to merge 1 commit into
mransan:masterfrom
MavenRain:fix/json-snake-case-field-names-158

Conversation

@MavenRain

Copy link
Copy Markdown
Contributor

Fixes #158.

Summary

ocaml-protoc currently ignores the json_name field option. A .proto file
can 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 the
bs backend.

Problem

Given this .proto:

message M {
  string renamed_field = 1 [json_name = "custom_JSON_name"];
}

ocaml-protoc --yojson on master generates:

assoc := ("renamedField", Pbrt_yojson.make_string v.renamed_field) :: !assoc;
...
| (("renamedField" | "renamed_field"), json_value) -> ...

custom_JSON_name appears nowhere. Anything exchanging JSON with a peer that
uses the same .proto therefore disagrees with it on the wire, in both
directions, 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_constructor resolve a
field's JSON name once: the json_name option when the .proto sets one to a
string, and camel_case_of_label otherwise. The four codegen files thread that
value where they previously called camel_case_of_label inline. rf_options
and vc_options were already carried on the IR, so nothing new had to be
plumbed through the compiler.

Per ProtoJSON, json_name replaces the camelCase key rather than adding to
it. So for a renamed field the encoders emit the json_name, and the decoders
accept exactly two spellings, the json_name and the original proto field name.
The camelCase name the field would otherwise have had is deliberately not
accepted. The existing field_name_pattern helper already collapses to a single
pattern when the two coincide, so unrenamed fields generate byte-identical code
to before.

The bs decoder gets the same dual-name treatment. Because it looks the value
up with Js.Dict.unsafeGet json "<literal>", the matched key has to be bound
when the two spellings differ:

| (("custom_JSON_name" | "renamed_field") as key) ->
  let json = Js.Dict.unsafeGet json key in

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 is
the name as written in the .proto, which is what ProtoJSON specifies.

Testing

dune runtest is green, including the pre-existing yojson suite and the
Google unittest conformance run.

New JsonName message in src/tests/yojson/yojson_unittest.proto plus a block
in yojson_unittest_ml.ml covering, for a plain field and for a oneof case:
encoding uses the json_name; decoding accepts the json_name; decoding
accepts the proto field name; decoding does not accept the superseded
camelCase name; and fields with no json_name behave exactly as before.

The expected behavior was not taken from memory. It was measured against
protoc 23.2 and the protobuf 7.35.1 Python runtime: every accepted and
rejected spelling in the test mirrors a row of that oracle, including the
decisive one, that {"renamedField": ...} is rejected once json_name is 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_name in each of the four codegen sites, make
the option reader always return None, and drop each half of the decoder's
two-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 .expected file if you would like that coverage.

Notes for the reviewer

  • A json_name whose value is not a string constant is ignored and the field
    falls back to camelCase, rather than being rejected. That matches how the rest
    of the compiler treats options it does not recognise. protoc errors on it,
    so tightening this is an easy follow-up if you would prefer it.
  • protoc also rejects a json_name that collides with another field's JSON
    name. 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.
  • The issue also asks about disabling camelCasing wholesale, which upstream
    exposes as a printer option (preserve_proto_field_names in C++,
    UseProtoNames in Go). That is a separate feature and I left it out: codegen
    plugins 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_name is
    the per-field, spec-defined way to get the same result and needs no new
    surface.

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support snake_case for field name in json

1 participant