fix(Spring CodeGen): Update missing and placeholder annotations #1045
Conversation
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
There was a problem hiding this comment.
LGTM! I just have two comments about reusing defined types.
- About variable naming conventions, I think your naming looks good. I also try to spell-out the actual content as well as the ast type e.g.
AssignmentExpr, and realize sometimes it can get quite long. In my draft code, you might still see some naming that's not comprehensive enough, feel free to change them. I will also try to do another cleanup later on. - On pr naming, @blakeli0 I've tried to follow "fix():...", "feat():..." but wonder if we have any guidelines on naming? Also, I'd like to add a "spring" label to these PRs if that does not interfere with any of the automated tasks?
src/main/java/com/google/api/generator/spring/composer/SpringAutoConfigClassComposer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/api/generator/spring/composer/SpringAutoConfigClassComposer.java
Outdated
Show resolved
Hide resolved
emmileaf
changed the title
emmileaf
changed the title
emmileaf
changed the title
|
Updated this PR with the requested changes as well as the two other annotation-related items. Also checked locally that there will be a couple of conflicts with both #1044 and #1046, |
zhumin8
added
the
spring
|
Kudos, SonarCloud Quality Gate passed! |
| @@ -189,8 +190,9 @@ public void generatePropertiesTest() { | |||
| + " @Bean\n" | |||
| + " @ConditionalOnMissingBean\n" | |||
| + " public EchoClient echoClient(\n" | |||
| + " CredentialsProvider credentialsProvider,\n" | |||
| + " TransportChannelProvider defaultTransportChannelProvider)\n" | |||
| + " @Qualifier(\"googleCredentials\") CredentialsProvider credentialsProvider,\n" | |||
There was a problem hiding this comment.
Totally missed this: We want to add this @Qualifier so that when multiple services are present, there won't be conflict in which CredentialsProvider bean it takes. So need to rename googleCredentials() to [serviceName]Credentials() and match the name here. (same as defaultEchoTransportChannelProvider())
There was a problem hiding this comment.
Thanks for catching this - will address in a separate PR!
* Updates annotation parameter placeholder strings in SpringAutoConfigClassComposer for parameters of type class * Updates annotation parameter placeholder strings in SpringAutoConfigClassComposer for multiple parameters * Adds @qualifier annotations on variables * Adds @NestedConfigurationProperty annotation on credentials field in SpringPropertiesClassComposer
…tatic types instead of vapor references (#1046) Adding Spring related dependencies to build files and use static types instead of vapor references. Benefit of this change: - easier to test - less code change needed for future changes (e.g. package change of classes). One caveat is that because `gapic-generator-java` is brought into `googleapis` as `http_archive` code addition to `googleapis` is also needed to include these libraries. Add below code snippet to [WORKSPACE](https://github.com/googleapis/googleapis/blob/38e8b447d173909d3b2fe8fdc3e6cbb3c85442fd/WORKSPACE#L239-L245): ``` SPRING_MAVEN_ARTIFACTS = [ "org.springframework.boot:spring-boot-starter:2.7.4", "com.google.cloud:spring-cloud-gcp-core:3.3.0", ] maven_install( artifacts = PROTOBUF_MAVEN_ARTIFACTS + SPRING_MAVEN_ARTIFACTS, generate_compat_repositories = True, repositories = [ "https://repo.maven.apache.org/maven2/", ], ) ``` ------------ Updates: - Updated pr with #1065, - Moved annotation classed added in #1045 to static types. - Fixed conflicts from merged changes in #1093 - Moved logging classes added in #1053 to static types.
In this PR:
SpringAutoConfigClassComposerfor parameters of type classSpringAutoConfigClassComposerfor multiple parameters@Qualifierannotations on variables@NestedConfigurationPropertyannotation on credentials field inSpringPropertiesClassComposer@zhumin8
Do you have any preferences on whether I use this this branch/PR to also work on the other annotation TODOs in the code, or have the changes be separate for now?(Edit: will use this PR since they are pretty closely related) I also wasn't too sure what naming conventions to use for the variables, so would appreciate any feedback here as well.Potential conflicts:
* With #1046 (Switching Spring dependencies from dynamic to static types)
* With #1044 (
@Configuration(proxyBeanMethods = false)is no longer used in Spring Boot 2.7)