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: [gapic-generator-java] align writer behavior for nested types #1709

Merged
merged 8 commits into from
Jun 5, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,15 @@ public ImmutableList<String> enclosingClassNames() {
if (!hasEnclosingClass()) {
return ImmutableList.of();
}
// The innermost type will be the last element in the list.
// Builds list in order of inner to outer.
// Return the reversed list, since the outermost type is expected to lie at index 0.
ImmutableList.Builder<String> listBuilder = new ImmutableList.Builder<>();
Class<?> currentClz = clazz();
while (currentClz.getEnclosingClass() != null) {
listBuilder.add(currentClz.getEnclosingClass().getSimpleName());
currentClz = currentClz.getEnclosingClass();
}
return listBuilder.build();
return listBuilder.build().reverse();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we achieve the same thing without this change? Seems we can try to get the last element from the list in the writer?

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 for the import writer, but the JavaWriterVisitor also relies on this ordering for the enclosing class writes.

This fix to ConcreteReference wasn't a part of the original issue investigated, but it looked like an implementation bug that perhaps didn't surface in generated code anywhere yet. I think the intended result of this enclosingClassNames call should have outer to inner ordering, given this original comment and upstream usages of this method in the generator.

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -508,9 +508,11 @@ private void handleReference(Reference reference) {
staticImports.add(reference.fullName());
} else {
if (reference.hasEnclosingClass()) {
// Only import outermost enclosing class, e.g. import com.foo.bar.Outer
addImport(
String.format(
"%s.%s", reference.pakkage(), String.join(DOT, reference.enclosingClassNames())));
"%s.%s",
reference.pakkage(), String.join(DOT, reference.enclosingClassNames().get(0))));
} else {
addImport(reference.fullName());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import com.google.api.generator.test.utils.LineFormatter;
import com.google.common.base.Function;
import com.google.common.base.Strings;
import com.google.testgapic.v1beta1.Outer;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
Expand Down Expand Up @@ -428,6 +429,27 @@ public void writeVariableExprImports_annotationsWithDescription() {
writerVisitor.write());
}

@Test
public void writeVaporReferenceImport_outermostForNestedClass() {
VaporReference nestedVaporReference =
VaporReference.builder()
.setName("Inner")
.setEnclosingClassNames(Arrays.asList("Outer", "Middle"))
.setPakkage("com.google.testgapic.v1beta1")
.build();

TypeNode.withReference(nestedVaporReference).accept(writerVisitor);
assertEquals("import com.google.testgapic.v1beta1.Outer;\n\n", writerVisitor.write());
}

@Test
public void writeConcreteReferenceImport_outermostForNestedClass() {
ConcreteReference nestedConcreteReference =
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be break into two test cases if we want to test both type of references. However, I think we only need to test one of them if the only change is in the writer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the additional change in ConcreteReference is still needed given #1709 (comment). Breaking this into separate test cases makes sense though - will update.

ConcreteReference.withClazz(Outer.Middle.Inner.class);
TypeNode.withReference(nestedConcreteReference).accept(writerVisitor);
assertEquals("import com.google.testgapic.v1beta1.Outer;\n\n", writerVisitor.write());
}

@Test
public void writeAnonymousClassExprImports() {
// [Constructing] Function<List<IOException>, MethodDefinition>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,16 @@
import com.google.api.generator.engine.ast.Variable;
import com.google.api.generator.engine.ast.VariableExpr;
import com.google.api.generator.engine.ast.WhileStatement;
import com.google.api.generator.gapic.composer.grpc.ServiceClientClassComposer;
import com.google.api.generator.gapic.model.GapicClass;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.test.framework.Assert;
import com.google.api.generator.test.protoloader.TestProtoLoader;
import com.google.api.generator.test.utils.LineFormatter;
import com.google.api.generator.test.utils.TestExprBuilder;
import com.google.common.base.Function;
import com.google.testgapic.v1beta1.Outer;
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -129,6 +136,26 @@ public void writeReferenceType_basic() {
assertEquals("FooBar", writerVisitor.write());
}

@Test
public void writeVaporReferenceType_nestedClasses() {
VaporReference nestedVaporReference =
VaporReference.builder()
.setName("Inner")
.setEnclosingClassNames(Arrays.asList("Outer", "Middle"))
.setPakkage("com.google.testgapic.v1beta1")
.build();
TypeNode.withReference(nestedVaporReference).accept(writerVisitor);
assertEquals("Outer.Middle.Inner", writerVisitor.write());
}

@Test
public void writeConcreteReferenceType_nestedClasses() {
ConcreteReference nestedConcreteReference =
ConcreteReference.withClazz(Outer.Middle.Inner.class);
TypeNode.withReference(nestedConcreteReference).accept(writerVisitor);
assertEquals("Outer.Middle.Inner", writerVisitor.write());
}

@Test
public void writeReferenceType_useFullName() {
TypeNode.withReference(
Expand Down Expand Up @@ -2796,6 +2823,17 @@ public void writePackageInfoDefinition() {
writerVisitor.write());
}

/** =============================== GOLDEN TESTS =============================== */
@Test
public void writeSGrpcServiceClientWithNestedClassImport() {
GapicContext context = TestProtoLoader.instance().parseNestedMessage();
Service nestedService = context.services().get(0);
GapicClass clazz = ServiceClientClassComposer.instance().generate(context, nestedService);

Assert.assertGoldenClass(
this.getClass(), clazz, "GrpcServiceClientWithNestedClassImport.golden");
}

/** =============================== HELPERS =============================== */
private static AssignmentExpr createAssignmentExpr(
String variableName, String value, TypeNode type) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
package com.google.types.testing;

import com.google.api.gax.core.BackgroundResource;
import com.google.api.gax.rpc.UnaryCallable;
import com.google.testgapic.v1beta1.Outer;
import com.google.types.testing.stub.NestedMessageServiceStub;
import com.google.types.testing.stub.NestedMessageServiceStubSettings;
import java.io.IOException;
import java.util.concurrent.TimeUnit;
import javax.annotation.Generated;

// AUTO-GENERATED DOCUMENTATION AND CLASS.
/**
* This class provides the ability to make remote calls to the backing service through method calls
* that map to API methods. Sample code to get started:
*
* <pre>{@code
* // This snippet has been automatically generated and should be regarded as a code template only.
* // It will require modifications to work:
* // - It may require correct/in-range values for request initialization.
* // - It may require specifying regional endpoints when creating the service client as shown in
* // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
* try (NestedMessageServiceClient nestedMessageServiceClient =
* NestedMessageServiceClient.create()) {
* Outer.Middle request = Outer.Middle.newBuilder().setX(120).build();
* Outer.Middle.Inner response = nestedMessageServiceClient.nestedMessageMethod(request);
* }
* }</pre>
*
* <p>Note: close() needs to be called on the NestedMessageServiceClient object to clean up
* resources such as threads. In the example above, try-with-resources is used, which automatically
* calls close().
*
* <p>The surface of this class includes several types of Java methods for each of the API's
* methods:
*
* <ol>
* <li>A "flattened" method. With this type of method, the fields of the request type have been
* converted into function parameters. It may be the case that not all fields are available as
* parameters, and not every API method will have a flattened method entry point.
* <li>A "request object" method. This type of method only takes one parameter, a request object,
* which must be constructed before the call. Not every API method will have a request object
* method.
* <li>A "callable" method. This type of method takes no parameters and returns an immutable API
* callable object, which can be used to initiate calls to the service.
* </ol>
*
* <p>See the individual methods for example code.
*
* <p>Many parameters require resource names to be formatted in a particular way. To assist with
* these names, this class includes a format method for each type of name, and additionally a parse
* method to extract the individual identifiers contained within names that are returned.
*
* <p>This class can be customized by passing in a custom instance of NestedMessageServiceSettings
* to create(). For example:
*
* <p>To customize credentials:
*
* <pre>{@code
* // This snippet has been automatically generated and should be regarded as a code template only.
* // It will require modifications to work:
* // - It may require correct/in-range values for request initialization.
* // - It may require specifying regional endpoints when creating the service client as shown in
* // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
* NestedMessageServiceSettings nestedMessageServiceSettings =
* NestedMessageServiceSettings.newBuilder()
* .setCredentialsProvider(FixedCredentialsProvider.create(myCredentials))
* .build();
* NestedMessageServiceClient nestedMessageServiceClient =
* NestedMessageServiceClient.create(nestedMessageServiceSettings);
* }</pre>
*
* <p>To customize the endpoint:
*
* <pre>{@code
* // This snippet has been automatically generated and should be regarded as a code template only.
* // It will require modifications to work:
* // - It may require correct/in-range values for request initialization.
* // - It may require specifying regional endpoints when creating the service client as shown in
* // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
* NestedMessageServiceSettings nestedMessageServiceSettings =
* NestedMessageServiceSettings.newBuilder().setEndpoint(myEndpoint).build();
* NestedMessageServiceClient nestedMessageServiceClient =
* NestedMessageServiceClient.create(nestedMessageServiceSettings);
* }</pre>
*
* <p>Please refer to the GitHub repository's samples for more quickstart code snippets.
*/
@Generated("by gapic-generator-java")
public class NestedMessageServiceClient implements BackgroundResource {
private final NestedMessageServiceSettings settings;
private final NestedMessageServiceStub stub;

/** Constructs an instance of NestedMessageServiceClient with default settings. */
public static final NestedMessageServiceClient create() throws IOException {
return create(NestedMessageServiceSettings.newBuilder().build());
}

/**
* Constructs an instance of NestedMessageServiceClient, using the given settings. The channels
* are created based on the settings passed in, or defaults for any settings that are not set.
*/
public static final NestedMessageServiceClient create(NestedMessageServiceSettings settings)
throws IOException {
return new NestedMessageServiceClient(settings);
}

/**
* Constructs an instance of NestedMessageServiceClient, using the given stub for making calls.
* This is for advanced usage - prefer using create(NestedMessageServiceSettings).
*/
public static final NestedMessageServiceClient create(NestedMessageServiceStub stub) {
return new NestedMessageServiceClient(stub);
}

/**
* Constructs an instance of NestedMessageServiceClient, using the given settings. This is
* protected so that it is easy to make a subclass, but otherwise, the static factory methods
* should be preferred.
*/
protected NestedMessageServiceClient(NestedMessageServiceSettings settings) throws IOException {
this.settings = settings;
this.stub = ((NestedMessageServiceStubSettings) settings.getStubSettings()).createStub();
}

protected NestedMessageServiceClient(NestedMessageServiceStub stub) {
this.settings = null;
this.stub = stub;
}

public final NestedMessageServiceSettings getSettings() {
return settings;
}

public NestedMessageServiceStub getStub() {
return stub;
}

// AUTO-GENERATED DOCUMENTATION AND METHOD.
/**
* Sample code:
*
* <pre>{@code
* // This snippet has been automatically generated and should be regarded as a code template only.
* // It will require modifications to work:
* // - It may require correct/in-range values for request initialization.
* // - It may require specifying regional endpoints when creating the service client as shown in
* // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
* try (NestedMessageServiceClient nestedMessageServiceClient =
* NestedMessageServiceClient.create()) {
* Outer.Middle request = Outer.Middle.newBuilder().setX(120).build();
* Outer.Middle.Inner response = nestedMessageServiceClient.nestedMessageMethod(request);
* }
* }</pre>
*
* @param request The request object containing all of the parameters for the API call.
* @throws com.google.api.gax.rpc.ApiException if the remote call fails
*/
public final Outer.Middle.Inner nestedMessageMethod(Outer.Middle request) {
return nestedMessageMethodCallable().call(request);
}

// AUTO-GENERATED DOCUMENTATION AND METHOD.
/**
* Sample code:
*
* <pre>{@code
* // This snippet has been automatically generated and should be regarded as a code template only.
* // It will require modifications to work:
* // - It may require correct/in-range values for request initialization.
* // - It may require specifying regional endpoints when creating the service client as shown in
* // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
* try (NestedMessageServiceClient nestedMessageServiceClient =
* NestedMessageServiceClient.create()) {
* Outer.Middle request = Outer.Middle.newBuilder().setX(120).build();
* ApiFuture<Outer.Middle.Inner> future =
* nestedMessageServiceClient.nestedMessageMethodCallable().futureCall(request);
* // Do something.
* Outer.Middle.Inner response = future.get();
* }
* }</pre>
*/
public final UnaryCallable<Outer.Middle, Outer.Middle.Inner> nestedMessageMethodCallable() {
return stub.nestedMessageMethodCallable();
}

@Override
public final void close() {
stub.close();
}

@Override
public void shutdown() {
stub.shutdown();
}

@Override
public boolean isShutdown() {
return stub.isShutdown();
}

@Override
public boolean isTerminated() {
return stub.isTerminated();
}

@Override
public void shutdownNow() {
stub.shutdownNow();
}

@Override
public boolean awaitTermination(long duration, TimeUnit unit) throws InterruptedException {
return stub.awaitTermination(duration, unit);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
import com.google.showcase.v1beta1.MessagingOuterClass;
import com.google.showcase.v1beta1.TestingOuterClass;
import com.google.testdata.v1.DeprecatedServiceOuterClass;
import com.google.testgapic.v1beta1.NestedMessageProto;
import com.google.types.testing.TypesTestingProto;
import google.cloud.CommonResources;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand Down Expand Up @@ -131,6 +133,30 @@ public GapicContext parseBookshopService() {
.build();
}

public GapicContext parseNestedMessage() {
FileDescriptor fileDescriptor = TypesTestingProto.getDescriptor();
ServiceDescriptor serviceDescriptor = fileDescriptor.getServices().get(0);
assertEquals(serviceDescriptor.getName(), "NestedMessageService");
Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor);

FileDescriptor messageFileDescriptor = NestedMessageProto.getDescriptor();
messageTypes.putAll(Parser.parseMessages(messageFileDescriptor));

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

return GapicContext.builder()
.setMessages(messageTypes)
.setResourceNames(resourceNames)
.setServices(services)
.setHelperResourceNames(outputResourceNames)
.setTransport(transport)
.build();
}

public GapicContext parseShowcaseEcho() {
FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor();
ServiceDescriptor echoServiceDescriptor = echoFileDescriptor.getServices().get(0);
Expand Down
Loading