From 41d2e001c8a97e391097ba4e1e157f3ac656da72 Mon Sep 17 00:00:00 2001 From: Kenneth Bandes Date: Tue, 14 Sep 2021 17:46:39 +0000 Subject: [PATCH 1/4] Support alternative http bindings in the gapic schema. --- gapic/schema/wrappers.py | 92 ++++++++++++++++++----- tests/unit/schema/wrappers/test_method.py | 78 +++++++++++++++++++ 2 files changed, 150 insertions(+), 20 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 8c2313f8a7..6044ecd260 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -36,6 +36,7 @@ from google.api import annotations_pb2 # type: ignore from google.api import client_pb2 from google.api import field_behavior_pb2 +from google.api import http_pb2 from google.api import resource_pb2 from google.api_core import exceptions # type: ignore from google.protobuf import descriptor_pb2 # type: ignore @@ -821,33 +822,84 @@ def field_headers(self) -> Sequence[str]: return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ()) + def http_rule_to_tuple(self, http_rule: http_pb2.HttpRule) -> Tuple[Optional[str], Optional[str], Optional[str]]: + """Represent salient info in an http rule as a tuple. + + Args: + http_rule: the http option message to examine. + + Returns: + A tuple of (method, uri pattern, body or None), + or None if no method is specified. + """ + + http_dict: Mapping[str, str] + + method = http_rule.WhichOneof('pattern') + if method is None or method == 'custom': + return (None, None, None) + + uri = getattr(http_rule, method) + if not uri: + return (None, None, None) + body = http_rule.body if http_rule.body else None + return (method, uri, body) + + @property + def http_options(self) -> List[Dict[str, str]]: + """Return a list of the http options for this method. + + e.g. [{'method': 'post' + 'uri': '/some/path' + 'body': '*'},] + + """ + http = self.options.Extensions[annotations_pb2.http] + # shallow copy is fine here (elements are not modified) + http_options = list(http.additional_bindings) + # Main pattern comes first + http_options.insert(0, http) + answers : List[Dict[str, str]] = [] + + for http_rule in http_options: + method, uri, body = self.http_rule_to_tuple(http_rule) + if method is None: + continue + answer : Dict[str, str] = {} + answer['method'] = method + answer['uri'] = uri + if body is not None: + answer['body'] = body + answers.append(answer) + return answers + @property def http_opt(self) -> Optional[Dict[str, str]]: - """Return the http option for this method. + """Return the (main) http option for this method. e.g. {'verb': 'post' 'url': '/some/path' 'body': '*'} - """ - http: List[Tuple[descriptor_pb2.FieldDescriptorProto, str]] - http = self.options.Extensions[annotations_pb2.http].ListFields() - - if len(http) < 1: - return None - - http_method = http[0] - answer: Dict[str, str] = { - 'verb': http_method[0].name, - 'url': http_method[1], - } - if len(http) > 1: - body_spec = http[1] - answer[body_spec[0].name] = body_spec[1] - - # TODO(yon-mg): handle nested fields & fields past body i.e. 'additional bindings' - # TODO(yon-mg): enums for http verbs? - return answer + """ + http: List[Tuple[descriptor_pb2.FieldDescriptorProto, str]] + http = self.options.Extensions[annotations_pb2.http].ListFields() + + if len(http) < 1: + return None + + http_method = http[0] + answer: Dict[str, str] = { + 'verb': http_method[0].name, + 'url': http_method[1], + } + if len(http) > 1: + body_spec = http[1] + answer[body_spec[0].name] = body_spec[1] + + # TODO(yon-mg): handle nested fields & fields past body i.e. 'additional bindings' + # TODO(yon-mg): enums for http verbs? + return answer @property def path_params(self) -> Sequence[str]: diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index c13a9afb28..a61c342fd8 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -328,6 +328,84 @@ def test_method_path_params_no_http_rule(): assert method.path_params == [] +def test_method_http_options(): + verbs = [ + 'get', + 'put', + 'post', + 'delete', + 'patch' + ] + for v in verbs: + http_rule = http_pb2.HttpRule(**{v:'/v1/{parent=projects/*}/topics'}) + method = make_method('DoSomething', http_rule=http_rule) + assert method.http_options == [{ + 'method': v, + 'uri':'/v1/{parent=projects/*}/topics' + }] + + +def test_method_http_options_empty_http_rule(): + http_rule = http_pb2.HttpRule() + method = make_method('DoSomething', http_rule=http_rule) + assert method.http_options == [] + + http_rule = http_pb2.HttpRule(get='') + method = make_method('DoSomething', http_rule=http_rule) + assert method.http_options == [] + + +def test_method_http_options_no_http_rule(): + method = make_method('DoSomething') + assert method.path_params == [] + + +def test_method_http_options_body(): + http_rule = http_pb2.HttpRule( + post='/v1/{parent=projects/*}/topics', + body='*' + ) + method = make_method('DoSomething', http_rule=http_rule) + assert method.http_options == [{ + 'method': 'post', + 'uri': '/v1/{parent=projects/*}/topics', + 'body': '*' + }] + +def test_method_http_options_additional_bindings(): + http_rule = http_pb2.HttpRule( + post='/v1/{parent=projects/*}/topics', + body='*', + additional_bindings=[ + http_pb2.HttpRule( + post='/v1/{parent=projects/*/regions/*}/topics', + body='*', + ), + http_pb2.HttpRule( + post='/v1/projects/p1/topics', + body='body_field', + ), + ] + ) + method = make_method('DoSomething', http_rule=http_rule) + assert len(method.http_options) == 3 + assert { + 'method':'post', + 'uri':'/v1/{parent=projects/*}/topics', + 'body':'*' + } in method.http_options + assert { + 'method':'post', + 'uri':'/v1/{parent=projects/*/regions/*}/topics', + 'body':'*' + } in method.http_options + assert { + 'method':'post', + 'uri':'/v1/projects/p1/topics', + 'body':'body_field' + } in method.http_options + + def test_method_query_params(): # tests only the basic case of grpc transcoding http_rule = http_pb2.HttpRule( From 4ed24d58adbea0390381a3858f52e628e77ef80e Mon Sep 17 00:00:00 2001 From: Kenneth Bandes Date: Tue, 14 Sep 2021 19:40:58 +0000 Subject: [PATCH 2/4] Fixed some style and pytype check failures. --- gapic/schema/wrappers.py | 102 +++++++++++----------- tests/unit/schema/wrappers/test_method.py | 23 ++--- 2 files changed, 63 insertions(+), 62 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 6044ecd260..8b663d3b1f 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -823,27 +823,27 @@ def field_headers(self) -> Sequence[str]: return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ()) def http_rule_to_tuple(self, http_rule: http_pb2.HttpRule) -> Tuple[Optional[str], Optional[str], Optional[str]]: - """Represent salient info in an http rule as a tuple. + """Represent salient info in an http rule as a tuple. - Args: - http_rule: the http option message to examine. + Args: + http_rule: the http option message to examine. - Returns: - A tuple of (method, uri pattern, body or None), - or None if no method is specified. - """ + Returns: + A tuple of (method, uri pattern, body or None), + or None if no method is specified. + """ - http_dict: Mapping[str, str] + http_dict: Mapping[str, str] - method = http_rule.WhichOneof('pattern') - if method is None or method == 'custom': - return (None, None, None) + method = http_rule.WhichOneof('pattern') + if method is None or method == 'custom': + return (None, None, None) - uri = getattr(http_rule, method) - if not uri: - return (None, None, None) - body = http_rule.body if http_rule.body else None - return (method, uri, body) + uri = getattr(http_rule, method) + if not uri: + return (None, None, None) + body = http_rule.body if http_rule.body else None + return (method, uri, body) @property def http_options(self) -> List[Dict[str, str]]: @@ -859,47 +859,47 @@ def http_options(self) -> List[Dict[str, str]]: http_options = list(http.additional_bindings) # Main pattern comes first http_options.insert(0, http) - answers : List[Dict[str, str]] = [] + answers: List[Dict[str, str]] = [] for http_rule in http_options: - method, uri, body = self.http_rule_to_tuple(http_rule) - if method is None: - continue - answer : Dict[str, str] = {} - answer['method'] = method - answer['uri'] = uri - if body is not None: - answer['body'] = body - answers.append(answer) + method, uri, body = self.http_rule_to_tuple(http_rule) + if method is None or uri is None: + continue + answer: Dict[str, str] = {} + answer['method'] = method + answer['uri'] = uri + if body is not None: + answer['body'] = body + answers.append(answer) return answers @property def http_opt(self) -> Optional[Dict[str, str]]: - """Return the (main) http option for this method. - - e.g. {'verb': 'post' - 'url': '/some/path' - 'body': '*'} - - """ - http: List[Tuple[descriptor_pb2.FieldDescriptorProto, str]] - http = self.options.Extensions[annotations_pb2.http].ListFields() - - if len(http) < 1: - return None - - http_method = http[0] - answer: Dict[str, str] = { - 'verb': http_method[0].name, - 'url': http_method[1], - } - if len(http) > 1: - body_spec = http[1] - answer[body_spec[0].name] = body_spec[1] - - # TODO(yon-mg): handle nested fields & fields past body i.e. 'additional bindings' - # TODO(yon-mg): enums for http verbs? - return answer + """Return the (main) http option for this method. + + e.g. {'verb': 'post' + 'url': '/some/path' + 'body': '*'} + + """ + http: List[Tuple[descriptor_pb2.FieldDescriptorProto, str]] + http = self.options.Extensions[annotations_pb2.http].ListFields() + + if len(http) < 1: + return None + + http_method = http[0] + answer: Dict[str, str] = { + 'verb': http_method[0].name, + 'url': http_method[1], + } + if len(http) > 1: + body_spec = http[1] + answer[body_spec[0].name] = body_spec[1] + + # TODO(yon-mg): handle nested fields & fields past body i.e. 'additional bindings' + # TODO(yon-mg): enums for http verbs? + return answer @property def path_params(self) -> Sequence[str]: diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index a61c342fd8..73cde0488e 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -337,11 +337,11 @@ def test_method_http_options(): 'patch' ] for v in verbs: - http_rule = http_pb2.HttpRule(**{v:'/v1/{parent=projects/*}/topics'}) + http_rule = http_pb2.HttpRule(**{v: '/v1/{parent=projects/*}/topics'}) method = make_method('DoSomething', http_rule=http_rule) assert method.http_options == [{ 'method': v, - 'uri':'/v1/{parent=projects/*}/topics' + 'uri': '/v1/{parent=projects/*}/topics' }] @@ -372,6 +372,7 @@ def test_method_http_options_body(): 'body': '*' }] + def test_method_http_options_additional_bindings(): http_rule = http_pb2.HttpRule( post='/v1/{parent=projects/*}/topics', @@ -390,19 +391,19 @@ def test_method_http_options_additional_bindings(): method = make_method('DoSomething', http_rule=http_rule) assert len(method.http_options) == 3 assert { - 'method':'post', - 'uri':'/v1/{parent=projects/*}/topics', - 'body':'*' + 'method': 'post', + 'uri': '/v1/{parent=projects/*}/topics', + 'body': '*' } in method.http_options assert { - 'method':'post', - 'uri':'/v1/{parent=projects/*/regions/*}/topics', - 'body':'*' + 'method': 'post', + 'uri': '/v1/{parent=projects/*/regions/*}/topics', + 'body': '*' } in method.http_options assert { - 'method':'post', - 'uri':'/v1/projects/p1/topics', - 'body':'body_field' + 'method': 'post', + 'uri': '/v1/projects/p1/topics', + 'body': 'body_field' } in method.http_options From 105b2e34715ed127c007ab8b26bd0b804ff3f40c Mon Sep 17 00:00:00 2001 From: Kenneth Bandes Date: Tue, 14 Sep 2021 20:37:41 +0000 Subject: [PATCH 3/4] fix: corrected code coverage issue. --- gapic/schema/wrappers.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 8b663d3b1f..8c34fdcd18 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -822,7 +822,7 @@ def field_headers(self) -> Sequence[str]: return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ()) - def http_rule_to_tuple(self, http_rule: http_pb2.HttpRule) -> Tuple[Optional[str], Optional[str], Optional[str]]: + def http_rule_to_tuple(self, http_rule: http_pb2.HttpRule) -> Tuple[str, str, str]: """Represent salient info in an http rule as a tuple. Args: @@ -837,11 +837,11 @@ def http_rule_to_tuple(self, http_rule: http_pb2.HttpRule) -> Tuple[Optional[str method = http_rule.WhichOneof('pattern') if method is None or method == 'custom': - return (None, None, None) + return ('', '', '') uri = getattr(http_rule, method) if not uri: - return (None, None, None) + return ('', '', '') body = http_rule.body if http_rule.body else None return (method, uri, body) @@ -863,12 +863,12 @@ def http_options(self) -> List[Dict[str, str]]: for http_rule in http_options: method, uri, body = self.http_rule_to_tuple(http_rule) - if method is None or uri is None: + if not method: continue answer: Dict[str, str] = {} answer['method'] = method answer['uri'] = uri - if body is not None: + if body: answer['body'] = body answers.append(answer) return answers From 4167b093eadf29d5eea43512ac49d10d9caea6e5 Mon Sep 17 00:00:00 2001 From: Kenneth Bandes Date: Wed, 15 Sep 2021 02:35:43 +0000 Subject: [PATCH 4/4] refactor: made HttpRule a dataclass. --- gapic/schema/wrappers.py | 75 ++++++++--------------- tests/unit/schema/wrappers/test_method.py | 40 ++++++------ 2 files changed, 48 insertions(+), 67 deletions(-) diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 8c34fdcd18..2af844d0a1 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -707,6 +707,27 @@ class RetryInfo: retryable_exceptions: FrozenSet[exceptions.GoogleAPICallError] +@dataclasses.dataclass(frozen=True) +class HttpRule: + """Representation of the method's http bindings.""" + method: str + uri: str + body: Optional[str] + + @classmethod + def try_parse_http_rule(cls, http_rule) -> Optional['HttpRule']: + method = http_rule.WhichOneof("pattern") + if method is None or method == "custom": + return None + + uri = getattr(http_rule, method) + if not uri: + return None + + body = http_rule.body or None + return cls(method, uri, body) + + @dataclasses.dataclass(frozen=True) class Method: """Description of a method (defined with the ``rpc`` keyword).""" @@ -822,56 +843,14 @@ def field_headers(self) -> Sequence[str]: return next((tuple(pattern.findall(verb)) for verb in potential_verbs if verb), ()) - def http_rule_to_tuple(self, http_rule: http_pb2.HttpRule) -> Tuple[str, str, str]: - """Represent salient info in an http rule as a tuple. - - Args: - http_rule: the http option message to examine. - - Returns: - A tuple of (method, uri pattern, body or None), - or None if no method is specified. - """ - - http_dict: Mapping[str, str] - - method = http_rule.WhichOneof('pattern') - if method is None or method == 'custom': - return ('', '', '') - - uri = getattr(http_rule, method) - if not uri: - return ('', '', '') - body = http_rule.body if http_rule.body else None - return (method, uri, body) - @property - def http_options(self) -> List[Dict[str, str]]: - """Return a list of the http options for this method. - - e.g. [{'method': 'post' - 'uri': '/some/path' - 'body': '*'},] - - """ + def http_options(self) -> List[HttpRule]: + """Return a list of the http bindings for this method.""" http = self.options.Extensions[annotations_pb2.http] - # shallow copy is fine here (elements are not modified) - http_options = list(http.additional_bindings) - # Main pattern comes first - http_options.insert(0, http) - answers: List[Dict[str, str]] = [] - - for http_rule in http_options: - method, uri, body = self.http_rule_to_tuple(http_rule) - if not method: - continue - answer: Dict[str, str] = {} - answer['method'] = method - answer['uri'] = uri - if body: - answer['body'] = body - answers.append(answer) - return answers + http_options = [http] + list(http.additional_bindings) + opt_gen = (HttpRule.try_parse_http_rule(http_rule) + for http_rule in http_options) + return [rule for rule in opt_gen if rule] @property def http_opt(self) -> Optional[Dict[str, str]]: diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 73cde0488e..00ade8aefb 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -13,6 +13,7 @@ # limitations under the License. import collections +import dataclasses from typing import Sequence from google.api import field_behavior_pb2 @@ -339,9 +340,10 @@ def test_method_http_options(): for v in verbs: http_rule = http_pb2.HttpRule(**{v: '/v1/{parent=projects/*}/topics'}) method = make_method('DoSomething', http_rule=http_rule) - assert method.http_options == [{ + assert [dataclasses.asdict(http) for http in method.http_options] == [{ 'method': v, - 'uri': '/v1/{parent=projects/*}/topics' + 'uri': '/v1/{parent=projects/*}/topics', + 'body': None }] @@ -366,7 +368,7 @@ def test_method_http_options_body(): body='*' ) method = make_method('DoSomething', http_rule=http_rule) - assert method.http_options == [{ + assert [dataclasses.asdict(http) for http in method.http_options] == [{ 'method': 'post', 'uri': '/v1/{parent=projects/*}/topics', 'body': '*' @@ -389,22 +391,22 @@ def test_method_http_options_additional_bindings(): ] ) method = make_method('DoSomething', http_rule=http_rule) - assert len(method.http_options) == 3 - assert { - 'method': 'post', - 'uri': '/v1/{parent=projects/*}/topics', - 'body': '*' - } in method.http_options - assert { - 'method': 'post', - 'uri': '/v1/{parent=projects/*/regions/*}/topics', - 'body': '*' - } in method.http_options - assert { - 'method': 'post', - 'uri': '/v1/projects/p1/topics', - 'body': 'body_field' - } in method.http_options + assert [dataclasses.asdict(http) for http in method.http_options] == [ + { + 'method': 'post', + 'uri': '/v1/{parent=projects/*}/topics', + 'body': '*' + }, + { + 'method': 'post', + 'uri': '/v1/{parent=projects/*/regions/*}/topics', + 'body': '*' + }, + { + 'method': 'post', + 'uri': '/v1/projects/p1/topics', + 'body': 'body_field' + }] def test_method_query_params():