Skip to content
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

Support oneof types of fields #82

Closed
yugui opened this issue Jan 5, 2016 · 13 comments
Closed

Support oneof types of fields #82

yugui opened this issue Jan 5, 2016 · 13 comments

Comments

@yugui
Copy link
Member

yugui commented Jan 5, 2016

Extracted from #5.

Currently oneof fields are not supported. They should be allowed in request body and query string as well as other types.

  • This feature requires allow use of jsonpb for marshaling #79 because "encoding/json".Marshal returns error for oneof fields
  • No plan to allow them in path parameters because path parameters are "mandatory" parameters. So they don't fit to oneof very well.
@johansja
Copy link
Contributor

Use of https://godoc.org/github.com/golang/protobuf/jsonpb instead of encoding/json can help.

@yugui
Copy link
Member Author

yugui commented Jan 12, 2016

@johansja Thank you. Yes, it is #79.

@gyuho
Copy link

gyuho commented Jul 6, 2016

@yugui etcd is hitting this issue, though it's happening only in experimental feature.

Do we have any workaround for this? Or ETA to support this?

Thanks!

@johansja
Copy link
Contributor

johansja commented Jul 7, 2016

You should be able to use one_of with the help of runtime.WithMarshalerOption.

@yugui
Copy link
Member Author

yugui commented Jul 7, 2016

Now the default marshaller uses 'jsonpb'. So I think it is supported.

@gyuho Could you re-confirm with the HEAD of the master branch?
Let me know if you still have the issue.
I'll close this issue if otherwise.

@gyuho
Copy link

gyuho commented Jul 7, 2016

@yugui First thanks for helping debug this.

etcd still complains {"Error":"json: cannot unmarshal object into Go value of type etcdserverpb.isWatchRequest_RequestUnion","Code":2}. And tried the latest master branch of grpc-gateway and golang/protobuf, and regenerate the gateway files, as here etcd-io/etcd@8759230.

I added reflect.TypeOf(marshaler) to this line https://github.com/gyuho/etcd/blob/3108859828f851e9a46772fe8e16d257b40f3639/etcdserver/etcdserverpb/rpc.pb.gw.go#L91 and confirm that *runtime.JSONPb typed marshaler is being used, but still seeting that error.

*runtime.JSONPb
2016-07-06 22:07:13.861689 I | v3rpc/grpc: Failed to decode request: json: cannot unmarshal object into Go value of type etcdserverpb.isWatchRequest_RequestUnion

Maybe we generate gateway files in the wrong way? I tried to regenerate *.gw.go files but don't see any diff.

Thanks.

@yugui
Copy link
Member Author

yugui commented Jul 7, 2016

@gyuho I see. I'll take a look.

@yugui
Copy link
Member Author

yugui commented Jul 11, 2016

@gyuho
Does this work fine?

curl -L http://localhost:2379/v3alpha/watch -X POST -d '{"create_request": {"key": "Zm9v"}}'

You need to specify literally one of the fields grouped by request_union.

@gyuho
Copy link

gyuho commented Jul 11, 2016

@yugui Still doesn't seem to work.

curl -L http://localhost:2379/v3alpha/watch -X POST -d '{"create_request": {"key": "Zm9v"}}'

{"Error":"unknown field \"create_request\" in etcdserverpb.WatchRequest","Code":2}

Thanks!

@yugui
Copy link
Member Author

yugui commented Jul 12, 2016

@gyuho
It does not look to be an issue of grpc-gateway. I could reproduce the issue without grpc-gateway.

package main

import (
        "log"

        "github.com/coreos/etcd/etcdserver/etcdserverpb"
        "github.com/golang/protobuf/jsonpb"
)

func main() {
        const input = `{"create_request": {"key": "Zm9v"}}`
        var protoReq etcdserverpb.WatchRequest
        if err := jsonpb.UnmarshalString(input, &protoReq); err != nil {
                log.Fatal(err)
        }
}
$ go run main.go
2016/07/12 12:15:28 unknown field "create_request" in etcdserverpb.WatchRequest
exit status 1

I don't know what the root cause actually is, but I suspect incompatibility between github.com/golang/protobuf/jsonpb and gogoproto.

@yugui
Copy link
Member Author

yugui commented Jul 12, 2016

@gyuho Confirmed in https://gist.github.com/yugui/4425e705680fb4f6c99423b99b8a83d2

$ go get gist.github.com/4425e705680fb4f6c99423b99b8a83d2.git
$ go build -o test gist.github.com/4425e705680fb4f6c99423b99b8a83d2.git
$ ./test
$ echo $?
0
$ go build --tags=gogo -o test gist.github.com/4425e705680fb4f6c99423b99b8a83d2.git
$ ./test
2016/07/12 12:53:05 unknown field "create_request" in main.WatchRequest
$ echo $?
1

@yugui yugui closed this as completed Jul 12, 2016
@gyuho
Copy link

gyuho commented Jul 12, 2016

@yugui I will see if I can find some workarounds or file an issue to gogoproto. Thanks a lot!

@yugui
Copy link
Member Author

yugui commented Jul 12, 2016

@gyuho
I am not yet confident but I suspect if this proto in (*WatchRequest).XXX_OneofFuncs,

func (*WatchRequest) XXX_OneofFuncs() (func(msg proto.Message, b *proto.Buffer) error, func(msg proto.Message, tag, wire int, b *proto.Buffer) (bool, error), func(msg proto.Message) (n int), []interface{}) {

at https://github.com/gyuho/etcd/blob/3108859828f851e9a46772fe8e16d257b40f3639/etcdserver/etcdserverpb/rpc.pb.go#L1084
should have been github.com/golang/protobuf/proto but not github.com/gogo/protobuf/proto so that it is compatible to https://github.com/golang/protobuf/blob/ba6f978a1a6606adf3ccb6987f15c64262bfdbc2/proto/properties.go#L729.

nilium added a commit to nilium/grpc-gateway that referenced this issue Mar 3, 2017
This modifies fieldByProtoName to do two things:

1. It may now return an error. It only does this for the oneof case
   right now, as the not-found case only causes the field to be
   skipped.

2. It will first look for the field name in the message's OneofTypes
   map. If the field matches a OneOf type, it will check first that the
   field is nil -- if it isn't, the field is already set and it returns
   an error. If nil, it allocates a new oneof type for the field and
   returns the first field of the oneof type.

The reason for rejecting multiple oneof fields is that the runtime
iterates over url.Values and gets pseudo-random key order, meaning that
one field is selected out of several. We could address this by sorting
the url.Values and picking the last seen, in which case the code is far
simpler but involves allocating and sorting an array of field names
before walking them (only to get predictable field selection).

This change amends the error result tested for in
tTestPopulateParameters to include the name of oneof_value.

Related to issue grpc-ecosystem#82.
nilium added a commit to nilium/grpc-gateway that referenced this issue Mar 3, 2017
This modifies fieldByProtoName to do two things:

1. It may now return an error. It only does this for the oneof case
   right now, as the not-found case only causes the field to be
   skipped.

2. It will first look for the field name in the message's OneofTypes
   map. If the field matches a OneOf type, it will check first that the
   field is nil -- if it isn't, the field is already set and it returns
   an error. If nil, it allocates a new oneof type for the field and
   returns the first field of the oneof type.

The reason for rejecting multiple oneof fields is that the runtime
iterates over url.Values and gets pseudo-random key order, meaning that
one field is selected out of several. We could address this by sorting
the url.Values and picking the last seen, in which case the code is far
simpler but involves allocating and sorting an array of field names
before walking them (only to get predictable field selection).

This change amends the error result tested for in
TestPopulateParameters to include the name of oneof_value.

Related to issue grpc-ecosystem#82.

Change-Id: I7a5ecc9ce397bd71156cd4ac8690bae782ecc55e
tmc pushed a commit that referenced this issue Apr 4, 2017
This modifies fieldByProtoName to do two things:

1. It may now return an error. It only does this for the oneof case
   right now, as the not-found case only causes the field to be
   skipped.

2. It will first look for the field name in the message's OneofTypes
   map. If the field matches a OneOf type, it will check first that the
   field is nil -- if it isn't, the field is already set and it returns
   an error. If nil, it allocates a new oneof type for the field and
   returns the first field of the oneof type.

The reason for rejecting multiple oneof fields is that the runtime
iterates over url.Values and gets pseudo-random key order, meaning that
one field is selected out of several. We could address this by sorting
the url.Values and picking the last seen, in which case the code is far
simpler but involves allocating and sorting an array of field names
before walking them (only to get predictable field selection).

This change amends the error result tested for in
TestPopulateParameters to include the name of oneof_value.

Related to issue #82.

Change-Id: I7a5ecc9ce397bd71156cd4ac8690bae782ecc55e
adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
This modifies fieldByProtoName to do two things:

1. It may now return an error. It only does this for the oneof case
   right now, as the not-found case only causes the field to be
   skipped.

2. It will first look for the field name in the message's OneofTypes
   map. If the field matches a OneOf type, it will check first that the
   field is nil -- if it isn't, the field is already set and it returns
   an error. If nil, it allocates a new oneof type for the field and
   returns the first field of the oneof type.

The reason for rejecting multiple oneof fields is that the runtime
iterates over url.Values and gets pseudo-random key order, meaning that
one field is selected out of several. We could address this by sorting
the url.Values and picking the last seen, in which case the code is far
simpler but involves allocating and sorting an array of field names
before walking them (only to get predictable field selection).

This change amends the error result tested for in
TestPopulateParameters to include the name of oneof_value.

Related to issue grpc-ecosystem#82.

Change-Id: I7a5ecc9ce397bd71156cd4ac8690bae782ecc55e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants