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

…sport (#702)

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

* fix: typing errors

* Update case.py

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

* fix: camel_case bug with constant case

* fix: to_camel_case to produce lower camel case instead of PascalCase where relevant

* fix: addressing pr comments

* fix: adding appropriate todos, addressing comments

* fix: dataclass dependency issue

* Update wrappers.py

Co-authored-by: Dov Shlachter <dovs@google.com>

.circleci/config.yml

@@ -364,6 +364,7 @@ jobs:
 
             cd ..
             nox -s showcase_mtls_alternative_templates
+  # TODO(yon-mg): add compute unit tests
   showcase-unit-3.6:
     docker:
       - image: python:3.6-slim

gapic/generator/generator.py

@@ -54,6 +54,7 @@ def __init__(self, opts: Options) -> None:
         # Add filters which templates require.
         self._env.filters["rst"] = utils.rst
         self._env.filters["snake_case"] = utils.to_snake_case
+        self._env.filters["camel_case"] = utils.to_camel_case
         self._env.filters["sort_lines"] = utils.sort_lines
         self._env.filters["wrap"] = utils.wrap
         self._env.filters["coerce_response_name"] = coerce_response_name

gapic/schema/wrappers.py

@@ -767,6 +767,31 @@ 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 path template"""
+        # TODO(yon-mg): fully implement grpc transcoding (currently only handles basic case)
+        if self.http_opt is None:
+            return []
+
+        pattern = r'\{(\w+)\}'
+        return re.findall(pattern, self.http_opt['url'])
+
+    @property
+    def query_params(self) -> Set[str]:
+        """Return query parameters for API call as determined by http annotation and grpc transcoding"""
+        # TODO(yon-mg): fully implement grpc transcoding (currently only handles basic case)
+        # TODO(yon-mg): remove this method and move logic to generated client
+        if self.http_opt is None:
+            return set()
+
+        params = set(self.path_params)
+        body = self.http_opt.get('body')
+        if body:
+            params.add(body)
+
+        return set(self.input.fields) - params
+
     # TODO(yon-mg): refactor as there may be more than one method signature
     @utils.cached_property
     def flattened_fields(self) -> Mapping[str, Field]:

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

@@ -133,31 +133,59 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):
         {%- endif %}
         """
 
-        {%- if 'body' in method.http_opt.keys() %}
-        # Jsonify the input
-        data = {{ method.output.ident }}.to_json(
-            {%- if method.http_opt['body'] == '*' %}
+        {# TODO(yon-mg): refactor when implementing grpc transcoding 
+                         - parse request pb & assign body, path params
+                         - shove leftovers into query params
+                         - make sure dotted nested fields preserved
+                         - format url and send the request
+        #}
+        {%- if 'body' in method.http_opt %}
+        # Jsonify the request body
+        {%- if method.http_opt['body'] != '*' %}
+        body = {{ method.input.fields[method.http_opt['body']].type.ident }}.to_json(
+            request.{{ method.http_opt['body'] }},
+            including_default_value_fields=False
+        )
+        {%- else %}
+        body = {{ method.input.ident }}.to_json(
             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
-        # Send the request
+        #               current impl assumes basic case of grpc transcoding
         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 %}
         )
+
+        {# TODO(yon-mg): move all query param logic out of wrappers into here to handle
+                         nested fields correctly (can't just use set of top level fields
+        #}
+        # TODO(yon-mg): handle nested fields corerctly rather than using only top level fields
+        #               not required for GCE
+        query_params = {
+            {%- for field in method.query_params %}
+            '{{ field|camel_case }}': request.{{ field }},
+            {%- endfor %}
+        }
+        # TODO(yon-mg): further discussion needed whether 'python truthiness' is appropriate here
+        #               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):
+            q = '?' if i == 0 else '&'
+            url += "{q}{name}={value}".format(q=q, name=param_name, value=param_value.replace(' ', '+'))
+
+        # Send the request
         {% if not method.void %}response = {% endif %}self._session.{{ method.http_opt['verb'] }}(
-            url,
-            {%- if 'body' in method.http_opt.keys() %}
-            json=data,
+            url
+            {%- if 'body' in method.http_opt %},
+            json=body,
             {%- endif %}
         )
         {%- if not method.void %}

gapic/utils/__init__.py

@@ -14,6 +14,7 @@
 
 from gapic.utils.cache import cached_property
 from gapic.utils.case import to_snake_case
+from gapic.utils.case import to_camel_case
 from gapic.utils.code import empty
 from gapic.utils.code import nth
 from gapic.utils.code import partition
@@ -38,6 +39,7 @@
     'rst',
     'sort_lines',
     'to_snake_case',
+    'to_camel_case',
     'to_valid_filename',
     'to_valid_module_name',
     'wrap',

gapic/utils/case.py

@@ -45,3 +45,19 @@ def to_snake_case(s: str) -> str:
 
     # Done; return the camel-cased string.
     return s.lower()
+
+
+def to_camel_case(s: str) -> str:
+    '''Convert any string to camel case.
+
+    This is provided to templates as the ``camel_case`` filter.
+
+    Args:
+        s (str): The input string, provided in any sane case system
+
+    Returns:
+        str: The string in lower camel case.
+    '''
+
+    items = re.split(r'[_-]', to_snake_case(s))
+    return items[0].lower() + "".join(x.capitalize() for x in items[1:])

noxfile.py

@@ -52,6 +52,10 @@ def unit(session):
     )
 
 
+# TODO(yon-mg): -add compute context manager that includes rest transport
+#               -add compute unit tests
+#               (to test against temporarily while rest transport is incomplete)
+#               (to be removed once all features are complete)
 @contextmanager
 def showcase_library(
     session, templates="DEFAULT", other_opts: typing.Iterable[str] = ()
@@ -87,6 +91,8 @@ def showcase_library(
 
         # Write out a client library for Showcase.
         template_opt = f"python-gapic-templates={templates}"
+        # TODO(yon-mg): add "transports=grpc+rest" when all rest features required for
+        #               Showcase are implemented i.e. (grpc transcoding, LROs, etc)
         opts = "--python_gapic_opt="
         opts += ",".join(other_opts + (f"{template_opt}",))
         cmd_tup = (

setup.py

@@ -51,9 +51,9 @@
         "protobuf >= 3.12.0",
         "pypandoc >= 1.4",
         "PyYAML >= 5.1.1",
-        "dataclasses<0.8; python_version < '3.7'"
+        "dataclasses < 0.8; python_version < '3.7'"
     ),
-    extras_require={':python_version<"3.7"': ("dataclasses >= 0.4",),},
+    extras_require={':python_version<"3.7"': ("dataclasses >= 0.4, < 0.8",),},
     tests_require=("pyfakefs >= 3.6",),
     python_requires=">=3.6",
     classifiers=(

tests/unit/schema/wrappers/test_method.py

@@ -279,6 +279,57 @@ def test_method_http_opt_no_http_rule():
     assert method.http_opt == None
 
 
+def test_method_path_params():
+    # tests only the basic case of grpc transcoding
+    http_rule = http_pb2.HttpRule(post='/v1/{project}/topics')
+    method = make_method('DoSomething', http_rule=http_rule)
+    assert method.path_params == ['project']
+
+
+def test_method_path_params_no_http_rule():
+    method = make_method('DoSomething')
+    assert method.path_params == []
+
+
+def test_method_query_params():
+    # tests only the basic case of grpc transcoding
+    http_rule = http_pb2.HttpRule(
+        post='/v1/{project}/topics',
+        body='address'
+    )
+    input_message = make_message(
+        'MethodInput',
+        fields=(
+            make_field('region'),
+            make_field('project'),
+            make_field('address')
+        )
+    )
+    method = make_method('DoSomething', http_rule=http_rule,
+                         input_message=input_message)
+    assert method.query_params == {'region'}
+
+
+def test_method_query_params_no_body():
+    # tests only the basic case of grpc transcoding
+    http_rule = http_pb2.HttpRule(post='/v1/{project}/topics')
+    input_message = make_message(
+        'MethodInput',
+        fields=(
+            make_field('region'),
+            make_field('project'),
+        )
+    )
+    method = make_method('DoSomething', http_rule=http_rule,
+                         input_message=input_message)
+    assert method.query_params == {'region'}
+
+
+def test_method_query_params_no_http_rule():
+    method = make_method('DoSomething')
+    assert method.query_params == set()
+
+
 def test_method_idempotent_yes():
     http_rule = http_pb2.HttpRule(get='/v1/{parent=projects/*}/topics')
     method = make_method('DoSomething', http_rule=http_rule)

tests/unit/utils/test_case.py

@@ -25,3 +25,19 @@ def test_camel_to_snake():
 
 def test_constant_to_snake():
     assert case.to_snake_case('CONSTANT_CASE_THING') == 'constant_case_thing'
+
+
+def test_pascal_to_camel():
+    assert case.to_camel_case('PascalCaseThing') == 'pascalCaseThing'
+
+
+def test_snake_to_camel():
+    assert case.to_camel_case('snake_case_thing') == 'snakeCaseThing'
+
+
+def test_constant_to_camel():
+    assert case.to_camel_case('CONSTANT_CASE_THING') == 'constantCaseThing'
+
+
+def test_kebab_to_camel():
+    assert case.to_camel_case('kebab-case-thing') == 'kebabCaseThing'