jsonpb: Adds support for other []byte types to be base64 encoded. #188
Conversation
|
Yes it doesn't seem like this code will break any previous assumptions it just allows a very cases. |
| @@ -282,6 +282,18 @@ func init() { | |||
| } | |||
| } | |||
|
|
|||
| // Bytes is used to test that []byte type aliases are serialized to base64. | |||
| type Bytes []byte | |||
There was a problem hiding this comment.
this test can be made slightly more complete with:
// Byte is used to test that []byte type aliases are base64-encoded.
type Byte byte
// Bytes is used to test that []byte type aliases are base64-encoded.
type Bytes []Byte
The proto3 spec encodes the bytes type to be a base64 encoded string when marshalling to jsonpb. This adds support to jsonpb to correctly encode go types in the format `type Bytes []byte` into base64 instead of being represented as an uint8 array. While these types won't be generated by the standard protobuf compiler, they can be generated by others such as gogo protobuf.
|
Any update on this? This is blocking cockroachdb/cockroach#6973. |
|
Hi! I'm unfamiliar with gogoprotobuf and the bug this is blocking. Could you help fill us in on some of the details? I wasn't able to figure out how cockroachdb/cockroach#6973 uses this change. Could you provide some more information? Is there a way to unblock that change without making this change? It would also be really helpful to get some more context on gogoprotobuf. When do the other []byte types get generated? If they're only generated by gogoprotobuf, could this change be made the gogoprotobuf fork directly? Thanks, |
That change wants to use jsonpb marshalling of messages which include aliases of
No. This change has to be made - either here or in gogoprotobuf.
They are generated when a field is tagged with the
Yes, this PR has been submitted to gogoprotobuf as well, but @awalterschulze would like to minimize drift. I think this change is fine here -- it's subtle, but the proposed code is more correct. |
|
Thanks! I think we have a better understanding of the issue here. Please correct me if we got anything wrong. If possible, it would be ideal for the change made in gogoprotobuf since the other []byte fields are generated by the gogoprotobuf code generator. We'd prefer to avoid adding a test case on a generated field type that the proto package doesn't produce. Michael |
|
I agree regarding the tests, but I don't know that there's any downside in accepting the slightly more correct reflection code. The upside is that it reduces the burden of maintainership downstream. |
|
We understand the desire to reduce the burden of maintainership downstream, but doing so would increase the burden of maintainership upstream. The test in this change enforces a new invariant in how the proto package handles generated structs. The upstream protobuf library would then implicitly support the 'gogoproto.casttype' option in the 'downstream' gogoprotobuf library. |
|
Right, I was suggesting that the tests be backed out.
|
|
Ah, sorry -- I misunderstood that! In that case I don't see any issues in making this change. Let me double check, though. I'l get back to this thread tomorrow. |
|
@awalterschulze Any ETA on when 3b06fc7 will get merged into gogo/protobuf? |
|
I don't think this is the place. Lets discuss it here gogo/protobuf#183 |
This is meant to fix golang/protobuf#188. There are no tests because we don't guarantee that we're going to maintain this behavior in the future. Fixes #188
The proto3 spec encodes the bytes type to be a base64 encoded string
when marshalling to jsonpb. This adds support to jsonpb to correctly
encode go types in the format
type Bytes []byteinto base64 instead ofbeing represented as an uint8 array.
While these types won't be generated by the standard protobuf compiler,
they can be generated by others such as gogo protobuf.