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

cleanup(generator): fewer query params already in body #14560

Merged

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Jul 24, 2024

Part of the work for #14492

When body = "*", we include the whole request as the payload. This means we do not need to include any individual fields in the request as query parameters.

Spec:
https://github.com/googleapis/googleapis/blob/a91d1a37cb86a0e49fdc21d8b4416eb1c2a083ec/google/api/http.proto#L169-L173


I removed a failing unit test which was setting a different expectation.


This change is Reviewable

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.59%. Comparing base (424ca3c) to head (8e1a747).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14560   +/-   ##
=======================================
  Coverage   93.58%   93.59%           
=======================================
  Files        2318     2318           
  Lines      206995   206974   -21     
=======================================
- Hits       193723   193708   -15     
+ Misses      13272    13266    -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbolduc dbolduc marked this pull request as ready for review July 25, 2024 02:19
@dbolduc dbolduc requested a review from a team as a code owner July 25, 2024 02:19
Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 29 files reviewed, 1 unresolved discussion (waiting on @dbolduc)


generator/internal/http_option_utils_test.cc line 608 at r1 (raw file):

}

TEST_F(HttpOptionUtilsTest, SetHttpGetQueryParametersGetPaginated) {

Instead of removing this test, let's change the rpc definition in the service.proto to have bindings that mimic an paginated rpc.

@dbolduc dbolduc force-pushed the cleanup-generator-query-params-in-body branch from 8e1a747 to 08ede7f Compare July 29, 2024 15:51
@dbolduc
Copy link
Member Author

dbolduc commented Jul 29, 2024

Instead of removing this test, let's change the rpc definition in the service.proto to have bindings that mimic an paginated rpc.

I don't totally agree, but I think I found a compromise.

I think the important thing is that we verify that the emitted generator code sets the right query parameters. There are golden unit tests for that, e.g. this one which is paginated:

EXPECT_THAT(request.GetQueryParameter("page_token"),
Contains("my_page_token"));


To support repeated fields, we are going to have to refactor the generator output. We will need to go from:

rest_internal::Get<>(..., {std::make_pair("foo", request.foo()});

to

// This code is not exact, but it demonstrates the point.
std::vector<std::pair<std::string, std::string>>> query_params;
query_params.push_back(std::make_pair("foo", request.foo()));
for (auto bar : request.bars()) {
  query_params.push_back(std::make_pair("bar", bar));
}

rest_internal::Get<>(..., std::move(query_params));

So I think verifying that the http_method_query_parameters variable utters the names of the fields (e.g. foo, bar) we want to set is a good checkpoint that should be refactor-proof.

Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 26 of 26 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dbolduc)

@dbolduc dbolduc merged commit bf26e4c into googleapis:main Jul 29, 2024
69 of 70 checks passed
@dbolduc dbolduc deleted the cleanup-generator-query-params-in-body branch July 29, 2024 16:45
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.

2 participants