feat: add proper handling of query/path/body parameters for rest transport #702

yon-mg · 2020-11-20T22:46:13Z

Implementation and tests assume basic case of grpc transcoding for handling of all query/path parameters and body field.

These changes are dependent upon a PR in googleapis/proto-plus-python repo:
googleapis/proto-plus-python#164

…sport

software-dov

Looks good so far, just took an early readthrough.

gapic/schema/wrappers.py

software-dov · 2020-11-20T23:33:15Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+        potentialParams = {k: v for k, v in potentialParams.items() if v}
+        for i, (param_name, param_value) in enumerate(potentialParams.items()):
+            q = '?' if i == 0 else '&'
+            url += q + param_name + '=' + str(param_value).replace(' ', '+')


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(' ', '+'))

software-dov · 2020-11-20T23:33:46Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

-            url,
-            {%- if 'body' in method.http_opt.keys() %}
+            url
+            {%- if 'body' in method.http_opt.keys() %},


No need to use keys here, if 'body' in method.http_opt is more idiomatic.

software-dov · 2020-11-20T23:38:42Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+            '{{ field|camel_case }}': request.{{ field }},
+            {%- endfor %}
+        }
+        potentialParams = {k: v for k, v in potentialParams.items() if v}


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.

software-dov · 2020-11-20T23:41:52Z

gapic/utils/case.py

+        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:]])


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:])

software-dov · 2020-11-20T23:46:27Z

gapic/generator/generator.py

+                #for method in service.methods.values():
+                    #breakpoint()


Just a reminder to clean this up.

vam-google

Overall looks good, please try to get LGTM when Dov comes back and push this

vam-google · 2020-11-24T19:47:53Z

gapic/utils/case.py

@@ -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:


Are there any built-in/standard python functions which do the same? Please prefer using standard ones to custom, if there are any.

I'm fairly certain there is no standard library function for to camel-case.

vam-google · 2020-11-24T19:51:45Z

gapic/schema/wrappers.py

+    def path_params(self) -> Sequence[str]:
+        if self.http_opt is None:
+            return []
+        pattern = r'\{\w+\}'


Please add a comment that this handles grpc encoding in its simples case

…where relevant

codecov · 2020-12-01T00:50:29Z

Codecov Report

Merging #702 (a4f34e7) into master (bdd5a66) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #702   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         1578      1598   +20     
  Branches       320       324    +4     
=========================================
+ Hits          1578      1598   +20

Impacted Files	Coverage Δ
gapic/generator/generator.py	`100.00% <100.00%> (ø)`
gapic/schema/wrappers.py	`100.00% <100.00%> (ø)`
gapic/utils/__init__.py	`100.00% <100.00%> (ø)`
gapic/utils/case.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdd5a66...a4f34e7. Read the comment docs.

vchudnov-g

I flagged my more important comments with !!

It looks like this generator does not have golden files or baselines, correct? Too bad; it would be good to see a sample of the generated GAPIC showing off these features.

gapic/schema/wrappers.py

vchudnov-g · 2020-11-30T22:34:21Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

@@ -135,28 +135,42 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):

        {%- if 'body' in method.http_opt.keys() %}
        # Jsonify the input


nit: for clarity, maybe s/input/request/

vchudnov-g · 2020-11-30T22:43:47Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

-        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(


!! 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

It does not handle it but I have added a todo in the appropriate place in wrappers.Method. Pretty much anything dealing with full handling of grpc transcoding (or special cases outside grpc transcoding) is not yet taken care of in this PR as per the PR desc.

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.

vchudnov-g · 2020-11-30T22:54:05Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+            including_default_value_fields=False
+        )
+        {%- else %}
+        data = {{ method.input.ident }}.to_json(


Does request already have the body and query params stripped out at this point, so they don't get sent in two places?

As we clarified yesterday: params in the path may be (but don't have to be) repeated in the body if we're using "*", though we should never have a body: "foo" if foo is a path param. And query params are whatever's left over from what's not included in the path and body. So we just need to ensure no body params wind up as query params.

vchudnov-g · 2020-12-01T01:15:25Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+            '{{ field|camel_case }}': request.{{ field }},
+            {%- endfor %}
+        }
+        potentialParams = ((k, v) for k, v in potentialParams.items() if v)


This is fine, but another approach is to potentialParams=["{k}={v}".format(k=k, v=v) for k, v in ...] here and then url += "?{}".format("&".join(potentialParams)) below

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

vchudnov-g · 2020-12-01T01:25:13Z

gapic/utils/case.py

+
+
+def to_camel_case(s: str) -> str:
+    '''Convert any string to camel case.


nit: In this and the previous, pre-existing function, we're not touching spaces, right? Might be worth mentioning that.

vchudnov-g · 2020-12-01T01:29:07Z

gapic/utils/case.py

+        str: The string in lower camel case.
+    '''
+
+    items = re.split(r'[_-]', to_snake_case(s))


I'm surprised by the hyphen -. It almost seems like that should be handled by to_snake_case, though that's outside the scope of this PR. Does it come up in practice, that an input to this function has hyphens/kebab-case?

Opinion, @software-dov ?

It actually might handle it: I didn't check. As far as use, it doesn't get used but I wouldn't want a future user to attempt o try it and have it fail.

My vote: write a test for to_snake_case's behavior, add the functionality if the test fails, just split on '_'.

If to_snake_case does not handle it, could adding the functionality break existing clients? is there an easy way to check? (This is why I was suggesting "outside the scope of this PR")

I doubt it would break anything since it's additive but this is why I didn't change to_snake_case's behavior

But the generator has already produced libraries that were published, so if one of those had a hyphen somewhere, it made it through to_snake_case and changing that implementation could lead to an incompatible surface.

vchudnov-g · 2020-12-01T01:32:40Z

gapic/schema/wrappers.py

+        return [x[1:-1] for x in re.findall(pattern, self.http_opt['url'])]
+
+    @property
+    def query_params(self) -> Set[str]:


Just curious: why can't query_params and path_params return the same type, rather than a Set and a Sequence?

I suppose they could. Just thought it's more appropriate since ordering seems important for path_params but not for query_params.

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)

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.

gapic/schema/wrappers.py

vchudnov-g

Looking good. Two of my previous !! comments are still open. The truthiness one we should mark with a TODO, since we can't fully resolve it right now. The URL-encoding one, if it's not trivial to address now, we should mark with a TODO and address in a follow-up PR.

vchudnov-g · 2020-12-03T20:17:11Z

gapic/schema/wrappers.py

@@ -767,6 +767,29 @@ def http_opt(self) -> Optional[Dict[str, str]]:
        # TODO(yon-mg): enums for http verbs?
        return answer

+    @property
+    def path_params(self) -> Sequence[str]:
+        """Return the path parameters found in the http annotation url"""


WDYM? You're returning the {foo} which is a field path, right?

gapic/schema/wrappers.py

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

vchudnov-g

Looks good! Thanks for doing this!

For clarity when looking back at this commit, maybe you can expand in the merge commit description what the "basic case" is: path params in simple form, no body, no query param.

vchudnov-g · 2020-12-04T20:42:00Z

gapic/schema/wrappers.py

+        return [x[1:-1] for x in re.findall(pattern, self.http_opt['url'])]
+
+    @property
+    def query_params(self) -> Set[str]:


gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

vchudnov-g · 2020-12-04T20:47:52Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

            {%- for field in method.query_params %}
            '{{ field|camel_case }}': request.{{ field }},
            {%- endfor %}
        }
-        potentialParams = ((k, v) for k, v in potentialParams.items() if v)
-        for i, (param_name, param_value) in enumerate(potentialParams):
+        # TODO(yon-mg): further discussion needed whether 'python truthiness' is appropriate here


Thanks for the TODO on truthiness!

vchudnov-g · 2020-12-04T20:50:34Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+        #               discards default values
+        # TODO(yon-mg): add test for proper url encoded strings
+        query_params = ((k, v) for k, v in query_params.items() if v)
+        for i, (param_name, param_value) in enumerate(query_params):


nit: Do consider the string.Join suggestion from earlier (but it's certainly not a blocker)

🤖 I have created a release \*beep\* \*boop\* --- ## [0.37.0](https://www.github.com/googleapis/gapic-generator-python/compare/v0.36.0...v0.37.0) (2020-12-08) ### Features * add proper handling of query/path/body parameters for rest transport ([#702](https://www.github.com/googleapis/gapic-generator-python/issues/702)) ([6b2de5d](https://www.github.com/googleapis/gapic-generator-python/commit/6b2de5dd9fbf15e6b0a42b428b01eb03f1a3820a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).

yon-mg and others added 19 commits November 2, 2020 17:49

feat: add rest transport generation for clients

c1a6f89

feat: add rest transport generation for clients

bd7c32d

Merge branch 'master' of github.com:yon-mg/gapic-generator-python

cd3a7f1

feat: add transport flag

b41db31

refactor: moved template logic outside

ce69e7d

fix: small fixes in transport option logic

89fa08b

test: added unit test for transport flag

86f3a9c

test: add unit test for http option method

234cc09

test: add unit test for http option method branch

35ac276

fix: fix import paths

c8155a5

fix: style check issues

55a815c

fix: more style check issues

ac0bdef

fix: addressing pr reviews

e79e8c7

fix: typo in test_method

824ad11

fix: style check fixes

2d11e19

Merge branch 'master' into master

b5c6d06

feat: add proper handling of query/path/body parameters for rest tran…

221cd44

…sport

Merge branch 'master' of github.com:yon-mg/gapic-generator-python

475f903

Merge remote-tracking branch 'upstream/master'

f08ca32

yon-mg requested review from software-dov, vchudnov-g and vam-google November 20, 2020 22:46

yon-mg requested a review from a team as a code owner November 20, 2020 22:46

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 20, 2020

fix: typing errors

efa0ed6

yon-mg marked this pull request as draft November 20, 2020 23:15

Update case.py

d3e08c0

software-dov reviewed Nov 20, 2020

View reviewed changes

vam-google reviewed Nov 24, 2020

View reviewed changes

fix: minor changes adding a test, refactor and style check

df88ef9

yon-mg added 3 commits November 30, 2020 23:23

Merge branch 'master' of github.com:yon-mg/gapic-generator-python

67c6bd1

fix: camel_case bug with constant case

37e3d28

fix: to_camel_case to produce lower camel case instead of PascalCase …

c964fba

…where relevant

vchudnov-g requested changes Dec 1, 2020

View reviewed changes

software-dov reviewed Dec 1, 2020

View reviewed changes

gapic/schema/wrappers.py Outdated Show resolved Hide resolved

fix: addressing pr comments

85be88d

vchudnov-g reviewed Dec 3, 2020

View reviewed changes

fix: adding appropriate todos, addressing comments

5d1c6a3

vam-google marked this pull request as ready for review December 4, 2020 19:54

vchudnov-g approved these changes Dec 4, 2020

View reviewed changes

yon-mg and others added 2 commits December 8, 2020 00:49

fix: dataclass dependency issue

f6c64cc

Update wrappers.py

a4f34e7

software-dov approved these changes Dec 8, 2020

View reviewed changes

yon-mg merged commit 6b2de5d into googleapis:master Dec 8, 2020

busunkim96 linked an issue Jan 6, 2021 that may be closed by this pull request

Re-add HTTP transport and make it the fallback #267

Closed

vchudnov-g mentioned this pull request Feb 10, 2021

feat: add grpc transcoding + tests googleapis/python-api-core#146

Closed

This was referenced Jan 18, 2022

chore(master): release reverted #1133

Merged

chore(master): release 0.39.1 #1146

Closed

This was referenced Jan 26, 2022

chore(main): release 0.39.1 #1160

Closed

chore(main): release 0.39.1 #1162

Closed

chore(main): release 0.61.0 #1163

Merged

		@@ -135,28 +135,42 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):

		{%- if 'body' in method.http_opt.keys() %}
		# Jsonify the input



		def to_camel_case(s: str) -> str:
		'''Convert any string to camel case.

feat: add proper handling of query/path/body parameters for rest transport #702

feat: add proper handling of query/path/body parameters for rest transport #702

Conversation

yon-mg commented Nov 20, 2020 • edited Loading

software-dov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2020 • edited Loading

Codecov Report

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yon-mg Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yon-mg Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yon-mg commented Nov 20, 2020 •

edited

Loading

codecov bot commented Dec 1, 2020 •

edited

Loading

yon-mg Dec 1, 2020 •

edited

Loading

yon-mg Dec 3, 2020 •

edited

Loading