diff --git a/dev/src/index.ts b/dev/src/index.ts index 74a81f300..9abb4964d 100644 --- 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.onSnapshot()`, `CollectionReference.onSnapshot()`, or - * `Query.onSnapshot()`. + * `Query.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 index 5a189269c..cf91f4ee0 100644 --- 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 index d87114381..964b832e4 100644 --- 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 index 4fa7d6e93..12df809ec 100644 --- 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 index c4687eef7..7806f74d2 100644 --- 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 index 476804b37..ea9b1802f 100644 --- 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( } 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 index b17b62c14..efbc4033e 100644 --- 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",