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 <partheniou@google.com>
diff --git a/google/api_core/path_template.py b/google/api_core/path_template.py
index a99a4c8..2639459 100644
--- a/google/api_core/path_template.py
+++ b/google/api_core/path_template.py
@@ -176,7 +176,7 @@
"""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 @@
"""
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 @@
"""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 @@
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 @@
'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 @@
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 c12b35f..73d351c 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 @@
@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 @@
{"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 @@
{},
],
],
+ [
+ [["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 @@
["post", "/v1/{field=a/*}/b/{name=**}", "extra_data"],
["post", "/v1/{field=a/*}/b/{name=**}", "*"],
],
+ None,
{
"field": "a/parent",
"name": "first/last",
@@ -339,36 +534,103 @@
],
[
[
+ [
+ "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):
