-
Notifications
You must be signed in to change notification settings - Fork 130
feat: Java REGAPIC Pagination implementation #3295
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3295 +/- ##
============================================
- Coverage 87.14% 87.13% -0.01%
- Complexity 6105 6115 +10
============================================
Files 495 495
Lines 24146 24159 +13
Branches 2628 2634 +6
============================================
+ Hits 21041 21051 +10
Misses 2242 2242
- Partials 863 866 +3 Continue to review full report at Codecov.
|
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.
Generally LGTM.
public boolean isDiscogapic() { | ||
return (getTransportProtocol() == TransportProtocol.HTTP | ||
&& getConfigSchemaVersion() != null | ||
&& getConfigSchemaVersion().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.
Add a brief motivating comment for 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.
Both DIREGAPIC and DISCOGAPIC have TransportProtocol
as HTTP
, but discogapic is always gapic yaml v1, while DIREGAPIC is always gapic yaml v2 or no gapic yaml at all. This is the most reliable way to distinguish between the two without introducing a new "transport protocol". Also discogapic and diregapic share significant portion of logic, that is why I'm reusing TransportProtocol.HTTP
in DIREGAPIC..
@@ -384,7 +384,11 @@ private InitCodeContext createResponseInitCodeContext( | |||
FieldModel field = config.getResourcesField(); | |||
InitCodeNode initCodeNode; | |||
if (field.isRepeated()) { | |||
initCodeNode = InitCodeNode.createSingletonList(config.getResourcesFieldName()); | |||
if (field.isMap()) { |
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.
Add a brief comment that only the first map
or repeated
(list) field is used? This may not be obvious to folks who have not read the design doc.
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
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.
(If you look at the PR in conversation view, you may not see [I don't, for sure] that I am +1 Mira's PR comment, where she suggests a code comment. If you go the file view, you'll see the whole comment thread. The GH review system has its issues....)
@@ -146,6 +146,9 @@ private TypeName getTypeNameForElementType(TypeRef type, boolean shouldBoxPrimit | |||
} | |||
switch (type.getKind()) { | |||
case TYPE_MESSAGE: | |||
if (type.isMap() && type.isMessage() && type.isRepeated()) { | |||
return getMapTypeName(type); |
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.
Do we need something similar for !type.isMap() && type.isRepeated()
?
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.
Actually if isMap() == true
then isMessage() == true
and isRepeated() == true
automatically (just checked the implementation of those method in api-compiler). Removed the other 2 conditions from the if statement as they are 100% redundant.
@@ -45,6 +45,10 @@ | |||
|
|||
public abstract boolean resourcesFieldIsMap(); | |||
|
|||
public boolean resourcesFieldAsEntrySet() { | |||
return resourceTypeName() != null && resourceTypeName().contains("<"); |
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: Did you mean
IsEntrySet
? When a reader sees "as," they may expect this method to convertresourcesField
into an entry set. - Consider adding a comment with a sample
resourceTypeName
value.
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.
Agree, no idea why I called that resourceFieldAsEntrySet
instead of resourceFieldIsEntrySet
. Renamed.
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 still says As
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.
Opps, forgot to push =) Thanks!
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 think the issues I mention below about "diregapic mode" and about generated methods need to be addressed.
Also, a question: how does the streaming stuff fit into the paging changes? Why couldn't that be a separate PR?
@@ -100,6 +100,12 @@ | |||
@Nullable | |||
public abstract String getConfigSchemaVersion(); | |||
|
|||
public boolean isDiscogapic() { |
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.
Thanks for having this function! It will make understanding the code easier.
ProtoField tokenField = methodModel.getInputField(pagingParams.getNameForPageToken()); | ||
ProtoField pageSizeField = methodModel.getInputField(pagingParams.getNameForPageSize()); | ||
if (pageSizeField == null) { | ||
if (language == TargetLanguage.JAVA && transportProtocol == TransportProtocol.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.
As per the open discussion in the design doc, let's decide whether we want the generators aware of "diregapic mode", which is essentially what you're doing here. The assumption so far has been generators would not be aware of the provenance of the protos, and that's something I still want to keep if possible (though I'm certainly open to counter-arguments).
In the meantime, in the interest of being able to generate testing clients soon, please add (just inside the outermost if
) something like TODO: Conform to design doc spec, once approved, for using non-standard paging fields,
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 generators cannot not know which client they generated. The number of places where it is needed is limited, but is still present. It is easy for me to remove this check, but I'm afraid of breaking some of the existing clients OnePlatform 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.
(qq: "where it is needed" -> what does "it" refer to? this check?)
We shouldn't have this check in the final product unless it turns out we really need it (eg breaking the current clients---which I don't think we will since the cascading rules you formulated should prevent that). When we remove the check, yes, it makes sense to regenerate existing clients to make sure there are no changes, or, if there are, that they be additive.
For now, for this MValpha, I'm fine keeping this check as long as we include a TODO flagging it prominently wherever it appears.
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.
Thanks for the TODO, but please make it a bit stronger. (I'm concerned about people copying this as a reference implementation when we may decide something else in the design doc).
How about TODO: Conform to design doc spec, once approved, for using non-standard paging fields (such as max_results for page_size)
?
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.
Added.
I noticed you almost always ask for reformulating my original comments/todos. I don't have a strong opinion about wording of these things. For those comments which you have strong opinion about please consider providing the exact wording you want it to be in the original review comment, this will let us save some review cycles.
|
||
private static final ImmutableList<String> IGNORED_PARAMETERS = | ||
ImmutableList.of(PARAMETER_PAGE_TOKEN, PARAMETER_MAX_RESULTS); | ||
ImmutableList.of(PARAMETER_PAGE_TOKEN, PARAMETER_PAGE_SIZE); |
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.
Wouldn't we want PARAMETER_MAX_RESULTS
in here as well? (I might be miunderstanding how this is used)
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 logic is used only in gapic config generation, i.e. in basically dead code not used anywhere now. We can add it there but it will not affect anything (I just did not want to touch dead code if not necessary).
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.
Oh, OK. Right, we're in configgen
. But you are changing/adding PARAMETER_PAGE_SIZE
and PARAMETER_MAX_RESULTS
, so then it's not really dead?
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 was just a queer coincidence. the java constant for the "page_size" string literal was called PARAMETER_MAX_RESULTS
. So to add "max_results" parameter and that code to keep making sense I had to rename the old constant to PARAMETER_PAGE_SIZE
first =)
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, that part I saw. But my point is that those PARAMETER_*
are being used elsewhere, so they're not dead code. But you're saying IGNORED_PARAMETERS
is dead code, yet you changed the parameter name without adding the new parameter. So it seems to me that for consistency, you would add the new parameter here, since it would also be ignored if we were generating a config file. I would either add it or add a comment saying it would be added if it mattered....
But this is totally minor and not a blocker.
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 is dead code but it may be still used in discogapic (i'm not sure if it is, but discogapic depends on gapic yaml generation). I did not want to touch that if not necessary. Discogapic is essentially dead code at this point.
@@ -146,6 +146,9 @@ private TypeName getTypeNameForElementType(TypeRef type, boolean shouldBoxPrimit | |||
} | |||
switch (type.getKind()) { | |||
case TYPE_MESSAGE: | |||
if (type.isMap() && type.isMessage() && type.isRepeated()) { |
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 structure in the previous file led me to conclude that isMap
⇒ isRepeated
. Is that true (making the last condition redundant)?
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.
Probably, but I was not sure, so made it as rigorous as possible.
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 confirmed by checking the implementation of the isMessage()
and isRepeated()
and isMap()
method that if isMap() is true than the other two are guaranteed to be true as well. Removed the other method calls since they are redundant.
src/main/java/com/google/api/codegen/transformer/java/JavaSurfaceTransformer.java
Show resolved
Hide resolved
@@ -682,6 +697,15 @@ message ListShelvesResponse { | |||
string next_page_token = 2; | |||
} | |||
|
|||
// Response message for LibraryService.ListAggregatedShelves. | |||
message ListShelvesAggregatedResponse { |
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.
For consistency with the other message pairs, could your rename this AggregatedListShelvesResponse
to match the request?
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.
Renamed it to match the other similar method/request/response:
// existing sample
rpc ListShelves(ListShelvesRequest) returns (ListShelvesResponse)
// Renamed the aggregated one to
rpc ListAggregatedShelves(ListAggregatedShelvesRequest) returns (ListAggregatedShelvesResponse)
@@ -84,6 +84,12 @@ service LibraryService { | |||
option (google.api.method_signature) = ""; | |||
} | |||
|
|||
// Lists shelves. | |||
rpc ListAggregatedShelves(AggregatedListShelvesRequest) returns (ListShelvesAggregatedResponse) { |
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 AggregatedListShelves
for similarity with GCE. Or better yet, a more descriptive name, like MapPageListShelves
(not that I like that particular one).
Whatever you choose, make sure the request and response names are consistent with this (currently they're not): rpc Foo(FooRequest) returns (FooResponse)
@@ -84,6 +84,12 @@ service LibraryService { | |||
option (google.api.method_signature) = ""; | |||
} | |||
|
|||
// Lists shelves. |
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 you add a brief comment as to why we have this since we already have ListShelves
?
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 is to have a paginated method with map<>
resource in it. ListShelves is a regular paginated method with repeated resource in 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.
Could you add that to the comment?
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 not do it. All of the comments in the proto become the generated docs in 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.
And the problem with that is what? More changes to the baseline files? Anything else?
How about if you create the comment separated from the proto by a newline? I'd be happy with that, as it would explain things for someone reading the .proto, and maybe that wouldn't change the baseline files?
// ListAggregatedShelves tests paged calls that return maps of lists paged in parallel.
// Lists shelves.
rpc ListAggregatedShelves.(...) returns (...) {
...
}
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.
Added the comment. You are right, if there is a blank line it is not recognized as the method comment.
On a side note this is not what is done i the rest of the library.proto. All the documentation/comments ther is API-specific, not gapic-generator specific.
* <pre><code> | ||
* try (LibraryClient libraryClient = LibraryClient.create()) { | ||
* AggregatedListShelvesRequest request = AggregatedListShelvesRequest.newBuilder().build(); | ||
* ApiFuture<ListShelvesAggregatedResponse> future = libraryClient.listAggregatedShelvesCallable().futureCall(request); |
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 a bit confused.
- This looks very similar to the comment for
UnaryCallable<ListShelvesRequest, ListShelvesPagedResponse>
, which is sensible, but it does not have thefor (Shelf element : future.get().iterateAll())
. Why not? - Expanding on the above: IIUC, the
AggregatedListShelvesRequest
method tests the non-standard paged response, but we want that to be transparent to the user. So just likeListShelves
causeslistShelves()
andlistShelvedPagedCallable()
to be generated in the client, I would expectAggregatedListShelves()
andAggregatedListShelvesPageable()
to be generated. We want the surface to be be what they would have if they were using standard pagination fields.
(I realize GCE does not have a previous GAPIC to compare to, and that AIP-compliant APIs won't have this issue, but we should still do this, particularly as allowing both patterns is a GAPIC feature.)
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.
Nothing in this file is expected to have new logic in it. It is gRPC-based GAPIC expected output, which is shielded from the new logic by line 247 in PageStreamingConfig (i.e. the same controversial check). The only reason why this file (and most of the other baseline files except java_library_no_gapic_config_http.baseline
) was changed is because new messages were added in the library.proto
file used for testing.
@@ -2205,6 +2241,18 @@ func (s *mockLibraryServer) ListShelves(ctx context.Context, req *librarypb.List | |||
return s.resps[0].(*librarypb.ListShelvesResponse), nil | |||
} | |||
|
|||
func (s *mockLibraryServer) ListAggregatedShelves(ctx context.Context, req *librarypb.AggregatedListShelvesRequest) (*librarypb.ListShelvesAggregatedResponse, error) { |
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 expect that (if we were still using the monolith for Go), this would have a similar structure to func (c *LibClient) ListShelves(...){...}
, dealing with NextPageToken
, etc. Is that right? Is the only reason that's not the case simply that real Go generation is happening in the µgenerator? (That's totally fine; I just want to make sure I'm not missing anything.)
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.
Line 247 in PageStreamingConfig.java explicitly asks to use the new logic for Java only. It is important, because it will start breaking other languages in monolith (most importantly PHP). I.e. only java gax supports ma-typed pagination, that is why only Java gapic generation does 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.
Ah, got it. So this is a monolith-only problem. ;-)
@vchudnov-g Streaming is not related to pagination, but the term streaming is used for whatever reason in conjunction with pagination in gapic-generator implementation (I guess pagination is considered one of the ways of "streaming" but strictly speaking it has nothing to do with grpc-based streamin, they are just similar concepts). |
@vchudnov-g Let's discuss in person the explicit DIREGAPIC mode. Removing that single check is super simple for me, but it is potentially breaking for all the existing oneplatofmr APIs this generator may be generating. |
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.
If you want to keep the DIREGAPIC check in this PR, please mark it with a prominent TODO
that this may not be consistent with the final design. We can then remove it "later" (probably in the microgenerator)
Everything else looks good. I only have minor comments. Thanks for answering my questions!
ProtoField tokenField = methodModel.getInputField(pagingParams.getNameForPageToken()); | ||
ProtoField pageSizeField = methodModel.getInputField(pagingParams.getNameForPageSize()); | ||
if (pageSizeField == null) { | ||
if (language == TargetLanguage.JAVA && transportProtocol == TransportProtocol.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.
(qq: "where it is needed" -> what does "it" refer to? this check?)
We shouldn't have this check in the final product unless it turns out we really need it (eg breaking the current clients---which I don't think we will since the cascading rules you formulated should prevent that). When we remove the check, yes, it makes sense to regenerate existing clients to make sure there are no changes, or, if there are, that they be additive.
For now, for this MValpha, I'm fine keeping this check as long as we include a TODO flagging it prominently wherever it appears.
|
||
private static final ImmutableList<String> IGNORED_PARAMETERS = | ||
ImmutableList.of(PARAMETER_PAGE_TOKEN, PARAMETER_MAX_RESULTS); | ||
ImmutableList.of(PARAMETER_PAGE_TOKEN, PARAMETER_PAGE_SIZE); |
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.
Oh, OK. Right, we're in configgen
. But you are changing/adding PARAMETER_PAGE_SIZE
and PARAMETER_MAX_RESULTS
, so then it's not really dead?
@@ -384,7 +384,11 @@ private InitCodeContext createResponseInitCodeContext( | |||
FieldModel field = config.getResourcesField(); | |||
InitCodeNode initCodeNode; | |||
if (field.isRepeated()) { | |||
initCodeNode = InitCodeNode.createSingletonList(config.getResourcesFieldName()); | |||
if (field.isMap()) { |
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
@@ -45,6 +45,10 @@ | |||
|
|||
public abstract boolean resourcesFieldIsMap(); | |||
|
|||
public boolean resourcesFieldAsEntrySet() { | |||
return resourceTypeName() != null && resourceTypeName().contains("<"); |
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 still says As
@@ -84,6 +84,12 @@ service LibraryService { | |||
option (google.api.method_signature) = ""; | |||
} | |||
|
|||
// Lists shelves. |
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 you add that to the comment?
@@ -2205,6 +2241,18 @@ func (s *mockLibraryServer) ListShelves(ctx context.Context, req *librarypb.List | |||
return s.resps[0].(*librarypb.ListShelvesResponse), nil | |||
} | |||
|
|||
func (s *mockLibraryServer) ListAggregatedShelves(ctx context.Context, req *librarypb.AggregatedListShelvesRequest) (*librarypb.ListShelvesAggregatedResponse, error) { |
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.
Ah, got it. So this is a monolith-only problem. ;-)
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.
Looks good. Just please make the one TODO a bit stronger as suggested, because people will be looking to this as a reference implementation. The other comments are minor.
ProtoField tokenField = methodModel.getInputField(pagingParams.getNameForPageToken()); | ||
ProtoField pageSizeField = methodModel.getInputField(pagingParams.getNameForPageSize()); | ||
if (pageSizeField == null) { | ||
if (language == TargetLanguage.JAVA && transportProtocol == TransportProtocol.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.
Thanks for the TODO, but please make it a bit stronger. (I'm concerned about people copying this as a reference implementation when we may decide something else in the design doc).
How about TODO: Conform to design doc spec, once approved, for using non-standard paging fields (such as max_results for page_size)
?
|
||
private static final ImmutableList<String> IGNORED_PARAMETERS = | ||
ImmutableList.of(PARAMETER_PAGE_TOKEN, PARAMETER_MAX_RESULTS); | ||
ImmutableList.of(PARAMETER_PAGE_TOKEN, PARAMETER_PAGE_SIZE); |
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, that part I saw. But my point is that those PARAMETER_*
are being used elsewhere, so they're not dead code. But you're saying IGNORED_PARAMETERS
is dead code, yet you changed the parameter name without adding the new parameter. So it seems to me that for consistency, you would add the new parameter here, since it would also be ignored if we were generating a config file. I would either add it or add a comment saying it would be added if it mattered....
But this is totally minor and not a blocker.
@@ -384,7 +384,11 @@ private InitCodeContext createResponseInitCodeContext( | |||
FieldModel field = config.getResourcesField(); | |||
InitCodeNode initCodeNode; | |||
if (field.isRepeated()) { | |||
initCodeNode = InitCodeNode.createSingletonList(config.getResourcesFieldName()); | |||
if (field.isMap()) { |
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.
(If you look at the PR in conversation view, you may not see [I don't, for sure] that I am +1 Mira's PR comment, where she suggests a code comment. If you go the file view, you'll see the whole comment thread. The GH review system has its issues....)
@@ -84,6 +84,12 @@ service LibraryService { | |||
option (google.api.method_signature) = ""; | |||
} | |||
|
|||
// Lists shelves. |
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.
And the problem with that is what? More changes to the baseline files? Anything else?
How about if you create the comment separated from the proto by a newline? I'd be happy with that, as it would explain things for someone reading the .proto, and maybe that wouldn't change the baseline files?
// ListAggregatedShelves tests paged calls that return maps of lists paged in parallel.
// Lists shelves.
rpc ListAggregatedShelves.(...) returns (...) {
...
}
@vchudnov-g PTAL |
No description provided.