fix: Use parent type instead of child_type in method doc sample #862

meltsufin · 2021-10-20T21:47:44Z

Fixes: #852.

codecov · 2021-10-20T22:01:57Z

Codecov Report

Merging #862 (8c8ab88) into main (64d8374) will increase coverage by 0.05%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##             main     #862      +/-   ##
==========================================
+ Coverage   87.59%   87.65%   +0.05%     
==========================================
  Files         152      153       +1     
  Lines       15982    16000      +18     
  Branches     1162     1168       +6     
==========================================
+ Hits        14000    14025      +25     
+ Misses       1641     1634       -7     
  Partials      341      341

Impacted Files	Coverage Δ
...google/api/generator/gapic/protoparser/Parser.java	`45.11% <50.00%> (-0.08%)`	⬇️
...ic/composer/defaultvalue/DefaultValueComposer.java	`82.94% <94.44%> (+0.53%)`	⬆️
...er/samplecode/ServiceClientSampleCodeComposer.java	`99.28% <100.00%> (+<0.01%)`	⬆️
...tor/gapic/protoparser/ResourceReferenceParser.java	`80.26% <100.00%> (+7.22%)`	⬆️
.../generator/gapic/utils/ResourceReferenceUtils.java	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64d8374...8c8ab88. Read the comment docs.

vam-google

LGTM in substance, but please address the two concerns first:

Make it clear that the fix is not complete in a sense that the samples for non-flattened methods are still broken, as well as any potential flattened methods with more than one resource name input )
Please strongly consider modifying the testing approach.

src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/DatastreamClient.golden

src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel

src/main/java/com/google/api/generator/gapic/protoparser/MethodSignatureParser.java

Fixes: #852.

meltsufin · 2021-10-26T02:46:17Z

Make it clear that the fix is not complete in a sense that the samples for non-flattened methods are still broken, as well as any potential flattened methods with more than one resource name input )

@vam-google Can you point to some examples please?

Please strongly consider modifying the testing approach.

The datastream was never intended to stay. It was there just to work on the PR. I did follow your suggestion to use messaging.proto. It has pretty nice coverage for this issue.

I decided to make the condition a bit more flexible to allow the case of multiple parents, in which case we just choose the first one.

Please take another look. Thanks!

chanseokoh · 2021-10-26T14:48:53Z

in which case we just choose the first one

I may be talking something different, but isn't it that we need to choose the best match (the longest match) when there are multiple parent candidates?

meltsufin · 2021-10-26T16:12:28Z

in which case we just choose the first one

I may be talking something different, but isn't it that we need to choose the best match (the longest match) when there are multiple parent candidates?

I don't think that's specified. Have you seen that somewhere?

test/integration/goldens/logging/com/google/cloud/logging/v2/ConfigClient.java

…type

vam-google

LGTM to unblock things. There are two non-blocking concerns though (check the corresponding comments for details):

Extra util class, with logic which hopefully can be kept in the parser class
The parseParentPattern method logic is probably no accurate (but it was like that before this PR).

vam-google · 2021-11-01T17:20:18Z

src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

@@ -881,7 +881,11 @@ static String parsePageSizeFieldName(
    // with monolith-gnerated libraries.
    String pagedFieldName = null;

-    if (inputMessage.fieldMap().containsKey("page_token")
+    if (inputMessage != null
+        && inputMessage.fieldMap() != null


Why did this if statement get so much bigger? Specifically, looks like inputMessage.fieldMap() was guaranteed to not be null (as we would get NPE otherwise).

Good point on fieldMap() being non-nullible. However, inputMessage can be null as the new messaging test revealed.

vam-google · 2021-11-01T17:21:41Z

src/main/java/com/google/api/generator/gapic/utils/ResourceReferenceUtils.java

@@ -0,0 +1,49 @@
+// Copyright 2020 Google LLC


vam-google · 2021-11-01T17:33:17Z

src/main/java/com/google/api/generator/gapic/utils/ResourceReferenceUtils.java

+import java.util.Arrays;
+import java.util.Optional;
+
+public final class ResourceReferenceUtils {


tl;dr; can we keep this in parser class?

It seems we can avoid creating a new util class for this:
It is one-method util class.
The only method is called parse* and we already have ResourceReferenceParser class, so it is hard to justify presence of ResourceReferenceUtils which does the parser's job.
I guess it is so because this new parse method is now used in two places - ResourceReferenceParser and DefaultValueComposer.

WDYT about keeping the parser method in parser class, but make the parent-child resolution logic persisted in ResourceName class (or another/new data structure returned by the parser class) and make the parser class precompute everything only once. If parent-child resolutin is not a property of a resource name but a property of an rpc method using that resource name, then it is not clear why that logic is still called in resource name parser class.

vam-google · 2021-11-01T17:37:38Z

src/test/java/com/google/api/generator/gapic/composer/common/TestProtoLoader.java

+    Map<String, ResourceName> identityResourceNames = Parser
+        .parseResourceNames(identityFileDescriptor);
+
+    resourceNames.put("showcase.googleapis.com/User",


Why is this needed? If there is stuff in the messaging.proto file, which just causes troubles but is not essential for the tests, please feel free to remove it. If there are missing imports please just add them on proot/bazel lavel.
Basically any API definition problems are expected to be solved on proto level, but not in the logic which loads/parse the protos.

vam-google · 2021-11-01T17:39:42Z

src/test/java/com/google/api/generator/gapic/testdata/compliance.proto

@@ -16,7 +16,6 @@ syntax = "proto3";

 import "google/api/annotations.proto";
 import "google/api/client.proto";
-import "google/cloud/extended_operations.proto";


Please put it back. It will be needed soon, even though it is technically not used now.

vam-google · 2021-11-01T17:42:22Z

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

      List<ResourceName> resnames,
      String fieldOrMessageName,
      boolean allowAnonResourceNameClass) {

+    if (isChildType) {
+      resourceName = findParentResource(resourceName, resnames).orElse(resourceName);


does the orElse part mean that if we did not manage to find a parent, the code will behave as before, basically meaning that the child is its own parent?

Yep. Do you think we should fail instead?

No, just wanted to make sure I understood it right.

vam-google · 2021-11-01T17:49:32Z

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

+    }
+
+    for (String childPattern : childResource.patterns()) {
+      Optional<String> parentPattern = ResourceReferenceUtils.parseParentPattern(childPattern);


*Please see the other comment in ResourceReferenceUtils class for context

I could be missing some more complicated interactions, but at first glance:
ResourceReferenceUtils.parseParentPattern(childPattern) is called for every pattern in childResource which is of type ResourceName, created by ResourceReferenceParser. So instead of computing the parentPattern portion here, it may be precomputed and included in ResourceName class itself, which is constructed in ResourceReferenceParser, thus eliminating the need to have the utils class.

As far as I can tell, ResourceReferenceUtils.parseParentPattern(childPattern) is only called by MethodSigantureParser and by Parser, but only for Google Ads API. So, I don't see a straightforward way to include the mapping to the parent in the ResourceName itself, especially since the class is immutable, and you need to have access to all ResourceNames before computing the mapping from child to parent.

vam-google · 2021-11-01T17:52:31Z

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

+        resourceName, isChildType, resnames, fieldOrMessageName, true);
+  }
+
+  private static Optional<ResourceName> findParentResource(


Can this whole thing be included in ResourceReferenceParser class? This seems like a general resource names resolution logic, not necessarily specific to sample generation.

vam-google · 2021-11-01T18:50:30Z

src/main/java/com/google/api/generator/gapic/utils/ResourceReferenceUtils.java

+
+  private ResourceReferenceUtils() {}
+
+  public static Optional<String> parseParentPattern(String pattern) {


This logic was there before, but it does not look accurate, as it makes many different assumption, which may be false.

for examle, if there is a pattern /a/{b}/c/e/{f}
when we search for its parent we must check at least 3 possible cases:

/a/{b}/c/e/{f} - child /a/{b}/c/e - parent candidate 1 /a/{b}/c - parent candidate 2 (if 1 did not match anything) /a/{b} - parent candidate 3 (if neither 1 nor 2 matched anything)

Instead this logic always returns only one candidate. Not sure how many (if any) practical cases we have which the current implementation would not satisfy. I guess it is ok to keep the implementaiton as is for now, but maybe put a comment/request for refactoring it.

It's true, but according to https://google.aip.dev/122, this example is not a recommended pattern.

Resource name components should usually alternate between collection identifiers (example: publishers, books, users) and resource IDs (example: 123, les-miserables, vhugo1802).

Correct, but if it was guaranteed they would phrase it as "must" in the AIP. I think it is ok to keep the implementation as is, since it is beyond the scope of this PR, just maybe add a comment about it.

….5.0 (#862) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.api.grpc:proto-google-iam-v1](https://togithub.com/googleapis/java-iam) | `1.4.1` -> `1.5.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.api.grpc:proto-google-iam-v1/1.5.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.api.grpc:proto-google-iam-v1/1.5.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.api.grpc:proto-google-iam-v1/1.5.0/compatibility-slim/1.4.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.api.grpc:proto-google-iam-v1/1.5.0/confidence-slim/1.4.1)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>googleapis/java-iam</summary> ### [`v1.5.0`](https://togithub.com/googleapis/java-iam/blob/HEAD/CHANGELOG.md#150-httpsgithubcomgoogleapisjava-iamcomparev141v150-2022-06-30) [Compare Source](https://togithub.com/googleapis/java-iam/compare/v1.4.1...v1.5.0) ##### Features - add v2beta client ([#364](https://togithub.com/googleapis/java-iam/issues/364)) ([0904baa](https://togithub.com/googleapis/java-iam/commit/0904baa7be7dda2b0c8ec9e68ade548d173761fd)) ##### Dependencies - update dependency com.google.cloud:google-cloud-shared-dependencies to v2.13.0 ([#367](https://togithub.com/googleapis/java-iam/issues/367)) ([ece5f7d](https://togithub.com/googleapis/java-iam/commit/ece5f7dda5fcba488f1416593114ba6ab8ab31ef)) - update dependency com.google.protobuf:protobuf-java to v3.21.2 ([#368](https://togithub.com/googleapis/java-iam/issues/368)) ([64b6349](https://togithub.com/googleapis/java-iam/commit/64b6349880792f3a534aa879ca1f0044d25603be)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core).

🤖 I have created a release *beep* *boop* --- ## [2.8.2](googleapis/java-core@v2.8.1...v2.8.2) (2022-07-13) ### Bug Fixes * enable longpaths support for windows test ([#1485](https://github.com/googleapis/java-core/issues/1485)) ([#866](googleapis/java-core#866)) ([8a8ac99](googleapis/java-core@8a8ac99)) ### Dependencies * update dependency com.google.api-client:google-api-client-bom to v1.35.2 ([#859](googleapis/java-core#859)) ([6b51a1c](googleapis/java-core@6b51a1c)) * update dependency com.google.api:gax-bom to v2.18.3 ([#860](googleapis/java-core#860)) ([f5a5278](googleapis/java-core@f5a5278)) * update dependency com.google.api.grpc:proto-google-common-protos to v2.9.1 ([#855](googleapis/java-core#855)) ([4ec6635](googleapis/java-core@4ec6635)) * update dependency com.google.api.grpc:proto-google-iam-v1 to v1.5.0 ([#862](googleapis/java-core#862)) ([19aebbe](googleapis/java-core@19aebbe)) * update dependency com.google.http-client:google-http-client-bom to v1.42.1 ([#861](googleapis/java-core#861)) ([4d7548b](googleapis/java-core@4d7548b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

product-auto-label bot added the samples Issues that are directly related to samples. label Oct 20, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 20, 2021

meltsufin requested a review from vam-google October 20, 2021 21:47

meltsufin force-pushed the fix_method_doc_child_type branch from 556da01 to 3389f74 Compare October 20, 2021 21:49

meltsufin changed the title ~~fix: User parent type instead of child_type in method doc sample~~ fix: Use parent type instead of child_type in method doc sample Oct 20, 2021

meltsufin force-pushed the fix_method_doc_child_type branch from 3389f74 to ababe29 Compare October 20, 2021 21:51

vam-google reviewed Oct 25, 2021

View reviewed changes

fix: Use parent type instead of child_type in method doc sample

49f0b39

Fixes: #852.

meltsufin force-pushed the fix_method_doc_child_type branch from 1e49e88 to 00028b3 Compare October 26, 2021 02:28

Add messaging.proto instread of datastream

8c59769

meltsufin force-pushed the fix_method_doc_child_type branch from 00028b3 to 8c59769 Compare October 26, 2021 02:40

meltsufin marked this pull request as ready for review October 26, 2021 02:41

meltsufin requested review from miraleung and a team as code owners October 26, 2021 02:41

meltsufin requested a review from vam-google October 26, 2021 02:41

Redo the fix to cover both flattened and non-flattened methods

7578d45

meltsufin force-pushed the fix_method_doc_child_type branch from 96318ad to 7578d45 Compare October 28, 2021 04:02

meltsufin commented Oct 28, 2021

View reviewed changes

test/integration/goldens/logging/com/google/cloud/logging/v2/ConfigClient.java Outdated Show resolved Hide resolved

meltsufin added 4 commits October 28, 2021 17:30

Re-use parseParentPattern logic

c9f0b07

Include ResourceReferenceUtilsTest in build

9350266

Merge remote-tracking branch 'origin/main' into fix_method_doc_child_…

a4543a7

…type

Make Codecov happy

bde778e

meltsufin requested a review from a team as a code owner October 29, 2021 18:47

vam-google approved these changes Nov 1, 2021

View reviewed changes

PR feedback

8c8ab88

meltsufin merged commit 6a39c7f into main Nov 3, 2021

meltsufin deleted the fix_method_doc_child_type branch November 3, 2021 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use parent type instead of child_type in method doc sample #862

fix: Use parent type instead of child_type in method doc sample #862

meltsufin commented Oct 20, 2021

codecov bot commented Oct 20, 2021 •

edited

vam-google left a comment

meltsufin commented Oct 26, 2021

chanseokoh commented Oct 26, 2021

meltsufin commented Oct 26, 2021

vam-google left a comment

vam-google Nov 1, 2021 •

edited

meltsufin Nov 2, 2021

vam-google Nov 1, 2021

vam-google Nov 1, 2021

vam-google Nov 1, 2021

vam-google Nov 1, 2021

vam-google Nov 1, 2021

meltsufin Nov 2, 2021

vam-google Nov 3, 2021

vam-google Nov 1, 2021 •

edited

meltsufin Nov 2, 2021

vam-google Nov 1, 2021

vam-google Nov 1, 2021

meltsufin Nov 2, 2021

vam-google Nov 3, 2021


		private ResourceReferenceUtils() {}

		public static Optional<String> parseParentPattern(String pattern) {

fix: Use parent type instead of child_type in method doc sample #862

fix: Use parent type instead of child_type in method doc sample #862

Conversation

meltsufin commented Oct 20, 2021

codecov bot commented Oct 20, 2021 • edited

Codecov Report

vam-google left a comment

Choose a reason for hiding this comment

meltsufin commented Oct 26, 2021

chanseokoh commented Oct 26, 2021

meltsufin commented Oct 26, 2021

vam-google left a comment

Choose a reason for hiding this comment

vam-google Nov 1, 2021 • edited

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

vam-google Nov 1, 2021 • edited

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

codecov bot commented Oct 20, 2021 •

edited

vam-google Nov 1, 2021 •

edited

vam-google Nov 1, 2021 •

edited