Improve JSON support; move most ser/deser from generated code into runtime library #69

garyp · 2020-06-30T12:25:57Z

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.

seanadkinson

Very cool... excited to upgrade to this!

runtime/src/commonMain/kotlin/pbandk/json/JsonWireMarshaller.kt

runtime/src/commonMain/kotlin/pbandk/json/JsonWireUnmarshaller.kt

runtime/src/commonMain/kotlin/pbandk/json/MessageExtensions.kt

garyp · 2020-07-01T01:04:12Z

A couple things I meant to mention:

This PR is still creating an unmarshal() method in each generated class that has to know about the structure of the message. That's temporary. Once we add support for message builders, we can create a generic version of unmarshal() that's driven entirely off of the Message.fieldDescriptors list, the same way that this PR is doing for marshalling. I'm pretty excited about that because it'll really slim down the generated classes without a big runtime performance impact.
This PR only converts the jsonMarshal() and jsonUnmarshal() methods over to these new WireMarshaller and WireUnmarshaller interfaces. My intent is to eventually rewrite the current protoMarshal() and protoUnmarshal() implementations to use these new interfaces as well. That will centralize all knowledge about (de)ser into the runtime library and make the generated code completely unaware about the serialization format.

garyp · 2020-07-07T17:54:48Z

@seanadkinson This is finally ready for review. One small fix led to another and I ended up converting all of the binary marshall/unmarshall code as well 😁 The new code happens to fix some additional conformance tests we were previously failing. I'll create GitHub issues for the remaining gaps in our JSON support.

dustinbrown

Quite the change set, looks good to me.

seanadkinson

Couple questions, but overall looks really clean. Love all the failed tests we get to remove 👍

protoc-gen-kotlin/lib/src/commonMain/kotlin/pbandk/gen/CodeGenerator.kt

seanadkinson · 2020-07-31T17:34:28Z

runtime/src/commonMain/kotlin/pbandk/internal/binary/BinaryMessageMarshaller.kt

+        sizeFn: (T) -> Int
+    ) {
+        val valueType = type.messageCompanion.fieldDescriptors[0].type
+        if (valueType.isDefaultValue(value)) {


Theres another default value check up on line 14. Is one of these not needed?

They're slightly different for wrapper types. Using the StringValue wrapper type as an example:

The call on line 14 is doing !fd.type.hasPresence && fd.type.isDefaultValue(value). StringValue is a message type and message types always "have presence" (i.e. they're nullable), so the fd.type.isDefaultValue(value) check won't even be called on line 14.

Here on line 62, we've looked up the underlying type of the StringValue wrapper: String. We're now checking if value is the default value of String (not StringValue). If it is, we'll omit writing the empty string on the wire. But we're still writing the StringValue message header so the receiver knows that the value was non-null.

runtime/src/commonMain/kotlin/pbandk/internal/json/JsonMessageMarshaller.kt

seanadkinson · 2020-07-31T17:41:13Z

runtime/src/jsMain/kotlin/pbandk/internal/binary/ProtobufjsBinaryWireMarshaller.kt

+import pbandk.protobufjs.Writer
+import pbandk.protobufjs.protobufjsLong
+
+private fun Writer.writeValueNoTag(type: FieldDescriptor.Type, value: Any) {


Is this code unable to use the JsonValueMarshaller and related classes from commonMain? I guess I'm unclear about the relation of this class (and CodedStreamBinaryWireMarshaller below) to the ones in commonMain.

I think you're confusing the JSON marshallers and the "binary" marshallers. ProtobufjsBinaryWireMarshaller and CodedStreamBinaryWireMarshaller are both responsible for converting a field into protobuf binary format. Whereas JsonValueMarshaller is responsible for converting a field into protobuf JSON format.

Yes, sorry, I understand the different between the JSON and the Binary - bad example specifying JsonValueMarshaller in my comment.

The point I was trying to make was that it seemed like there was a lot of duplicate work being done in this jsMain module, that is already done in commonMain, but looking closer, this probably isn't the case. I guess I just saw enough when statements that rolled through each type, either reading or writing the value, and I thought we wouldn't need to do that outside of commonMain. But if I understand correctly, this code is just providing the javascript bindings of doing the actual reading/writing, and code up in commonMain is using this marshaller for doing the work in javascript. Is my understanding correct?

garyp · 2020-08-11T23:57:09Z

Now that the Kotlin/Native PR finally got merged, I've rebased on top of that one. I've also added a detailed description to this PR with all of the changes being made. I'll merge this as soon as the tests pass.

* 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 requested a review from seanadkinson June 30, 2020 12:25

garyp force-pushed the garyp/json-refactor branch from 468ce35 to 6f68bea Compare June 30, 2020 21:42

seanadkinson approved these changes Jun 30, 2020

View reviewed changes

garyp force-pushed the garyp/json-refactor branch from 6f68bea to eb5e3cf Compare July 1, 2020 08:07

garyp changed the title ~~WIP: Refactor and improve JSON support~~ Improve JSON support; move most ser/deser from generated code into runtime library Jul 7, 2020

garyp marked this pull request as ready for review July 7, 2020 17:51

garyp requested a review from seanadkinson July 7, 2020 17:54

garyp mentioned this pull request Jul 13, 2020

Why 'unknownFields' and 'protoSize$delegate' sent on request message from kotlin app? #70

Closed

garyp linked an issue Jul 15, 2020 that may be closed by this pull request

Failing ValidDataRepeated.*.PackedInput conformance tests #23

Closed

garyp requested a review from dustinbrown July 17, 2020 21:53

dustinbrown approved these changes Jul 22, 2020

View reviewed changes

seanadkinson approved these changes Jul 31, 2020

View reviewed changes

garyp mentioned this pull request Jul 31, 2020

Fix protoc tasks to use a consistent version of the protoc binary #74

Merged

garyp force-pushed the garyp/json-refactor branch 2 times, most recently from 0d4c15e to e98834e Compare August 11, 2020 23:54

garyp force-pushed the garyp/json-refactor branch from e98834e to 1c88951 Compare August 12, 2020 00:59

garyp merged commit 7c3d970 into master Aug 12, 2020

garyp deleted the garyp/json-refactor branch August 12, 2020 01:09

Improve JSON support; move most ser/deser from generated code into runtime library #69

Improve JSON support; move most ser/deser from generated code into runtime library #69

Uh oh!

Conversation

garyp commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanadkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garyp commented Jul 1, 2020

Uh oh!

garyp commented Jul 7, 2020

Uh oh!

dustinbrown left a comment

Choose a reason for hiding this comment

Uh oh!

seanadkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seanadkinson Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

garyp Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seanadkinson Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

garyp Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

seanadkinson Aug 1, 2020

Choose a reason for hiding this comment

Uh oh!

garyp commented Aug 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

garyp commented Jun 30, 2020 •

edited

Loading