From ee2a82eb880bbaf2933e071b62eb78a5acd7c711 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Wed, 8 Sep 2021 13:12:41 -0700 Subject: [PATCH 1/3] fix: Decoupled FirebaseNamespace from new module entry points --- etc/firebase-admin.app.api.md | 4 +- src/app/firebase-app.ts | 148 ++--------------- src/app/firebase-namespace.ts | 199 +++++++++++------------ src/app/lifecycle.ts | 154 +++++++++++++++--- src/default-namespace.ts | 2 +- src/instance-id/instance-id.ts | 4 +- test/resources/mocks.ts | 17 +- test/unit/app/firebase-app.spec.ts | 25 ++- test/unit/app/firebase-namespace.spec.ts | 60 ------- test/unit/app/index.spec.ts | 14 +- test/unit/firebase.spec.ts | 27 ++- test/unit/utils.ts | 6 +- 12 files changed, 282 insertions(+), 378 deletions(-) diff --git a/etc/firebase-admin.app.api.md b/etc/firebase-admin.app.api.md index 13b925d89e..a6ad65cd57 100644 --- a/etc/firebase-admin.app.api.md +++ b/etc/firebase-admin.app.api.md @@ -54,7 +54,7 @@ export interface FirebaseError { } // @public (undocumented) -export function getApp(name?: string): App; +export function getApp(appName?: string): App; // @public (undocumented) export function getApps(): App[]; @@ -68,7 +68,7 @@ export interface GoogleOAuthAccessToken { } // @public (undocumented) -export function initializeApp(options?: AppOptions, name?: string): App; +export function initializeApp(options?: AppOptions, appName?: string): App; // @public export function refreshToken(refreshTokenPathOrObject: string | object, httpAgent?: Agent): Credential; diff --git a/src/app/firebase-app.ts b/src/app/firebase-app.ts index 920dce3323..42ce2fd6dc 100644 --- a/src/app/firebase-app.ts +++ b/src/app/firebase-app.ts @@ -15,34 +15,16 @@ * limitations under the License. */ -import { AppOptions, app } from '../firebase-namespace-api'; +import { AppOptions, App } from './core'; +import { AppStore } from './lifecycle'; import { Credential } from './credential'; import { getApplicationDefault } from './credential-internal'; import * as validator from '../utils/validator'; import { deepCopy } from '../utils/deep-copy'; -import { FirebaseNamespaceInternals } from './firebase-namespace'; import { AppErrorCodes, FirebaseAppError } from '../utils/error'; -import { Auth } from '../auth/index'; -import { AppCheck } from '../app-check/index'; -import { MachineLearning } from '../machine-learning/index'; -import { Messaging } from '../messaging/index'; -import { Storage } from '../storage/index'; -import { Database } from '../database/index'; -import { Firestore } from '../firestore/index'; -import { InstanceId } from '../instance-id/index'; -import { Installations } from '../installations/index'; -import { ProjectManagement } from '../project-management/index'; -import { SecurityRules } from '../security-rules/index'; -import { RemoteConfig } from '../remote-config/index'; - const TOKEN_EXPIRY_THRESHOLD_MILLIS = 5 * 60 * 1000; -/** - * Type representing a callback which is called every time an app lifecycle event occurs. - */ -export type AppHook = (event: string, app: app.App) => void; - /** * Type representing a Firebase OAuth access token (derived from a Google OAuth2 access token) which * can be used to authenticate to Firebase services such as the Realtime Database and Auth. @@ -159,7 +141,8 @@ export class FirebaseAppInternals { * * @internal */ -export class FirebaseApp implements app.App { +export class FirebaseApp implements App { + public INTERNAL: FirebaseAppInternals; private name_: string; @@ -167,7 +150,7 @@ export class FirebaseApp implements app.App { private services_: {[name: string]: unknown} = {}; private isDeleted_ = false; - constructor(options: AppOptions, name: string, private firebaseInternals_: FirebaseNamespaceInternals) { + constructor(options: AppOptions, name: string, private readonly appStore?: AppStore) { this.name_ = name; this.options_ = deepCopy(options); @@ -197,121 +180,6 @@ export class FirebaseApp implements app.App { this.INTERNAL = new FirebaseAppInternals(credential); } - /** - * Returns the AppCheck service instance associated with this app. - * - * @returns The AppCheck service instance of this app. - */ - public appCheck(): AppCheck { - const fn = require('../app-check/index').getAppCheck; - return fn(this); - } - - /** - * Returns the Auth service instance associated with this app. - * - * @returns The Auth service instance of this app. - */ - public auth(): Auth { - const fn = require('../auth/index').getAuth; - return fn(this); - } - - /** - * Returns the Database service for the specified URL, and the current app. - * - * @returns The Database service instance of this app. - */ - public database(url?: string): Database { - const fn = require('../database/index').getDatabaseWithUrl; - return fn(url, this); - } - - /** - * Returns the Messaging service instance associated with this app. - * - * @returns The Messaging service instance of this app. - */ - public messaging(): Messaging { - const fn = require('../messaging/index').getMessaging; - return fn(this); - } - - /** - * Returns the Storage service instance associated with this app. - * - * @returns The Storage service instance of this app. - */ - public storage(): Storage { - const fn = require('../storage/index').getStorage; - return fn(this); - } - - public firestore(): Firestore { - const fn = require('../firestore/index').getFirestore; - return fn(this); - } - - /** - * Returns the InstanceId service instance associated with this app. - * - * @returns The InstanceId service instance of this app. - */ - public instanceId(): InstanceId { - const fn = require('../instance-id/index').getInstanceId; - return fn(this); - } - - /** - * Returns the InstanceId service instance associated with this app. - * - * @returns The InstanceId service instance of this app. - */ - public installations(): Installations { - const fn = require('../installations/index').getInstallations; - return fn(this); - } - - /** - * Returns the MachineLearning service instance associated with this app. - * - * @returns The Machine Learning service instance of this app - */ - public machineLearning(): MachineLearning { - const fn = require('../machine-learning/index').getMachineLearning; - return fn(this); - } - - /** - * Returns the ProjectManagement service instance associated with this app. - * - * @returns The ProjectManagement service instance of this app. - */ - public projectManagement(): ProjectManagement { - const fn = require('../project-management/index').getProjectManagement; - return fn(this); - } - - /** - * Returns the SecurityRules service instance associated with this app. - * - * @returns The SecurityRules service instance of this app. - */ - public securityRules(): SecurityRules { - const fn = require('../security-rules/index').getSecurityRules; - return fn(this); - } - - /** - * Returns the RemoteConfig service instance associated with this app. - * - * @returns The RemoteConfig service instance of this app. - */ - public remoteConfig(): RemoteConfig { - const fn = require('../remote-config/index').getRemoteConfig; - return fn(this); - } - /** * Returns the name of the FirebaseApp instance. * @@ -346,7 +214,11 @@ export class FirebaseApp implements app.App { */ public delete(): Promise { this.checkDestroyed_(); - this.firebaseInternals_.removeApp(this.name_); + + // Also remove the instance from the AppStore. This is needed to support the existing + // app.delete() use case. In the future we can remove this API, and deleteApp() will + // become the only way to tear down an App. + this.appStore?.removeApp(this.name); return Promise.all(Object.keys(this.services_).map((serviceName) => { const service = this.services_[serviceName]; diff --git a/src/app/firebase-namespace.ts b/src/app/firebase-namespace.ts index f39eed6b9d..44716e0cae 100644 --- a/src/app/firebase-namespace.ts +++ b/src/app/firebase-namespace.ts @@ -15,18 +15,13 @@ * limitations under the License. */ -import fs = require('fs'); - -import { AppErrorCodes, FirebaseAppError } from '../utils/error'; +import { App as AppCore } from './core'; +import { AppStore, defaultAppStore } from './lifecycle'; import { app, appCheck, auth, messaging, machineLearning, storage, firestore, database, instanceId, installations, projectManagement, securityRules , remoteConfig, AppOptions, } from '../firebase-namespace-api'; -import { FirebaseApp } from './firebase-app'; import { cert, refreshToken, applicationDefault } from './credential-factory'; -import { getApplicationDefault } from './credential-internal'; - -import * as validator from '../utils/validator'; import { getSdkVersion } from '../utils/index'; import App = app.App; @@ -43,15 +38,6 @@ import RemoteConfig = remoteConfig.RemoteConfig; import SecurityRules = securityRules.SecurityRules; import Storage = storage.Storage; -const DEFAULT_APP_NAME = '[DEFAULT]'; - -/** - * Constant holding the environment variable name with the default config. - * If the environment variable contains a string that starts with '{' it will be parsed as JSON, - * otherwise it will be assumed to be pointing to a file. - */ -export const FIREBASE_CONFIG_VAR = 'FIREBASE_CONFIG'; - export interface FirebaseServiceNamespace { (app?: App): T; [key: string]: any; @@ -62,8 +48,7 @@ export interface FirebaseServiceNamespace { */ export class FirebaseNamespaceInternals { - private apps_: {[appName: string]: App} = {}; - constructor(public firebase_: {[key: string]: any}) {} + constructor(private readonly appStore: AppStore) {} /** * Initializes the App instance. @@ -76,39 +61,9 @@ export class FirebaseNamespaceInternals { * * @returns A new App instance. */ - public initializeApp(options?: AppOptions, appName = DEFAULT_APP_NAME): App { - if (typeof options === 'undefined') { - options = this.loadOptionsFromEnvVar(); - options.credential = getApplicationDefault(); - } - if (typeof appName !== 'string' || appName === '') { - throw new FirebaseAppError( - AppErrorCodes.INVALID_APP_NAME, - `Invalid Firebase app name "${appName}" provided. App name must be a non-empty string.`, - ); - } else if (appName in this.apps_) { - if (appName === DEFAULT_APP_NAME) { - throw new FirebaseAppError( - AppErrorCodes.DUPLICATE_APP, - 'The default Firebase app already exists. This means you called initializeApp() ' + - 'more than once without providing an app name as the second argument. In most cases ' + - 'you only need to call initializeApp() once. But if you do want to initialize ' + - 'multiple apps, pass a second argument to initializeApp() to give each app a unique ' + - 'name.', - ); - } else { - throw new FirebaseAppError( - AppErrorCodes.DUPLICATE_APP, - `Firebase app named "${appName}" already exists. This means you called initializeApp() ` + - 'more than once with the same app name as the second argument. Make sure you provide a ' + - 'unique name every time you call initializeApp().', - ); - } - } - - const app = new FirebaseApp(options, appName, this); - this.apps_[appName] = app; - return app; + public initializeApp(options?: AppOptions, appName?: string): App { + const app = this.appStore.initializeApp(options, appName); + return extendApp(app); } /** @@ -118,67 +73,16 @@ export class FirebaseNamespaceInternals { * @param appName Optional name of the FirebaseApp instance to return. * @returns The App instance which has the provided name. */ - public app(appName = DEFAULT_APP_NAME): App { - if (typeof appName !== 'string' || appName === '') { - throw new FirebaseAppError( - AppErrorCodes.INVALID_APP_NAME, - `Invalid Firebase app name "${appName}" provided. App name must be a non-empty string.`, - ); - } else if (!(appName in this.apps_)) { - let errorMessage: string = (appName === DEFAULT_APP_NAME) - ? 'The default Firebase app does not exist. ' : `Firebase app named "${appName}" does not exist. `; - errorMessage += 'Make sure you call initializeApp() before using any of the Firebase services.'; - - throw new FirebaseAppError(AppErrorCodes.NO_APP, errorMessage); - } - - return this.apps_[appName]; + public app(appName?: string): App { + const app = this.appStore.getApp(appName); + return extendApp(app); } /* * Returns an array of all the non-deleted App instances. */ public get apps(): App[] { - // Return a copy so the caller cannot mutate the array - return Object.keys(this.apps_).map((appName) => this.apps_[appName]); - } - - /* - * Removes the specified App instance. - */ - public removeApp(appName: string): void { - if (typeof appName === 'undefined') { - throw new FirebaseAppError( - AppErrorCodes.INVALID_APP_NAME, - 'No Firebase app name provided. App name must be a non-empty string.', - ); - } - - const appToRemove = this.app(appName); - delete this.apps_[appToRemove.name]; - } - - /** - * Parse the file pointed to by the FIREBASE_CONFIG_VAR, if it exists. - * Or if the FIREBASE_CONFIG_ENV contains a valid JSON object, parse it directly. - * If the environment variable contains a string that starts with '{' it will be parsed as JSON, - * otherwise it will be assumed to be pointing to a file. - */ - private loadOptionsFromEnvVar(): AppOptions { - const config = process.env[FIREBASE_CONFIG_VAR]; - if (!validator.isNonEmptyString(config)) { - return {}; - } - try { - const contents = config.startsWith('{') ? config : fs.readFileSync(config, 'utf8'); - return JSON.parse(contents) as AppOptions; - } catch (error) { - // Throw a nicely formed error message if the file contents cannot be parsed - throw new FirebaseAppError( - AppErrorCodes.INVALID_APP_OPTIONS, - 'Failed to parse app options file: ' + error, - ); - } + return this.appStore.getApps().map((app) => extendApp(app)); } } @@ -205,8 +109,8 @@ export class FirebaseNamespace { public Promise: any = Promise; /* tslint:enable */ - constructor() { - this.INTERNAL = new FirebaseNamespaceInternals(this); + constructor(appStore?: AppStore) { + this.INTERNAL = new FirebaseNamespaceInternals(appStore ?? new AppStore()); } /** @@ -417,3 +321,82 @@ export class FirebaseNamespace { return app; } } + +/** + * In order to maintain backward compatibility, we instantiate a default namespace instance in + * this module, and delegate all app lifecycle operations to it. In a future implementation where + * the old admin namespace is no longer supported, we should remove this. + * + * @internal + */ +export const defaultNamespace = new FirebaseNamespace(defaultAppStore); + +function extendApp(app: AppCore): App { + const result: App = app as App; + if ((result as any).__extended) { + return result; + } + + result.auth = () => { + const fn = require('../auth/index').getAuth; + return fn(app); + }; + + result.appCheck = () => { + const fn = require('../app-check/index').getAppCheck; + return fn(app); + }; + + result.database = (url?: string) => { + const fn = require('../database/index').getDatabaseWithUrl; + return fn(url, app); + }; + + result.messaging = () => { + const fn = require('../messaging/index').getMessaging; + return fn(app); + }; + + result.storage = () => { + const fn = require('../storage/index').getStorage; + return fn(app); + }; + + result.firestore = () => { + const fn = require('../firestore/index').getFirestore; + return fn(app); + }; + + result.instanceId = () => { + const fn = require('../instance-id/index').getInstanceId; + return fn(app); + } + + result.installations = () => { + const fn = require('../installations/index').getInstallations; + return fn(app); + }; + + result.machineLearning = () => { + const fn = require('../machine-learning/index').getMachineLearning; + return fn(app); + } + + result.projectManagement = () => { + const fn = require('../project-management/index').getProjectManagement; + return fn(app); + }; + + result.securityRules = () => { + const fn = require('../security-rules/index').getSecurityRules; + return fn(app); + }; + + result.remoteConfig = () => { + const fn = require('../remote-config/index').getRemoteConfig; + return fn(app); + }; + + (result as any).__extended = true; + return result; +} diff --git a/src/app/lifecycle.ts b/src/app/lifecycle.ts index ed999e6650..0ca7d2a78f 100644 --- a/src/app/lifecycle.ts +++ b/src/app/lifecycle.ts @@ -15,30 +15,116 @@ * limitations under the License. */ +import fs = require('fs'); + +import * as validator from '../utils/validator'; import { AppErrorCodes, FirebaseAppError } from '../utils/error'; import { App, AppOptions } from './core'; -import { FirebaseNamespace } from './firebase-namespace'; +import { getApplicationDefault } from './credential-internal'; +import { FirebaseApp } from './firebase-app'; -/** - * In order to maintain backward compatibility, we instantiate a default namespace instance in - * this module, and delegate all app lifecycle operations to it. In a future implementation where - * the old admin namespace is no longer supported, we should remove this, and implement app - * lifecycle management in this module itself. - * - * @internal - */ -export const defaultNamespace = new FirebaseNamespace(); +const DEFAULT_APP_NAME = '[DEFAULT]'; + +export class AppStore { + + private readonly appStore = new Map(); + + public initializeApp(options?: AppOptions, appName: string = DEFAULT_APP_NAME): App { + if (typeof options === 'undefined') { + options = loadOptionsFromEnvVar(); + options.credential = getApplicationDefault(); + } + + if (typeof appName !== 'string' || appName === '') { + throw new FirebaseAppError( + AppErrorCodes.INVALID_APP_NAME, + `Invalid Firebase app name "${appName}" provided. App name must be a non-empty string.`, + ); + } else if (this.appStore.has(appName)) { + if (appName === DEFAULT_APP_NAME) { + throw new FirebaseAppError( + AppErrorCodes.DUPLICATE_APP, + 'The default Firebase app already exists. This means you called initializeApp() ' + + 'more than once without providing an app name as the second argument. In most cases ' + + 'you only need to call initializeApp() once. But if you do want to initialize ' + + 'multiple apps, pass a second argument to initializeApp() to give each app a unique ' + + 'name.', + ); + } else { + throw new FirebaseAppError( + AppErrorCodes.DUPLICATE_APP, + `Firebase app named "${appName}" already exists. This means you called initializeApp() ` + + 'more than once with the same app name as the second argument. Make sure you provide a ' + + 'unique name every time you call initializeApp().', + ); + } + } + + const app = new FirebaseApp(options, appName, this); + this.appStore.set(app.name, app); + return app; + } + + public getApp(appName: string = DEFAULT_APP_NAME): App { + if (typeof appName !== 'string' || appName === '') { + throw new FirebaseAppError( + AppErrorCodes.INVALID_APP_NAME, + `Invalid Firebase app name "${appName}" provided. App name must be a non-empty string.`, + ); + } else if (!this.appStore.has(appName)) { + let errorMessage: string = (appName === DEFAULT_APP_NAME) + ? 'The default Firebase app does not exist. ' : `Firebase app named "${appName}" does not exist. `; + errorMessage += 'Make sure you call initializeApp() before using any of the Firebase services.'; + + throw new FirebaseAppError(AppErrorCodes.NO_APP, errorMessage); + } + + return this.appStore.get(appName)!; + } + + public getApps(): App[] { + // Return a copy so the caller cannot mutate the array + return Array.from(this.appStore.values()); + } + + public deleteApp(app: App): Promise { + if (typeof app !== 'object' || app === null || !('options' in app)) { + throw new FirebaseAppError(AppErrorCodes.INVALID_ARGUMENT, 'Invalid app argument.'); + } + + // Make sure the given app already exists. + const existingApp = getApp(app.name); -export function initializeApp(options?: AppOptions, name?: string): App { - return defaultNamespace.initializeApp(options, name); + // Delegate delete operation to the App instance itself. + return (existingApp as FirebaseApp).delete(); + } + + public clearAllApps(): Promise { + const promises: Array> = []; + this.getApps().forEach((app) => { + promises.push(this.deleteApp(app)); + }) + + return Promise.all(promises).then(); + } + + public removeApp(appName: string): void { + this.appStore.delete(appName); + } +} + +export const defaultAppStore = new AppStore(); + +export function initializeApp(options?: AppOptions, appName: string = DEFAULT_APP_NAME): App { + return defaultAppStore.initializeApp(options, appName); } -export function getApp(name?: string): App { - return defaultNamespace.app(name); +export function getApp(appName: string = DEFAULT_APP_NAME): App { + return defaultAppStore.getApp(appName); } export function getApps(): App[] { - return defaultNamespace.apps; + return defaultAppStore.getApps(); } /** @@ -59,14 +145,36 @@ export function getApps(): App[] { * ``` */ export function deleteApp(app: App): Promise { - if (typeof app !== 'object' || app === null || !('options' in app)) { - throw new FirebaseAppError(AppErrorCodes.INVALID_ARGUMENT, 'Invalid app argument.'); - } + return defaultAppStore.deleteApp(app); +} - // Make sure the given app already exists. - const existingApp = getApp(app.name); +/** + * Constant holding the environment variable name with the default config. + * If the environment variable contains a string that starts with '{' it will be parsed as JSON, + * otherwise it will be assumed to be pointing to a file. + */ +export const FIREBASE_CONFIG_VAR = 'FIREBASE_CONFIG'; - // Delegate delete operation to the App instance itself for now. This will tear down any - // local app state, and also remove it from the global map. - return (existingApp as any).delete(); +/** + * Parse the file pointed to by the FIREBASE_CONFIG_VAR, if it exists. + * Or if the FIREBASE_CONFIG_ENV contains a valid JSON object, parse it directly. + * If the environment variable contains a string that starts with '{' it will be parsed as JSON, + * otherwise it will be assumed to be pointing to a file. + */ +function loadOptionsFromEnvVar(): AppOptions { + const config = process.env[FIREBASE_CONFIG_VAR]; + if (!validator.isNonEmptyString(config)) { + return {}; + } + + try { + const contents = config.startsWith('{') ? config : fs.readFileSync(config, 'utf8'); + return JSON.parse(contents) as AppOptions; + } catch (error) { + // Throw a nicely formed error message if the file contents cannot be parsed + throw new FirebaseAppError( + AppErrorCodes.INVALID_APP_OPTIONS, + 'Failed to parse app options file: ' + error, + ); + } } diff --git a/src/default-namespace.ts b/src/default-namespace.ts index 60dd22fe3a..12e855e2d8 100644 --- a/src/default-namespace.ts +++ b/src/default-namespace.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { defaultNamespace as firebaseAdmin } from './app/lifecycle'; +import { defaultNamespace as firebaseAdmin } from './app/firebase-namespace'; // Inject a circular default export to allow users to use both: // diff --git a/src/instance-id/instance-id.ts b/src/instance-id/instance-id.ts index cd01932007..6bd92d991d 100644 --- a/src/instance-id/instance-id.ts +++ b/src/instance-id/instance-id.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { FirebaseApp } from '../app/firebase-app'; +import { getInstallations } from '../installations'; import { App } from '../app/index'; import { FirebaseInstallationsError, FirebaseInstanceIdError, @@ -63,7 +63,7 @@ export class InstanceId { * @returns A promise fulfilled when the instance ID is deleted. */ public deleteInstanceId(instanceId: string): Promise { - return (this.app as FirebaseApp).installations().deleteInstallation(instanceId) + return getInstallations(this.app).deleteInstallation(instanceId) .catch((err) => { if (err instanceof FirebaseInstallationsError) { let code = err.code.replace('installations/', ''); diff --git a/test/resources/mocks.ts b/test/resources/mocks.ts index b6b4b6bc97..026faa4f01 100644 --- a/test/resources/mocks.ts +++ b/test/resources/mocks.ts @@ -26,7 +26,6 @@ import * as _ from 'lodash'; import * as jwt from 'jsonwebtoken'; import { AppOptions } from '../../src/firebase-namespace-api'; -import { FirebaseNamespace } from '../../src/app/firebase-namespace'; import { FirebaseApp } from '../../src/app/firebase-app'; import { Credential, GoogleOAuthAccessToken, cert } from '../../src/app/index'; @@ -91,22 +90,18 @@ export class MockCredential implements Credential { } export function app(): FirebaseApp { - const namespaceInternals = new FirebaseNamespace().INTERNAL; - namespaceInternals.removeApp = _.noop; - return new FirebaseApp(appOptions, appName, namespaceInternals); + return new FirebaseApp(appOptions, appName); } export function mockCredentialApp(): FirebaseApp { return new FirebaseApp({ credential: new MockCredential(), databaseURL, - }, appName, new FirebaseNamespace().INTERNAL); + }, appName); } export function appWithOptions(options: AppOptions): FirebaseApp { - const namespaceInternals = new FirebaseNamespace().INTERNAL; - namespaceInternals.removeApp = _.noop; - return new FirebaseApp(options, appName, namespaceInternals); + return new FirebaseApp(options, appName); } export function appReturningNullAccessToken(): FirebaseApp { @@ -117,7 +112,7 @@ export function appReturningNullAccessToken(): FirebaseApp { } as any, databaseURL, projectId, - }, appName, new FirebaseNamespace().INTERNAL); + }, appName); } export function appReturningMalformedAccessToken(): FirebaseApp { @@ -127,7 +122,7 @@ export function appReturningMalformedAccessToken(): FirebaseApp { } as any, databaseURL, projectId, - }, appName, new FirebaseNamespace().INTERNAL); + }, appName); } export function appRejectedWhileFetchingAccessToken(): FirebaseApp { @@ -137,7 +132,7 @@ export function appRejectedWhileFetchingAccessToken(): FirebaseApp { } as any, databaseURL, projectId, - }, appName, new FirebaseNamespace().INTERNAL); + }, appName); } export const refreshToken = { diff --git a/test/unit/app/firebase-app.spec.ts b/test/unit/app/firebase-app.spec.ts index 043d8eb679..c6ec9eaaf6 100644 --- a/test/unit/app/firebase-app.spec.ts +++ b/test/unit/app/firebase-app.spec.ts @@ -29,9 +29,8 @@ import * as mocks from '../../resources/mocks'; import { GoogleOAuthAccessToken } from '../../../src/app/index'; import { ServiceAccountCredential } from '../../../src/app/credential-internal'; import { FirebaseApp, FirebaseAccessToken } from '../../../src/app/firebase-app'; -import { - FirebaseNamespace, FirebaseNamespaceInternals, FIREBASE_CONFIG_VAR -} from '../../../src/app/firebase-namespace'; +import { FirebaseNamespace } from '../../../src/app/firebase-namespace'; +import { AppStore, FIREBASE_CONFIG_VAR } from '../../../src/app/lifecycle'; import { auth, messaging, machineLearning, storage, firestore, database, instanceId, installations, projectManagement, securityRules , remoteConfig, appCheck, @@ -77,7 +76,6 @@ describe('FirebaseApp', () => { let clock: sinon.SinonFakeTimers; let getTokenStub: sinon.SinonStub; let firebaseNamespace: FirebaseNamespace; - let firebaseNamespaceInternals: FirebaseNamespaceInternals; let firebaseConfigVar: string | undefined; beforeEach(() => { @@ -90,10 +88,7 @@ describe('FirebaseApp', () => { firebaseConfigVar = process.env[FIREBASE_CONFIG_VAR]; delete process.env[FIREBASE_CONFIG_VAR]; firebaseNamespace = new FirebaseNamespace(); - firebaseNamespaceInternals = firebaseNamespace.INTERNAL; - - sinon.stub(firebaseNamespaceInternals, 'removeApp'); - mockApp = new FirebaseApp(mocks.appOptions, mocks.appName, firebaseNamespaceInternals); + mockApp = new FirebaseApp(mocks.appOptions, mocks.appName); }); afterEach(() => { @@ -106,7 +101,6 @@ describe('FirebaseApp', () => { } deleteSpy.resetHistory(); - (firebaseNamespaceInternals.removeApp as any).restore(); }); describe('#name', () => { @@ -124,14 +118,14 @@ describe('FirebaseApp', () => { it('should be case sensitive', () => { const newMockAppName = mocks.appName.toUpperCase(); - mockApp = new FirebaseApp(mocks.appOptions, newMockAppName, firebaseNamespaceInternals); + mockApp = new FirebaseApp(mocks.appOptions, newMockAppName); expect(mockApp.name).to.not.equal(mocks.appName); expect(mockApp.name).to.equal(newMockAppName); }); it('should respect leading and trailing whitespace', () => { const newMockAppName = ' ' + mocks.appName + ' '; - mockApp = new FirebaseApp(mocks.appOptions, newMockAppName, firebaseNamespaceInternals); + mockApp = new FirebaseApp(mocks.appOptions, newMockAppName); expect(mockApp.name).to.not.equal(mocks.appName); expect(mockApp.name).to.equal(newMockAppName); }); @@ -328,10 +322,11 @@ describe('FirebaseApp', () => { }); it('should call removeApp() on the Firebase namespace internals', () => { - return mockApp.delete().then(() => { - expect(firebaseNamespaceInternals.removeApp) - .to.have.been.calledOnce - .and.calledWith(mocks.appName); + const store = new AppStore(); + const stub = sinon.stub(store, 'removeApp').resolves(); + const app = new FirebaseApp(mockApp.options, mockApp.name, store); + return app.delete().then(() => { + expect(stub).to.have.been.calledOnce.and.calledWith(mocks.appName); }); }); diff --git a/test/unit/app/firebase-namespace.spec.ts b/test/unit/app/firebase-namespace.spec.ts index aee55a965f..7cfa53429e 100644 --- a/test/unit/app/firebase-namespace.spec.ts +++ b/test/unit/app/firebase-namespace.spec.ts @@ -260,66 +260,6 @@ describe('FirebaseNamespace', () => { }); }); - describe('#INTERNAL.removeApp()', () => { - const invalidAppNames = [null, NaN, 0, 1, true, false, [], ['a'], {}, { a: 1 }, _.noop]; - invalidAppNames.forEach((invalidAppName) => { - it('should throw given non-string app name: ' + JSON.stringify(invalidAppName), () => { - expect(() => { - firebaseNamespace.INTERNAL.removeApp(invalidAppName as any); - }).to.throw(`Invalid Firebase app name "${invalidAppName}" provided. App name must be a non-empty string.`); - }); - }); - - it('should throw given empty string app name', () => { - expect(() => { - firebaseNamespace.INTERNAL.removeApp(''); - }).to.throw('Invalid Firebase app name "" provided. App name must be a non-empty string.'); - }); - - it('should throw given an app name which does not correspond to an existing app', () => { - expect(() => { - firebaseNamespace.INTERNAL.removeApp(mocks.appName); - }).to.throw(`Firebase app named "${mocks.appName}" does not exist.`); - }); - - it('should throw given no app name if the default app does not exist', () => { - expect(() => { - (firebaseNamespace as any).INTERNAL.removeApp(); - }).to.throw('No Firebase app name provided. App name must be a non-empty string.'); - }); - - it('should throw given no app name even if the default app exists', () => { - firebaseNamespace.initializeApp(mocks.appOptions); - expect(() => { - (firebaseNamespace as any).INTERNAL.removeApp(); - }).to.throw('No Firebase app name provided. App name must be a non-empty string.'); - }); - - it('should remove the app corresponding to the provided app name from the namespace\'s app list', () => { - firebaseNamespace.initializeApp(mocks.appOptions, mocks.appName); - firebaseNamespace.INTERNAL.removeApp(mocks.appName); - expect(() => { - return firebaseNamespace.app(mocks.appName); - }).to.throw(`Firebase app named "${mocks.appName}" does not exist.`); - }); - - it('should remove the default app from the namespace\'s app list if the default app name is provided', () => { - firebaseNamespace.initializeApp(mocks.appOptions); - firebaseNamespace.INTERNAL.removeApp(DEFAULT_APP_NAME); - expect(() => { - return firebaseNamespace.app(); - }).to.throw('The default Firebase app does not exist.'); - }); - - it('should not be idempotent', () => { - firebaseNamespace.initializeApp(mocks.appOptions, mocks.appName); - firebaseNamespace.INTERNAL.removeApp(mocks.appName); - expect(() => { - firebaseNamespace.INTERNAL.removeApp(mocks.appName); - }).to.throw(`Firebase app named "${mocks.appName}" does not exist.`); - }); - }); - describe('#auth()', () => { it('should throw when called before initializing an app', () => { expect(() => { diff --git a/test/unit/app/index.spec.ts b/test/unit/app/index.spec.ts index 4cf35282f5..9de58baf84 100644 --- a/test/unit/app/index.spec.ts +++ b/test/unit/app/index.spec.ts @@ -30,6 +30,7 @@ import { Credential, applicationDefault, cert, refreshToken, } from '../../../src/app/index'; import { clearGlobalAppDefaultCred } from '../../../src/app/credential-factory'; +import { defaultAppStore } from '../../../src/app/lifecycle'; chai.should(); chai.use(chaiAsPromised); @@ -39,12 +40,7 @@ const expect = chai.expect; describe('firebase-admin/app', () => { afterEach(() => { - const deletePromises: Array> = []; - getApps().forEach((app) => { - deletePromises.push(deleteApp(app)); - }); - - return Promise.all(deletePromises); + return defaultAppStore.clearAllApps(); }); describe('#initializeApp()', () => { @@ -95,6 +91,12 @@ describe('firebase-admin/app', () => { } as any); }).to.throw('Invalid Firebase app options'); }); + + it('should initialize App instance without extended service methods', () => { + const app = initializeApp(mocks.appOptions); + expect((app as any).__extended).to.be.undefined; + expect((app as any).auth).to.be.undefined; + }); }); describe('#getApp()', () => { diff --git a/test/unit/firebase.spec.ts b/test/unit/firebase.spec.ts index 0edb814ce0..3313499b5f 100644 --- a/test/unit/firebase.spec.ts +++ b/test/unit/firebase.spec.ts @@ -32,6 +32,7 @@ import { FirebaseApp, FirebaseAppInternals } from '../../src/app/firebase-app'; import { RefreshTokenCredential, ServiceAccountCredential, isApplicationDefault } from '../../src/app/credential-internal'; +import { defaultAppStore, initializeApp } from '../../src/app/lifecycle'; chai.should(); chai.use(chaiAsPromised); @@ -54,12 +55,7 @@ describe('Firebase', () => { }); afterEach(() => { - const deletePromises: Array> = []; - firebaseAdmin.apps.forEach((app) => { - deletePromises.push(app.delete()); - }); - - return Promise.all(deletePromises); + return defaultAppStore.clearAllApps(); }); describe('#initializeApp()', () => { @@ -165,6 +161,23 @@ describe('Firebase', () => { return getAppInternals().getToken() .should.eventually.have.keys(['accessToken', 'expirationTime']); }); + + it('should initialize App instance with extended service methods', () => { + const app = firebaseAdmin.initializeApp(mocks.appOptions); + expect((app as any).__extended).to.be.true; + expect(app.auth).to.be.not.undefined; + }); + + it('should add extended service methods when retrieved via namespace', () => { + const app = initializeApp(mocks.appOptions); + expect((app as any).__extended).to.be.undefined; + expect((app as any).auth).to.be.undefined; + + const extendedApp = firebaseAdmin.app(); + expect(app).to.equal(extendedApp); + expect((app as any).__extended).to.be.true; + expect((app as any).auth).to.be.not.undefined; + }); }); describe('#database()', () => { @@ -266,6 +279,6 @@ describe('Firebase', () => { }); function getAppInternals(): FirebaseAppInternals { - return (firebaseAdmin.app() as FirebaseApp).INTERNAL; + return (firebaseAdmin.app() as unknown as FirebaseApp).INTERNAL; } }); diff --git a/test/unit/utils.ts b/test/unit/utils.ts index dee46114e6..2eb608397e 100644 --- a/test/unit/utils.ts +++ b/test/unit/utils.ts @@ -17,10 +17,7 @@ import * as _ from 'lodash'; import * as sinon from 'sinon'; - import * as mocks from '../resources/mocks'; - -import { FirebaseNamespace } from '../../src/app/firebase-namespace'; import { AppOptions } from '../../src/firebase-namespace-api'; import { FirebaseApp, FirebaseAppInternals, FirebaseAccessToken } from '../../src/app/firebase-app'; import { HttpError, HttpResponse } from '../../src/utils/api-request'; @@ -32,8 +29,7 @@ import { HttpError, HttpResponse } from '../../src/utils/api-request'; * @return A new FirebaseApp instance with the provided options. */ export function createAppWithOptions(options: object): FirebaseApp { - const mockFirebaseNamespaceInternals = new FirebaseNamespace().INTERNAL; - return new FirebaseApp(options as AppOptions, mocks.appName, mockFirebaseNamespaceInternals); + return new FirebaseApp(options as AppOptions, mocks.appName); } From 9b47bf5156e462a184625807fd12f39440e88ca0 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Tue, 14 Sep 2021 15:24:52 -0700 Subject: [PATCH 2/3] fix: Updated comments --- src/app/lifecycle.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/app/lifecycle.ts b/src/app/lifecycle.ts index 0ca7d2a78f..9c7cfdd31d 100644 --- a/src/app/lifecycle.ts +++ b/src/app/lifecycle.ts @@ -95,7 +95,8 @@ export class AppStore { // Make sure the given app already exists. const existingApp = getApp(app.name); - // Delegate delete operation to the App instance itself. + // Delegate delete operation to the App instance itself. That will also remove the App + // instance from the AppStore. return (existingApp as FirebaseApp).delete(); } @@ -108,6 +109,11 @@ export class AppStore { return Promise.all(promises).then(); } + /** + * Removes the specified App instance from the store. This is currently called by the + * {@link FirebaseApp.delete} method. Can be removed once the app deletion is handled + * entirely by the {@link deleteApp} top-level function. + */ public removeApp(appName: string): void { this.appStore.delete(appName); } From 306cd056a860aef7efc9f79d748988fefc6918ba Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Mon, 20 Sep 2021 12:56:38 -0700 Subject: [PATCH 3/3] Trigger checks