Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

feat: REST GAPIC (REGAPIC) Support for Java #3275

Merged
merged 13 commits into from Oct 12, 2020
Merged

feat: REST GAPIC (REGAPIC) Support for Java #3275

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7666bdf
feat: Implement rest-gapic prototype
vam-google Jul 24, 2020
59c36da
fix: Fix DIREGAPIC path elements name format
vam-google Aug 8, 2020
45022ca
fix: Add transport_protocol parameter to java assembly pkg rule
vam-google Aug 9, 2020
7b32072
fix: Fix pathTemplate issue
vam-google Aug 18, 2020
e2b56c0
Merge remote-tracking branch 'upstream/master' into rest-gapic
vam-google Sep 8, 2020
0bae1fc
fix formatting
vam-google Sep 8, 2020
676b82c
Add rest transport tests
vam-google Sep 10, 2020
63d4e38
Adjust to the corresponding gax-java PR feedback changes (toQueryPara…
vam-google Sep 12, 2020
0f4eee3
Address PR feedback
vam-google Sep 16, 2020
4395976
increase test coverage
vam-google Sep 17, 2020
3157ca5
Merge remote-tracking branch 'upstream/master' into rest-gapic
vam-google Sep 22, 2020
6045612
Address PR feedback
vam-google Sep 23, 2020
9219acf
Merge remote-tracking branch 'upstream/master' into rest-gapic
vam-google Oct 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions rules_gapic/gapic.bzl
View file
Open in desktop
Expand Up @@ -40,11 +40,13 @@ def _gapic_srcjar_impl(ctx):
_set_args(attr.service_yaml, "--service_yaml=", arguments, inputs)
_set_args(attr.package_yaml2, "--package_yaml2=", arguments, inputs)
_set_args(attr.grpc_service_config, "--grpc_service_config=", arguments, inputs)
_set_args(attr.transport_protocol, "--transport_protocol=", arguments)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we please document the valid values of transport_protocol somewhere?

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 updated the code to match it the main regapic spec. Those values are documented there (and this argument was changed from transport_protocol to just transport). The values are grpc, rest, grpc+rest. This implementation does not support grpc+rest because it is not needed for GCE (the main thing we need now), and investing time end effort into supporting the proper grpc+rest in monolith is probably not worth it.

Added the list of possible values in the comment for the corresponding command line argumetn in GeneratorMain.

else:
_set_args(attr.language, "--language=", arguments)
_set_args(attr.src, "--descriptor=", arguments, inputs)
_set_args(attr.package, "--package=", arguments)
_set_args(attr.grpc_service_config, "--grpc_service_config=", arguments, inputs)
_set_args(attr.transport_protocol, "--transport_protocol=", arguments)

gapic_generator = ctx.executable.gapic_generator
ctx.actions.run(
Expand Down Expand Up @@ -72,6 +74,7 @@ gapic_srcjar = rule(
"package": attr.string(mandatory = False),
"output_suffix": attr.string(mandatory = False, default = ".srcjar"),
"grpc_service_config": attr.label(mandatory = False, allow_single_file = True),
"transport_protocol": attr.string(mandatory = False),
"gapic_generator": attr.label(
default = Label("//:gapic_generator"),
executable = True,
Expand Down
36 changes: 28 additions & 8 deletions rules_gapic/java/java_gapic.bzl
View file
Open in desktop
Expand Up @@ -78,6 +78,7 @@ def java_gapic_srcjar(
package = None,
service_yaml = None,
grpc_service_config = None,
transport_protocol = None,
**kwargs):
raw_srcjar_name = "%s_raw" % name

Expand All @@ -90,6 +91,7 @@ def java_gapic_srcjar(
language = "java",
package = package,
grpc_service_config = grpc_service_config,
transport_protocol = transport_protocol,
**kwargs
)

Expand Down Expand Up @@ -132,6 +134,7 @@ def java_gapic_library(
package = None,
gen_resource_name = True,
grpc_service_config = None,
transport_protocol = None,
deps = [],
test_deps = [],
**kwargs):
Expand All @@ -144,6 +147,7 @@ def java_gapic_library(
artifact_type = "GAPIC_CODE",
package = package,
grpc_service_config = grpc_service_config,
transport_protocol = transport_protocol,
**kwargs
)

Expand All @@ -162,10 +166,7 @@ def java_gapic_library(
"@com_google_protobuf//:protobuf_java",
"@com_google_api_api_common//jar",
"@com_google_api_gax_java//gax:gax",
"@com_google_api_gax_java//gax-grpc:gax_grpc",
"@com_google_guava_guava//jar",
"@io_grpc_grpc_java//core:core",
"@io_grpc_grpc_java//protobuf:protobuf",
"@com_google_code_findbugs_jsr305//jar",
"@org_threeten_threetenbp//jar",
"@io_opencensus_opencensus_api//jar",
Expand All @@ -175,6 +176,17 @@ def java_gapic_library(
"@javax_annotation_javax_annotation_api//jar",
]

if transport_protocol == "http":
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like either HTTP or gRPC will be generated, not both. Would it make sense to make transport_protocol a boolean flag that indicates whether it's HTTP or not?

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 changed transport_protocol to transport. The name and the possible values for the argument now match the general REGAPIC spec.

actual_deps += [
"@com_google_api_gax_java//gax-httpjson:gax_httpjson",
]
else:
actual_deps += [
"@com_google_api_gax_java//gax-grpc:gax_grpc",
"@io_grpc_grpc_java//core:core",
"@io_grpc_grpc_java//protobuf:protobuf",
]

native.java_library(
name = name,
srcs = [":%s.srcjar" % srcjar_name],
Expand All @@ -183,16 +195,24 @@ def java_gapic_library(
)

actual_test_deps = test_deps + [
"@com_google_api_gax_java//gax-grpc:gax_grpc_testlib",
"@com_google_api_gax_java//gax:gax_testlib",
"@com_google_code_gson_gson//jar",
"@io_grpc_grpc_java//auth:auth",
"@io_grpc_grpc_netty_shaded//jar",
"@io_grpc_grpc_java//stub:stub",
"@io_opencensus_opencensus_contrib_grpc_metrics//jar",
"@junit_junit//jar",
]

if transport_protocol == "http":
actual_test_deps += [
"@com_google_api_gax_java//gax-httpjson:gax_httpjson_testlib",
]
else:
actual_test_deps += [
"@com_google_api_gax_java//gax-grpc:gax_grpc_testlib",
"@io_grpc_grpc_java//auth:auth",
"@io_grpc_grpc_netty_shaded//jar",
"@io_grpc_grpc_java//stub:stub",
"@io_opencensus_opencensus_contrib_grpc_metrics//jar",
]

native.java_library(
name = "%s_test" % name,
srcs = [":%s-test.srcjar" % srcjar_name],
Expand Down
8 changes: 7 additions & 1 deletion rules_gapic/java/java_gapic_pkg.bzl
View file
Open in desktop
Expand Up @@ -160,6 +160,7 @@ def java_gapic_assembly_gradle_pkg(
name,
deps,
assembly_name = None,
transport_protocol = None,
**kwargs):
package_dir = name
if assembly_name:
Expand Down Expand Up @@ -206,9 +207,14 @@ def java_gapic_assembly_gradle_pkg(
grpc_target_dep = ["%s" % grpc_target]

if client_deps:
if transport_protocol == "http":
template_label = Label("//rules_gapic/java:resources/gradle/client_disco.gradle.tmpl")
else:
vchudnov-g marked this conversation as resolved.
Show resolved Hide resolved
template_label = Label("//rules_gapic/java:resources/gradle/client.gradle.tmpl")

_java_gapic_gradle_pkg(
name = client_target,
template_label = Label("//rules_gapic/java:resources/gradle/client.gradle.tmpl"),
template_label = template_label,
deps = proto_target_dep + client_deps,
test_deps = grpc_target_dep + client_test_deps,
**kwargs
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/com/google/api/codegen/GeneratorMain.java
View file
Open in desktop
Expand Up @@ -149,6 +149,14 @@ public class GeneratorMain {
.argName("GRPC-SERVICE-CONFIG")
.required(false)
.build();
private static final Option TRANSPORT_PROTOCOL =
Option.builder()
.longOpt("transport_protocol")
.desc("Transport used by the generated clients.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest removing the comment above and including the info in the description:
"List of transports the GAPIC should support. Valid transport names ('grpc' or 'rest') are separated by '+'. Default is 'grpc+rest'. NOTE: for now, GAPICs support only the first transport in the list."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but keeping grpc as default for backward compatibility.

.hasArg()
.argName("TRANSPORT-PROTOCOL")
.required(false)
.build();

public static void printAvailableCommands() {
System.err.println(" Available artifact types:");
Expand Down Expand Up @@ -249,6 +257,7 @@ public static void gapicGeneratorMain(ArtifactType artifactType, String[] args)
options.addOption(OUTPUT_OPTION);
options.addOption(SAMPLE_YAML_NONREQUIRED_OPTION);
options.addOption(GRPC_SERVICE_CONFIG_OPTION);
options.addOption(TRANSPORT_PROTOCOL);
Option enabledArtifactsOption =
Option.builder()
.longOpt("enabled_artifacts")
Expand Down Expand Up @@ -299,6 +308,10 @@ public static void gapicGeneratorMain(ArtifactType artifactType, String[] args)

checkFile(toolOptions.get(ToolOptions.DESCRIPTOR_SET));

if (cl.getOptionValue(TRANSPORT_PROTOCOL.getLongOpt()) != null) {
toolOptions.set(
GapicGeneratorApp.TRANSPORT_PROTOCOL, cl.getOptionValue(TRANSPORT_PROTOCOL.getLongOpt()));
}
if (cl.getOptionValues(SERVICE_YAML_NONREQUIRED_OPTION.getLongOpt()) != null) {
toolOptions.set(
ToolOptions.CONFIG_FILES,
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/api/codegen/config/GapicInterfaceContext.java
View file
Open in desktop
Expand Up @@ -246,8 +246,7 @@ public List<MethodModel> getPublicMethods() {

@Override
public boolean isSupported(MethodModel method) {
boolean supported = true;
supported &=
boolean supported =
getFeatureConfig().enableGrpcStreaming() || !MethodConfig.isGrpcStreamingMethod(method);
supported &= getMethodConfig(method).getVisibility() != VisibilityConfig.DISABLED;
return supported;
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/google/api/codegen/config/GapicProductConfig.java
View file
Open in desktop
Expand Up @@ -135,7 +135,7 @@ public GapicProductConfig withPackageName(String packageName) {
@Nullable
public static GapicProductConfig create(
Model model, ConfigProto configProto, TargetLanguage language) {
return create(model, configProto, null, null, null, language, null);
return create(model, configProto, null, null, null, language, null, TransportProtocol.GRPC);
}

/**
Expand All @@ -159,7 +159,8 @@ public static GapicProductConfig create(
@Nullable String protoPackage,
@Nullable String clientPackage,
TargetLanguage language,
@Nullable ServiceConfig grpcServiceConfig) {
@Nullable ServiceConfig grpcServiceConfig,
TransportProtocol transportProtocol) {
vchudnov-g marked this conversation as resolved.
Show resolved Hide resolved

final String defaultPackage;
SymbolTable symbolTable = model.getSymbolTable();
Expand Down Expand Up @@ -305,7 +306,7 @@ public static GapicProductConfig create(
return null;
}

TransportProtocol transportProtocol = TransportProtocol.GRPC;
// TransportProtocol transportProtocol = TransportProtocol.GRPC;
Copy link
Contributor

Choose a reason for hiding this comment

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

cruft?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed


String clientPackageName;
LanguageSettingsProto settings =
Expand Down
9 changes: 8 additions & 1 deletion src/main/java/com/google/api/codegen/gapic/GapicGeneratorApp.java
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@
import com.google.api.codegen.config.GapicProductConfig;
import com.google.api.codegen.config.PackageMetadataConfig;
import com.google.api.codegen.config.PackagingConfig;
import com.google.api.codegen.config.TransportProtocol;
import com.google.api.codegen.grpc.ServiceConfig;
import com.google.api.codegen.samplegen.v1p2.SampleConfigProto;
import com.google.api.codegen.util.MultiYamlReader;
Expand Down Expand Up @@ -112,6 +113,10 @@ public class GapicGeneratorApp extends ToolDriverBase {
"The filepath of the JSON gRPC Service Config file.",
"");

public static final Option<String> TRANSPORT_PROTOCOL =
ToolOptions.createOption(
String.class, "transport_protocol", "The generated client transport.", "grpc");

private ArtifactType artifactType;

private final GapicWriter gapicWriter;
Expand Down Expand Up @@ -208,6 +213,7 @@ protected void process() throws Exception {
}

String clientPackage = Strings.emptyToNull(options.get(CLIENT_PACKAGE));
TransportProtocol tp = TransportProtocol.valueOf(options.get(TRANSPORT_PROTOCOL).toUpperCase());

GapicProductConfig productConfig =
GapicProductConfig.create(
Expand All @@ -217,7 +223,8 @@ protected void process() throws Exception {
protoPackage,
clientPackage,
language,
gRPCServiceConfig);
gRPCServiceConfig,
tp);
if (productConfig == null) {
ToolUtil.reportDiags(model.getDiagReporter().getDiagCollector(), true);
return;
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/api/codegen/gapic/GapicGeneratorFactory.java
View file
Open in desktop
Expand Up @@ -262,7 +262,7 @@ public static List<CodeGenerator<?>> create(
new JavaSurfaceTestTransformer<>(
javaTestPathMapper,
new JavaGapicSurfaceTransformer(javaTestPathMapper),
"java/grpc_test.snip")));
"java/test.snip")));
}
}
return generators;
Expand Down