-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Allow custom HttpRules for REST LROs #1288
Merged
Merged
Changes from 7 commits
Commits
Show all changes
92 commits
Select commit
Hold shift + click to select a range
4c14e48
fix: Allow custom http bindings for LROs
lqiu96 8f5e313
fix: Update generator and GAX to support custom HTTP Bindings for ope…
lqiu96 a693085
fix: Use static Map for custom Operation REST Http Bindings
lqiu96 f4baa0a
chore: Update golden test cases
lqiu96 7bde2cf
chore: Update showcase tests
lqiu96 a3527e1
chore: Update golden ITs
lqiu96 2e1acbc
chore: Add back origin HttpJsonOperationStub.create() methods
lqiu96 5f3dfe2
fix(deps): update dependency com.google.auth:google-auth-library-bom …
renovate-bot d8761bc
doc: Update DEVELOPMENT.md for local development. (#1237)
blakeli0 d837f97
chore: Create a default mapping in OperationStub
lqiu96 dfd1e5a
chore: Do not generate custom bindings if there are none
lqiu96 0a891dc
chore: Update golden units
lqiu96 51bde57
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 64fc726
chore: Update all test cases
lqiu96 09031c8
chore: Fix format issues
lqiu96 568ad44
fix: remove constant operation binding field
lqiu96 3a72781
chore: Clean up code
lqiu96 d7d1b12
chore: Resolve sonar comments
lqiu96 9b050d3
chore: DEVELOPMENT.md formatting fix (#1289)
meltsufin 5b31d41
ci: use java-shared-dependencies in google-cloud-java repository for …
suztomo 316621c
fix(java): initialize netty-shaded at run-time and add reflection con…
mpeddada1 d8e488f
ci(showcase): disable rest_numeric_enum for showcase testing (#1284)
mpeddada1 46156e0
chore(main): release 2.15.0 (#1269)
release-please[bot] 8a3ba93
chore(main): release 2.15.1-SNAPSHOT (#1292)
release-please[bot] 1cd51fb
chore: Pin Bazel version to 5.2.0 (#1304)
blakeli0 ec8ce91
build(deps): bump cryptography from 38.0.3 to 39.0.1 in /.kokoro (#1297)
dependabot[bot] 443adec
chore: (cleanup) removing unused files (#1265)
ddixit14 c70d183
chore: removing unused Gradle files in api-common-java, gax-java, jav…
suztomo db34e1d
chore: updated gax-java contribution doc (#1334)
suztomo 5fce328
fix: use pkg_tar from rules_pkg (#1303)
parthea 5af299d
chore: Fix pre-commit. (#1294)
blakeli0 5f3e732
chore: update CONTRIBUTING.md (#1346)
JoeWang1127 78c6d89
fix(deps): update dependency io.grpc:grpc-bom to v1.53.0 (#1345)
renovate-bot 472a3ba
chore(deps): update dependency org.apache.maven.plugins:maven-deploy-…
renovate-bot ebe84cc
chore(deps): update dependency org.apache.maven.plugins:maven-surefir…
renovate-bot 6f8cbe0
chore: add rules_pkg to renovate bot ignoreDeps (#1349)
emmileaf 1ae7726
chore: fix renovate bot ignoreDeps (#1353)
emmileaf e8d86df
fix(batcher): exceptions in unaryCaller bubble up (#1166)
diegomarquezp b8afde0
fix(deps): update dependency com.google.auth:google-auth-library-bom …
renovate-bot 445769c
chore(deps): update dependency org.apache.maven.plugins:maven-javadoc…
renovate-bot 9b42094
chore(main): release 2.15.1 (#1339)
release-please[bot] 24dabca
chore: refactoring README and DEVELOPMENT.md (#1351)
suztomo dc40d5a
chore(main): release 2.15.2-SNAPSHOT (#1358)
release-please[bot] eeb5665
chore: renovate to group Protobuf artifacts (#1362)
suztomo 40e49da
chore: README.md to explain service_config.proto (#1361)
suztomo 4356783
chore: Telling owlbot to ignore these files & it's a monorepo (#1372)
ddixit14 9a636bb
fix: Change the default scope of gax from implementation to api in au…
blakeli0 1b3e292
chore: Update variable name
lqiu96 a64100f
chore: Fix format issues
lqiu96 e8fb415
fix: Use HttpRule as Value for Custom Bindings
lqiu96 09b787c
fix: Use HttpRule as Value for Custom Bindings
lqiu96 94c00fb
chore: Add comments
lqiu96 d5453b7
chore: Update tests
lqiu96 667c3cf
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 e24b42f
fix(deps): update dependency com.google.auth:google-auth-library-bom …
renovate-bot 62edb0e
doc: Update DEVELOPMENT.md for local development. (#1237)
blakeli0 c2cc0a8
chore: Add tests
lqiu96 4f9015f
chore: Cleanup files
lqiu96 c7631ba
chore: Format the files
lqiu96 674d814
chore: Add NoCredentialsProvider
lqiu96 52aaa23
chore: Fix sonar comments
lqiu96 1670957
chore: Add serviceyaml file for parsing for rest showcase tests
lqiu96 abebe3d
chore: Use service yaml file in test
lqiu96 0a70fe7
chore: Fix Echo showcase test
lqiu96 ff142e2
chore: Clean up tests
lqiu96 3f250ab
chore: Sort the map entry to get a consistent ordering for the test
lqiu96 71b1244
chore: Update showcase and integration tests
lqiu96 98c1cfb
chore: Resolve sonar comments
lqiu96 25acf97
chore: Update comments
lqiu96 f0a1692
chore: Remove a few public constructors
lqiu96 85bcf90
chore: test ci
lqiu96 166f6c4
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 7af9882
chore: Remove the cache for java 8
lqiu96 a68927e
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 0c4eec3
chore: Update from PR feedback
lqiu96 da232d2
chore: Update comments
lqiu96 1f40c11
chore: Fix sonar issue
lqiu96 88f7961
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 8f7aed0
chore: Fix comment
lqiu96 31932b1
chore: Update to have multiple additional_bindings
lqiu96 88487e3
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 4f6e39c
chore: Update grpcrest golden test to include httprule
lqiu96 8d4d54c
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 9a4f437
chore: HttpJsonOperationsStub's MethodDescriptors are not static
lqiu96 350b811
chore: Add unit tests for HttpJson Operations logic
lqiu96 bfe3753
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 1d5d967
chore: Resolve lint issues
lqiu96 fe6c4c7
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 c15e2b5
chore: Clean up test code
lqiu96 2c489ff
chore: Resolve pr comments
lqiu96 78be800
chore: Add VisibleForTesting annotation
lqiu96 f179078
Merge branch 'main' into main-fix_custom_LRO_httpbindings
lqiu96 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import com.google.api.gax.httpjson.ProtoMessageResponseParser; | |
import com.google.api.gax.httpjson.ProtoRestSerializer; | ||
import com.google.api.gax.rpc.ClientContext; | ||
import com.google.api.gax.rpc.UnaryCallable; | ||
import com.google.common.collect.ImmutableMap; | ||
import com.google.protobuf.TypeRegistry; | ||
import com.google.showcase.v1beta1.EnumRequest; | ||
import com.google.showcase.v1beta1.EnumResponse; | ||
|
@@ -350,6 +351,9 @@ public class HttpJsonComplianceStub extends ComplianceStub { | |
.build()) | ||
.build(); | ||
|
||
private static final Map<String, String> operationCustomHttpBindings = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably don't want to create unused variables if we can avoid it in the generator. |
||
ImmutableMap.<String, String>builder().build(); | ||
|
||
private final UnaryCallable<RepeatRequest, RepeatResponse> repeatDataBodyCallable; | ||
private final UnaryCallable<RepeatRequest, RepeatResponse> repeatDataBodyInfoCallable; | ||
private final UnaryCallable<RepeatRequest, RepeatResponse> repeatDataQueryCallable; | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 can add all the default
operation -> url
mapping to this map and override then if needed. Basically we need to move as much code as possible to the generator from gax, as gax is a runtime dependency and it would be much harder to make changes there later.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 can add the defaults for bunch of mixins (or for only Operation module). I just saw that it is possible to have way more mixins (https://github.com/googleapis/gapic-showcase/blob/b94ecfc51059b49770e5bdb6f0d7ea07903158e8/schema/google/showcase/v1beta1/showcase_v1beta1.yaml#L46-L85) so I didn't want to just create a default list in the gapic-generator that needed to be manually updated later on.
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 see, I think that makes sense, we could do it the other way also, construct a map with all the bindings from the yaml, and populate default for the LRO operations. Or if we are confident about the list of mixins, we could pre-populate them as well, a related question, did you get a chance to see how mixins work for location and iam? Are we going to have similar problem for them?
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.
From what I can see, I think only the Operation proto is currently being used. I would imagine if Location and IAM are used, they would be created the same way the Operations Client was in GAX: googleapis/gax-java#1456 and we would reference it via https://github.com/googleapis/google-cloud-java/blob/10bb0cb494f64b864408ede46834e1046351370c/java-speech/google-cloud-speech/src/main/java/com/google/cloud/speech/v1/SpeechClient.java#L164-L166.
I was running under the assumption that they would be added in sometime in the future, but I'm not sure about it. Plus each generated client (operation/ iam/ location) would have the initial defaults set from the proto file: https://github.com/googleapis/gapic-generator-java/blob/d1a16195937df041f65a52717ddf9dc6ceb09b4f/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java#L84