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.

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(generator): handle explicit routing params for nested fields #9408

Merged

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Jul 1, 2022

Part of the work for #9121

Handle fields that look like field.sub_field. I re-read the explicit routing parameters spec and they allow for these nested fields.

We copy the implementation from the HttpRule:

std::vector<std::string> chunks;
auto const* input_type = method.input_type();
for (auto const& sv : absl::StrSplit(param, '.')) {
auto const chunk = std::string(sv);
auto const* chunk_descriptor = input_type->FindFieldByName(chunk);
chunks.push_back(FieldName(chunk_descriptor));
input_type = chunk_descriptor->message_type();
}
method_vars["method_request_param_value"] =
absl::StrJoin(chunks, "().") + "()";

In order to test this, IsContextMDValid needs to learn how to generically access this field name. (It did not need to before because the HttpRule only checks that a routing key exists and matches some known pattern. It does not set its expectations by looking at the request's contents like the RoutingRule does.)

I added a new test for the nested case. And updated all explicit routing tests to both use IsContextMDValid and to verify the expected headers manually. This sorta-kinda tests IsContextMDValid.


This change is Reviewable

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 77fa685de25b45a41206ab3873ab762767e1947a

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #9408 (c739b3b) into main (d67dce6) will increase coverage by 0.00%.
The diff coverage is 98.70%.

@@           Coverage Diff           @@
##             main    #9408   +/-   ##
=======================================
  Coverage   94.60%   94.61%           
=======================================
  Files        1485     1485           
  Lines      136008   136054   +46     
=======================================
+ Hits       128669   128721   +52     
+ Misses       7339     7333    -6     
Impacted Files Coverage Δ
...egration_tests/golden/golden_kitchen_sink_client.h 100.00% <ø> (ø)
google/cloud/testing_util/validate_metadata.cc 92.56% <90.90%> (+1.07%) ⬆️
...internal/golden_kitchen_sink_metadata_decorator.cc 88.18% <100.00%> (+1.14%) ⬆️
...sts/golden_kitchen_sink_metadata_decorator_test.cc 100.00% <100.00%> (ø)
generator/internal/descriptor_utils.cc 88.22% <100.00%> (+0.13%) ⬆️
...e/cloud/pubsublite/internal/alarm_registry_impl.cc 97.05% <0.00%> (-2.95%) ⬇️
...integration_tests/schema_admin_integration_test.cc 98.88% <0.00%> (-1.12%) ⬇️
...le/cloud/internal/default_completion_queue_impl.cc 97.15% <0.00%> (-0.57%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 98.50% <0.00%> (+0.74%) ⬆️
... and 2 more

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 d67dce6...c739b3b. Read the comment docs.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: cc5f8d9c7bde315ef0f6126edf879c42fd8faa47

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: c739b3bae565a4ac74fd6e0c4cebc3cae891b1fa

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc dbolduc marked this pull request as ready for review July 1, 2022 18:11
@dbolduc dbolduc requested a review from a team as a code owner July 1, 2022 18:11
@dbolduc dbolduc merged commit 548f0f1 into googleapis:main Jul 1, 2022
@dbolduc dbolduc deleted the explicit-routing-params-nested-fields branch July 1, 2022 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants