-
Notifications
You must be signed in to change notification settings - Fork 61
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: gapic-generator-java to perform a no-op when no services are detected #2460
Changes from 62 commits
00c1eb1
d150747
a79329f
96b357b
4bc092a
0372f9e
392305a
97a38ad
204fff8
4d85490
7e06287
45c1ff5
6c8aa53
c2ca534
f5d595c
a58b4ef
0ff112e
1639b61
69fce38
799b23e
4caa419
71cc3f3
827e27e
4eccad0
bfe0d6c
9678c7b
dd140d2
3fd8bfa
1634112
3fffa13
5504d21
95696c0
e1fe64d
bf5d28f
f9bbc7f
31aa98d
0c43cb5
3e4a2d6
cb5c605
7315c5d
1931845
7df26e2
080d8e6
f5e4342
ab02afa
347b8cd
5b5f13f
64e8ef3
c1e7d9e
c4dff95
20ad638
28e6168
b86dd2a
8d76c06
fd7cc3b
3ee8163
1e3578d
c5286e2
09f3722
49b139a
0e73927
dc542b3
0838d76
2abdfb5
93270e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,29 +36,29 @@ | |
import java.util.jar.JarOutputStream; | ||
|
||
public class Writer { | ||
static class GapicWriterException extends RuntimeException { | ||
public GapicWriterException(String errorMessage) { | ||
super(errorMessage); | ||
} | ||
|
||
static class GapicWriterException extends RuntimeException { | ||
public GapicWriterException(String errorMessage, Throwable cause) { | ||
super(errorMessage, cause); | ||
} | ||
} | ||
|
||
public static CodeGeneratorResponse write( | ||
public static final CodeGeneratorResponse EMPTY_RESPONSE = null; | ||
|
||
@VisibleForTesting | ||
protected static CodeGeneratorResponse write( | ||
GapicContext context, | ||
List<GapicClass> clazzes, | ||
GapicPackageInfo gapicPackageInfo, | ||
List<ReflectConfig> reflectConfigInfo, | ||
String outputFilePath) { | ||
ByteString.Output output = ByteString.newOutput(); | ||
String outputFilePath, | ||
JarOutputStream jos, | ||
ByteString.Output output) | ||
throws IOException { | ||
JavaWriterVisitor codeWriter = new JavaWriterVisitor(); | ||
JarOutputStream jos; | ||
try { | ||
jos = new JarOutputStream(output); | ||
} catch (IOException e) { | ||
throw new GapicWriterException(e.getMessage(), e); | ||
|
||
if (!context.containsServices()) { | ||
return EMPTY_RESPONSE; | ||
} | ||
|
||
for (GapicClass gapicClazz : clazzes) { | ||
|
@@ -69,15 +69,13 @@ public static CodeGeneratorResponse write( | |
writeSamples(gapicClazz, getSamplePackage(gapicClazz), classPath, jos); | ||
} | ||
|
||
// package info may come as null if we have no services, but we will exit | ||
// this function early if so. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. qq, is this also still true? I don't see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not true anymore either. I removed the comment. |
||
writeMetadataFile(context, writePackageInfo(gapicPackageInfo, codeWriter, jos), jos); | ||
writeReflectConfigFile(gapicPackageInfo.packageInfo().pakkage(), reflectConfigInfo, jos); | ||
|
||
try { | ||
jos.finish(); | ||
jos.flush(); | ||
} catch (IOException e) { | ||
throw new GapicWriterException(e.getMessage(), e); | ||
} | ||
jos.finish(); | ||
jos.flush(); | ||
|
||
CodeGeneratorResponse.Builder response = CodeGeneratorResponse.newBuilder(); | ||
response | ||
|
@@ -88,6 +86,23 @@ public static CodeGeneratorResponse write( | |
return response.build(); | ||
} | ||
|
||
public static CodeGeneratorResponse write( | ||
blakeli0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
GapicContext context, | ||
List<GapicClass> clazzes, | ||
GapicPackageInfo gapicPackageInfo, | ||
List<ReflectConfig> reflectConfigInfo, | ||
String outputFilePath) { | ||
ByteString.Output output = ByteString.newOutput(); | ||
CodeGeneratorResponse response; | ||
try (JarOutputStream jos = new JarOutputStream(output)) { | ||
response = | ||
write(context, clazzes, gapicPackageInfo, reflectConfigInfo, outputFilePath, jos, output); | ||
} catch (IOException e) { | ||
throw new GapicWriterException(e.getMessage(), e); | ||
} | ||
return response; | ||
} | ||
|
||
@VisibleForTesting | ||
static void writeReflectConfigFile( | ||
String pakkage, List<ReflectConfig> reflectConfigInfo, JarOutputStream jos) { | ||
|
@@ -167,7 +182,8 @@ private static void writeSamples( | |
} | ||
} | ||
|
||
private static String writePackageInfo( | ||
@VisibleForTesting | ||
static String writePackageInfo( | ||
GapicPackageInfo gapicPackageInfo, JavaWriterVisitor codeWriter, JarOutputStream jos) { | ||
PackageInfoDefinition packageInfo = gapicPackageInfo.packageInfo(); | ||
packageInfo.accept(codeWriter); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
|
||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertFalse; | ||
import static org.junit.Assert.assertNull; | ||
import static org.junit.Assert.assertTrue; | ||
|
||
import com.google.api.generator.engine.ast.ClassDefinition; | ||
import com.google.api.generator.engine.ast.ScopeNode; | ||
|
@@ -35,6 +37,7 @@ | |
import java.nio.file.Paths; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import org.junit.Before; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class ComposerTest { | ||
|
@@ -53,8 +56,13 @@ class ComposerTest { | |
.build(); | ||
private List<Sample> ListofSamples = Arrays.asList(new Sample[] {sample}); | ||
|
||
@Before | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you change this tag to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
public void initialSanityCheck() { | ||
assertTrue(context.containsServices()); | ||
} | ||
|
||
@Test | ||
void gapicClass_addApacheLicense() { | ||
public void gapicClass_addApacheLicense_validInput_succeeds() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
ClassDefinition classDef = | ||
ClassDefinition.builder() | ||
.setPackageString("com.google.showcase.v1beta1.stub") | ||
|
@@ -154,6 +162,16 @@ void composeSamples_parseProtoPackage() { | |
} | ||
} | ||
|
||
@Test | ||
public void testEmptyGapicContext_doesNotThrow() { | ||
Composer.composeServiceClasses(GapicContext.EMPTY); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Can this be something like assert empty list or assert null instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
|
||
@Test | ||
public void testComposePackageInfo_emptyGapicContext_returnsNull() { | ||
assertNull(Composer.composePackageInfo(GapicContext.EMPTY)); | ||
} | ||
|
||
private List<GapicClass> getTestClassListFromService(Service testService) { | ||
GapicClass testClass = | ||
GrpcServiceCallableFactoryClassComposer.instance() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ | |
import com.google.api.generator.engine.ast.TypeNode; | ||
import com.google.api.generator.engine.ast.VaporReference; | ||
import com.google.api.generator.gapic.model.Field; | ||
import com.google.api.generator.gapic.model.GapicContext; | ||
import com.google.api.generator.gapic.model.Message; | ||
import com.google.api.generator.gapic.model.Method; | ||
import com.google.api.generator.gapic.model.MethodArgument; | ||
|
@@ -44,6 +45,7 @@ | |
import com.google.protobuf.Descriptors.FileDescriptor; | ||
import com.google.protobuf.Descriptors.MethodDescriptor; | ||
import com.google.protobuf.Descriptors.ServiceDescriptor; | ||
import com.google.protobuf.compiler.PluginProtos.CodeGeneratorRequest; | ||
import com.google.showcase.v1beta1.EchoOuterClass; | ||
import com.google.showcase.v1beta1.TestingOuterClass; | ||
import com.google.testgapic.v1beta1.LockerProto; | ||
|
@@ -609,6 +611,17 @@ void parseNestedProtoTypeName() { | |
"google.ads.googleads.v3.resources.MutateJob.MutateJobMetadata")); | ||
} | ||
|
||
@Test | ||
public void testParse_noServices_returnsEmptyGapicContext() { | ||
GapicContext result = Parser.parse(CodeGeneratorRequest.newBuilder().build()); | ||
assertFalse(result.containsServices()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: to match the test method, can this be assert that the result == GapicContext.Empty? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
|
||
@Test | ||
public void testParseServiceJavaPackage_emptyRequest_noop() { | ||
assertThat(Parser.parseServiceJavaPackage(CodeGeneratorRequest.newBuilder().build())).isEmpty(); | ||
} | ||
|
||
@Test | ||
void parseServiceApiVersionTest() { | ||
FileDescriptor apiVersionFileDescriptor = ApiVersionTestingOuterClass.getDescriptor(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq, is this still correct? If
doesn't write anything to System.out does it generate a zip file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not correct anymore. I corrected the log entry. Thanks for the catch.