fix(storage): consistent project id overrides #11754
Conversation
Use the same function to determine the project id in all functions that
require one:
- If the function consumes a `project_id` argument, then use that.
- If the application provides a `OverrideDefaultProject` request option
(i.e., via some variadic argument) then use that.
- Otherwise use the value configured via the `Options{}`, including any
per-call `Options`.
- Use `GOOGLE_CLOUD_PROJECT` to initialize the `Options{}`, if present.
- Return an error if no value can be found for the project id.
product-auto-label
bot
added
the
api: storage
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #11754 +/- ##
==========================================
+ Coverage 93.77% 93.79% +0.01%
==========================================
Files 1831 1834 +3
Lines 165133 165800 +667
==========================================
+ Hits 154852 155510 +658
- Misses 10281 10290 +9
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @coryan)
google/cloud/storage/client.h line 358 at r1 (raw file):
auto opts = SpanOptions(std::forward<Options>(options)...); auto project_id = storage_internal::RequestProjectId( GCP_ERROR_INFO(), opts, std::forward<Options>(options)...);
are we allowed to std::forward<>(options) multiple times like this?
I think it is ok because we only std::move() the google::cloud::Optionstypes in SpanOptions(...). And we ignore their values in RequestProjectId(...)
google/cloud/storage/client_bucket_test.cc line 74 at r1 (raw file):
EXPECT_THAT(list, ElementsAre(StatusIs(StatusCode::kInvalidArgument))); // Ensure the errors are coming from the right source. EXPECT_NE(PermanentError().code(), StatusCode::kInvalidArgument);
I think it would be clearer to just check:
EXPECT_THAT(list, ElementsAre(StatusIs(StatusCode::kInvalidArgument,
HasSubstr("missing project id"))));
google/cloud/storage/client_service_account_test.cc line 136 at r1 (raw file):
auto actual = client.GetServiceAccount(Options{}
nit: reclaim vertical space
google/cloud/storage/client_service_account_test.cc line 157 at r1 (raw file):
client.GetServiceAccount(OverrideDefaultProject("override-project-id"), Options{}
ditto
google/cloud/storage/internal/generic_request.h line 119 at r1 (raw file):
/** * Specialize for `OverrideDefaultProject` as a no-op class.
Ah. Good idea. I was worried about the request.set_multiple_options(...) calls.
google/cloud/storage/internal/request_project_id.h line 41 at r1 (raw file):
internal::ErrorInfoBuilder ei, storage::OverrideDefaultProject const& o, RequestOptions&&... ro) { if (!o.has_value()) {
optional nit: save a line with: if (o.has_value()) return o.value();
google/cloud/storage/internal/request_project_id.h line 68 at r1 (raw file):
* `google::cloud::Options` configured in the client. Before we introduced * per-call `Options` parameters GCS had "request options" as a variadic * list of template arguments. One of request options could be of type
s/One of/One of the/
There was a problem hiding this comment.
PTAL
Reviewable status: 13 of 16 files reviewed, 5 unresolved discussions (waiting on @dbolduc)
google/cloud/storage/client.h line 358 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
are we allowed to
std::forward<>(options)multiple times like this?I think it is ok because we only
std::move()thegoogle::cloud::Optionstypes inSpanOptions(...). And we ignore their values inRequestProjectId(...)
std::forward and std::move do not change their parameter, they are just a fancy way to cast. So we are certainly allowed to use std::forward<Options>(options)... multiple times provided one does not accidentally trigger some UB in the second call (e.g. because the first call results in a move-from), which as you observe, we don't.
google/cloud/storage/client_bucket_test.cc line 74 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
I think it would be clearer to just check:
EXPECT_THAT(list, ElementsAre(StatusIs(StatusCode::kInvalidArgument, HasSubstr("missing project id"))));
Part of the goal is to make sure the subsequent tests that use PermanentError()
google/cloud/storage/client_service_account_test.cc line 157 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
ditto
Done.
google/cloud/storage/internal/generic_request.h line 119 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
Ah. Good idea. I was worried about the
request.set_multiple_options(...)calls.
Done.
google/cloud/storage/internal/request_project_id.h line 41 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
optional nit: save a line with:
if (o.has_value()) return o.value();
Done.
google/cloud/storage/internal/request_project_id.h line 68 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
s/One of/One of the/
Done.
Use the same function to determine the project id in all functions that require one:
project_idargument, then use that.OverrideDefaultProjectrequest option (i.e., via some variadic argument) then use that.Options{}, including any per-callOptions.GOOGLE_CLOUD_PROJECTto initialize theOptions{}, if present.Fixes #11742
I apologize for the large PR. Most of the changes are in large (and maybe repetitive) tests. But I hope the tests are easy to follow.
This change is