fix: fix bug with customheadersoption #15053

eliekozah · Mar 25, 2025

This is a Draft PR for PR issue b/402481452

Please do not merge.

This change is

ddelgrosso1 · Mar 25, 2025

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.

google/cloud/storage/internal/rest/stub.cc

github-advanced-security · Apr 1, 2025

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 · Apr 1, 2025

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"}})
    );

codecov · 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.

scotthart

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 · Apr 2, 2025

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

eliekozah

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.

eliekozah

@scotthart Done

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

google/cloud/storage/internal/rest/stub.cc

google/cloud/storage/internal/rest/stub.h

google/cloud/storage/internal/rest/stub.cc

google/cloud/storage/internal/rest/stub_test.cc

ddelgrosso1

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

scotthart

Reviewed 1 of 3 files at r6.
Reviewable status: 1 of 10 files reviewed, 13 unresolved discussions (waiting on @bajajneha27, @cuiy0006, and @eliekozah)

* 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 <85250797+ddelgrosso1@users.noreply.github.com>

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 requested review from a team as code owners March 25, 2025 20:48

ddelgrosso1 reviewed Mar 25, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.cc Outdated Show resolved Hide resolved

cuiy0006 reviewed Mar 26, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.cc Outdated Show resolved Hide resolved

google/cloud/storage/internal/rest/stub.cc Outdated Show resolved Hide resolved

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

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

fix: fix bug with customheadersoption

f55ff55

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

fix: fix bug with customheadersoption

c3a9166

eliekozah temporarily deployed to external April 2, 2025 14:35 — with GitHub Actions Inactive

scotthart requested changes Apr 2, 2025

View reviewed changes

eliekozah marked this pull request as draft April 2, 2025 19:03

fix: fix bug with customheadersoption

824cffc

eliekozah had a problem deploying to external April 3, 2025 18:43 — with GitHub Actions Error

fix: fix bug with customheadersoption

0f20450

eliekozah had a problem deploying to external April 3, 2025 19:47 — with GitHub Actions Error

fix: fix bug with customheadersoption

6809d59

eliekozah had a problem deploying to external April 4, 2025 18:11 — with GitHub Actions Error

eliekozah marked this pull request as ready for review April 7, 2025 14:23

eliekozah commented Apr 7, 2025

View reviewed changes

ddelgrosso1 reviewed Apr 7, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.cc Outdated Show resolved Hide resolved

ddelgrosso1 reviewed Apr 7, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.cc Show resolved Hide resolved

ddelgrosso1 reviewed Apr 7, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.cc Outdated Show resolved Hide resolved

ddelgrosso1 reviewed Apr 7, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.h Outdated Show resolved Hide resolved

fix: fix bug with customheadersoption

b127147

eliekozah had a problem deploying to external April 7, 2025 20:50 — with GitHub Actions Error

ddelgrosso1 reviewed Apr 8, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.cc Outdated Show resolved Hide resolved

ddelgrosso1 reviewed Apr 8, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.cc Show resolved Hide resolved

fix: fix bug with customheadersoption

ee1798c

eliekozah had a problem deploying to external April 8, 2025 12:44 — with GitHub Actions Error

bajajneha27 reviewed Apr 9, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub.cc Show resolved Hide resolved

google/cloud/storage/internal/rest/stub_test.cc Show resolved Hide resolved

bajajneha27 approved these changes Apr 11, 2025

View reviewed changes

google/cloud/storage/internal/rest/stub_test.cc Show resolved Hide resolved

Merge branch 'main' into bugfix-headers-for-json

b51b3b8

ddelgrosso1 temporarily deployed to external April 11, 2025 14:45 — with GitHub Actions Inactive

ddelgrosso1 approved these changes Apr 11, 2025

View reviewed changes

Merge branch 'main' into bugfix-headers-for-json

f765e10

ddelgrosso1 temporarily deployed to external April 15, 2025 21:02 — with GitHub Actions Inactive

ddelgrosso1 approved these changes Apr 15, 2025

View reviewed changes

scotthart approved these changes Apr 16, 2025

View reviewed changes

ddelgrosso1 merged commit fc70c16 into googleapis:main Apr 16, 2025
78 of 79 checks passed

Search code, repositories, users, issues, pull requests...

fix: fix bug with customheadersoption #15053

fix: fix bug with customheadersoption #15053

Uh oh!

Conversation

eliekozah commented Mar 25, 2025 • edited by scotthart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddelgrosso1 commented Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-advanced-security bot commented Apr 1, 2025

Uh oh!

eliekozah commented Apr 1, 2025

Uh oh!

codecov bot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

scotthart left a comment

Choose a reason for hiding this comment

Uh oh!

ddelgrosso1 commented Apr 2, 2025

Uh oh!

eliekozah left a comment

Choose a reason for hiding this comment

Uh oh!

eliekozah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ddelgrosso1 left a comment

Choose a reason for hiding this comment

Uh oh!

scotthart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eliekozah commented Mar 25, 2025 •

edited by scotthart

Loading

codecov bot commented Apr 2, 2025 •

edited

Loading