From c3ad8ea67447e3d8a1154d7a9221e116f60d425a Mon Sep 17 00:00:00 2001 From: Vadym Matsishevskyi <25311427+vam-google@users.noreply.github.com> Date: Tue, 30 Aug 2022 11:02:04 -0700 Subject: [PATCH] feat: Make grpc transcode logic work in terms of protobuf python objects (#428) * feat: Make grpc transcode logic work in terms of protobuf python objects (for context: [gRPC Transcoding](https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44)) Previously it worked on dictionaries only, but that causes problems. In GAPIC the dictionaries are created through the same logic as JSON (there is no better built-in way), thus applying [protobuf json mapping](https://developers.google.com/protocol-buffers/docs/proto3#json) conversion logic in the process. Unfortunately converting a protobuf object to a dictionary and to JSON, although similar, are not the same thing. Specifically the `Timestamp`, `Duration`, `FieldMask`, `uint64`, `int64`, and `*Value` protobuf messages are converted to strings for JSON (instead of being properly converted to dicts for most of those types, and `int64/uint64` converted to `int` respectively). As a result a rountrip that GAPIC was relying on (protobuf object -> dict -> transcode -> protobuf object) did not work properly. For example, when converted to dictionary, every int64 field would be converted to `string` (because it is what proto-JSON mapping spec requires), but later, when we need to rebuild a message from a transcoded dictionary that would fail with the following error: ``` TypeError: '0' has type str, but expected one of: int ``` Note, `*Rules` thing from proto-plus does not help, becuase the type may happen inside common native protobuf stub messsages (like `google.type.Money`), fields of which are outside of scope of the proto-plus custom conversion logic. Also, this change greatly simplifies the procedure of transcodding, eliminating multiple conversion steps (to and from dictionaries multiple times) making the whole logic significanly more efficient (python gapics are nutoriously known to be slow due to proto-plus stuff, so efficiency is important) and robust (JSON conversion logic does not interfere anymore with pure protobuf objects grpc transcoding) * reformat code using black * reformat code according to flake8 Co-authored-by: Anthonios Partheniou --- google/api_core/path_template.py | 60 ++++-- tests/unit/test_path_template.py | 308 ++++++++++++++++++++++++++++--- 2 files changed, 325 insertions(+), 43 deletions(-) diff --git a/google/api_core/path_template.py b/google/api_core/path_template.py index a99a4c81..2639459a 100644 --- a/google/api_core/path_template.py +++ b/google/api_core/path_template.py @@ -176,7 +176,7 @@ def get_field(request, field): """Get the value of a field from a given dictionary. Args: - request (dict): A dictionary object. + request (dict | Message): A dictionary or a Message object. field (str): The key to the request in dot notation. Returns: @@ -184,10 +184,12 @@ def get_field(request, field): """ parts = field.split(".") value = request + for part in parts: if not isinstance(value, dict): - return - value = value.get(part) + value = getattr(value, part, None) + else: + value = value.get(part) if isinstance(value, dict): return return value @@ -197,19 +199,27 @@ def delete_field(request, field): """Delete the value of a field from a given dictionary. Args: - request (dict): A dictionary object. + request (dict | Message): A dictionary object or a Message. field (str): The key to the request in dot notation. """ parts = deque(field.split(".")) while len(parts) > 1: - if not isinstance(request, dict): - return part = parts.popleft() - request = request.get(part) + if not isinstance(request, dict): + if hasattr(request, part): + request = getattr(request, part, None) + else: + return + else: + request = request.get(part) part = parts.popleft() if not isinstance(request, dict): - return - request.pop(part, None) + if hasattr(request, part): + request.ClearField(part) + else: + return + else: + request.pop(part, None) def validate(tmpl, path): @@ -237,7 +247,7 @@ def validate(tmpl, path): return True if re.match(pattern, path) is not None else False -def transcode(http_options, **request_kwargs): +def transcode(http_options, message=None, **request_kwargs): """Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here, https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44-L312 @@ -248,18 +258,20 @@ def transcode(http_options, **request_kwargs): 'body' (str): The body field name (optional) (This is a simplified representation of the proto option `google.api.http`) + message (Message) : A request object (optional) request_kwargs (dict) : A dict representing the request object Returns: dict: The transcoded request with these keys, 'method' (str) : The http method 'uri' (str) : The expanded uri - 'body' (dict) : A dict representing the body (optional) - 'query_params' (dict) : A dict mapping query parameter variables and values + 'body' (dict | Message) : A dict or a Message representing the body (optional) + 'query_params' (dict | Message) : A dict or Message mapping query parameter variables and values Raises: ValueError: If the request does not match the given template. """ + transcoded_value = message or request_kwargs for http_option in http_options: request = {} @@ -268,27 +280,35 @@ def transcode(http_options, **request_kwargs): path_fields = [ match.group("name") for match in _VARIABLE_RE.finditer(uri_template) ] - path_args = {field: get_field(request_kwargs, field) for field in path_fields} + path_args = {field: get_field(transcoded_value, field) for field in path_fields} request["uri"] = expand(uri_template, **path_args) - # Remove fields used in uri path from request - leftovers = copy.deepcopy(request_kwargs) - for path_field in path_fields: - delete_field(leftovers, path_field) if not validate(uri_template, request["uri"]) or not all(path_args.values()): continue + # Remove fields used in uri path from request + leftovers = copy.deepcopy(transcoded_value) + for path_field in path_fields: + delete_field(leftovers, path_field) + # Assign body and query params body = http_option.get("body") if body: if body == "*": request["body"] = leftovers - request["query_params"] = {} + if message: + request["query_params"] = message.__class__() + else: + request["query_params"] = {} else: try: - request["body"] = leftovers.pop(body) - except KeyError: + if message: + request["body"] = getattr(leftovers, body) + delete_field(leftovers, body) + else: + request["body"] = leftovers.pop(body) + except (KeyError, AttributeError): continue request["query_params"] = leftovers else: diff --git a/tests/unit/test_path_template.py b/tests/unit/test_path_template.py index c12b35fc..73d351c0 100644 --- a/tests/unit/test_path_template.py +++ b/tests/unit/test_path_template.py @@ -17,6 +17,7 @@ import mock import pytest +from google.api import auth_pb2 from google.api_core import path_template @@ -171,113 +172,264 @@ def test__replace_variable_with_pattern(): @pytest.mark.parametrize( - "http_options, request_kwargs, expected_result", + "http_options, message, request_kwargs, expected_result", [ [ [["get", "/v1/no/template", ""]], + None, {"foo": "bar"}, ["get", "/v1/no/template", {}, {"foo": "bar"}], ], + [ + [["get", "/v1/no/template", ""]], + auth_pb2.AuthenticationRule(selector="bar"), + {}, + [ + "get", + "/v1/no/template", + None, + auth_pb2.AuthenticationRule(selector="bar"), + ], + ], # Single templates [ [["get", "/v1/{field}", ""]], + None, {"field": "parent"}, ["get", "/v1/parent", {}, {}], ], + [ + [["get", "/v1/{selector}", ""]], + auth_pb2.AuthenticationRule(selector="parent"), + {}, + ["get", "/v1/parent", None, auth_pb2.AuthenticationRule()], + ], [ [["get", "/v1/{field.sub}", ""]], + None, {"field": {"sub": "parent"}, "foo": "bar"}, ["get", "/v1/parent", {}, {"field": {}, "foo": "bar"}], ], + [ + [["get", "/v1/{oauth.canonical_scopes}", ""]], + auth_pb2.AuthenticationRule( + selector="bar", + oauth=auth_pb2.OAuthRequirements(canonical_scopes="parent"), + ), + {}, + [ + "get", + "/v1/parent", + None, + auth_pb2.AuthenticationRule( + selector="bar", oauth=auth_pb2.OAuthRequirements() + ), + ], + ], ], ) -def test_transcode_base_case(http_options, request_kwargs, expected_result): +def test_transcode_base_case(http_options, message, request_kwargs, expected_result): http_options, expected_result = helper_test_transcode(http_options, expected_result) - result = path_template.transcode(http_options, **request_kwargs) + result = path_template.transcode(http_options, message, **request_kwargs) assert result == expected_result @pytest.mark.parametrize( - "http_options, request_kwargs, expected_result", + "http_options, message, request_kwargs, expected_result", [ [ [["get", "/v1/{field.subfield}", ""]], + None, {"field": {"subfield": "parent"}, "foo": "bar"}, ["get", "/v1/parent", {}, {"field": {}, "foo": "bar"}], ], + [ + [["get", "/v1/{oauth.canonical_scopes}", ""]], + auth_pb2.AuthenticationRule( + selector="bar", + oauth=auth_pb2.OAuthRequirements(canonical_scopes="parent"), + ), + {}, + [ + "get", + "/v1/parent", + None, + auth_pb2.AuthenticationRule( + selector="bar", oauth=auth_pb2.OAuthRequirements() + ), + ], + ], [ [["get", "/v1/{field.subfield.subsubfield}", ""]], + None, {"field": {"subfield": {"subsubfield": "parent"}}, "foo": "bar"}, ["get", "/v1/parent", {}, {"field": {"subfield": {}}, "foo": "bar"}], ], [ [["get", "/v1/{field.subfield1}/{field.subfield2}", ""]], + None, {"field": {"subfield1": "parent", "subfield2": "child"}, "foo": "bar"}, ["get", "/v1/parent/child", {}, {"field": {}, "foo": "bar"}], ], + [ + [["get", "/v1/{selector}/{oauth.canonical_scopes}", ""]], + auth_pb2.AuthenticationRule( + selector="parent", + oauth=auth_pb2.OAuthRequirements(canonical_scopes="child"), + ), + {"field": {"subfield1": "parent", "subfield2": "child"}, "foo": "bar"}, + [ + "get", + "/v1/parent/child", + None, + auth_pb2.AuthenticationRule(oauth=auth_pb2.OAuthRequirements()), + ], + ], ], ) -def test_transcode_subfields(http_options, request_kwargs, expected_result): +def test_transcode_subfields(http_options, message, request_kwargs, expected_result): http_options, expected_result = helper_test_transcode(http_options, expected_result) - result = path_template.transcode(http_options, **request_kwargs) + result = path_template.transcode(http_options, message, **request_kwargs) assert result == expected_result @pytest.mark.parametrize( - "http_options, request_kwargs, expected_result", + "http_options, message, request_kwargs, expected_result", [ # Single segment wildcard [ [["get", "/v1/{field=*}", ""]], + None, {"field": "parent"}, ["get", "/v1/parent", {}, {}], ], + [ + [["get", "/v1/{selector=*}", ""]], + auth_pb2.AuthenticationRule(selector="parent"), + {}, + ["get", "/v1/parent", None, auth_pb2.AuthenticationRule()], + ], [ [["get", "/v1/{field=a/*/b/*}", ""]], + None, {"field": "a/parent/b/child", "foo": "bar"}, ["get", "/v1/a/parent/b/child", {}, {"foo": "bar"}], ], + [ + [["get", "/v1/{selector=a/*/b/*}", ""]], + auth_pb2.AuthenticationRule( + selector="a/parent/b/child", allow_without_credential=True + ), + {}, + [ + "get", + "/v1/a/parent/b/child", + None, + auth_pb2.AuthenticationRule(allow_without_credential=True), + ], + ], # Double segment wildcard [ [["get", "/v1/{field=**}", ""]], + None, {"field": "parent/p1"}, ["get", "/v1/parent/p1", {}, {}], ], + [ + [["get", "/v1/{selector=**}", ""]], + auth_pb2.AuthenticationRule(selector="parent/p1"), + {}, + ["get", "/v1/parent/p1", None, auth_pb2.AuthenticationRule()], + ], [ [["get", "/v1/{field=a/**/b/**}", ""]], + None, {"field": "a/parent/p1/b/child/c1", "foo": "bar"}, ["get", "/v1/a/parent/p1/b/child/c1", {}, {"foo": "bar"}], ], + [ + [["get", "/v1/{selector=a/**/b/**}", ""]], + auth_pb2.AuthenticationRule( + selector="a/parent/p1/b/child/c1", allow_without_credential=True + ), + {}, + [ + "get", + "/v1/a/parent/p1/b/child/c1", + None, + auth_pb2.AuthenticationRule(allow_without_credential=True), + ], + ], # Combined single and double segment wildcard [ [["get", "/v1/{field=a/*/b/**}", ""]], + None, {"field": "a/parent/b/child/c1"}, ["get", "/v1/a/parent/b/child/c1", {}, {}], ], + [ + [["get", "/v1/{selector=a/*/b/**}", ""]], + auth_pb2.AuthenticationRule(selector="a/parent/b/child/c1"), + {}, + ["get", "/v1/a/parent/b/child/c1", None, auth_pb2.AuthenticationRule()], + ], [ [["get", "/v1/{field=a/**/b/*}/v2/{name}", ""]], + None, {"field": "a/parent/p1/b/child", "name": "first", "foo": "bar"}, ["get", "/v1/a/parent/p1/b/child/v2/first", {}, {"foo": "bar"}], ], + [ + [["get", "/v1/{selector=a/**/b/*}/v2/{oauth.canonical_scopes}", ""]], + auth_pb2.AuthenticationRule( + selector="a/parent/p1/b/child", + oauth=auth_pb2.OAuthRequirements(canonical_scopes="first"), + ), + {"field": "a/parent/p1/b/child", "name": "first", "foo": "bar"}, + [ + "get", + "/v1/a/parent/p1/b/child/v2/first", + None, + auth_pb2.AuthenticationRule(oauth=auth_pb2.OAuthRequirements()), + ], + ], ], ) -def test_transcode_with_wildcard(http_options, request_kwargs, expected_result): +def test_transcode_with_wildcard( + http_options, message, request_kwargs, expected_result +): http_options, expected_result = helper_test_transcode(http_options, expected_result) - result = path_template.transcode(http_options, **request_kwargs) + result = path_template.transcode(http_options, message, **request_kwargs) assert result == expected_result @pytest.mark.parametrize( - "http_options, request_kwargs, expected_result", + "http_options, message, request_kwargs, expected_result", [ # Single field body [ [["post", "/v1/no/template", "data"]], + None, {"data": {"id": 1, "info": "some info"}, "foo": "bar"}, ["post", "/v1/no/template", {"id": 1, "info": "some info"}, {"foo": "bar"}], ], + [ + [["post", "/v1/no/template", "oauth"]], + auth_pb2.AuthenticationRule( + selector="bar", + oauth=auth_pb2.OAuthRequirements(canonical_scopes="child"), + ), + {}, + [ + "post", + "/v1/no/template", + auth_pb2.OAuthRequirements(canonical_scopes="child"), + auth_pb2.AuthenticationRule(selector="bar"), + ], + ], [ [["post", "/v1/{field=a/*}/b/{name=**}", "data"]], + None, { "field": "a/parent", "name": "first/last", @@ -291,9 +443,29 @@ def test_transcode_with_wildcard(http_options, request_kwargs, expected_result): {"foo": "bar"}, ], ], + [ + [["post", "/v1/{selector=a/*}/b/{oauth.canonical_scopes=**}", "oauth"]], + auth_pb2.AuthenticationRule( + selector="a/parent", + allow_without_credential=True, + requirements=[auth_pb2.AuthRequirement(provider_id="p")], + oauth=auth_pb2.OAuthRequirements(canonical_scopes="first/last"), + ), + {}, + [ + "post", + "/v1/a/parent/b/first/last", + auth_pb2.OAuthRequirements(), + auth_pb2.AuthenticationRule( + requirements=[auth_pb2.AuthRequirement(provider_id="p")], + allow_without_credential=True, + ), + ], + ], # Wildcard body [ [["post", "/v1/{field=a/*}/b/{name=**}", "*"]], + None, { "field": "a/parent", "name": "first/last", @@ -307,16 +479,38 @@ def test_transcode_with_wildcard(http_options, request_kwargs, expected_result): {}, ], ], + [ + [["post", "/v1/{selector=a/*}/b/{oauth.canonical_scopes=**}", "*"]], + auth_pb2.AuthenticationRule( + selector="a/parent", + allow_without_credential=True, + oauth=auth_pb2.OAuthRequirements(canonical_scopes="first/last"), + ), + { + "field": "a/parent", + "name": "first/last", + "data": {"id": 1, "info": "some info"}, + "foo": "bar", + }, + [ + "post", + "/v1/a/parent/b/first/last", + auth_pb2.AuthenticationRule( + allow_without_credential=True, oauth=auth_pb2.OAuthRequirements() + ), + auth_pb2.AuthenticationRule(), + ], + ], ], ) -def test_transcode_with_body(http_options, request_kwargs, expected_result): +def test_transcode_with_body(http_options, message, request_kwargs, expected_result): http_options, expected_result = helper_test_transcode(http_options, expected_result) - result = path_template.transcode(http_options, **request_kwargs) + result = path_template.transcode(http_options, message, **request_kwargs) assert result == expected_result @pytest.mark.parametrize( - "http_options, request_kwargs, expected_result", + "http_options, message, request_kwargs, expected_result", [ # Additional bindings [ @@ -324,6 +518,7 @@ def test_transcode_with_body(http_options, request_kwargs, expected_result): ["post", "/v1/{field=a/*}/b/{name=**}", "extra_data"], ["post", "/v1/{field=a/*}/b/{name=**}", "*"], ], + None, { "field": "a/parent", "name": "first/last", @@ -337,38 +532,105 @@ def test_transcode_with_body(http_options, request_kwargs, expected_result): {}, ], ], + [ + [ + [ + "post", + "/v1/{selector=a/*}/b/{oauth.canonical_scopes=**}", + "extra_data", + ], + ["post", "/v1/{selector=a/*}/b/{oauth.canonical_scopes=**}", "*"], + ], + auth_pb2.AuthenticationRule( + selector="a/parent", + allow_without_credential=True, + oauth=auth_pb2.OAuthRequirements(canonical_scopes="first/last"), + ), + {}, + [ + "post", + "/v1/a/parent/b/first/last", + auth_pb2.AuthenticationRule( + allow_without_credential=True, oauth=auth_pb2.OAuthRequirements() + ), + auth_pb2.AuthenticationRule(), + ], + ], [ [ ["get", "/v1/{field=a/*}/b/{name=**}", ""], ["get", "/v1/{field=a/*}/b/first/last", ""], ], + None, {"field": "a/parent", "foo": "bar"}, ["get", "/v1/a/parent/b/first/last", {}, {"foo": "bar"}], ], + [ + [ + ["get", "/v1/{selector=a/*}/b/{oauth.allow_without_credential=**}", ""], + ["get", "/v1/{selector=a/*}/b/first/last", ""], + ], + auth_pb2.AuthenticationRule( + selector="a/parent", + allow_without_credential=True, + oauth=auth_pb2.OAuthRequirements(), + ), + {}, + [ + "get", + "/v1/a/parent/b/first/last", + None, + auth_pb2.AuthenticationRule( + allow_without_credential=True, oauth=auth_pb2.OAuthRequirements() + ), + ], + ], ], ) def test_transcode_with_additional_bindings( - http_options, request_kwargs, expected_result + http_options, message, request_kwargs, expected_result ): http_options, expected_result = helper_test_transcode(http_options, expected_result) - result = path_template.transcode(http_options, **request_kwargs) + result = path_template.transcode(http_options, message, **request_kwargs) assert result == expected_result @pytest.mark.parametrize( - "http_options, request_kwargs", + "http_options, message, request_kwargs", [ - [[["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"}], - [[["post", "/v1/{first_name}", "data"]], {"last_name": "last"}], + [[["get", "/v1/{name}", ""]], None, {"foo": "bar"}], + [[["get", "/v1/{selector}", ""]], auth_pb2.AuthenticationRule(), {}], + [[["get", "/v1/{name}", ""]], auth_pb2.AuthenticationRule(), {}], + [[["get", "/v1/{name}", ""]], None, {"name": "first/last"}], + [ + [["get", "/v1/{selector}", ""]], + auth_pb2.AuthenticationRule(selector="first/last"), + {}, + ], + [[["get", "/v1/{name=mr/*/*}", ""]], None, {"name": "first/last"}], + [ + [["get", "/v1/{selector=mr/*/*}", ""]], + auth_pb2.AuthenticationRule(selector="first/last"), + {}, + ], + [[["post", "/v1/{name}", "data"]], None, {"name": "first/last"}], + [ + [["post", "/v1/{selector}", "data"]], + auth_pb2.AuthenticationRule(selector="first"), + {}, + ], + [[["post", "/v1/{first_name}", "data"]], None, {"last_name": "last"}], + [ + [["post", "/v1/{first_name}", ""]], + auth_pb2.AuthenticationRule(selector="first"), + {}, + ], ], ) -def test_transcode_fails(http_options, request_kwargs): +def test_transcode_fails(http_options, message, request_kwargs): http_options, _ = helper_test_transcode(http_options, range(4)) with pytest.raises(ValueError): - path_template.transcode(http_options, **request_kwargs) + path_template.transcode(http_options, message, **request_kwargs) def helper_test_transcode(http_options_list, expected_result_list):