-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat: add grpc transcoding + tests #146
Conversation
[[['get','/v1/{name}','']], {'foo':'bar'}], | ||
[[['get','/v1/{name}','']], {'name':'first/last'}], | ||
[[['get','/v1/{name=mr/*/*}','']], {'name':'first/last'}], | ||
[[['post','/v1/{name}','data']], {'name':'first/last'}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add comments explaining these test cases (like the ones above for test_transcode
)
It looks like
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on test cases. If any need implementation changes, it's fine to add TODOs so we remember to come back in a later PR.
{'field':'parent/p1'}, | ||
['get','/v1/parent/p1',{},{}]], | ||
|
||
[[['get','/v1/{field=a/**/b/**}','']], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: **
can only happen at the end of the URL (only the verb can come after)
https://cloud.google.com/endpoints/docs/grpc-service-config/reference/rpc/google.api#grpc-transcoding
However, I don't think this affects you; this should be filtered upstream when the annotations are checked for validity.
['get','/v1/parent',{},{}]], | ||
|
||
[[['get','/v1/{field.sub}','']], | ||
{'field.sub':'parent', 'foo':'bar'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if you have this?
[[['get','/v1/{field.sub}','']],
'field': {'name': 'alice', 'sub':'parent'}
We should get
['get','/v1/parent',{},{'field.name':'alice'}]]
[[['post','/v1/{field=a/*}/b/{name=**}','data']], | ||
{'field':'a/parent','name':'first/last','data':{'id':1, 'info':'some info'},'foo':'bar'}, | ||
['post','/v1/a/parent/b/first/last',{'id':1, 'info':'some info'},{'foo':'bar'}]], | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another test case: where a URI field is a member of body:
[[['post','/v1/{foo.name}/b','foo']],
{'foo': {'name':'bob', 'state':'wa'}},
['post','/v1/bob/b',{'name':'bob', 'state':'wa'},{}]],
['post','/v1/a/parent/b/first/last',{'data':{'id':1, 'info':'some info'},'foo':'bar'},{}]], | ||
|
||
# Additional bindings | ||
[[['post','/v1/{field=a/*}/b/{name=**}','extra_data'], ['post','/v1/{field=a/*}/b/{name=**}','*']], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the second pattern, I suggest changing /b/
to /c/
to make it super clear where the answer is coming from
BTW, once this PR goes in, we'll need to refactor some of the functionality introduced in googleapis/gapic-generator-python#702 to do the transcoding here, correct? |
Hi @yon-mg , I'm going to close this PR due to inactivity but please feel free to re-open it. |
To provide REST/JSON transport for the python-gapic-generator, it is necessary to implement the gRPC transcoding rules outlined here,
https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44-L312.
gRPC transcoding essentially outlines how to convert a gRPC request to an HTTP one. In this PR is only an implementation the pattern matching aspects of gRPC transcoding. It does not have any logic to process requests as proto messages, perform JSON encoding or make the request itself. That is left the generated clients.