Skip to content
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.

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

Fully implement the proto3 JSON mapping spec #72

Open
14 of 19 tasks
garyp opened this issue Jul 17, 2020 · 2 comments
Open
14 of 19 tasks

Fully implement the proto3 JSON mapping spec #72

garyp opened this issue Jul 17, 2020 · 2 comments
Milestone

Comments

@garyp
Copy link
Collaborator

garyp commented Jul 17, 2020

From https://developers.google.com/protocol-buffers/docs/proto3#json.

Well-known types

  • Any (see also Add special handling for the Any protobuf type #63)
  • Duration
  • Empty
  • FieldMask
  • Struct / Value / NullValue / ListValue
  • Timestamp:
    • "9999-12-31T23:59:59.999999999Z" fails on Kotlin/JS
    • lowercase 't' and lowercase 'z' should fail on Kotlin/JVM and Kotlin/JS
    • values outside of the allowed range ("0001-01-01T00:00:00Z" to "9999-12-31T23:59:59.999999999Z") should fail on Kotlin/JVM and Kotlin/JS
  • Wrapper types

Conformance test edge cases

  • more than one value for a oneof field in a message should be rejected (currently the last value in the JSON input wins)

    Failing conformance test:

    Required.Proto2.JsonInput.OneofFieldDuplicate
    Required.Proto3.JsonInput.OneofFieldDuplicate
    
  • base64-url support for bytes fields on Kotlin/JVM and Kotlin/JS

    Failing conformance tests:

    Recommended.Proto2.JsonInput.BytesFieldBase64Url.JsonOutput
    Recommended.Proto2.JsonInput.BytesFieldBase64Url.ProtobufOutput
    Recommended.Proto3.JsonInput.BytesFieldBase64Url.JsonOutput
    Recommended.Proto3.JsonInput.BytesFieldBase64Url.ProtobufOutput
    
  • Invalid Unicode surrogate pairs in string fields should be rejected (currently they're accepted and the invalid characters are replaced with ?)

    Failing conformance tests:

    Recommended.Proto2.JsonInput.StringFieldSurrogateInWrongOrder
    Recommended.Proto2.JsonInput.StringFieldUnpairedHighSurrogate
    Recommended.Proto2.JsonInput.StringFieldUnpairedLowSurrogate
    Recommended.Proto3.JsonInput.StringFieldSurrogateInWrongOrder
    Recommended.Proto3.JsonInput.StringFieldUnpairedHighSurrogate
    Recommended.Proto3.JsonInput.StringFieldUnpairedLowSurrogate
    
  • Recently-added conformance tests that still need to be triaged for root cause:

    Recommended.Proto2.JsonInput.FieldNameExtension.Validator
    Recommended.Proto3.JsonInput.NullValueInOtherOneofNewFormat.Validator
    Recommended.Proto3.JsonInput.NullValueInOtherOneofOldFormat.Validator
    Recommended.Proto3.ValueRejectInfNumberValue.JsonOutput
    Recommended.Proto3.ValueRejectNanNumberValue.JsonOutput
    
  • exponent notation and floating-point numbers for unmarshalling numeric types: 1e5, 100000.000, 2.147483647e9, -2.147483648e9, 4.294967295e9

    Failing conformance tests:

    Required.Proto3.JsonInput.Int32FieldExponentialFormat.JsonOutput
    Required.Proto3.JsonInput.Int32FieldExponentialFormat.ProtobufOutput
    Required.Proto3.JsonInput.Int32FieldFloatTrailingZero.JsonOutput
    Required.Proto3.JsonInput.Int32FieldFloatTrailingZero.ProtobufOutput
    Required.Proto3.JsonInput.Int32FieldMaxFloatValue.JsonOutput
    Required.Proto3.JsonInput.Int32FieldMaxFloatValue.ProtobufOutput
    Required.Proto3.JsonInput.Int32FieldMinFloatValue.JsonOutput
    Required.Proto3.JsonInput.Int32FieldMinFloatValue.ProtobufOutput
    Required.Proto3.JsonInput.Uint32FieldMaxFloatValue.JsonOutput
    Required.Proto3.JsonInput.Uint32FieldMaxFloatValue.ProtobufOutput
    
  • uint32 max value (4294967295)

  • uint64 max value (18446744073709549568, "18446744073709549568")

  • floating-point numbers outside of the min/max range for float/double should be rejected (currently they're being read in as -Inf/Inf)

    Failing conformance tests:

    Required.Proto3.JsonInput.DoubleFieldTooLarge
    Required.Proto3.JsonInput.DoubleFieldTooSmall
    Required.Proto3.JsonInput.FloatFieldTooLarge
    Required.Proto3.JsonInput.FloatFieldTooSmall
    
  • unquoted NaN, Infinity, -Infinity values for float/double fields in JSON input should be rejected (currently they're accepted)

    Failing conformance tests:

    Recommended.Proto3.JsonInput.DoubleFieldInfinityNotQuoted
    Recommended.Proto3.JsonInput.DoubleFieldNanNotQuoted
    Recommended.Proto3.JsonInput.DoubleFieldNegativeInfinityNotQuoted
    Recommended.Proto3.JsonInput.FloatFieldInfinityNotQuoted
    Recommended.Proto3.JsonInput.FloatFieldNanNotQuoted
    Recommended.Proto3.JsonInput.FloatFieldNegativeInfinityNotQuoted
    
@garyp garyp added bug Something isn't working good first issue Good for newcomers labels Jul 17, 2020
@garyp garyp added this to the 0.9.0 milestone Jul 17, 2020
garyp added a commit that referenced this issue Aug 11, 2020
* Rewrote JSON support to better implement the proto3 JSON spec. This
includes support for the `json_name` protobuf option.

* Added `JsonConfig` class that can be used for configuring JSON
marshalling/unmarshalling at runtime. Currently supported configuration
includes `ignoreUnknownFieldsInInput`, `outputDefaultValues`, and
`outputProtoFieldNames`, which match the options described at
https://developers.google.com/protocol-buffers/docs/proto3#json_options.

* Got rid of the need for projects using `pbandk` to use the
`kotlinx-serialization` gradle plugin and add a dependency on the
`kotlinx-serialization` library. The library is now an internal
implementation detail of `pbandk`.

* Moved all of the binary marshalling functionality and most of the
binary unmarshalling functionality from the generated code to the
runtime library.

* Added `protoMarshal(OutputStream)`, `protoUnmarshal(InputStream)`, and
`protoUnmarshal(ByteBuffer)` overloads on the JVM.

* Fixed some bugs with handling of `packed` fields, marshalling
of enums in Javascript, and base64 encoding/decoding in Javascript.

Breaking API changes:

* Changed `Message` so that it no longer needs to extend from itself.

* Added `@PbandkInternal` and `@PublicForGeneratedCode` annotations on
relevant portions of the public API that are only public for pbandk's
internal use.

* The below methods are now defined as extension methods rather than
being part of the `Message` interface. Code that calls these methods
will now need to import them first.

    * `Message.jsonMarshal()`
    * `Message.Companion.jsonUnmarshal(String)`
    * `Message.protoMarshal()`
    * `Message.Companion.protoUnmarshal(ByteArray)`

* These two method overloads:

    * `Message.jsonMarshal(kotlinx.serialization.json.Json)`
    * `Message.Companion.jsonUnmarshal(kotlinx.serialization.json.Json, String)`

    have been replaced with:

    * `Message.jsonMarshal(pbandk.json.JsonConfig)`
    * `Message.Companion.jsonUnmarshal(String, pbandk.json.JsonConfig)`

* These two method overloads:

    * `Message.protoMarshal(pbandk.Marshaller)`
    * `Message.Companion.protoUnmarshal(pbandk.Unmarshaller)`

    have been replaced with:

    * `Message.marshal(pbandk.MessageMarshaller)`
    * `Message.Companion.unmarshal(pbandk.MessageUnmarshaller)`

* Replaced `Marshaller` and `Unmarshaller` interfaces with
`MessageMarshaller` and `MessageUnmarshaller`, which are much simpler
and function differently from the previous interfaces.

* Removed `Sizer` and `Util` from the public API.

* Removed `UnknownField` constructors and the `UnknownField.size()`
method from the public API.

* `MessageMap.entries` is now a `Set` instead of a `List`.

Fixes #23, fixes #61. Portions of the proto3 JSON spec that still need
to be implemented are tracked in #72.
garyp added a commit that referenced this issue Aug 12, 2020
* Rewrote JSON support to better implement the proto3 JSON spec. This
includes support for the `json_name` protobuf option.

* Added `JsonConfig` class that can be used for configuring JSON
marshalling/unmarshalling at runtime. Currently supported configuration
includes `ignoreUnknownFieldsInInput`, `outputDefaultValues`, and
`outputProtoFieldNames`, which match the options described at
https://developers.google.com/protocol-buffers/docs/proto3#json_options.

* Got rid of the need for projects using `pbandk` to use the
`kotlinx-serialization` gradle plugin and add a dependency on the
`kotlinx-serialization` library. The library is now an internal
implementation detail of `pbandk`.

* Moved all of the binary marshalling functionality and most of the
binary unmarshalling functionality from the generated code to the
runtime library.

* Added `protoMarshal(OutputStream)`, `protoUnmarshal(InputStream)`, and
`protoUnmarshal(ByteBuffer)` overloads on the JVM.

* Fixed some bugs with handling of `packed` fields, marshalling
of enums in Javascript, and base64 encoding/decoding in Javascript.

Breaking API changes:

* Changed `Message` so that it no longer needs to extend from itself.

* Added `@PbandkInternal` and `@PublicForGeneratedCode` annotations on
relevant portions of the public API that are only public for pbandk's
internal use.

* The below methods are now defined as extension methods rather than
being part of the `Message` interface. Code that calls these methods
will now need to import them first.

    * `Message.jsonMarshal()`
    * `Message.Companion.jsonUnmarshal(String)`
    * `Message.protoMarshal()`
    * `Message.Companion.protoUnmarshal(ByteArray)`

* These two method overloads:

    * `Message.jsonMarshal(kotlinx.serialization.json.Json)`
    * `Message.Companion.jsonUnmarshal(kotlinx.serialization.json.Json, String)`

    have been replaced with:

    * `Message.jsonMarshal(pbandk.json.JsonConfig)`
    * `Message.Companion.jsonUnmarshal(String, pbandk.json.JsonConfig)`

* These two method overloads:

    * `Message.protoMarshal(pbandk.Marshaller)`
    * `Message.Companion.protoUnmarshal(pbandk.Unmarshaller)`

    have been replaced with:

    * `Message.marshal(pbandk.MessageMarshaller)`
    * `Message.Companion.unmarshal(pbandk.MessageUnmarshaller)`

* Replaced `Marshaller` and `Unmarshaller` interfaces with
`MessageMarshaller` and `MessageUnmarshaller`, which are much simpler
and function differently from the previous interfaces.

* Removed `Sizer` and `Util` from the public API.

* Removed `UnknownField` constructors and the `UnknownField.size()`
method from the public API.

* `MessageMap.entries` is now a `Set` instead of a `List`.

Fixes #23, fixes #61. Portions of the proto3 JSON spec that still need
to be implemented are tracked in #72.
garyp added a commit that referenced this issue Aug 13, 2020
We accidentally forgot to re-enable these in
`pbandk.conformance.doTestIo()` after we added full JSON support to
pbandk.

Also enable the "recommended" tests since we're already passing most of
the ones that aren't related to JSON.

After enabling the tests, fixed a few bugs in the JSON code that the
tests exposed. The remaining failing tests were added to the
`failing_tests.txt` exception list and also to the checklist in
#72.
garyp added a commit that referenced this issue Aug 13, 2020
We accidentally forgot to re-enable these in
`pbandk.conformance.doTestIo()` after we added full JSON support to
pbandk.

Also enable the "recommended" tests since we're already passing most of
the ones that aren't related to JSON.

After enabling the tests, fixed a few bugs in the JSON code that the
tests exposed. The remaining failing tests were added to the
`failing_tests.txt` exception list and also to the checklist in
#72.
@garyp garyp modified the milestones: 0.9.0, 0.10.0 Sep 21, 2020
@garyp garyp modified the milestones: 0.10.0, 0.11.0 Mar 12, 2021
@garyp garyp modified the milestones: 0.11.0, 0.12.0 Sep 24, 2021
@garyp garyp modified the milestones: 0.12.0, 1.0 Oct 29, 2021
@kolloch
Copy link

kolloch commented Nov 9, 2022

Parsers accept both the lowerCamelCase name (or the one specified by the json_name option) and the original proto field name. null is an accepted value for all field types and treated as the default value of the corresponding field type.

https://developers.google.com/protocol-buffers/docs/proto3#json

This is not implemented yet. The behavior is dependent on the outputProtoFieldNames option which it shouldn't. It should always accept both.

This is important so that disregarding of the options of the writer/reader, they stay compatible.

@garyp
Copy link
Collaborator Author

garyp commented Nov 11, 2022

@kolloch that quote is how the pbandk JSON parser is already implemented. The relevant code is here:

private val FieldDescriptor<*, *>.jsonNames: List<String>

If you've found a case where pbandk is not doing the correct thing, can you please open a new issue with more details about how to reproduce the problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants