google-cloud-rust icon indicating copy to clipboard operation
google-cloud-rust copied to clipboard

Detect if two variants of a `oneof` field are set in JSON inputs

Open coryan opened this issue 7 months ago • 1 comments

When deserializing a oneof from JSON we need to detect if there are multiple variants set. Ignoring this could lead to silent data loss, or at least confusing behavior.

  • [ ] Required.Proto3.JsonInput.OneofFieldDuplicate # Should have failed to parse, but didn't.
  • [ ] Required.Proto3.JsonInput.OneofFieldNullFirst.JsonOutput # JSON output we received from test was unparseable.

ERROR, test=Required.Proto3.JsonInput.OneofFieldDuplicate: Should have failed to parse, but didn't., request=goo.gle/debugonly   json_payload: "{\"oneofUint32\": 1, \"oneofString\": \"test\"}" requested_output_format: JSON message_type: "protobuf_test_messages.proto3.TestAllTypesProto3" test_category: JSON_TEST, response=goo.gle/debugonly   json_payload: "{\"oneofUint32\":1,\"oneofString\":\"test\"}"
ERROR, test=Required.Proto3.JsonInput.OneofFieldNullFirst.JsonOutput: JSON output we received from test was unparseable., request=goo.gle/debugonly   json_payload: "{\"oneofUint32\": null, \"oneofString\": \"test\"}" requested_output_format: JSON message_type: "protobuf_test_messages.proto3.TestAllTypesProto3" test_category: JSON_TEST, response=goo.gle/debugonly   json_payload: "{\"oneofUint32\":0,\"oneofString\":\"test\"}"

coryan avatar Jun 03 '25 22:06 coryan

The fixes for this are going to take multiple steps, which I think deserve at least an outline.

  1. I have improved testing to include messages with all the primitive types as required, optional, repeated, and map fields.
  2. I have improved testing to include messages with enum types as required, optional, repeated, and map fields.
  3. I have improved testing to include message types as optional (by default and explicitly), repeated, and map fields.
  4. I have improved testing to include a well-known message type as optional (by default and explicitly), repeated, and map fields that serializes to strings.
  5. I have improved testing to include the google.protobuf.Value type as optional (by default and explicitly), repeated, and map fields.
  6. For all these messages, I will change the generator to emit a helper type with hand-crafted serialization
  7. Initially only the body (no fields) will be serialized.
  8. I will slowly add different field types to this serialization, including unit tests.
  9. I will start with fields that do not require serde_with and serde_as() mapping.
  10. Once all the fields are working, I will switch the generator and tests to use the new serialization in the normal messages.
  11. We should be able to remove a lot of serde annotations.
  12. We should also be able to cleanup generator code and tests after these changes.

coryan avatar Jun 10 '25 03:06 coryan