Closed sduchesneau closed 11 months ago
@sduchesneau, I'm afraid I can't accept such a patch, for a couple of reasons:
The JSON format for Protobuf has a spec. So doing things outside the spec will lead to interop problems and is not really a good idea. That link is not as thorough as the conformance tests, which include tests for the JSON format and indicate expected behavior for all kinds of corner cases.
Full disclosure: I've not run the conformance tests against this implementation; I'm sure there are some edge cases where my implementation is incorrect. But I've tried to implement this per the spec, and adding an option for a format that cannot interop with any other implementation doesn't seem valuable to me. Sorry.
Per the note at the top of this repo's main README, v1.15.1 is the last expected v1 version of this repo. And a v2 of this repo will not have a dynamic
package. That's because the v2 API of the Protobuf Go runtime (released a few years after I wrote this stuff) now includes its own version of that functionality: google.golang.org/protobuf/types/dynamicpb
.
So I would recommend using that dynamicpb
package linked above for dynamic messaging, instead of the dynamic
package in this repo.
If you really need a custom JSON format, I would implement it in a separate package. Marshalling should be pretty straight-forward to implement on top of the Protobuf Go runtime's protoreflect
features. Unmarshaling is always harder than marshalling, though the Go standard library includes a decent decoder library. If you were to fork existing code to implement this, you might consider forking the implementation in protojson
instead of the implementation here.
Feature request:
We would like to specify how Bytes (
reflect.Slice
) fields will be decoded. By default, base64 is used, but for some use cases, decoding dynamic protobuf messages with byte fields as hexadecimal representation can be powerful for readability.This would be a nice option inside
jsonpb.Marshaler
, but I'm wondering if there is another way to do this within this package ?We hacked our way using a global "default" variable, but there must be a better way to do this..
https://github.com/streamingfast/protoreflect/commit/9691b6ec99b1cea2f0baca63f8adf997751eab30