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.

Sign up for GitHub

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: Fix BetaApi annotaiton usage for REST transport and clean BetaApi for default stubs in all transports #987

Merged
merged 4 commits into from
May 9, 2022
Merged

fix: Fix BetaApi annotaiton usage for REST transport and clean BetaApi for default stubs in all transports #987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c867280
fix: Fix `BetaApi` annotaiton usage for REST transport and clean Beta…
vam-google May 4, 2022
4d642c2
address PR feedback
vam-google May 9, 2022
a886a01
address PR feedback
vam-google May 9, 2022
51f04f1
address PR feedback
vam-google May 9, 2022
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
1 change: 1 addition & 0 deletions BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ GOLDEN_UPDATING_UNIT_TESTS = [
"com.google.api.generator.gapic.composer.grpcrest.ServiceClientClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.ServiceClientTestClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.ServiceSettingsClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.ServiceStubSettingsClassComposerTest",
"com.google.api.generator.gapic.composer.resourcename.ResourceNameHelperClassComposerTest",
"com.google.api.generator.gapic.composer.rest.HttpJsonServiceCallableFactoryClassComposerTest",
"com.google.api.generator.gapic.composer.rest.HttpJsonServiceStubClassComposerTest",
Expand Down
21 changes: 21 additions & 0 deletions src/main/java/com/google/api/generator/gapic/composer/comment/SettingsCommentComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,27 @@ public class SettingsCommentComposer {
.map(c -> CommentStatement.withComment(c))
.collect(Collectors.toList());

private final CommentStatement newTransportBuilderMethodComment;
private final CommentStatement transportProviderBuilderMethodComment;

public SettingsCommentComposer(String transportPrefix) {
this.newTransportBuilderMethodComment =
toSimpleComment(String.format("Returns a new %s builder for this class.", transportPrefix));
this.transportProviderBuilderMethodComment =
toSimpleComment(
String.format(
"Returns a builder for the default %s ChannelProvider for this service.",
transportPrefix));
}

public CommentStatement getNewTransportBuilderMethodComment() {
return newTransportBuilderMethodComment;
}

public CommentStatement getTransportProviderBuilderMethodComment() {
return transportProviderBuilderMethodComment;
}

public static CommentStatement createCallSettingsGetterComment(
String javaMethodName, boolean isMethodDeprecated) {
String methodComment = String.format(CALL_SETTINGS_METHOD_DOC_PATTERN, javaMethodName);
Expand Down
2 changes: 1 addition & 1 deletion ...ogle/api/generator/gapic/composer/common/AbstractServiceCallableFactoryClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public GapicClass generate(GapicContext context, Service service) {
String pakkage = String.format("%s.stub", service.pakkage());

StubCommentComposer commentComposer =
new StubCommentComposer(getTransportContext().transportName());
new StubCommentComposer(getTransportContext().transportNames().get(0));
ClassDefinition classDef =
ClassDefinition.builder()
.setPackageString(pakkage)
Expand Down
24 changes: 0 additions & 24 deletions ...va/com/google/api/generator/gapic/composer/common/AbstractServiceClientClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,18 +340,11 @@ private static List<MethodDefinition> createStaticCreatorMethods(
.setType(typeStore.get(ClassNames.getServiceStubClassName(service)))
.setName("stub")
.build());
AnnotationNode betaAnnotation =
AnnotationNode.builder()
.setType(typeStore.get("BetaApi"))
.setDescription(
"A restructuring of stub classes is planned, so this may break in the future")
.build();
methods.add(
MethodDefinition.builder()
.setHeaderCommentStatements(
ServiceClientCommentComposer.createCreateMethodStubArgComment(
ClassNames.getServiceClientClassName(service), settingsVarExpr.type()))
.setAnnotations(Arrays.asList(betaAnnotation))
.setScope(ScopeNode.PUBLIC)
.setIsStatic(true)
.setIsFinal(true)
Expand Down Expand Up @@ -448,15 +441,8 @@ private List<MethodDefinition> createConstructorMethods(
if (hasLroClient) {
ctorAssignmentExprs.addAll(operationsClientAssignExprs);
}
AnnotationNode betaAnnotation =
AnnotationNode.builder()
.setType(typeStore.get("BetaApi"))
.setDescription(
"A restructuring of stub classes is planned, so this may break in the future")
.build();
methods.add(
MethodDefinition.constructorBuilder()
.setAnnotations(Arrays.asList(betaAnnotation))
.setScope(ScopeNode.PROTECTED)
.setReturnType(thisClassType)
.setArguments(stubVarExpr.toBuilder().setIsDecl(true).build())
Expand Down Expand Up @@ -534,12 +520,6 @@ private List<MethodDefinition> createGetterMethods(
methodNameToTypes.put(opClientMethodName, opClientTypesIt.next());
}
}
AnnotationNode betaStubAnnotation =
AnnotationNode.builder()
.setType(typeStore.get("BetaApi"))
.setDescription(
"A restructuring of stub classes is planned, so this may break in the future")
.build();

return methodNameToTypes.entrySet().stream()
.map(
Expand All @@ -554,10 +534,6 @@ private List<MethodDefinition> createGetterMethods(
ServiceClientCommentComposer.GET_OPERATIONS_CLIENT_METHOD_COMMENT);
}
return methodBuilder
.setAnnotations(
methodName.equals("getStub")
? Arrays.asList(betaStubAnnotation)
: Collections.emptyList())
.setScope(ScopeNode.PUBLIC)
.setName(methodName)
.setIsFinal(!methodName.equals("getStub"))
Expand Down
59 changes: 48 additions & 11 deletions .../com/google/api/generator/gapic/composer/common/AbstractServiceSettingsClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public abstract class AbstractServiceSettingsClassComposer implements ClassCompo

private static final String OPERATION_SETTINGS_LITERAL = "OperationSettings";
private static final String SETTINGS_LITERAL = "Settings";
private static final TypeStore FIXED_TYPESTORE = createStaticTypes();
protected static final TypeStore FIXED_TYPESTORE = createStaticTypes();

private final TransportContext transportContext;

Expand Down Expand Up @@ -185,7 +185,14 @@ private List<MethodDefinition> createClassMethods(Service service, TypeStore typ
javaMethods.addAll(createSettingsGetterMethods(service, typeStore));
javaMethods.add(createCreatorMethod(service, typeStore));
javaMethods.addAll(createDefaultGetterMethods(service, typeStore));
javaMethods.addAll(createNewBuilderMethods(service, typeStore, "newBuilder", "createDefault"));
javaMethods.addAll(
createNewBuilderMethods(
service,
typeStore,
"newBuilder",
"createDefault",
ImmutableList.of(),
SettingsCommentComposer.NEW_BUILDER_METHOD_COMMENT));
javaMethods.addAll(createBuilderHelperMethods(service, typeStore));
javaMethods.add(createConstructorMethod(service, typeStore));
return javaMethods;
Expand Down Expand Up @@ -370,13 +377,37 @@ private List<MethodDefinition> createDefaultGetterMethods(Service service, TypeS
getTransportContext().defaultTransportProviderBuilderNames().iterator();
Iterator<Class<?>> channelProviderClassesIt =
getTransportContext().instantiatingChannelProviderBuilderClasses().iterator();
while (providerBuilderNamesIt.hasNext() && channelProviderClassesIt.hasNext()) {
Iterator<String> transportNamesIt = getTransportContext().transportNames().iterator();

boolean secondaryTransportProviderBuilder = false;
while (providerBuilderNamesIt.hasNext()
&& channelProviderClassesIt.hasNext()
&& transportNamesIt.hasNext()) {
List<AnnotationNode> annotations = ImmutableList.of();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm understanding correctly, this part is saying that we don't generate BetaApi annotation for the first getter, but generate it from the second one. I understand we may only have GRPC and HttpJson for now, but it's not easy to read and looks like a hack to me. Can we add the annotation based on something more concrete? Like override this method or part of the method in the subclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an already present paradigm in the TransportContext - it has a bunch of transport-specific lists, where first element in transport is a primary one, all others are secondary. For now there are yes, just two transports, but this is a current architecture decision which I don't think are to be changed in scope of this PR. Overriding method is a valid thing but in that case TransportContext should not exist at all in that case (the difference between transports should be done via overriding methods in each transport). I never liked TransportContext as a design decision (and originally it all was done via overriding methods), but i is the current design.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just checked how TransportContext is used and I see where you are coming from. Without changing the design, I think there is still something we can do to improve it:

  1. Extract the hardcoded method names as constants, check if the method name matches HttpJson method names, and add BetaApi annotation accordingly. The current logic depends on the order of hardcoded method names which could be easily messed up with.
  2. Add some comments to explain this logic. I know it maybe all in your head and looks simple to you, but with more people starting to maintain this repo, it could benefit us tremendously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment.
For the hardcoded method names - changing that is beyond the scope of this PR. There are way too many inline string constants here and there, so changing a few here won't change much. Also to the best of my knowledge, having inline constants is idiomatic an preferred to having separate explicit constants if they remain private and are not reused. I think about it this way: we are generating code here, that code has lots of names, having htem as a bunch of constant scattered all over the place does not seem helping but instead making things harder because it adds another level of indirection when inspecting code. Another example: if "printf" style is used for generation then all of those method are also a bunch of strings. It templates is used, then those method names are again just string in templates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree we should just used the string literal if it is not reused, I was suggesting more like extracting all the method names and centralize them, then reuse them whenever we need it, and this is a good place for us to reuse it, instead of relying on the order of method names in the list. However, I think this refactoring effort can also be done in a separate PR, it does not have to block this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it is a totally valid point and would be probably a good architectural decision, but looks like it would make sense only as a separate global refactoring (which will create that centralized naming repoisitory). Before that "repository" is created, we can't do it properly in scope of this PR.


// For clients supporting multiple transports (mainly grpc+rest case) make secondary transport
// declared as @BetaApi for now.
if (secondaryTransportProviderBuilder) {
annotations =
Arrays.asList(AnnotationNode.builder().setType(FIXED_TYPESTORE.get("BetaApi")).build());
}
CommentStatement commentStatement =
SettingsCommentComposer.DEFAULT_TRANSPORT_PROVIDER_BUILDER_METHOD_COMMENT;
if (getTransportContext().transportNames().size() > 1) {
commentStatement =
new SettingsCommentComposer(transportNamesIt.next())
.getTransportProviderBuilderMethodComment();
}

javaMethods.add(
methodMakerFn.apply(
methodStarterFn.apply(
providerBuilderNamesIt.next(),
typeMakerFn.apply(channelProviderClassesIt.next())),
SettingsCommentComposer.DEFAULT_TRANSPORT_PROVIDER_BUILDER_METHOD_COMMENT));
methodStarterFn
.apply(
providerBuilderNamesIt.next(),
typeMakerFn.apply(channelProviderClassesIt.next()))
.setAnnotations(annotations),
commentStatement));
secondaryTransportProviderBuilder = true;
}

javaMethods.add(
Expand Down Expand Up @@ -408,11 +439,14 @@ protected List<MethodDefinition> createNewBuilderMethods(
Service service,
TypeStore typeStore,
String newBuilderMethodName,
String createDefaultMethodName) {
String createDefaultMethodName,
List<AnnotationNode> annotations,
CommentStatement comment) {
TypeNode builderType = typeStore.get(BUILDER_CLASS_NAME);
return ImmutableList.of(
MethodDefinition.builder()
.setHeaderCommentStatements(SettingsCommentComposer.NEW_BUILDER_METHOD_COMMENT)
.setHeaderCommentStatements(comment)
.setAnnotations(annotations)
.setScope(ScopeNode.PUBLIC)
.setIsStatic(true)
.setReturnType(builderType)
Expand Down Expand Up @@ -500,7 +534,8 @@ private List<MethodDefinition> createNestedBuilderClassMethods(
List<MethodDefinition> javaMethods = new ArrayList<>();
javaMethods.addAll(createNestedBuilderConstructorMethods(service, typeStore));
javaMethods.addAll(
createNestedBuilderCreatorMethods(service, typeStore, "newBuilder", "createDefault"));
createNestedBuilderCreatorMethods(
service, typeStore, "newBuilder", "createDefault", ImmutableList.of()));
javaMethods.add(createNestedBuilderStubSettingsBuilderMethod(service, typeStore));
javaMethods.add(createNestedBuilderApplyToAllUnaryMethod(service, typeStore));
javaMethods.addAll(createNestedBuilderSettingsGetterMethods(service, typeStore));
Expand Down Expand Up @@ -591,7 +626,8 @@ protected List<MethodDefinition> createNestedBuilderCreatorMethods(
Service service,
TypeStore typeStore,
String newBuilderMethodName,
String createDefaultMethodName) {
String createDefaultMethodName,
List<AnnotationNode> annotations) {
MethodInvocationExpr ctorArg =
MethodInvocationExpr.builder()
.setStaticReferenceType(
Expand All @@ -602,6 +638,7 @@ protected List<MethodDefinition> createNestedBuilderCreatorMethods(
TypeNode builderType = typeStore.get(BUILDER_CLASS_NAME);
return ImmutableList.of(
MethodDefinition.builder()
.setAnnotations(annotations)
.setScope(ScopeNode.PRIVATE)
.setIsStatic(true)
.setReturnType(builderType)
Expand Down
41 changes: 28 additions & 13 deletions .../google/api/generator/gapic/composer/common/AbstractServiceStubSettingsClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,12 +249,14 @@ protected List<MethodDefinition> createDefaultTransportTransportProviderBuilderM
getTransportContext().instantiatingChannelProviderBuilderClasses().iterator();
Iterator<String> builderNamesIt =
getTransportContext().defaultTransportProviderBuilderNames().iterator();
Iterator<String> transportNamesIt = getTransportContext().transportNames().iterator();

List<MethodDefinition> methods = new ArrayList<>();

while (providerClassIt.hasNext()
&& providerBuilderClassIt.hasNext()
&& builderNamesIt.hasNext()) {
&& builderNamesIt.hasNext()
&& transportNamesIt.hasNext()) {
TypeNode returnType =
TypeNode.withReference(ConcreteReference.withClazz(providerBuilderClassIt.next()));
TypeNode channelProviderType =
Expand All @@ -269,16 +271,29 @@ protected List<MethodDefinition> createDefaultTransportTransportProviderBuilderM
Expr returnExpr =
initializeTransportProviderBuilder(transportChannelProviderBuilderExpr, returnType);

List<AnnotationNode> annotations = new ArrayList<>();
if (!methods.isEmpty()) {
annotations.add(AnnotationNode.builder().setType(FIXED_TYPESTORE.get("BetaApi")).build());
}
CommentStatement commentStatement =
SettingsCommentComposer.DEFAULT_TRANSPORT_PROVIDER_BUILDER_METHOD_COMMENT;
if (getTransportContext().transportNames().size() > 1) {
commentStatement =
new SettingsCommentComposer(transportNamesIt.next())
.getTransportProviderBuilderMethodComment();
}

MethodDefinition method =
MethodDefinition.builder()
.setHeaderCommentStatements(
SettingsCommentComposer.DEFAULT_TRANSPORT_PROVIDER_BUILDER_METHOD_COMMENT)
.setHeaderCommentStatements(commentStatement)
.setAnnotations(annotations)
.setScope(ScopeNode.PUBLIC)
.setIsStatic(true)
.setReturnType(returnType)
.setName(builderNamesIt.next())
.setReturnExpr(returnExpr)
.build();

methods.add(method);
}

Expand Down Expand Up @@ -981,7 +996,13 @@ private List<MethodDefinition> createClassMethods(
createMethodSettingsGetterMethods(methodSettingsMemberVarExprs, deprecatedSettingVarNames));
javaMethods.add(createCreateStubMethod(service, typeStore));
javaMethods.addAll(createDefaultHelperAndGetterMethods(service, typeStore));
javaMethods.addAll(createNewBuilderMethods(service, typeStore, "newBuilder", "createDefault"));
javaMethods.addAll(
createNewBuilderMethods(
service,
typeStore,
"newBuilder",
"createDefault",
SettingsCommentComposer.NEW_BUILDER_METHOD_COMMENT));
javaMethods.addAll(createBuilderHelperMethods(service, typeStore));
javaMethods.add(createClassConstructor(service, methodSettingsMemberVarExprs, typeStore));
return javaMethods;
Expand Down Expand Up @@ -1086,15 +1107,8 @@ private MethodDefinition createCreateStubMethod(Service service, TypeStore typeS

// Put the method together.
TypeNode returnType = typeStore.get(ClassNames.getServiceStubClassName(service));
AnnotationNode annotation =
AnnotationNode.builder()
.setType(FIXED_TYPESTORE.get("BetaApi"))
.setDescription(
"A restructuring of stub classes is planned, so this may break in the future")
.build();

return MethodDefinition.builder()
.setAnnotations(Arrays.asList(annotation))
.setScope(ScopeNode.PUBLIC)
.setReturnType(returnType)
.setName("createStub")
Expand Down Expand Up @@ -1186,12 +1200,13 @@ protected List<MethodDefinition> createNewBuilderMethods(
Service service,
TypeStore typeStore,
String newBuilderMethodName,
String createDefaultMethodName) {
String createDefaultMethodName,
CommentStatement methodComment) {
// Create the newBuilder() method.
final TypeNode builderReturnType = typeStore.get(NESTED_BUILDER_CLASS_NAME);
return ImmutableList.of(
MethodDefinition.builder()
.setHeaderCommentStatements(SettingsCommentComposer.NEW_BUILDER_METHOD_COMMENT)
.setHeaderCommentStatements(methodComment)
.setScope(ScopeNode.PUBLIC)
.setIsStatic(true)
.setReturnType(builderReturnType)
Expand Down
2 changes: 1 addition & 1 deletion ...google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ public GapicClass generate(GapicContext context, Service service) {
messageTypes);

StubCommentComposer commentComposer =
new StubCommentComposer(getTransportContext().transportName());
new StubCommentComposer(getTransportContext().transportNames().get(0));

ClassDefinition classDef =
ClassDefinition.builder()
Expand Down
5 changes: 2 additions & 3 deletions src/main/java/com/google/api/generator/gapic/composer/common/TransportContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ public abstract class TransportContext {
// For AbstractServiceStubClassComposer
public abstract Transport transport();

@Nullable
public abstract String transportName();
public abstract List<String> transportNames();

@Nullable
public abstract Class<?> callSettingsClass();
Expand Down Expand Up @@ -98,7 +97,7 @@ public abstract static class Builder {

public abstract Builder setTransport(Transport transport);

public abstract Builder setTransportName(String value);
public abstract Builder setTransportNames(List<String> values);

public abstract Builder setCallSettingsClass(Class<?> callSettingsClass);

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public abstract class GrpcContext extends TransportContext {
GrpcContext.builder()
.setClassNames(new ClassNames("Grpc"))
.setTransport(Transport.GRPC)
.setTransportName("gRPC")
.setTransportNames(ImmutableList.of("gRPC"))
// For grpc.GrpcServiceStubClassComposer
.setCallSettingsClass(GrpcCallSettings.class)
.setStubCallableFactoryType(classToType(GrpcStubCallableFactory.class))
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/api/generator/gapic/composer/grpcrest/GrpcRestContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public abstract class GrpcRestContext extends TransportContext {
GrpcRestContext.builder()
.setClassNames(new ClassNames("Grpc", "HttpJson"))
.setTransport(Transport.GRPC_REST)
.setTransportName(null)
.setTransportNames(ImmutableList.of("gRPC", "REST"))
// For grpcrest.GrpcServiceStubClassComposer
.setCallSettingsClass(null)
.setStubCallableFactoryType(null)
Expand Down
Loading