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

feat: Do not generate Service REST code if there are no matching RPC in a Service #1236

Merged
merged 69 commits into from
Jan 27, 2023
Merged
Show file tree
Hide file tree
Changes from 66 commits
Commits
Show all changes
69 commits
Select commit Hold shift + click to select a range
d13b388
feat: Add callable getter methods for REST
lqiu96 Jan 4, 2023
1fc3cc7
chore: Update showcase tests
lqiu96 Jan 4, 2023
516e2e8
chore: Update error message
lqiu96 Jan 5, 2023
3ad09dc
feat: Move httpjson specific logic to sub composer
lqiu96 Jan 5, 2023
edf088a
feat: Move method supported logic to Method
lqiu96 Jan 5, 2023
66423e7
feat: Move method supported logic to Method
lqiu96 Jan 5, 2023
dc6f0cd
chore: Format the files
lqiu96 Jan 5, 2023
1940216
chore: Cleanup Abstract composer
lqiu96 Jan 5, 2023
eb37263
chore: Move code to httpjson composer
lqiu96 Jan 5, 2023
5a3515f
chore: Resolve code smell
lqiu96 Jan 5, 2023
0169f6a
feat: Use generic error message
lqiu96 Jan 6, 2023
44e7580
chore: Fix format issues
lqiu96 Jan 6, 2023
3708806
feat: Add tests for Method.isSupportedByTransport()
lqiu96 Jan 9, 2023
e05b385
feat: Resolve PR comments
lqiu96 Jan 10, 2023
738200f
feat: Update back to private
lqiu96 Jan 10, 2023
715fcdc
Merge branch 'main' into main-rest_method_generation
lqiu96 Jan 10, 2023
cc0d97b
feat: Update error message
lqiu96 Jan 10, 2023
14e7d9e
feat: Update javadoc comment
lqiu96 Jan 10, 2023
246ffee
feat: Do not generate Service REST code if there are no matching RPC …
lqiu96 Jan 11, 2023
fde1b68
chore: Pull in latest part 1 changes
lqiu96 Jan 11, 2023
168a925
chore: Pull in latest part 1 changes
lqiu96 Jan 11, 2023
d7ebc76
fix: Update Kind to NON_GENERATED
lqiu96 Jan 11, 2023
4b5f606
feat: Do not generate secondary Transport sample if no rest code is g…
lqiu96 Jan 12, 2023
240ee8f
feat: Do not generate httpjson tests if no matching RPCs
lqiu96 Jan 12, 2023
4541239
chore: Run mvn test -DupdateUnitGoldens
lqiu96 Jan 12, 2023
7b6ade0
chore: Update formatting
lqiu96 Jan 12, 2023
2b40a47
chore: Add apigeeconnect to test REGAPIC
lqiu96 Jan 12, 2023
5ea2938
chore: Create directory for new modules
lqiu96 Jan 12, 2023
83473a1
chore: Update googleapis commit to a later version for grpc+rest enab…
lqiu96 Jan 12, 2023
58bf7b5
chore Add in goldens for apigeeconnect
lqiu96 Jan 12, 2023
cd00ab8
Merge branch 'main' into main-rest_method_generation_2
lqiu96 Jan 17, 2023
e2e5f45
chore: Add comments for funcs
lqiu96 Jan 17, 2023
bf57544
chore: Refactor ServiceClientClassComposer for GRPC_REST
lqiu96 Jan 17, 2023
d180320
chore: Remove unused import
lqiu96 Jan 18, 2023
8979217
chore: Remove unnecessary if/else check
lqiu96 Jan 18, 2023
5a6b014
chore: Fix transport sample name
lqiu96 Jan 18, 2023
9420be8
chore: Update apigeeconnect IT goldens
lqiu96 Jan 18, 2023
e5fad03
chore: Update asset IT goldens
lqiu96 Jan 18, 2023
64baf50
chore: Update asset goldens
lqiu96 Jan 18, 2023
0c2893c
chore: Update credentials IT goldens
lqiu96 Jan 18, 2023
ea804b5
chore: Update library IT goldens
lqiu96 Jan 18, 2023
c9a236a
chore: Update redis IT goldens
lqiu96 Jan 18, 2023
4986e85
chore: Fix linting issues
lqiu96 Jan 18, 2023
766e9e7
chore: Add showcase extended proto framework
lqiu96 Jan 19, 2023
983ab9b
chore: Use seperate Bazel rules for showcase extended
lqiu96 Jan 19, 2023
2915149
chore: Seperate GRPC jar into separate jobs
lqiu96 Jan 20, 2023
aa666b2
chore: Update bazel build file
lqiu96 Jan 20, 2023
485a222
Apply suggestions from code review
lqiu96 Jan 20, 2023
f1d8b9c
chore: Update showcase tests
lqiu96 Jan 23, 2023
10040ed
chore: Resolve sonar comments
lqiu96 Jan 23, 2023
27d1f1c
chore: Update unit tests for wicked proto
lqiu96 Jan 23, 2023
219ae2c
chore: Resolve format
lqiu96 Jan 23, 2023
b5810e0
chore: Update sonar comments
lqiu96 Jan 23, 2023
53eaa6c
chore: Update PR comments
lqiu96 Jan 24, 2023
892395e
chore: Update golden test cases
lqiu96 Jan 24, 2023
dc02ecc
chore: Revert back to original sample name
lqiu96 Jan 24, 2023
76c67e1
chore: Update showcase and goldens
lqiu96 Jan 24, 2023
0aa24df
chore: Leave framework but remove wicked proto from showcase extended
lqiu96 Jan 24, 2023
717a1b7
chore: Update showcase project with extended info
lqiu96 Jan 24, 2023
55d4cfa
chore: Remove comment
lqiu96 Jan 24, 2023
a93b8f4
chore: Use TransportContext instead of duplicate transportName()
lqiu96 Jan 24, 2023
1a11c3b
chore: Fix formatting issues
lqiu96 Jan 24, 2023
31a486b
chore: Remove the changed spacing in the file
lqiu96 Jan 24, 2023
55ffcd3
chore: Use transportContext where possible
lqiu96 Jan 24, 2023
f2e6b17
chore: Fix format issues
lqiu96 Jan 24, 2023
3435225
chore: Fix compile issue
lqiu96 Jan 24, 2023
7f613ed
Merge branch 'main' into main-rest_method_generation_2
lqiu96 Jan 25, 2023
30b81bc
Merge branch 'main' into main-rest_method_generation_2
lqiu96 Jan 26, 2023
8a061df
Merge branch 'main' into main-rest_method_generation_2
lqiu96 Jan 27, 2023
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 @@ -62,6 +62,11 @@ protected TransportContext getTransportContext() {

@Override
public GapicClass generate(GapicContext context, Service service) {
// Do not generate the Callable Factory if there are no RPCs enabled for the Transport
if (!service.hasAnyEnabledMethodsForTransport(getTransportContext().transport())) {
return GapicClass.createNonGeneratedGapicClass();
}

TypeStore typeStore = createTypes(service);

String className =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ public GapicClass generate(GapicContext context, Service service) {
}

protected GapicClass generate(String className, GapicContext context, Service service) {
// Do not generate Client Test code for Transport if there are no matching RPCs for a Transport
if (!service.hasAnyEnabledMethodsForTransport(getTransportContext().transport())) {
return GapicClass.createNonGeneratedGapicClass();
}

Map<String, ResourceName> resourceNames = context.helperResourceNames();
String pakkage = service.pakkage();
TypeStore typeStore = new TypeStore();
Expand All @@ -131,10 +136,6 @@ protected GapicClass generate(String className, GapicContext context, Service se
return GapicClass.create(kind, classDef);
}

protected boolean isSupportedMethod(Method method) {
return true;
}

private List<AnnotationNode> createClassAnnotations() {
return Arrays.asList(
AnnotationNode.builder()
Expand Down Expand Up @@ -230,7 +231,7 @@ private List<MethodDefinition> createTestMethods(
Map<String, Message> messageTypes = context.messages();
List<MethodDefinition> javaMethods = new ArrayList<>();
for (Method method : service.methods()) {
if (!isSupportedMethod(method)) {
if (!method.isSupportedByTransport(getTransportContext().transport())) {
javaMethods.add(createUnsupportedTestMethod(method));
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,14 @@ private List<MethodDefinition> createDefaultGetterMethods(Service service, TypeS
while (providerBuilderNamesIt.hasNext()
&& channelProviderClassesIt.hasNext()
&& transportNamesIt.hasNext()) {
String providerBuilderName = providerBuilderNamesIt.next();
Class<?> channelProviderClass = channelProviderClassesIt.next();
String transportName = transportNamesIt.next();

if (!service.hasAnyEnabledMethodsForTransport(transportName)) {
continue;
}

List<AnnotationNode> annotations = ImmutableList.of();

// For clients supporting multiple transports (mainly grpc+rest case) make secondary transport
Expand All @@ -397,16 +405,13 @@ private List<MethodDefinition> createDefaultGetterMethods(Service service, TypeS
SettingsCommentComposer.DEFAULT_TRANSPORT_PROVIDER_BUILDER_METHOD_COMMENT;
if (getTransportContext().transportNames().size() > 1) {
commentStatement =
new SettingsCommentComposer(transportNamesIt.next())
.getTransportProviderBuilderMethodComment();
new SettingsCommentComposer(transportName).getTransportProviderBuilderMethodComment();
}

javaMethods.add(
methodMakerFn.apply(
methodStarterFn
.apply(
providerBuilderNamesIt.next(),
typeMakerFn.apply(channelProviderClassesIt.next()))
.apply(providerBuilderName, typeMakerFn.apply(channelProviderClass))
.setAnnotations(annotations),
commentStatement));
secondaryTransportProviderBuilder = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,8 @@ protected MethodDefinition createDefaultCredentialsProviderBuilderMethod() {
.build();
}

protected List<MethodDefinition> createDefaultTransportTransportProviderBuilderMethods() {
protected List<MethodDefinition> createDefaultTransportTransportProviderBuilderMethods(
Service service) {
// Create the defaultGrpcTransportProviderBuilder method.
Iterator<Class<?>> providerClassIt =
getTransportContext().instantiatingChannelProviderClasses().iterator();
Expand All @@ -259,10 +260,19 @@ protected List<MethodDefinition> createDefaultTransportTransportProviderBuilderM
&& providerBuilderClassIt.hasNext()
&& builderNamesIt.hasNext()
&& transportNamesIt.hasNext()) {
Class<?> providerClass = providerClassIt.next();
Class<?> providerBuilderClass = providerBuilderClassIt.next();
String builderName = builderNamesIt.next();
String transportName = transportNamesIt.next();

if (!service.hasAnyEnabledMethodsForTransport(transportName)) {
continue;
}

TypeNode returnType =
TypeNode.withReference(ConcreteReference.withClazz(providerBuilderClassIt.next()));
TypeNode.withReference(ConcreteReference.withClazz(providerBuilderClass));
TypeNode channelProviderType =
TypeNode.withReference(ConcreteReference.withClazz(providerClassIt.next()));
TypeNode.withReference(ConcreteReference.withClazz(providerClass));

MethodInvocationExpr transportChannelProviderBuilderExpr =
MethodInvocationExpr.builder()
Expand All @@ -281,8 +291,7 @@ protected List<MethodDefinition> createDefaultTransportTransportProviderBuilderM
SettingsCommentComposer.DEFAULT_TRANSPORT_PROVIDER_BUILDER_METHOD_COMMENT;
if (getTransportContext().transportNames().size() > 1) {
commentStatement =
new SettingsCommentComposer(transportNamesIt.next())
.getTransportProviderBuilderMethodComment();
new SettingsCommentComposer(transportName).getTransportProviderBuilderMethodComment();
}

MethodDefinition method =
Expand All @@ -292,7 +301,7 @@ protected List<MethodDefinition> createDefaultTransportTransportProviderBuilderM
.setScope(ScopeNode.PUBLIC)
.setIsStatic(true)
.setReturnType(returnType)
.setName(builderNamesIt.next())
.setName(builderName)
.setReturnExpr(returnExpr)
.build();

Expand Down Expand Up @@ -1047,17 +1056,23 @@ private MethodDefinition createCreateStubMethod(Service service, TypeStore typeS
.setMethodName("getTransportName")
.build();

Iterator<String> transportNamesIt = getTransportContext().transportNames().iterator();
Iterator<TypeNode> channelTypesIt = getTransportContext().transportChannelTypes().iterator();
Iterator<String> getterNameIt = getTransportContext().transportGetterNames().iterator();
Iterator<String> serivceStubClassNameIt =
getTransportContext().classNames().getTransportServiceStubClassNames(service).iterator();

while (channelTypesIt.hasNext() && getterNameIt.hasNext()) {
String transportName = transportNamesIt.next();
TypeNode channelType = channelTypesIt.next();
String getterName = getterNameIt.next();
String serivceStubClassName = serivceStubClassNameIt.next();

Expr tRansportNameExpr =
if (!service.hasAnyEnabledMethodsForTransport(transportName)) {
continue;
}

Expr transportNameExpr =
MethodInvocationExpr.builder()
.setStaticReferenceType(channelType)
.setMethodName(getterName)
Expand All @@ -1067,7 +1082,7 @@ private MethodDefinition createCreateStubMethod(Service service, TypeStore typeS
MethodInvocationExpr.builder()
.setExprReferenceExpr(getTransportNameExpr)
.setMethodName("equals")
.setArguments(tRansportNameExpr)
.setArguments(transportNameExpr)
.setReturnType(TypeNode.BOOLEAN)
.build();

Expand Down Expand Up @@ -1191,7 +1206,7 @@ private List<MethodDefinition> createDefaultHelperAndGetterMethods(
.build());

javaMethods.add(createDefaultCredentialsProviderBuilderMethod());
javaMethods.addAll(createDefaultTransportTransportProviderBuilderMethods());
javaMethods.addAll(createDefaultTransportTransportProviderBuilderMethods(service));
javaMethods.add(createDefaultTransportChannelProviderMethod());
javaMethods.addAll(createApiClientHeaderProviderBuilderMethods(service, typeStore));

Expand Down Expand Up @@ -1424,7 +1439,7 @@ private List<MethodDefinition> createNestedClassMethods(
nestedClassMethods.addAll(
createNestedClassConstructorMethods(
service, serviceConfig, nestedMethodSettingsMemberVarExprs, typeStore));
nestedClassMethods.addAll(createNestedClassCreateDefaultMethods(typeStore));
nestedClassMethods.addAll(createNestedClassCreateDefaultMethods(service, typeStore));
nestedClassMethods.add(createNestedClassInitDefaultsMethod(service, serviceConfig, typeStore));
nestedClassMethods.add(createNestedClassApplyToAllUnaryMethodsMethod(superType, typeStore));
nestedClassMethods.add(createNestedClassUnaryMethodSettingsBuilderGetterMethod());
Expand Down Expand Up @@ -1762,14 +1777,19 @@ private static List<MethodDefinition> createNestedClassConstructorMethods(
return ctorMethods;
}

protected List<MethodDefinition> createNestedClassCreateDefaultMethods(TypeStore typeStore) {
return Collections.singletonList(
createNestedClassCreateDefaultMethod(
typeStore,
"createDefault",
"defaultTransportChannelProvider",
null,
"defaultApiClientHeaderProviderBuilder"));
protected List<MethodDefinition> createNestedClassCreateDefaultMethods(
Service service, TypeStore typeStore) {
if (service.hasAnyEnabledMethodsForTransport(getTransportContext().transport())) {
return Arrays.asList(
createNestedClassCreateDefaultMethod(
typeStore,
"createDefault",
"defaultTransportChannelProvider",
null,
"defaultApiClientHeaderProviderBuilder"));
} else {
return Collections.emptyList();
}
}

protected MethodDefinition createNestedClassCreateDefaultMethod(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.model.Transport;
import com.google.api.generator.gapic.utils.JavaStyle;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -94,7 +93,7 @@ public abstract class AbstractTransportServiceStubClassComposer implements Class
private static final String OPERATION_CALLABLE_CLASS_MEMBER_PATTERN = "%sOperationCallable";
private static final String OPERATION_CALLABLE_NAME = "OperationCallable";
// private static final String OPERATIONS_STUB_MEMBER_NAME = "operationsStub";
private static final String PAGED_CALLABLE_NAME = "PagedCallable";
protected static final String PAGED_CALLABLE_NAME = "PagedCallable";

protected static final TypeStore FIXED_TYPESTORE = createStaticTypes();

Expand Down Expand Up @@ -134,6 +133,10 @@ private static TypeStore createStaticTypes() {

@Override
public GapicClass generate(GapicContext context, Service service) {
if (!service.hasAnyEnabledMethodsForTransport(getTransportContext().transport())) {
return GapicClass.createNonGeneratedGapicClass();
}

String pakkage = service.pakkage() + ".stub";
TypeStore typeStore = createDynamicTypes(service, pakkage);
String className = getTransportContext().classNames().getTransportServiceStubClassName(service);
Expand Down Expand Up @@ -225,10 +228,6 @@ public GapicClass generate(GapicContext context, Service service) {
return GapicClass.create(kind, classDef);
}

protected Transport getTransport() {
return Transport.GRPC;
}

protected abstract Statement createMethodDescriptorVariableDecl(
Service service,
Method protoMethod,
Expand Down Expand Up @@ -314,7 +313,7 @@ protected List<Statement> createMethodDescriptorVariableDecls(
Map<String, Message> messageTypes,
boolean restNumericEnumsEnabled) {
return service.methods().stream()
.filter(x -> x.isSupportedByTransport(getTransport()))
.filter(x -> x.isSupportedByTransport(getTransportContext().transport()))
.map(
m ->
createMethodDescriptorVariableDecl(
Expand Down Expand Up @@ -343,7 +342,7 @@ private static List<Statement> createClassMemberFieldDeclarations(
protected Map<String, VariableExpr> createProtoMethodNameToDescriptorClassMembers(
Service service, Class<?> descriptorClass) {
return service.methods().stream()
.filter(x -> x.isSupportedByTransport(getTransport()))
.filter(x -> x.isSupportedByTransport(getTransportContext().transport()))
.collect(
Collectors.toMap(
Method::name,
Expand Down Expand Up @@ -375,7 +374,7 @@ private Map<String, VariableExpr> createCallableClassMembers(
Map<String, VariableExpr> callableClassMembers = new LinkedHashMap<>();
// Using a for-loop because the output cardinality is not a 1:1 mapping to the input set.
for (Method protoMethod : service.methods()) {
if (!protoMethod.isSupportedByTransport(getTransport())) {
if (!protoMethod.isSupportedByTransport(getTransportContext().transport())) {
continue;
}
String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(protoMethod.name());
Expand Down Expand Up @@ -664,7 +663,7 @@ protected List<MethodDefinition> createConstructorMethods(
// Transport settings local variables.
Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs =
service.methods().stream()
.filter(x -> x.isSupportedByTransport(getTransport()))
.filter(x -> x.isSupportedByTransport(getTransportContext().transport()))
.collect(
Collectors.toMap(
m -> JavaStyle.toLowerCamelCase(m.name()),
Expand All @@ -688,7 +687,7 @@ protected List<MethodDefinition> createConstructorMethods(

secondCtorExprs.addAll(
service.methods().stream()
.filter(x -> x.isSupportedByTransport(getTransport()))
.filter(x -> x.isSupportedByTransport(getTransportContext().transport()))
.map(
m ->
createTransportSettingsInitExpr(
Expand Down Expand Up @@ -1059,7 +1058,7 @@ private List<MethodDefinition> createStubOverrideMethods(

private boolean checkOperationPollingMethod(Service service) {
return service.methods().stream()
.filter(x -> x.isSupportedByTransport(getTransport()))
.filter(x -> x.isSupportedByTransport(getTransportContext().transport()))
.anyMatch(Method::isOperationPollingMethod);
}

Expand Down Expand Up @@ -1098,7 +1097,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
typeStore.putAll(
service.pakkage(),
service.methods().stream()
.filter(x -> x.isSupportedByTransport(getTransport()))
.filter(x -> x.isSupportedByTransport(getTransportContext().transport()))
.filter(Method::isPaged)
.map(m -> String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, m.name()))
.collect(Collectors.toList()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
import com.google.api.generator.gapic.composer.store.TypeStore;
import com.google.api.generator.gapic.model.GapicClass;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.Method.Stream;
import com.google.api.generator.gapic.model.Service;
import java.util.Map;

Expand All @@ -46,12 +44,6 @@ protected GapicClass generate(String className, GapicContext context, Service se
service);
}

protected boolean isSupportedMethod(Method method) {
return method.httpBindings() != null
&& method.stream() != Stream.BIDI
&& method.stream() != Stream.CLIENT;
}

@Override
protected MethodDefinition createStartStaticServerMethod(
Service service,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public static HttpJsonServiceStubClassComposer instance() {
@Override
protected List<MethodDefinition> createStaticCreatorMethods(
Service service, TypeStore typeStore, String newBuilderMethod) {
// No need to check if REST Transport is enabled
// AbstractTransportServiceStubClassComposer won't generate a file if REST isn't enabled
return super.createStaticCreatorMethods(service, typeStore, "newHttpJsonBuilder");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.api.generator.gapic.model.ResourceName;
import com.google.api.generator.gapic.model.Sample;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.model.Transport;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
Expand All @@ -48,6 +49,10 @@ protected List<CommentStatement> createClassHeaderComments(
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes,
List<Sample> samples) {
if (!service.hasAnyEnabledMethodsForTransport(Transport.REST)) {
return super.createClassHeaderComments(
service, typeStore, resourceNames, messageTypes, samples);
}
TypeNode clientType = typeStore.get(ClassNames.getServiceClientClassName(service));
TypeNode settingsType = typeStore.get(ClassNames.getServiceSettingsClassName(service));
Sample classMethodSampleCode =
Expand Down
Loading