gogo / protobuf

[Deprecated] Protocol Buffers for Go with Gadgets
Other
5.67k stars 807 forks source link

NULL deserialization of Oneof when using GRPC #238

Closed buckhx closed 7 years ago

buckhx commented 7 years ago

A message with a oneof is not properly serializing using GRPC.

Here's an example of the a json-encoded message payload before it is written and then after is is read. I encountered this having a client send a message to a GRPC method and the oneof be null on the server.

WRITE: {
    "label": "test",
    "contact": {
        "email": "test@email.net"
    },
    "created": {
        "seconds": 1481225819,
        "nanos": 422943804
    },
    "Entity": {
        "Person": {
            "name": {
                "first": "test",
                "last": "user"
            }
        }
    }
}

READ: {
    "label": "test",
    "contact": {
        "email": "test@email.net"
    },
    "created": {
        "seconds": 1481225819,
        "nanos": 422943804
    },
    "Entity": null
}

I'm sure this could be replicated w/out GRPC for easy debugging.

Switching to golang/protobuf has proper oneof serialization

awalterschulze commented 7 years ago

It would be great if you could reproduce this without grpc?

buckhx commented 7 years ago

My initial tests seem to work fine. I'll add in pieces that are being used outside of GRPC (WKT, annotations..) to a test repo to reflect the production case I ran into as I go and reach out when I find something. Haven't looked into grpc's proto marshaling, but I'm assuming the use "github.com/golang/protobuf/proto".

Here's a repo w/ the tests https://github.com/buckhx/gogo-test

awalterschulze commented 7 years ago

wow nice effort. Thank you. Give me a few minutes.

awalterschulze commented 7 years ago

I just cloned it and ran the test and it is passing, is this intended?

awalterschulze commented 7 years ago

Also note I just merge alot of updates from golang/protobuf

buckhx commented 7 years ago

Yeah those tests passed, but it's not an exact replica of my environment in the project that is getting the nulls so I'll add in a few more things and see if they break these tests.

Also I'll try your updates and see if I'm still getting nulls in my other project.

The only funky thing I noticed in these tests is that the json serialization is CamelCase on the oneofs and lower_snake on the other fields.

On Sat, Dec 10, 2016, 1:26 PM Walter Schulze notifications@github.com wrote:

Also note I just merge alot of updates from golang/protobuf

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gogo/protobuf/issues/238#issuecomment-266227613, or mute the thread https://github.com/notifications/unsubscribe-auth/AATNZx_K41kGZUgtw12Fjn32wH_S-NE1ks5rGu7GgaJpZM4LINox .

awalterschulze commented 7 years ago

Ok wait I think I am seeing a problem in your code. You should use the jsonpb package to serialize your json.

awalterschulze commented 7 years ago

Also this might be interesting for you, since you are using grpc and json https://github.com/gogo/protobuf/issues/178

buckhx commented 7 years ago

Good call on the jsonpb. I was just using it for debugging, but will be using grpc-gateway so that's a good data point. I pulled out the json parts to avoid an red herrings.

The tests now break in https://github.com/buckhx/gogo-test w/ what I am seeing in my original grpc project.

Using "github.com/golang/protobuf/proto" w/ a gogo proto that includes a oneof seems to be the issue. Not sure if there's a mechanism that can be hooked into to pick the serializer for grpc as was suggested in the jsonpb thread or not.

Here's the test log using "github.com/golang/protobuf/proto"

--- FAIL: TestGogoRoundTrip (0.00s)
    serialize_test.go:34: label:"ash" contact:<email:"ash@pallet.gov" > person:<name:<first:"ash" last:"ketchum" > > 
    serialize_test.go:43: label:"ash" contact:<email:"ash@pallet.gov" > 
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x6f8ff]

goroutine 17 [running]:
panic(0x147920, 0xc42000a0b0)
    /Users/buck/.gvm/gos/go1.7.3/src/runtime/panic.go:500 +0x1a1
testing.tRunner.func1(0xc4200a0180)
    /Users/buck/.gvm/gos/go1.7.3/src/testing/testing.go:579 +0x25d
panic(0x147920, 0xc42000a0b0)
    /Users/buck/.gvm/gos/go1.7.3/src/runtime/panic.go:458 +0x243
github.com/buckhx/gogo-test_test.TestGogoRoundTrip(0xc4200a0180)
    /Users/buck/.gvm/pkgsets/go1.7.3/global/src/github.com/buckhx/gogo-test/serialize_test.go:49 +0x6cf
testing.tRunner(0xc4200a0180, 0x182380)
    /Users/buck/.gvm/gos/go1.7.3/src/testing/testing.go:610 +0x81
created by testing.(*T).Run
    /Users/buck/.gvm/gos/go1.7.3/src/testing/testing.go:646 +0x2ec
FAIL    github.com/buckhx/gogo-test 0.008s
?       github.com/buckhx/gogo-test/gen/gogo    [no test files]
?       github.com/buckhx/gogo-test/gen/golang  [no test files]
?       github.com/buckhx/gogo-test/vendor/github.com/gogo/protobuf/proto   [no test files]
?       github.com/buckhx/gogo-test/vendor/github.com/golang/protobuf/proto [no test files]
awalterschulze commented 7 years ago

grpc has a WithCodec method that might be useful when used with this https://github.com/gogo/protobuf/blob/master/codec/codec.go Although I have been told its not thread safe https://github.com/gogo/protobuf/issues/205

You are not supposed to serialize a gogoprotobuf generated proto message with golang/protobuf. You should always use the appropriate library.

Except when using gogoproto_import=false, because then the generated code will still using the golang/protobuf library. This is what gofast_out uses internally.

awalterschulze commented 7 years ago

I assume my answer fixed this issue, otherwise please reopen