Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Adjust Field Names in URI Templates #1041

Merged
merged 2 commits into from
Oct 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions gapic/schema/wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,7 @@ def try_parse_http_rule(cls, http_rule) -> Optional['HttpRule']:
uri = getattr(http_rule, method)
if not uri:
return None
uri = utils.convert_uri_fieldnames(uri)

body = http_rule.body or None
return cls(method, uri, body)
Expand Down
2 changes: 2 additions & 0 deletions gapic/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@
from gapic.utils.options import Options
from gapic.utils.reserved_names import RESERVED_NAMES
from gapic.utils.rst import rst
from gapic.utils.uri_conv import convert_uri_fieldnames
from gapic.utils.uri_sample import sample_from_path_fields


__all__ = (
'cached_property',
'convert_uri_fieldnames',
'doc',
'empty',
'is_msg_field_pb',
Expand Down
49 changes: 49 additions & 0 deletions gapic/utils/uri_conv.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Copyright 2021 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from gapic.utils.reserved_names import RESERVED_NAMES
from google.api_core import path_template # type: ignore


def convert_uri_fieldnames(uri: str) -> str:
"""Modify field names in uri_templates to avoid reserved names.

Args:
uri: a uri template, optionally containing field references in {} braces.

Returns:
the uri with any field names modified if they conflict with
reserved names.
"""

def _fix_name_segment(name_seg: str) -> str:
return name_seg + "_" if name_seg in RESERVED_NAMES else name_seg

def _fix_field_path(field_path: str) -> str:
return ".".join(
(_fix_name_segment(name_seg) for name_seg in field_path.split(".")))

last = 0
pieces = []
for match in path_template._VARIABLE_RE.finditer(uri):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can _VARIABLE_RE be made public in google-api-core? Otherwise someone might assume it's only used in google-api-core and make a change that breaks the generator.

Slightly lower priority since it's in the generator code and not in generated library code.

start_pos, end_pos = match.span("name")
if start_pos == end_pos:
continue
pieces.append(uri[last:start_pos])
fixed_field_path = _fix_field_path(uri[start_pos:end_pos])
pieces.append(fixed_field_path)
last = end_pos
pieces.append(uri[last:])

return "".join(pieces)
2 changes: 1 addition & 1 deletion noxfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def unit(session):
"""Run the unit test suite."""

session.install(
"coverage", "pytest-cov", "pytest", "pytest-xdist", "pyfakefs",
"coverage", "pytest-cov", "pytest", "pytest-xdist", "pyfakefs", "grpcio-status",
)
session.install("-e", ".")

Expand Down
13 changes: 13 additions & 0 deletions tests/unit/schema/wrappers/test_method.py
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,19 @@ def test_method_http_options_additional_bindings():
}]


def test_method_http_options_reserved_name_in_url():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add the short template format ({license}) here too (and ideally some other reserverd name besides license).

In general, please treat the short format as a first-class citizen for all grpc transcodding work, because this is the only format used in compute, which is the most importantlibrary relying on rest format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have that form in the unit test test_uri_conv.py. I can add other names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the test there already includes reversed as well as license:

def test_convert_uri_fieldname():
     uri = "abc/*/license/{license}/{xyz.reversed=reversed/*}"
     expected_uri = "abc/*/license/{license_}/{xyz.reversed_=reversed/*}"
     assert utils.convert_uri_fieldnames(uri) == expected_uri

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is license a reserved keyword? I think this should be fixed.

http_rule = http_pb2.HttpRule(
post='/v1/license/{license=lic/*}',
body='*'
)
method = make_method('DoSomething', http_rule=http_rule)
assert [dataclasses.asdict(http) for http in method.http_options] == [{
'method': 'post',
'uri': '/v1/license/{license_=lic/*}',
'body': '*'
}]


def test_method_http_options_generate_sample():
http_rule = http_pb2.HttpRule(
get='/v1/{resource.id=projects/*/regions/*/id/**}/stuff',
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/utils/test_uri_conv.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Copyright 2021 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from unittest import mock

import pypandoc

from gapic import utils


def test_convert_uri_fieldname():
uri = "abc/*/license/{license}/{xyz.reversed=reversed/*}"
expected_uri = "abc/*/license/{license_}/{xyz.reversed_=reversed/*}"
assert utils.convert_uri_fieldnames(uri) == expected_uri


def test_convert_uri_fieldname_no_fields():
uri = "abc/license"
assert utils.convert_uri_fieldnames(uri) == uri


def test_convert_uri_fieldname_no_reserved_names():
uri = "abc/*/books/{book}/{xyz.chapter=page/*}"
assert utils.convert_uri_fieldnames(uri) == uri