Support combination of query params and body for POSTs with body: "*"

[atombender]

Currently, according to http.proto, it's not possible to combine both query parameters and a body. I don't see any technical reason why this is not possible; manually tweaking the genertaed .pb.gw.go with a call to PopulateQueryParameters seems to confirm that it works fine.

For example, assume that every RPC request accepts a parameter accessToken. It's convenient to say that the parameter may occur either in the body or in the query. These should be equivalent:

$ curl -XPOST http://localhost:20000/ -d'{
  "accessToken": "foo",
  "message": "hello world"
}'

and:

$ curl -XPOST http://localhost:20000/?accessToken=foo -d'{
  "message": "hello world"
}'

However, grpc-gateway only seems to offer the choice between body: "*" and body: "message". I'd actually be happy with a compromise that mandates accessToken always be a query-string param, but you can't choose a subset of fields, either.

[tmc]

@atombender this would be a great addition, I'm happy to review any work or answer any questions.

[atombender]

Cool! Naively, would there be any harm in changing the heuristic so that if the body annotation isn't set, we simply pick from both query params and body? HasQueryParam would return true, and as far as I can see, the current logic just works.

[tmc]

See https://github.com/googleapis/googleapis/blob/master/google/api/http.proto for semantics.

In particular https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L130 shows that path params should be used for those that aren't provided in body (for '*' case).

[atombender]

Maybe I'm misunderstanding you, but with body: "*", no query params are used. The generated code never calls runtime.PopulateQueryParameters. Bug?

I have to admit that the wording in http.proto (line 195) is not very clear. It seems to say that if body is *, then fields should be picked from both the query string and the body.

[achew22]

I would suggest sending a pull request to googleapis/googleapis to clarify the wording in whichever way is easiest and see what they do. If they accept then implement that, else prefer the other.

[tmc]

@atombender sorry I misread your original post. On reading further my understanding is that in the context of body: "*" that fields not bound by path parameters (/{message_id}/) are bound by the body but HTTP paramters (/method/?message_id=foo) are not bound.

[atombender]

I find the spec confusingly worded. Why are query params singled out? A few paragraphs down:

// Note that when using * in the body mapping, it is not possible to
// have HTTP parameters, as all fields not bound by the path end in
// the body. This makes this option more rarely used in practice of
// defining REST APIs. The common usage of * is in custom methods
// which don't use the URL at all for transferring data.

This doesn't make sense to me. REST-style POSTs don't preclude query parameters, like so:

$ curl -XPOST -d'{"answer": 42}` http://localhost/someroute?name=bob

In the context of gRPC, I don't see any reason not to map name to a body field (the gRPC message).

[zinuga]

@jayantkolhe comments?

[yugui]

IIUC, the spec by http.proto:197-202 are trying to define a canonical way to specify values for each parameter. So http.proto prohibits this feature by design.

Could you give me some real use cases of this feature? If we have enough number of reasonable use cases, probably we can relax the restriction with a certain explicit configuration, like a command line option of protoc-gen-grpc-gateway or our own custom options in .proto.

@tmc Thank you for your PR. But I cannot agree on it at this moment. Let's discuss the expected spec here.

[atombender]

I did give a use case in my original comment. Query parameters are generally more user-friendly than JSON. Let's say I have a big message stored in a file called data.json. I want to post it:

$ curl -d @data.json http://localhost/ingest

Something goes wrong and I want to debug this. Say my service takes a parameter verbose, which increases logging for that request. Instead of editing data.json (which is nice and clean), I could just do this:

$ curl -d @data.json http://localhost/ingest?verbose=true

However, this is not possible.

[tmc]

@yugui shouldn't path parameters be included but not query string parameters?

[jayantkolhe]

For the use-case Alexander mentions, we have typically recommended using
headers for sending those parameters. We are doing the same in gRPC
(through metadata) and it would be more aligned if we do the same for REST
conversion.

On Wed, Oct 19, 2016 at 11:37 AM, Travis Cline [email protected]
wrote:

@yugui https://github.com/yugui shouldn't path parameters be included
but not query string parameters?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#234 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGkTt0yF1ot_GUJoq8guGTSmXDqE3ikEks5q1mOAgaJpZM4KXgiY
.

[atombender]

Sure, it would be nice if grpc-gateway had a general-purpose mechanism for mapping headers. For a public-facing API it's a bit ridiculous to require users to format a header such as Grpc-Metadata-Verbose: true.

However, headers aren't very ergonomic, since they are generally treated as out-of-band data. It's common for apps to log URLs and query params, for example, but not headers. With headers it's one more thing to consider.

[tmc]

FWIW I've personally wanted the PUT to /entity/{ID} use case to populate the ID from the path.

[yugui]

@atombender
Sorry for my unclear comment. Your original example looked to be quite artificial.

Thank you for the second example. Based on the example, right, headers are out-of-band. But it looks to be natural in the second example because verbose flag is basically out-of-band metadata rather than the content of the data.

I'm not strongly against this feature. But I need to understand what we actually need better with good examples because we are trying to go out of the combat-proven design by http.proto.
At this moment, I don't have clear examples which support this feature.

[atombender]

Sure, headers are less ergonomic than query params, but a decent compromise. But grpc-gateway doesn't support a generic mapping of headers, either (that I know of). So you can't argue that headers are better unless you propose that http.proto is extended to support headers. I don't want to expose a header like Grpc-Metadata-Verbose, because gRPC is an implementation detail from the point of view of the API consumer. The header would need to be called Verbose or something similar, and I would need to be able to specify this mapping in my .proto file.

[mattolson]

Whether we agree with it or not, it seems pretty clear from the proto file that if body: "*" is specified, we should not pull params from the query string, but only from the body and path params.

[achew22]

I think this has stalled out. Please feel free to reopen if you have a strong opinion about this and would like to revisit it..

[ratsirarar]

What was the consensus on this issue?

[johanbrandhorst]

Just to recap, I think the goal of this issue is for the grpc-gateway to support using body:"*" and still allow some fields in the query; http.proto explicitly states that the use of body:"*" precludes the use of query parameters altogether. I understand that this project wants to stay true to the design of http.proto, so the right place for that discussion to happen is probably the googleapis repo.

However, using specific body parameters (body: "message") in combination with query parameters should work. If that is not the case, it is a separate issue.

[johanbrandhorst]

@atombender please correct any incorrect information in my previous post.

[glerchundi]

However, using specific body parameters (body: "message") in combination with query parameters should work. If that is not the case, it is a separate issue.

Sorry for poping up this issue again but this is a legitimate use case, take this "standard method" (as stated by the google api design guidelines) as an example:

service UserService {
  rpc CreateUser(CreateUserRequest) returns User {
    option (google.api.http) = {
      post: "/users",
      body: "user"
    };
  }
}

message CreateUserRequest {
  string password = 1;
  User user = 2;
}

message User {
  string first_name = 1;
  string last_name = 2;
}

So the User entity is not tainted with adjacent information. In this case this would allow to do the following HTTP query:

POST /users?password=1234
{
  "first_name": "Gorka",
  "last_name": "Lerchundi Osa",
}

[johanbrandhorst]

This should be supported? Is this not working?

[glerchundi]

Sorry for not mentioning that it was a mere comment just to make it clear it’s a legitimate use case. 🙏

[NeoyeElf]

This should be supported? Is this not working?

For http client, it's seems not working. The path will not bind the query params if i use the generated xxxServiceHTTPClientImpl. I think this is related to the code path := binding.EncodeURL(pattern, in, false) in the implementation of the interface in the generated xxx_http.proto.go

[johanbrandhorst]

Hi, could you provide an example of some code that's not working? Preferably a github repo. I also think we need a new issue, this is very old.

[nehalkumarsingh]

Hey folks, I have a usecase of having a GET call say /check?user=me&type=check with body being {purpose:"reqest"}. I suppose while we define the request message it would have

message CheckRequest {
  string user = 1;
  string type = 2;
  string purpose = 3;
}
message CheckResponse {
  bool success = 1;
}

Now what I understand is that it is not allowed for me to have a get call with both query param and body. While I define the request in the proto as follows

service CheckService {
  rpc Check(CheckRequest) returns (CheckResponse) {
    option (google.api.http) = {
      get: "/check{?user,type}"
      body: "purpose"
    };
  }
}

Is it fine to define like this? But we cannot define with body: "*". As I have read somewhere it is not allowed?

[johanbrandhorst]

GET requests with a body are not supported by the HTTP spec, it's not a grpc-gateway limitation. If you want a body, use POST, PATCH or PUT.