fix(spring): extend hasRestOption fix to properties composer and add golden tests #1348
Conversation
emmileaf
added
spring
| @@ -82,4 +83,35 @@ public GapicContext parseShowcaseEcho() { | |||
| .setTransport(getTransport()) | |||
| .build(); | |||
| } | |||
|
|
|||
| public GapicContext parseShowcaseWicked() { | |||
There was a problem hiding this comment.
Are we going to need to add an entry in this class for every Showcase client that we generate?
There was a problem hiding this comment.
We do need an entry in the test loader for every new test proto as the unit tests need to access the protoc generated Java files.
The name is a little confusing in this test due to historic reasons, but I don't consider this proto a showcase proto that will be used by showcase clients, this proto is used for unit tests only.
| ServiceDescriptor messagingService = fileDescriptor.getServices().get(0); | ||
| assertEquals("Wicked", messagingService.getName()); | ||
|
|
||
| Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor); |
There was a problem hiding this comment.
This logic looks duplicated with the above method's. If this pattern will be repeated for further clients, consider refactoring this common logic out into something like createGapicContext(fileDescriptor)
There was a problem hiding this comment.
Makes sense! This might be refactoring to look into for the main branch's GrpcRestTestProtoLoader.
Some additional context on this PR: for now the spring branch has diverged from the main branch (after the monorepo migration), so this is just a copied over change from #1236 to re-use this latest addition in main. The next-phase plan would have spring-related test code such as in this PR's https://github.com/googleapis/gapic-generator-java/blob/90838fe105931feb0cca0433e478e81889da1d0e/src/test/java/com/google/api/generator/spring/composer/SpringAutoConfigClassComposerTest.java#L37 switched to call this method from the published gapic-generator-java jar, once the spring generator is migrated to depend on it.
| @@ -75,7 +75,18 @@ public GapicClass generate(GapicContext context, Service service) { | |||
| String className = Utils.getServicePropertiesClassName(service); | |||
| GapicServiceConfig gapicServiceConfig = context.serviceConfig(); | |||
| Map<String, TypeNode> dynamicTypes = createDynamicTypes(service, packageName); | |||
| boolean hasRestOption = context.transport().equals(Transport.GRPC_REST); | |||
There was a problem hiding this comment.
How is this property used in this composer? Why we don't need this logic in #1343?
There was a problem hiding this comment.
@blakeli0 The changes from #1343 removed the generation of SpringAutoConfiguration code that condition on this property (if (this.clientProperties.getUseRest())) to set rest-specific defaults. (e.g. this change to TetherSpringAutoconfiguration)
The property itself is still there (just ignored by the autoconfig), and will be removed as part of this PR's changes. I realized when updating these tests that we shouldn't be generating the property either, and it was something I had overlooked in #1343's patch.
There was a problem hiding this comment.
I see, thanks for the explanation! It was an unused property hence not causing any compilation issues.
I did a quick search for rest and see that it is mentioned in this comment , we may want to update it to something like unless the useRest option is supported and provided, as not every client library support rest.
There was a problem hiding this comment.
That makes sense - I've updated the comment composer correspondingly. Thanks for the suggestion!
|
Kudos, SonarCloud Quality Gate passed! |
This PR is a follow-up on #1343:
SpringPropertiesClassComposer, so that for services without REST-enabled rpcs, the unuseduseRestproperty is also not generatedhasRestOptionscenario using the wicked protoSpringAutoconfigCommentComposerfor javadoc comments alluding to theuseRestoption