Skip to content

Conversation

@eliekozah
Copy link
Contributor

@eliekozah eliekozah commented Mar 25, 2025

This is a Draft PR for PR issue b/402481452

Please do not merge.


This change is Reviewable

@eliekozah eliekozah added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 25, 2025
@eliekozah eliekozah requested review from a team as code owners March 25, 2025 20:48
@ddelgrosso1
Copy link
Contributor

A few general comments, it looks like this is breaking at least one test in the asan-pr build. Also, looking at CLang tidy there are some other problems with copy construction there.

That said, as we discussed I'm not sure adding a new function is the way to best fix this. Perhaps this is a good topic to raise in the weekly C++ meeting tomorrow but I would think the existing functions should behave correctly.

@eliekozah eliekozah force-pushed the bugfix-headers-for-json branch from 94a750e to 61f39df Compare April 1, 2025 00:15
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@eliekozah eliekozah removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 1, 2025
@eliekozah
Copy link
Contributor Author

I deleted the previous "quick fix" and added a new class CustomHeaders in GCS.
You can now add multiple custom headers to your request.

example:

    auto response = client.ListObjects(
        "my_bucket",
        gcs::CustomHeaders({{"name1", "value1"}, {"name2", "value2"}})
    );

@eliekozah eliekozah force-pushed the bugfix-headers-for-json branch from 8982fd2 to f55ff55 Compare April 2, 2025 12:17
@codecov
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.87%. Comparing base (3498e58) to head (f765e10).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15053   +/-   ##
=======================================
  Coverage   92.86%   92.87%           
=======================================
  Files        2356     2356           
  Lines      210713   210741   +28     
=======================================
+ Hits       195675   195718   +43     
+ Misses      15038    15023   -15     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Can we add some unit tests that verify the desired behavior?

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @cuiy0006, @ddelgrosso1, and @eliekozah)


google/cloud/storage/internal/rest/stub.cc line 121 at r4 (raw file):

}

Status AddCustomHeaders(Options const& options, RestRequestBuilder& builder) {

It doesn't appear that anything called in this function can fail, so let's change the return type to void.


google/cloud/storage/internal/rest/stub.cc line 129 at r4 (raw file):

}

Status AddHeaders(Options const& options, RestRequestBuilder& builder) {

It appears AddAuthortizationHeader can fail, so this function needs to continue to return Status. Additionally, calls to this function need to propagate up any non-OK Status that are returned.

@ddelgrosso1
Copy link
Contributor

@eliekozah can you convert this to a draft pr if you are still working on it. Will help avoid people reviewing it.

@eliekozah eliekozah marked this pull request as draft April 2, 2025 19:03
@eliekozah eliekozah marked this pull request as ready for review April 7, 2025 14:23
Copy link
Contributor Author

@eliekozah eliekozah 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 10 files reviewed, 5 unresolved discussions (waiting on @cuiy0006, @ddelgrosso1, and @scotthart)


google/cloud/storage/internal/rest/stub.cc line 121 at r4 (raw file):

Previously, scotthart (Scott Hart) wrote…

It doesn't appear that anything called in this function can fail, so let's change the return type to void.

Done.


google/cloud/storage/internal/rest/stub.cc line 129 at r4 (raw file):

Previously, scotthart (Scott Hart) wrote…

It appears AddAuthortizationHeader can fail, so this function needs to continue to return Status. Additionally, calls to this function need to propagate up any non-OK Status that are returned.

Done.

Copy link
Contributor Author

@eliekozah eliekozah left a comment

Choose a reason for hiding this comment

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

@scotthart Done

Reviewable status: 0 of 10 files reviewed, 5 unresolved discussions (waiting on @cuiy0006, @ddelgrosso1, and @scotthart)

Copy link
Contributor

@ddelgrosso1 ddelgrosso1 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 10 files reviewed, 13 unresolved discussions (waiting on @bajajneha27, @cuiy0006, @eliekozah, and @scotthart)

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 3 files at r6.
Reviewable status: 1 of 10 files reviewed, 13 unresolved discussions (waiting on @bajajneha27, @cuiy0006, and @eliekozah)

@ddelgrosso1 ddelgrosso1 merged commit fc70c16 into googleapis:main Apr 16, 2025
78 of 79 checks passed
devbww pushed a commit to devbww/google-cloud-cpp that referenced this pull request Apr 25, 2025
* fix: fix bug with customheadersoption

* fix: fix bug with customheadersoption

* fix: fix bug with customheadersoption

* fix: fix bug with customheadersoption

* fix: fix bug with customheadersoption

* fix: fix bug with customheadersoption

* fix: fix bug with customheadersoption

---------

Co-authored-by: Denis DelGrosso <[email protected]>
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.

5 participants