feat(storage): Respect custom endpoint for SignedUrl #14179
Conversation
product-auto-label
bot
added
the
api: storage
|
I manually started the GHA builds and added you to the magic list that can start such jobs. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14179 +/- ##
========================================
Coverage 92.94% 92.94%
========================================
Files 2048 2048
Lines 180801 180917 +116
========================================
+ Hits 168039 168154 +115
- Misses 12762 12763 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
We can make some small improvements to make the Request classes immutable (or less mutable). Your call. I am happy either way.
Regarding the changes to operator<<: we use operator<< in debugging and it would be nice to see all the information, but these functions rarely require debugging. Also your call.
google/cloud/storage/client.h
Outdated
| @@ -3084,6 +3085,7 @@ class Client { | |||
| SpanOptions(std::forward<Options>(options)...)); | |||
| internal::PolicyDocumentV4Request request(std::move(document)); | |||
| request.set_multiple_options(std::forward<Options>(options)...); | |||
| request.SetEndpointAuthority(EndpointAuthority()); | |||
There was a problem hiding this comment.
I think we should change the constructor of PolicyDocumentV4Request to consume this argument. Thoughts?
There was a problem hiding this comment.
I kept it in default constructor and provided a setter, just like scheme . I wanted to do similar in SignUrlRequest as well but I couldn't do that there because I was putting the new variable in SignUrlRequestCommon.
There was a problem hiding this comment.
But I'm not sure which one is the right / better approach. :D
There was a problem hiding this comment.
Fair. In general, immutable objects (or at least fields) are easier to reason about. I think we would prefer to create the request with (as much of) its state set correctly.
By this, did you mean passing the |
| PolicyDocumentV4Request() | ||
| : scheme_("https"), endpoint_authority_("storage.googleapis.com") {} |
There was a problem hiding this comment.
Consider:
| PolicyDocumentV4Request() | |
| : scheme_("https"), endpoint_authority_("storage.googleapis.com") {} | |
| PolicyDocumentV4Request() | |
| : PolicyDocumentV4("storage.googleapis.com") {} | |
| explicit PolicyDocumentV4Request(std::string endpoint_authority) | |
| : scheme_("https"), endpoint_authority_(std::move(endpoint_authority)) {} | |
| PolicyDocumentV4Request(PolicyDocumentV4 document, std::string endpoint_authority) | |
| : PolicyDocumentV4Request(std::move(endpoint_authority)), | |
| document_(std::move(document)) {} |
and you can remove the SetEndpointAuthority() function. You may need to change a few places where the constructor from PolicyDocumentV4 is called.
| using nlohmann::json; | ||
| json j; | ||
| j["endpoint_authority"] = r.endpoint_authority(); | ||
|
|
||
| return os << "PolicyDocumentRequest={" << std::move(j.dump()) << "," |
There was a problem hiding this comment.
Consider:
| using nlohmann::json; | |
| json j; | |
| j["endpoint_authority"] = r.endpoint_authority(); | |
| return os << "PolicyDocumentRequest={" << std::move(j.dump()) << "," | |
| return os << "PolicyDocumentRequest={endpoint_authority=" << r.endpoint_authority() << "," |
For future reference, you can initialize nlohmann::json objects using:
auto const j = nlohmann::json{{"endpoint_authority", r.endpoint_authority()}, {"foo", true}, {"bar", 12345}};And even create recursive objects.
closes #13068
This change is