Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(spring): extend hasRestOption fix to properties composer and add golden tests #1348

Merged
merged 4 commits into from
Feb 16, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this property used in this composer? Why we don't need this logic in #1343?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense - I've updated the comment composer correspondingly. Thanks for the suggestion!

// 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 @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to need to add an entry in this class for every Showcase client that we generate?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
@@ -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 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);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package com.google.showcase.v1beta1.spring;

import com.google.cloud.spring.core.Credentials;
import com.google.cloud.spring.core.CredentialsSupplier;
import com.google.cloud.spring.core.Retry;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.NestedConfigurationProperty;

// AUTO-GENERATED DOCUMENTATION AND CLASS.
/** Provides default property values for Wicked client bean */
@ConfigurationProperties("com.google.showcase.v1beta1.wicked")
public class WickedSpringProperties implements CredentialsSupplier {
/** OAuth2 credentials to authenticate and authorize calls to Google Cloud Client Libraries. */
@NestedConfigurationProperty private final Credentials credentials = new Credentials();
/** Quota project to use for billing. */
private String quotaProjectId;
/** Number of threads used for executors. */
private Integer executorThreadCount;
/** Allow override of retry settings at service level, applying to all of its RPC methods. */
@NestedConfigurationProperty private Retry retry;
/**
* Allow override of retry settings at method-level for craftEvilPlan. If defined, this takes
* precedence over service-level retry configurations for that RPC method.
*/
@NestedConfigurationProperty private Retry craftEvilPlanRetry;

@Override
public Credentials getCredentials() {
return this.credentials;
}

public String getQuotaProjectId() {
return this.quotaProjectId;
}

public void setQuotaProjectId(String quotaProjectId) {
this.quotaProjectId = quotaProjectId;
}

public Integer getExecutorThreadCount() {
return this.executorThreadCount;
}

public void setExecutorThreadCount(Integer executorThreadCount) {
this.executorThreadCount = executorThreadCount;
}

public Retry getRetry() {
return this.retry;
}

public void setRetry(Retry retry) {
this.retry = retry;
}

public Retry getCraftEvilPlanRetry() {
return this.craftEvilPlanRetry;
}

public void setCraftEvilPlanRetry(Retry craftEvilPlanRetry) {
this.craftEvilPlanRetry = craftEvilPlanRetry;
}
}
Loading