feat: Add support for reading MethodSettings from service configuration YAML #1975

parthea · 2024-03-13T13:25:12Z

Adds support for reading Method settings in the service configuration YAML

https://github.com/googleapis/googleapis/blob/7dab3de7ec79098bb367b6b2ac3815512a49dd56/google/api/client.proto#L325

Towards b/322910372

…on YAML

gapic/schema/api.py

vchudnov-g

Some initial comments, all minor (nothing blocking).

I haven't yet looked at the test cases, Will do that next.

gapic/schema/api.py

vchudnov-g · 2024-03-14T16:00:43Z

gapic/schema/api.py

+        - 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.


Nit: this item doesn't read right under a list introduced as
'All conditions below must be true". I say take take this out of the list (outdent it)

Suggested change

- The field presence requirements in AIP-4235 are checked at run time.

Note that the field presence requirements in AIP-4235 are checked at run time.

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:01:01Z

gapic/schema/api.py

+        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 


Suggested change

(https://google.aip.dev/client-libraries/4235). All the configurations

(https://google.aip.dev/client-libraries/4235). All the conditions

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:04:18Z

gapic/schema/api.py

+
+        if all_errors:
+            raise ValueError(
+                f"Fields cannot be automatically populated in the top level "


This error header text should be more general. For example, maybe the only error condition is the not method_descriptor above, and nothing in the config even intended to have auto-populated fields.

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:07:46Z

gapic/schema/api.py

+                    invalid_selectors.append(method_settings.selector)
+
+        if all_errors:
+            raise ValueError(


Consider defining a sub-type of ValueError and raising that. Maybe MethodSettingsError or ConfigurationError?

Not strictly necessary; up to you. It would make it easier for us and users to test for the error condition without brittle examination of the error text.

And the custom type gives us the option to internally keep the various individual errors in a list, so in our tests we could check each element of the list contains key parts (eg method selector and "UUID4"). In fact, you could have invalid_selectors be a list of pairs, where the second element of the pair is the list of error strings for that selector. I.e. by doing invalid_selectors.append((method_settings.selector, errors_for_this_method)) the structure could be (in loose pseudo-yaml form)

invalid_selectors: - google.example.v1beta1.SomeExample.DoFoo" - "Field `auto_fill_me` is a required field" - "Field `auto_fill_me` is not a UUID4 field" - google.example.v1beta1.SomeExample.DoBar - "Field `request_id` is not of type string" - "Field `request_id` is a required field"

(To be clear, I do NOT think we should define custom error types for each of these conditions. That seems like too much pedantic overhead at this point. Nor should we use error chaining. But the single custom error type with some internal structure seems reasonable to me.)

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:14:25Z

tests/unit/schema/test_api.py

+    )
+    api_schema = api.API.build(fd, "google.example.v1beta1", opts=opts)
+
+    exception_message = """Fields cannot be automatically populated in the top level \


We shouldn't compare exact text of error messages. We don't want users to rely on that and we shouldn't either. I suggest checking the custom error type (if you go with my other suggestion) and then checking that the error message contains the relevant strings (eg selector strings defined in this test).

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:21:14Z

gapic/schema/api.py

+            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. "


Suggested change

all_errors += 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. Only unary methods support the `auto_populated_fields` setting. "

(Just so we can capitalize the first word of the second sentence ;-) )

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:25:13Z

gapic/schema/api.py

+        for method_settings in service_method_settings:
+            method_descriptor = self.all_methods.get(method_settings.selector)
+            if not method_descriptor:
+                all_errors += f"Selector {method_settings.selector} is not a valid method in this API. "


These += result in run-on text. At the very least, insert "\n".

I suggest having all_errors be a list you .append() to, and in the end the exception message can "\n".join(all_errors). This will work nicely with my suggestion for a custom exception and testing that exception.

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:44:21Z

tests/unit/schema/test_api.py

+    clam = make_field_pb2(
+        name="clam", type="TYPE_STRING", options=field_options, number=3
+    )
+    # Also include a field which is not of type string


This tests both non-string and a non-UUID4. I suggest separate test cases.

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:49:39Z

tests/unit/schema/test_api.py

+'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 octopus is testing several failing conditions at once. I think we we should test fields with one failing condition at a time. I think a better, accepted practice is to test simple cases and assume if those work then combinations ought to work too, rather than test a combination case and assume that if it works, its constituent parts will then work by themselves.

(The full coverage solution is to test all combinations, but I think that's overkill here.)

Fixed in 5a2d33b

vchudnov-g · 2024-03-14T16:57:05Z

gapic/schema/api.py

+            for method_key, method_value in service_value.methods.items()
+        }
+
+    def check_method_settings_validity(


nit nit suggestion: def enforce_valid_method_settings

Fixed in 5a2d33b

parthea · 2024-03-18T20:17:44Z

@vchudnov-g Please could you take another look?

vchudnov-g

Looks good!

I have one suggestion about using YAML rather than JSON in error text, and some more minor, non-blocking suggestions about possible enhancements.

gapic/schema/api.py

vchudnov-g · 2024-03-18T19:53:12Z

gapic/schema/api.py

        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"]


So this will overwrite any errors found the first time through the selector. It does seem like this is the more important error, but we might as well present all the errors found at once, which would mean appending (or maybe in this case prepending) to the previous list value in the map.

If something wrong with the selector (it is a duplicate, doesn't map to a method or maps to a streaming method), I decided not to continue checking the validity of the auto-populated fields. The issue with the selector should be fixed and providing additional detail in the error message on the auto-populated fields would take away from the more important error with the selector.

vchudnov-g · 2024-03-18T19:56:30Z

gapic/schema/api.py

-                f"request message for selectors: {invalid_selectors}. "
-                f"{all_errors.strip()}"
-            )
+            raise MethodSettingsError(json.dumps(all_errors))


Why JSON? IMO, YAML would be more concise and hence better for someone getting the exception on their console.

Fixed in 4161702 and e1c8785

vchudnov-g · 2024-03-18T20:12:02Z

tests/unit/schema/test_api.py

    assert api_schema.has_iam_mixin
+
+
+def get_file_descriptor_proto_for_method_settings_tests(


Thanks for factoring this out. It's much easier to follow!

Minor, non-blocking: I thought I left the following comment when I reviewed last week, but I don't see it on GitHub so it must not have gotten saved. I'll put it here just for your consideration. It's less pressing now that you've done this refactoring, but it would make it nicer when defining the various fields, etc.

If it's trivial to read from a proto "file" (string) in tests, it might be simpler to simply encode the protofile inline rather than calling make_file_pb2 and its ilk... something like

proto_descriptor = make_protodescriptor(""" package xx; message Foo { string foo = 1; } """)

(This is similar in spirit to what you do with the YAML file below!)

This would make it easier to keep the proto concepts being tested next to the code without a lot of boilerplate. If it's not trivial to do now, let's defer it. (But I personally would love to eventually have most of our tests use this declarative style rather than the harder-to-follow procedural set-up of they test cases.)

I created #1986 as a follow up issue to explore whether this is feasible

vchudnov-g · 2024-03-18T20:19:11Z

tests/unit/schema/test_api.py

+            ),
+            services=(
+                descriptor_pb2.ServiceDescriptorProto(
+                    name="SomeExample",


nit, non-blocking: consider more descriptive names, like ServiceOne and ServiceTwo for theservices, and DoFoo for the methods. Just to make it easier for humans to track which names are used where. (ExampleRequest and ExampleResponse do that, which is great.)

Fixed in 04fa73e

vchudnov-g · 2024-03-18T20:24:57Z

tests/unit/schema/test_api.py

+    api_schema = api.API.build(fd, "google.example.v1beta1")
+    methodsettings = [
+        client_pb2.MethodSettings(
+            selector="google.example.v1beta1.DoesNotExist.Example1",


nit: Do you want to test a non-existent method in a valid service as well?

Fixed in eac51b6

vchudnov-g · 2024-03-18T20:27:31Z

tests/unit/schema/test_api.py

+    `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()


Consider populating some other field of the message.

Also, is it simple to test when the field in auto_populated_fields is nested one level in ? (I.e., it still exists but not at the top-level, so we still get the exception)

Done in 1ab06ea

Co-authored-by: Victor Chudnovsky <[email protected]>

…aises_error

product-auto-label bot added the size: m Pull request size is medium. label Mar 13, 2024

parthea force-pushed the add-support-for-method-settings branch 2 times, most recently from c6124db to 77eaeb2 Compare March 13, 2024 14:41

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Mar 13, 2024

parthea force-pushed the add-support-for-method-settings branch from 08eb8e6 to 160f578 Compare March 13, 2024 15:26

parthea marked this pull request as ready for review March 13, 2024 15:26

parthea requested a review from a team as a code owner March 13, 2024 15:26

feat: Add support for reading MethodSettings from service configurati…

ecf0f97

…on YAML

parthea force-pushed the add-support-for-method-settings branch from 160f578 to ecf0f97 Compare March 13, 2024 15:59

style

16f3784

parthea assigned vchudnov-g, dizcology and ohmayr Mar 13, 2024

ohmayr reviewed Mar 13, 2024

View reviewed changes

gapic/schema/api.py Outdated Show resolved Hide resolved

gapic/schema/api.py Outdated Show resolved Hide resolved

gapic/schema/api.py Outdated Show resolved Hide resolved

parthea added 3 commits March 13, 2024 18:58

refactor based on review feedback

41c582c

refactor

9f5c5d5

docs

884fda2

vchudnov-g reviewed Mar 13, 2024

View reviewed changes

parthea added 11 commits March 14, 2024 10:03

message->method_descriptor; update comments

12f4333

update docstring of _check_service_yaml_validity

b12753d

typo

8d64606

update docstring of _check_service_yaml_validity

b760a62

grammar

e7f8a02

refactor for readability

c3563c1

refactor for readability

8a9bbcb

update comment

dc7ac89

mypy

42cb7e8

formatting

1d03fb2

update comment

2499b59

parthea requested review from ohmayr and vchudnov-g March 14, 2024 13:18

parthea added 2 commits March 14, 2024 13:31

fix test case

57493b0

update test name

9f01b28

vchudnov-g reviewed Mar 14, 2024

View reviewed changes

ohmayr approved these changes Mar 14, 2024

View reviewed changes

parthea assigned parthea and unassigned vchudnov-g, dizcology and ohmayr Mar 15, 2024

parthea added 2 commits March 17, 2024 10:16

address review feedback

5a2d33b

style

d5d0aee

parthea assigned vchudnov-g and unassigned parthea Mar 18, 2024

vchudnov-g approved these changes Mar 18, 2024

View reviewed changes

parthea and others added 3 commits March 18, 2024 16:34

fix typo

73b06ca

Co-authored-by: Victor Chudnovsky <[email protected]>

json->yaml

4161702

json->yaml

e1c8785

parthea mentioned this pull request Mar 19, 2024

In tests, explore creating descriptors from strings rather than helper functions to improve readability #1986

Open

parthea added 5 commits March 19, 2024 01:24

SomeService->ServiceOne;AnotherService->ServiceTwo

04fa73e

add test case for non-existent method

eac51b6

rename field in test_method_settings_auto_populated_field_not_found_r…

4d7b4ca

…aises_error

add test case for nested field

1ab06ea

Merge branch 'main' into add-support-for-method-settings

c05936a

parthea enabled auto-merge (squash) March 19, 2024 02:06

parthea merged commit 24a23a1 into main Mar 19, 2024

parthea deleted the add-support-for-method-settings branch March 19, 2024 02:10

release-please bot mentioned this pull request Mar 19, 2024

chore(main): release 1.16.0 #1980

Merged

parthea restored the add-support-for-method-settings branch March 19, 2024 02:13

	- The field presence requirements in AIP-4235 are checked at run time.
	Note that the field presence requirements in AIP-4235 are checked at run time.

	(https://google.aip.dev/client-libraries/4235). All the configurations
	(https://google.aip.dev/client-libraries/4235). All the conditions

	all_errors += 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. Only unary methods support the `auto_populated_fields` setting. "

		assert api_schema.has_iam_mixin


		def get_file_descriptor_proto_for_method_settings_tests(

feat: Add support for reading MethodSettings from service configuration YAML #1975

feat: Add support for reading MethodSettings from service configuration YAML #1975

Uh oh!

Conversation

parthea commented Mar 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthea Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchudnov-g Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthea commented Mar 18, 2024

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthea Mar 18, 2024 •

edited

Loading

vchudnov-g Mar 14, 2024 •

edited

Loading

vchudnov-g Mar 18, 2024 •

edited

Loading