json: Ignore unknown enum values #170

whs · 2021-10-10T04:46:03Z

While parsing proto from JSON, any unknown enum values would result in assertion error. This would cause backward compatibility issue.

I looked on how other implementations are doing it and found that the protobuf team has decided that ignoringUnknownFields not ignoring missing enum values is a bug. The Java implementation now returns 0 if ignoringUnknownFields is set.

timostamm · 2021-10-11T14:09:53Z

Thanks for this PR, @whs.

It looks like the PR you mentioned has not landed. This PR however has.

The difference between those PRs is that the latter one ignores unrecognized values instead of setting the field to 0 or anything else. This is not relevant most of the time, because you would usually read from JSON into a blank message (that already has field value 0), but I think it is important to be precise here.

whs · 2021-10-11T14:16:22Z

I suppose returning undefined for the merging algorithm to sort that out would be the more correct behavior here. I've updated the PR.

timostamm

Left some comments on how fields with unrecognized values could be skipped without rewriting the entire class. It's not pretty, but I'd still prefer.

timostamm · 2021-10-11T14:10:58Z

packages/runtime/spec/reflection-json-reader.spec.ts

+        it('return -1 if unknown value was found, but ignoreUnknownFields was set', function () {
+            const reader = new ReflectionJsonReader({typeName: '.test.Message', fields: []});
+            expect(reader.enum([".spec.SimpleEnum", SimpleEnum], 'test', '', true)).toEqual(0);
+        })


If the string value is not recognized, nothing should be done here.

timostamm · 2021-10-11T14:16:55Z

packages/runtime/src/reflection-json-reader.ts

@@ -196,7 +196,7 @@ export class ReflectionJsonReader {
    /**
     * google.protobuf.NullValue accepts only JSON `null`.
     */
-    enum(type: EnumInfo, json: unknown, fieldName: string): UnknownEnum {
+    enum(type: EnumInfo, json: unknown, fieldName: string, ignoreUnknownFields = false): UnknownEnum {


Can you omit the default argument? ignoreUnknownStringValue: boolean is enough.

For unrecognized string values, we should return false. The return type of the method can be UnknownEnum | false. The method needs a doc explaining that false means an unknown string value was encountered.

timostamm · 2021-10-11T14:26:43Z

packages/runtime/src/reflection-json-reader.ts

                        case "enum":
-                            val = this.enum(field.V.T(), jsonObjValue, field.name);
+                            val = this.enum(field.V.T(), jsonObjValue, field.name, options.ignoreUnknownFields);
                            break;


Unfortunately, the existing code isn't suited well to skipping fields here. We'll have to resort to labels 🙄

case "enum": val = this.enum(field.V.T(), jsonObjValue, field.name, options.ignoreUnknownFields); if (val === false) break map_entries; break;

And further above:

// read entries map_entries: for (const [jsonObjKey, jsonObjValue] of Object.entries(jsonValue)) {

Shouldn't this be continue instead of break? (eg. {key1: "INVALID_ENUM", key2: "VALID_ENUM"} would break skip the second key?)

Sorry, you're right, let's not break the entire loop!

timostamm · 2021-10-11T14:27:28Z

packages/runtime/src/reflection-json-reader.ts

@@ -154,7 +154,7 @@ export class ReflectionJsonReader {
                            break;

                        case "enum":
-                            val = this.enum(field.T(), jsonItem, field.name);
+                            val = this.enum(field.T(), jsonItem, field.name, options.ignoreUnknownFields);


Same as for maps, we'll need a label array_entries and break that here.

For array if we continue I suppose we would cause the output length to be less than the input length?

Yes, same for map. That's what JsonFormat.java does.

timostamm · 2021-10-11T14:28:40Z

packages/runtime/src/reflection-json-reader.ts

@@ -180,7 +180,7 @@ export class ReflectionJsonReader {
                        break;

                    case "enum":
-                        target[localName] = this.enum(field.T(), jsonValue, field.name);
+                        target[localName] = this.enum(field.T(), jsonValue, field.name, options.ignoreUnknownFields);


This one will work just by not setting the value:

case "enum": let val = this.enum(field.T(), jsonValue, field.name, options.ignoreUnknownFields); if (val === false) break; target[localName] = val; break;

timostamm · 2021-10-11T14:36:09Z

I suppose returning undefined for the merging algorithm to sort that out would be the more correct behavior here. I've updated the PR.

Thanks, just saw that while reviewing 😅 . Whether we return false or undefined does not matter to me, but the read method cannot set undefined values - this would break its contract, it really has to skip.

whs · 2021-10-11T15:43:41Z

Updated the PR to skip fields - also added additional tests on read()

timostamm

Thank you 🙏

timostamm · 2021-10-11T18:43:38Z

Released in v2.0.7

whs · 2021-10-12T02:15:18Z

Would you mind labeling this PR with hacktoberfest-accepted. It's a new rule this year.

whs added 2 commits October 10, 2021 11:37

Parse unknown enum values as -1

9f57783

Set unknown value to 0 instead of -1

6f6ac6c

whs changed the title ~~json: Parse unknown enum values as -1~~ json: Parse unknown enum values as 0 Oct 10, 2021

Return undefined instead of 0 on invalid enum value

3c634f7

timostamm reviewed Oct 11, 2021

View reviewed changes

Skip the field entirely on invalid enum

5f82d07

timostamm approved these changes Oct 11, 2021

View reviewed changes

timostamm changed the title ~~json: Parse unknown enum values as 0~~ json: Ignore unknown enum values Oct 11, 2021

timostamm merged commit 995728c into timostamm:master Oct 11, 2021

timostamm added the hacktoberfest-accepted label Oct 12, 2021

antongrbin mentioned this pull request Dec 22, 2021

[MIN-2347] (early draft, testing the current state) Implementing Enum schema evolution protocolbuffers/protobuf#9334

Closed

antongrbin mentioned this pull request Feb 23, 2022

Conformance test: JSON parsing of unknown enum string values protocolbuffers/protobuf#9534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

json: Ignore unknown enum values #170

json: Ignore unknown enum values #170

whs commented Oct 10, 2021

timostamm commented Oct 11, 2021

whs commented Oct 11, 2021

timostamm left a comment

timostamm Oct 11, 2021

timostamm Oct 11, 2021

timostamm Oct 11, 2021

timostamm Oct 11, 2021

whs Oct 11, 2021 •

edited

Loading

timostamm Oct 11, 2021

timostamm Oct 11, 2021

whs Oct 11, 2021

timostamm Oct 11, 2021

timostamm Oct 11, 2021

timostamm commented Oct 11, 2021

whs commented Oct 11, 2021

timostamm left a comment

timostamm commented Oct 11, 2021

whs commented Oct 12, 2021

json: Ignore unknown enum values #170

json: Ignore unknown enum values #170

Conversation

whs commented Oct 10, 2021

timostamm commented Oct 11, 2021

whs commented Oct 11, 2021

timostamm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whs Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timostamm commented Oct 11, 2021

whs commented Oct 11, 2021

timostamm left a comment

Choose a reason for hiding this comment

timostamm commented Oct 11, 2021

whs commented Oct 12, 2021

whs Oct 11, 2021 •

edited

Loading