feat: Add support for environment variable FIRESTORE_PREFER_REST (#1848)

MarkDuckworth · web-flow · commit 96b1d2ab3248 · 2023-05-15T13:45:46.000-06:00
Add support for environment variable FIRESTORE_PREFER_REST
diff --git a/dev/src/index.ts b/dev/src/index.ts
@@ -60,6 +60,7 @@ import {
   isPermanentRpcError,
   requestTag,
   wrapError,
+  tryGetPreferRestEnvironmentVariable,
 } from './util';
 import {
   validateBoolean,
@@ -534,7 +535,12 @@ export class Firestore implements firestore.Firestore {
    * for communication from that point forward. Currently the only operation
    * that requires gRPC is creating a snapshot listener with the method
    * `DocumentReference<T>.onSnapshot()`, `CollectionReference<T>.onSnapshot()`, or
-   * `Query<T>.onSnapshot()`.
+   * `Query<T>.onSnapshot()`. If specified, this setting value will take precedent over the
+   * environment variable `FIRESTORE_PREFER_REST`. If not specified, the
+   * SDK will use the value specified in the environment variable `FIRESTORE_PREFER_REST`.
+   * Valid values of `FIRESTORE_PREFER_REST` are `true` ('1') or `false` (`0`). Values are
+   * not case-sensitive. Any other value for the environment variable will be ignored and
+   * a warning will be logged to the console.
    */
   constructor(settings?: firestore.Settings) {
     const libraryHeader = {
@@ -663,6 +669,16 @@ export class Firestore implements firestore.Firestore {
 
     let url: URL | null = null;
 
+    // If preferRest is not specified in settings, but is set as environment variable,
+    // then use the environment variable value.
+    const preferRestEnvValue = tryGetPreferRestEnvironmentVariable();
+    if (settings.preferRest === undefined && preferRestEnvValue !== undefined) {
+      settings = {
+        ...settings,
+        preferRest: preferRestEnvValue,
+      };
+    }
+
     // If the environment variable is set, it should always take precedence
     // over any user passed in settings.
     if (process.env.FIRESTORE_EMULATOR_HOST) {
diff --git a/dev/src/util.ts b/dev/src/util.ts
@@ -217,3 +217,32 @@ export function wrapError(err: Error, stack: string): Error {
   err.stack += '\nCaused by: ' + stack;
   return err;
 }
+
+/**
+ * Parses the value of the environment variable FIRESTORE_PREFER_REST, and
+ * returns a value indicating if the environment variable enables or disables
+ * preferRest.
+ *
+ * This function will warn to the console if the environment variable is set
+ * to an unsupported value.
+ *
+ * @return `true` if the environment variable enables `preferRest`,
+ * `false` if the environment variable disables `preferRest`, or `undefined`
+ * if the environment variable is not set or is set to an unsupported value.
+ */
+export function tryGetPreferRestEnvironmentVariable(): boolean | undefined {
+  const rawValue = process.env.FIRESTORE_PREFER_REST?.trim().toLowerCase();
+  if (rawValue === undefined) {
+    return undefined;
+  } else if (rawValue === '1' || rawValue === 'true') {
+    return true;
+  } else if (rawValue === '0' || rawValue === 'false') {
+    return false;
+  } else {
+    // eslint-disable-next-line no-console
+    console.warn(
+      `An unsupported value was specified for the environment variable FIRESTORE_PREFER_REST. Value ${rawValue} is unsupported.`
+    );
+    return undefined;
+  }
+}
diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts
@@ -84,7 +84,6 @@ if (process.env.NODE_ENV === 'DEBUG') {
 function getTestRoot(settings: Settings = {}) {
   const firestore = new Firestore({
     ...settings,
-    preferRest: !!process.env.USE_REST_FALLBACK,
   });
   return firestore.collection(`node_${version}_${autoId()}`);
 }
diff --git a/dev/test/index.ts b/dev/test/index.ts
@@ -639,6 +639,62 @@ describe('instantiation', () => {
     );
   });
 
+  describe('preferRest configuration', () => {
+    let originalValue: string | undefined;
+
+    beforeEach(() => {
+      originalValue = process.env.FIRESTORE_PREFER_REST;
+    });
+
+    afterEach(() => {
+      if (originalValue === undefined) {
+        delete process.env.FIRESTORE_PREFER_REST;
+      } else {
+        process.env.FIRESTORE_PREFER_REST = originalValue;
+      }
+    });
+
+    it('preferRest is disabled (falsy) by default', async () => {
+      delete process.env.FIRESTORE_PREFER_REST;
+      const firestore = new Firestore.Firestore({});
+      // Convention with settings is to leave undefined settings as
+      // undefined. We could test for undefined here, but converting
+      // to boolean and testing for falsy-ness is consistent with the
+      // code that consumes settings.
+      expect(!!firestore['_settings'].preferRest).to.be.false;
+    });
+
+    it('preferRest can be enabled by setting', async () => {
+      delete process.env.FIRESTORE_PREFER_REST;
+      const firestore = new Firestore.Firestore({
+        preferRest: true,
+      });
+      expect(firestore['_settings'].preferRest).to.be.true;
+    });
+
+    it('preferRest can be enabled by environment variable', async () => {
+      process.env.FIRESTORE_PREFER_REST = 'true';
+      const firestore = new Firestore.Firestore({});
+      expect(firestore['_settings'].preferRest).to.be.true;
+    });
+
+    it('the preferRest value from settings takes precedent over the environment var - disable', async () => {
+      process.env.FIRESTORE_PREFER_REST = 'true';
+      const firestore = new Firestore.Firestore({
+        preferRest: false,
+      });
+      expect(firestore['_settings'].preferRest).to.be.false;
+    });
+
+    it('the preferRest value from settings takes precedent over the environment var - enable', async () => {
+      process.env.FIRESTORE_PREFER_REST = 'false';
+      const firestore = new Firestore.Firestore({
+        preferRest: true,
+      });
+      expect(firestore['_settings'].preferRest).to.be.true;
+    });
+  });
+
   it('exports all types', () => {
     // Ordering as per firestore.d.ts
     expect(Firestore.Firestore).to.exist;
diff --git a/dev/test/util.ts b/dev/test/util.ts
@@ -14,7 +14,8 @@
 
 import {describe, it} from 'mocha';
 import {expect} from 'chai';
-import {isPlainObject} from '../src/util';
+import {isPlainObject, tryGetPreferRestEnvironmentVariable} from '../src/util';
+import * as sinon from 'sinon';
 
 describe('isPlainObject()', () => {
   it('allows Object.create()', () => {
@@ -33,4 +34,71 @@ describe('isPlainObject()', () => {
     expect(isPlainObject(new Foo())).to.be.false;
     expect(isPlainObject(Object.create(new Foo()))).to.be.false;
   });
+
+  describe('tryGetPreferRestEnvironmentVariable', () => {
+    const sandbox = sinon.createSandbox();
+
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    let warnSpy: any;
+    let originalValue: string | undefined;
+
+    beforeEach(() => {
+      warnSpy = sandbox.spy(console, 'warn');
+      originalValue = process.env.FIRESTORE_PREFER_REST;
+    });
+
+    afterEach(() => {
+      sandbox.restore();
+      if (originalValue === undefined) {
+        delete process.env.FIRESTORE_PREFER_REST;
+      } else {
+        process.env.FIRESTORE_PREFER_REST = originalValue;
+      }
+    });
+
+    it('reads true', async () => {
+      process.env.FIRESTORE_PREFER_REST = 'true';
+      expect(tryGetPreferRestEnvironmentVariable()).to.be.true;
+    });
+
+    it('reads 1', async () => {
+      process.env.FIRESTORE_PREFER_REST = '1';
+      expect(tryGetPreferRestEnvironmentVariable()).to.be.true;
+    });
+
+    it('reads false', async () => {
+      process.env.FIRESTORE_PREFER_REST = 'false';
+      expect(tryGetPreferRestEnvironmentVariable()).to.be.false;
+    });
+
+    it('reads 0', async () => {
+      process.env.FIRESTORE_PREFER_REST = '0';
+      expect(tryGetPreferRestEnvironmentVariable()).to.be.false;
+    });
+
+    it('ignores case', async () => {
+      process.env.FIRESTORE_PREFER_REST = 'True';
+      expect(tryGetPreferRestEnvironmentVariable()).to.be.true;
+    });
+
+    it('trims whitespace', async () => {
+      process.env.FIRESTORE_PREFER_REST = '  true  ';
+      expect(tryGetPreferRestEnvironmentVariable()).to.be.true;
+    });
+
+    it('returns undefined when the environment variable is not set', async () => {
+      delete process.env.FIRESTORE_PREFER_REST;
+      expect(tryGetPreferRestEnvironmentVariable()).to.be.undefined;
+      expect(warnSpy.calledOnce).to.be.false;
+    });
+
+    it('returns undefined and warns when the environment variable is set to an unsupported value', async () => {
+      process.env.FIRESTORE_PREFER_REST = 'enable';
+      expect(tryGetPreferRestEnvironmentVariable()).to.be.undefined;
+      expect(warnSpy.calledOnce).to.be.true;
+      expect(warnSpy.getCall(0).args[0]).to.match(
+        /unsupported value.*FIRESTORE_PREFER_REST/
+      );
+    });
+  });
 });
diff --git a/dev/test/util/helpers.ts b/dev/test/util/helpers.ts
@@ -35,7 +35,7 @@ import {GapicClient} from '../../src/types';
 import api = proto.google.firestore.v1;
 
 let SSL_CREDENTIALS: grpc.ChannelCredentials | null = null;
-if (!process.env.USE_REST_FALLBACK) {
+if (!isPreferRest()) {
   const grpc = require('google-gax').grpc;
   SSL_CREDENTIALS = grpc.credentials.createInsecure();
 }
@@ -444,3 +444,16 @@ export async function collect<T, TReturn, TNext>(
   }
   return values;
 }
+
+/**
+ * Returns a value indicating whether preferRest is enabled
+ * via the environment variable `FIRESTORE_PREFER_REST`.
+ *
+ * @return `true` if preferRest is enabled via the environment variable `FIRESTORE_PREFER_REST`.
+ */
+export function isPreferRest(): boolean {
+  return (
+    process.env.FIRESTORE_PREFER_REST === '1' ||
+    process.env.FIRESTORE_PREFER_REST === 'true'
+  );
+}
diff --git a/package.json b/package.json
@@ -30,9 +30,9 @@
   "scripts": {
     "predocs": "npm run compile",
     "docs": "jsdoc -c .jsdoc.js",
-    "system-test:rest": "USE_REST_FALLBACK=YES mocha build/system-test --timeout 600000",
+    "system-test:rest": "FIRESTORE_PREFER_REST=true mocha build/system-test --timeout 600000",
     "system-test:grpc": "mocha build/system-test --timeout 600000",
-    "system-test:emulator:rest": "FIRESTORE_EMULATOR_HOST=localhost:8080 USE_REST_FALLBACK=YES mocha build/system-test --timeout 600000",
+    "system-test:emulator:rest": "FIRESTORE_EMULATOR_HOST=localhost:8080 FIRESTORE_PREFER_REST=true mocha build/system-test --timeout 600000",
     "system-test:emulator:grpc": "FIRESTORE_EMULATOR_HOST=localhost:8080 mocha build/system-test --timeout 600000",
     "system-test": "npm run system-test:grpc && npm run system-test:rest",
     "system-test:emulator": "npm run system-test:emulator:grpc && npm run system-test:emulator:rest",

Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,6 @@ if (process.env.NODE_ENV === 'DEBUG') {`
`84`	`84`	`function getTestRoot(settings: Settings = {}) {`
`85`	`85`	`const firestore = new Firestore({`
`86`	`86`	`...settings,`
`87`		`- preferRest: !!process.env.USE_REST_FALLBACK,`
`88`	`87`	`});`
`89`	`88`	return firestore.collection(`node_${version}_${autoId()}`);
`90`	`89`	`}`