From ecf0f97df113b08f17af08a7712b074c44456553 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Wed, 13 Mar 2024 13:23:56 +0000 Subject: [PATCH 01/36] feat: Add support for reading MethodSettings from service configuration YAML --- gapic/schema/api.py | 111 +++++++++++++++ tests/unit/schema/test_api.py | 256 ++++++++++++++++++++++++++++++++++ 2 files changed, 367 insertions(+) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 084349d488..968c5004dd 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -27,6 +27,7 @@ from types import MappingProxyType from google.api_core import exceptions +from google.api import client_pb2 # type: ignore from google.api import http_pb2 # type: ignore from google.api import resource_pb2 # type: ignore from google.api import service_pb2 # type: ignore @@ -560,6 +561,116 @@ def mixin_http_options(self): res[s] = [rule for rule in opt_gen if rule] return res + @cached_property + def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: + """Return a map of API-wide `MethodSettings` from the service YAML config to be used + when generating methods. + https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 + + Return: + Mapping[str, Sequence[client_pb2.MethodSettings]]: A mapping of all API-wide method + settings read from the service YAML, taking into account the requirements + from AIPs, such as AIP-4235. + + Raises: + ValueError: if `auto_populated_fields` are incorrectly set in the service config + """ + + def _get_all_methods(self) -> Mapping[str, MethodDescriptorProto]: + """Return a map of all API-wide methods. + Return: + Mapping[str, MethodDescriptorProto]: A mapping of MethodDescriptorProto + values for the API. + """ + methods = {} + for service_key, service_value in self.services.items(): + for method_key, method_value in service_value.methods.items(): + methods[f"{service_key}.{method_key}"] = method_value + return methods + + methods = _get_all_methods(self) + + def _get_auto_populated_fields_candidates( + message: MethodDescriptorProto, + ) -> Sequence[str]: + """Return fields which meet the criteria to be automatically populated for a given message. + For a field to be automatically populated, all the below configurations must + be true according to https://google.aip.dev/client-libraries/4235: + + - The field must be of type string + - The field must be at the top-level of the request message + - The field must not be annotated with google.api.field_behavior = REQUIRED. + - The field must be annotated with google.api.field_info.format = UUID4. + + Args: + message[MethodDescriptorProto]: Describes a method of a service. + Return: + Sequence[str]: A list of candidates to be automatically populated + according to https://google.aip.dev/client-libraries/4235. + """ + top_level_request_message = self.messages[message.input_type.lstrip(".")] + return [ + field.name + for field in top_level_request_message.fields.values() + if field.type == wrappers.PrimitiveType.build(str) + and not field.required + and field.uuid4 + ] + + def _get_auto_populated_fields( + method_settings: client_pb2.MethodSettings, + ) -> Sequence[str]: + """ + Return a list of auto-populated fields based on the service YAML. + + Args: + method_settings (client_pb2.MethodSettings): The settings + to be used when generating API methods. + Return: + Sequence[str]: A sequence of all fields to be automatically + populated, taking into account the requirements + from AIP-4235. + https://google.aip.dev/client-libraries/4235 + + Raises: + ValueError: if `auto_populated_fields` are incorrectly set in the service config + """ + try: + method_in_scope = methods[method_settings.selector] + except KeyError: + raise ValueError( + f"Selector {method_settings.selector} is not a valid method in this API." + ) + + if ( + method_in_scope.client_streaming or method_in_scope.server_streaming + ) and method_settings.auto_populated_fields: + raise ValueError( + f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." + ) + + # The field name must be listed in the google.api.MethodSettings.auto_populated_fields entry in google.api.Publishing.method_settings for the target method. + auto_populated_fields = [ + field for field in method_settings.auto_populated_fields + ] + for field in auto_populated_fields: + if field not in _get_auto_populated_fields_candidates(method_in_scope): + raise ValueError( + f"Field {field} is not valid as an auto populated field for the top level request message of selector {method_settings.selector}" + ) + + return auto_populated_fields + + return { + method_setting.selector: client_pb2.MethodSettings( + selector=method_setting.selector, + long_running=method_setting.long_running, + auto_populated_fields=_get_auto_populated_fields(method_setting), + ) + for method_setting in self.service_yaml_config.publishing.method_settings + } + + @cached_property def has_location_mixin(self) -> bool: return len(list(filter(lambda api: api.name == "google.cloud.location.Locations", self.service_yaml_config.apis))) > 0 diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index e493e68d0c..c5d8d82a06 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -20,6 +20,8 @@ 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 field_info_pb2 from google.api import resource_pb2 from google.api_core import exceptions from google.cloud import extended_operations_pb2 as ex_ops_pb2 @@ -2602,3 +2604,257 @@ def test_has_iam_mixin(): }) api_schema = api.API.build(fd, 'google.example.v1', opts=opts) assert api_schema.has_iam_mixin + + + +def test_get_method_settings(): + """ + Tests the `all_method_settings` method of `gapic.schema.api` which + reads `MethodSettings` from the service config YAML. + https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 + """ + field_options = descriptor_pb2.FieldOptions() + field_options.Extensions[field_info_pb2.field_info].format = ( + field_info_pb2.FieldInfo.Format.Value("UUID4") + ) + + # See AIP https://google.aip.dev/client-libraries/4235 + # Only fields which are not required should be auto-populated + squid = make_field_pb2( + name="squid", type="TYPE_STRING", options=field_options, number=1 + ) + mollusc = make_field_pb2( + name="mollusc", type="TYPE_STRING", options=field_options, number=2 + ) + # Also include a required field + field_options.Extensions[field_behavior_pb2.field_behavior].append( + field_behavior_pb2.FieldBehavior.Value("REQUIRED") + ) + clam = make_field_pb2( + name="clam", type="TYPE_STRING", options=field_options, number=3 + ) + fd = ( + make_file_pb2( + name="someexample.proto", + package="google.example.v1beta1", + messages=( + make_message_pb2(name="ExampleRequest", fields=(squid, mollusc, clam)), + make_message_pb2(name="ExampleResponse", fields=()), + make_message_pb2(name="AnotherRequest", fields=(squid,)), + make_message_pb2(name="AnotherResponse", fields=()), + ), + services=( + descriptor_pb2.ServiceDescriptorProto( + name="SomeExample", + method=( + descriptor_pb2.MethodDescriptorProto( + name="Example1", + # Input and output types don't matter. + input_type="google.example.v1beta1.ExampleRequest", + output_type="google.example.v1beta1.ExampleResponse", + ), + descriptor_pb2.MethodDescriptorProto( + name="Example2", + # Input and output types don't matter. + input_type="google.example.v1beta1.ExampleRequest", + output_type="google.example.v1beta1.ExampleResponse", + client_streaming=True, + ), + descriptor_pb2.MethodDescriptorProto( + name="Example3", + # Input and output types don't matter. + input_type="google.example.v1beta1.ExampleRequest", + output_type="google.example.v1beta1.ExampleResponse", + server_streaming=True, + ), + descriptor_pb2.MethodDescriptorProto( + name="Example4", + # Input and output types don't matter. + input_type="google.example.v1beta1.AnotherRequest", + output_type="google.example.v1beta1.AnotherResponse", + ), + descriptor_pb2.MethodDescriptorProto( + name="Example5", + # Input and output types don't matter. + input_type="google.example.v1beta1.AnotherRequest", + output_type="google.example.v1beta1.AnotherResponse", + ), + ), + ), + ), + ), + ) + service_yaml_config = { + "apis": [ + {"name": "google.example.v1beta1.SomeExample.Example1"}, + {"name": "google.example.v1beta1.SomeExample.Example2"}, + {"name": "google.example.v1beta1.SomeExample.Example3"}, + {"name": "google.example.v1beta1.SomeExample.Example4"}, + {"name": "google.example.v1beta1.SomeExample.Example5"}, + ], + "publishing": { + "method_settings": [ + { + "selector": "google.example.v1beta1.SomeExample.Example1", + "auto_populated_fields": [ + "squid", + "mollusc", + ], + }, + { + "selector": "google.example.v1beta1.SomeExample.Example2", + "auto_populated_fields": [], + }, + { + "selector": "google.example.v1beta1.SomeExample.Example3", + "auto_populated_fields": [], + }, + { + "selector": "google.example.v1beta1.SomeExample.Example4", + "auto_populated_fields": [], + }, + { + "selector": "google.example.v1beta1.SomeExample.Example5", + }, + ] + }, + } + opts = Options(service_yaml_config=service_yaml_config) + + api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) + + assert api_schema.all_method_settings == { + "google.example.v1beta1.SomeExample.Example1": client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", + auto_populated_fields=["squid", "mollusc"], + long_running=client_pb2.MethodSettings.LongRunning(), + ), + "google.example.v1beta1.SomeExample.Example2": client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example2", + auto_populated_fields=[], + long_running=client_pb2.MethodSettings.LongRunning(), + ), + "google.example.v1beta1.SomeExample.Example3": client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example3", + auto_populated_fields=[], + long_running=client_pb2.MethodSettings.LongRunning(), + ), + "google.example.v1beta1.SomeExample.Example4": client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example4", + auto_populated_fields=[], + long_running=client_pb2.MethodSettings.LongRunning(), + ), + "google.example.v1beta1.SomeExample.Example5": client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example5", + auto_populated_fields=[], + long_running=client_pb2.MethodSettings.LongRunning(), + ), + } + + # Test that ValueError is raised when there is an invalid value for publishing.method_settings.selector + opts = Options( + service_yaml_config={ + "apis": [ + {"name": "google.example.v1beta1.SomeExample.Example1"}, + ], + "publishing": { + "method_settings": [ + { + "selector": "google.example.v1beta1.DoesNotExist.Example1", + "auto_populated_fields": [ + "squid", + "mollusc", + ], + } + ] + }, + } + ) + api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) + + with pytest.raises( + ValueError, + match="Selector google.example.v1beta1.DoesNotExist.Example1 is not a valid method in this API", + ): + api_schema.all_method_settings + + # Test that ValueError is raised when the rpc in publishing.method_settings.selector is not a unary one + opts = Options( + service_yaml_config={ + "apis": [ + {"name": "google.example.v1beta1.SomeExample.Example1"}, + ], + "publishing": { + "method_settings": [ + { + "selector": "google.example.v1beta1.SomeExample.Example2", + "auto_populated_fields": [ + "squid", + "mollusc", + ], + } + ] + }, + } + ) + api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) + + with pytest.raises( + ValueError, + match="Selector google.example.v1beta1.SomeExample.Example2 is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs", + ): + api_schema.all_method_settings + + # Test that ValueError is raised when the rpc in publishing.method_settings.selector is not a unary one + opts = Options( + service_yaml_config={ + "apis": [ + {"name": "google.example.v1beta1.SomeExample.Example1"}, + ], + "publishing": { + "method_settings": [ + { + "selector": "google.example.v1beta1.SomeExample.Example3", + "auto_populated_fields": [ + "squid", + "mollusc", + ], + } + ] + }, + } + ) + api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) + + with pytest.raises( + ValueError, + match="Selector google.example.v1beta1.SomeExample.Example3 is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs", + ): + api_schema.all_method_settings + + # Test that ValueError is raised when the field in publishing.method_settings.auto_populated_fields does not exist + opts = Options( + service_yaml_config={ + "apis": [ + {"name": "google.example.v1beta1.SomeExample.Example1"}, + ], + "publishing": { + "method_settings": [ + { + "selector": "google.example.v1beta1.SomeExample.Example1", + "auto_populated_fields": [ + "doesnotexist", + "mollusc", + ], + } + ] + }, + } + ) + api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) + + with pytest.raises( + ValueError, + match="Field doesnotexist is not valid as an auto populated field for the top level request message of selector google.example.v1beta1.SomeExample.Example1", + ): + api_schema.all_method_settings From 16f3784d2d972b6c07ed5b5b685d8ffb38ae15a9 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Wed, 13 Mar 2024 16:56:10 +0000 Subject: [PATCH 02/36] style --- gapic/schema/api.py | 9 ++++++--- tests/unit/schema/test_api.py | 6 ++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 968c5004dd..1e2ce211e1 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -608,7 +608,9 @@ def _get_auto_populated_fields_candidates( Sequence[str]: A list of candidates to be automatically populated according to https://google.aip.dev/client-libraries/4235. """ - top_level_request_message = self.messages[message.input_type.lstrip(".")] + top_level_request_message = self.messages[ + message.input_type.lstrip(".") + ] return [ field.name for field in top_level_request_message.fields.values() @@ -665,12 +667,13 @@ def _get_auto_populated_fields( method_setting.selector: client_pb2.MethodSettings( selector=method_setting.selector, long_running=method_setting.long_running, - auto_populated_fields=_get_auto_populated_fields(method_setting), + auto_populated_fields=_get_auto_populated_fields( + method_setting + ), ) for method_setting in self.service_yaml_config.publishing.method_settings } - @cached_property def has_location_mixin(self) -> bool: return len(list(filter(lambda api: api.name == "google.cloud.location.Locations", self.service_yaml_config.apis))) > 0 diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index c5d8d82a06..e2ce370763 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2606,7 +2606,6 @@ def test_has_iam_mixin(): assert api_schema.has_iam_mixin - def test_get_method_settings(): """ Tests the `all_method_settings` method of `gapic.schema.api` which @@ -2638,7 +2637,10 @@ def test_get_method_settings(): name="someexample.proto", package="google.example.v1beta1", messages=( - make_message_pb2(name="ExampleRequest", fields=(squid, mollusc, clam)), + make_message_pb2( + name="ExampleRequest", + fields=(squid, mollusc, clam) + ), make_message_pb2(name="ExampleResponse", fields=()), make_message_pb2(name="AnotherRequest", fields=(squid,)), make_message_pb2(name="AnotherResponse", fields=()), From 41c582c6a1f87481b94b237bc76a65eea0273b0c Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Wed, 13 Mar 2024 18:58:23 +0000 Subject: [PATCH 03/36] refactor based on review feedback --- gapic/schema/api.py | 178 +++++++++++++++++++++++--------------------- 1 file changed, 94 insertions(+), 84 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 1e2ce211e1..b5143eae67 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -561,113 +561,123 @@ def mixin_http_options(self): res[s] = [rule for rule in opt_gen if rule] return res - @cached_property - def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: - """Return a map of API-wide `MethodSettings` from the service YAML config to be used - when generating methods. - https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 + def get_auto_populated_fields_candidates( + self, + message: MethodDescriptorProto, + ) -> Sequence[str]: + """Return fields which meet the criteria to be automatically populated for a given message. + For a field to be automatically populated, all the below configurations must + be true according to https://google.aip.dev/client-libraries/4235: + - The field must be of type string + - The field must be at the top-level of the request message + - The field must not be annotated with google.api.field_behavior = REQUIRED. + - The field must be annotated with google.api.field_info.format = UUID4. + + Args: + message[MethodDescriptorProto]: Describes a method of a service. Return: - Mapping[str, Sequence[client_pb2.MethodSettings]]: A mapping of all API-wide method - settings read from the service YAML, taking into account the requirements - from AIPs, such as AIP-4235. + Sequence[str]: A list of candidates to be automatically populated + according to https://google.aip.dev/client-libraries/4235. + """ + top_level_request_message = self.messages[ + message.input_type.lstrip(".") + ] + return [ + field.name + for field in top_level_request_message.fields.values() + if field.type == wrappers.PrimitiveType.build(str) + and not field.required + and field.uuid4 + ] + + def get_auto_populated_fields( + self, + method_settings: client_pb2.MethodSettings, + ) -> Sequence[str]: + """ + Return a list of auto-populated fields based on the service YAML. + + Args: + method_settings (client_pb2.MethodSettings): The settings + to be used when generating API methods. + Return: + Sequence[str]: A sequence of all fields to be automatically + populated, taking into account the requirements + from AIP-4235. + https://google.aip.dev/client-libraries/4235 Raises: ValueError: if `auto_populated_fields` are incorrectly set in the service config """ + auto_populated_fields = [] - def _get_all_methods(self) -> Mapping[str, MethodDescriptorProto]: - """Return a map of all API-wide methods. - Return: - Mapping[str, MethodDescriptorProto]: A mapping of MethodDescriptorProto - values for the API. - """ - methods = {} - for service_key, service_value in self.services.items(): - for method_key, method_value in service_value.methods.items(): - methods[f"{service_key}.{method_key}"] = method_value - return methods - - methods = _get_all_methods(self) - - def _get_auto_populated_fields_candidates( - message: MethodDescriptorProto, - ) -> Sequence[str]: - """Return fields which meet the criteria to be automatically populated for a given message. - For a field to be automatically populated, all the below configurations must - be true according to https://google.aip.dev/client-libraries/4235: - - - The field must be of type string - - The field must be at the top-level of the request message - - The field must not be annotated with google.api.field_behavior = REQUIRED. - - The field must be annotated with google.api.field_info.format = UUID4. - - Args: - message[MethodDescriptorProto]: Describes a method of a service. - Return: - Sequence[str]: A list of candidates to be automatically populated - according to https://google.aip.dev/client-libraries/4235. + def _check_service_yaml_validity(method_in_scope: MethodDescriptorProto, method_settings: client_pb2.MethodSettings): """ - top_level_request_message = self.messages[ - message.input_type.lstrip(".") - ] - return [ - field.name - for field in top_level_request_message.fields.values() - if field.type == wrappers.PrimitiveType.build(str) - and not field.required - and field.uuid4 - ] - - def _get_auto_populated_fields( - method_settings: client_pb2.MethodSettings, - ) -> Sequence[str]: + Ensure that the method and fields meet the criteria of AIP-4235. + Use the helper function get_auto_populated_fields_candidates() to return on fields + which meet the criteria of AIP-4235. + https://google.aip.dev/client-libraries/4235 """ - Return a list of auto-populated fields based on the service YAML. - - Args: - method_settings (client_pb2.MethodSettings): The settings - to be used when generating API methods. - Return: - Sequence[str]: A sequence of all fields to be automatically - populated, taking into account the requirements - from AIP-4235. - https://google.aip.dev/client-libraries/4235 - - Raises: - ValueError: if `auto_populated_fields` are incorrectly set in the service config - """ - try: - method_in_scope = methods[method_settings.selector] - except KeyError: - raise ValueError( - f"Selector {method_settings.selector} is not a valid method in this API." - ) - if ( - method_in_scope.client_streaming or method_in_scope.server_streaming - ) and method_settings.auto_populated_fields: + # The method must not be a streaming rpc + if method_in_scope.client_streaming or method_in_scope.server_streaming: raise ValueError( f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." ) - # The field name must be listed in the google.api.MethodSettings.auto_populated_fields entry in google.api.Publishing.method_settings for the target method. - auto_populated_fields = [ - field for field in method_settings.auto_populated_fields - ] - for field in auto_populated_fields: - if field not in _get_auto_populated_fields_candidates(method_in_scope): + for field in method_settings.auto_populated_fields: + if field not in self.get_auto_populated_fields_candidates(method_in_scope): raise ValueError( f"Field {field} is not valid as an auto populated field for the top level request message of selector {method_settings.selector}" ) - return auto_populated_fields + # Ensure that the method defined in the service YAML exists + try: + method_in_scope = self.all_methods[method_settings.selector] + except KeyError: + raise ValueError( + f"Selector {method_settings.selector} is not a valid method in this API." + ) + + if method_settings.auto_populated_fields: + _check_service_yaml_validity(method_in_scope) + auto_populated_fields = method_settings.auto_populated_fields + + return auto_populated_fields + + @cached_property + def all_methods(self) -> Mapping[str, MethodDescriptorProto]: + """Return a map of all API-wide methods. + Return: + Mapping[str, MethodDescriptorProto]: A mapping of MethodDescriptorProto + values for the API. + """ + return { + f"{service_key}.{method_key}": method_value + for service_key, service_value in self.services.items() + for method_key, method_value in service_value.methods.items() + } + @cached_property + def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: + """Return a map of API-wide `MethodSettings` from the service YAML config to be used + when generating methods. + https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 + + Return: + Mapping[str, Sequence[client_pb2.MethodSettings]]: A mapping of all API-wide method + settings read from the service YAML, taking into account the requirements + from AIPs, such as AIP-4235. + + Raises: + ValueError: if `auto_populated_fields` are incorrectly set in the service config + """ return { method_setting.selector: client_pb2.MethodSettings( selector=method_setting.selector, long_running=method_setting.long_running, - auto_populated_fields=_get_auto_populated_fields( + auto_populated_fields=self.get_auto_populated_fields( method_setting ), ) From 9f5c5d56fd7fc6860f369deb2429a7236991c521 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Wed, 13 Mar 2024 19:03:11 +0000 Subject: [PATCH 04/36] refactor --- gapic/schema/api.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index b5143eae67..1d438e8de4 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -626,8 +626,11 @@ def _check_service_yaml_validity(method_in_scope: MethodDescriptorProto, method_ f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." ) # The field name must be listed in the google.api.MethodSettings.auto_populated_fields entry in google.api.Publishing.method_settings for the target method. + candidates = self.get_auto_populated_fields_candidates( + method_in_scope + ) for field in method_settings.auto_populated_fields: - if field not in self.get_auto_populated_fields_candidates(method_in_scope): + if field not in candidates: raise ValueError( f"Field {field} is not valid as an auto populated field for the top level request message of selector {method_settings.selector}" ) @@ -641,7 +644,7 @@ def _check_service_yaml_validity(method_in_scope: MethodDescriptorProto, method_ ) if method_settings.auto_populated_fields: - _check_service_yaml_validity(method_in_scope) + _check_service_yaml_validity(method_in_scope, method_settings) auto_populated_fields = method_settings.auto_populated_fields return auto_populated_fields From 884fda2fe000502c000fe9e7584517d11a23ed56 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Wed, 13 Mar 2024 19:08:09 +0000 Subject: [PATCH 05/36] docs --- gapic/schema/api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 1d438e8de4..6447c9649d 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -652,6 +652,7 @@ def _check_service_yaml_validity(method_in_scope: MethodDescriptorProto, method_ @cached_property def all_methods(self) -> Mapping[str, MethodDescriptorProto]: """Return a map of all API-wide methods. + Return: Mapping[str, MethodDescriptorProto]: A mapping of MethodDescriptorProto values for the API. From 12f4333d8881e0fd284199ae375b599185ce6ddd Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 10:03:26 +0000 Subject: [PATCH 06/36] message->method_descriptor; update comments --- gapic/schema/api.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 6447c9649d..bae60a4729 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -563,16 +563,18 @@ def mixin_http_options(self): def get_auto_populated_fields_candidates( self, - message: MethodDescriptorProto, + method_descriptor: MethodDescriptorProto, ) -> Sequence[str]: - """Return fields which meet the criteria to be automatically populated for a given message. - For a field to be automatically populated, all the below configurations must - be true according to https://google.aip.dev/client-libraries/4235: + """Return fields in the request message for a given method which meet the + criteria for being automatically populated. For a field to be automatically + populated, all the configurations below must be true according to + https://google.aip.dev/client-libraries/4235: - The field must be of type string - The field must be at the top-level of the request message - The field must not be annotated with google.api.field_behavior = REQUIRED. - The field must be annotated with google.api.field_info.format = UUID4. + - The field presence requirements in AIP-4235 are checked at run time. Args: message[MethodDescriptorProto]: Describes a method of a service. @@ -581,7 +583,7 @@ def get_auto_populated_fields_candidates( according to https://google.aip.dev/client-libraries/4235. """ top_level_request_message = self.messages[ - message.input_type.lstrip(".") + method_descriptor.input_type.lstrip(".") ] return [ field.name From b12753d4afd12aaef4a98d7f4633ee5a188a5a27 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 10:06:27 +0000 Subject: [PATCH 07/36] update docstring of _check_service_yaml_validity --- gapic/schema/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index bae60a4729..e3f2528503 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -598,7 +598,7 @@ def get_auto_populated_fields( method_settings: client_pb2.MethodSettings, ) -> Sequence[str]: """ - Return a list of auto-populated fields based on the service YAML. + Return a list of fields with requested auto-population in the `method_settings`. Args: method_settings (client_pb2.MethodSettings): The settings From 8d64606a88f54f6d641fb958327adab48a62798b Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 10:07:45 +0000 Subject: [PATCH 08/36] typo --- gapic/schema/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index e3f2528503..2b4614bf02 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -617,7 +617,7 @@ def get_auto_populated_fields( def _check_service_yaml_validity(method_in_scope: MethodDescriptorProto, method_settings: client_pb2.MethodSettings): """ Ensure that the method and fields meet the criteria of AIP-4235. - Use the helper function get_auto_populated_fields_candidates() to return on fields + Use the helper function get_auto_populated_fields_candidates() to return fields which meet the criteria of AIP-4235. https://google.aip.dev/client-libraries/4235 """ From b760a62b2a4f7878c74149c8cbc4ac9c45905f33 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 10:09:46 +0000 Subject: [PATCH 09/36] update docstring of _check_service_yaml_validity --- gapic/schema/api.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 2b4614bf02..89e1721a7e 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -618,8 +618,7 @@ def _check_service_yaml_validity(method_in_scope: MethodDescriptorProto, method_ """ Ensure that the method and fields meet the criteria of AIP-4235. Use the helper function get_auto_populated_fields_candidates() to return fields - which meet the criteria of AIP-4235. - https://google.aip.dev/client-libraries/4235 + which meet the criteria of AIP-4235 (https://google.aip.dev/client-libraries/4235). """ # The method must not be a streaming rpc From e7f8a02f0863a8849cba93b872c142b97e816ddb Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 10:13:02 +0000 Subject: [PATCH 10/36] grammar --- gapic/schema/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 89e1721a7e..1b526b56fa 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -652,7 +652,7 @@ def _check_service_yaml_validity(method_in_scope: MethodDescriptorProto, method_ @cached_property def all_methods(self) -> Mapping[str, MethodDescriptorProto]: - """Return a map of all API-wide methods. + """Return a map of all methods for the API. Return: Mapping[str, MethodDescriptorProto]: A mapping of MethodDescriptorProto From c3563c18660e4bef34f62381f91ba22f0e42f5a8 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 11:31:29 +0000 Subject: [PATCH 11/36] refactor for readability --- gapic/schema/api.py | 154 ++++++++++++++++++---------------- tests/unit/schema/test_api.py | 11 ++- 2 files changed, 86 insertions(+), 79 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 1b526b56fa..854f4c571b 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -561,94 +561,31 @@ def mixin_http_options(self): res[s] = [rule for rule in opt_gen if rule] return res - def get_auto_populated_fields_candidates( - self, - method_descriptor: MethodDescriptorProto, - ) -> Sequence[str]: - """Return fields in the request message for a given method which meet the - criteria for being automatically populated. For a field to be automatically - populated, all the configurations below must be true according to - https://google.aip.dev/client-libraries/4235: - - - The field must be of type string - - The field must be at the top-level of the request message - - The field must not be annotated with google.api.field_behavior = REQUIRED. - - The field must be annotated with google.api.field_info.format = UUID4. - - The field presence requirements in AIP-4235 are checked at run time. - - Args: - message[MethodDescriptorProto]: Describes a method of a service. - Return: - Sequence[str]: A list of candidates to be automatically populated - according to https://google.aip.dev/client-libraries/4235. - """ - top_level_request_message = self.messages[ - method_descriptor.input_type.lstrip(".") - ] - return [ - field.name - for field in top_level_request_message.fields.values() - if field.type == wrappers.PrimitiveType.build(str) - and not field.required - and field.uuid4 - ] - def get_auto_populated_fields( self, method_settings: client_pb2.MethodSettings, ) -> Sequence[str]: """ - Return a list of fields with requested auto-population in the `method_settings`. + Return a list of fields to be automatically populated as defined in + `method_settings.auto_populated_fields`. Args: - method_settings (client_pb2.MethodSettings): The settings - to be used when generating API methods. + method_settings (client_pb2.MethodSettings): The settings to be used + when generating API methods. Return: Sequence[str]: A sequence of all fields to be automatically populated, taking into account the requirements - from AIP-4235. - https://google.aip.dev/client-libraries/4235 + from AIP-4235 (https://google.aip.dev/client-libraries/4235). Raises: - ValueError: if `auto_populated_fields` are incorrectly set in the service config + ValueError: if `method_settings.auto_populated_fields` do not meet + the requirements of AIP-4235 + (https://google.aip.dev/client-libraries/4235). """ - auto_populated_fields = [] - def _check_service_yaml_validity(method_in_scope: MethodDescriptorProto, method_settings: client_pb2.MethodSettings): - """ - Ensure that the method and fields meet the criteria of AIP-4235. - Use the helper function get_auto_populated_fields_candidates() to return fields - which meet the criteria of AIP-4235 (https://google.aip.dev/client-libraries/4235). - """ - - # The method must not be a streaming rpc - if method_in_scope.client_streaming or method_in_scope.server_streaming: - raise ValueError( - f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." - ) - # The field name must be listed in the google.api.MethodSettings.auto_populated_fields entry in google.api.Publishing.method_settings for the target method. - candidates = self.get_auto_populated_fields_candidates( - method_in_scope - ) - for field in method_settings.auto_populated_fields: - if field not in candidates: - raise ValueError( - f"Field {field} is not valid as an auto populated field for the top level request message of selector {method_settings.selector}" - ) - - # Ensure that the method defined in the service YAML exists - try: - method_in_scope = self.all_methods[method_settings.selector] - except KeyError: - raise ValueError( - f"Selector {method_settings.selector} is not a valid method in this API." - ) - - if method_settings.auto_populated_fields: - _check_service_yaml_validity(method_in_scope, method_settings) - auto_populated_fields = method_settings.auto_populated_fields + self.check_method_settings_validity(method_settings) - return auto_populated_fields + return method_settings.auto_populated_fields @cached_property def all_methods(self) -> Mapping[str, MethodDescriptorProto]: @@ -664,6 +601,77 @@ def all_methods(self) -> Mapping[str, MethodDescriptorProto]: for method_key, method_value in service_value.methods.items() } + def check_method_settings_validity( + self, method_settings: client_pb2.MethodSettings + ) -> None: + """ + Check `method_settings` for validity. If `method_settings.auto_populated_fields` + is set, verify each field against the criteria of AIP-4235 + (https://google.aip.dev/client-libraries/4235). All the configurations + below must be true: + + - The field must be of type string + - The field must be at the top-level of the request message + - The RPC must be a unary RPC (i.e. streaming RPCs are not supported) + - The field must not be annotated with google.api.field_behavior = REQUIRED. + - The field must be annotated with google.api.field_info.format = UUID4. + - The field presence requirements in AIP-4235 are checked at run time. + + Args: + method_settings (client_pb2.MethodSettings): The settings to be used + when generating API methods. + Return: + None + Raises: + ValueError: if fields in `method_settings.auto_populated_fields` cannot be + automatically populated. + """ + + try: + method_descriptor = self.all_methods[method_settings.selector] + except KeyError: + raise ValueError( + f"Selector {method_settings.selector} is not a valid method in this API." + ) + + if method_settings.auto_populated_fields: + # The method must not be a streaming rpc. + if method_descriptor.client_streaming or method_descriptor.server_streaming: + raise ValueError( + f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." + ) + + top_level_request_message = self.messages[ + method_descriptor.input_type.lstrip(".") + ] + message: str = None + for field_str in method_settings.auto_populated_fields: + if field_str not in top_level_request_message.fields: + message = ( + f"Field `{field_str}` is not in the top level request message." + ) + else: + field = top_level_request_message.fields[field_str] + if field.type != wrappers.PrimitiveType.build(str): + message.append( + f"Field `{field}` is not of type string.\n" + ) + if field.required: + message.append( + f"Field `{field}` is a required field.\n" + ) + if not field.uuid4: + message.append( + f"Field `{field}` is not a UUID4 field.\n" + ) + + if message: + raise ValueError( + f"Field cannot be automatically populated " + f"in the top level request message of selector {method_settings.selector}. " + f"{message}" + ) + @cached_property def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: """Return a map of API-wide `MethodSettings` from the service YAML config to be used diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index e2ce370763..847f605695 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2613,9 +2613,9 @@ def test_get_method_settings(): https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 """ field_options = descriptor_pb2.FieldOptions() - field_options.Extensions[field_info_pb2.field_info].format = ( - field_info_pb2.FieldInfo.Format.Value("UUID4") - ) + field_options.Extensions[ + field_info_pb2.field_info + ].format = field_info_pb2.FieldInfo.Format.Value("UUID4") # See AIP https://google.aip.dev/client-libraries/4235 # Only fields which are not required should be auto-populated @@ -2638,8 +2638,7 @@ def test_get_method_settings(): package="google.example.v1beta1", messages=( make_message_pb2( - name="ExampleRequest", - fields=(squid, mollusc, clam) + name="ExampleRequest", fields=(squid, mollusc, clam) ), make_message_pb2(name="ExampleResponse", fields=()), make_message_pb2(name="AnotherRequest", fields=(squid,)), @@ -2857,6 +2856,6 @@ def test_get_method_settings(): with pytest.raises( ValueError, - match="Field doesnotexist is not valid as an auto populated field for the top level request message of selector google.example.v1beta1.SomeExample.Example1", + match="Field cannot be automatically populated in the top level request message of selector google.example.v1beta1.SomeExample.Example1. Field `doesnotexist` is not in the top level request message.", ): api_schema.all_method_settings From 8a9bbcb46b0b2bfd3f8813bc48a24603328b99e8 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 11:44:30 +0000 Subject: [PATCH 12/36] refactor for readability --- gapic/schema/api.py | 120 ++++++++++++++++++-------------------------- 1 file changed, 49 insertions(+), 71 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 854f4c571b..01ed3c2bcf 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -561,32 +561,6 @@ def mixin_http_options(self): res[s] = [rule for rule in opt_gen if rule] return res - def get_auto_populated_fields( - self, - method_settings: client_pb2.MethodSettings, - ) -> Sequence[str]: - """ - Return a list of fields to be automatically populated as defined in - `method_settings.auto_populated_fields`. - - Args: - method_settings (client_pb2.MethodSettings): The settings to be used - when generating API methods. - Return: - Sequence[str]: A sequence of all fields to be automatically - populated, taking into account the requirements - from AIP-4235 (https://google.aip.dev/client-libraries/4235). - - Raises: - ValueError: if `method_settings.auto_populated_fields` do not meet - the requirements of AIP-4235 - (https://google.aip.dev/client-libraries/4235). - """ - - self.check_method_settings_validity(method_settings) - - return method_settings.auto_populated_fields - @cached_property def all_methods(self) -> Mapping[str, MethodDescriptorProto]: """Return a map of all methods for the API. @@ -602,10 +576,11 @@ def all_methods(self) -> Mapping[str, MethodDescriptorProto]: } def check_method_settings_validity( - self, method_settings: client_pb2.MethodSettings + self, all_method_settings: Sequence[client_pb2.MethodSettings] ) -> None: """ - Check `method_settings` for validity. If `method_settings.auto_populated_fields` + Checks each `google.api.client.MethodSettings` for validity. If + `google.api.client.MethodSettings.auto_populated_fields` is set, verify each field against the criteria of AIP-4235 (https://google.aip.dev/client-libraries/4235). All the configurations below must be true: @@ -618,7 +593,7 @@ def check_method_settings_validity( - The field presence requirements in AIP-4235 are checked at run time. Args: - method_settings (client_pb2.MethodSettings): The settings to be used + all_method_settings (Sequence[client_pb2.MethodSettings]): Method settings to be used when generating API methods. Return: None @@ -627,50 +602,51 @@ def check_method_settings_validity( automatically populated. """ - try: - method_descriptor = self.all_methods[method_settings.selector] - except KeyError: - raise ValueError( - f"Selector {method_settings.selector} is not a valid method in this API." - ) - - if method_settings.auto_populated_fields: - # The method must not be a streaming rpc. - if method_descriptor.client_streaming or method_descriptor.server_streaming: + for method_settings in all_method_settings: + try: + method_descriptor = self.all_methods[method_settings.selector] + except KeyError: raise ValueError( - f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." + f"Selector {method_settings.selector} is not a valid method in this API." ) - top_level_request_message = self.messages[ - method_descriptor.input_type.lstrip(".") - ] - message: str = None - for field_str in method_settings.auto_populated_fields: - if field_str not in top_level_request_message.fields: - message = ( - f"Field `{field_str}` is not in the top level request message." + if method_settings.auto_populated_fields: + # The method must not be a streaming rpc. + if method_descriptor.client_streaming or method_descriptor.server_streaming: + raise ValueError( + f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." ) - else: - field = top_level_request_message.fields[field_str] - if field.type != wrappers.PrimitiveType.build(str): - message.append( - f"Field `{field}` is not of type string.\n" - ) - if field.required: - message.append( - f"Field `{field}` is a required field.\n" - ) - if not field.uuid4: - message.append( - f"Field `{field}` is not a UUID4 field.\n" + + top_level_request_message = self.messages[ + method_descriptor.input_type.lstrip(".") + ] + message: str = None + for field_str in method_settings.auto_populated_fields: + if field_str not in top_level_request_message.fields: + message = ( + f"Field `{field_str}` is not in the top level request message." ) + else: + field = top_level_request_message.fields[field_str] + if field.type != wrappers.PrimitiveType.build(str): + message.append( + f"Field `{field}` is not of type string.\n" + ) + if field.required: + message.append( + f"Field `{field}` is a required field.\n" + ) + if not field.uuid4: + message.append( + f"Field `{field}` is not a UUID4 field.\n" + ) - if message: - raise ValueError( - f"Field cannot be automatically populated " - f"in the top level request message of selector {method_settings.selector}. " - f"{message}" - ) + if message: + raise ValueError( + f"Field cannot be automatically populated " + f"in the top level request message of selector {method_settings.selector}. " + f"{message}" + ) @cached_property def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: @@ -679,20 +655,22 @@ def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 Return: - Mapping[str, Sequence[client_pb2.MethodSettings]]: A mapping of all API-wide method + Mapping[str, Sequence[client_pb2.MethodSettings]]: A mapping of all method settings read from the service YAML, taking into account the requirements from AIPs, such as AIP-4235. Raises: ValueError: if `auto_populated_fields` are incorrectly set in the service config """ + self.check_method_settings_validity( + self.service_yaml_config.publishing.method_settings + ) + return { method_setting.selector: client_pb2.MethodSettings( selector=method_setting.selector, long_running=method_setting.long_running, - auto_populated_fields=self.get_auto_populated_fields( - method_setting - ), + auto_populated_fields=method_setting.auto_populated_fields, ) for method_setting in self.service_yaml_config.publishing.method_settings } From dc7ac894f1853a3c5959d5703a7d8fb8e422e03b Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 11:47:48 +0000 Subject: [PATCH 13/36] update comment --- gapic/schema/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 01ed3c2bcf..a626cd788d 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -650,7 +650,7 @@ def check_method_settings_validity( @cached_property def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: - """Return a map of API-wide `MethodSettings` from the service YAML config to be used + """Return a map of all `google.api.client.MethodSettings` to be used when generating methods. https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 From 42cb7e82e8aaa1b75cc879745dca51bc7f0b069a Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 11:52:16 +0000 Subject: [PATCH 14/36] mypy --- gapic/schema/api.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index a626cd788d..3358cc590c 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -620,24 +620,24 @@ def check_method_settings_validity( top_level_request_message = self.messages[ method_descriptor.input_type.lstrip(".") ] - message: str = None + message: str = "" for field_str in method_settings.auto_populated_fields: if field_str not in top_level_request_message.fields: - message = ( - f"Field `{field_str}` is not in the top level request message." + message.join( + f"Field `{field_str}` is not in the top level request message.\n" ) else: field = top_level_request_message.fields[field_str] if field.type != wrappers.PrimitiveType.build(str): - message.append( + message.join( f"Field `{field}` is not of type string.\n" ) if field.required: - message.append( + message.join( f"Field `{field}` is a required field.\n" ) if not field.uuid4: - message.append( + message.join( f"Field `{field}` is not a UUID4 field.\n" ) From 1d03fb2ab6f8d2f6933a1a8aa78300edaf47bb2b Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 11:53:54 +0000 Subject: [PATCH 15/36] formatting --- gapic/schema/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 3358cc590c..a28e32daae 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -643,8 +643,8 @@ def check_method_settings_validity( if message: raise ValueError( - f"Field cannot be automatically populated " - f"in the top level request message of selector {method_settings.selector}. " + f"Field cannot be automatically populated in the top level " + f"request message of selector {method_settings.selector}. " f"{message}" ) From 2499b598b99d5409167d2d0e431cd317a2bf10d5 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 11:57:19 +0000 Subject: [PATCH 16/36] update comment --- gapic/schema/api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index a28e32daae..0385df496e 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -656,11 +656,11 @@ def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings Return: Mapping[str, Sequence[client_pb2.MethodSettings]]: A mapping of all method - settings read from the service YAML, taking into account the requirements - from AIPs, such as AIP-4235. + settings read from the service YAML. Raises: - ValueError: if `auto_populated_fields` are incorrectly set in the service config + ValueError: if the method settings do not meet the requirements of AIPs, + including AIP-4235 (https://google.aip.dev/client-libraries/4235). """ self.check_method_settings_validity( self.service_yaml_config.publishing.method_settings From c55fa67ed96f0b983cf99fe65ca9f70c24d2a6a5 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 12:00:46 +0000 Subject: [PATCH 17/36] all_method_settings->service_method_settings --- gapic/schema/api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 0385df496e..e7a23c53f8 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -576,7 +576,7 @@ def all_methods(self) -> Mapping[str, MethodDescriptorProto]: } def check_method_settings_validity( - self, all_method_settings: Sequence[client_pb2.MethodSettings] + self, service_method_settings: Sequence[client_pb2.MethodSettings] ) -> None: """ Checks each `google.api.client.MethodSettings` for validity. If @@ -593,7 +593,7 @@ def check_method_settings_validity( - The field presence requirements in AIP-4235 are checked at run time. Args: - all_method_settings (Sequence[client_pb2.MethodSettings]): Method settings to be used + service_method_settings (Sequence[client_pb2.MethodSettings]): Method settings to be used when generating API methods. Return: None @@ -602,7 +602,7 @@ def check_method_settings_validity( automatically populated. """ - for method_settings in all_method_settings: + for method_settings in service_method_settings: try: method_descriptor = self.all_methods[method_settings.selector] except KeyError: From 349735c0549dac971c084e05feae41f50f520ba5 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 12:06:02 +0000 Subject: [PATCH 18/36] clean up --- gapic/schema/api.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index e7a23c53f8..8b79e5b8a6 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -623,23 +623,15 @@ def check_method_settings_validity( message: str = "" for field_str in method_settings.auto_populated_fields: if field_str not in top_level_request_message.fields: - message.join( - f"Field `{field_str}` is not in the top level request message.\n" - ) + message += f"Field `{field_str}` is not in the top level request message.\n" else: field = top_level_request_message.fields[field_str] if field.type != wrappers.PrimitiveType.build(str): - message.join( - f"Field `{field}` is not of type string.\n" - ) + message += f"Field `{field}` is not of type string.\n" if field.required: - message.join( - f"Field `{field}` is a required field.\n" - ) + message += f"Field `{field}` is a required field.\n" if not field.uuid4: - message.join( - f"Field `{field}` is not a UUID4 field.\n" - ) + message += f"Field `{field}` is not a UUID4 field.\n" if message: raise ValueError( From 0c7d073b65bfc06e6c298fa3843c1d185f8a9584 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 12:25:23 +0000 Subject: [PATCH 19/36] fix coverage --- gapic/schema/api.py | 38 ++++++++++++++++------------------- tests/unit/schema/test_api.py | 22 ++++++++++++-------- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 8b79e5b8a6..b64b271e79 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -602,43 +602,39 @@ def check_method_settings_validity( automatically populated. """ + message: str = "" for method_settings in service_method_settings: - try: - method_descriptor = self.all_methods[method_settings.selector] - except KeyError: - raise ValueError( - f"Selector {method_settings.selector} is not a valid method in this API." - ) - + method_descriptor = self.all_methods.get(method_settings.selector) + if not method_descriptor: + message += f"Selector {method_settings.selector} is not a valid method in this API. " + continue if method_settings.auto_populated_fields: # The method must not be a streaming rpc. if method_descriptor.client_streaming or method_descriptor.server_streaming: - raise ValueError( - f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." - ) + message = f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." + continue top_level_request_message = self.messages[ method_descriptor.input_type.lstrip(".") ] - message: str = "" for field_str in method_settings.auto_populated_fields: if field_str not in top_level_request_message.fields: - message += f"Field `{field_str}` is not in the top level request message.\n" + message += f"Field `{field_str}` is not in the top level request message. " else: field = top_level_request_message.fields[field_str] if field.type != wrappers.PrimitiveType.build(str): - message += f"Field `{field}` is not of type string.\n" + message += f"Field `{field_str}` is not of type string. " if field.required: - message += f"Field `{field}` is a required field.\n" + message += f"Field `{field_str}` is a required field. " if not field.uuid4: - message += f"Field `{field}` is not a UUID4 field.\n" + message += f"Field `{field_str}` is not a UUID4 field. " - if message: - raise ValueError( - f"Field cannot be automatically populated in the top level " - f"request message of selector {method_settings.selector}. " - f"{message}" - ) + if message: + raise ValueError( + f"Fields cannot be automatically populated in the top level " + f"request message of selector {method_settings.selector}. " + f"{message.strip()}" + ) @cached_property def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 847f605695..fd9c3197d1 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2618,7 +2618,7 @@ def test_get_method_settings(): ].format = field_info_pb2.FieldInfo.Format.Value("UUID4") # See AIP https://google.aip.dev/client-libraries/4235 - # Only fields which are not required should be auto-populated + # for the specific requirements to be tested. squid = make_field_pb2( name="squid", type="TYPE_STRING", options=field_options, number=1 ) @@ -2632,13 +2632,17 @@ def test_get_method_settings(): clam = make_field_pb2( name="clam", type="TYPE_STRING", options=field_options, number=3 ) + field_options.ClearExtension(field_info_pb2.field_info) + octopus = make_field_pb2( + name="octopus", type="TYPE_INT32", options=field_options, number=4 + ) fd = ( make_file_pb2( name="someexample.proto", package="google.example.v1beta1", messages=( make_message_pb2( - name="ExampleRequest", fields=(squid, mollusc, clam) + name="ExampleRequest", fields=(squid, mollusc, clam, octopus) ), make_message_pb2(name="ExampleResponse", fields=()), make_message_pb2(name="AnotherRequest", fields=(squid,)), @@ -2846,16 +2850,18 @@ def test_get_method_settings(): "auto_populated_fields": [ "doesnotexist", "mollusc", - ], + "octopus", + ] } ] }, } ) api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) - - with pytest.raises( - ValueError, - match="Field cannot be automatically populated in the top level request message of selector google.example.v1beta1.SomeExample.Example1. Field `doesnotexist` is not in the top level request message.", - ): + exception_message = """Fields cannot be automatically populated in the top level \ +request message of selector google.example.v1beta1.SomeExample.Example1. \ +Field `doesnotexist` is not in the top level request message. Field \ +`octopus` is not of type string. Field `octopus` is a required field. \ +Field `octopus` is not a UUID4 field.""" + with pytest.raises(ValueError, match=exception_message): api_schema.all_method_settings From bdf8b10d1da09e4edae272ce6e4f16c2685f8697 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 12:26:55 +0000 Subject: [PATCH 20/36] update comment --- gapic/schema/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index b64b271e79..5694b44522 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -609,9 +609,9 @@ def check_method_settings_validity( message += f"Selector {method_settings.selector} is not a valid method in this API. " continue if method_settings.auto_populated_fields: - # The method must not be a streaming rpc. + # The method must not be a streaming method. if method_descriptor.client_streaming or method_descriptor.server_streaming: - message = f"Selector {method_settings.selector} is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs." + message = f"Selector {method_settings.selector} is a streaming method. `auto_populated_fields` are only supported in unary methods." continue top_level_request_message = self.messages[ From 52b8d42ff1665d0d6420a3805f31d9826ce5c92f Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 12:27:20 +0000 Subject: [PATCH 21/36] grammar --- gapic/schema/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 5694b44522..d9c7c0de6b 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -611,7 +611,7 @@ def check_method_settings_validity( if method_settings.auto_populated_fields: # The method must not be a streaming method. if method_descriptor.client_streaming or method_descriptor.server_streaming: - message = f"Selector {method_settings.selector} is a streaming method. `auto_populated_fields` are only supported in unary methods." + message = f"Selector {method_settings.selector} is a streaming method. `auto_populated_fields` are only supported for unary methods." continue top_level_request_message = self.messages[ From af8e756a25ebf29c22749506ff8733cf585d9af5 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 12:37:35 +0000 Subject: [PATCH 22/36] consolidate test cases --- gapic/schema/api.py | 2 +- tests/unit/schema/test_api.py | 68 +++++++++-------------------------- 2 files changed, 17 insertions(+), 53 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index d9c7c0de6b..9abd53832e 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -611,7 +611,7 @@ def check_method_settings_validity( if method_settings.auto_populated_fields: # The method must not be a streaming method. if method_descriptor.client_streaming or method_descriptor.server_streaming: - message = f"Selector {method_settings.selector} is a streaming method. `auto_populated_fields` are only supported for unary methods." + message += f"Selector {method_settings.selector} is a streaming method. `auto_populated_fields` are only supported for unary methods. " continue top_level_request_message = self.messages[ diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index fd9c3197d1..7c13179bac 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2791,33 +2791,21 @@ def test_get_method_settings(): ], "publishing": { "method_settings": [ + { + "selector": "google.example.v1beta1.SomeExample.Example1", + "auto_populated_fields": [ + "doesnotexist", + "mollusc", + "octopus", + ] + }, { "selector": "google.example.v1beta1.SomeExample.Example2", "auto_populated_fields": [ "squid", "mollusc", ], - } - ] - }, - } - ) - api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) - - with pytest.raises( - ValueError, - match="Selector google.example.v1beta1.SomeExample.Example2 is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs", - ): - api_schema.all_method_settings - - # Test that ValueError is raised when the rpc in publishing.method_settings.selector is not a unary one - opts = Options( - service_yaml_config={ - "apis": [ - {"name": "google.example.v1beta1.SomeExample.Example1"}, - ], - "publishing": { - "method_settings": [ + }, { "selector": "google.example.v1beta1.SomeExample.Example3", "auto_populated_fields": [ @@ -2831,37 +2819,13 @@ def test_get_method_settings(): ) api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) - with pytest.raises( - ValueError, - match="Selector google.example.v1beta1.SomeExample.Example3 is a streaming rpc. `auto_populated_fields` are only supported in unary rpcs", - ): - api_schema.all_method_settings - - # Test that ValueError is raised when the field in publishing.method_settings.auto_populated_fields does not exist - opts = Options( - service_yaml_config={ - "apis": [ - {"name": "google.example.v1beta1.SomeExample.Example1"}, - ], - "publishing": { - "method_settings": [ - { - "selector": "google.example.v1beta1.SomeExample.Example1", - "auto_populated_fields": [ - "doesnotexist", - "mollusc", - "octopus", - ] - } - ] - }, - } - ) - api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) exception_message = """Fields cannot be automatically populated in the top level \ -request message of selector google.example.v1beta1.SomeExample.Example1. \ -Field `doesnotexist` is not in the top level request message. Field \ -`octopus` is not of type string. Field `octopus` is a required field. \ -Field `octopus` is not a UUID4 field.""" +request message of selector google.example.v1beta1.SomeExample.Example3. Field \ +`doesnotexist` is not in the top level request message. Field `octopus` is not \ +of type string. Field `octopus` is a required field. Field `octopus` is not a \ +UUID4 field. Selector google.example.v1beta1.SomeExample.Example2 is a \ +streaming method. `auto_populated_fields` are only supported for unary \ +methods. Selector google.example.v1beta1.SomeExample.Example3 is a streaming \ +method. `auto_populated_fields` are only supported for unary methods.""" with pytest.raises(ValueError, match=exception_message): api_schema.all_method_settings From 0c374ba5849034651369aba9f55546251a889ebe Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 12:41:13 +0000 Subject: [PATCH 23/36] consolidate test cases --- tests/unit/schema/test_api.py | 39 +++++++++-------------------------- 1 file changed, 10 insertions(+), 29 deletions(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 7c13179bac..88f5b266be 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2756,7 +2756,7 @@ def test_get_method_settings(): ), } - # Test that ValueError is raised when there is an invalid value for publishing.method_settings.selector + # Test that ValueError is raised when the rpc in publishing.method_settings.selector is not a unary one opts = Options( service_yaml_config={ "apis": [ @@ -2770,27 +2770,7 @@ def test_get_method_settings(): "squid", "mollusc", ], - } - ] - }, - } - ) - api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) - - with pytest.raises( - ValueError, - match="Selector google.example.v1beta1.DoesNotExist.Example1 is not a valid method in this API", - ): - api_schema.all_method_settings - - # Test that ValueError is raised when the rpc in publishing.method_settings.selector is not a unary one - opts = Options( - service_yaml_config={ - "apis": [ - {"name": "google.example.v1beta1.SomeExample.Example1"}, - ], - "publishing": { - "method_settings": [ + }, { "selector": "google.example.v1beta1.SomeExample.Example1", "auto_populated_fields": [ @@ -2820,12 +2800,13 @@ def test_get_method_settings(): api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) exception_message = """Fields cannot be automatically populated in the top level \ -request message of selector google.example.v1beta1.SomeExample.Example3. Field \ -`doesnotexist` is not in the top level request message. Field `octopus` is not \ -of type string. Field `octopus` is a required field. Field `octopus` is not a \ -UUID4 field. Selector google.example.v1beta1.SomeExample.Example2 is a \ -streaming method. `auto_populated_fields` are only supported for unary \ -methods. Selector google.example.v1beta1.SomeExample.Example3 is a streaming \ -method. `auto_populated_fields` are only supported for unary methods.""" +request message of selector google.example.v1beta1.SomeExample.Example3. Selector \ +google.example.v1beta1.DoesNotExist.Example1 is not a valid method in this API. \ +Field `doesnotexist` is not in the top level request message. Field `octopus` is not \ +of type string. Field `octopus` is a required field. Field `octopus` is not a UUID4 \ +field. Selector google.example.v1beta1.SomeExample.Example2 is a streaming method. \ +`auto_populated_fields` are only supported for unary methods. Selector \ +google.example.v1beta1.SomeExample.Example3 is a streaming method. \ +`auto_populated_fields` are only supported for unary methods.""" with pytest.raises(ValueError, match=exception_message): api_schema.all_method_settings From 588d215ff474eaed9cd2f75902297c7af3895d99 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 13:14:03 +0000 Subject: [PATCH 24/36] add test case --- tests/unit/schema/test_api.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 88f5b266be..921afdca6c 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2727,7 +2727,14 @@ def test_get_method_settings(): opts = Options(service_yaml_config=service_yaml_config) api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) - + assert len(api_schema.all_methods) == 5 + assert list(api_schema.all_methods.keys()) == [ + 'google.example.v1beta1.SomeExample.Example1', + 'google.example.v1beta1.SomeExample.Example2', + 'google.example.v1beta1.SomeExample.Example3', + 'google.example.v1beta1.SomeExample.Example4', + 'google.example.v1beta1.SomeExample.Example5' + ] assert api_schema.all_method_settings == { "google.example.v1beta1.SomeExample.Example1": client_pb2.MethodSettings( selector="google.example.v1beta1.SomeExample.Example1", From 67335de3d84941770b8a601f0007746914a0e2d0 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 13:17:24 +0000 Subject: [PATCH 25/36] clean up --- tests/unit/schema/test_api.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 921afdca6c..dcdfe73b75 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2632,6 +2632,7 @@ def test_get_method_settings(): clam = make_field_pb2( name="clam", type="TYPE_STRING", options=field_options, number=3 ) + # Also include a field which is not of type string field_options.ClearExtension(field_info_pb2.field_info) octopus = make_field_pb2( name="octopus", type="TYPE_INT32", options=field_options, number=4 @@ -2654,33 +2655,28 @@ def test_get_method_settings(): method=( descriptor_pb2.MethodDescriptorProto( name="Example1", - # Input and output types don't matter. input_type="google.example.v1beta1.ExampleRequest", output_type="google.example.v1beta1.ExampleResponse", ), descriptor_pb2.MethodDescriptorProto( name="Example2", - # Input and output types don't matter. input_type="google.example.v1beta1.ExampleRequest", output_type="google.example.v1beta1.ExampleResponse", client_streaming=True, ), descriptor_pb2.MethodDescriptorProto( name="Example3", - # Input and output types don't matter. input_type="google.example.v1beta1.ExampleRequest", output_type="google.example.v1beta1.ExampleResponse", server_streaming=True, ), descriptor_pb2.MethodDescriptorProto( name="Example4", - # Input and output types don't matter. input_type="google.example.v1beta1.AnotherRequest", output_type="google.example.v1beta1.AnotherResponse", ), descriptor_pb2.MethodDescriptorProto( name="Example5", - # Input and output types don't matter. input_type="google.example.v1beta1.AnotherRequest", output_type="google.example.v1beta1.AnotherResponse", ), From 57493b0518a6e14cafcef82736c9bbb514dfed54 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 13:31:41 +0000 Subject: [PATCH 26/36] fix test case --- gapic/schema/api.py | 28 ++++++++++++++++++---------- tests/unit/schema/test_api.py | 5 ++++- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 9abd53832e..51ee7eab62 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -602,38 +602,46 @@ def check_method_settings_validity( automatically populated. """ - message: str = "" + all_errors: str = "" + invalid_selectors = [] for method_settings in service_method_settings: method_descriptor = self.all_methods.get(method_settings.selector) if not method_descriptor: - message += f"Selector {method_settings.selector} is not a valid method in this API. " + all_errors += f"Selector {method_settings.selector} is not a valid method in this API. " + invalid_selectors.append(method_settings.selector) continue if method_settings.auto_populated_fields: # The method must not be a streaming method. if method_descriptor.client_streaming or method_descriptor.server_streaming: - message += f"Selector {method_settings.selector} is a streaming method. `auto_populated_fields` are only supported for unary methods. " + all_errors += f"Selector {method_settings.selector} is a streaming method. `auto_populated_fields` are only supported for unary methods. " + invalid_selectors.append(method_settings.selector) continue + selector_error_message: str = "" top_level_request_message = self.messages[ method_descriptor.input_type.lstrip(".") ] for field_str in method_settings.auto_populated_fields: if field_str not in top_level_request_message.fields: - message += f"Field `{field_str}` is not in the top level request message. " + selector_error_message += f"Field `{field_str}` is not in the top level request message. " else: field = top_level_request_message.fields[field_str] if field.type != wrappers.PrimitiveType.build(str): - message += f"Field `{field_str}` is not of type string. " + selector_error_message += f"Field `{field_str}` is not of type string. " if field.required: - message += f"Field `{field_str}` is a required field. " + selector_error_message += f"Field `{field_str}` is a required field. " if not field.uuid4: - message += f"Field `{field_str}` is not a UUID4 field. " + selector_error_message += f"Field `{field_str}` is not a UUID4 field. " - if message: + if selector_error_message: + all_errors += selector_error_message + invalid_selectors.append(method_settings.selector) + + if all_errors: raise ValueError( f"Fields cannot be automatically populated in the top level " - f"request message of selector {method_settings.selector}. " - f"{message.strip()}" + f"request message for selectors: {invalid_selectors}. " + f"{all_errors.strip()}" ) @cached_property diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index dcdfe73b75..6552b6f4c6 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2803,7 +2803,10 @@ def test_get_method_settings(): api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) exception_message = """Fields cannot be automatically populated in the top level \ -request message of selector google.example.v1beta1.SomeExample.Example3. Selector \ +request message for selectors: \['google.example.v1beta1.DoesNotExist.Example1', \ +'google.example.v1beta1.SomeExample.Example1', \ +'google.example.v1beta1.SomeExample.Example2', \ +'google.example.v1beta1.SomeExample.Example3'\]. Selector \ google.example.v1beta1.DoesNotExist.Example1 is not a valid method in this API. \ Field `doesnotexist` is not in the top level request message. Field `octopus` is not \ of type string. Field `octopus` is a required field. Field `octopus` is not a UUID4 \ From 9f01b289bde40c95da019d89ef08963c590ed11c Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 14 Mar 2024 13:33:13 +0000 Subject: [PATCH 27/36] update test name --- tests/unit/schema/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 6552b6f4c6..4cddea6350 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2606,7 +2606,7 @@ def test_has_iam_mixin(): assert api_schema.has_iam_mixin -def test_get_method_settings(): +def test_all_method_settings(): """ Tests the `all_method_settings` method of `gapic.schema.api` which reads `MethodSettings` from the service config YAML. From 5a2d33b8b160a246945e3752e145878581a351b1 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Sun, 17 Mar 2024 09:47:28 +0000 Subject: [PATCH 28/36] address review feedback --- gapic/schema/api.py | 97 ++++--- tests/unit/schema/test_api.py | 468 +++++++++++++++++++++++----------- 2 files changed, 377 insertions(+), 188 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 51ee7eab62..b13824a941 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -20,6 +20,7 @@ import collections import dataclasses import itertools +import json import keyword import os import sys @@ -59,6 +60,15 @@ TRANSPORT_REST = "rest" + +class MethodSettingsError(ValueError): + """ + Raised when `google.api.client_pb2.MethodSettings` contains + an invalid value. + """ + pass + + @dataclasses.dataclass(frozen=True) class Proto: """A representation of a particular proto file within an API.""" @@ -575,14 +585,15 @@ def all_methods(self) -> Mapping[str, MethodDescriptorProto]: for method_key, method_value in service_value.methods.items() } - def check_method_settings_validity( + def enforce_valid_method_settings( self, service_method_settings: Sequence[client_pb2.MethodSettings] ) -> None: """ - Checks each `google.api.client.MethodSettings` for validity. If + Checks each `google.api.client.MethodSettings` provided for validity and + raises an exception if invalid values are found. If `google.api.client.MethodSettings.auto_populated_fields` is set, verify each field against the criteria of AIP-4235 - (https://google.aip.dev/client-libraries/4235). All the configurations + (https://google.aip.dev/client-libraries/4235). All of the conditions below must be true: - The field must be of type string @@ -590,59 +601,75 @@ def check_method_settings_validity( - The RPC must be a unary RPC (i.e. streaming RPCs are not supported) - The field must not be annotated with google.api.field_behavior = REQUIRED. - The field must be annotated with google.api.field_info.format = UUID4. - - The field presence requirements in AIP-4235 are checked at run time. + + Note that the field presence requirements in AIP-4235 should be checked at run + time. Args: - service_method_settings (Sequence[client_pb2.MethodSettings]): Method settings to be used - when generating API methods. + service_method_settings (Sequence[client_pb2.MethodSettings]): Method + settings to be used when generating API methods. Return: None Raises: - ValueError: if fields in `method_settings.auto_populated_fields` cannot be - automatically populated. + MethodSettingsError: if fields in `method_settings.auto_populated_fields` + cannot be automatically populated. """ - all_errors: str = "" - invalid_selectors = [] + all_errors: dict = {} + selectors_seen = [] for method_settings in service_method_settings: + # Check if this selector is defind more than once + if method_settings.selector in selectors_seen: + all_errors[method_settings.selector] = ["Duplicate selector"] + continue + selectors_seen.append(method_settings.selector) + method_descriptor = self.all_methods.get(method_settings.selector) + # Check if this selector can be mapped to a method in the API. if not method_descriptor: - all_errors += f"Selector {method_settings.selector} is not a valid method in this API. " - invalid_selectors.append(method_settings.selector) + all_errors[method_settings.selector] = [ + "Method was not found." + ] continue + if method_settings.auto_populated_fields: - # The method must not be a streaming method. - if method_descriptor.client_streaming or method_descriptor.server_streaming: - all_errors += f"Selector {method_settings.selector} is a streaming method. `auto_populated_fields` are only supported for unary methods. " - invalid_selectors.append(method_settings.selector) + # Check if the selector maps to a streaming method + if ( + method_descriptor.client_streaming + or method_descriptor.server_streaming + ): + all_errors[method_settings.selector] = [ + "Method is not a unary method." + ] continue - - selector_error_message: str = "" top_level_request_message = self.messages[ method_descriptor.input_type.lstrip(".") ] + selector_errors = [] for field_str in method_settings.auto_populated_fields: if field_str not in top_level_request_message.fields: - selector_error_message += f"Field `{field_str}` is not in the top level request message. " + selector_errors.append( + f"Field `{field_str}` was not found" + ) else: field = top_level_request_message.fields[field_str] if field.type != wrappers.PrimitiveType.build(str): - selector_error_message += f"Field `{field_str}` is not of type string. " + selector_errors.append( + f"Field `{field_str}` is not of type string." + ) if field.required: - selector_error_message += f"Field `{field_str}` is a required field. " + selector_errors.append( + f"Field `{field_str}` is a required field." + ) if not field.uuid4: - selector_error_message += f"Field `{field_str}` is not a UUID4 field. " - - if selector_error_message: - all_errors += selector_error_message - invalid_selectors.append(method_settings.selector) - + selector_errors.append( + f"Field `{field_str}` is not annotated with " + "`google.api.field_info.format = \"UUID4\"." + ) + if selector_errors: + all_errors[method_settings.selector] = selector_errors if all_errors: - raise ValueError( - f"Fields cannot be automatically populated in the top level " - f"request message for selectors: {invalid_selectors}. " - f"{all_errors.strip()}" - ) + raise MethodSettingsError(json.dumps(all_errors)) @cached_property def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: @@ -655,10 +682,10 @@ def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings settings read from the service YAML. Raises: - ValueError: if the method settings do not meet the requirements of AIPs, - including AIP-4235 (https://google.aip.dev/client-libraries/4235). + gapic.schema.api.MethodSettingsError: if the method settings do not + meet the requirements of https://google.aip.dev/client-libraries/4235. """ - self.check_method_settings_validity( + self.enforce_valid_method_settings( self.service_yaml_config.publishing.method_settings ) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 4cddea6350..6888447727 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -13,6 +13,9 @@ # limitations under the License. import collections +import json +import re +from typing import Sequence from unittest import mock @@ -2606,48 +2609,29 @@ def test_has_iam_mixin(): assert api_schema.has_iam_mixin -def test_all_method_settings(): +def get_file_descriptor_proto_for_method_settings_tests( + fields: Sequence[descriptor_pb2.FieldDescriptorProto] = None, + client_streaming: bool = False, + server_streaming: bool = False, +) -> descriptor_pb2.FileDescriptorProto: """ - Tests the `all_method_settings` method of `gapic.schema.api` which - reads `MethodSettings` from the service config YAML. - https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 + Args: + fields (Sequence[descriptor_pb2.FieldDescriptorProto]): Fields to include + in messages in the return object `descriptor_pb2.FileDescriptorProto`. + client_streaming (bool): Whether the methods in the return object + `descriptor_pb2.FileDescriptorProto` should use client streaming. + server_streaming (bool): Whether the methods in the return object + `descriptor_pb2.FileDescriptorProto` should use server streaming. + Return: + descriptor_pb2.FileDescriptorProto: Returns an object describing the API. """ - field_options = descriptor_pb2.FieldOptions() - field_options.Extensions[ - field_info_pb2.field_info - ].format = field_info_pb2.FieldInfo.Format.Value("UUID4") - - # See AIP https://google.aip.dev/client-libraries/4235 - # for the specific requirements to be tested. - squid = make_field_pb2( - name="squid", type="TYPE_STRING", options=field_options, number=1 - ) - mollusc = make_field_pb2( - name="mollusc", type="TYPE_STRING", options=field_options, number=2 - ) - # Also include a required field - field_options.Extensions[field_behavior_pb2.field_behavior].append( - field_behavior_pb2.FieldBehavior.Value("REQUIRED") - ) - clam = make_field_pb2( - name="clam", type="TYPE_STRING", options=field_options, number=3 - ) - # Also include a field which is not of type string - field_options.ClearExtension(field_info_pb2.field_info) - octopus = make_field_pb2( - name="octopus", type="TYPE_INT32", options=field_options, number=4 - ) fd = ( make_file_pb2( name="someexample.proto", package="google.example.v1beta1", messages=( - make_message_pb2( - name="ExampleRequest", fields=(squid, mollusc, clam, octopus) - ), + make_message_pb2(name="ExampleRequest", fields=fields), make_message_pb2(name="ExampleResponse", fields=()), - make_message_pb2(name="AnotherRequest", fields=(squid,)), - make_message_pb2(name="AnotherResponse", fields=()), ), services=( descriptor_pb2.ServiceDescriptorProto( @@ -2657,41 +2641,52 @@ def test_all_method_settings(): name="Example1", input_type="google.example.v1beta1.ExampleRequest", output_type="google.example.v1beta1.ExampleResponse", + client_streaming=client_streaming, + server_streaming=server_streaming, ), + ), + ), + descriptor_pb2.ServiceDescriptorProto( + name="AnotherExample", + method=( descriptor_pb2.MethodDescriptorProto( - name="Example2", - input_type="google.example.v1beta1.ExampleRequest", - output_type="google.example.v1beta1.ExampleResponse", - client_streaming=True, - ), - descriptor_pb2.MethodDescriptorProto( - name="Example3", + name="Example1", input_type="google.example.v1beta1.ExampleRequest", output_type="google.example.v1beta1.ExampleResponse", - server_streaming=True, - ), - descriptor_pb2.MethodDescriptorProto( - name="Example4", - input_type="google.example.v1beta1.AnotherRequest", - output_type="google.example.v1beta1.AnotherResponse", - ), - descriptor_pb2.MethodDescriptorProto( - name="Example5", - input_type="google.example.v1beta1.AnotherRequest", - output_type="google.example.v1beta1.AnotherResponse", + client_streaming=client_streaming, + server_streaming=server_streaming, ), ), ), ), ), ) + return fd + + +def test_api_all_methods(): + """ + Tests the `all_methods` method of `gapic.schema.api` method which returns a map of + all methods for the API. + """ + fd = get_file_descriptor_proto_for_method_settings_tests() + api_schema = api.API.build(fd, "google.example.v1beta1") + assert len(api_schema.all_methods) == 2 + assert list(api_schema.all_methods.keys()) == [ + "google.example.v1beta1.SomeExample.Example1", + "google.example.v1beta1.AnotherExample.Example1", + ] + + +def test_read_method_settings_from_service_yaml(): + """ + Tests the `gapic.schema.api.all_method_settings` method which reads + `MethodSettings` from the service config YAML. + https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325 + """ service_yaml_config = { "apis": [ {"name": "google.example.v1beta1.SomeExample.Example1"}, - {"name": "google.example.v1beta1.SomeExample.Example2"}, - {"name": "google.example.v1beta1.SomeExample.Example3"}, - {"name": "google.example.v1beta1.SomeExample.Example4"}, - {"name": "google.example.v1beta1.SomeExample.Example5"}, ], "publishing": { "method_settings": [ @@ -2702,117 +2697,284 @@ def test_all_method_settings(): "mollusc", ], }, - { - "selector": "google.example.v1beta1.SomeExample.Example2", - "auto_populated_fields": [], - }, - { - "selector": "google.example.v1beta1.SomeExample.Example3", - "auto_populated_fields": [], - }, - { - "selector": "google.example.v1beta1.SomeExample.Example4", - "auto_populated_fields": [], - }, - { - "selector": "google.example.v1beta1.SomeExample.Example5", - }, ] }, } - opts = Options(service_yaml_config=service_yaml_config) + cli_options = Options(service_yaml_config=service_yaml_config) + field_options = descriptor_pb2.FieldOptions() + field_options.Extensions[ + field_info_pb2.field_info + ].format = field_info_pb2.FieldInfo.Format.Value("UUID4") - api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) - assert len(api_schema.all_methods) == 5 - assert list(api_schema.all_methods.keys()) == [ - 'google.example.v1beta1.SomeExample.Example1', - 'google.example.v1beta1.SomeExample.Example2', - 'google.example.v1beta1.SomeExample.Example3', - 'google.example.v1beta1.SomeExample.Example4', - 'google.example.v1beta1.SomeExample.Example5' - ] + squid = make_field_pb2( + name="squid", type="TYPE_STRING", options=field_options, number=1 + ) + mollusc = make_field_pb2( + name="mollusc", type="TYPE_STRING", options=field_options, number=2 + ) + fields = [squid, mollusc] + fd = get_file_descriptor_proto_for_method_settings_tests(fields=fields) + api_schema = api.API.build(fd, "google.example.v1beta1", opts=cli_options) assert api_schema.all_method_settings == { "google.example.v1beta1.SomeExample.Example1": client_pb2.MethodSettings( selector="google.example.v1beta1.SomeExample.Example1", auto_populated_fields=["squid", "mollusc"], long_running=client_pb2.MethodSettings.LongRunning(), + ) + } + + +def test_method_settings_duplicate_selector_raises_error(): + """ + Test that `MethodSettingsError` is raised when there are duplicate selectors in + `client_pb2.MethodSettings`. + """ + fd = get_file_descriptor_proto_for_method_settings_tests() + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", ), - "google.example.v1beta1.SomeExample.Example2": client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example2", - auto_populated_fields=[], - long_running=client_pb2.MethodSettings.LongRunning(), + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", ), - "google.example.v1beta1.SomeExample.Example3": client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example3", - auto_populated_fields=[], - long_running=client_pb2.MethodSettings.LongRunning(), + ] + with pytest.raises( + api.MethodSettingsError, match="(?i)duplicate selector" + ): + api_schema.enforce_valid_method_settings(methodsettings) + + +def test_method_settings_invalid_selector_raises_error(): + """ + Test that `MethodSettingsError` when `client_pb2.MethodSettings.selector` + cannot be mapped to a method in the API. + """ + fd = get_file_descriptor_proto_for_method_settings_tests() + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.DoesNotExist.Example1", ), - "google.example.v1beta1.SomeExample.Example4": client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example4", - auto_populated_fields=[], - long_running=client_pb2.MethodSettings.LongRunning(), + ] + with pytest.raises( + api.MethodSettingsError, match="(?i)method was not found" + ): + api_schema.enforce_valid_method_settings(methodsettings) + + +def test_method_settings_unsupported_auto_populated_field_type_raises_error(): + """ + Test that `MethodSettingsError` is raised when a field in + `client_pb2.MethodSettings.auto_populated_fields` is not of type string. + """ + squid = make_field_pb2(name="squid", type="TYPE_INT32", number=1) + fd = get_file_descriptor_proto_for_method_settings_tests(fields=[squid]) + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", + auto_populated_fields=["squid"], ), - "google.example.v1beta1.SomeExample.Example5": client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example5", - auto_populated_fields=[], - long_running=client_pb2.MethodSettings.LongRunning(), + ] + with pytest.raises(api.MethodSettingsError, match="(?i)type string"): + api_schema.enforce_valid_method_settings(methodsettings) + + +def test_method_settings_auto_populated_field_not_found_raises_error(): + """ + Test that `MethodSettingsError` is raised when a field in + `client_pb2.MethodSettings.auto_populated_fields` is not found in the top-level + request message of the selector. + """ + fd = get_file_descriptor_proto_for_method_settings_tests() + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", + auto_populated_fields=["squid"], ), - } + ] + with pytest.raises(api.MethodSettingsError, match="(?i)not found"): + api_schema.enforce_valid_method_settings(methodsettings) + + +def test_method_settings_auto_populated_field_client_streaming_rpc_raises_error(): + """ + Test that `MethodSettingsError` is raised when the selector in + `client_pb2.MethodSettings.selector` maps to a method which uses client streaming. + """ + fd = get_file_descriptor_proto_for_method_settings_tests( + client_streaming=True + ) + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", + auto_populated_fields=["squid"], + ), + ] + with pytest.raises( + api.MethodSettingsError, match="(?i)not a unary method" + ): + api_schema.enforce_valid_method_settings(methodsettings) + + +def test_method_settings_auto_populated_field_server_streaming_rpc_raises_error(): + """ + Test that `MethodSettingsError` is raised when the selector in + `client_pb2.MethodSettings.selector` maps to a method which uses server streaming. + """ + fd = get_file_descriptor_proto_for_method_settings_tests( + server_streaming=True + ) + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", + auto_populated_fields=["squid"], + ), + ] + with pytest.raises( + api.MethodSettingsError, match="(?i)not a unary method" + ): + api_schema.enforce_valid_method_settings(methodsettings) + + +def test_method_settings_unsupported_auto_populated_field_behavior_raises_error(): + """ + Test that `MethodSettingsError` is raised when a field in + `client_pb2.MethodSettings.auto_populated_fields` is a required field. + """ + field_options = descriptor_pb2.FieldOptions() + field_options.Extensions[field_behavior_pb2.field_behavior].append( + field_behavior_pb2.FieldBehavior.Value("REQUIRED") + ) + squid = make_field_pb2( + name="squid", type="TYPE_STRING", options=field_options, number=1 + ) + fd = get_file_descriptor_proto_for_method_settings_tests(fields=[squid]) + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", + auto_populated_fields=["squid"], + ), + ] + with pytest.raises( + api.MethodSettingsError, match="(?i)required field" + ): + api_schema.enforce_valid_method_settings(methodsettings) + + +def test_method_settings_auto_populated_field_field_info_format_not_specified_raises_error(): + """ + Test that `MethodSettingsError` is raised when a field in + `client_pb2.MethodSettings.auto_populated_fields` is not annotated with + `google.api.field_info.format = UUID4`. For this test case, + the format of the field is not specified. + """ + squid = make_field_pb2(name="squid", type="TYPE_STRING", number=1) + fd = get_file_descriptor_proto_for_method_settings_tests(fields=[squid]) + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", + auto_populated_fields=["squid"], + ), + ] + with pytest.raises(api.MethodSettingsError): + api_schema.enforce_valid_method_settings(methodsettings) + + +def test_method_settings_unsupported_auto_populated_field_field_info_format_raises_error(): + """ + Test that `MethodSettingsError` is raised when a field in + `client_pb2.MethodSettings.auto_populated_fields` is not annotated with + `google.api.field_info.format = UUID4`.For this test case, + the format of the field is `IPV4`. + """ + field_options = descriptor_pb2.FieldOptions() + field_options.Extensions[ + field_info_pb2.field_info + ].format = field_info_pb2.FieldInfo.Format.Value("IPV4") + squid = make_field_pb2( + name="squid", type="TYPE_STRING", options=field_options, number=1 + ) + fd = get_file_descriptor_proto_for_method_settings_tests(fields=[squid]) + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.SomeExample.Example1", + auto_populated_fields=["squid"], + ), + ] + with pytest.raises(api.MethodSettingsError): + api_schema.enforce_valid_method_settings(methodsettings) - # Test that ValueError is raised when the rpc in publishing.method_settings.selector is not a unary one - opts = Options( - service_yaml_config={ - "apis": [ - {"name": "google.example.v1beta1.SomeExample.Example1"}, + +def test_method_settings_invalid_multiple_issues(): + """ + A kitchen sink type of test to ensure `MethodSettingsError` is raised and the contents + of the exception includes sufficient detail describing each issue. + """ + method_example1 = "google.example.v1beta1.SomeExample.Example1" + method_example2 = "google.example.v1beta1.SomeExample2.Example2" + field_options = descriptor_pb2.FieldOptions() + + # Field Squid Errors + # - Not annotated with google.api.field_info.format = UUID4 + # - Not of type string + # - Required field + field_options.Extensions[ + field_info_pb2.field_info + ].format = field_info_pb2.FieldInfo.Format.Value("IPV4") + squid = make_field_pb2( + name="squid", type="TYPE_INT32", options=field_options, number=1 + ) + field_options = descriptor_pb2.FieldOptions() + field_options.Extensions[field_behavior_pb2.field_behavior].append( + field_behavior_pb2.FieldBehavior.Value("REQUIRED") + ) + + # Field Octopus Errors + # - Not annotated with google.api.field_info.format = UUID4 + octopus = make_field_pb2(name="octopus", type="TYPE_STRING", number=1) + fd = get_file_descriptor_proto_for_method_settings_tests( + fields=[squid, octopus] + ) + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector=method_example1, + auto_populated_fields=[ + "squid", + "octopus", ], - "publishing": { - "method_settings": [ - { - "selector": "google.example.v1beta1.DoesNotExist.Example1", - "auto_populated_fields": [ - "squid", - "mollusc", - ], - }, - { - "selector": "google.example.v1beta1.SomeExample.Example1", - "auto_populated_fields": [ - "doesnotexist", - "mollusc", - "octopus", - ] - }, - { - "selector": "google.example.v1beta1.SomeExample.Example2", - "auto_populated_fields": [ - "squid", - "mollusc", - ], - }, - { - "selector": "google.example.v1beta1.SomeExample.Example3", - "auto_populated_fields": [ - "squid", - "mollusc", - ], - } - ] - }, - } + ), + client_pb2.MethodSettings( + selector=method_example2, + auto_populated_fields=["squid", "octopus"], + ), + ] + with pytest.raises(api.MethodSettingsError) as ex: + api_schema.enforce_valid_method_settings(methodsettings) + + error_json = json.loads(ex.value.args[0]) + + assert re.match( + ".*squid.*not.*string.*", + error_json[method_example1][0].lower() + ) + assert re.match( + ".*squid.*not.*uuid4.*", + error_json[method_example1][1].lower() + ) + assert re.match( + ".*octopus.*not.*uuid4.*", + error_json[method_example1][2].lower() + ) + assert re.match( + ".*method.*not found.*", + error_json[method_example2][0].lower() ) - api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts) - - exception_message = """Fields cannot be automatically populated in the top level \ -request message for selectors: \['google.example.v1beta1.DoesNotExist.Example1', \ -'google.example.v1beta1.SomeExample.Example1', \ -'google.example.v1beta1.SomeExample.Example2', \ -'google.example.v1beta1.SomeExample.Example3'\]. Selector \ -google.example.v1beta1.DoesNotExist.Example1 is not a valid method in this API. \ -Field `doesnotexist` is not in the top level request message. Field `octopus` is not \ -of type string. Field `octopus` is a required field. Field `octopus` is not a UUID4 \ -field. Selector google.example.v1beta1.SomeExample.Example2 is a streaming method. \ -`auto_populated_fields` are only supported for unary methods. Selector \ -google.example.v1beta1.SomeExample.Example3 is a streaming method. \ -`auto_populated_fields` are only supported for unary methods.""" - with pytest.raises(ValueError, match=exception_message): - api_schema.all_method_settings From d5d0aee518514cf10a1474ca5ad41aadf098500f Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Mon, 18 Mar 2024 17:03:08 +0000 Subject: [PATCH 29/36] style --- gapic/schema/api.py | 1 - 1 file changed, 1 deletion(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index b13824a941..34620d9996 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -60,7 +60,6 @@ TRANSPORT_REST = "rest" - class MethodSettingsError(ValueError): """ Raised when `google.api.client_pb2.MethodSettings` contains From 73b06cab617845b1d420dd2ef2a2f73545679b2b Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Mon, 18 Mar 2024 16:34:05 -0400 Subject: [PATCH 30/36] fix typo Co-authored-by: Victor Chudnovsky --- gapic/schema/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 34620d9996..887d7853a1 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -610,7 +610,7 @@ def enforce_valid_method_settings( Return: None Raises: - MethodSettingsError: if fields in `method_settings.auto_populated_fields` + MethodSettingsError: if fields in `method_settings.auto_populated_fields` cannot be automatically populated. """ From 4161702a982d0f4c3bd57fe7a393f797b1fe9c4f Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Tue, 19 Mar 2024 01:00:40 +0000 Subject: [PATCH 31/36] json->yaml --- gapic/schema/api.py | 6 +++--- tests/unit/schema/test_api.py | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/gapic/schema/api.py b/gapic/schema/api.py index 887d7853a1..d3aa9f07ca 100644 --- a/gapic/schema/api.py +++ b/gapic/schema/api.py @@ -20,12 +20,12 @@ import collections import dataclasses import itertools -import json import keyword import os import sys -from typing import Callable, Container, Dict, FrozenSet, Mapping, Optional, Sequence, Set, Tuple from types import MappingProxyType +from typing import Callable, Container, Dict, FrozenSet, Mapping, Optional, Sequence, Set, Tuple +import yaml from google.api_core import exceptions from google.api import client_pb2 # type: ignore @@ -668,7 +668,7 @@ def enforce_valid_method_settings( if selector_errors: all_errors[method_settings.selector] = selector_errors if all_errors: - raise MethodSettingsError(json.dumps(all_errors)) + raise MethodSettingsError(yaml.dump(all_errors)) @cached_property def all_method_settings(self) -> Mapping[str, Sequence[client_pb2.MethodSettings]]: diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 6888447727..d75c727319 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -13,11 +13,10 @@ # limitations under the License. import collections -import json import re from typing import Sequence - from unittest import mock +import yaml import pytest @@ -2960,7 +2959,7 @@ def test_method_settings_invalid_multiple_issues(): with pytest.raises(api.MethodSettingsError) as ex: api_schema.enforce_valid_method_settings(methodsettings) - error_json = json.loads(ex.value.args[0]) + error_json = yaml.safe_load(ex.value.args[0]) assert re.match( ".*squid.*not.*string.*", From e1c87858bd12ea813fce637ff23872d6ebb49816 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Tue, 19 Mar 2024 01:07:12 +0000 Subject: [PATCH 32/36] json->yaml --- tests/unit/schema/test_api.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index d75c727319..70ad107483 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2959,21 +2959,21 @@ def test_method_settings_invalid_multiple_issues(): with pytest.raises(api.MethodSettingsError) as ex: api_schema.enforce_valid_method_settings(methodsettings) - error_json = yaml.safe_load(ex.value.args[0]) + error_yaml = yaml.safe_load(ex.value.args[0]) assert re.match( ".*squid.*not.*string.*", - error_json[method_example1][0].lower() + error_yaml[method_example1][0].lower() ) assert re.match( ".*squid.*not.*uuid4.*", - error_json[method_example1][1].lower() + error_yaml[method_example1][1].lower() ) assert re.match( ".*octopus.*not.*uuid4.*", - error_json[method_example1][2].lower() + error_yaml[method_example1][2].lower() ) assert re.match( ".*method.*not found.*", - error_json[method_example2][0].lower() + error_yaml[method_example2][0].lower() ) From 04fa73e4ab7801ddfdd0bdca34e47867b375a7fb Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Tue, 19 Mar 2024 01:24:16 +0000 Subject: [PATCH 33/36] SomeService->ServiceOne;AnotherService->ServiceTwo --- tests/unit/schema/test_api.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 70ad107483..eb51bf34d0 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2634,7 +2634,7 @@ def get_file_descriptor_proto_for_method_settings_tests( ), services=( descriptor_pb2.ServiceDescriptorProto( - name="SomeExample", + name="ServiceOne", method=( descriptor_pb2.MethodDescriptorProto( name="Example1", @@ -2646,7 +2646,7 @@ def get_file_descriptor_proto_for_method_settings_tests( ), ), descriptor_pb2.ServiceDescriptorProto( - name="AnotherExample", + name="ServiceTwo", method=( descriptor_pb2.MethodDescriptorProto( name="Example1", @@ -2672,8 +2672,8 @@ def test_api_all_methods(): api_schema = api.API.build(fd, "google.example.v1beta1") assert len(api_schema.all_methods) == 2 assert list(api_schema.all_methods.keys()) == [ - "google.example.v1beta1.SomeExample.Example1", - "google.example.v1beta1.AnotherExample.Example1", + "google.example.v1beta1.ServiceOne.Example1", + "google.example.v1beta1.ServiceTwo.Example1", ] @@ -2685,12 +2685,12 @@ def test_read_method_settings_from_service_yaml(): """ service_yaml_config = { "apis": [ - {"name": "google.example.v1beta1.SomeExample.Example1"}, + {"name": "google.example.v1beta1.ServiceOne.Example1"}, ], "publishing": { "method_settings": [ { - "selector": "google.example.v1beta1.SomeExample.Example1", + "selector": "google.example.v1beta1.ServiceOne.Example1", "auto_populated_fields": [ "squid", "mollusc", @@ -2715,8 +2715,8 @@ def test_read_method_settings_from_service_yaml(): fd = get_file_descriptor_proto_for_method_settings_tests(fields=fields) api_schema = api.API.build(fd, "google.example.v1beta1", opts=cli_options) assert api_schema.all_method_settings == { - "google.example.v1beta1.SomeExample.Example1": client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example1", + "google.example.v1beta1.ServiceOne.Example1": client_pb2.MethodSettings( + selector="google.example.v1beta1.ServiceOne.Example1", auto_populated_fields=["squid", "mollusc"], long_running=client_pb2.MethodSettings.LongRunning(), ) @@ -2732,10 +2732,10 @@ def test_method_settings_duplicate_selector_raises_error(): api_schema = api.API.build(fd, "google.example.v1beta1") methodsettings = [ client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example1", + selector="google.example.v1beta1.ServiceOne.Example1", ), client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example1", + selector="google.example.v1beta1.ServiceOne.Example1", ), ] with pytest.raises( @@ -2772,7 +2772,7 @@ def test_method_settings_unsupported_auto_populated_field_type_raises_error(): api_schema = api.API.build(fd, "google.example.v1beta1") methodsettings = [ client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example1", + selector="google.example.v1beta1.ServiceOne.Example1", auto_populated_fields=["squid"], ), ] @@ -2790,7 +2790,7 @@ def test_method_settings_auto_populated_field_not_found_raises_error(): api_schema = api.API.build(fd, "google.example.v1beta1") methodsettings = [ client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example1", + selector="google.example.v1beta1.ServiceOne.Example1", auto_populated_fields=["squid"], ), ] @@ -2809,7 +2809,7 @@ def test_method_settings_auto_populated_field_client_streaming_rpc_raises_error( api_schema = api.API.build(fd, "google.example.v1beta1") methodsettings = [ client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example1", + selector="google.example.v1beta1.ServiceOne.Example1", auto_populated_fields=["squid"], ), ] @@ -2830,7 +2830,7 @@ def test_method_settings_auto_populated_field_server_streaming_rpc_raises_error( api_schema = api.API.build(fd, "google.example.v1beta1") methodsettings = [ client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example1", + selector="google.example.v1beta1.ServiceOne.Example1", auto_populated_fields=["squid"], ), ] @@ -2856,7 +2856,7 @@ def test_method_settings_unsupported_auto_populated_field_behavior_raises_error( api_schema = api.API.build(fd, "google.example.v1beta1") methodsettings = [ client_pb2.MethodSettings( - selector="google.example.v1beta1.SomeExample.Example1", + selector="google.example.v1beta1.ServiceOne.Example1", auto_populated_fields=["squid"], ), ] @@ -2917,8 +2917,8 @@ def test_method_settings_invalid_multiple_issues(): A kitchen sink type of test to ensure `MethodSettingsError` is raised and the contents of the exception includes sufficient detail describing each issue. """ - method_example1 = "google.example.v1beta1.SomeExample.Example1" - method_example2 = "google.example.v1beta1.SomeExample2.Example2" + method_example1 = "google.example.v1beta1.ServiceTwo.Example1" + method_example2 = "google.example.v1beta1.ServiceThree.Example2" field_options = descriptor_pb2.FieldOptions() # Field Squid Errors From eac51b69af4a2f3b03102a3e609f62e442a52e3e Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Tue, 19 Mar 2024 01:30:09 +0000 Subject: [PATCH 34/36] add test case for non-existent method --- tests/unit/schema/test_api.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index eb51bf34d0..1ddacfe57d 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2749,18 +2749,34 @@ def test_method_settings_invalid_selector_raises_error(): Test that `MethodSettingsError` when `client_pb2.MethodSettings.selector` cannot be mapped to a method in the API. """ + method_example1 = "google.example.v1beta1.DoesNotExist.Example1" + method_example2 = "google.example.v1beta1.ServiceOne.DoesNotExist" + fd = get_file_descriptor_proto_for_method_settings_tests() api_schema = api.API.build(fd, "google.example.v1beta1") methodsettings = [ client_pb2.MethodSettings( - selector="google.example.v1beta1.DoesNotExist.Example1", + selector=method_example1, + ), + client_pb2.MethodSettings( + selector=method_example2, ), ] - with pytest.raises( - api.MethodSettingsError, match="(?i)method was not found" - ): + + with pytest.raises(api.MethodSettingsError) as ex: api_schema.enforce_valid_method_settings(methodsettings) + error_yaml = yaml.safe_load(ex.value.args[0]) + + assert re.match( + ".*not found.*", + error_yaml[method_example1][0].lower() + ) + assert re.match( + ".*not found.*", + error_yaml[method_example2][0].lower() + ) + def test_method_settings_unsupported_auto_populated_field_type_raises_error(): """ From 4d7b4ca61ade783ddc8b9187d76f913be1a65205 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Tue, 19 Mar 2024 01:35:42 +0000 Subject: [PATCH 35/36] rename field in test_method_settings_auto_populated_field_not_found_raises_error --- tests/unit/schema/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 1ddacfe57d..4a290fa6a3 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -2807,7 +2807,7 @@ def test_method_settings_auto_populated_field_not_found_raises_error(): methodsettings = [ client_pb2.MethodSettings( selector="google.example.v1beta1.ServiceOne.Example1", - auto_populated_fields=["squid"], + auto_populated_fields=["whelk"], ), ] with pytest.raises(api.MethodSettingsError, match="(?i)not found"): From 1ab06eae2427367f3bdf5f7ce5dc659345265e2b Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Tue, 19 Mar 2024 02:05:20 +0000 Subject: [PATCH 36/36] add test case for nested field --- tests/unit/schema/test_api.py | 48 ++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/tests/unit/schema/test_api.py b/tests/unit/schema/test_api.py index 4a290fa6a3..b242fd9e81 100644 --- a/tests/unit/schema/test_api.py +++ b/tests/unit/schema/test_api.py @@ -43,10 +43,10 @@ from test_utils.test_utils import ( make_enum_pb2, + make_field, make_field_pb2, make_file_pb2, make_message_pb2, - make_method, make_naming, make_oneof_pb2, ) @@ -2624,6 +2624,12 @@ def get_file_descriptor_proto_for_method_settings_tests( Return: descriptor_pb2.FileDescriptorProto: Returns an object describing the API. """ + + field_options = descriptor_pb2.FieldOptions() + field_options.Extensions[ + field_info_pb2.field_info + ].format = field_info_pb2.FieldInfo.Format.Value("UUID4") + fd = ( make_file_pb2( name="someexample.proto", @@ -2631,6 +2637,18 @@ def get_file_descriptor_proto_for_method_settings_tests( messages=( make_message_pb2(name="ExampleRequest", fields=fields), make_message_pb2(name="ExampleResponse", fields=()), + make_message_pb2( + name='NestedMessage', + fields=( + make_field_pb2( + name="squid", + options=field_options, + type="TYPE_STRING", + number=1 + ), + ), + options=descriptor_pb2.MessageOptions(map_entry=True), + ) ), services=( descriptor_pb2.ServiceDescriptorProto( @@ -2814,6 +2832,34 @@ def test_method_settings_auto_populated_field_not_found_raises_error(): api_schema.enforce_valid_method_settings(methodsettings) +def test_method_settings_auto_populated_nested_field_raises_error(): + """ + Test that `MethodSettingsError` is raised when a field in + `client_pb2.MethodSettings.auto_populated_fields` is not found in the top-level + request message of the selector. Instead, the field exists in a nested message. + """ + + octopus = make_field( + name='octopus', + type_name='google.example.v1beta1.NestedMessage', + label=3, + type='TYPE_MESSAGE', + ) + + fd = get_file_descriptor_proto_for_method_settings_tests( + fields=[octopus.field_pb] + ) + api_schema = api.API.build(fd, "google.example.v1beta1") + methodsettings = [ + client_pb2.MethodSettings( + selector="google.example.v1beta1.ServiceOne.Example1", + auto_populated_fields=["squid"], + ), + ] + with pytest.raises(api.MethodSettingsError, match="(?i)not found"): + api_schema.enforce_valid_method_settings(methodsettings) + + def test_method_settings_auto_populated_field_client_streaming_rpc_raises_error(): """ Test that `MethodSettingsError` is raised when the selector in