fix: HttpJson tests generate request for Mixins with custom paths #2840

lqiu96 · 2024-05-31T20:09:45Z

Changes

When trying to generate default values for a message, the logic is updated for resource_references.

If the field has resource_reference.type, the generator will try to match resource's pattern with the method's HttpBindings. If there are no bindings, then the configured resource reference is returned. If there are bindings and the resource's pattern does not match, then the resource_reference is not used. If everything looks fine, return the matched resource.
If the field has resource_reference.child_type, the generator will search for the resource's parent type(s). It will run the above logic on all the parent resources.

If there is no resource matched, then the fallback will be to create a default value using createValue().

Fix for ServiceManagement Tests

Output of the generated tests:

com.google.cloud.api.servicemanagement.v1.ServiceManagerClientHttpJsonTest > getIamPolicyTest PASSED
com.google.cloud.api.servicemanagement.v1.ServiceManagerClientHttpJsonTest > getIamPolicyExceptionTest PASSED
com.google.cloud.api.servicemanagement.v1.ServiceManagerClientHttpJsonTest > testIamPermissionsExceptionTest PASSED
com.google.cloud.api.servicemanagement.v1.ServiceManagerClientHttpJsonTest > setIamPolicyExceptionTest PASSED
com.google.cloud.api.servicemanagement.v1.ServiceManagerClientHttpJsonTest > testIamPermissionsTest PASSED
com.google.cloud.api.servicemanagement.v1.ServiceManagerClientHttpJsonTest > setIamPolicyTest PASSED

This was done after re-generating the library locally and running ./gradlew clean build.

Fix for NetworkManagment Tests

Output of the generated tests:

com.google.cloud.networkmanagement.v1.ReachabilityServiceClientHttpJsonTest > getIamPolicyTest PASSED
com.google.cloud.networkmanagement.v1.ReachabilityServiceClientHttpJsonTest > getIamPolicyExceptionTest PASSED
com.google.cloud.networkmanagement.v1.ReachabilityServiceClientHttpJsonTest > testIamPermissionsExceptionTest PASSED
com.google.cloud.networkmanagement.v1.ReachabilityServiceClientHttpJsonTest > setIamPolicyExceptionTest PASSED
com.google.cloud.networkmanagement.v1.ReachabilityServiceClientHttpJsonTest > testIamPermissionsTest PASSED
com.google.cloud.networkmanagement.v1.ReachabilityServiceClientHttpJsonTest > setIamPolicyTest PASSED

This was done after re-generating the library locally and running ./gradlew clean build.

Golden Files Changes

IAM: The reason there are file changes is because the each request message has a wildcard resource reference (*). The generator will attempt to use any other known resource to populate, but the IAMPolicy protos have no known resources. Since there are no other resources it can use, the generator will not use a resource reference.
Logging: This seems to be a proto misconfiguration from the logging library.
The resource configuration for logging.googleapis.com/Settings:

message Settings {
  option (google.api.resource) = {
    type: "logging.googleapis.com/Settings"
    pattern: "projects/{project}/settings"
    pattern: "organizations/{organization}/settings"
    pattern: "folders/{folder}/settings"
    pattern: "billingAccounts/{billing_account}/settings"
  };

does not match the path in GetSettings:

  rpc GetSettings(GetSettingsRequest) returns (Settings) {
    option (google.api.http) = {
      get: "/v2/{name=*/*}/settings"
      additional_bindings { get: "/v2/{name=projects/*}/settings" }
      additional_bindings { get: "/v2/{name=organizations/*}/settings" }
      additional_bindings { get: "/v2/{name=folders/*}/settings" }
      additional_bindings { get: "/v2/{name=billingAccounts/*}/settings" }
    };

This results in the name being the value from createValue().

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

blakeli0 · 2024-06-07T20:57:07Z

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

+        }
+      }
+      // Unable to match the possible resources with the HttpBindings
+      return null;


What's the behavior if we couldn't find a matching resource with the HttpBindings? Is the generated tests going to pass?
I feel we should throw exception as this implies something is misconfigured. IIRC, this is one of the common reason that we receive YAQS questions.

It will create a default value with the createValue() method. The tests will pass since the default value will just be a value that matches the HttpBindings.

i.e. The tests will go from something like

setResource(ProjectName.of("[Project]"]))

to

setResource("projects/project-134")

where the resource value is just the string version of the resource.

I would like to throw an exception here, but I think this would end up breaking a bunch of existing services that have some misconfigured protos. I think we may need to ask the service teams to fix their protos before I start to throwing an exception here.

I think this would end up breaking a bunch of existing services that have some misconfigured protos.

I see, I don't think we should break the generation for those services but I think those generated tests should never worked?
In your example, assuming we have a HttpBinding misconfigured, and the test is generated to the following

setResource("projects/project-134/foo")

Is the test going to pass?

With this misconfiguration, the test would fail previously because we would generate setResource based on the resource pattern only, hence ProjectName.of("[Project]"]) would not match projects/project-134/foo on running the tests.

With this misconfiguration, the test would fail previously because we would generate setResource based on the resource pattern only

Currently, for this bindings not matching the resource pattern misconfiguration, that's true. It would fail the test and we would see it in the CI.

However, if we throw an exception on this case now, I think we may break for very niche cases. i.e. ServiceManagement comes to mind. It does not have any resources defined inside the library, but uses IAM mixins. The IAM mixin RPCs have resources defined in the httpbindings and the request messages have fields with resource_reference. It's not an RPC that the service team wrote, but is using. This would require them to add a resource for the service/* pattern (to be clear, I think it's probably something they should do, but currently don't).

blakeli0 · 2024-06-07T20:59:29Z

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

+    boolean isFieldUsedInPath =
+        bindings != null && bindings.getPathParametersValuePatterns().containsKey(field.name());
+    if (isFieldUsedInPath) {
+      for (ResourceName resourceName : matchedResources) {


nit: Rename resourceName to matchedResourceName so that we can change x to resourceName below. Maybe we can change the parameter resourceNames to allResourceNames as well.

Sounds good. I'm also thinking about changing this method to return all matched resource names

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

blakeli0 · 2024-06-07T21:02:22Z

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

+      for (ResourceName resourceName : matchedResources) {
+        // If the resource is matched as a wildcard `*`, the field's resource_reference is a
+        // wildcard. Attempt to match with a known resource.
+        if (resourceName.isOnlyWildcard()) {


What exactly does isOnlyWildcard mean?

I believe If the resource's pattern is *

Thanks! Do you have an example of it?

Asset resource is defined as wildcard pattern: https://github.com/googleapis/googleapis/blob/573205a81b9b96ea6d4e0365373839b59f4b427e/google/cloud/asset/v1/assets.proto#L96-L99

so it could be something with this kind of resource reference: https://github.com/googleapis/googleapis/blob/573205a81b9b96ea6d4e0365373839b59f4b427e/google/cloud/asset/v1/asset_service.proto#L451-L456

The resource_reference for a field can be wildcard: https://github.com/googleapis/googleapis/blob/573205a81b9b96ea6d4e0365373839b59f4b427e/google/iam/v1/iam_policy.proto#L104-L106

blakeli0 · 2024-06-07T21:11:54Z

...test/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposerTest.java

Can we create a golden test with a test service yaml that mimics the exact scenario(overrides mixin path)?

Sounds good. I will attempt to re-create this.

I added a resource_name_testing.proto to generate a bunch of common test scenarios. I can't find an easy way to incorporate IAM mixins as the logic for golden files parses an individual service and not multiple services (i.e. Echo + IAM) to combine the RPCs.

I modelled the ServiceManagement situation in the WildcardResourceReference and WildcardResourceReferenceStrictBindings RPCs

blakeli0 · 2024-06-07T21:21:53Z

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

+      List<ResourceName> parentResources =
+          findParentResources(matchedResources.get(0), new ArrayList<>(resourceNames.values()));
+      if (!parentResources.isEmpty()) {
+        matchedResources = parentResources;


The logic here is OK, but I feel we may want to differentiate the concept of matchedResources and derivedResources/flattenedResources. The matched resource is actually always one, but we could derive more resources from it if it is of child type. Hence putting the matched resource to a list and reusing/reassigning the same list is a little confusing.

I see. Let me rename this from matchedResources into something less confusing since I do think I'm using the term match to mean multiple things.

For fields with direct resource_references, matched resources is always one (assuming that the resource exists and is known). However, it is possible that the pattern doesn't match to the bindings so it really isn't "matched" yet.

For child_type fields, we must try to use the parent resources. However, we still need to match the parent resources with the bindings, so they're technically not matched yet either.

snippet-bot · 2024-06-11T14:43:51Z

Here is the summary of possible violations 😱

There are 3 possible violations for not having product prefix.

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/identityclient/SyncCreateUserStringString.golden:19, tag goldensample_generated_Identity_CreateUser_StringString_sync
gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/identityclient/SyncCreateUserStringStringIntStringBooleanDouble.golden:19, tag goldensample_generated_Identity_CreateUser_StringStringIntStringBooleanDouble_sync
gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/identityclient/SyncCreateUserStringStringStringStringIntStringStringStringString.golden:19, tag goldensample_generated_Identity_CreateUser_StringStringStringStringIntStringStringStringString_sync

The end of the violation section. All the stuff below is FYI purposes only.

Here is the summary of changes.

You are about to add 3 region tags.

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/identityclient/SyncCreateUserStringString.golden:19, tag goldensample_generated_Identity_CreateUser_StringString_sync
gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/identityclient/SyncCreateUserStringStringIntStringBooleanDouble.golden:19, tag goldensample_generated_Identity_CreateUser_StringStringIntStringBooleanDouble_sync
gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/identityclient/SyncCreateUserStringStringStringStringIntStringStringStringString.golden:19, tag goldensample_generated_Identity_CreateUser_StringStringStringStringIntStringStringStringString_sync

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

lqiu96 · 2024-06-11T19:12:03Z

Generated output for Java files via Hermetic Build: googleapis/google-cloud-java#10952

JoeWang1127 · 2024-06-13T15:41:22Z

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

@@ -249,11 +251,11 @@ private static Optional<ResourceName> findParentResource(
    for (String childPattern : childResource.patterns()) {
      Optional<String> parentPattern = ResourceReferenceUtils.parseParentPattern(childPattern);
      if (parentPattern.isPresent() && patternToResourceName.containsKey(parentPattern.get())) {
-        return Optional.of(patternToResourceName.get(parentPattern.get()));
+        parentResources.add(patternToResourceName.get(parentPattern.get()));


Since at most one parent resources is used, why not stop adding to the list once we find a resource?

It is possible that the first parent resource found does not match the bindings.

For example:
Resource has these patterns:

parentResources/{parentResource}/resources/{resource}

projects/{project}/locations/{location}

examples/{example}/testing/{test}

This would have these parent patterns:

parentResources/{parentResource}

projects/{project}

examples/{example}

If an RPC has these bindings:

parent={parentResources/*}

parent={projects/*}

It may be possible that the first matched parent resource is examples/*, which does not match the bindings.

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

…pic/composer/defaultvalue/DefaultValueComposer.java Co-authored-by: Joe Wang <106995533+JoeWang1127@users.noreply.github.com>

sonarcloud · 2024-06-13T16:25:15Z

Quality Gate passed for 'gapic-generator-java-root'

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarcloud · 2024-06-13T16:28:26Z

Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

lqiu96 requested a review from blakeli0 June 6, 2024 14:50

lqiu96 marked this pull request as ready for review June 6, 2024 14:50

lqiu96 requested a review from a team as a code owner June 6, 2024 14:50

blakeli0 reviewed Jun 7, 2024

View reviewed changes

This was referenced Jun 10, 2024

chore: Mixin Hermetic Build Diff googleapis/google-cloud-java#10951

Closed

chore: Add java files from Mixin Hermetic Build googleapis/google-cloud-java#10952

Draft

fix: Match resource pattern with bindings

9a1b1c7

lqiu96 force-pushed the fix-custom-mixin-paths-test branch from d6eae01 to 9a1b1c7 Compare June 10, 2024 21:11

chore: Resolve merge conflicts

56c8310

Merge branch 'main' into fix-custom-mixin-paths-test

e6fc158

lqiu96 added 3 commits June 12, 2024 18:13

chore: Add generator golden files

9cdd156

chore: Add gRPC golden files

59048ce

chore: Fix lint issues

37a95e5

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Jun 12, 2024

lqiu96 added 3 commits June 12, 2024 22:13

Merge branch 'main' into fix-custom-mixin-paths-test

90ed468

chore: Fix proto file

277ba15

chore: Update docs for the proto file

e00f970

JoeWang1127 reviewed Jun 13, 2024

View reviewed changes

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java Outdated Show resolved Hide resolved

JoeWang1127 reviewed Jun 13, 2024

View reviewed changes

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java Show resolved Hide resolved

JoeWang1127 reviewed Jun 13, 2024

View reviewed changes

...src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java Show resolved Hide resolved

lqiu96 and others added 2 commits June 13, 2024 11:59

Update gapic-generator-java/src/main/java/com/google/api/generator/ga…

90ced5c

…pic/composer/defaultvalue/DefaultValueComposer.java Co-authored-by: Joe Wang <106995533+JoeWang1127@users.noreply.github.com>

chore: Add @VisibleForTesting annotation to getMatchingResources

7f4353a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: HttpJson tests generate request for Mixins with custom paths #2840

fix: HttpJson tests generate request for Mixins with custom paths #2840

lqiu96 commented May 31, 2024 •

edited

Loading

blakeli0 Jun 7, 2024

lqiu96 Jun 11, 2024 •

edited

Loading

blakeli0 Jun 11, 2024

lqiu96 Jun 11, 2024

blakeli0 Jun 7, 2024

lqiu96 Jun 11, 2024

blakeli0 Jun 7, 2024

lqiu96 Jun 11, 2024

blakeli0 Jun 11, 2024

lqiu96 Jun 11, 2024

blakeli0 Jun 7, 2024

lqiu96 Jun 11, 2024

lqiu96 Jun 12, 2024

blakeli0 Jun 7, 2024

lqiu96 Jun 11, 2024

snippet-bot bot commented Jun 11, 2024 •

edited

Loading

lqiu96 commented Jun 11, 2024

JoeWang1127 Jun 13, 2024

lqiu96 Jun 13, 2024

sonarcloud bot commented Jun 13, 2024

sonarcloud bot commented Jun 13, 2024

fix: HttpJson tests generate request for Mixins with custom paths #2840

Are you sure you want to change the base?

fix: HttpJson tests generate request for Mixins with custom paths #2840

Conversation

lqiu96 commented May 31, 2024 • edited Loading

Changes

Fix for ServiceManagement Tests

Fix for NetworkManagment Tests

Golden Files Changes

Choose a reason for hiding this comment

lqiu96 Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snippet-bot bot commented Jun 11, 2024 • edited Loading

lqiu96 commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 13, 2024

Quality Gate passed for 'gapic-generator-java-root'

sonarcloud bot commented Jun 13, 2024

Quality Gate passed for 'java_showcase_integration_tests'

lqiu96 commented May 31, 2024 •

edited

Loading

lqiu96 Jun 11, 2024 •

edited

Loading

snippet-bot bot commented Jun 11, 2024 •

edited

Loading