Unified emulator settings for Database #1672

samtstern · 2020-06-17T16:05:26Z

This is the first PR for http://go/firebase-emulator-connection-api

This implements the global changes needed in FirebaseApp as well as the changes in FirebaseDatabase to support them.

TODO:

Properly hide the FirebaseApp changes so that this doesn't leak before Functions / Firestore are implemented.

firebase-common/src/androidTest/java/com/google/firebase/FirebaseAppTest.java

google-oss-bot · 2020-06-17T16:13:18Z

Binary Size Report

Affected SDKs

firebase-common

Type Base (a7096ac) Head (2a276c10) Diff

aar 34.3 kB 37.6 kB +3.28 kB (+9.6%)
firebase-database

Type Base (a7096ac) Head (2a276c10) Diff

aar 481 kB 482 kB +451 B (+0.1%)

Test Logs

Notes

Head commit (2a276c10) is created by Prow via merging commits: a7096ac b072f54.

google-oss-bot · 2020-06-17T16:13:32Z

The public api surface has changed for the subproject firebase-common:
error: Added method com.google.firebase.FirebaseApp.enableEmulators(com.google.firebase.emulators.EmulatorSettings) [AddedMethod]
error: Added method com.google.firebase.FirebaseApp.getEmulatorSettings() [AddedMethod]
error: Added package com.google.firebase.emulators [AddedPackage]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2020-06-17T16:18:47Z

Coverage Report

Affected SDKs

`firebase-common`

SDK overall coverage changed from 44.86% (a7096ac) to 43.41% (2a276c10) by -1.45%.

Filename	Base (`a7096ac`)	Head (2a276c10)	Diff
EmulatedServiceSettings.java	?	0.00%	?
EmulatorSettings.java	?	46.67%	?
FirebaseApp.java	52.97%	51.42%	-1.56%
FirebaseEmulator.java	?	0.00%	?

`firebase-database`

SDK overall coverage changed from 51.22% (a7096ac) to 51.19% (2a276c10) by -0.04%.

Filename	Base (`a7096ac`)	Head (2a276c10)	Diff
BooleanNode.java	100.00%	92.31%	-7.69%
ChildChangeAccumulator.java	96.77%	83.87%	-12.90%
FirebaseDatabase.java	30.14%	32.00%	+1.86%
Utilities.java	68.99%	69.92%	+0.93%
ViewProcessor.java	92.35%	92.05%	-0.31%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (2a276c10) is created by Prow via merging commits: a7096ac b072f54.

vkryachko

I am wondering whether EmulatedServiceSettings in firebase-common. Wouldn't it make sense to create a dedicated library for it?
The overhead of adding+launching a new library is pretty low these days. wdyt?

samtstern · 2020-06-17T17:10:35Z

@vkryachko well common will always have to depend on these emulator classes and they are useless outside of common so I don't see a reason to break them out. They're just tiny data classes they won't gain any actual functionality.

firebase-common/src/main/java/com/google/firebase/emulators/FirebaseEmulators.java

firebase-common/src/androidTest/java/com/google/firebase/FirebaseAppTest.java

firebase-common/src/main/java/com/google/firebase/FirebaseApp.java

firebase-common/src/main/java/com/google/firebase/emulators/EmulatorSettings.java

firebase-common/src/main/java/com/google/firebase/emulators/FirebaseEmulators.java

vkryachko · 2020-06-18T15:17:42Z

firebase-common/src/main/java/com/google/firebase/FirebaseApp.java

+  @KeepForSdk
+  public void enableEmulators(@NonNull EmulatorSettings emulatorSettings) {
+    checkNotDeleted();
+    Preconditions.checkState(


How will this interact with firestore's terminate()? afaicr one can terminate one instance of firestore at which point the call to FirebaseFirestore#getInstance() will create a new firestore instance, should we allow the following sequence of calls?

firestore.terminate(); app.updateFirestoresEmulator(); Firebase.firestore.use();

Thanks for calling this out, I resolved all the other comments since they were straightforward but I will have to look into this one more deeply.

Ok actually I do not want to allow that sequence of calls. One of the main reasons for this is to make sure that app developers can reason about their emulation settings at a single point before any Firebase service has done any communication.

If you call terminate() the settings should still apply. If you want to nuke them you'll need to call FirebaseApp#delete() and then make a new app.

WDYT?

That's a good point and sgtm. You may want to confirm this behavior with firestore folks though

Will do during the Firestore PR.

schmidt-sebastian

I hope you have some sort of copy/paste text snippet that you can use to rebuke all my comments :)

firebase-common/src/main/java/com/google/firebase/emulators/EmulatedServiceSettings.java

firebase-common/src/main/java/com/google/firebase/emulators/EmulatorSettings.java

schmidt-sebastian · 2020-06-18T19:09:09Z

firebase-database/src/main/java/com/google/firebase/database/core/utilities/Utilities.java

+      if (serviceSettings != null) {
+        repoInfo.secure = false;
+        repoInfo.host = serviceSettings.host + ":" + serviceSettings.port;
+        repoInfo.internalHost = repoInfo.host;


Should we add an explicit way to decide which credentials to use? I am asking because of Yuchen's feedback here: firebase/firebase-js-sdk#3228

I do think that would be a nice addition one day but this API was approved cross-platform for Android, iOS, and JS without any mention of a credentials setting so I think I'd need to do a new API review for that.

Do you anticipate bugs like that JS one in this implementation?

This should be low risk since Android SDK is not reused in Admin SDKs. The expected behavior is the same as production -- get credentials from Firebase Auth if used and logged in, or no credentials if not.

samtstern · 2020-06-18T20:52:18Z

@schmidt-sebastian all of your comments were about the FirebaseApp stuff (and they were useful!) but I added you as a database reviewer.

Is there anyone else I should add?
Do you have any concerns about the database stuff?

firebase-common/src/main/java/com/google/firebase/FirebaseApp.java

firebase-database/src/main/java/com/google/firebase/database/core/utilities/Utilities.java

vkryachko · 2020-06-19T13:12:47Z

firebase-common/src/main/java/com/google/firebase/FirebaseApp.java

@@ -110,6 +111,9 @@
  private final FirebaseOptions options;
  private final ComponentRuntime componentRuntime;

+  private final AtomicBoolean emulatorSettingsFrozen = new AtomicBoolean(false);
+  private EmulatorSettings emulatorSettings = EmulatorSettings.getDefault();


nit: now that settings are immutable, consider sharing the default instance across app instances.

Ah yeah good call!

vkryachko

LGTM overall, a few minor comments

firebase-common/src/main/java/com/google/firebase/emulators/EmulatedServiceSettings.java

firebase-common/src/main/java/com/google/firebase/emulators/EmulatorSettings.java

firebase-common/src/main/java/com/google/firebase/emulators/FirebaseEmulator.java

…ulatedServiceSettings.java Co-authored-by: Vladimir Kryachko <[email protected]>

…rebaseEmulator.java Co-authored-by: Vladimir Kryachko <[email protected]>

…ulatorSettings.java Co-authored-by: Vladimir Kryachko <[email protected]>

vkryachko

LGTM with one final getter nit

firebase-common/src/main/java/com/google/firebase/emulators/EmulatedServiceSettings.java

google-oss-bot · 2020-06-19T14:16:05Z

@samtstern: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
device-check-changed	`b072f54`	link	`/test device-check-changed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

samtstern · 2020-06-19T14:18:03Z

12: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':firebase-firestore:firebase-test-labUpload'.
> Process 'command 'gcloud'' finished with non-zero exit value 10

That's unrelated and this PR was previously green, so gonna merge anyway.

Unified emulator settings for Database

9ef8a4b

google-oss-bot added the do-not-merge/work-in-progress label Jun 17, 2020

googlebot added the cla: yes Override cla label Jun 17, 2020

google-oss-bot added the size/L label Jun 17, 2020

samtstern commented Jun 17, 2020

View reviewed changes

firebase-common/src/androidTest/java/com/google/firebase/FirebaseAppTest.java Outdated Show resolved Hide resolved

API surface

9d4be7d

samtstern marked this pull request as ready for review June 17, 2020 16:24

google-oss-bot removed the do-not-merge/work-in-progress label Jun 17, 2020

samtstern requested review from schmidt-sebastian, vkryachko and yuchenshi June 17, 2020 16:25

vkryachko reviewed Jun 17, 2020

View reviewed changes

Hide new methods

1ca25d4

samtstern commented Jun 18, 2020

View reviewed changes

firebase-common/src/main/java/com/google/firebase/emulators/FirebaseEmulators.java Outdated Show resolved Hide resolved

vkryachko reviewed Jun 18, 2020

View reviewed changes

samtstern added 6 commits June 18, 2020 12:09

Remove the enum, flip the dependency

f39c11f

Google Java Format

0f931bb

No more @KeepForSdk

fb262d3

Respond to most review comments

2762764

Clone the map

24b8f3a

Fix bug arising from shared DEFAULT

4dd3c23

samtstern assigned schmidt-sebastian Jun 18, 2020

samtstern added api: core api: database labels Jun 18, 2020

schmidt-sebastian reviewed Jun 18, 2020

View reviewed changes

schmidt-sebastian assigned samtstern Jun 18, 2020

samtstern removed their assignment Jun 18, 2020

samtstern requested a review from schmidt-sebastian June 18, 2020 19:24

samtstern added 2 commits June 18, 2020 16:47

Change to freeze verbiage, make FirebaseApp responsible for freezing

f7e2832

No more unnecessary builder

4ef9919

Move frozen to FirebaseApp

a2a4cd6

schmidt-sebastian approved these changes Jun 18, 2020

View reviewed changes

Take sebastian's url parsing suggestion

4e1373f

samtstern requested a review from vkryachko June 19, 2020 12:54

vkryachko reviewed Jun 19, 2020

View reviewed changes

samtstern and others added 4 commits June 19, 2020 09:39

Update firebase-common/src/main/java/com/google/firebase/emulators/Em…

7f173e0

…ulatedServiceSettings.java Co-authored-by: Vladimir Kryachko <[email protected]>

Update firebase-common/src/main/java/com/google/firebase/emulators/Fi…

6d1e179

…rebaseEmulator.java Co-authored-by: Vladimir Kryachko <[email protected]>

Update firebase-common/src/main/java/com/google/firebase/emulators/Em…

2d67b78

…ulatorSettings.java Co-authored-by: Vladimir Kryachko <[email protected]>

Final review nits

8a51a73

samtstern requested a review from vkryachko June 19, 2020 13:43

vkryachko approved these changes Jun 19, 2020

View reviewed changes

firebase-common/src/main/java/com/google/firebase/emulators/EmulatedServiceSettings.java Outdated Show resolved Hide resolved

One more getter

b072f54

samtstern merged commit 5e5111b into master Jun 19, 2020

This was referenced Jun 19, 2020

Functions unified emulator settings #1688

Merged

Unified emulator settings for Firestore #1690

Merged

thatfiredev mentioned this pull request Jun 19, 2020

re-generate rtdb's api.txt and fail build if an error occurs parsing the file #1692

Merged

samtstern mentioned this pull request Jun 24, 2020

Un-hide EmulatorSettings API #1712

Merged

2 tasks

firebase locked and limited conversation to collaborators Jul 20, 2020

kaibolay deleted the ss-emulator-settings branch September 14, 2022 17:55

Unified emulator settings for Database #1672

Unified emulator settings for Database #1672

Uh oh!

Conversation

samtstern commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

google-oss-bot commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-common

firebase-database

Test Logs

Notes

Uh oh!

google-oss-bot commented Jun 17, 2020

Uh oh!

google-oss-bot commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-common

firebase-database

Test Logs

Notes

Uh oh!

vkryachko left a comment

Choose a reason for hiding this comment

Uh oh!

samtstern commented Jun 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuchenshi Jun 18, 2020 • 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!

samtstern commented Jun 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkryachko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vkryachko left a comment

samtstern commented Jun 17, 2020 •

edited

Loading

google-oss-bot commented Jun 17, 2020 •

edited

Loading

`firebase-common`

`firebase-database`

google-oss-bot commented Jun 17, 2020 •

edited

Loading

`firebase-common`

`firebase-database`

yuchenshi Jun 18, 2020 •

edited

Loading