Skip to content

Add tests for duplicate field ordering and unknown field errors when performing JSON deserialization #1132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add tests for duplicate field ordering and unknown field errors when performing JSON deserialization #1132

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8127b73
Add tests for field casing, duplicate field ordering, and unknown fie…
hudlow May 17, 2025
2c10929
Remove field casing test that duplicates protobuf conformance tests
hudlow May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions packages/protobuf-test/src/json.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,30 @@ describe("extensions in JSON", () => {
});
});

describe("JsonReadOptions", () => {
describe("ignoreUnknownFields", () => {
test("throws error when false", () => {
expect(() =>
fromJsonString(proto3_ts.Proto3MessageSchema, '{ "unknown": 1 }', {
ignoreUnknownFields: false,
}),
).toThrow();
});
test("does not throw error when true", () => {
expect(() =>
fromJsonString(proto3_ts.Proto3MessageSchema, '{ "unknown": 1 }', {
ignoreUnknownFields: true,
}),
).not.toThrow();
});
test("defaults to false", () => {
expect(() =>
fromJsonString(proto3_ts.Proto3MessageSchema, '{ "unknown": 1 }'),
).toThrow();
});
});
});

describe("JsonWriteOptions", () => {
describe("alwaysEmitImplicit", () => {
test("emits proto3 implicit fields", async () => {
Expand Down Expand Up @@ -833,6 +857,41 @@ describe("JsonWriteOptions", () => {
});
});

describe("parsing funny JSON", () => {
describe("duplicate fields", () => {
// This depends on the ECMA-262-defined JSON.parse() behavior, which is itself
// specified to choose the last field value for a duplicate field.
Comment on lines +861 to +863
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior has been discussed in #1038. I'm not sure these tests are particularly useful, because they basically test JSON.parse 🤔

I just noticed that the ProtoJSON documentation was recently updated with the following:

Multiple values for
singular fields (using duplicate or equivalent JSON keys) are accepted and the
last value is retained, as with binary format parsing. Note that not all
protobuf JSON parser implementations are conformant, and some nonconformant
implementations may reject duplicate keys instead.

With JSON.parse, the last property also wins for non-singular fields:

    const a = fromJsonString(
      proto3_ts.Proto3MessageSchema,
      '{ "repeatedStringField": ["b"], "repeatedStringField": ["a"] }',
    );
    const b = fromJsonString(
      proto3_ts.Proto3MessageSchema,
      '{ "repeatedStringField": ["a"], "repeatedStringField": ["b"] }',
    );

    expect(a.repeatedStringField).toStrictEqual(["a"]);
    expect(b.repeatedStringField).toStrictEqual(["b"]);

Quite different to the binary format, where repeated values accumulate...

Copy link
Contributor Author

@hudlow hudlow May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior has been discussed in #1038.

I commented there.

I'm not sure these tests are particularly useful, because they basically test JSON.parse 🤔

That's right, and in that sense they are certainly not unit tests. I think they are worthwhile for completeness, but I'm certainly open to being convinced they're not necessary in this testing context.

I just noticed that the ProtoJSON documentation was recently updated

Yeah, I read that and it's part of what prompted me to write tests for the as-is behavior as the "correct" baseline. I was actually doing that in preparation for filing what I now know would have been a dupe of #1038. 🤪

Given this update to the spec, are tests being added to [disappears for an hour] uhm, where was I? Oh, I was going to ask if tests would be added to the conformance suite, but there are already tests verifying the opposite behavior, but the tests are broken. This was quite the bunny trail: protocolbuffers/protobuf#21873

Quite different to the binary format, where repeated values accumulate...

I think you mean to say that ProtoJSON arrays are different from the binary format where array elements can be serialized non-contiguously. 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I noticed the test cases too... Thanks for filing an issue - depending on the outcome, we can decide whether the tests here are worthwhile or not.

FWIW, I didn't see a conformance test confirming that casing other than the original field name or json name must be rejected 😬

Arrays are non-contiguous in the binary format - well said. It just occurred to me that behavior is also different with singular message fields. They can occur multiple times in the binary format, which will merge them, unlike ProtoJSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrays are non-contiguous in the binary format - well said. It just occurred to me that behavior is also different with singular message fields. They can occur multiple times in the binary format, which will merge them, unlike ProtoJSON.

Interesting; I don't think I comprehend the benefit of this for a wire format. (And, as with ignoring duplicated JSON fields, I do see the drawbacks.)

There are also some interesting things around the edges of the merge algorithm, if I'm reading it right:

  • Map values for the same key don't get recursively merged even when they are a mergeable type (a repeated field, a message, or another map). This means (I think) that you couldn't replicate the merge logic in two ProtoJSON representations without considering the underlying types. (Probably not a huge deal, but a small blow to the self-describing benefits of JSON serialization.)
  • Like JSON, Protobuf is missing a set type, so there's no way to signal that a merged repeated field should be deduplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I comprehend the benefit of this for a wire format.

Is the benefit that you can stream a Protobuf message and update a field along the way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unaware of documentation explaining the design choices.

It could be useful for some (constrained) applications of the event sourcing pattern.

test("chooses last field when duplicated", () => {
const a = fromJsonString(
proto3_ts.Proto3MessageSchema,
'{ "singularStringField": "b", "singularStringField": "a" }',
);
const b = fromJsonString(
proto3_ts.Proto3MessageSchema,
'{ "singularStringField": "a", "singularStringField": "b" }',
);

expect(a.singularStringField).toBe("a");
expect(b.singularStringField).toBe("b");
});
// This depends on the ECMA-262-defined behavior for JSON.parse() and
// Object.entries() and the internal preservation of object key order.
test("chooses last field when duplicated, even when fields have different casing", () => {
const a = fromJsonString(
proto3_ts.Proto3MessageSchema,
'{ "singular_string_field": "b", "singularStringField": "a" }',
);
const b = fromJsonString(
proto3_ts.Proto3MessageSchema,
'{ "singularStringField": "a", "singular_string_field": "b" }',
);

expect(a.singularStringField).toBe("a");
expect(b.singularStringField).toBe("b");
});
});
});

// Coverage for JSON parse errors to guard against regressions.
// We do not cover all cases here. Map fields and oneofs are incomplete,
// and bytes, string, and other scalar types are not tested.
Expand Down