-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Update jsonpb to handle Map, Slice, Ptr nil values for EmitDefaults #338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed review. I just got assigned to start looking at these. Changes look good. Please to sync to head first and I'll approve.
Thanks.
@cybrcodr Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@cybrcodr @ewang this commit seems to have the unfortunate effect of syntax = "proto2";
package cockroach.build;
option go_package = "build";
import "gogoproto/gogo.proto";
message Info {
optional string go_version = 1 [(gogoproto.nullable) = false];
optional string tag = 2 [(gogoproto.nullable) = false];
optional string time = 3 [(gogoproto.nullable) = false];
optional string revision = 4 [(gogoproto.nullable) = false];
optional string cgo_compiler = 5 [(gogoproto.nullable) = false];
optional string platform = 6 [(gogoproto.nullable) = false];
optional string distribution = 7 [(gogoproto.nullable) = false];
optional string type = 8 [(gogoproto.nullable) = false];
optional string dependencies = 10000;
} Here's my test case: func TestProtobuf(t *testing.T) {
var b bytes.Buffer
var in build.Info
if err := (&jsonpb.Marshaler{EmitDefaults: true}).Marshal(&b, &in); err != nil {
t.Fatal(err)
}
var out build.Info
if err := jsonpb.Unmarshal(&b, &out); err != nil {
t.Fatal(err)
}
if out != in {
t.Error(pretty.Diff(out, in))
}
} and it fails with
|
Can you try out head version? In #394, deserializing "null" for all message types except wkt ValueType should translate to nil value. I also tried it out with a simplified example from yours.
Output...
Let me know if head doesn't work out for you. |
Thanks! We're using gogo/protobuf, so I've filed gogo/protobuf#319. I'll give it a shot once #394 is brought over. |
My attempt at making the Map, Slice, nil Ptr type marshaling match the proto3 JSON spec.
Fixes: #323
Related PRs:
#232
#287