-
Notifications
You must be signed in to change notification settings - Fork 66
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 proper handling of query/path/body parameters for rest transport #702
Changes from 20 commits
c1a6f89
bd7c32d
cd3a7f1
b41db31
ce69e7d
89fa08b
86f3a9c
234cc09
35ac276
c8155a5
55a815c
ac0bdef
e79e8c7
824ad11
2d11e19
b5c6d06
221cd44
475f903
f08ca32
efa0ed6
d3e08c0
df88ef9
67c6bd1
37e3d28
c964fba
85be88d
5d1c6a3
f6c64cc
a4f34e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -767,6 +767,23 @@ def http_opt(self) -> Optional[Dict[str, str]]: | |
# TODO(yon-mg): enums for http verbs? | ||
return answer | ||
|
||
@property | ||
def path_params(self) -> Sequence[str]: | ||
if self.http_opt is None: | ||
return [] | ||
pattern = r'\{\w+\}' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment that this handles grpc encoding in its simples case |
||
return [x[1:-1] for x in re.findall(pattern, self.http_opt['url'])] | ||
|
||
@property | ||
def query_params(self) -> Set[str]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious: why can't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose they could. Just thought it's more appropriate since ordering seems important for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed. That seems reasonable, but a complication is that repeated fields map to repeated query params. In that case, a sequence may be best. (If order doesn't matter, a multiset might be enough, but we can't assume that order doesn't matter for repeated fields) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right set is not appropriate even if order doesn't matter but I should mention though that once query param logic is moved out, this won't matter anymore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True! |
||
if self.http_opt is None: | ||
return set() | ||
body = [] | ||
if 'body' in self.http_opt.keys(): | ||
body.append(self.http_opt['body']) | ||
all_params = self.input.fields.keys() | ||
software-dov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return set(all_params) ^ set(body + list(self.path_params)) | ||
software-dov marked this conversation as resolved.
Show resolved
Hide resolved
software-dov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# TODO(yon-mg): refactor as there may be more than one method signature | ||
@utils.cached_property | ||
def flattened_fields(self) -> Mapping[str, Field]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,28 +135,42 @@ class {{ service.name }}RestTransport({{ service.name }}Transport): | |
|
||
{%- if 'body' in method.http_opt.keys() %} | ||
# Jsonify the input | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: for clarity, maybe s/ |
||
data = {{ method.output.ident }}.to_json( | ||
{%- if method.http_opt['body'] == '*' %} | ||
{%- if method.http_opt['body'] != '*' %} | ||
data = {{ method.input.fields[method.http_opt['body']].type.ident }}.to_json( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. !! This will handle dot-notation nested fields, correct? If not, add a TODO. Context: while https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L350 suggests that's not allowed, some APIs do have it (see gRPC transcoding doc), eg. https://github.com/googleapis/googleapis/blob/836f0eaf5f21f300f63ac635e5ef263d183e0cdd/google/cloud/dialogflow/cx/v3beta1/session.proto#L95 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not handle it but I have added a todo in the appropriate place in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. As discussed, we should err on the side of duplicate TODOs rather than too few. If we address one, we're likely to see the others and address/delete them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe have the generated code call this variable |
||
request.{{ method.http_opt['body'] }}, | ||
including_default_value_fields=False | ||
) | ||
{%- else %} | ||
data = {{ method.input.ident }}.to_json( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does request already have the body and query params stripped out at this point, so they don't get sent in two places? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we clarified yesterday: params in the path may be (but don't have to be) repeated in the body if we're using |
||
request | ||
{%- else %} | ||
request.body | ||
{%- endif %} | ||
) | ||
{%- endif %} | ||
{%- endif %} | ||
|
||
{# TODO(yon-mg): Write helper method for handling grpc transcoding url #} | ||
# TODO(yon-mg): need to handle grpc transcoding and parse url correctly | ||
# current impl assumes simpler version of grpc transcoding | ||
# current impl assumes basic case of grpc transcoding | ||
# Send the request | ||
url = 'https://{host}{{ method.http_opt['url'] }}'.format( | ||
host=self._host, | ||
{%- for field in method.input.fields.keys() %} | ||
{%- for field in method.path_params %} | ||
{{ field }}=request.{{ field }}, | ||
{%- endfor %} | ||
) | ||
|
||
potentialParams = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would probably be clearer to s/ |
||
{%- for field in method.query_params %} | ||
'{{ field|camel_case }}': request.{{ field }}, | ||
vchudnov-g marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{%- endfor %} | ||
} | ||
potentialParams = {k: v for k, v in potentialParams.items() if v} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very minor premature optimization nit: since we're immediately iterating over and then discarding this dictionary, we can turn it into a generator instead and prevent looping over the same data multiple times. potential_params = ((k, v) for k, v in potentialParams.items() if v) # The parentheses make this a generator expression.
for i, (param_name, param_value) in enumerate(potentialParams): This is a good rundown on generators and generator expressions. Dave Beazley also has a really fun youtube talk on generators. |
||
for i, (param_name, param_value) in enumerate(potentialParams.items()): | ||
q = '?' if i == 0 else '&' | ||
url += q + param_name + '=' + str(param_value).replace(' ', '+') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: prefer format strings to concatenating using +. It's easier to eyeball parse for longer or more convoluted strings. url += "{q}{name}={value}".format(q=q, name=param_name, value=param_value.replace(' ', '+')) |
||
|
||
{% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['verb'] }}( | ||
url, | ||
{%- if 'body' in method.http_opt.keys() %} | ||
url | ||
{%- if 'body' in method.http_opt.keys() %}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to use |
||
json=data, | ||
{%- endif %} | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,3 +45,17 @@ def to_snake_case(s: str) -> str: | |
|
||
# Done; return the camel-cased string. | ||
return s.lower() | ||
|
||
def to_camel_case(s: str) -> str: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any built-in/standard python functions which do the same? Please prefer using standard ones to custom, if there are any. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fairly certain there is no standard library function for to camel-case. |
||
'''Convert any string to camel case. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: In this and the previous, pre-existing function, we're not touching spaces, right? Might be worth mentioning that. |
||
|
||
This is provided to templates as the ``camel_case`` filter. | ||
|
||
Args: | ||
s (str): The input string, provided in snake case. | ||
|
||
Returns: | ||
str: The string in camel case with the first letter unchanged. | ||
''' | ||
items = s.split('_') | ||
return items[0] + "".join([x.capitalize() for x in items[1:]]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: no need to make a list as the argument to join. We could replace the square brackets with parentheses to make it a generator expression, but there's a minor syntactic optimization where if a generator expression is the sole function argument you can remove the parens. join(x.capitalize() for x in items[1:]) |
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.
Just a reminder to clean this up.