fix(core): merge per-call restOptions with the client-level ones - #9437
fix(core): merge per-call restOptions with the client-level ones#9437smichaelsen wants to merge 2 commits into
Conversation
RequestWrapper::getRequestOptions() replaced the client-level restOptions with the per-call ones whenever a call supplied any, so transport settings configured once on the client — proxy, verify, cert — were silently dropped for those calls. Storage made this visible in v1.51.0: the X-Goog-Hash checksum header added in googleapis#8825 travels as a per-call restOptions entry, so every upload lost the client's proxy configuration. Downloads hit the same path through the on_headers callback used to detect transcoded objects. In proxy-only environments uploads and downloads stopped working while metadata and auth calls, which pass no per-call restOptions, kept succeeding. Per-call options now take precedence over the client-level ones on conflict, and nested `headers` arrays are merged on their own so a per-call header does not drop the client's default headers. The merge is otherwise shallow, which avoids array_merge_recursive turning colliding scalars such as `proxy` into arrays that Guzzle rejects. Root cause analysis and the merge strategy are from @salilg-eng in googleapis#9212. Fixes googleapis#9212
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
bshaffer
left a comment
There was a problem hiding this comment.
I have one minor suggestion, but in general this is LGTM. We should get a review from @thiyaguk09 as well
|
Looks good to me! This is exactly what I had in mind for #9212. The array union approach is clean, and handling the headers separately was a good call. |
|
@smichaelsen thank you for your contribution! Please sign the github CLA and we can merge your changes. |
Fixes #9212.
Problem
RequestWrapper::getRequestOptions()replaces the client-levelrestOptionswith the per-call ones whenever a call supplies any:Transport settings configured once on the client —
proxy,verify,cert— are therefore silently dropped for any call that passes its ownrestOptions.Storage made this visible in v1.51.0. The
X-Goog-Hashchecksum header added in #8825 travels as a per-callrestOptionsentry, so every upload loses the client's proxy configuration. Downloads reach the same code path through theon_headerscallback used to detect transcoded objects. In proxy-only environments uploads and downloads stop working, while metadata and auth calls — which pass no per-callrestOptions— keep succeeding, which makes the failure look unrelated to the upgrade.This is not Storage-specific: any component that sets per-call
restOptionsdrops client transport configuration the same way.Fix
Merge instead of replace. Per-call options win on conflict, and nested
headersarrays are merged on their own so a per-call header does not drop the client's default headers.The merge is deliberately shallow otherwise.
array_merge_recursiveis not usable here: Guzzle options mix scalars and arrays, and colliding scalars such asproxywould be turned into arrays that Guzzle rejects.Tests
Three cases added to
Core/tests/Unit/RequestWrapperTest.php:proxy/verifysurvive a call that supplies its ownrestOptions(the Storage upload scenario)headersmerge rather than replaceThe first two fail on
mainwithUndefined array key "proxy". The fullCoreunit suite passes (534 tests), as doesphpcs --standard=phpcs-ruleset.xmlon both changed files.Credit
The root cause analysis and the merge strategy are @salilg-eng's, from the discussion in #9212. I'm opening the PR since the issue has been sitting for a couple of months and it is currently blocking us from upgrading past
google/cloud-storage1.50.