fix(spanner): do not drop Spanner and GAX options on Database and Transaction methods by cy-yun · Pull Request #9381 · googleapis/google-cloud-php

cy-yun · 2026-07-22T00:24:18Z

This PR addresses an issue where certain Google Cloud Spanner and underlying GAX options (such as requestOptions, headers, retrySettings, timeoutMillis, and transportOptions) were being inadvertently dropped by various Database and Transaction methods.

Previously, methods used pluckArray() with a highly restrictive and hardcoded list of allowed keys, silently ignoring and discarding valid user-provided configuration. This PR replaces that scattered logic with a centralized, unified OptionsValidator that preserves all allowed Spanner and GAX options, ensuring they correctly propagate down to the underlying Operation calls and API requests.

Changes Include:

Core\OptionsValidator: Introduced stripUnknownOptions() as a centralized utility to automatically strip unknown array keys while safely preserving all recognized GAPIC request properties, common Google Cloud PHP options, and GAX configurations.
Spanner\Database: Updated array extractions in insert, insertBatch, update, updateBatch, insertOrUpdate, insertOrUpdateBatch, replace, replaceBatch, delete, execute, and read to rely on the unified stripUnknownOptions() approach.
Spanner\TransactionalReadTrait: Updated execute() and read() to pass user configuration through the validator, preserving requestOptions and transportOptions.
Spanner\Operation: Updated partitionQuery() and partitionRead() to use the new option validator.
Spanner\Transaction: Updated commit() to propagate options via the unified validator while continuing to cleanly ignore requestTag (which is unsupported by the Spanner backend for commits).

Unrelated Unit Test Fix:

Spanner\ValueMapper: Resolved a PHP 8.1+ compatibility issue (Using null as an array offset is deprecated) by explicitly casting potentially null array keys to strings during type annotations.

Verification

Added Core\Tests\Unit\OptionsValidatorTest to comprehensively verify that recognized Spanner properties, Google Cloud options, and GAX options are preserved, while arbitrary invalid keys are correctly stripped.
Restructured Spanner\Tests\Unit\DatabaseTest, TransactionTest, and OperationTest to concisely verify option propagation across core methods. The tests now explicitly assert that a fabricated unknownOption is cleanly filtered out by the validator without crashing.

bshaffer

Looks great! A few thoughts/suggestions

bshaffer · 2026-07-22T02:08:31Z

+            'parameters',
+            'types',
+            'queryOptions',
+            'sessionOptions',


Where is sessionOptions coming from? I don't see it in ExecuteSqlRequest or in Operation::execute.

I did a brief search and I think it might be possible that this option is no longer supported

I added it because it was in the docs above. But you're right that it's not in the ExecuteSqlRequest. I'll remove it from everywhere.

bshaffer · 2026-07-22T02:08:44Z

+            'retrySettings',
+            'timeoutMillis',
+            'transportOptions'
+        ], $options);


I know we said pluckArray works, but we also discussed potentially adding OptionsValidator::stripUnknownOptions so we could do something like this:

$options = $this->stripUnknownOptions( $options, ['parameters', 'types'] CallOptions::class, ExecuteSqlRequest::class, );

How that I'm seeing the size of these lists, and the number of their occurrences, what are your thoughts on going that direction instead?

I like the stripUnknownOptions approach more as well. It's more elegant, extendable, and maintainable. I'll make that change.

bshaffer · 2026-07-22T02:12:10Z

            new ExecuteBatchDmlRequest(),
-            CallOptions::class
+            CallOptions::class,
+            [],


What is this here for?

That empty array acts as the 4th parameter ($transactionOptions) in order to pass the ['route-to-leader'] configuration to the 5th parameter ($additionalOptions). It's solely for parameter positioning.

I'm confused because validateOptions uses variable parameters, and $transactionOptions is not a parameter.

google-cloud-php/Core/src/ApiHelperTrait.php

Line 283 in a9ffd8f

private function validateOptions(array $options, array|Message|string ...$optionTypes): array

What seems to be happening is you're defining an empty set of parameters via [], and then saving them in an empty variable $_. This should instead look like this:

[$dmlRequest, $callOptions, $rtl] = $this->validateOptions( $options, new ExecuteBatchDmlRequest(), ['route-to-leader'] );

bshaffer · 2026-07-22T02:14:47Z

        }
+        if (isset($commitOptions['requestOptions']['requestTag'])) {
+            unset($commitOptions['requestOptions']['requestTag']);
+            if (empty($commitOptions['requestOptions'])) {


nit: this is a bit confusing as written, as empty also returns true for non-existant keys

Suggested change

if (empty($commitOptions['requestOptions'])) {

if (0 === count($commitOptions['requestOptions'])) {

…nsaction methods Fixes #9378

…lidatorTest

bshaffer

All small nits and suggestions! Looking really good.

bshaffer · 2026-07-22T21:50:22Z

            new ExecuteBatchDmlRequest(),
-            CallOptions::class
+            CallOptions::class,
+            [],


I'm confused because validateOptions uses variable parameters, and $transactionOptions is not a parameter.

google-cloud-php/Core/src/ApiHelperTrait.php

Line 283 in a9ffd8f

private function validateOptions(array $options, array|Message|string ...$optionTypes): array

What seems to be happening is you're defining an empty set of parameters via [], and then saving them in an empty variable $_. This should instead look like this:

[$dmlRequest, $callOptions, $rtl] = $this->validateOptions( $options, new ExecuteBatchDmlRequest(), ['route-to-leader'] );

bshaffer · 2026-07-22T21:51:04Z

    public function executeUpdateBatch(array $statements, array $options = []): BatchDmlResult
    {
-        $options = $this->buildUpdateOptions($options);
+        $options = $this->buildUpdateOptions($options, \Google\Cloud\Spanner\V1\ExecuteBatchDmlRequest::class);


nit: import this class

bshaffer · 2026-07-22T21:51:23Z

+        $commitOptions = (new \Google\Cloud\Core\OptionsValidator())->stripUnknownOptions(
+            $options,
+            \Google\ApiCore\Options\CallOptions::class,
+            \Google\Cloud\Spanner\V1\CommitRequest::class


nit: import these classes

bshaffer · 2026-07-22T21:52:33Z

-        $commitOptions = $this->pluckArray(
-            ['returnCommitStats', 'maxCommitDelay'],
-            $options
+        $commitOptions = (new \Google\Cloud\Core\OptionsValidator())->stripUnknownOptions(


I think we should either...

add stripUnknownOptions to ApiHelperTrait and import it in this class, or

add $this->optionsValidator in the constructor

...rather than create this object on the fly each time.

Because we are doing this in both Transaction and TransactionalReadTrait, if we add property $optionsValidator in the trait, we will be able to reference it here in Transaction as well.

Then we need to instantiate the $optionsValidator in Transaction::__construct(), and in SnapshotTrait::initialize() and we should be good to go

bshaffer · 2026-07-22T21:52:54Z

-        if (isset($options['requestOptions'])) {
-            $updateOptions['requestOptions'] = $options['requestOptions'];
-        }
+        $updateOptions = (new \Google\Cloud\Core\OptionsValidator())->stripUnknownOptions(


Same here: if we use ApiHelperTrait, then we can just call $this->stripUnknownOptions

bshaffer · 2026-07-22T21:55:43Z

+            } elseif ($optionType === CallOptions::class) {
+                $callOptionKeys = array_keys((new CallOptions([]))->toArray());
+                $allowedKeys = array_merge($allowedKeys, $callOptionKeys);
+            } elseif ($optionType instanceof Message


I think we should only support message class name in this case. There's no reason to provide a message instance (as there is for validateOptions).

bshaffer · 2026-07-22T21:56:50Z

+            } elseif (is_string($optionType)) {
+                $allowedKeys[] = $optionType;


nit but we probably don't need this... we can just say scalars should always be string[]

bshaffer · 2026-07-22T21:59:18Z

+    /**
+     * @var array
+     */
+    private static array $allowedKeysCache = [];


This should be at the top of the class between use ArrayTrait; and __construct

bshaffer · 2026-07-22T22:02:30Z

        }

-        $options = $this->buildUpdateOptions($options);
+        $options = $this->buildUpdateOptions($options, \Google\Cloud\Spanner\V1\ExecuteSqlRequest::class);


nit: import this class

product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jul 22, 2026

cy-yun force-pushed the fix/spanner-gax-options-propagation branch 3 times, most recently from 727ef6b to 19a0ff1 Compare July 22, 2026 00:59

cy-yun marked this pull request as ready for review July 22, 2026 01:16

cy-yun requested a review from a team as a code owner July 22, 2026 01:16

cy-yun assigned bshaffer Jul 22, 2026

cy-yun enabled auto-merge (squash) July 22, 2026 02:12

bshaffer reviewed Jul 22, 2026

View reviewed changes

cy-yun force-pushed the fix/spanner-gax-options-propagation branch 10 times, most recently from 6f60e2a to 35386e0 Compare July 22, 2026 03:05

cy-yun added 3 commits July 21, 2026 20:08

fix(spanner): do not drop Spanner and GAX options on Database and Tra…

bba16a0

…nsaction methods Fixes #9378

test(spanner): add tests for Spanner and GAX options propagation

5c099f0

fix(spanner): avoid null array offset deprecation warning in ValueMapper

c7e2ee6

cy-yun force-pushed the fix/spanner-gax-options-propagation branch from 35386e0 to c7e2ee6 Compare July 22, 2026 03:08

cy-yun added 3 commits July 21, 2026 20:30

test(core): replace Spanner dependency with DummyMessage in OptionsVa…

b785bed

…lidatorTest

fix(spanner): pass request class dynamically to buildUpdateOptions

711d7a6

style: fix style violations in recent commits

3ae10c6

cy-yun force-pushed the fix/spanner-gax-options-propagation branch from 18d66ba to 3ae10c6 Compare July 22, 2026 03:45

cy-yun added 2 commits July 21, 2026 21:00

build(spanner): bump google/cloud-core dependency to ^1.73.0

c27cfae

build(spanner): add nikic/php-parser to require-dev for snippet tests

f857d44

bshaffer reviewed Jul 22, 2026

View reviewed changes

	if (empty($commitOptions['requestOptions'])) {
	if (0 === count($commitOptions['requestOptions'])) {

		} elseif (is_string($optionType)) {
		$allowedKeys[] = $optionType;

Uh oh!

Conversation

cy-yun commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verification

Uh oh!

bshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshaffer Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cy-yun commented Jul 22, 2026 •

edited

Loading

bshaffer Jul 22, 2026 •

edited

Loading