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 for path collapsing for embedded structs? #187

Closed
dmitshur opened this issue Jul 1, 2016 · 6 comments
Closed

Support for path collapsing for embedded structs? #187

dmitshur opened this issue Jul 1, 2016 · 6 comments

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Jul 1, 2016

This is a question or potential feature request. Let me describe the situation.

Suppose you have a gRPC method that accepts a ExampleOptions type as the parameter:

type ExampleOptions struct {
    Foo         bool   `protobuf:"varint,4,opt,name=Foo,proto3" json:"Foo,omitempty"`
    Bar         string `protobuf:"bytes,5,opt,name=Bar,proto3" json:"Bar,omitempty"`
    Baz         string `protobuf:"bytes,6,opt,name=Baz,proto3" json:"Baz,omitempty"`
    ListOptions `protobuf:"bytes,3,opt,name=ListOptions,embedded=ListOptions" json:""`
}

// ListOptions specifies general pagination options for fetching a list of results.
type ListOptions struct {
    PerPage int32 `protobuf:"varint,1,opt,name=PerPage,proto3" json:"PerPage,omitempty" url:",omitempty"`
    Page    int32 `protobuf:"varint,2,opt,name=Page,proto3" json:"Page,omitempty" url:",omitempty"`
}

(Notice that ListOptions is embedded into ExampleOptions.)

grpc-protobuf supports method parameters in query string, so it's possible to define this in a .proto file:

rpc Example(ExampleOptions) returns (ExampleResult) {
    option (google.api.http) = {
        get: "/example/{Bar}"
    };
}

And then do an HTTP GET request to:

http://localhost:8080/api/example/bar?Foo=true&ListOptions.Page=2

But I'm wondering if it'd be possible to do this instead:

http://localhost:8080/api/example/bar?Foo=true&Page=2

Just like in Go, when a struct is embedded, you can access it via the short form of opt.Page instead of opt.ListOptions.Page. Would the same be possible for grpc-gateway parameter names?

@dmitshur
Copy link
Contributor Author

dmitshur commented Jul 1, 2016

After looking at this more, I suspect it may be not possible because we've used a custom gogoproto option (gogoproto.embed) = true to get ListOptions embedded into ExampleOptions:

message ExampleOptions {
    bool Foo = 4;
    string Bar = 5;
    string Baz = 6;
    ListOptions ListOptions = 3 [(gogoproto.nullable) = false, (gogoproto.embed) = true, (gogoproto.jsontag) = ""];
}

// ListOptions specifies general pagination options for fetching a list of results.
message ListOptions {
    int32 PerPage = 1 [(gogoproto.moretags) = "url:\",omitempty\""];
    int32 Page = 2 [(gogoproto.moretags) = "url:\",omitempty\""];
}

But curious to hear your thoughts on this.

@yugui
Copy link
Member

yugui commented Jul 1, 2016

I am trying to keep grpc-gateway as consistent to Google's internal similar solution as possible.
So I am a bit worrying about if it breaks compatibility to the spec described in google/api/http.proto.

@dmitshur
Copy link
Contributor Author

dmitshur commented Jul 1, 2016

I am in agreement and not breaking that spec is important to me too.

I am primarily asking if there's a spec-complaint way to achieve this ability to shorten some paths. It's reasonable if that turns out to be out of scope and not possible.

@yugui
Copy link
Member

yugui commented Jul 11, 2016

@shurcooL Sorry for being late.

I thought about that and I concluded that nothing technically prevents us from implementing that.
On the other hand, personally I still don't have much enthusiasm for this idea because I am not sure how common it is to combine gogoproto with grpc. This is also because the only benefit from this change is to make the representation in querystring clearer.

However, I could change my mind if I had more concrete use cases.

@dmitshur
Copy link
Contributor Author

dmitshur commented Jul 11, 2016

No problem, thanks for considering this issue.

I created it because I wanted us to think through this potentially unwanted scenario and see if there's a good solution to it. I agree it's helpful to not diverge too much from the underlying http.proto spec.

After thinking about this in the back of my mind for many days, I still haven't come up with an idea that I would consider viable.

So, I'm okay to close this feature request issue as "no viable solution" that is in the spirit of this library.

I don't want to try to provide convincing use cases because I don't think there's any that would warrant straying away from the spec.

Is it all right if I close the issue then?

@yugui
Copy link
Member

yugui commented Jul 11, 2016

Yes. I'll close this issue.

@yugui yugui closed this as completed Jul 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants