Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Support explicit dynamic routing header #887

Merged
merged 12 commits into from
Jan 28, 2022
Merged

feat: Support explicit dynamic routing header #887

merged 12 commits into from
Jan 28, 2022

Conversation

blakeli0
Copy link
Collaborator

@blakeli0 blakeli0 commented Jan 6, 2022 •
edited
Loading

For issue: #869.
This PR is to support explicit dynamic routing header feature proposed in go/actools-dynamic-routing-proposal.
A few notes:

  • All examples provided in routing.proto are covered by golden files unit tests.
  • Added validation that the field has to be of String type as suggested(Compare to implicit routing headers support primitive types as well).
  • For nested fields, when getting field values, added null check for each level to prevent null pointer exception during runtime. Something like this.
  • Will create another PR to generate some unit tests for the client code.

@blakeli0 blakeli0 linked an issue Jan 6, 2022 that may be closed by this pull request
Add support for explicit routing headers and the google.api.routing annotation #869
Closed
@meltsufin meltsufin changed the title #869. Explicit dynamic routing header feat: explicit dynamic routing header Jan 6, 2022
@meltsufin meltsufin mentioned this pull request Jan 6, 2022
Closed
chanseokoh
chanseokoh reviewed Jan 10, 2022
View reviewed changes
Copy link
Contributor

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

Initial pass. I'll take a deeper look later.

src/main/java/com/google/api/generator/gapic/model/Message.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/model/Message.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/model/Message.java Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/model/Message.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/protoparser/RoutingRuleParser.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/protoparser/RoutingRuleParser.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
meltsufin
meltsufin reviewed Jan 11, 2022
View reviewed changes
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

This is a massive amount of work and it's looking pretty good. Let's finalize the smaller PR in gax-java and then update this one. After that, I think we'll be ready to merge this in. We can do additional tests and edge case handling in follow-up PRs.

src/main/java/com/google/api/generator/gapic/model/RoutingHeaders.java Outdated Show resolved Hide resolved
vam-google
vam-google reviewed Jan 12, 2022
View reviewed changes
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 with a few comments.

src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/model/Message.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/model/RoutingHeaders.java Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/protoparser/BUILD.bazel Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/GrpcTestingStub.golden Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
@blakeli0
#869. Add null checks for field values before using them to construct…
b824c94 
… routing header params map.

- Add test cases for all the examples provided in routing.proto. Move all testing protos for explicit routing headers to its own package and bazel rule.
- Switch to inline String format for error messages.
vam-google
vam-google reviewed Jan 18, 2022
View reviewed changes
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.

Overall looks good. Please consider adding a corresponding section in generated unit tests to cover this new feature. A lightweight version of it (like adding extra assert in existing tests) should be sufficient.

src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/GrpcTestingStub.golden Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/GrpcTestingStub.golden Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Show resolved Hide resolved
chanseokoh
chanseokoh reviewed Jan 19, 2022
View reviewed changes
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/protoparser/Parser.java Outdated Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/protoparser/RoutingRuleParser.java Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/composer/common/TestProtoLoader.java Show resolved Hide resolved
src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/model/MessageTest.java Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/model/MessageTest.java Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/model/RoutingHeadersTest.java Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/protoparser/RoutingRuleParserTest.java Outdated Show resolved Hide resolved
src/test/java/com/google/api/generator/gapic/protoparser/RoutingRuleParserTest.java Outdated Show resolved Hide resolved
@blakeli0 blakeli0 marked this pull request as ready for review January 27, 2022 02:49
@blakeli0 blakeli0 requested review from a team as code owners January 27, 2022 02:49
vam-google
vam-google approved these changes Jan 27, 2022
View reviewed changes
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

@codecov
Copy link

codecov bot commented Jan 28, 2022 •
edited
Loading

Codecov Report

Merging #887 (f29f222) into main (50a8693) will increase coverage by 0.12%.
The diff coverage is 98.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #887      +/-   ##
==========================================
+ Coverage   87.84%   87.96%   +0.12%     
==========================================
  Files         153      156       +3     
  Lines       16046    16198     +152     
  Branches     1166     1179      +13     
==========================================
+ Hits        14095    14248     +153     
- Misses       1607     1608       +1     
+ Partials      344      342       -2
Impacted Files Coverage Δ
...omposer/rest/HttpJsonServiceStubClassComposer.java 53.29% <ø> (ø)
...api/generator/gapic/protoparser/PatternParser.java 85.71% <85.71%> (ø)
...generator/gapic/protoparser/RoutingRuleParser.java 95.65% <95.65%> (ø)
...mon/AbstractTransportServiceStubClassComposer.java 93.74% <100.00%> (ø)
...ic/composer/grpc/GrpcServiceStubClassComposer.java 98.54% <100.00%> (+0.77%) ⬆️
.../com/google/api/generator/gapic/model/Message.java 77.04% <100.00%> (+11.19%) ⬆️
...a/com/google/api/generator/gapic/model/Method.java 92.85% <100.00%> (+0.54%) ⬆️
...e/api/generator/gapic/model/RoutingHeaderRule.java 100.00% <100.00%> (ø)
...pi/generator/gapic/protoparser/HttpRuleParser.java 85.29% <100.00%> (+1.96%) ⬆️
...google/api/generator/gapic/protoparser/Parser.java 45.38% <100.00%> (+0.26%) ⬆️

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 50a8693...f29f222. Read the comment docs.

@blakeli0 blakeli0 changed the title feat: explicit dynamic routing header feat: Support explicit dynamic routing header Jan 28, 2022
@blakeli0 blakeli0 merged commit bcc1bdb into main Jan 28, 2022
@blakeli0 blakeli0 deleted the explicit-dynamic-routing-header branch January 28, 2022 05:48
@release-please release-please bot mentioned this pull request Jan 28, 2022
Merged
suztomo pushed a commit that referenced this pull request Dec 16, 2022
@blakeli0
feat: Support explicit dynamic routing header (#887)
e26a2ae 
This PR is to support explicit dynamic routing header feature proposed in go/actools-dynamic-routing-proposal.

All examples provided in routing.proto are covered by golden files unit tests.
Added validation that the field has to be of String type as suggested(Compare to implicit routing headers support primitive types as well).
For nested fields, when getting field values, added null check for each level to prevent null pointer exception during runtime.
suztomo pushed a commit that referenced this pull request Mar 21, 2023
@renovate-bot
deps: update dependency com.google.code.gson:gson to v2.10 (#887)
cbe8973 
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.code.gson:gson](https://github.com/google/gson) | `2.9.1` -> `2.10` | [![age](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10/compatibility-slim/2.9.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.code.gson:gson/2.10/confidence-slim/2.9.1)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-shared-dependencies).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yNDEuMTEiLCJ1cGRhdGVkSW5WZXIiOiIzMi4yNDEuMTEifQ==-->
suztomo pushed a commit that referenced this pull request Mar 21, 2023
@release-please
chore(main): release 3.0.6 (#888)
38cd9f3 
🤖 I have created a release *beep* *boop*
---


## [3.0.6](https://github.com/googleapis/java-shared-dependencies/compare/v3.0.5...v3.0.6) (2022-11-07)


### Dependencies

* Update dependency com.fasterxml.jackson:jackson-bom to v2.14.0 ([#901](https://github.com/googleapis/java-shared-dependencies/issues/901)) ([4e3d116](https://github.com/googleapis/java-shared-dependencies/commit/4e3d1162403a236443c8dbb00cbe23bd6c6c225a))
* Update dependency com.google.api-client:google-api-client-bom to v2.0.1 ([#899](https://github.com/googleapis/java-shared-dependencies/issues/899)) ([d2baed5](https://github.com/googleapis/java-shared-dependencies/commit/d2baed57f798b7c153678ce87bd486f9808dbc46))
* Update dependency com.google.api:api-common to v2.2.2 ([#892](https://github.com/googleapis/java-shared-dependencies/issues/892)) ([292cd39](https://github.com/googleapis/java-shared-dependencies/commit/292cd39d3b5fca9be15621b5e483e3b6386ec2ef))
* Update dependency com.google.cloud:grpc-gcp to v1.3.1 ([#884](https://github.com/googleapis/java-shared-dependencies/issues/884)) ([f22fce6](https://github.com/googleapis/java-shared-dependencies/commit/f22fce69481f0ecec1c5438b9f1a938db074cf4c))
* Update dependency com.google.code.gson:gson to v2.10 ([#887](https://github.com/googleapis/java-shared-dependencies/issues/887)) ([cbe8973](https://github.com/googleapis/java-shared-dependencies/commit/cbe8973436da3c34be00d0742b3db11af106f585))
* Update dependency com.google.http-client:google-http-client-bom to v1.42.3 ([#893](https://github.com/googleapis/java-shared-dependencies/issues/893)) ([21e7515](https://github.com/googleapis/java-shared-dependencies/commit/21e7515d351cf8a53cb7590267231930bedfaf88))
* Update dependency com.google.protobuf:protobuf-bom to v3.21.9 ([#889](https://github.com/googleapis/java-shared-dependencies/issues/889)) ([30effe6](https://github.com/googleapis/java-shared-dependencies/commit/30effe65dc103a694fab5bf9537e96a0def0d4d9))
* Update dependency io.grpc:grpc-bom to v1.50.2 ([#878](https://github.com/googleapis/java-shared-dependencies/issues/878)) ([0e155c4](https://github.com/googleapis/java-shared-dependencies/commit/0e155c476ee8280921d234286eed8732997dd2a6))
* Update dependency org.checkerframework:checker-qual to v3.27.0 ([#896](https://github.com/googleapis/java-shared-dependencies/issues/896)) ([f6c1155](https://github.com/googleapis/java-shared-dependencies/commit/f6c1155bbd0a01b6a25948f7c6117a50fd85e9de))
* Update dependency org.threeten:threetenbp to v1.6.4 ([#894](https://github.com/googleapis/java-shared-dependencies/issues/894)) ([478ae53](https://github.com/googleapis/java-shared-dependencies/commit/478ae530c8140d92f4e083c5e06ecd6f4f228f05))
* Update gax.version to v2.19.5 ([#903](https://github.com/googleapis/java-shared-dependencies/issues/903)) ([ba1ae38](https://github.com/googleapis/java-shared-dependencies/commit/ba1ae389185f2fffaec10cf69ad6644389af9571))
* Update google.common-protos.version to v2.10.0 ([#900](https://github.com/googleapis/java-shared-dependencies/issues/900)) ([46aeddf](https://github.com/googleapis/java-shared-dependencies/commit/46aeddfe2ce2325ab8ae9a6654c500f847855acb))
* Update google.core.version to v2.8.23 ([#885](https://github.com/googleapis/java-shared-dependencies/issues/885)) ([1092bbe](https://github.com/googleapis/java-shared-dependencies/commit/1092bbe5f7a9d84dc1013f8a3c8e62e5c26ab2ab))
* Update google.core.version to v2.8.24 ([#890](https://github.com/googleapis/java-shared-dependencies/issues/890)) ([70791a5](https://github.com/googleapis/java-shared-dependencies/commit/70791a5ce678c5c7ebbb70e1527cab69587307ec))
* Update google.core.version to v2.8.27 ([#902](https://github.com/googleapis/java-shared-dependencies/issues/902)) ([a53f404](https://github.com/googleapis/java-shared-dependencies/commit/a53f404799ac6fd4e3005ea781f74a245fd7b7c7))
* Update iam.version to v1.6.6 ([#886](https://github.com/googleapis/java-shared-dependencies/issues/886)) ([122cf9d](https://github.com/googleapis/java-shared-dependencies/commit/122cf9d01a73d78768544b0638efc8cca995fd87))
* Update iam.version to v1.6.7 ([#895](https://github.com/googleapis/java-shared-dependencies/issues/895)) ([feda2e7](https://github.com/googleapis/java-shared-dependencies/commit/feda2e7d2d9026dffcdfe71f443199505e5bb215))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
suztomo pushed a commit that referenced this pull request Mar 21, 2023
@release-please
chore(main): release 2.8.6-SNAPSHOT (#887)
b25e36b 
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meltsufin meltsufin meltsufin approved these changes

@vam-google vam-google vam-google approved these changes

@chanseokoh chanseokoh Awaiting requested review from chanseokoh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for explicit routing headers and the google.api.routing annotation
4 participants
@blakeli0 @chanseokoh @meltsufin @vam-google