impl(generator): support protobuf wrapper types as query params #14493
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14493 +/- ##
==========================================
- Coverage 93.60% 93.59% -0.01%
==========================================
Files 2316 2316
Lines 207319 207364 +45
==========================================
+ Hits 194054 194092 +38
- Misses 13265 13272 +7 ☔ View full report in Codecov by Sentry. |
scotthart
force-pushed
the
generator_query_param_values
branch
from
9b26c95
to
452cafc
Compare
| service_file_descriptor->service(0)->method(0); | ||
| VarsDictionary vars; | ||
| SetHttpQueryParameters(ParseHttpExtension(*method), *method, vars); | ||
| EXPECT_THAT(vars.at("method_http_query_parameters"), Eq(R"""(, |
There was a problem hiding this comment.
comment: This is nearly a change detector test, but I think it's fine. I don't think saying: AllOf(Contains(...), Contains(...), ...) is any nicer.
| // Check presence for MESSAGE types as their default values may result in | ||
| // undesired behavior. | ||
| bool check_presence; |
There was a problem hiding this comment.
TIL that nested protobuf messages are inherently optional and that: "" != "foo { }"
for:
message Foo {}
message Bar {
Foo foo = 1;
}| if (!field.is_repeated() && !field.options().deprecated()) { | ||
| if (field.cpp_type() != protobuf::FieldDescriptor::CPPTYPE_MESSAGE) { | ||
| param_info = QueryParameterInfo{ | ||
| field.cpp_type(), absl::StrCat("request.", field.name(), "()"), |
There was a problem hiding this comment.
Here and below, I think we might need to look up the accessor name. (in case the field is called something like delete or namespace and the accessor is delete_()).
I'd guess there are 0 such cases in practice, so we can defer.
| @@ -104,9 +104,18 @@ DefaultJobServiceRestStub::ListJobs( | |||
| rest_internal::DetermineApiVersion("v2", options), "/", | |||
| "projects", "/", request.project_id(), "/", "jobs"), | |||
| rest_internal::TrimEmptyQueryParameters( | |||
| {std::make_pair("all_users", request.all_users() ? "1" : "0"), | |||
| {std::make_pair("all_users", (request.all_users() ? "1" : "0")), | |||
| std::make_pair("max_results", | |||
There was a problem hiding this comment.
FYI (mostly for myself): I was expecting that we would need to say max_results.value here. But from experimenting, both max_results and max_results.value work.
$ curl -X GET -H "Authorization: Bearer $(gcloud auth print-access-token)" \
https://bigquery.googleapis.com/bigquery/v2/projects/cloud-cpp-testing-resources/jobs?maxResults=1
# response with 1 result
curl -X GET -H "Authorization: Bearer $(gcloud auth print-access-token)" \
https://bigquery.googleapis.com/bigquery/v2/projects/cloud-cpp-testing-resources/jobs?maxResults.value=1
# response with 1 resultStrange... 🤷
There was a problem hiding this comment.
Reviewable status: 0 of 105 files reviewed, 3 unresolved discussions (waiting on @dbolduc)
generator/internal/http_option_utils.cc line 227 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
Here and below, I think we might need to look up the accessor name. (in case the field is called something like
deleteornamespaceand the accessor isdelete_()).I'd guess there are 0 such cases in practice, so we can defer.
created #14505
generator/internal/http_option_utils_test.cc line 661 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
comment: This is nearly a change detector test, but I think it's fine. I don't think saying:
AllOf(Contains(...), Contains(...), ...)is any nicer.
Ack
google/cloud/bigquerycontrol/v2/internal/job_rest_stub.cc line 108 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
FYI (mostly for myself): I was expecting that we would need to say
max_results.valuehere. But from experimenting, bothmax_resultsandmax_results.valuework.$ curl -X GET -H "Authorization: Bearer $(gcloud auth print-access-token)" \ https://bigquery.googleapis.com/bigquery/v2/projects/cloud-cpp-testing-resources/jobs?maxResults=1 # response with 1 result curl -X GET -H "Authorization: Bearer $(gcloud auth print-access-token)" \ https://bigquery.googleapis.com/bigquery/v2/projects/cloud-cpp-testing-resources/jobs?maxResults.value=1 # response with 1 resultStrange... 🤷
It may be a compatibility allowance on the part of the service, but I'm purely speculating.
fixes #14491
This change is