Handle rendering of URL Query Parameters in renderURL

[atreya2011]

The following is extracted from this file: http.proto

// HTTP | gRPC
// -----|-----
// `GET /v1/messages/123456`  | `GetMessage(name: "messages/123456")`
//
// Any fields in the request message which are not bound by the path template
// automatically become HTTP query parameters if there is no HTTP request body.
// For example:
//
//     service Messaging {
//       rpc GetMessage(GetMessageRequest) returns (Message) {
//         option (google.api.http) = {
//             get:"/v1/messages/{message_id}"
//         };
//       }
//     }
//     message GetMessageRequest {
//       message SubMessage {
//         string subfield = 1;
//       }
//       string message_id = 1; // Mapped to URL path.
//       int64 revision = 2;    // Mapped to URL query parameter `revision`.
//       SubMessage sub = 3;    // Mapped to URL query parameter `sub.subfield`.
//     }
//
// This enables a HTTP JSON to RPC mapping as below:
//
// HTTP | gRPC
// -----|-----
// `GET /v1/messages/123456?revision=2&sub.subfield=foo` |
// `GetMessage(message_id: "123456" revision: 2 sub: SubMessage(subfield:
// "foo"))`
//

However it seems like renderURL function in protoc-gen-grpc-gateway doesn't render the fields in the request message which are not bound by the path template to become URL query parameters if there is no HTTP request body.

Any advice on how to handle this would be great! Thanks in advance 🙏🏼

[lyonlai]

ah, right it doesn't do that for the moment. How often do you run into this use case? We mostly just leave the google.api.http option out that it's not often we need this.

Are you putting your hand up to implement this feature? Do you have a rough idea in mind?

[atreya2011]

Ah I see. I run into this use-case quite often especially with a GET endpoint like this /api/users?limit=10&pageToken=xxx.

It would be awesome if the plugin can take care of injecting the URL query parameters otherwise I may have to think of an alternate implementation for a GET endpoint like this /api/users?limit=10&pageToken=xxx.

I do have a rough idea in mind but that would require modifying the TypeInformation struct to hold list of fields (example: FieldList []string) for each type and then modify the analyzeMessage function to analyze and store the list of fields per message in the FieldList.

If we do that, then we can create a buildURLQueryParams function to use this list of fields present in the TypeInformation for each input message. And if it is a GET method, it will add to the URL, the query parameters which are not bound by the path template.

Please let me know what you think of this idea 🙏🏼

[lyonlai]

I think there are three things that I can think of for you to consider, @atreya2011.

1. Message in proto is a nested structure, a field inside could refer to any type from any file, which might or might not have other nested types.
2. When the client side code takes the request object and constructing the URL, it need to beware of that fields are optional.
3. be aware the length of the request url. try to make the length count as over length url might cause problem on communication path.

[atreya2011]

Thank you for your feedback and comments 🙇🏼 I will take all of them into consideration when I finalize my PR.

1. Message in proto is a nested structure, a field inside could refer to any type from any file, which might or might not have other nested types.
2. be aware the length of the request url. try to make the length count as over length url might cause problem on communication path.

Basically I am trying to following the design convention according to the following comments mentioned in http.proto

// Note that fields which are mapped to URL query parameters must have a
// primitive type or a repeated primitive type or a non-repeated message type.
// In the case of a repeated type, the parameter can be repeated in the URL
// as `...?param=A&param=B`. In the case of a message type, each field of the
// message is mapped to a separate parameter, such as
// `...?foo.a=A&foo.b=B&foo.c=C`.

However, as you mentioned, the length of the URL needs to considered, so I was thinking that maybe we can support only primitive types and repeated primitive types for now and ignore message types and nested types... because if there are deeply nested message types, then the URL can become very long...

Please let me know what you think about this idea 🙇🏼

2. When the client side code takes the request object and constructing the URL, it need to beware of that fields are optional.

We can perhaps try building the URL Query Parameters like this

  static ListUsers(req: ListUsersRequest, initReq?: fm.InitReq): Promise<ListUsersResponse> {
    return fm.fetchReq<ListUsersRequest, ListUsersResponse>(`/api/v1/users` + new URLSearchParams(req), {...initReq, method: "GET"})
  }

[lyonlai]

... so I was thinking that maybe we can support only primitive types and repeated primitive types for now and ignore message types and nested types... 

message type and nested types are important. and logically when you start the implementation the request message itself is a message already. might as well take the chance to handle nested types to have a rather complete solution. Sure you need to care about the length of the url, but also the following simple case doesn't really blow up the length of URL too.

// ...
message Request {
  Payload payload.= 1;
  int32 status = 2
}

message Payload {
  string fieldA;
  string fieldB;
}
// ...

---

  static ListUsers(req: ListUsersRequest, initReq?: fm.InitReq): Promise<ListUsersResponse> {
    return fm.fetchReq<ListUsersRequest, ListUsersResponse>(`/api/v1/users` + new URLSearchParams(req), {...initReq, method: "GET"})
  }

using URLSearchParams is handy but it

1. doesn't filter out the empty fields, which doesn't help the length of the URL.
2. nested object doesn't work at all as per described in http.proto
3. the output of repeated type also not something that matches http.proto

You might need to write a custom handler somehow.

[atreya2011]

You might need to write a custom handler somehow.

I agree regarding the disadvantages of using URLSearchParams and will write a custom handler.

message type and nested types are important. and logically when you start the implementation the request message itself is a message already. might as well take the chance to handle nested types to have a rather complete solution. Sure you need to care about the length of the url, but also the following simple case doesn't really blow up the length of URL too.

// ...
message Request {
  Payload payload.= 1;
  int32 status = 2
}

message Payload {
  string fieldA;
  string fieldB;
}
// ...

Handling of nested fields for a simple case as above is possible and the URL length won't be too long as you have mentioned. But I have a concern and that is, if there are a lot of deeply nested messages there is a possibility that the URL can get long.

Let me try custom handler implementation which renders nested messages as well and check how long the URL gets in case of deep nesting 🙇🏼

Thank you for your comments and feedback 🙏🏼

[lyonlai]

Well, if a user feed a long array in the repeated field then the URL could blow up, or a long text in a single string field could too. Hence why URL is a concern, but it's not against the nested type requirement.

We can't control how much user put in the request object. However, we could have optimal URL generation, like how we ditch URLSearchParams as it doesn't do empty field filtering and meaninglessly extend the length of the URL.

Well, give it a try and let me know how it goes. Thanks for taking this on.

[atreya2011]

Well, if a user feed a long array in the repeated field then the URL could blow up, or a long text in a single string field could too. Hence why URL is a concern, but it's not against the nested type requirement.

We can't control how much user put in the request object. However, we could have optimal URL generation, like how we ditch URLSearchParams as it doesn't do empty field filtering and meaninglessly extend the length of the URL.

Well, give it a try and let me know how it goes. Thanks for taking this on.

Thank you for letting me working on this 🙇🏼 When you get the time, can you let me know if I am going in the right direction in my draft PR. https://github.com/grpc-ecosystem/protoc-gen-grpc-gateway-ts/pull/11