This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Setup the Autoconfiguration for Firestore Emulator #2244

Merged

dzou merged 12 commits into master from firestore-emulator-wip

Apr 21, 2020

Contributor

dzou commented Mar 9, 2020 •

edited

Loading

Add support for using the Firestore emulator.

It is now enabled with spring.cloud.gcp.firestore.emulator.enabled=true and host port set via spring.cloud.gcp.firestore.host-port=xxxx.
In order to get it to work, we must disable stream tokens in the firestore template if the emulator is disabled; see notes below.

Fixes #1956.

Previous Notes:

If you try to run the reactive firestore sample with the emulator, you will get the error:

"Resuming streams is not supported, do not set stream ID."

The emulator will throw an error if you attempt to set stream/resume tokens in the request. However, our reactive Firestore Template uses this functionality and includes these tokens in the requests: see code

So resolving #1956 may not be possible in the end. Maybe this could benefit from a second set of eyes too.


          Firestore Emulator setup WIP.

f0f1cb2

dzou requested review from dmitry-s and meltsufin

March 9, 2020 18:29

dzou added firestore help wanted labels

Contributor

dmitry-s commented Mar 10, 2020

@dzou Can we disable setStreamToken if spring.cloud.gcp.firestore.emulator.enabled is set?

Contributor Author

dzou commented Mar 10, 2020

@dzou Can we disable setStreamToken if spring.cloud.gcp.firestore.emulator.enabled is set?

Ya I considered that, though I wasn't sure if it made sense to make the code accommodate the emulator since the emulator is test-only. What do you both think? @dmitry-s @meltsufin

Contributor

meltsufin commented Mar 10, 2020

Let's wait for a response from the Firestore team.
b/151148092.

Contributor Author

dzou commented Apr 9, 2020

Will try following up again.

dzou added 4 commits

April 13, 2020 14:47


          Merge branch 'master' into firestore-emulator-wip

06f48f9


          fix property for emulator

c6012db


          Merge branch 'master' into firestore-emulator-wip

b22c30a


          template changes

641444a

codecov bot commented Apr 15, 2020 •

edited

Loading

Codecov Report

Merging #2244 into master will increase coverage by 0.25%.
The diff coverage is 89.36%.

@@             Coverage Diff              @@
##             master    #2244      +/-   ##
============================================
+ Coverage     73.54%   73.79%   +0.25%     
- Complexity     2078     2090      +12     
============================================
  Files           258      259       +1     
  Lines          7469     7510      +41     
  Branches        772      774       +2     
============================================
+ Hits           5493     5542      +49     
+ Misses         1620     1613       -7     
+ Partials        356      355       -1

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`73.79% <89.36%> (+0.25%)`	`2090.00 <10.00> (+12.00)`

Impacted Files	Coverage Δ	Complexity Δ
...restore/GcpFirestoreEmulatorAutoConfiguration.java	`84.84% <84.84%> (ø)`	`4.00 <4.00> (?)`
...rk/cloud/gcp/data/firestore/FirestoreTemplate.java	`76.25% <100.00%> (+9.08%)`	`49.00 <6.00> (+6.00)`
...igure/firestore/GcpFirestoreAutoConfiguration.java	`94.11% <0.00%> (+2.94%)`	`5.00% <0.00%> (+1.00%)`
...utoconfigure/firestore/GcpFirestoreProperties.java	`100.00% <0.00%> (+18.18%)`	`6.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac639ce...e0589fd. Read the comment docs.

eddumelendez reviewed

View reviewed changes

Contributor

eddumelendez left a comment

@dzou I have left few messages :)

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Outdated Show resolved Hide resolved

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Show resolved Hide resolved

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Outdated Show resolved Hide resolved

dzou added 5 commits

April 15, 2020 15:49


          add best-effort tests

e9c76a3


          PR Comments

ef44d54


          PR Comments

2d7abd6


          checkstyle

c76548a


          checkstyle

c3ec709

Contributor Author

dzou commented Apr 15, 2020

Thanks for the review @eddumelendez !

@meltsufin , @dmitry-s - I updated this PR so users should now be able to specify the host-port of a locally running emulator for firestore and it will work. The work to make this into a test resource can be deferred and grouped with the other work like with spanner emulator.

elefeint suggested changes

View reviewed changes

Contributor

elefeint left a comment

A couple of technicality nits.

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Outdated Show resolved Hide resolved

docs/src/main/asciidoc/firestore.adoc Show resolved Hide resolved

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Outdated

+              	private static Credentials emulatorCredentials() {
+              		final Map<String, List<String>> headerMap = new HashMap<>();
+              		headerMap.put("Authorization", Collections.singletonList("Bearer owner"));
+              		headerMap.put("google-cloud-resource-prefix", Collections.singletonList("projects/my-project/databases/(default)"));

Contributor

elefeint Apr 15, 2020

"my-project" -- is it because it's a fake value that's unused? Can you leave a comment about that?

Contributor Author

dzou Apr 17, 2020

Done. I just changed it to plug in the current project-id. I guess this is more technically correct.

Contributor Author

dzou Apr 17, 2020

Or maybe I should make this so the emulator config doesn't require a real project (and not inject a projectIdProvider into the configuration)... Maybe that would make more sense; what do you think?

Contributor

elefeint Apr 20, 2020

Maybe just hardcode "unused" as project ID?

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Show resolved Hide resolved

meltsufin suggested changes

View reviewed changes

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Outdated Show resolved Hide resolved

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java Show resolved Hide resolved


          PR Comments

f017e12

dzou requested review from meltsufin and elefeint

April 17, 2020 22:24

elefeint previously approved these changes

View reviewed changes

Contributor

elefeint left a comment

Looks good; I don't think the commit changing the project to current default came though, but then perhaps hardcoding a fake value is better anyway.

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Outdated

+              	private static Credentials emulatorCredentials() {
+              		final Map<String, List<String>> headerMap = new HashMap<>();
+              		headerMap.put("Authorization", Collections.singletonList("Bearer owner"));
+              		headerMap.put("google-cloud-resource-prefix", Collections.singletonList("projects/my-project/databases/(default)"));

Contributor

elefeint Apr 20, 2020

Maybe just hardcode "unused" as project ID?

meltsufin suggested changes

View reviewed changes

docs/src/main/asciidoc/firestore.adoc Show resolved Hide resolved

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Outdated Show resolved Hide resolved

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java Show resolved Hide resolved

...springframework/cloud/gcp/autoconfigure/firestore/GcpFirestoreEmulatorAutoConfiguration.java Show resolved Hide resolved

dzou dismissed elefeint’s stale review via

f017e12

April 20, 2020 19:03


          add docs and test

e0589fd

sonarqubecloud bot commented Apr 20, 2020

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
2 Code Smells

89.4% Coverage
0.0% Duplication

dzou requested a review from meltsufin

April 20, 2020 20:58

meltsufin approved these changes

View reviewed changes

Contributor

meltsufin left a comment

LGTM

elefeint approved these changes

View reviewed changes

Contributor

elefeint commented Apr 21, 2020

Can you update the PR description, since you've worked through all the problems by now? I use PR descriptions a lot when doing archaeological digs for why something is.

dzou changed the title ~~[WIP] Setup the Autoconfiguration for Firestore Emulator~~ Setup the Autoconfiguration for Firestore Emulator

Contributor Author

dzou commented Apr 21, 2020

Can you update the PR description, since you've worked through all the problems by now? I use PR descriptions a lot when doing archaeological digs for why something is.

Will do!

dzou merged commit 85ff410 into master

dzou deleted the firestore-emulator-wip branch

April 21, 2020 13:58

batigoal82 mentioned this pull request

Firestore Autoconfiguration emulator not setting the project id #2514

Closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

firestore help wanted