feat: add support for rest-only services in generator #1849

emmileaf · 2023-05-15T14:57:20Z

Generator changes:

Removed exclusion for rest-only libraries in generate-library-list.sh
Removed exclusion for services with rest-only transport in SpringComposer
Added golden tests
In java-compute, newBuilder(), defaultTransportChannelProvider(), and defaultApiClientHeaderProviderBuilder() all default to REST implementation, so spring composer logic is kept the same as handling GRPC-only libraries: do not generate configurable useRest property, and use the default builders above in generated auto-configuration.

For reviewers:

Looking for a review on generator changes to include a starter module for compute (REST-only, GAPIC library): spring-cloud-previews/google-cloud-compute-spring-starter
Corresponding changes to generated code are now included this PR at 34495a0. (Previously in 213add0, #1850)
- Added module google-cloud-compute-spring-starter
- Across all generated modules: updated javadoc comment for TransportChannelProvider bean to reflect the current default behavior
- Updated copyright year in license headers for auto-generated files (from upstream template change in gapic-generator-java

Memo on outstanding blocking changes:

Update to next release of gapic-generator-java containing license header year fix, and re-run changes to generated code (thread)

Memo on non-blocking discussions / open follow-ups:

Alternative strategies for disabling/enabling autoconfiguration classes, and how these affect application start-up time (discussion)
Consider renaming and perhaps expanding configuration scope of the <service.prefix>.useRest property (discussion)
Workflow to push generated code changes introduced by generator changes as part of CI (discussion)

BEGIN_COMMIT_OVERRIDE

feat: add google-cloud-compute-spring-starter to starter modules available for preview

END_COMMIT_OVERRIDE

blakeli0

In java-compute, newBuilder(), defaultTransportChannelProvider(), and defaultApiClientHeaderProviderBuilder() all default to REST implementation

This is good to know but also surprising, because it means if we change a REST only client to GRPC+REST, it would be a breaking change.

spring-cloud-generator/java_gapic_spring.bzl

.../src/main/java/com/google/cloud/generator/spring/composer/SpringAutoConfigClassComposer.java

spring-cloud-generator/src/test/proto/compliance.proto

...oud-generator/src/test/java/com/google/cloud/generator/spring/utils/RestTestProtoLoader.java

blakeli0 · 2023-05-18T23:15:29Z

Please see #1850 for showing of generated code corresponding to these changes

Not part of this PR, I wonder if it makes sense to regenerate the auto configs as part of the CI if we have changes in SpringCodeGen, so that we can see the changes directly in the same PR.

This reverts commit b80e2b5.

emmileaf · 2023-05-19T16:33:16Z

Not part of this PR, I wonder if it makes sense to regenerate the auto configs as part of the CI if we have changes in SpringCodeGen, so that we can see the changes directly in the same PR.

@blakeli0 This suggestion actually makes me wonder - what would be the preferred timing to push any generated code changes introduced by changes to the generator? Currently I see the possibility for:

As a commit added to the PR with generator changes (similar to your suggestion)
As a separate PR along with (or after) merging the generator changes
Not merged when introudcing generator changes, but will be included in the pre-release workflow in the PR to re-generate libraries as part of updating libraries-bom.

The status quo is 3, though this PR has been the first large generator feature change since initial release. Do we have any preference on what approach to take with this PR (which I will do manually), and which (if not the same) approach to follow in general? I think the possible CI/automation enhancements will look slightly different for each option.

.../src/main/java/com/google/cloud/generator/spring/composer/SpringAutoConfigClassComposer.java

...va/com/google/cloud/generator/spring/composer/goldens/EchoSpringAutoConfigurationRest.golden

meltsufin · 2023-05-23T01:49:31Z

.../src/main/java/com/google/cloud/generator/spring/composer/SpringPropertiesClassComposer.java

-    if (hasRestOption) {
+    // For GRPC+REST libraries, provide configuration property to override GRPC default and use REST transport instead
+    // For GRPC-only and REST-only libraries, this property is not provided
+    if (ComposerUtils.shouldSupportRestOptionWithGrpcDefault(transport, service)) {
      ExprStatement useRestVarStatement =
          ComposerUtils.createMemberVarStatement(
              "useRest",


Have you considered a transport property with an enum value of "REST" or "GRPC" instead? I feel that would be more natural than useRest.

Hmm, I can't recall if this was discussed when the useRest property was initially implemented, but two small complications I can see are:

This would be a breaking change for any existing users of this property (though now would be the time if it's a change we'd like to eventually make)

Possibly needing to document or account for case sensitivity in user-provided property strings? (e.g. REST vs. Rest)

If I'm understanding correctly, the user configuration to override would change from something like

<service.prefix>.useRest=true

to

<service.prefix>.transport=REST

This property is only exposed for libraries that support grpc+rest (and default to grpc), and having a property named useRest as an explicit override makes sense in my head as well, so I don't feel too strongly about either naming choice. Curious to hear any other concerns behind this suggestion though!

We're able to make this kind of change because we're still in preview. I believe that Spring Boot's relaxed binding should be case-insensitive for enums.
@blakeli0 Do you have an opinion on this?

I do think explicitly specify<service.prefix>.transport=REST might be better in the long term, but there are more things we need to consider for this change.
For example, what would be the default value if this property is not specified? REST for compute and GRPC for the others? What's the criteria for us to decide the default value? What if a customer configured GRPC for compute, are we going to error out on start up?
In addition, are we going to support this property for existing spring-cloud-gcp products as well? Some of the service does not support REST yet or only support REST(bigquery), but I think some service like spanner already support both.

Spring Boot's relaxed binding should be case-insensitive for enums

Oh that’s good to know! I can give this change a try in a separate PR.

For example, what would be the default value if this property is not specified? REST for compute and GRPC for the others? What's the criteria for us to decide the default value? What if a customer configured GRPC for compute, are we going to error out on start up?

@blakeli0 My understanding of this suggested change is just in name/type (useRest boolean => transport enum) rather than the underlying behaviour: the current strategy is to only provide and support this property for libraries that support both GRPC+REST transport options. For GRPC and REST-only libraries where this property is not supported, any user configuration will not do anything (unless they override the transport provider and setting beans themselves), and the client library default builders will be used.

Would there be concern in this strategy itself, or is it more that having a renamed transport property only supported and applicable to some libraries could be confusing to users?

I think @blakeli0 brings up a good point about more things to consider. For instance, we can imagine a global spring.cloud.gcp.transport property that could switch all integrations to that transport. I almost feel like the property should be a list of desired transports. For instance, GRPC,REST would be the default and mean that choose REST only if GRPC is unavailable. Users, can still specify something like 'REST', which should fail with an unsupported-transport exception for libraries that only support GRPC.

I feel this is a relatively large topic that worth it's own discussion, can we leave it as it is for now? And put all the discussion around the naming together in a separate doc? We can always change it in preview but we should finalize it before GA.

For sure - I can put together a doc on this topic to further explore/discuss (similar to the other conversation on strategy to enable/disable configuration classes).

blakeli0 · 2023-05-23T16:08:35Z

Not part of this PR, I wonder if it makes sense to regenerate the auto configs as part of the CI if we have changes in SpringCodeGen, so that we can see the changes directly in the same PR.

@blakeli0 This suggestion actually makes me wonder - what would be the preferred timing to push any generated code changes introduced by changes to the generator? Currently I see the possibility for:

As a commit added to the PR with generator changes (similar to your suggestion)

As a separate PR along with (or after) merging the generator changes

Not merged when introudcing generator changes, but will be included in the pre-release workflow in the PR to re-generate libraries as part of updating libraries-bom.

The status quo is 3, though this PR has been the first large generator feature change since initial release. Do we have any preference on what approach to take with this PR (which I will do manually), and which (if not the same) approach to follow in general? I think the possible CI/automation enhancements will look slightly different for each option.

We would always need 3 because the libraries bom could include new proto changes. In order for us to easily the generator changes, I think either 1 or 2 makes sense, otherwise all the generator changes and proto changes are in one huge PR before a new release. I would prefer 1 because it's easier to see all the generated changes in one PR, but 2 also has the advantage of seeing the generated changes separately so it's easier to review. This is not urgent, we can discuss it in next project meeting.

…nerator-java

.../test/java/com/google/cloud/generator/spring/composer/SpringAutoConfigClassComposerTest.java

…rt in GapicContext

emmileaf · 2023-05-25T21:20:24Z

@blakeli0 @meltsufin Thanks for your patience in reviewing this! I’ve run the code generation workflow on this feature branch to now include the generated code changes as well, but please let me know if you have preferences on whether to merge these together or separately.

I think this PR is otherwise ready for a re-review at your convenience. Quick summary of three items from discussions above that I will follow-up on, separately from this PR:

Alternative strategies for disabling/enabling autoconfiguration classes, and how these affect application start-up time (discussion)
Consider renaming and perhaps expanding configuration scope of the <service.prefix>.useRest property (discussion)
Workflow to push generated code changes introduced by generator changes as part of CI (discussion)

blakeli0 · 2023-05-26T18:36:41Z

...rc/main/java/com/google/cloud/compute/v1/spring/AcceleratorTypesSpringAutoConfiguration.java

@@ -0,0 +1,223 @@
+/*
+ * Copyright 2022 Google LLC


Noticed the copyright year is still 2022, this is probably a bug in the generator though, similar to what I did last year. Can you take a look?

Hmm yeah this part is being handled by the gapic-generator-java composers; I've opened googleapis/sdk-platform-java#1720 for the corresponding fix. Thanks for catching!

License headers (note: for all generated modules) updated in 34495a0.

This reverts commit 213add0.

emmileaf · 2023-06-12T15:05:24Z

Reverted changes to generated code, and retriggering to include generator updates above in https://github.com/GoogleCloudPlatform/spring-cloud-gcp/actions/runs/5245239436.

sonarqubecloud · 2023-06-12T16:01:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

100.0% Coverage
0.0% Duplication

emmileaf · 2023-06-12T16:13:49Z

Merging this in, with corresponding release note message (through commit override) as feat: add google-cloud-compute-spring-starter to starter modules available for preview

emmileaf added 5 commits May 15, 2023 10:37

Remove transport type exclusion in generate-library-list

e27c8f0

Update hasRestOption

950ad5b

Add composer unit tests for rest transport

0a206db

Add rest_numeric_enums as bazel rule argument

b80e2b5

Remove rest transport exclusion in SpringComposer

9a2be17

emmileaf added the codegen label May 15, 2023

emmileaf mentioned this pull request May 15, 2023

feat: add starter module for compute to spring-cloud-previews #1850

Closed

emmileaf marked this pull request as ready for review May 16, 2023 17:42

emmileaf requested review from zhumin8, blakeli0, meltsufin and diegomarquezp May 16, 2023 18:03

blakeli0 reviewed May 18, 2023

View reviewed changes

emmileaf added 2 commits May 19, 2023 10:35

Revert "Add rest_numeric_enums as bazel rule argument"

3e611e4

This reverts commit b80e2b5.

Switch rest-only golden tests to use echo instead of compliance proto

2db516b

emmileaf requested a review from blakeli0 May 19, 2023 16:34

Refactoring of logic related to hasRestOption

8d7954f

meltsufin reviewed May 23, 2023

View reviewed changes

emmileaf added 4 commits May 23, 2023 14:31

Update generated javadoc for transport channel provider bean

06b437c

Clean up comments with generated code

2628a46

Merge branch 'main' into codegen-add-rest

f418b3e

Delete copy of RestTestProtoLoader and use helper class from gapic-ge…

5c1614f

…nerator-java

emmileaf commented May 25, 2023

View reviewed changes

.../test/java/com/google/cloud/generator/spring/composer/SpringAutoConfigClassComposerTest.java Outdated Show resolved Hide resolved

emmileaf and others added 3 commits May 25, 2023 13:50

test: use TestProtoLoader for parsing echo proto and override transpo…

0ff01b2

…rt in GapicContext

chore: remove unused RestTestProtoLoader import

eaf2ef5

chore: update starter modules in spring-cloud-previews

213add0

emmileaf closed this May 25, 2023

emmileaf reopened this May 25, 2023

meltsufin approved these changes May 26, 2023

View reviewed changes

blakeli0 reviewed May 26, 2023

View reviewed changes

emmileaf added 2 commits June 12, 2023 11:01

Revert "chore: update starter modules in spring-cloud-previews"

5a7c1f6

This reverts commit 213add0.

Merge branch 'main' into codegen-add-rest

6e6a18a

emmileaf requested a review from a team as a code owner June 12, 2023 15:01

chore: update starter modules in spring-cloud-previews

34495a0

emmileaf closed this Jun 12, 2023

emmileaf reopened this Jun 12, 2023

emmileaf requested a review from blakeli0 June 12, 2023 15:33

blakeli0 approved these changes Jun 12, 2023

View reviewed changes

emmileaf merged commit 41b5c19 into main Jun 12, 2023

emmileaf deleted the codegen-add-rest branch June 12, 2023 16:17

emmileaf added the release-please:force-run label Jun 12, 2023

release-please bot removed the release-please:force-run label Jun 12, 2023

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

chore(main): release 4.5.0 #1941

Merged

emmileaf added the release-please:force-run label Jun 12, 2023

release-please bot removed the release-please:force-run label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for rest-only services in generator #1849

feat: add support for rest-only services in generator #1849

emmileaf commented May 15, 2023 •

edited

Loading

blakeli0 left a comment

blakeli0 commented May 18, 2023

emmileaf commented May 19, 2023

meltsufin May 23, 2023

emmileaf May 23, 2023

meltsufin May 23, 2023 •

edited

Loading

blakeli0 May 23, 2023

emmileaf May 23, 2023

meltsufin May 24, 2023

blakeli0 May 24, 2023

emmileaf May 24, 2023

blakeli0 commented May 23, 2023

emmileaf commented May 25, 2023

blakeli0 May 26, 2023

emmileaf May 29, 2023

emmileaf Jun 12, 2023

emmileaf commented Jun 12, 2023

sonarqubecloud bot commented Jun 12, 2023

emmileaf commented Jun 12, 2023

feat: add support for rest-only services in generator #1849

feat: add support for rest-only services in generator #1849

Conversation

emmileaf commented May 15, 2023 • edited Loading

Generator changes:

For reviewers:

blakeli0 left a comment

Choose a reason for hiding this comment

blakeli0 commented May 18, 2023

emmileaf commented May 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin May 23, 2023 • 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

blakeli0 commented May 23, 2023

emmileaf commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmileaf commented Jun 12, 2023

sonarqubecloud bot commented Jun 12, 2023

emmileaf commented Jun 12, 2023

emmileaf commented May 15, 2023 •

edited

Loading

meltsufin May 23, 2023 •

edited

Loading