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 nested messages in GET request queries #492

Open
DmitriyMV opened this issue Dec 1, 2017 · 12 comments
Open

Support nested messages in GET request queries #492

DmitriyMV opened this issue Dec 1, 2017 · 12 comments

Comments

@DmitriyMV
Copy link

DmitriyMV commented Dec 1, 2017

Given the

service MyService {
    rpc myMethod (myMethodRequest) returns (myMethodResponse) {
        option (google.api.http) = {
            get: "/method/all"
        };
    }
}

message myMethodRequest {
    repeated Scoped scope = 1;

    message Scoped {
        oneof condition {
            ById by_id = 1;
        }
        message ById {
            uint32 id = 1;
        }
    }
}

message myMethodResponse {
    repeated uint32 ids = 1;
}

with GET request

localhost:8080/method/all?scope.by_id.id=3

returns an error

unexpected repeated field in scope.by_id.id
@rogerhub
Copy link

rogerhub commented Dec 2, 2017

As far as I can tell, there isn't a way to specify a repeated message field using GET query parameters alone. Only repeated primitive fields (strings, ints, bools, floats, etc) are supported in query parameters. See runtime/query.go.

Can you use POST instead?

@achew22
Copy link
Collaborator

achew22 commented Dec 14, 2017

I think @rogerhub's answer is probably the best way to go here. I'm not sure what the standard syntax for repeated fields in a query string would be. I guess the PHP format? I'm not sure though and think you would probably be happier with this as a different verb.

@achew22 achew22 closed this as completed Dec 14, 2017
@kwoodhouse93
Copy link

I think there's a valid use case for this. In my case, I have a ListResource method as a GET request. This method takes a parameter called filter, which is a structured message that defines the various fields that can be filtered by.

A simplified example:

service ResourceService {
    rpc ListResource (ListResourceRequest) returns (ListResourceResponse) {
        option (google.api.http) = {
            get: "/api/resource"
        };
    }
}

message ListResourceRequest {
    ResourceFilter filter = 1;
}

message ResourceFilter {
    repeated string ID = 1;
    repeated RangeFilter range = 2;
}

message RangeFilter {
    int64 minimum = 1;
    int64 maximum = 2;
}

message myMethodResponse {
    repeated Resource resources = 1;
}

Trying a request:

/api/resource?filter.resources.ranges.minimum=4
{"error":"unexpected repeated field in filter.resources.ranges.minimum","message":"unexpected repeated field in filter.resources.ranges.minimum","code":3,"details":[]}

In this case, the method really shouldn't be POST. It's ok as a temporary workaround, but I wouldn't consider this issue fixed just because I can use POST instead. Doing so makes the API less consistent and weakens the meaning of the request verbs.

I also understand the syntax isn't obvious, but there are a few alternatives that could work, and the benefit of supporting a use case like the one above is quite significant.

For me, being able to specify the structure of the filters in the protos format helps make sense of an otherwise complex part of the API. The commonly seen alternative of using a string field as generic filter (e.g. /api/resource?filter=blahblahblah...) ignores that complexity, and leads to a fragile, implicit contract between the caller and the server, throwing away some of the benefits that drove me to choose gRPC in the first place.

So, for the use case I've laid out above (and I could conceive of others), I'd be grateful if you could reconsider supporting repeated message fields in query strings.

@stale
Copy link

stale bot commented Sep 9, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Sep 9, 2019
@craigberry1
Copy link

*Bump

I agree with @kwoodhouse93 there is valid use case for this.

@stale stale bot removed the wontfix label Sep 10, 2019
@johanbrandhorst johanbrandhorst changed the title Repeated nested messages doesn't work in GET requests queries. Support nested messages in GET request queries Sep 10, 2019
@sum2000
Copy link

sum2000 commented Sep 10, 2019

I agree with the proposal and willing to solve this, I am new to this repo so what will be a good starting point to solve this?

@johanbrandhorst
Copy link
Collaborator

Hi @sum2000, thanks for showing an interest in contributing! I think all the query parameter parsing happens in

func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values []string) error {
, so that would be the place to start looking. The first thing to do would be to add a new test case to https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/query_test.go#L85 and see if you can make it pass. Let me know if you need anymore pointers!

@sum2000
Copy link

sum2000 commented Sep 10, 2019

Hi @sum2000, thanks for showing an interest in contributing! I think all the query parameter parsing happens in

func populateFieldValueFromPath(msg proto.Message, fieldPath []string, values []string) error {

, so that would be the place to start looking. The first thing to do would be to add a new test case to /runtime/query_test.go@master#L85 and see if you can make it pass. Let me know if you need anymore pointers!

Thanks, will do! Is there a discord or chat for grpc-gateway ?

@johanbrandhorst
Copy link
Collaborator

We have a channel on the gophers slack: #grpc-gateway after joining https://invite.slack.golangbridge.org/.

@npuichigo
Copy link

any updates? quite a good feature to support

@johanbrandhorst
Copy link
Collaborator

I don't think there's been any work on this recently unfortunately, but if you'd like to see this feature in the gateway, please feel free to have a go at implementing it. My advice above is still relevant.

@mauza
Copy link

mauza commented May 29, 2024

I just ran into this same issue. We created an ordering struct Page so you could specify multiple (repeated) fields to order the response by on the GetPersons endpoint. The easiest way to fix this is to make it a Post to "GetPersons" but that just seems silly.

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

9 participants