Skip to content

Commit

Permalink
fix(spring): extend hasRestOption fix to properties composer and add …
Browse files Browse the repository at this point in the history
…golden tests (#1348)

This PR is a follow-up on #1343:
 - Extends fix to SpringPropertiesClassComposer, so that for services without REST-enabled rpcs, the unused useRest property is also not generated
- Adds golden tests for the updated hasRestOption scenario using the wicked proto
- Updates SpringAutoconfigCommentComposer for javadoc comments alluding to the useRest option
  • Loading branch information
emmileaf authored Feb 16, 2023
1 parent 3f5db4d commit dcd0823
Show file tree
Hide file tree
Showing 11 changed files with 354 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
// TODO(emmwang): this condition is adapted from latest gapic-generator-java changes as part of
// https://github.com/googleapis/gapic-generator-java/issues/1117, but should be updated to use
// the gapic-implemented helpers once spring generator code is migrated.
boolean hasRestSupportedMethod =
service.methods().stream()
.anyMatch(
x ->
x.httpBindings() != null
&& x.stream() != Method.Stream.BIDI
&& x.stream() != Method.Stream.CLIENT);
boolean hasRestOption =
context.transport().equals(Transport.GRPC_REST) && hasRestSupportedMethod;

// TODO: this is the prefix user will use to set properties, may need to change depending on
// branding.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public class SpringAutoconfigCommentComposer {

public static final String TRANSPORT_CHANNEL_PROVIDER_GENERAL_DESCRIPTION =
"Provides a default transport channel provider bean. The default is gRPC and will default to it unless the "
+ "useRest option is provided to use HTTP transport instead";
+ "useRest option is supported and provided to use HTTP transport instead";
public static final String TRANSPORT_CHANNEL_PROVIDER_RETURN =
"a default transport channel provider.";
public static final String CLIENT_SETTINGS_BEAN_GENERAL_DESCRIPTION =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.protobuf.Descriptors.ServiceDescriptor;
import com.google.protobuf.StructProto;
import com.google.showcase.grpcrest.v1beta1.EchoGrpcrest;
import com.google.showcase.v1beta1.WickedOuterClass;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.HashSet;
Expand Down Expand Up @@ -82,4 +83,35 @@ public GapicContext parseShowcaseEcho() {
.setTransport(getTransport())
.build();
}

public GapicContext parseShowcaseWicked() {
FileDescriptor fileDescriptor = WickedOuterClass.getDescriptor();
ServiceDescriptor messagingService = fileDescriptor.getServices().get(0);
assertEquals("Wicked", messagingService.getName());

Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor);
messageTypes.putAll(Parser.parseMessages(OperationsProto.getDescriptor()));
messageTypes.putAll(Parser.parseMessages(StructProto.getDescriptor()));

Map<String, ResourceName> resourceNames = Parser.parseResourceNames(fileDescriptor);
Set<ResourceName> outputResourceNames = new HashSet<>();
List<Service> services =
Parser.parseService(
fileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);

String jsonFilename = "showcase_grpc_service_config.json";
Path jsonPath = Paths.get(getTestFilesDirectory(), jsonFilename);
Optional<GapicServiceConfig> configOpt = ServiceConfigParser.parse(jsonPath.toString());
assertTrue(configOpt.isPresent());
GapicServiceConfig config = configOpt.get();

return GapicContext.builder()
.setMessages(messageTypes)
.setResourceNames(resourceNames)
.setServices(services)
.setServiceConfig(config)
.setHelperResourceNames(outputResourceNames)
.setTransport(getTransport())
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.api.generator.spring.composer;

import com.google.api.generator.gapic.composer.common.TestProtoLoader;
import com.google.api.generator.gapic.composer.grpcrest.GrpcRestTestProtoLoader;
import com.google.api.generator.gapic.model.GapicClass;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.Service;
Expand All @@ -25,12 +26,16 @@

public class SpringAutoConfigClassComposerTest {
private GapicContext context;
private GapicContext wickedContext;
private Service echoProtoService;
private Service wickedProtoService;

@Before
public void setUp() {
this.context = TestProtoLoader.instance().parseShowcaseEcho();
this.echoProtoService = this.context.services().get(0);
this.wickedContext = GrpcRestTestProtoLoader.instance().parseShowcaseWicked();
this.wickedProtoService = this.wickedContext.services().get(0);
}

@Test
Expand All @@ -43,12 +48,21 @@ public void generateAutoConfigClazzGrpcTest() {

@Test
public void generateAutoConfigClazzGrpcRestTest() {

GapicContext contextGrpcRest =
this.context.toBuilder().setTransport(Transport.GRPC_REST).build();
GapicClass clazz =
SpringAutoConfigClassComposer.instance().generate(contextGrpcRest, this.echoProtoService);
String fileName = clazz.classDefinition().classIdentifier() + "GrpcRest.golden";
Assert.assertGoldenClass(this.getClass(), clazz, fileName);
}

@Test
public void generateAutoConfigClazzNoRestRpcsTest() {
GapicContext contextGrpcRest =
this.wickedContext.toBuilder().setTransport(Transport.GRPC_REST).build();
GapicClass clazz =
SpringAutoConfigClassComposer.instance().generate(contextGrpcRest, this.wickedProtoService);
String fileName = clazz.classDefinition().classIdentifier() + "NoRestRpcs.golden";
Assert.assertGoldenClass(this.getClass(), clazz, fileName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.api.generator.spring.composer;

import com.google.api.generator.gapic.composer.common.TestProtoLoader;
import com.google.api.generator.gapic.composer.grpcrest.GrpcRestTestProtoLoader;
import com.google.api.generator.gapic.model.GapicClass;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.Service;
Expand All @@ -25,12 +26,16 @@

public class SpringPropertiesClassComposerTest {
private GapicContext context;
private GapicContext wickedContext;
private Service echoProtoService;
private Service wickedProtoService;

@Before
public void setUp() {
this.context = TestProtoLoader.instance().parseShowcaseEcho();
this.echoProtoService = this.context.services().get(0);
this.wickedContext = GrpcRestTestProtoLoader.instance().parseShowcaseWicked();
this.wickedProtoService = this.wickedContext.services().get(0);
}

@Test
Expand All @@ -50,4 +55,14 @@ public void generatePropertiesClazzGrpcRestTest() {
String fileName = clazz.classDefinition().classIdentifier() + "GrpcRest.golden";
Assert.assertGoldenClass(this.getClass(), clazz, fileName);
}

@Test
public void generatePropertiesClazzNoRestRpcsTest() {
GapicContext contextGrpcRest =
this.wickedContext.toBuilder().setTransport(Transport.GRPC_REST).build();
GapicClass clazz =
SpringPropertiesClassComposer.instance().generate(contextGrpcRest, this.wickedProtoService);
String fileName = clazz.classDefinition().classIdentifier() + "NoRestRpcs.golden";
Assert.assertGoldenClass(this.getClass(), clazz, fileName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public class EchoSpringAutoConfiguration {

/**
* Provides a default transport channel provider bean. The default is gRPC and will default to it
* unless the useRest option is provided to use HTTP transport instead
* unless the useRest option is supported and provided to use HTTP transport instead
*
* @return a default transport channel provider.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public class EchoSpringAutoConfiguration {

/**
* Provides a default transport channel provider bean. The default is gRPC and will default to it
* unless the useRest option is provided to use HTTP transport instead
* unless the useRest option is supported and provided to use HTTP transport instead
*
* @return a default transport channel provider.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public class EchoSpringAutoConfiguration {

/**
* Provides a default transport channel provider bean. The default is gRPC and will default to it
* unless the useRest option is provided to use HTTP transport instead
* unless the useRest option is supported and provided to use HTTP transport instead
*
* @return a default transport channel provider.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
package com.google.showcase.v1beta1.spring;

import com.google.api.gax.core.CredentialsProvider;
import com.google.api.gax.core.ExecutorProvider;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.rpc.HeaderProvider;
import com.google.api.gax.rpc.TransportChannelProvider;
import com.google.cloud.spring.autoconfigure.core.GcpContextAutoConfiguration;
import com.google.cloud.spring.core.Credentials;
import com.google.cloud.spring.core.DefaultCredentialsProvider;
import com.google.cloud.spring.core.Retry;
import com.google.cloud.spring.core.util.RetryUtil;
import com.google.showcase.v1beta1.WickedClient;
import com.google.showcase.v1beta1.WickedSettings;
import java.io.IOException;
import java.util.Collections;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.boot.autoconfigure.AutoConfiguration;
import org.springframework.boot.autoconfigure.AutoConfigureAfter;
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.context.annotation.Bean;

// AUTO-GENERATED DOCUMENTATION AND CLASS.
/**
* Auto-configuration for {@link WickedClient}.
*
* <p>Provides auto-configuration for Spring Boot
*
* <p>The default instance has everything set to sensible defaults:
*
* <ul>
* <li>The default transport provider is used.
* <li>Credentials are acquired automatically through Application Default Credentials.
* <li>Retries are configured for idempotent methods but not for non-idempotent methods.
* </ul>
*/
@AutoConfiguration
@AutoConfigureAfter(GcpContextAutoConfiguration.class)
@ConditionalOnClass(WickedClient.class)
@ConditionalOnProperty(value = "com.google.showcase.v1beta1.wicked.enabled", matchIfMissing = true)
@EnableConfigurationProperties(WickedSpringProperties.class)
public class WickedSpringAutoConfiguration {
private final WickedSpringProperties clientProperties;
private final CredentialsProvider credentialsProvider;
private static final Log LOGGER = LogFactory.getLog(WickedSpringAutoConfiguration.class);

protected WickedSpringAutoConfiguration(
WickedSpringProperties clientProperties, CredentialsProvider credentialsProvider)
throws IOException {
this.clientProperties = clientProperties;
if (this.clientProperties.getCredentials().hasKey()) {
if (LOGGER.isTraceEnabled()) {
LOGGER.trace("Using credentials from Wicked-specific configuration");
}
this.credentialsProvider =
((CredentialsProvider) new DefaultCredentialsProvider(this.clientProperties));
} else {
this.credentialsProvider = credentialsProvider;
}
}

/**
* Provides a default transport channel provider bean. The default is gRPC and will default to it
* unless the useRest option is supported and provided to use HTTP transport instead
*
* @return a default transport channel provider.
*/
@Bean
@ConditionalOnMissingBean(name = "defaultWickedTransportChannelProvider")
public TransportChannelProvider defaultWickedTransportChannelProvider() {
return WickedSettings.defaultTransportChannelProvider();
}

/**
* Provides a WickedSettings bean configured to use a DefaultCredentialsProvider and the client
* library's default transport channel provider (defaultWickedTransportChannelProvider()). It also
* configures the quota project ID and executor thread count, if provided through properties.
*
* <p>Retry settings are also configured from service-level and method-level properties specified
* in WickedSpringProperties. Method-level properties will take precedence over service-level
* properties if available, and client library defaults will be used if neither are specified.
*
* @param defaultTransportChannelProvider TransportChannelProvider to use in the settings.
* @return a {@link WickedSettings} bean configured with {@link TransportChannelProvider} bean.
*/
@Bean
@ConditionalOnMissingBean
public WickedSettings wickedSettings(
@Qualifier("defaultWickedTransportChannelProvider")
TransportChannelProvider defaultTransportChannelProvider)
throws IOException {
WickedSettings.Builder clientSettingsBuilder = WickedSettings.newBuilder();
clientSettingsBuilder
.setCredentialsProvider(this.credentialsProvider)
.setTransportChannelProvider(defaultTransportChannelProvider)
.setHeaderProvider(this.userAgentHeaderProvider());
if (this.clientProperties.getQuotaProjectId() != null) {
clientSettingsBuilder.setQuotaProjectId(this.clientProperties.getQuotaProjectId());
if (LOGGER.isTraceEnabled()) {
LOGGER.trace(
"Quota project id set to "
+ this.clientProperties.getQuotaProjectId()
+ ", this overrides project id from credentials.");
}
}
if (this.clientProperties.getExecutorThreadCount() != null) {
ExecutorProvider executorProvider =
WickedSettings.defaultExecutorProviderBuilder()
.setExecutorThreadCount(this.clientProperties.getExecutorThreadCount())
.build();
clientSettingsBuilder.setBackgroundExecutorProvider(executorProvider);
if (LOGGER.isTraceEnabled()) {
LOGGER.trace(
"Background executor thread count is "
+ this.clientProperties.getExecutorThreadCount());
}
}
Retry serviceRetry = clientProperties.getRetry();
if (serviceRetry != null) {
RetrySettings craftEvilPlanRetrySettings =
RetryUtil.updateRetrySettings(
clientSettingsBuilder.craftEvilPlanSettings().getRetrySettings(), serviceRetry);
clientSettingsBuilder.craftEvilPlanSettings().setRetrySettings(craftEvilPlanRetrySettings);

if (LOGGER.isTraceEnabled()) {
LOGGER.trace("Configured service-level retry settings from properties.");
}
}
Retry craftEvilPlanRetry = clientProperties.getCraftEvilPlanRetry();
if (craftEvilPlanRetry != null) {
RetrySettings craftEvilPlanRetrySettings =
RetryUtil.updateRetrySettings(
clientSettingsBuilder.craftEvilPlanSettings().getRetrySettings(), craftEvilPlanRetry);
clientSettingsBuilder.craftEvilPlanSettings().setRetrySettings(craftEvilPlanRetrySettings);
if (LOGGER.isTraceEnabled()) {
LOGGER.trace("Configured method-level retry settings for craftEvilPlan from properties.");
}
}
return clientSettingsBuilder.build();
}

/**
* Provides a WickedClient bean configured with WickedSettings.
*
* @param wickedSettings settings to configure an instance of client bean.
* @return a {@link WickedClient} bean configured with {@link WickedSettings}
*/
@Bean
@ConditionalOnMissingBean
public WickedClient wickedClient(WickedSettings wickedSettings) throws IOException {
return WickedClient.create(wickedSettings);
}

private HeaderProvider userAgentHeaderProvider() {
String springLibrary = "spring-autogen-wicked";
String version = this.getClass().getPackage().getImplementationVersion();
return () -> Collections.singletonMap("user-agent", springLibrary + "/" + version);
}
}
Loading

0 comments on commit dcd0823

Please sign in to comment.