Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix(lro): Add Operation name to headers in {Get,List}Operation requests [gax-java] #1281

Merged
merged 6 commits into from Feb 4, 2021

Conversation

miraleung
Copy link
Contributor

Similar to Python's issue. See internal issue 173104871 for more details.

Some APIs require routing headers (location info in x-goog-request-params) for successful requests (see also https://google.aip.dev/client-libraries/4222). These headers are added to normal requests to the API but are missing in calls to the Operations API. This results in requests failing with permission denied errors.

@miraleung miraleung requested review from a team as code owners January 28, 2021 20:00
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 28, 2021
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

can this be tested?

@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #1281 (8ee57a9) into master (5be66cd) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1281      +/-   ##
============================================
+ Coverage     79.19%   79.34%   +0.15%     
  Complexity     1235     1235              
============================================
  Files           209      209              
  Lines          5378     5398      +20     
  Branches        454      454              
============================================
+ Hits           4259     4283      +24     
+ Misses          939      936       -3     
+ Partials        180      179       -1     
Impacted Files Coverage Δ Complexity Δ
...om/google/longrunning/stub/GrpcOperationsStub.java 91.58% <100.00%> (+1.93%) 11.00 <0.00> (ø)
...le/api/gax/grpc/GrpcUnaryRequestParamCallable.java 100.00% <0.00%> (+10.00%) 3.00% <0.00%> (ø%)
...com/google/api/gax/grpc/GrpcHeaderInterceptor.java 100.00% <0.00%> (+11.53%) 5.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5be66cd...0cf443c. Read the comment docs.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM if we can confirm that this will not break the other LRO operations which do not need routing.

@@ -149,11 +152,29 @@ protected GrpcOperationsStub(
GrpcCallSettings<GetOperationRequest, Operation> getOperationTransportSettings =
GrpcCallSettings.<GetOperationRequest, Operation>newBuilder()
.setMethodDescriptor(getOperationMethodDescriptor)
.setParamsExtractor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add it to all methods, not only to "get" ones (for completeness).

@miraleung
Copy link
Contributor Author

can this be tested?

Not without breaking the layers of abstraction between OperationsClient and GrpcOperationsStub, as well as that between the LRO client and the rest of gax-java.

@miraleung
Copy link
Contributor Author

LGTM if we can confirm that this will not break the other LRO operations which do not need routing.

Tested with CancelOperation with java-workflows. DeleteOperation and ListOperations aren't hooked up to anything client-facing (i.e. not accessible via OperationFuture), so those changes should have no noticeable effect.

@miraleung miraleung changed the title fix(lro): Add Operation name to headers in {Get,List}Operation requests fix(lro): Add Operation name to headers in {Get,List}Operation requests [gax-java] Jan 28, 2021
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM

@miraleung miraleung merged commit 721617b into master Feb 4, 2021
@miraleung miraleung deleted the fix/lro-headers branch February 4, 2021 00:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants