Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for environment variable FIRESTORE_PREFER_REST #1848

Merged
merged 6 commits into from
May 15, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion dev/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import {
isPermanentRpcError,
requestTag,
wrapError,
tryGetPreferRestEnvironmentVariable,
} from './util';
import {
validateBoolean,
Expand Down Expand Up @@ -534,7 +535,10 @@ 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 environment variable `FIRESTORE_PREFER_REST=true|false`. And if the
dconeybe marked this conversation as resolved.
Show resolved Hide resolved
* environment variable is also not set, then the setting will default to `false`.
*/
constructor(settings?: firestore.Settings) {
const libraryHeader = {
Expand Down Expand Up @@ -663,6 +667,17 @@ 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 [preferRestEnvIsSet, preferRestEnvValue] =
tryGetPreferRestEnvironmentVariable();
if (settings.preferRest === undefined && preferRestEnvIsSet) {
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) {
Expand Down
26 changes: 26 additions & 0 deletions dev/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,3 +217,29 @@ export function wrapError(err: Error, stack: string): Error {
err.stack += '\nCaused by: ' + stack;
return err;
}

/**
* Returns a tuple indicating if FIRESTORE_PREFER_REST is set to a valid value,
* and if so, whether the value enables preferRest or disables preferRest.
*
* @return A tuple where the first value indicates if the environment variable is set to a valid value,
* and the second value indicates if the environment variable enables preferRest or disables preferRest.
*/
export function tryGetPreferRestEnvironmentVariable():
| [false, undefined]
dconeybe marked this conversation as resolved.
Show resolved Hide resolved
| [true, boolean] {
const rawValue = process.env.FIRESTORE_PREFER_REST?.trim().toLowerCase();
if (rawValue === undefined) {
return [false, undefined];
} else if (rawValue === '1' || rawValue === 'true') {
return [true, true];
} else if (rawValue === '0' || rawValue === 'false') {
return [true, 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 [false, undefined];
}
}
1 change: 0 additions & 1 deletion dev/system-test/firestore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()}`);
}
Expand Down
52 changes: 52 additions & 0 deletions dev/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,58 @@ 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({});
expect(!!firestore['_settings'].preferRest).to.be.false;
dconeybe marked this conversation as resolved.
Show resolved Hide resolved
});

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;
Expand Down
79 changes: 78 additions & 1 deletion dev/test/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()', () => {
Expand All @@ -33,4 +34,80 @@ 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';
const [prevSet, prevValue] = tryGetPreferRestEnvironmentVariable();
expect(prevSet).to.be.true;
expect(prevValue).to.be.true;
});

it('reads 1', async () => {
process.env.FIRESTORE_PREFER_REST = '1';
const [prevSet, prevValue] = tryGetPreferRestEnvironmentVariable();
expect(prevSet).to.be.true;
expect(prevValue).to.be.true;
});

it('reads false', async () => {
process.env.FIRESTORE_PREFER_REST = 'false';
const [prevSet, prevValue] = tryGetPreferRestEnvironmentVariable();
expect(prevSet).to.be.true;
expect(prevValue).to.be.false;
});

it('reads 0', async () => {
process.env.FIRESTORE_PREFER_REST = '0';
const [prevSet, prevValue] = tryGetPreferRestEnvironmentVariable();
expect(prevSet).to.be.true;
expect(prevValue).to.be.false;
});

it('ignores case', async () => {
dconeybe marked this conversation as resolved.
Show resolved Hide resolved
process.env.FIRESTORE_PREFER_REST = 'True';
const [prevSet, prevValue] = tryGetPreferRestEnvironmentVariable();
expect(prevSet).to.be.true;
expect(prevValue).to.be.true;
});

it('indicates when the environment variable is not set', async () => {
dconeybe marked this conversation as resolved.
Show resolved Hide resolved
delete process.env.FIRESTORE_PREFER_REST;
const [prevSet, prevValue] = tryGetPreferRestEnvironmentVariable();
expect(prevSet).to.be.false;
expect(prevValue).to.be.undefined;
expect(warnSpy.calledOnce).to.be.false;
});

it('indicates when the environment variable is set to an unsupported value', async () => {
process.env.FIRESTORE_PREFER_REST = 'enable';
const [prevSet, prevValue] = tryGetPreferRestEnvironmentVariable();
expect(prevSet).to.be.false;
expect(prevValue).to.be.undefined;
expect(warnSpy.calledOnce).to.be.true;
expect(warnSpy.getCall(0).args[0]).to.match(
/unsupported value.*FIRESTORE_PREFER_REST/
);
});
});
});
15 changes: 14 additions & 1 deletion dev/test/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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'
);
}
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down