Skip to content

Conversation

@bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Apr 29, 2025

  • This change will improve latency for every request the Firestore handwritten library supports
  • Handwritten Firestore client does NOT support the listen BIDI streaming method, this is GAPIC only, but this call can still be made by supplying the routing headers manually
$stream = $firestoreClient->listen([
    'headers' => [
        'x-goog-request-params' => [
            'database=' . $database
        ]
    ]
]);

@bshaffer bshaffer changed the title fix: remove legacy google-cloud-resource-prefix in favor of x-goog-re… fix: remove legacy google-cloud-resource-prefix in favor of x-goog-request-params May 1, 2025
@bshaffer bshaffer changed the title fix: remove legacy google-cloud-resource-prefix in favor of x-goog-request-params fix(Firestore): use x-goog-request-params header May 16, 2025
@product-auto-label product-auto-label bot added the api: firestore Issues related to the Firestore API. label May 18, 2025
@bshaffer
Copy link
Contributor Author

bshaffer commented May 20, 2025

This requires more research, mostly because when calling the listen methods (which are the methods where this is necessary), the API throws an exception because the routing headers aren't supplied

Call failed with message: Missing required http header ('google-cloud-resource-prefix' or 'x-goog-request-params') or query param 'database'.

I can fix this by supplying the headers parameter directly to the listen call:

$stream = $firestoreClient->listen([
    'headers' => [
        'x-goog-request-params' => [
            'database=' . $database
        ]
    ]
]);

But this is non-intuitive and error-prone. With minimal changes to the GAPIC layer, we could make this work by only supplying the "database" option:

$stream = $firestoreClient->listen(['database' => $database]);

And follow this up by generating a "PHPDoc" explaining the database option's usage, which would hopefully be enough to make this discoverable and implementable by developers:

    /**
     * Listens to changes. This method is only available via gRPC or WebChannel
     * (not REST).
     *
     * @example samples/V1/FirestoreClient/listen.php
     *
     * @param array $callOptions {
     *     Optional.
     *
+    *     @type string $datbase
+    *           Set the database of the call, to be added as a routing header
     *     @type int $timeoutMillis
     *           Timeout to use for this call.
     * }
     *
     * @return BidiStream
     *
     * @throws ApiException Thrown if the API call fails.
     */
    public function listen(array $callOptions = []): BidiStream
    {
+       $requestParamHeaders = [];
+       if (isset($callOptions['database'])) {
+           $requestParamHeaders['database'] = $callOptions['database'];
+       }
+       $requestParams = new \Google\ApiCore\RequestParamsHeaderDescriptor($requestParamHeaders);
+       $callOptions['headers'] = isset($optionalArgs['headers']) ? array_merge($requestParams->getHeader(), $callOptions['headers']) : $requestParams->getHeader();
        return $this->startApiCall('Listen', null, $callOptions);
    }

@viacheslav-rostovtsev
Copy link
Member

LGTM

@bshaffer bshaffer marked this pull request as ready for review August 27, 2025 21:55
@bshaffer bshaffer requested review from a team as code owners August 27, 2025 21:55
@bshaffer bshaffer added the next release PRs to be included in the next release label Sep 3, 2025
@bshaffer bshaffer merged commit fd78680 into main Sep 5, 2025
32 checks passed
@bshaffer bshaffer deleted the firestore-change-headers branch September 5, 2025 18:28
purva9413 pushed a commit to purva9413/google-cloud-php that referenced this pull request Oct 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the Firestore API. next release PRs to be included in the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants