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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions rules_gapic/gapic.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
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, "--transport=", arguments)
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, "--transport=", 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": 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
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ def java_gapic_srcjar(
package = None,
service_yaml = None,
grpc_service_config = None,
transport = 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 = transport,
**kwargs
)

Expand Down Expand Up @@ -132,6 +134,7 @@ def java_gapic_library(
package = None,
gen_resource_name = True,
grpc_service_config = None,
transport = 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 = transport,
**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 == "rest":
Copy link
Contributor

Choose a reason for hiding this comment

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

How about doing these additively?

all_transports=transport.split("+") if transport else ["grpc","rest"]  # [2]
# [1]
if "rest" in all_transports:
  # add rest deps
if "grpc" in all_transports:
  # add grpc deps

If you don't want to support rest+grpc for now, in #[1] you can add
all_transports = all_transports[0:1] # TODO: remove when we generate multi-transport GAPIC

(Maybe you can factor out #[1] and #[2] into a helper function, since this conditional appears another time below.)

Copy link
Contributor Author

@vam-google vam-google Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm trying to keep it backward-compatible with the older version of the rules, when no transport argument meant grpc. If the implementation of gapic generator does not support rest+grpc I don't think it makes much sense to support that in the rules. The current implementation is both backward compatible and the simplest I could come up with. Doing removal would mean that rest+grpc and grpc+rest "rest" only which is confusing and more complicated behavior.

Note, it is very unlikely that grpc+rest will ever be implemented in monolith. This is the main reason why the rest of this implementation avoids going into complications of grpc+rest support (including bazel rules or command line args parsing).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to backwards-compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

The code I suggest above is backwards-compatible (if you include the line I suggest for #[1]). It does everything your existing code does, but makes it clear that the intent is to support both protocols eventually. The line I suggest for #[1] above makes it clear that dual protocols are not supported yet (and as you say, may never be in the monolith).

I strongly urge you to consider this because it's only a hair less simple than what you have now and it makes the project intention very clear by putting in the structure that we would have in a full implementation. In my experience, this is a good principle because of that clarity and because it diminishes the technical debt should, say, the monolith be in use longer than we're planning now.

(I'm not going to block on this, but do consider it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, all this is essentially a throw away code, because the final destination of it will be in java microgenerator. Please lets not try to polish throwaway code or add features which most likely will never be fully implemented here (like the http+grpc option).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a very simple change that increases clarity, IMO. My preference would be to add it. But as I said, I'm not going to block on this.

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 == "rest":
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please lets keep it simple, not trying to support in the rules something which is not supported in the generator itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I fully agree with YAGNI, but I think the structure I propose is only a hair less simple and makes this part just work, increasing clarity.

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
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ def java_gapic_assembly_gradle_pkg(
name,
deps,
assembly_name = None,
transport = 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 == "rest":
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use a similar construct to above to get all the transports, and then be explicit about the special casing if we're not yet generating multi-transport GAPICs.

Copy link
Contributor Author

@vam-google vam-google Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This rest, grpc, rest+grpc, grpc+rest thing gets propagated in many places (like the above and the bottom comments). By doing it this way I was trying to keep it as simple as possible and not trying to implement in the rules something which is not there in the generator.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was aiming to have the interface (the rules) have the desired surface, and then provide stubs for what is not implemented (ie dual transport fails for now)

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
16 changes: 16 additions & 0 deletions src/main/java/com/google/api/codegen/GeneratorMain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,17 @@ public class GeneratorMain {
.argName("GRPC-SERVICE-CONFIG")
.required(false)
.build();
private static final Option TRANSPORT =
Option.builder()
.longOpt("transport")
.desc(
"List of transports to support. Valid transport names ('grpc' or 'rest') are"
+ " separated by '+'. Default is 'grpc'. NOTE: for now, GAPICs support only"
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment as phrased makes sense with the code changes I suggested in the last pass, where (a) the default list is grpc+rest and (b) because we're not yet implementing dual transport, we then look at only the first element in the list as per my #[1] comment: all_transports = all_transports[0:1]

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the comment if you're not handling the list option: you're not handling the "+" separated list unless you make changes like the ones I suggested,.

+ " the first transport in the list.")
.hasArg()
.argName("TRANSPORT")
.required(false)
.build();

public static void printAvailableCommands() {
System.err.println(" Available artifact types:");
Expand Down Expand Up @@ -249,6 +260,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);
Option enabledArtifactsOption =
Option.builder()
.longOpt("enabled_artifacts")
Expand Down Expand Up @@ -299,6 +311,9 @@ public static void gapicGeneratorMain(ArtifactType artifactType, String[] args)

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

if (cl.getOptionValue(TRANSPORT.getLongOpt()) != null) {
toolOptions.set(GapicGeneratorApp.TRANSPORT, cl.getOptionValue(TRANSPORT.getLongOpt()));
}
if (cl.getOptionValues(SERVICE_YAML_NONREQUIRED_OPTION.getLongOpt()) != null) {
toolOptions.set(
ToolOptions.CONFIG_FILES,
Expand Down Expand Up @@ -346,6 +361,7 @@ public static ToolOptions createCodeGeneratorOptionsFromProtoc(String[] args)
options.addOption(DESCRIPTOR_SET_OPTION);
options.addOption(LANGUAGE_OPTION);
options.addOption(TARGET_API_PROTO_PACKAGE);
options.addOption(TRANSPORT);

CommandLine cl = (new DefaultParser()).parse(options, args);

Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/api/codegen/config/GapicInterfaceContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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
9 changes: 5 additions & 4 deletions src/main/java/com/google/api/codegen/config/GapicProductConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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 @@ -150,6 +150,8 @@ public static GapicProductConfig create(
* generate clients for.
* @param clientPackage The desired package name for the generated client.
* @param language The language that this config will be used to generate a client in.
* @param grpcServiceConfig Method retries configuration.
* @param transportProtocol Transport protocol to support.
*/
@Nullable
public static GapicProductConfig create(
Expand All @@ -159,7 +161,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,8 +308,6 @@ public static GapicProductConfig create(
return null;
}

TransportProtocol transportProtocol = TransportProtocol.GRPC;

String clientPackageName;
LanguageSettingsProto settings =
configProto.getLanguageSettingsMap().get(language.toString().toLowerCase());
Expand Down
22 changes: 21 additions & 1 deletion src/main/java/com/google/api/codegen/gapic/GapicGeneratorApp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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,14 @@ public class GapicGeneratorApp extends ToolDriverBase {
"The filepath of the JSON gRPC Service Config file.",
"");

public static final Option<String> TRANSPORT =
ToolOptions.createOption(
String.class,
"transport",
"List of transports to use ('rest' or 'grpc') separated by '+'. NOTE: For now"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be "Transport to use" since it's not a List option?

Copy link
Contributor

Choose a reason for hiding this comment

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

But you can list more than one, separated by "+". It's a string representation of a +-separated list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, change the NOTE to be accurate: for now, we only support a single transport.

+ " we only support the first transport in the list.",
"grpc");

private ArtifactType artifactType;

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

String clientPackage = Strings.emptyToNull(options.get(CLIENT_PACKAGE));
String transport = options.get(TRANSPORT).toLowerCase();

TransportProtocol tp;
if (transport.equals("grpc")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a construct like I suggested for the Bazel file, taking just the first element in the transport list (with a TODO that this is not the intended ultimate behavior).

Let's also add a TODO that when we do support multiple transports, we'll have to change the code below. (If we were to do it in the monolith, what would be your approach? Having multiple productConfig or refactoring deeply so that either transport can be used from a single client? I assume from previous discussions it would be the latter....)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, lets keep it as is. It is very unlikely that we will ever support multiple transports in the monolith.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. My own preference, as I said above, is to have the right outer structure and just stubs for the unimplemented parts (ie multi-transport). It's just as simple and clearer. But I feel more strongly about it in the outermost surface (Bazel) than in the generator.

tp = TransportProtocol.GRPC;
} else if (transport.equals("rest")) {
tp = TransportProtocol.HTTP;
} else {
throw new IllegalArgumentException("Unknown transport protocol: " + transport);
}

GapicProductConfig productConfig =
GapicProductConfig.create(
Expand All @@ -217,7 +236,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
Original file line number Diff line number Diff line change
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
Loading