From 7d916d905ba16816ac8ac7c8748c83831ff614ce Mon Sep 17 00:00:00 2001 From: Feiyang Date: Mon, 19 Oct 2020 09:25:55 -0700 Subject: [PATCH] Move template data under a new `customData` field (#3946) * Move template data under a new `customData` field * Create stupid-dots-grab.md * fix tests * fix installations * fix analytics * fix rc * fix auth-exp * fix auth-exp again --- .changeset/stupid-dots-grab.md | 5 +++ packages-exp/auth-exp/src/api/index.test.ts | 12 ++++--- packages-exp/auth-exp/src/api/index.ts | 4 ++- .../src/core/providers/facebook.test.ts | 2 +- .../auth-exp/src/core/providers/facebook.ts | 2 +- .../src/core/providers/github.test.ts | 2 +- .../auth-exp/src/core/providers/github.ts | 2 +- .../src/core/providers/google.test.ts | 2 +- .../auth-exp/src/core/providers/google.ts | 2 +- .../src/core/providers/twitter.test.ts | 2 +- .../auth-exp/src/core/providers/twitter.ts | 2 +- packages-exp/auth-exp/src/mfa/mfa_error.ts | 5 +-- .../src/helpers/get-installation-entry.ts | 2 +- .../src/helpers/refresh-auth-token.ts | 5 ++- .../installations-exp/src/util/errors.ts | 2 +- packages/analytics/src/get-config.ts | 10 +++--- .../src/helpers/get-installation-entry.ts | 2 +- .../src/helpers/refresh-auth-token.ts | 5 ++- packages/installations/src/util/errors.ts | 2 +- .../src/client/retrying_client.ts | 7 ++-- .../test/client/rest_client.test.ts | 14 +++++--- packages/util/src/errors.ts | 36 ++++--------------- packages/util/test/errors.test.ts | 25 +------------ 23 files changed, 66 insertions(+), 86 deletions(-) create mode 100644 .changeset/stupid-dots-grab.md diff --git a/.changeset/stupid-dots-grab.md b/.changeset/stupid-dots-grab.md new file mode 100644 index 00000000000..85608f35687 --- /dev/null +++ b/.changeset/stupid-dots-grab.md @@ -0,0 +1,5 @@ +--- +"@firebase/util": patch +--- + +Write template data to a new `customData` field in` FirebaseError` instead of writing to the error object itself to avoid overwriting existing fields. diff --git a/packages-exp/auth-exp/src/api/index.test.ts b/packages-exp/auth-exp/src/api/index.test.ts index 99caaca6d14..846db20884c 100644 --- a/packages-exp/auth-exp/src/api/index.test.ts +++ b/packages-exp/auth-exp/src/api/index.test.ts @@ -284,7 +284,7 @@ describe('api/_performApiRequest', () => { assert.fail('Call should have failed'); } catch (e) { expect(e.code).to.eq(`auth/${AuthErrorCode.NEED_CONFIRMATION}`); - expect(e._tokenResponse).to.eql({ + expect((e as FirebaseError).customData!._tokenResponse).to.eql({ needConfirmation: true, idToken: 'id-token' }); @@ -314,7 +314,9 @@ describe('api/_performApiRequest', () => { assert.fail('Call should have failed'); } catch (e) { expect(e.code).to.eq(`auth/${AuthErrorCode.CREDENTIAL_ALREADY_IN_USE}`); - expect(e._tokenResponse).to.eql(response); + expect((e as FirebaseError).customData!._tokenResponse).to.eql( + response + ); } }); @@ -343,8 +345,10 @@ describe('api/_performApiRequest', () => { assert.fail('Call should have failed'); } catch (e) { expect(e.code).to.eq(`auth/${AuthErrorCode.EMAIL_EXISTS}`); - expect(e.email).to.eq('email@test.com'); - expect(e.phoneNumber).to.eq('+1555-this-is-a-number'); + expect((e as FirebaseError).customData!.email).to.eq('email@test.com'); + expect((e as FirebaseError).customData!.phoneNumber).to.eq( + '+1555-this-is-a-number' + ); } }); }); diff --git a/packages-exp/auth-exp/src/api/index.ts b/packages-exp/auth-exp/src/api/index.ts index 8b10a4a7ba7..2fee70291bd 100644 --- a/packages-exp/auth-exp/src/api/index.ts +++ b/packages-exp/auth-exp/src/api/index.ts @@ -251,6 +251,8 @@ function makeTaggedError( } const error = AUTH_ERROR_FACTORY.create(code, errorParams); - (error as TaggedWithTokenResponse)._tokenResponse = response; + + // We know customData is defined on error because errorParams is defined + (error.customData! as TaggedWithTokenResponse)._tokenResponse = response; return error; } diff --git a/packages-exp/auth-exp/src/core/providers/facebook.test.ts b/packages-exp/auth-exp/src/core/providers/facebook.test.ts index dab3fedce31..ed2d5dc9f99 100644 --- a/packages-exp/auth-exp/src/core/providers/facebook.test.ts +++ b/packages-exp/auth-exp/src/core/providers/facebook.test.ts @@ -59,7 +59,7 @@ describe('src/core/providers/facebook', () => { const error = AUTH_ERROR_FACTORY.create(AuthErrorCode.NEED_CONFIRMATION, { appName: 'foo' }); - (error as TaggedWithTokenResponse)._tokenResponse = { + (error.customData! as TaggedWithTokenResponse)._tokenResponse = { ...TEST_ID_TOKEN_RESPONSE, oauthAccessToken: 'access-token' }; diff --git a/packages-exp/auth-exp/src/core/providers/facebook.ts b/packages-exp/auth-exp/src/core/providers/facebook.ts index d35e79dfdf7..94717aa0cb0 100644 --- a/packages-exp/auth-exp/src/core/providers/facebook.ts +++ b/packages-exp/auth-exp/src/core/providers/facebook.ts @@ -48,7 +48,7 @@ export class FacebookAuthProvider extends OAuthProvider { error: FirebaseError ): externs.OAuthCredential | null { return FacebookAuthProvider.credentialFromTaggedObject( - error as TaggedWithTokenResponse + (error.customData || {}) as TaggedWithTokenResponse ); } diff --git a/packages-exp/auth-exp/src/core/providers/github.test.ts b/packages-exp/auth-exp/src/core/providers/github.test.ts index ecacbeb299e..1ab22f23567 100644 --- a/packages-exp/auth-exp/src/core/providers/github.test.ts +++ b/packages-exp/auth-exp/src/core/providers/github.test.ts @@ -59,7 +59,7 @@ describe('src/core/providers/github', () => { const error = AUTH_ERROR_FACTORY.create(AuthErrorCode.NEED_CONFIRMATION, { appName: 'foo' }); - (error as TaggedWithTokenResponse)._tokenResponse = { + (error.customData! as TaggedWithTokenResponse)._tokenResponse = { ...TEST_ID_TOKEN_RESPONSE, oauthAccessToken: 'access-token' }; diff --git a/packages-exp/auth-exp/src/core/providers/github.ts b/packages-exp/auth-exp/src/core/providers/github.ts index 4735f02041c..2e50e37dc95 100644 --- a/packages-exp/auth-exp/src/core/providers/github.ts +++ b/packages-exp/auth-exp/src/core/providers/github.ts @@ -48,7 +48,7 @@ export class GithubAuthProvider extends OAuthProvider { error: FirebaseError ): externs.OAuthCredential | null { return GithubAuthProvider.credentialFromTaggedObject( - error as TaggedWithTokenResponse + (error.customData || {}) as TaggedWithTokenResponse ); } diff --git a/packages-exp/auth-exp/src/core/providers/google.test.ts b/packages-exp/auth-exp/src/core/providers/google.test.ts index 5ae62384522..1a5a982f5fa 100644 --- a/packages-exp/auth-exp/src/core/providers/google.test.ts +++ b/packages-exp/auth-exp/src/core/providers/google.test.ts @@ -62,7 +62,7 @@ describe('src/core/providers/google', () => { const error = AUTH_ERROR_FACTORY.create(AuthErrorCode.NEED_CONFIRMATION, { appName: 'foo' }); - (error as TaggedWithTokenResponse)._tokenResponse = { + (error.customData! as TaggedWithTokenResponse)._tokenResponse = { ...TEST_ID_TOKEN_RESPONSE, oauthAccessToken: 'access-token', oauthIdToken: 'id-token' diff --git a/packages-exp/auth-exp/src/core/providers/google.ts b/packages-exp/auth-exp/src/core/providers/google.ts index 27fe7d3abf2..778fd7e847e 100644 --- a/packages-exp/auth-exp/src/core/providers/google.ts +++ b/packages-exp/auth-exp/src/core/providers/google.ts @@ -53,7 +53,7 @@ export class GoogleAuthProvider extends OAuthProvider { error: FirebaseError ): externs.OAuthCredential | null { return GoogleAuthProvider.credentialFromTaggedObject( - error as TaggedWithTokenResponse + (error.customData || {}) as TaggedWithTokenResponse ); } diff --git a/packages-exp/auth-exp/src/core/providers/twitter.test.ts b/packages-exp/auth-exp/src/core/providers/twitter.test.ts index 8d1c42edc04..51ef349f1e7 100644 --- a/packages-exp/auth-exp/src/core/providers/twitter.test.ts +++ b/packages-exp/auth-exp/src/core/providers/twitter.test.ts @@ -79,7 +79,7 @@ describe('src/core/providers/twitter', () => { const error = AUTH_ERROR_FACTORY.create(AuthErrorCode.NEED_CONFIRMATION, { appName: 'foo' }); - (error as TaggedWithTokenResponse)._tokenResponse = { + (error.customData! as TaggedWithTokenResponse)._tokenResponse = { ...TEST_ID_TOKEN_RESPONSE, oauthAccessToken: 'access-token', oauthTokenSecret: 'token-secret' diff --git a/packages-exp/auth-exp/src/core/providers/twitter.ts b/packages-exp/auth-exp/src/core/providers/twitter.ts index 8e9e456b828..305fcf03769 100644 --- a/packages-exp/auth-exp/src/core/providers/twitter.ts +++ b/packages-exp/auth-exp/src/core/providers/twitter.ts @@ -67,7 +67,7 @@ export class TwitterAuthProvider extends OAuthProvider { error: FirebaseError ): externs.OAuthCredential | null { return TwitterAuthProvider.credentialFromTaggedObject( - error as TaggedWithTokenResponse + (error.customData || {}) as TaggedWithTokenResponse ); } diff --git a/packages-exp/auth-exp/src/mfa/mfa_error.ts b/packages-exp/auth-exp/src/mfa/mfa_error.ts index 6fbc5985060..12a3f755ca4 100644 --- a/packages-exp/auth-exp/src/mfa/mfa_error.ts +++ b/packages-exp/auth-exp/src/mfa/mfa_error.ts @@ -46,8 +46,9 @@ export class MultiFactorError Object.setPrototypeOf(this, MultiFactorError.prototype); this.appName = auth.name; this.code = error.code; - this.tenantid = auth.tenantId; - this.serverResponse = error.serverResponse as IdTokenMfaResponse; + this.tenantId = auth.tenantId ?? undefined; + this.serverResponse = error.customData! + .serverResponse as IdTokenMfaResponse; } static _fromErrorAndCredential( diff --git a/packages-exp/installations-exp/src/helpers/get-installation-entry.ts b/packages-exp/installations-exp/src/helpers/get-installation-entry.ts index a0e1cc425ce..6b57563eb80 100644 --- a/packages-exp/installations-exp/src/helpers/get-installation-entry.ts +++ b/packages-exp/installations-exp/src/helpers/get-installation-entry.ts @@ -138,7 +138,7 @@ async function registerInstallation( ); return set(appConfig, registeredInstallationEntry); } catch (e) { - if (isServerError(e) && e.serverCode === 409) { + if (isServerError(e) && e.customData.serverCode === 409) { // Server returned a "FID can not be used" error. // Generate a new ID next time. await remove(appConfig); diff --git a/packages-exp/installations-exp/src/helpers/refresh-auth-token.ts b/packages-exp/installations-exp/src/helpers/refresh-auth-token.ts index 1ad5dc5da50..ac2a0ffc02d 100644 --- a/packages-exp/installations-exp/src/helpers/refresh-auth-token.ts +++ b/packages-exp/installations-exp/src/helpers/refresh-auth-token.ts @@ -150,7 +150,10 @@ async function fetchAuthTokenFromServer( await set(installations.appConfig, updatedInstallationEntry); return authToken; } catch (e) { - if (isServerError(e) && (e.serverCode === 401 || e.serverCode === 404)) { + if ( + isServerError(e) && + (e.customData.serverCode === 401 || e.customData.serverCode === 404) + ) { // Server returned a "FID not found" or a "Invalid authentication" error. // Generate a new ID next time. await remove(installations.appConfig); diff --git a/packages-exp/installations-exp/src/util/errors.ts b/packages-exp/installations-exp/src/util/errors.ts index 6332ff65901..25cc69b1ff0 100644 --- a/packages-exp/installations-exp/src/util/errors.ts +++ b/packages-exp/installations-exp/src/util/errors.ts @@ -61,7 +61,7 @@ export interface ServerErrorData { serverStatus: string; } -export type ServerError = FirebaseError & ServerErrorData; +export type ServerError = FirebaseError & { customData: ServerErrorData }; /** Returns true if error is a FirebaseError that is based on an error from the server. */ export function isServerError(error: unknown): error is ServerError { diff --git a/packages/analytics/src/get-config.ts b/packages/analytics/src/get-config.ts index 9a9024d058b..6be89ce4876 100644 --- a/packages/analytics/src/get-config.ts +++ b/packages/analytics/src/get-config.ts @@ -222,7 +222,7 @@ async function attemptFetchDynamicConfigWithRetry( } const backoffMillis = - Number(e.httpStatus) === 503 + Number(e.customData.httpStatus) === 503 ? calculateBackoffMillis( backoffCount, retryData.intervalMillis, @@ -284,16 +284,18 @@ function setAbortableTimeout( }); } +type RetriableError = FirebaseError & { customData: { httpStatus: string } }; + /** * Returns true if the {@link Error} indicates a fetch request may succeed later. */ -function isRetriableError(e: Error): boolean { - if (!(e instanceof FirebaseError)) { +function isRetriableError(e: Error): e is RetriableError { + if (!(e instanceof FirebaseError) || !e.customData) { return false; } // Uses string index defined by ErrorData, which FirebaseError implements. - const httpStatus = Number(e['httpStatus']); + const httpStatus = Number(e.customData['httpStatus']); return ( httpStatus === 429 || diff --git a/packages/installations/src/helpers/get-installation-entry.ts b/packages/installations/src/helpers/get-installation-entry.ts index 0edcf8e8b94..43cb443c533 100644 --- a/packages/installations/src/helpers/get-installation-entry.ts +++ b/packages/installations/src/helpers/get-installation-entry.ts @@ -138,7 +138,7 @@ async function registerInstallation( ); return set(appConfig, registeredInstallationEntry); } catch (e) { - if (isServerError(e) && e.serverCode === 409) { + if (isServerError(e) && e.customData.serverCode === 409) { // Server returned a "FID can not be used" error. // Generate a new ID next time. await remove(appConfig); diff --git a/packages/installations/src/helpers/refresh-auth-token.ts b/packages/installations/src/helpers/refresh-auth-token.ts index fa980597342..8c763f9820d 100644 --- a/packages/installations/src/helpers/refresh-auth-token.ts +++ b/packages/installations/src/helpers/refresh-auth-token.ts @@ -148,7 +148,10 @@ async function fetchAuthTokenFromServer( await set(dependencies.appConfig, updatedInstallationEntry); return authToken; } catch (e) { - if (isServerError(e) && (e.serverCode === 401 || e.serverCode === 404)) { + if ( + isServerError(e) && + (e.customData.serverCode === 401 || e.customData.serverCode === 404) + ) { // Server returned a "FID not found" or a "Invalid authentication" error. // Generate a new ID next time. await remove(dependencies.appConfig); diff --git a/packages/installations/src/util/errors.ts b/packages/installations/src/util/errors.ts index 6332ff65901..25cc69b1ff0 100644 --- a/packages/installations/src/util/errors.ts +++ b/packages/installations/src/util/errors.ts @@ -61,7 +61,7 @@ export interface ServerErrorData { serverStatus: string; } -export type ServerError = FirebaseError & ServerErrorData; +export type ServerError = FirebaseError & { customData: ServerErrorData }; /** Returns true if error is a FirebaseError that is based on an error from the server. */ export function isServerError(error: unknown): error is ServerError { diff --git a/packages/remote-config/src/client/retrying_client.ts b/packages/remote-config/src/client/retrying_client.ts index 0364d2db1c2..fe1737023df 100644 --- a/packages/remote-config/src/client/retrying_client.ts +++ b/packages/remote-config/src/client/retrying_client.ts @@ -61,16 +61,17 @@ export function setAbortableTimeout( }); } +type RetriableError = FirebaseError & { customData: { httpStatus: string } }; /** * Returns true if the {@link Error} indicates a fetch request may succeed later. */ -function isRetriableError(e: Error): boolean { - if (!(e instanceof FirebaseError)) { +function isRetriableError(e: Error): e is RetriableError { + if (!(e instanceof FirebaseError) || !e.customData) { return false; } // Uses string index defined by ErrorData, which FirebaseError implements. - const httpStatus = Number(e['httpStatus']); + const httpStatus = Number(e.customData['httpStatus']); return ( httpStatus === 429 || diff --git a/packages/remote-config/test/client/rest_client.test.ts b/packages/remote-config/test/client/rest_client.test.ts index 9219e13d503..89b745dacca 100644 --- a/packages/remote-config/test/client/rest_client.test.ts +++ b/packages/remote-config/test/client/rest_client.test.ts @@ -134,7 +134,10 @@ describe('RestClient', () => { await expect(fetchPromise) .to.eventually.be.rejectedWith(FirebaseError, firebaseError.message) - .with.property('originalErrorMessage', 'Network request failed'); + .with.nested.property( + 'customData.originalErrorMessage', + 'Network request failed' + ); }); it('throws on JSON parse failure', async () => { @@ -154,7 +157,10 @@ describe('RestClient', () => { await expect(fetchPromise) .to.eventually.be.rejectedWith(FirebaseError, firebaseError.message) - .with.property('originalErrorMessage', 'Unexpected end of input'); + .with.nested.property( + 'customData.originalErrorMessage', + 'Unexpected end of input' + ); }); it('handles 304 status code and empty body', async () => { @@ -200,7 +206,7 @@ describe('RestClient', () => { await expect(fetchPromise) .to.eventually.be.rejectedWith(FirebaseError, error.message) - .with.property('httpStatus', 500); + .with.nested.property('customData.httpStatus', 500); }); it('normalizes NO_CHANGE state to 304 status', async () => { @@ -257,7 +263,7 @@ describe('RestClient', () => { await expect(fetchPromise) .to.eventually.be.rejectedWith(FirebaseError, error.message) - .with.property('httpStatus', status); + .with.nested.property('customData.httpStatus', status); } }); }); diff --git a/packages/util/src/errors.ts b/packages/util/src/errors.ts index c55b9c87389..97384e78cca 100644 --- a/packages/util/src/errors.ts +++ b/packages/util/src/errors.ts @@ -69,26 +69,16 @@ export interface ErrorData { [key: string]: unknown; } -export interface FirebaseError extends Error, ErrorData { - // Unique code for error - format is service/error-code-string. - readonly code: string; - - // Developer-friendly error message. - readonly message: string; - - // Always 'FirebaseError'. - readonly name: typeof ERROR_NAME; - - // Where available - stack backtrace in a string. - readonly stack?: string; -} - // Based on code from: // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Custom_Error_Types export class FirebaseError extends Error { readonly name = ERROR_NAME; - constructor(readonly code: string, message: string) { + constructor( + readonly code: string, + message: string, + public customData?: Record + ) { super(message); // Fix For ES5 @@ -125,21 +115,7 @@ export class ErrorFactory< // Service Name: Error message (service/code). const fullMessage = `${this.serviceName}: ${message} (${fullCode}).`; - const error = new FirebaseError(fullCode, fullMessage); - - // Keys with an underscore at the end of their name are not included in - // error.data for some reason. - // TODO: Replace with Object.entries when lib is updated to es2017. - for (const key of Object.keys(customData)) { - if (key.slice(-1) !== '_') { - if (key in error) { - console.warn( - `Overwriting FirebaseError base field "${key}" can cause unexpected behavior.` - ); - } - error[key] = customData[key]; - } - } + const error = new FirebaseError(fullCode, fullMessage, customData); return error; } diff --git a/packages/util/test/errors.test.ts b/packages/util/test/errors.test.ts index 012b7762fd7..f570e096a5d 100644 --- a/packages/util/test/errors.test.ts +++ b/packages/util/test/errors.test.ts @@ -15,7 +15,6 @@ * limitations under the License. */ import { assert } from 'chai'; -import { stub } from 'sinon'; import { ErrorFactory, ErrorMap, FirebaseError } from '../src/errors'; type ErrorCode = @@ -60,14 +59,7 @@ describe('FirebaseError', () => { e.message, "Fake: Could not find file: 'foo.txt' (fake/file-not-found)." ); - assert.equal(e.file, 'foo.txt'); - }); - - it('anonymously replaces template values with data', () => { - const e = ERROR_FACTORY.create('anon-replace', { repl_: 'world' }); - assert.equal(e.code, 'fake/anon-replace'); - assert.equal(e.message, 'Fake: Hello, world! (fake/anon-replace).'); - assert.isUndefined(e.repl_); + assert.equal(e.customData!.file, 'foo.txt'); }); it('uses "Error" as template when template is missing', () => { @@ -88,21 +80,6 @@ describe('FirebaseError', () => { ); }); - it('warns if overwriting a base error field with custom data', () => { - const warnStub = stub(console, 'warn'); - const e = ERROR_FACTORY.create('overwrite-field', { - code: 'overwritten code' - }); - assert.equal(e.code, 'overwritten code'); - // TODO: use sinon-chai for this. - assert.ok( - warnStub.calledOnceWith( - 'Overwriting FirebaseError base field "code" can cause unexpected behavior.' - ) - ); - warnStub.restore(); - }); - it('has stack', () => { const e = ERROR_FACTORY.create('generic-error');