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

feat(storage): Respect custom endpoint for SignedUrl #14179

Merged
merged 16 commits into from
May 24, 2024

Conversation

bajajneha27
Copy link
Contributor

@bajajneha27 bajajneha27 commented May 9, 2024

closes #13068


This change is Reviewable

@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label May 9, 2024
@coryan
Copy link
Contributor

coryan commented May 9, 2024

I manually started the GHA builds and added you to the magic list that can start such jobs.

google/cloud/storage/client_sign_url_test.cc Outdated Show resolved Hide resolved
google/cloud/storage/client_sign_url_test.cc Outdated Show resolved Hide resolved
google/cloud/storage/client.cc Outdated Show resolved Hide resolved
google/cloud/storage/client.cc Outdated Show resolved Hide resolved
google/cloud/storage/client_sign_url_test.cc Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 10, 2024

Codecov Report

Attention: Patch coverage is 99.25926% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.94%. Comparing base (45ed21b) to head (f69266a).

Files Patch % Lines
google/cloud/storage/client.cc 90.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@coryan coryan marked this pull request as ready for review May 10, 2024 11:47
@coryan coryan requested review from a team as code owners May 10, 2024 11:47
coryan
coryan previously approved these changes May 10, 2024
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks. A lot of nitpicky comments and a few substantive comments. I tried to be explicit about which one is which.

google/cloud/storage/client.h Outdated Show resolved Hide resolved
google/cloud/storage/client.h Outdated Show resolved Hide resolved
google/cloud/storage/client.cc Outdated Show resolved Hide resolved
google/cloud/storage/internal/signed_url_requests.h Outdated Show resolved Hide resolved
google/cloud/storage/internal/signed_url_requests.h Outdated Show resolved Hide resolved
google/cloud/storage/client.h Show resolved Hide resolved
google/cloud/storage/client.h Outdated Show resolved Hide resolved
google/cloud/storage/internal/signed_url_requests.cc Outdated Show resolved Hide resolved
google/cloud/storage/internal/policy_document_request.cc Outdated Show resolved Hide resolved
google/cloud/storage/client.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

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.

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change the constructor of PolicyDocumentV4Request to consume this argument. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'm not sure which one is the right / better approach. :D

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@bajajneha27
Copy link
Contributor Author

We can make some small improvements to make the Request classes immutable (or less mutable). Your call. I am happy either way.

By this, did you mean passing the endpoint_authority to the constructor?

Comment on lines 107 to 108
PolicyDocumentV4Request()
: scheme_("https"), endpoint_authority_("storage.googleapis.com") {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider:

Suggested change
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.

Comment on lines 345 to 349
using nlohmann::json;
json j;
j["endpoint_authority"] = r.endpoint_authority();

return os << "PolicyDocumentRequest={" << std::move(j.dump()) << ","
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider:

Suggested change
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.

@bajajneha27 bajajneha27 merged commit 3ce6715 into googleapis:main May 24, 2024
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: Support custom endpoints for SignURLs
2 participants