Skip to content

Internal implementation of multiDb #1949

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

Merged
merged 3 commits into from
Oct 26, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 4 additions & 1 deletion etc/firebase-admin.firestore.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,13 @@ export { FirestoreDataConverter }

export { GeoPoint }

// @public
export function getFirestore(): Firestore;

// Warning: (ae-forgotten-export) The symbol "App" needs to be exported by the entry point index.d.ts
//
// @public
export function getFirestore(app?: App): Firestore;
export function getFirestore(app: App): Firestore;

export { GrpcStatus }

Expand Down
21 changes: 13 additions & 8 deletions src/firestore/firestore-internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,22 @@ import { App } from '../app';

export class FirestoreService {

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

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

getDatabase(databaseId: string): Firestore {
let database = this.databases.get(databaseId);
if (database === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (database === undefined) {
if (typeof database === undefined) {

Copy link
Member

Choose a reason for hiding this comment

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

Or we could also do if (!this.databases.has(databaseId)) { just a suggestion really. I will leave it up to you. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any of these solutions are valid. But I have a preference towards existing for a couple of reasons.

  • conciseness
  • performance (though only a small difference, calling has followed by get does consume more resources)
  • correctness (even if map has value, it could be 'undefined')

Just a note, using typeof will require putting quotes around undefined.

database = initFirestore(this.app, databaseId);
this.databases.set(databaseId, database);
}
return database;
}

/**
* Returns the app associated with this Storage instance.
*
Expand All @@ -41,10 +49,6 @@ export class FirestoreService {
get app(): App {
return this.appInternal;
}

get client(): Firestore {
return this.firestoreClient;
}
}

export function getFirestoreOptions(app: App): Settings {
Expand Down Expand Up @@ -85,8 +89,9 @@ export function getFirestoreOptions(app: App): Settings {
});
}

function initFirestore(app: App): Firestore {
function initFirestore(app: App, databaseId: string): Firestore {
const options = getFirestoreOptions(app);
options.databaseId = databaseId;
let firestoreDatabase: typeof Firestore;
try {
// Lazy-load the Firestore implementation here, which in turns loads gRPC.
Expand Down
44 changes: 38 additions & 6 deletions src/firestore/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { Firestore } from '@google-cloud/firestore';
import { App, getApp } from '../app';
import { FirebaseApp } from '../app/firebase-app';
import { FirestoreService } from './firestore-internal';
import { DEFAULT_DATABASE_ID } from '@google-cloud/firestore/build/src/path';

export {
AddPrefixToKeys,
Expand Down Expand Up @@ -71,7 +72,7 @@ export {

/**
* Gets the {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the default app or a given app.
* service for the default app.
*
* `getFirestore()` can be called with no arguments to access the default
* app's `Firestore` service or as `getFirestore(app)` to access the
Expand All @@ -82,6 +83,20 @@ export {
* // Get the Firestore service for the default app
* const defaultFirestore = getFirestore();
* ```

* @returns The default {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service if no app is provided or the `Firestore` service associated with the
* provided app.
*/
export function getFirestore(): Firestore;

/**
* Gets the {@link https://googleapis.dev/nodejs/firestore/latest/Firestore.html | Firestore}
* service for the given app.
*
* `getFirestore()` can be called with no arguments to access the default
* app's `Firestore` service or as `getFirestore(app)` to access the
* `Firestore` service associated with a specific app.
*
* @example
* ```javascript
Expand All @@ -96,13 +111,30 @@ export {
* service if no app is provided or the `Firestore` service associated with the
* provided app.
*/
export function getFirestore(app?: App): Firestore {
if (typeof app === 'undefined') {
app = getApp();
}
export function getFirestore(app: App): Firestore;

/**
* @param databaseId
* @internal
*/
export function getFirestore(databaseId: string): Firestore;

/**
* @param app
* @param databaseId
* @internal
Copy link
Member

Choose a reason for hiding this comment

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

@internal will hide this from both reference docs and autocomplete (it will remove the typescript annotation). Is that what we expect here? If we only want to hide from reference docs and would like to keep autocomplete we can use @alpha instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel safer to keep this completely hidden for the moment. We have a follow up task to expose API.

*/
export function getFirestore(app: App, databaseId: string): Firestore;

export function getFirestore(
appOrDatabaseId?: App | string,
optionalDatabaseId?: string
): Firestore {
const app: App = typeof appOrDatabaseId === 'object' ? appOrDatabaseId : getApp();
Copy link
Member

Choose a reason for hiding this comment

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

I think type of null is also object... we might want to check if app !== null as well. Alternatively, you can also move the implementations into the respective functions separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They would need to bypass Typescript type checking for this to happen, since appOrDatabaseId?: App | string.

According to this doc, we should not guard against invalid types.
https://g3doc.corp.google.com/firebase/g3doc/aip/3270.md?cl=head#input-validation

const databaseId =
(typeof appOrDatabaseId === 'string' ? appOrDatabaseId : optionalDatabaseId) || DEFAULT_DATABASE_ID;
const firebaseApp: FirebaseApp = app as FirebaseApp;
const firestoreService = firebaseApp.getOrInitService(
'firestore', (app) => new FirestoreService(app));
return firestoreService.client;
return firestoreService.getDatabase(databaseId);
}
28 changes: 9 additions & 19 deletions test/unit/firestore/firestore.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
ComputeEngineCredential, RefreshTokenCredential
} from '../../../src/app/credential-internal';
import { FirestoreService, getFirestoreOptions } from '../../../src/firestore/firestore-internal';
import { DEFAULT_DATABASE_ID } from '@google-cloud/firestore/build/src/path';

describe('Firestore', () => {
let mockApp: FirebaseApp;
Expand Down Expand Up @@ -98,23 +99,25 @@ describe('Firestore', () => {
it(`should throw given invalid app: ${ JSON.stringify(invalidApp) }`, () => {
expect(() => {
const firestoreAny: any = FirestoreService;
return new firestoreAny(invalidApp);
const firestoreService: FirestoreService = new firestoreAny(invalidApp);
return firestoreService.getDatabase(DEFAULT_DATABASE_ID)
}).to.throw('First argument passed to admin.firestore() must be a valid Firebase app instance.');
});
});

it('should throw given no app', () => {
expect(() => {
const firestoreAny: any = FirestoreService;
return new firestoreAny();
const firestoreService: FirestoreService = new firestoreAny();
return firestoreService.getDatabase(DEFAULT_DATABASE_ID)
}).to.throw('First argument passed to admin.firestore() must be a valid Firebase app instance.');
});

it('should throw given an invalid credential with project ID', () => {
// Project ID is read from the environment variable, but the credential is unsupported.
process.env.GOOGLE_CLOUD_PROJECT = 'project_id';
expect(() => {
return new FirestoreService(mockCredentialApp);
return new FirestoreService(mockCredentialApp).getDatabase(DEFAULT_DATABASE_ID);
}).to.throw(invalidCredError);
});

Expand All @@ -123,13 +126,13 @@ describe('Firestore', () => {
delete process.env.GOOGLE_CLOUD_PROJECT;
delete process.env.GCLOUD_PROJECT;
expect(() => {
return new FirestoreService(mockCredentialApp);
return new FirestoreService(mockCredentialApp).getDatabase(DEFAULT_DATABASE_ID);
}).to.throw(invalidCredError);
});

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

Expand All @@ -139,7 +142,7 @@ describe('Firestore', () => {
delete process.env.GOOGLE_CLOUD_PROJECT;
delete process.env.GCLOUD_PROJECT;
expect(() => {
return new FirestoreService(config.app);
return new FirestoreService(config.app).getDatabase(DEFAULT_DATABASE_ID);
}).not.to.throw();
});
});
Expand All @@ -158,19 +161,6 @@ describe('Firestore', () => {
});
});

describe('client', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be a potential breaking change in the public API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a private variable of an internal class. No one should rely on this externally.

it('returns the client from the constructor', () => {
// We expect referential equality here
expect(firestore.client).to.not.be.null;
});

it('is read-only', () => {
expect(() => {
firestore.client = mockApp;
}).to.throw('Cannot set property client of #<FirestoreService> which has only a getter');
});
});

describe('options.projectId', () => {
it('should return a string when project ID is present in credential', () => {
const options = getFirestoreOptions(mockApp);
Expand Down
18 changes: 17 additions & 1 deletion test/unit/firestore/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,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 { DEFAULT_DATABASE_ID } from '@google-cloud/firestore/build/src/path';

chai.should();
chai.use(sinonChai);
Expand Down Expand Up @@ -66,8 +67,23 @@ describe('Firestore', () => {

it('should return the same instance for a given app instance', () => {
const db1: Firestore = getFirestore(mockApp);
const db2: Firestore = getFirestore(mockApp);
const db2: Firestore = getFirestore(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 = getFirestore(mockApp, 'db');
const db2: Firestore = getFirestore(mockApp, 'db');
expect(db1).to.equal(db2);
});

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