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(fs): preferRest app option for Firestore #1901

Merged
merged 8 commits into from Dec 13, 2022
8 changes: 8 additions & 0 deletions etc/firebase-admin.firestore.api.md
Expand Up @@ -82,6 +82,11 @@ export { Firestore }

export { FirestoreDataConverter }

// @public
export interface FirestoreSettings {
lahirumaramba marked this conversation as resolved.
Show resolved Hide resolved
preferRest?: boolean;
}

export { GeoPoint }

// @public
Expand All @@ -94,6 +99,9 @@ export function getFirestore(app: App): Firestore;

export { GrpcStatus }

// @public
export function initializeFirestore(app: App, settings?: FirestoreSettings): Firestore;

export { NestedUpdateFields }

export { OrderByDirection }
Expand Down
59 changes: 53 additions & 6 deletions src/firestore/firestore-internal.ts
Expand Up @@ -23,24 +23,67 @@ import * as validator from '../utils/validator';
import * as utils from '../utils/index';
import { App } from '../app';

/**
* Settings to pass to the Firestore constructor.
*
* @public
*/
export interface FirestoreSettings {
/**
* Use HTTP/1.1 REST transport where possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth clarifying the behavior of this setting.

preferRest will force the use of HTTP/1.1 REST transport until an operation requires GRPC. When an operation requires GRPC, this Firestore client will load dependent GRPC libraries and then use GRPC transport for all communication from that point forward. Currently the only operation that requires GRPC is creating a snapshot listener using onSnapshot(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

*
* `preferRest` will force the use of HTTP/1.1 REST transport until a method
* that requires gRPC is called. When a method requires gRPC, this Firestore
* client will load dependent gRPC libraries and then use gRPC transport for
* all communication from that point forward. Currently the only operation
* that requires gRPC is creating a snapshot listener using `onSnapshot()`.
*
* @defaultValue `undefined`
*/
preferRest?: boolean;
}

export class FirestoreService {

private readonly appInternal: App;
private readonly databases: Map<string, Firestore> = new Map();
private readonly firestoreSettings: Map<string, FirestoreSettings> = new Map();

constructor(app: App) {
this.appInternal = app;
}

getDatabase(databaseId: string): Firestore {
getDatabase(databaseId: string, settings?: FirestoreSettings): Firestore {
settings ??= {};
let database = this.databases.get(databaseId);
if (database === undefined) {
database = initFirestore(this.app, databaseId);
database = initFirestore(this.app, databaseId, settings);
this.databases.set(databaseId, database);
this.firestoreSettings.set(databaseId, settings);
} else {
if (!this.checkIfSameSettings(databaseId, settings)) {
throw new FirebaseFirestoreError({
code: 'failed-precondition',
message: 'initializeFirestore() has already been called with ' +
'different options. To avoid this error, call initializeFirestore() with the ' +
'same options as when it was originally called, or call getFirestore() to return the' +
' already initialized instance.'
});
}
}
return database;
}

private checkIfSameSettings(databaseId: string, firestoreSettings: FirestoreSettings): boolean {
// If we start passing more settings to Firestore constructor,
// replace this with deep equality check.
const existingSettings = this.firestoreSettings.get(databaseId);
if (!existingSettings) {
return true;
}
return (existingSettings.preferRest === firestoreSettings.preferRest);
}

/**
* Returns the app associated with this Storage instance.
*
Expand All @@ -51,7 +94,7 @@ export class FirestoreService {
}
}

export function getFirestoreOptions(app: App): Settings {
export function getFirestoreOptions(app: App, firestoreSettings?: FirestoreSettings): Settings {
if (!validator.isNonNullObject(app) || !('options' in app)) {
throw new FirebaseFirestoreError({
code: 'invalid-argument',
Expand All @@ -63,6 +106,7 @@ export function getFirestoreOptions(app: App): Settings {
const credential = app.options.credential;
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { version: firebaseVersion } = require('../../package.json');
const preferRest = firestoreSettings?.preferRest;
lahirumaramba marked this conversation as resolved.
Show resolved Hide resolved
if (credential instanceof ServiceAccountCredential) {
return {
credentials: {
Expand All @@ -73,12 +117,15 @@ export function getFirestoreOptions(app: App): Settings {
// guaranteed to be available.
projectId: projectId!,
firebaseVersion,
preferRest,
};
} else if (isApplicationDefault(app.options.credential)) {
// Try to use the Google application default credentials.
// If an explicit project ID is not available, let Firestore client discover one from the
// environment. This prevents the users from having to set GOOGLE_CLOUD_PROJECT in GCP runtimes.
return validator.isNonEmptyString(projectId) ? { projectId, firebaseVersion } : { firebaseVersion };
return validator.isNonEmptyString(projectId)
? { projectId, firebaseVersion, preferRest }
: { firebaseVersion, preferRest };
}

throw new FirebaseFirestoreError({
Expand All @@ -89,8 +136,8 @@ export function getFirestoreOptions(app: App): Settings {
});
}

function initFirestore(app: App, databaseId: string): Firestore {
const options = getFirestoreOptions(app);
function initFirestore(app: App, databaseId: string, firestoreSettings?: FirestoreSettings): Firestore {
const options = getFirestoreOptions(app, firestoreSettings);
options.databaseId = databaseId;
let firestoreDatabase: typeof Firestore;
try {
Expand Down
51 changes: 49 additions & 2 deletions src/firestore/index.ts
Expand Up @@ -23,7 +23,7 @@
import { Firestore } from '@google-cloud/firestore';
import { App, getApp } from '../app';
import { FirebaseApp } from '../app/firebase-app';
import { FirestoreService } from './firestore-internal';
import { FirestoreService, FirestoreSettings } from './firestore-internal';
import { DEFAULT_DATABASE_ID } from '@google-cloud/firestore/build/src/path';

export {
Expand Down Expand Up @@ -71,6 +71,8 @@ export {
setLogFunction,
} from '@google-cloud/firestore';

export { FirestoreSettings };

/**
* Gets the {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the default app.
Expand Down Expand Up @@ -105,7 +107,7 @@ export function getFirestore(): Firestore;
* const otherFirestore = getFirestore(app);
* ```
*
* @param App - whose `Firestore` service to
* @param App - which `Firestore` service to
* return. If not provided, the default `Firestore` service will be returned.
*
* @returns The default {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
Expand Down Expand Up @@ -139,3 +141,48 @@ export function getFirestore(
'firestore', (app) => new FirestoreService(app));
return firestoreService.getDatabase(databaseId);
}

/**
* Gets the {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the given app, passing extra parameters to its constructor.
*
* @example
* ```javascript
* // Get the Firestore service for a specific app, require HTTP/1.1 REST transport
* const otherFirestore = initializeFirestore(app, {preferRest: true});
* ```
*
* @param App - which `Firestore` service to
* return. If not provided, the default `Firestore` service will be returned.
*
* @param settings - Settings object to be passed to the constructor.
*
* @returns The `Firestore` service associated with the provided app and settings.
*/
export function initializeFirestore(app: App, settings?: FirestoreSettings): Firestore;

/**
* @param app
* @param settings
* @param databaseId
* @internal
*/
export function initializeFirestore(
app: App,
settings: FirestoreSettings,
databaseId: string
): Firestore;
lahirumaramba marked this conversation as resolved.
Show resolved Hide resolved

export function initializeFirestore(
app: App,
settings?: FirestoreSettings,
databaseId?: string
): Firestore {
settings ??= {};
databaseId ??= DEFAULT_DATABASE_ID;
const firebaseApp: FirebaseApp = app as FirebaseApp;
const firestoreService = firebaseApp.getOrInitService(
'firestore', (app) => new FirestoreService(app));

return firestoreService.getDatabase(databaseId, settings);
}
7 changes: 6 additions & 1 deletion test/integration/firestore.spec.ts
Expand Up @@ -20,7 +20,7 @@ import { clone } from 'lodash';
import * as admin from '../../lib/index';
import {
DocumentReference, DocumentSnapshot, FieldValue, Firestore, FirestoreDataConverter,
QueryDocumentSnapshot, Timestamp, getFirestore, setLogFunction,
QueryDocumentSnapshot, Timestamp, getFirestore, initializeFirestore, setLogFunction,
} from '../../lib/firestore/index';

chai.should();
Expand All @@ -47,6 +47,11 @@ describe('admin.firestore', () => {
expect(firestore).to.not.be.undefined;
});

it('initializeFirestore returns a Firestore client', () => {
const firestore: Firestore = initializeFirestore(admin.app());
expect(firestore).to.not.be.undefined;
});

it('admin.firestore() returns a Firestore client', () => {
const firestore: admin.firestore.Firestore = admin.firestore();
expect(firestore).to.not.be.undefined;
Expand Down
12 changes: 12 additions & 0 deletions test/unit/firestore/firestore.spec.ts
Expand Up @@ -200,4 +200,16 @@ describe('Firestore', () => {
});
});
});

describe('options.preferRest', () => {
it('should not enable preferRest by default', () => {
const options = getFirestoreOptions(mockApp);
expect(options.preferRest).to.be.undefined;
});

it('should enable preferRest if provided', () => {
const options = getFirestoreOptions(mockApp, { preferRest: true });
expect(options.preferRest).to.be.true;
});
});
});
51 changes: 50 additions & 1 deletion test/unit/firestore/index.spec.ts
Expand Up @@ -23,7 +23,7 @@ import * as chaiAsPromised from 'chai-as-promised';

import * as mocks from '../../resources/mocks';
import { App } from '../../../src/app/index';
import { getFirestore, Firestore } from '../../../src/firestore/index';
import { getFirestore, initializeFirestore, Firestore } from '../../../src/firestore/index';
import { DEFAULT_DATABASE_ID } from '@google-cloud/firestore/build/src/path';

chai.should();
Expand Down Expand Up @@ -86,4 +86,53 @@ describe('Firestore', () => {
expect(db1).to.not.equal(db2);
});
});

describe('initializeFirestore()', () => {
it('should reject given an invalid credential without project ID', () => {
// Project ID not set in the environment.
delete process.env.GOOGLE_CLOUD_PROJECT;
delete process.env.GCLOUD_PROJECT;
expect(() => initializeFirestore(mockCredentialApp)).to.throw(noProjectIdError);
});

it('should not throw given a valid app', () => {
expect(() => {
return initializeFirestore(mockApp);
}).not.to.throw();
});

it('should return the same instance for a given app instance', () => {
const db1: Firestore = initializeFirestore(mockApp);
const db2: Firestore = initializeFirestore(mockApp, {}, DEFAULT_DATABASE_ID);
expect(db1).to.equal(db2);
});

it('should return the same instance for a given app instance and databaseId', () => {
const db1: Firestore = initializeFirestore(mockApp, {}, 'db');
const db2: Firestore = initializeFirestore(mockApp, {}, 'db');
expect(db1).to.equal(db2);
});

it('should return the different instance for given same app instance, but different databaseId', () => {
const db0: Firestore = initializeFirestore(mockApp, {}, DEFAULT_DATABASE_ID);
const db1: Firestore = initializeFirestore(mockApp, {}, 'db1');
const db2: Firestore = initializeFirestore(mockApp, {}, 'db2');
expect(db0).to.not.equal(db1);
expect(db0).to.not.equal(db2);
expect(db1).to.not.equal(db2);
});

it('getFirestore should return the same instance as initializeFirestore returned earlier', () => {
const db1: Firestore = initializeFirestore(mockApp, {}, 'db');
const db2: Firestore = getFirestore(mockApp, 'db');
expect(db1).to.equal(db2);
});

it('initializeFirestore should not allow create an instance with different settings', () => {
initializeFirestore(mockApp, {}, 'db');
expect(() => {
return initializeFirestore(mockApp, { preferRest: true }, 'db');
}).to.throw(/has already been called with different options/);
});
});
});