Support HttpRule with field response #712
Conversation
|
Ha, this seems to be exactly what @ivucica wanted in #707. Looking at https://github.com/googleapis/googleapis/blob/201d7be7f9da925df93bc52f8108963284f61aed/google/api/http.proto, this doesn't seem to be an official part of the |
|
I have not yet received upstream response for #707. I am opposed to merging this particular PR, as it may (cough cough) collide with upstream fields which are not publicly visible. I am also opposed to forking In that light, I suggest careful examination of what's currently present in google.api.HttpRule documentation, particularly look at |
|
This is improved, but if you do receive a response on googleapis/googleapis#512, I suspect it will be pushback. I'm re-contacting upstream. |
There was a problem hiding this comment.
-
Please regenerate tests.
-
Please consider the very, very probable outcome where you won't be able to merge the upstream
google.api.HttpRulechange -- can you reserve a new set of proto options in the option registry for grpc-gateway (in addition to the existing swagger-specific ones) and implement this using that?Let us know if you agree to update the PR like this and if you email the registry alias; otherwise I will do that in a day or two.
protoc-gen-grpc-gateway/main.go
Outdated
| allowDeleteBody = flag.Bool("allow_delete_body", false, "unless set, HTTP DELETE methods may not have a body") | ||
| grpcAPIConfiguration = flag.String("grpc_api_configuration", "", "path to gRPC API Configuration in YAML format") | ||
| allowRepeatedFieldsInBody = flag.Bool("allow_repeated_fields_in_body", false, "allows to use repeated field in `body` field of `google.api.http` annotation") | ||
| useJSONNameInSwaggerDef = flag.Bool("json_name_in_swgdef", false, "if it sets Field.GetJsonName() will be used for generating swagger definitions, otherwise Field.GetName() will be used") |
There was a problem hiding this comment.
Why is this flag only affecting the swagger definition? Won't this break the api generated with protoc-gen-grpc-gateway?
If this affects only protoc-gen-swagger, why is this a flag in protoc-gen-grpc-gateway?
There was a problem hiding this comment.
You're right, this flag should be in protoc-gen-swagger, i'll move it.
There was a problem hiding this comment.
- Please consider the very, very probable outcome where you won't be able to merge the upstream google.api.HttpRule change -- can you reserve a new set of proto options in the option registry for grpc-gateway (in addition to the existing swagger-specific ones) and implement this using that?
@ivucica do you mean implement option like this:
message AccountArray {
repeated Account account = 1 [(grpc.gateway.options).unwrap_field = true]; // or promote_field?
}
I think it's less obvious solution than response_body field, and in this case you can't add additional bindings with different requests/responses mapping for example, how do you implement this with you solution?
service Strings {
rpc ToUpper (String) returns (String) {
option (google.api.http) = {
post: "/strings/to_upper"
body: "str"
response_body: "str"
additional_bindings: {
post: "/strings/to_upper/v2",
body: "*",
response_body: "*",
}
};
}
}
message String {
repeated string str = 1;
}
There was a problem hiding this comment.
That was my original proposal, but I am fine with attaching it to rpc instead of message. We only need to move it to a proto field we actually control.
It is correct that you cannot add a different response_body with what I am proposing:
service Strings {
rpc ToUpper (String) returns (String) {
option (google.api.http) = {
post: "/strings/to_upper"
body: "str"
additional_bindings: {
post: "/strings/to_upper/v2",
body: "*",
}
};
option (grpc.gateway.rpc_options) = {
response_body: "str"
};
}
}
That is indeed unfortunate, but I see additional_bindings as a bonus, and for the most part, I just use a 1:1 mapping.
Anything I can think of is ugly, including this possibly tolerable addition (which is not incompatible with what's above):
rpc ToUpper (String) returns (String) {
option (google.api.http) = {
post: "/strings/to_upper"
body: "str"
additional_bindings: {
post: "/strings/to_upper/v2",
body: "*",
}
};
option (grpc.gateway.rpc_options) = {
configure_rule: {
rule: { post: "/strings/to_upper" }
config: { response_body: "str" }
}
configure_rule: {
rule: { post: "/strings/to_upper/v2" }
config: { response_body: "*" }
}
};
}
To get what you want to happen, we do need to get upstream to release the response_body, which may be difficult.
Once again: I am nearly certain your .HttpRule PR will not be accepted, due to insights that I cannot detail here without approval. So, once again: you should explore alternative designs unless upstream decides to provide the field.
I strongly suspect grpc-gateway will not move to a forked google.api.HttpRule either (especially one that may long-term conflict with upstream developments).
There was a problem hiding this comment.
I propose we take design discussions into #707 instead of this particular PR?
|
@ivucica could you help with this failed job, please: https://travis-ci.org/grpc-ecosystem/grpc-gateway/jobs/410260132 ? What's wrong here? |
|
Some ongoing CI issues: #711 (comment) I would not be worried and, were this PR otherwise mergable, I would not object to merging it. |
|
Please document why this feature is needed. We intentionally keep gRPC transcoding extremely simple. The goal for gRPC transcoding is to build REST API from RPC API. We don't intend to be feature rich as OpenAPI. We generally don't need feature to build great and useful APIs. The less features, the better performance and quality. |
|
@wora See #707 for my own feature request. As I said there, some preexisting APIs, such as Mastodon's, cannot currently be described with Without this change, when attempting to describe Mastodon's existing API, one would
While that's somewhat tolerably doable, it's way hackier than just allowing users to specify which field should be the top level JSON entity. Example endpoint that returns a list I do believe designing APIs like so is a bad design, but that's better to be strongly discouraged in documentation and in the API Design Guide. @doroginin might have another use. |
We use |
|
I am publishing the change from upstream. It should land in a few days. |
|
|
||
| // useJSONNameInSwaggerDef if it true Field.GetJsonName() will be used for generating swagger definitions, | ||
| // otherwise Field.GetName() will be used | ||
| useJSONNameInSwaggerDef bool |
There was a problem hiding this comment.
Please rename this into useJSONNamesForFields and update the comment appropriately :-)
Separately, can you then make protoc-gen-grpc-gateway use the JSON names as well if the flag is set? If not, can you please add a TODO here for someone to take a stab at it. It would be long-term important that -swagger defines the same API that -grpc-gateway exposes.
| r.useJSONNameInSwaggerDef = use | ||
| } | ||
|
|
||
| // GetUseJSONNameInSwaggerDef whether generation one swagger file out of multiple protos |
There was a problem hiding this comment.
This comment is incorrect and nonsensical*, could you please fix it? :)
- Its grammar is nonsensical even in
IsAllowMerge, but here the comment itself is outright wrong.
| @@ -318,6 +325,22 @@ func (r *Registry) SetMergeFileName(mergeFileName string) { | |||
| r.mergeFileName = mergeFileName | |||
| } | |||
|
|
|||
| // SetAllowRepeatedFieldsInBody controls whether repeated field can be used in body field path | |||
There was a problem hiding this comment.
Given you are providing a getter for useJSONNameInSwaggerDef, did you mean to provide it for allowRepeatedFieldsInBody?
| @@ -227,10 +234,10 @@ func (p Parameter) ConvertFuncExpr() (string, error) { | |||
| return conv, nil | |||
| } | |||
|
|
|||
| // Body describes a http requtest body to be sent to the method. | |||
| // Body describes a http request(response) body to be sent to the method(client). | |||
There was a problem hiding this comment.
How about the bash syntax:
// Body describes a http {request,response} body to be sent to the {method,client}.
or the regex syntax:
// Body describes a http (request|response) body to be sent to the (method|client).
Maybe expand the comment a bit, and mention this is used in body and response_body options in google.api.HttpRule?
| @@ -625,6 +631,23 @@ func renderServices(services []*descriptor.Service, paths swaggerPathsObject, re | |||
|
|
|||
| methProtoPath := protoPathIndex(reflect.TypeOf((*pbdescriptor.ServiceDescriptorProto)(nil)), "Method") | |||
| desc := "" | |||
| var responseSchema swaggerSchemaObject | |||
|
|
|||
| if b.Response == nil || len(b.Response.FieldPath) == 0 { | |||
There was a problem hiding this comment.
Add a comment that this is resolving the value of response_body in the google.api.HttpRule
Codecov Report
@@ Coverage Diff @@
## master #712 +/- ##
==========================================
- Coverage 56.16% 55.71% -0.46%
==========================================
Files 30 30
Lines 3112 3152 +40
==========================================
+ Hits 1748 1756 +8
- Misses 1193 1223 +30
- Partials 171 173 +2
Continue to review full report at Codecov.
|
|
@ivucica |
|
Now i'm preparing some tests for changes in this PR |
|
I previously asked:
If you don't plan on updating protoc-gen-grpc-gateway to respect JSON names flag, could you at least add a TODO for someone else to do it? Thanks |
I think it's not needed. I added more comments about it for new flag. |
|
@ivucica can you help to fix bazel test: https://travis-ci.org/grpc-ecosystem/grpc-gateway/jobs/414555020 ? I have never work with this stuff. We need to update version of google.golang.org/genproto to 383e8b2c3b9e36c4076b235b32537292176bae20 how can i do it? |
| // response, if it uses json tags for marshaling. | ||
| // Pay attention. By default grpc-gataway uses original proto name in responses. If you would like | ||
| // to use names from json tags change marshaller for gateway with runtime.ServeMuxOption | ||
| // runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.JSONPb{OrigName: false}) |
There was a problem hiding this comment.
While this is helpful, I don't like the tone of this. How about:
// otherwise the original proto name is used. It's helpful for synchronizing the swagger definition with grpc-gateway
// response, if it uses json tags for marshaling.
Gopkg.toml
Outdated
| @@ -51,7 +51,7 @@ required = [ | |||
| [[constraint]] | |||
| # Also defined in bazelbuild/rules_go | |||
| # https://github.com/bazelbuild/rules_go/blob/436452edc29a2f1e0edc22d180fbb57c27e6d0af/go/private/repositories.bzl#L131 | |||
| revision = "86e600f69ee4704c6efbf6a2a40a5c10700e76c2" | |||
| revision = "383e8b2c3b9e36c4076b235b32537292176bae20" | |||
There was a problem hiding this comment.
Will this change require a corresponding change to bazelbuild/rules_go?
There was a problem hiding this comment.
I see you already managed to confirm we will need this change.
There was a problem hiding this comment.
Can you update this comment as well since this is no longer correct?
There was a problem hiding this comment.
This comment still needs updating
|
@johanbrandhorst |
|
Here i changed |
|
Please squash the commits into a single commit with a nice descriptive commit message as well, in preparation for merge. |
…pc-ecosystem#712) Also: * add flag `allow_repeated_fields_in_body` in `protoc-gen-grpc-gateway` * add flag `json_names_for_fields` in `protoc-gen-swagger`
doroginin
force-pushed
the
httprule-with-response
branch
from
e44e19d
to
936b4c8
Compare
…pc-ecosystem#712) Also: * add flag `allow_repeated_fields_in_body` in `protoc-gen-grpc-gateway` * add flag `json_names_for_fields` in `protoc-gen-swagger`
| name = "org_golang_google_genproto", | ||
| commit = "383e8b2c3b9e36c4076b235b32537292176bae20", | ||
| importpath = "google.golang.org/genproto", | ||
| ) |
There was a problem hiding this comment.
Sorry, could you reference Gopkg.toml here as well? These two must be kept in sync, obviously.
There was a problem hiding this comment.
Really these should all reference Gopkg.toml 🤔
…pc-ecosystem#712) Also: * add flag `allow_repeated_fields_in_body` in `protoc-gen-grpc-gateway` * add flag `json_names_for_fields` in `protoc-gen-swagger`
doroginin
force-pushed
the
httprule-with-response
branch
from
72ba49e
to
b8fc556
Compare
|
This LGTM, but I'll leave it for @ivucica to merge as he's more involved than I with this. Thanks for your contribution @doroginin! |
|
Guys, any update here? |
|
@doroginin Thanks for your patience, I've reached out to Ivan, we'll try and get this merged as soon as possible :). |
* regenerate * using latest protoc-gen-go
|
Ouch. This will actually break people who don't use Gopkg.lock and just @johanbrandhorst Do you think we should rollback or roll forward? |
|
⚡ Can we roll forward? I'd prefer to get the right version ASAP. |
|
I'd like that too; I reopened the internal bug. |
|
Can we roll backwards? This just broke all of my builds that depend on this project. Unless the sync has yet to happen and that will fix this error: |
|
@bcdurden Fair enough, we'll sort this asynchronously. |
|
Much appreciated! I should have read upwards a bit before I commented anyhow 💯 |
|
All good! Thanks for the quick turn-around! |
|
Thank you for merging this, guys! Is there any plan to set tag for this version? |
|
And let's restart the build on master branch, cause now it's errored! |
|
@doroginin See #733 |
…pc-ecosystem#712) Also: * add flag `allow_repeated_fields_in_body` in `protoc-gen-grpc-gateway` * add flag `json_names_for_fields` in `protoc-gen-swagger`
This PR adds support optional
response_bodyfield for google.api.HttpRule. Also see PR: googleapis/go-genproto#87 and googleapis/googleapis#512This version of code can work with current
HttpRuleimplementation and with forked implementation with new field. This implementation does not break existing code.Also
allow_repeated_fields_in_bodyflag added to make it possible using repeated fields inbodyandresponse_body. This flag can be used for slices in request and response.Also flag
json_name_in_swgdefadded to make it possible to use Field.GetJsonName() instead of Field.GetName() for generating swagger definitions. See: #681 for more info.