feat: allow `Settings.host` to be used when `Settings.servicePath` is set #1275

thebrianchen · 2020-07-27T22:32:05Z

The existing behavior prioritized FIRESTORE_EMULATOR_HOST over any user-specified host when set, and prohibits setting the host more than once. This PR allows Settings.host to take precedence over Settings.servicePath and Settings.apiEndpoint if FIRESTORE_EMULATOR_HOST is not set and logs a warning when the host is overridden. Otherwise, FIRESTORE_EMULATOR_HOST is used as the host, regardless of any values that are passed via Settings.

codecov · 2020-07-27T22:39:11Z

Codecov Report

Merging #1275 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1275   +/-   ##
=======================================
  Coverage   98.50%   98.50%           
=======================================
  Files          30       30           
  Lines       18612    18617    +5     
  Branches     1431     1431           
=======================================
+ Hits        18333    18338    +5     
  Misses        276      276           
  Partials        3        3

Impacted Files	Coverage Δ
dev/src/index.ts	`97.51% <100.00%> (+<0.01%)`	⬆️

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 c34970d...be70f6b. Read the comment docs.

dev/src/index.ts

schmidt-sebastian · 2020-07-28T16:41:38Z

@thebrianchen Did you look at why this failed? I don't see how this can fail:

nodejs-firestore/dev/src/index.ts

Line 427 in 56355f1

delete emulatorSettings.servicePath;

unsets the servicePath if FIRESTORE_EMULATOR_HOST is set.

nodejs-firestore/dev/src/index.ts

Line 496 in 56355f1

this.validateAndApplySettings(mergedSettings);

doesn't seem to change it again.

schmidt-sebastian · 2020-07-28T23:33:30Z

dev/src/index.ts

+              settings.servicePath !== undefined
+                ? settings.servicePath
+                : settings.apiEndpoint


Suggested change

settings.servicePath !== undefined

? settings.servicePath

: settings.apiEndpoint

settings.servicePath ?? settings.apiEndpoint

thanks, done.

schmidt-sebastian · 2020-07-28T23:35:21Z

dev/src/index.ts

+        (settings.servicePath !== undefined &&
+          settings.servicePath !== settings.host) ||
+        (settings.apiEndpoint !== undefined &&
+          settings.apiEndpoint !== settings.host)


Host is "hostname:port". At least servicePath is just hostname. Can you verify that this works as expected?

Changed logic to compare hostnames.

schmidt-sebastian · 2020-07-28T23:37:17Z

dev/src/index.ts

      delete settings.host;
+      delete settings.apiEndpoint;


I think we can now delete the code that also does this in the constructor.

By the way, I am still a little uneasy about the order of operations here:

process.env.FIRESTORE_EMULATOR_HOST = 'chicago'; const firestore = new Firestore({host: 'sanfrancisco'}); // Now talking to 'chicago' vs: process.env.FIRESTORE_EMULATOR_HOST = 'chicago'; const firestore = new Firestore() firstore.setting({host: 'sanfrancisco'}); // Now talking to 'sanfrancisco'

Maybe all the constructor code should be moved to validateAndApplySettings, where we can then use a unified and sane behavior?

moved the deletion code over to validateAndApplySettings. However, we need to keep the env var check in the constructor in order to preserve existing behavior where the env var overrides anything passed into the constructor.

schmidt-sebastian · 2020-07-30T21:11:46Z

dev/src/index.ts

+    // calls `settings()`, which will compare the the provided `host` to the
+    // existing hostname stored on `servicePath`.
+    delete settings.host;
+    delete settings.apiEndpoint;


This should be moved into the if-statement above, shouldn't it?

good catch, done.

schmidt-sebastian · 2020-07-30T21:14:08Z

dev/src/index.ts

+    let url: URL | null = null;
+
+    // If the environment variable is set, it should always take precedence
+    // over any user passed in settings.


If we feel strongly that this is the wrong thing to do, should add a TODO to fix this?

I don't see how we would fix it without creating an inconvenient breaking change for developers. Since this is only used for emulators and 1p testing, I think the current behavior isn't "wrong". I mainly wanted to document the priority order to make the code easier to understand.

schmidt-sebastian · 2020-07-30T21:17:48Z

dev/src/index.ts

+          settings.apiEndpoint !== url.hostname)
+      ) {
+        // eslint-disable-next-line no-console
+        console.warn(


I am debating if it makes sense to log this warning even when FIRESTORE_EMULATOR_HOST is used. Most of the time, this shouldn't warn since servicePath and apiEndpoint are rarely set.

Yeah, I think that makes more sense. If anything, it will make it easier for developers to figure out what host is actually being used.

schmidt-sebastian · 2020-07-30T21:18:23Z

dev/src/index.ts

    }

+    // We need to remove the `host` and `apiEndpoint` setting, in case a user


Comment no longer applicable.

Isn't this still applicable if the user does not provide the env var? In that case, creating a new Firestore with a host passed in and then calling settings() with host specified would cause this situation.

schmidt-sebastian

LGTM. Please update PR title to match new behavior.

fix: allow 'host' to override existing settings

79510f2

thebrianchen requested a review from a team as a code owner July 27, 2020 22:32

thebrianchen self-assigned this Jul 27, 2020

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 27, 2020

thebrianchen commented Jul 28, 2020

View reviewed changes

dev/src/index.ts Outdated Show resolved Hide resolved

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jul 28, 2020

thebrianchen requested a review from schmidt-sebastian July 28, 2020 15:11

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jul 28, 2020

update error message and delete apiEndpoint

2bd0c8a

thebrianchen changed the title ~~fix: allow 'host' to override existing settings~~ feat: Allow Settings.host to override existing host when calling settings(). Jul 28, 2020

thebrianchen changed the title ~~feat: Allow Settings.host to override existing host when calling settings().~~ feat: allow Settings.host to override existing host when calling settings(). Jul 28, 2020

thebrianchen changed the title ~~feat: allow Settings.host to override existing host when calling settings().~~ feat: allow Settings.host to override existing host when calling settings() Jul 28, 2020

thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 28, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 28, 2020

schmidt-sebastian reviewed Jul 28, 2020

View reviewed changes

Brian Chen added 3 commits July 30, 2020 10:22

refactor settings code

acbabb2

cleanup tests

446ddf0

Merge branch 'master' into bc/host-override

24c4c1f

thebrianchen requested a review from schmidt-sebastian July 30, 2020 15:43

thebrianchen assigned schmidt-sebastian and thebrianchen and unassigned thebrianchen and schmidt-sebastian Jul 30, 2020

Brian Chen added 2 commits July 30, 2020 13:53

make env var take precedence

d09e3a8

typo

97117c7

thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 30, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 30, 2020

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jul 30, 2020

schmidt-sebastian reviewed Jul 30, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen Jul 30, 2020

sebastian comments, add logging in env-var case

be70f6b

thebrianchen requested a review from schmidt-sebastian July 30, 2020 21:58

thebrianchen removed their assignment Jul 30, 2020

schmidt-sebastian approved these changes Jul 30, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jul 30, 2020

thebrianchen changed the title ~~feat: allow Settings.host to override existing host when calling settings()~~ feat: allow Settings.host to be used when Settings.servicePath is set Jul 30, 2020

thebrianchen assigned thebrianchen and unassigned thebrianchen Jul 30, 2020

JustinBeckwith approved these changes Jul 30, 2020

View reviewed changes

thebrianchen merged commit 34d6728 into master Jul 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow `Settings.host` to be used when `Settings.servicePath` is set #1275

feat: allow `Settings.host` to be used when `Settings.servicePath` is set #1275

thebrianchen commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 27, 2020 •

edited

Loading

schmidt-sebastian commented Jul 28, 2020

schmidt-sebastian Jul 28, 2020

thebrianchen Jul 30, 2020

schmidt-sebastian Jul 28, 2020

thebrianchen Jul 30, 2020

schmidt-sebastian Jul 28, 2020

thebrianchen Jul 30, 2020

schmidt-sebastian Jul 30, 2020

thebrianchen Jul 30, 2020

schmidt-sebastian Jul 30, 2020

thebrianchen Jul 30, 2020

schmidt-sebastian Jul 30, 2020

thebrianchen Jul 30, 2020

schmidt-sebastian Jul 30, 2020

thebrianchen Jul 30, 2020

schmidt-sebastian left a comment

		}

		// We need to remove the `host` and `apiEndpoint` setting, in case a user

feat: allow Settings.host to be used when Settings.servicePath is set #1275

feat: allow Settings.host to be used when Settings.servicePath is set #1275

Conversation

thebrianchen commented Jul 27, 2020 • edited Loading

codecov bot commented Jul 27, 2020 • edited Loading

Codecov Report

schmidt-sebastian commented Jul 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

feat: allow `Settings.host` to be used when `Settings.servicePath` is set #1275

feat: allow `Settings.host` to be used when `Settings.servicePath` is set #1275

thebrianchen commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 27, 2020 •

edited

Loading