-
Notifications
You must be signed in to change notification settings - Fork 129
feat: REST GAPIC (REGAPIC) Support for Java #3275
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3275 +/- ##
============================================
- Coverage 87.12% 87.12% -0.01%
- Complexity 6081 6105 +24
============================================
Files 494 495 +1
Lines 24066 24143 +77
Branches 2615 2627 +12
============================================
+ Hits 20967 21034 +67
- Misses 2236 2247 +11
+ Partials 863 862 -1 Continue to review full report at Codecov.
|
rules_gapic/gapic.bzl
Outdated
@@ -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) |
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.
Could we please document the valid values of transport_protocol
somewhere?
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.
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
.
rules_gapic/java/java_gapic.bzl
Outdated
@@ -175,6 +176,17 @@ def java_gapic_library( | |||
"@javax_annotation_javax_annotation_api//jar", | |||
] | |||
|
|||
if transport_protocol == "http": |
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 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?
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.
I changed transport_protocol
to transport
. The name and the possible values for the argument now match the general REGAPIC spec.
@@ -42,9 +43,11 @@ | |||
private static final String SETTINGS_TEMPLATE_FILENAME = "java/settings.snip"; | |||
private static final String STUB_SETTINGS_TEMPLATE_FILENAME = "java/stub_settings.snip"; | |||
private static final String STUB_INTERFACE_TEMPLATE_FILENAME = "java/stub_interface.snip"; | |||
private static final String GRPC_STUB_TEMPLATE_FILENAME = "java/grpc_stub.snip"; | |||
private static final String STUB_TEMPLATE_FILENAME = "java/stub.snip"; |
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.
Since only one of HTTP or gRPC will be generated, would it make sense to split the template into grpc_stub.snip
and http_stub.snip
? As a reader, I would prefer duplicate code to parsing the diffs (which is less clear without doing a deep dive).
Ditto for test.snip
.
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.
GRPC and REST stubs share too much (like 90-80% of the file is the same). Keeping them separate makes it much easier for rest and grpc stubs to drift from each other leading to troubles supporting it.
I tried to make the .ship files match the philosophical view on the two different transports implementations - they are essentially the same thing with some transport-specific tweaks. Duplicating the .snip files for each transport views them differently - two transports are two separate independent things.
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.
SGTM.
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.
+1
…m -> putQueryParam, toPathParam -> putPathParam)
@miraleung PTAL |
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.
Looking good!
(You submitted a commit while I was reviewing, so I'll do another pass after you respond)
@@ -175,6 +176,17 @@ def java_gapic_library( | |||
"@javax_annotation_javax_annotation_api//jar", | |||
] | |||
|
|||
if transport == "rest": |
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.
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.)
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.
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).
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.
+1 to backwards-compatibility.
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.
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)
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.
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).
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 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.
"@junit_junit//jar", | ||
] | ||
|
||
if transport == "rest": |
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.
See comment above.
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.
Please lets keep it simple, not trying to support in the rules something which is not supported in the generator itself.
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.
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.
@@ -206,9 +207,14 @@ def java_gapic_assembly_gradle_pkg( | |||
grpc_target_dep = ["%s" % grpc_target] | |||
|
|||
if client_deps: | |||
if transport == "rest": |
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.
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.
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.
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.
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.
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)
private static final Option TRANSPORT = | ||
Option.builder() | ||
.longOpt("transport") // Possible values: grpc, rest or grpc+rest (not supported yet). | ||
.desc("Transport used by the generated clients.") |
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.
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."
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.
Done, but keeping grpc
as default for backward compatibility.
src/main/java/com/google/api/codegen/viewmodel/StaticLangRpcStubView.java
Show resolved
Hide resolved
@@ -58,6 +62,79 @@ | |||
.build(); | |||
@end | |||
|
|||
@private httpMethodDescriptor(methodDescriptor) |
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.
nit: I'd lean towards s/http
/rest
/ but if you think this is better for consistency with existing internal code, that's fine.
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.
Yes, this is for consistency and compatibility with discogapic. I can't make big changes here untill discogapic is removed.
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.
ok
@@ -53,7 +53,7 @@ public void testGenerator() { | |||
model.getFiles().stream().map(ProtoFile::getProto).collect(Collectors.toList())) | |||
// Only the file to generate a client for (don't generate dependencies) | |||
.addFileToGenerate("multiple_services.proto") | |||
.setParameter("language=java") | |||
.setParameter("language=java,transport=grpc") |
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.
can we also test for transport=rest
?
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.
We technically do, on line 76. But seriously speaking, these tests are useless, because gapic-generator is never executed as plugin in production, so they are testing dead logic.
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.
But we are providing the functionality in the generator, even if we ourselves don't use it, so it should be tested. Line 76 tests the failing case.
All that said, it's only worth doing if it exercises additional code. I think this will test that the high-level transport selection doesn't error out, right? So it might be worth including. Can you simply copy this existing test case and change the transport (and maybe check for the presence of a REST-specific string, if that's simple).
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.
ping
@@ -86,7 +86,7 @@ service LibraryService { | |||
|
|||
// Deletes a shelf. | |||
rpc DeleteShelf(DeleteShelfRequest) returns (google.protobuf.Empty) { | |||
option (google.api.http) = { delete: "/v1/{name=bookShelves/*}" }; | |||
option (google.api.http) = { delete: "/v1/bookShelves/{name}" }; |
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.
So we're supporting both /{name=bookShelves/*}
and /bookShelves/{name}
? It looks from https://google.aip.dev/127 that only the former is that standard. What am I overlooking?
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.
Both works, even if they are not listed in AIP127, so testing both.
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.
But if it's not in the AIP, we want to NOT support it. We don't want people annotating their gRPC APIs incorrectly, so we need to fail when they do. If the unapproved pattern is something that the converter is outputting into the synthetic protos, let's fix that. At the very least, could you file a bug in the converter repo and one in this repo, and add a TODO here to restrict to the supported pattern?
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.
This would require non-trivial unnecessary conversions in the converter which I really would like to avoid (one more thing to break potentially). This works out of the box in Java and PHP and I'm pretty sure it will work in other langauges as well. If anything, I believe it is AIP description itself is incomplete, not the pattern used here is wrong. Note, that AIP does not really attempt to provide a full spec of how the template strings may look like.
Lets discuss it in person, I feel strong about not making the unnecessary conversion here (we are converting something simple to something more complicted, in a situation when it really looks unnecessary, to follow AIP which is not attempting to rigorously define how these template strings should look like).
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.
Btw, Looks like the annotation itself officially supports the plain syntax: https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L96
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.
Short version: OK, but add a note.
Long version:
In our discussion today we said that the HTTP annotation is independent of resource name formatting. It just says how to substitute the fields into an HTTP pattern, not how that pattern should look like,
I think that's wrong, though: from the AIP:
URIs must use the {foo=bar/*} syntax to represent a variable that should be populated in the request proto. When extracting a resource name, the variable must include the entire resource name, not just the ID component.
That said: it's OK for the monolith to accept this input for now. I don't think microgenerators should. I would put a TODO here noting that this is non-standard, even though we won't fix it in the monolith
(And the converter emits protos in the correct format, right?)
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.
To be clear, this isn't blocking this PR.
On the generation side, I checked and you're right, this pattern is intended to be permissible. The relevant AIP here is AIP 4231 (Could you add a reference to that in the converter design doc, please?). I'll work with Luke to make the wording on AIP 127 less misleading (to me).
@vchudnov-g @miraleung PTAL |
@@ -175,6 +176,17 @@ def java_gapic_library( | |||
"@javax_annotation_javax_annotation_api//jar", | |||
] | |||
|
|||
if transport == "rest": |
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.
+1 to backwards-compatibility.
ToolOptions.createOption( | ||
String.class, | ||
"transport", | ||
"List of transports to use ('rest' or 'grpc') separated by '+'. NOTE: For now" |
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.
Should this just be "Transport to use" since it's not a List option?
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.
But you can list more than one, separated by "+". It's a string representation of a +
-separated list.
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.
Again, change the NOTE to be accurate: for now, we only support a single transport.
@@ -42,9 +43,11 @@ | |||
private static final String SETTINGS_TEMPLATE_FILENAME = "java/settings.snip"; | |||
private static final String STUB_SETTINGS_TEMPLATE_FILENAME = "java/stub_settings.snip"; | |||
private static final String STUB_INTERFACE_TEMPLATE_FILENAME = "java/stub_interface.snip"; | |||
private static final String GRPC_STUB_TEMPLATE_FILENAME = "java/grpc_stub.snip"; | |||
private static final String STUB_TEMPLATE_FILENAME = "java/stub.snip"; |
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.
SGTM.
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.
I have a concern about AIP127; help me understand that. And then the help text in GeneratorMain
I doesn't quite match the current implementation.
@@ -175,6 +176,17 @@ def java_gapic_library( | |||
"@javax_annotation_javax_annotation_api//jar", | |||
] | |||
|
|||
if transport == "rest": |
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.
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)
"@junit_junit//jar", | ||
] | ||
|
||
if transport == "rest": |
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.
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.
@@ -206,9 +207,14 @@ def java_gapic_assembly_gradle_pkg( | |||
grpc_target_dep = ["%s" % grpc_target] | |||
|
|||
if client_deps: | |||
if transport == "rest": |
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.
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)
ToolOptions.createOption( | ||
String.class, | ||
"transport", | ||
"List of transports to use ('rest' or 'grpc') separated by '+'. NOTE: For now" |
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.
But you can list more than one, separated by "+". It's a string representation of a +
-separated list.
String transport = options.get(TRANSPORT).toLowerCase(); | ||
|
||
TransportProtocol tp; | ||
if (transport.equals("grpc")) { |
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.
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.
typeTable.saveNicknameFor("com.google.api.gax.httpjson.ApiMessageHttpRequestFormatter"); | ||
typeTable.saveNicknameFor("com.google.api.gax.httpjson.ApiMessageHttpResponseParser"); | ||
String configSchemaVersion = context.getProductConfig().getConfigSchemaVersion(); | ||
if (configSchemaVersion != null && configSchemaVersion.startsWith("1.")) { |
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.
Yeah. The comment is very helpful! (As in the other file where you also added it.) Thanks!
@@ -58,6 +62,79 @@ | |||
.build(); | |||
@end | |||
|
|||
@private httpMethodDescriptor(methodDescriptor) |
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.
ok
@@ -53,7 +53,7 @@ public void testGenerator() { | |||
model.getFiles().stream().map(ProtoFile::getProto).collect(Collectors.toList())) | |||
// Only the file to generate a client for (don't generate dependencies) | |||
.addFileToGenerate("multiple_services.proto") | |||
.setParameter("language=java") | |||
.setParameter("language=java,transport=grpc") |
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.
But we are providing the functionality in the generator, even if we ourselves don't use it, so it should be tested. Line 76 tests the failing case.
All that said, it's only worth doing if it exercises additional code. I think this will test that the high-level transport selection doesn't error out, right? So it might be worth including. Can you simply copy this existing test case and change the transport (and maybe check for the presence of a REST-specific string, if that's simple).
@@ -86,7 +86,7 @@ service LibraryService { | |||
|
|||
// Deletes a shelf. | |||
rpc DeleteShelf(DeleteShelfRequest) returns (google.protobuf.Empty) { | |||
option (google.api.http) = { delete: "/v1/{name=bookShelves/*}" }; | |||
option (google.api.http) = { delete: "/v1/bookShelves/{name}" }; |
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.
But if it's not in the AIP, we want to NOT support it. We don't want people annotating their gRPC APIs incorrectly, so we need to fail when they do. If the unapproved pattern is something that the converter is outputting into the synthetic protos, let's fix that. At the very least, could you file a bug in the converter repo and one in this repo, and add a TODO here to restrict to the supported pattern?
.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" |
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.
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]
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.
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,.
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.
Please address the doc comments and the comments I pinged you on.
(BTW: I think if you reply to my review with a review of your own, then it will show up in my dashboard. I think that's why I didn't see this earlier.)
@@ -175,6 +176,17 @@ def java_gapic_library( | |||
"@javax_annotation_javax_annotation_api//jar", | |||
] | |||
|
|||
if transport == "rest": |
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 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.
.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" |
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.
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,.
ToolOptions.createOption( | ||
String.class, | ||
"transport", | ||
"List of transports to use ('rest' or 'grpc') separated by '+'. NOTE: For now" |
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.
Again, change the NOTE to be accurate: for now, we only support a single transport.
populateMethodSelectors(namer, httpAttr.getPathSelectors())); | ||
httpMethodView.queryParamSelectors( | ||
populateMethodSelectors(namer, httpAttr.getParamSelectors())); | ||
httpMethodView.bodySelectors(populateMethodSelectors(namer, httpAttr.getBodySelectors())); |
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.
ping
@@ -53,7 +53,7 @@ public void testGenerator() { | |||
model.getFiles().stream().map(ProtoFile::getProto).collect(Collectors.toList())) | |||
// Only the file to generate a client for (don't generate dependencies) | |||
.addFileToGenerate("multiple_services.proto") | |||
.setParameter("language=java") | |||
.setParameter("language=java,transport=grpc") |
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.
ping
@@ -86,7 +86,7 @@ service LibraryService { | |||
|
|||
// Deletes a shelf. | |||
rpc DeleteShelf(DeleteShelfRequest) returns (google.protobuf.Empty) { | |||
option (google.api.http) = { delete: "/v1/{name=bookShelves/*}" }; | |||
option (google.api.http) = { delete: "/v1/bookShelves/{name}" }; |
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.
Short version: OK, but add a note.
Long version:
In our discussion today we said that the HTTP annotation is independent of resource name formatting. It just says how to substitute the fields into an HTTP pattern, not how that pattern should look like,
I think that's wrong, though: from the AIP:
URIs must use the {foo=bar/*} syntax to represent a variable that should be populated in the request proto. When extracting a resource name, the variable must include the entire resource name, not just the ID component.
That said: it's OK for the monolith to accept this input for now. I don't think microgenerators should. I would put a TODO here noting that this is non-standard, even though we won't fix it in the monolith
(And the converter emits protos in the correct format, right?)
This PR depends on googleapis/gax-java#1177 and provides MVP for generating rest gapic clients. It was tested on real GCE API and was able to produce a valid client.