feat: add support for Firestore database id configuration #2164

meltsufin · 2023-09-13T19:51:38Z

...java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfigurationTests.java

suztomo · 2023-09-13T20:23:41Z

@diegomarquezp I found the error message your pull request should fix "Caused by: java.sql.SQLException: Access denied for user 'root'@'cloudsqlproxy~20.49.35.21' (using password: YES)"

https://github.com/GoogleCloudPlatform/spring-cloud-gcp/actions/runs/6177169510/job/16767718916?pr=2164

...main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java

...re/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreProperties.java

...java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfigurationTests.java

…-firestore "(default)"

meltsufin · 2023-09-14T02:29:03Z

Error:  Errors: 
Error:    FirestoreSampleApplicationIntegrationTests.saveUserTest » StatusRuntime INVALID_ARGUMENT: Missing routing header. Please fill in the request header with format x-goog-request-params:project_id=spring-cloud-gcp-ci&database_id=firestoredb. Please refer to https://firebase.google.com/docs/firestore/manage-databases#access_a_named_database_with_a_client_library for more details.

@blakeli0 Do I need a newer version of GAX or Firestore client lib?

lqiu96 · 2023-09-14T15:04:20Z

Iirc, Firestore temporarily removed code for gRPC's Dynamic Routing Headers a while back (due to an encoding issue). I think they only recently added that back in: googleapis/java-firestore#1418. I think this would work with Firestore v3.14.3+ (I have no idea how the tests work and it could be a different issue).

blakeli0 · 2023-09-14T15:17:45Z

As Lawrence mentioned, they recently added the routing headers back and Spring Cloud GCP didn't get it yet. But if this is the root cause, it means that the server made a behavior breaking change that requires the customers to update their client code, which is very bad.

tom-andersen · 2023-09-19T13:10:23Z

...main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java

          .withCallCredentials(
              MoreCallCredentials.from(
                  GcpFirestoreAutoConfiguration.this.credentialsProvider.getCredentials()));
+
+      // add routing header for custom database id
+      if (databaseId != null && !databaseId.equals("(default)")) {


Routing header should be passed in all requests. It just happens it is only enforced on non-default database ids.

The projectId and databaseId should be URL encoded. Basically, treat this string the same as you would treat query part of a URL.

Ask Sichen Liu if you need more details. There is a performance impact if you omit header for default database (I believe).

blakeli0 · 2023-09-19T15:34:13Z

...main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java

+      Metadata routingHeader = new Metadata();
+      Metadata.Key<String> key =
+          Metadata.Key.of(Headers.DYNAMIC_ROUTING_HEADER_KEY, Metadata.ASCII_STRING_MARSHALLER);
+      routingHeader.put(key,


This may work for this specific case, but routing header config could change in the future. I'm not sure it works for all the new routing header configs either, but maybe those are not required.
In long term, switching to GAPIC stubs should solve this issue, but I'm not sure it is compatible with the Reactive paradigm. Why did we choose to use the gRPC generated stubs instead of GAPIC ones? Only because of Reactive?

Yep, this integration was specifically made to support reactive, and I believe at the time GAPIC was not compatible.

I see. My concern is that every time the backend makes the routing header required for a new RPC, or change how the routing header is constructed, we would have to come back and manually change it in spring-cloud-gcp. It may not in scope of this PR, can we investigate the possibility of switching to the GAPIC stubs?

blakeli0 · 2023-09-19T16:00:02Z

...main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java

+      Metadata.Key<String> key =
+          Metadata.Key.of(Headers.DYNAMIC_ROUTING_HEADER_KEY, Metadata.ASCII_STRING_MARSHALLER);
+      routingHeader.put(key,
+          "project_id=" + URLEncoder.encode(projectId, StandardCharsets.US_ASCII)


We use PercentEscaper in gax, which I think has slight advantage over the Java default one based on @lqiu96's investigation.

Switch to `PercentEscaper1.

…db-id

tom-andersen

LGTM

sonarcloud · 2023-09-19T17:13:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

suztomo · 2023-09-19T18:07:52Z

docs/src/main/asciidoc/datastore.adoc

@@ -68,7 +68,7 @@ The following configuration options are available:
 | Name | Description | Required | Default value
 | `spring.cloud.gcp.datastore.enabled` | Enables the Cloud Datastore client | No | `true`
 | `spring.cloud.gcp.datastore.project-id` | Google Cloud project ID where the Google Cloud Datastore API is hosted, if different from the one in the <<spring-cloud-gcp-core,Spring Framework on Google Cloud Core Module>> | No |
-| `spring.cloud.gcp.datastore.database-id` | Google Cloud project can host multiple databases. You can specify which database will be used. | No |
+| `spring.cloud.gcp.datastore.database-id` | Google Cloud project can host multiple databases. You can specify which database will be used. If not specified, the database id will be "(default)". | No |


"Default value" column is missing.

Also adds routing headers for all Firestore requests. Fixes #2145.

Also adds routing headers for all Firestore requests. Backport of #2164. Fixes #2145.

thoastbrot · 2023-09-27T10:37:39Z

Hi @meltsufin, this breaks my (test) configuration where I only define service credentials via spring.cloud.gcp.firestore.credentials.encoded-key - this contains the project ID, and somehow this worked till 4.7.2, but crashes here, as the percent escaper receives projectId null. Do you have an idea how this should work?

meltsufin · 2023-09-27T14:16:54Z

@thoastbrot I guess it's because your project ID is null in the configuration. You can configure a dummy project ID for now as a workaround. Would you mind filing a separate issue?

thoastbrot · 2023-09-28T11:04:04Z

I'll do that. dummy project-id doesn't work the way you assume.

With feature in [#2164](#2164), now Firestore can run in same project as datastore. This PR cleans up settings for integration tests ci so that tests for Firestore module and samples run in the same ci project as the rest. Modules affected in this change: - autoconfigure/firestore - data-firestore - data-firestore-sample - starter-firestore-sample Before this change, these modules are setup to run with `spring-cloud-gcp-ci-firestore` project. After this change: - database-id=firestoredb is specified for above module's tests. - does not specify `firestore.project-id` and get project id info from `DefaultGcpProjectIdProvider`. For integration test ci, it is `spring-cloud-gcp-ci` set via gcloud [here](https://github.com/GoogleCloudPlatform/spring-cloud-gcp/blob/ef30225e60e270f8cc1a9213728f0cfdd7e4de50/.github/workflows/integrationTests.yaml#L51).

feat: add support for Firestore database id configuration

dc88042

Fixes #2145.

meltsufin requested a review from a team as a code owner September 13, 2023 19:51

JoeWang1127 reviewed Sep 13, 2023

View reviewed changes

...java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfigurationTests.java Show resolved Hide resolved

update default parent

7cb9669

blakeli0 reviewed Sep 13, 2023

View reviewed changes

meltsufin added 2 commits September 13, 2023 18:13

fix unit test and address feedback

86892e6

use firestoredb on spring-cloud-gcp-ci instead of spring-cloud-gcp-ci…

0ca1e40

…-firestore "(default)"

Add routing header for database id to stub

c873f8d

meltsufin requested review from JoeWang1127 and blakeli0 September 19, 2023 01:23

tom-andersen reviewed Sep 19, 2023

View reviewed changes

meltsufin added 3 commits September 19, 2023 09:41

Merge branch 'main' into firestore-db-id

1a35d97

url encode routing headers

68bf732

cleanup code

3d00420

meltsufin requested a review from tom-andersen September 19, 2023 15:19

blakeli0 reviewed Sep 19, 2023

View reviewed changes

meltsufin requested review from blakeli0 and lqiu96 and removed request for tom-andersen September 19, 2023 15:35

blakeli0 reviewed Sep 19, 2023

View reviewed changes

meltsufin added 3 commits September 19, 2023 12:38

switch to PercentEscaper

357373a

switch to PercentEscaper

de8f9c2

Merge remote-tracking branch 'origin/firestore-db-id' into firestore-…

55eb052

…db-id

tom-andersen approved these changes Sep 19, 2023

View reviewed changes

meltsufin enabled auto-merge (squash) September 19, 2023 18:07

suztomo approved these changes Sep 19, 2023

View reviewed changes

meltsufin merged commit 1ea0cfb into main Sep 19, 2023
28 of 29 checks passed

meltsufin deleted the firestore-db-id branch September 19, 2023 18:08

release-please bot mentioned this pull request Sep 19, 2023

chore(main): release 4.8.0 #2147

Merged

meltsufin added a commit that referenced this pull request Sep 19, 2023

feat: add support for Firestore database id configuration (#2164)

f4d028e

Also adds routing headers for all Firestore requests. Fixes #2145.

meltsufin mentioned this pull request Sep 19, 2023

feat(3.x): add support for Firestore database id configuration #2184

Merged

meltsufin added a commit that referenced this pull request Sep 20, 2023

feat(3.x): add support for Firestore database id configuration (#2184)

0c4ff8c

Also adds routing headers for all Firestore requests. Backport of #2164. Fixes #2145.

thoastbrot mentioned this pull request Sep 28, 2023

project ID is not read from the service account json set via credentials.encoded-key anymore #2205

Closed

zhumin8 mentioned this pull request Nov 1, 2023

chore: cleanup firestore ci test resource #2322

Merged

release-please bot mentioned this pull request Dec 18, 2023

chore(main): release 5.0.1 #2467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for Firestore database id configuration #2164

feat: add support for Firestore database id configuration #2164

meltsufin commented Sep 13, 2023

suztomo commented Sep 13, 2023

meltsufin commented Sep 14, 2023

lqiu96 commented Sep 14, 2023 •

edited

Loading

blakeli0 commented Sep 14, 2023

tom-andersen Sep 19, 2023 •

edited

Loading

blakeli0 Sep 19, 2023 •

edited

Loading

meltsufin Sep 19, 2023

blakeli0 Sep 19, 2023

blakeli0 Sep 19, 2023

meltsufin Sep 19, 2023

tom-andersen left a comment

sonarcloud bot commented Sep 19, 2023

suztomo Sep 19, 2023

thoastbrot commented Sep 27, 2023

meltsufin commented Sep 27, 2023

thoastbrot commented Sep 28, 2023

feat: add support for Firestore database id configuration #2164

feat: add support for Firestore database id configuration #2164

Conversation

meltsufin commented Sep 13, 2023

suztomo commented Sep 13, 2023

meltsufin commented Sep 14, 2023

lqiu96 commented Sep 14, 2023 • edited Loading

blakeli0 commented Sep 14, 2023

tom-andersen Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

blakeli0 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

meltsufin Sep 19, 2023

Choose a reason for hiding this comment

blakeli0 Sep 19, 2023

Choose a reason for hiding this comment

blakeli0 Sep 19, 2023

Choose a reason for hiding this comment

meltsufin Sep 19, 2023

Choose a reason for hiding this comment

tom-andersen left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 19, 2023

suztomo Sep 19, 2023

Choose a reason for hiding this comment

thoastbrot commented Sep 27, 2023

meltsufin commented Sep 27, 2023

thoastbrot commented Sep 28, 2023

lqiu96 commented Sep 14, 2023 •

edited

Loading

tom-andersen Sep 19, 2023 •

edited

Loading

blakeli0 Sep 19, 2023 •

edited

Loading