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
firebase · Oct 19, 2020 · 7d916d9 · 7d916d9
1 parent 959e21a
commit 7d916d9
Show file tree

Hide file tree

Showing 23 changed files with 66 additions and 86 deletions.
diff --git a/.changeset/stupid-dots-grab.md 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
@@ -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 protected]');
-        expect(e.phoneNumber).to.eq('+1555-this-is-a-number');
+        expect((e as FirebaseError).customData!.email).to.eq('[email protected]');
+        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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<string, unknown>
+  ) {
     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
@@ -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');