fix: [gapic-generator-java] align writer behavior for nested types #1709

emmileaf · 2023-05-24T19:21:36Z

This PR:

Removes duplicate handling of enclosing classes in JavaWriterVisitor and ImportWriterVisitor
- Change to only import outermost class, e.g. com.example.Outer), while type references will be written as Outer.Middle.Inner
Fixes issue in ConcreteReference where multiple levels of enclosed classes are written in reversed order

Fixes #1708 ☕️

…asses * To illustrate current unintended behavior

meltsufin

No impact on the goldens?

burkedavison · 2023-06-01T16:23:57Z

@meltsufin : No impact on the goldens?

If so, perhaps we can create a (very) simple extended showcase proto to exercise this behavior -- similar to how we did for validating REST-only client generation behavior?
https://github.com/googleapis/sdk-platform-java/tree/main/showcase/gapic-showcase-extended/proto

emmileaf · 2023-06-01T17:51:08Z

No impact on the goldens?

@meltsufin Not for this change. For a bit more context - this nested class import issue was initially uncovered when the generator mistakenly tried to pull in DescriptorProtos.GeneratedCodeInfo.Annotation, and this behavior is being fixed in #1726 (workaround in the specific proto was to fully specify package name).

If so, perhaps we can create a (very) simple extended showcase proto to exercise this behavior

@burkedavison I was considering this as well - actually tried doing this for validating the change in #1726, but this fix was easier to unit test so I just went with those instead. I can set up something similarly here if the approach taken in the other PR makes sense?

blakeli0 · 2023-06-02T03:44:06Z

No impact on the goldens?

@meltsufin Not for this change. For a bit more context - this nested class import issue was initially uncovered when the generator mistakenly tried to pull in DescriptorProtos.GeneratedCodeInfo.Annotation, and this behavior is being fixed in #1726 (workaround in the specific proto was to fully specify package name).

If so, perhaps we can create a (very) simple extended showcase proto to exercise this behavior

@burkedavison I was considering this as well - actually tried doing this for validating the change in #1726, but this fix was easier to unit test so I just went with those instead. I can set up something similarly here if the approach taken in the other PR makes sense?

I agree with Mike and Burke that it would be better to have a golden test to easily see the changes. Showcase may be a little overkill since this does not interact with the server, a golden unit test would be a great fit.

blakeli0 · 2023-06-02T03:37:37Z

gapic-generator-java/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java

    ImmutableList.Builder<String> listBuilder = new ImmutableList.Builder<>();
    Class<?> currentClz = clazz();
    while (currentClz.getEnclosingClass() != null) {
      listBuilder.add(currentClz.getEnclosingClass().getSimpleName());
      currentClz = currentClz.getEnclosingClass();
    }
-    return listBuilder.build();
+    return listBuilder.build().reverse();


Can we achieve the same thing without this change? Seems we can try to get the last element from the list in the writer?

Yes for the import writer, but the JavaWriterVisitor also relies on this ordering for the enclosing class writes.

This fix to ConcreteReference wasn't a part of the original issue investigated, but it looked like an implementation bug that perhaps didn't surface in generated code anywhere yet. I think the intended result of this enclosingClassNames call should have outer to inner ordering, given this original comment and upstream usages of this method in the generator.

blakeli0 · 2023-06-02T03:39:50Z

...rator-java/src/test/java/com/google/api/generator/engine/writer/ImportWriterVisitorTest.java

+
+    writerVisitor.clear();
+
+    ConcreteReference nestedConcreteReference =


This should be break into two test cases if we want to test both type of references. However, I think we only need to test one of them if the only change is in the writer.

I think the additional change in ConcreteReference is still needed given #1709 (comment). Breaking this into separate test cases makes sense though - will update.

emmileaf · 2023-06-02T14:48:16Z

I agree with Mike and Burke that it would be better to have a golden test to easily see the changes. Showcase may be a little overkill since this does not interact with the server, a golden unit test would be a great fit.

@blakeli0 Makes sense. Looks like there's actually a nested_message.proto that exists and is currently used in TypeParserTest, just without goldens. I'll try leveraging that to add some golden unit tests here.

emmileaf · 2023-06-02T18:47:05Z

Added a test proto and new golden unit test in 6ab1628.

(Had some uncertainty deciding where best to put this golden test since the existing ones are mostly separate for each Composer, and all of these exercise the writer behavior implicitly. However, this fix is more specific to writer logic and could be applicable to a number of Composers and the class files they generate.

I ended up adding one of these into JavaWriterVisitorTest that uses the grpc ServiceClientClassComposer to write an example file with nested types and imports - please let me know if this approach makes sense!)

sonarcloud · 2023-06-02T18:54:41Z

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

sonarcloud · 2023-06-02T18:58:19Z

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-06-02T19:03:08Z

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

blakeli0 · 2023-06-02T19:43:30Z

Added a test proto and new golden unit test in 6ab1628.

(Had some uncertainty deciding where best to put this golden test since the existing ones are mostly separate for each Composer, and all of these exercise the writer behavior implicitly. However, this fix is more specific to writer logic and could be applicable to a number of Composers and the class files they generate.

I ended up adding one of these into JavaWriterVisitorTest that uses the grpc ServiceClientClassComposer to write an example file with nested types and imports - please let me know if this approach makes sense!)

Thanks Emily! I think your approach is fine, it's very specific to what we want to test. As you mentioned, our golden tests are currently assuming writers behave perfectly so it is a little weird to add a golden test just for writers. If you prefer, we can add this test RPC to one of the existing protos, so that it's tested through other golden tests and don't have to load it separately.

blakeli0

LGTM. We could move the golden tests to composers or add the test case to one of the existing protos, but I don't think it should block this PR.

blakeli0

LGTM. We could move the golden tests to composers or add the test case to one of the existing protos, but I don't think it should block this PR.

emmileaf · 2023-06-05T17:54:23Z

Sounds good - I'll merge this in since the fix is also independent of #1726, so that it can make it into the upcoming release!

test: add test cases to capture type and import writing for nested cl…

f505036

…asses * To illustrate current unintended behavior

product-auto-label bot added the size: m Pull request size is medium. label May 24, 2023

emmileaf added 5 commits May 24, 2023 16:45

fix: fix order of enclosingClassNames for ConcreteReference

101162a

fix: only import outermost class in case of nesting

94e8fc6

Merge branch 'main' into writer-nested-types

3024342

Merge branch 'main' into writer-nested-types

e4764b2

chore: clean up comments

ca73ada

emmileaf marked this pull request as ready for review June 1, 2023 13:48

emmileaf requested a review from a team as a code owner June 1, 2023 13:48

emmileaf requested a review from blakeli0 June 1, 2023 13:55

meltsufin reviewed Jun 1, 2023

View reviewed changes

meltsufin approved these changes Jun 1, 2023

View reviewed changes

blakeli0 reviewed Jun 2, 2023

View reviewed changes

emmileaf added 2 commits June 2, 2023 12:59

test: split unit tests added for writers

158e6a9

test: add test proto to use nested type with golden test for writer

6ab1628

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jun 2, 2023

emmileaf requested a review from blakeli0 June 2, 2023 18:47

blakeli0 approved these changes Jun 2, 2023

View reviewed changes

emmileaf merged commit a21ffe8 into main Jun 5, 2023
18 checks passed

emmileaf deleted the writer-nested-types branch June 5, 2023 17:55

release-please bot mentioned this pull request Jun 5, 2023

chore(main): release 2.21.0 #1722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [gapic-generator-java] align writer behavior for nested types #1709

fix: [gapic-generator-java] align writer behavior for nested types #1709

emmileaf commented May 24, 2023 •

edited

meltsufin left a comment

burkedavison commented Jun 1, 2023

emmileaf commented Jun 1, 2023

blakeli0 commented Jun 2, 2023

blakeli0 Jun 2, 2023

emmileaf Jun 2, 2023

blakeli0 Jun 2, 2023

emmileaf Jun 2, 2023

emmileaf commented Jun 2, 2023

emmileaf commented Jun 2, 2023

sonarcloud bot commented Jun 2, 2023

sonarcloud bot commented Jun 2, 2023

sonarcloud bot commented Jun 2, 2023

blakeli0 commented Jun 2, 2023

blakeli0 left a comment

blakeli0 left a comment

emmileaf commented Jun 5, 2023 •

edited


		writerVisitor.clear();

		ConcreteReference nestedConcreteReference =

fix: [gapic-generator-java] align writer behavior for nested types #1709

fix: [gapic-generator-java] align writer behavior for nested types #1709

Conversation

emmileaf commented May 24, 2023 • edited

meltsufin left a comment

Choose a reason for hiding this comment

burkedavison commented Jun 1, 2023

emmileaf commented Jun 1, 2023

blakeli0 commented Jun 2, 2023

blakeli0 Jun 2, 2023

Choose a reason for hiding this comment

emmileaf Jun 2, 2023

Choose a reason for hiding this comment

blakeli0 Jun 2, 2023

Choose a reason for hiding this comment

emmileaf Jun 2, 2023

Choose a reason for hiding this comment

emmileaf commented Jun 2, 2023

emmileaf commented Jun 2, 2023

sonarcloud bot commented Jun 2, 2023

sonarcloud bot commented Jun 2, 2023

sonarcloud bot commented Jun 2, 2023

blakeli0 commented Jun 2, 2023

blakeli0 left a comment

Choose a reason for hiding this comment

blakeli0 left a comment

Choose a reason for hiding this comment

emmileaf commented Jun 5, 2023 • edited

emmileaf commented May 24, 2023 •

edited

emmileaf commented Jun 5, 2023 •

edited