-
Notifications
You must be signed in to change notification settings - Fork 391
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -25,14 +25,22 @@ import { App } from '../app'; | |||||
|
||||||
export class FirestoreService { | ||||||
|
||||||
private appInternal: App; | ||||||
private firestoreClient: Firestore; | ||||||
private readonly appInternal: App; | ||||||
private 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) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or we could also do There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Just a note, using |
||||||
database = initFirestore(this.app, databaseId); | ||||||
this.databases.set(databaseId, database); | ||||||
} | ||||||
return database; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Returns the app associated with this Storage instance. | ||||||
* | ||||||
|
@@ -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 { | ||||||
|
@@ -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. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,7 +71,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 | ||
|
@@ -82,6 +82,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 | ||
|
@@ -96,13 +110,29 @@ 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; | ||
tom-andersen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* @param databaseId | ||
* @internal | ||
*/ | ||
export function getFirestore(databaseId: string): Firestore; | ||
tom-andersen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* @param app | ||
* @param databaseId | ||
* @internal | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 According to this doc, we should not guard against invalid types. |
||
const databaseId = | ||
typeof appOrDatabaseId === 'string' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to check if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I think we should consider an empty string equivalent to the default database id. |
||
? appOrDatabaseId | ||
: optionalDatabaseId || '(default)'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I have imported it from Server SDK (though it is not part of public API). |
||
const firebaseApp: FirebaseApp = app as FirebaseApp; | ||
const firestoreService = firebaseApp.getOrInitService( | ||
'firestore', (app) => new FirestoreService(app)); | ||
return firestoreService.client; | ||
return firestoreService.getDatabase(databaseId); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,23 +98,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)') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imported from Server SDK |
||
}).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)') | ||
}).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)'); | ||
}).to.throw(invalidCredError); | ||
}); | ||
|
||
|
@@ -123,13 +125,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)'); | ||
}).to.throw(invalidCredError); | ||
}); | ||
|
||
it('should not throw given a valid app', () => { | ||
expect(() => { | ||
return new FirestoreService(mockApp); | ||
return new FirestoreService(mockApp).getDatabase('(default)'); | ||
}).not.to.throw(); | ||
}); | ||
|
||
|
@@ -139,7 +141,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)'); | ||
}).not.to.throw(); | ||
}); | ||
}); | ||
|
@@ -158,19 +160,6 @@ describe('Firestore', () => { | |
}); | ||
}); | ||
|
||
describe('client', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be a potential breaking change in the public API? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Uh oh!
There was an error while loading. Please reload this page.