firebase / firebase-admin-node

Firebase Admin Node.js SDK
https://firebase.google.com/docs/admin/setup
Apache License 2.0
1.63k stars 371 forks source link

FirebaseError type definition as class instead of interface #403

Open vlapo opened 5 years ago

vlapo commented 5 years ago

Description

Currently is FirebaseError defined as interface in typescript definition file. So in case of async/await we have to handle errors checking e.g. code property existence:

try {
    const decodedIdToken = await this.auth.verifyIdToken(firebaseIdToken);
    // some another calls async methods
} catch(err) {
    if (err.code === 'auth/argument-error') {
        // handle errors - but err is still any type
    }
}

This is proper way to handle errors but a little bit cleaner way would be:

try {
    const decodedIdToken = await this.auth.verifyIdToken(firebaseIdToken);
    // some another calls async methods
} catch(err) {
    if (err instanceof FirebaseError) {
        // handle error codes and proper typing here
    }
}

It is just a small improvement but in this way we have correct type in if statement and also FirebaseError.code would be some string literal type with defined all possible error codes.

type FirebaseErrorCode = 'auth/argument-error' | 'auth/id-token-expired' | ... ;

class FirebaseError extends Error {
    code: FirebaseErrorCode;
}

I think this could be really helpful in error handling.

Target environment

google-oss-bot commented 5 years ago

I found a few problems with this issue:

vlapo commented 5 years ago

Also using enum with comments instead of string literal could be a better improvement.

enum FirebaseErrorCode {
    /** Thrown if a method is called with incorrect arguments. */
    AUTH_ARGUMENT_ERROR = 'auth/argument-error',
    // ...
}

And comments are visible in IDE:

screenshot 2018-11-30 at 09 53 45

hiranya911 commented 5 years ago

I think we can expose FirebaseError as a class. But I'm not sure about error codes. We have way too many error codes, and we've been trying to find a way to phase them out entirely, and go for a different way of error handling.

hiranya911 commented 5 years ago

I've looked into this a bit, and it turns out to be more complicated than I originally imagined. Not only FirebaseError is defined as an interface, it is not really exported by the FirebaseNamespace. Therefore this cannot be implemented as a simple change to our typings.

it.only('supports instanceOf checks', () => {
    try {
      admin.app('non.existing');
    } catch (err) {
      expect(err.code).to.equal('app/no-app');
      expect(err).to.be.instanceOf(admin.FirebaseError);
    }
});

This results in:

supports instanceOf checks:
     TypeError: Cannot read property 'name' of undefined
      at Object.module.exports [as getName] (node_modules/chai/lib/chai/utils/getName.js:18:12)
      at Assertion.assertInstanceOf (node_modules/chai/lib/chai/core/assertions.js:794:18)
      at Assertion.ctx.(anonymous function) (node_modules/chai/lib/chai/utils/addMethod.js:41:25)
      at doAsserterAsyncAndAddThen (node_modules/chai-as-promised/lib/chai-as-promised.js:293:29)
      at Assertion.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:252:17)
      at Assertion.ctx.(anonymous function) [as instanceOf] (node_modules/chai/lib/chai/utils/overwriteMethod.js:49:33)
      at Context.<anonymous> (test/integration/app.spec.ts:100:25)

To make this work, we need something like the following:

// firebase-namespace.ts
import {FirebaseError} from './utils/error';

/**
 * Global Firebase context object.
 */
export class FirebaseNamespace {
    public credential = firebaseCredential;
    public SDK_VERSION = '<XXX_SDK_VERSION_XXX>';
    public INTERNAL: FirebaseNamespaceInternals;
    public FirebaseError = FirebaseError;
vinayuttam commented 5 years ago

For an expired token the following is the error message:

{ code: 'auth/argument-error',
  message:
   'Firebase ID token has expired. Get a fresh token from your client app and try again (auth/id-token-expired). See https://firebase.google.com/docs/auth/admin/verify-id-tokens for details on how to retrieve an ID token.' }

Shouldn't the code be auth/id-token-expired instead of an auth/argument-error' as per the documentation[1]?

[1] - https://firebase.google.com/docs/auth/admin/errors

cc: @hiranya911 @vlapo

vlapo commented 5 years ago

@vinayuttam https://github.com/firebase/firebase-admin-node/issues/179

vinayuttam commented 5 years ago

@vlapo thanks for pointing to that issue. I've missed it some how.

hiranya911 commented 5 years ago

I'm working on a proposal to revamp our error handling in general. I think we should expose our errors as classes instead of interfaces as part of this. Will keep this thread posted.

driescroons commented 4 years ago

Any update on this @hiranya911? I'm currently checking if my error object has a code property in my error middleware, but this interferes with other packages I'm using. (other errors thinking it's a FirebaseError by having a code property)

It would be a whole lot cleaner / easier if we could use error instanceof FirebaseError. The codes on the error object work perfectly fine for me, as I can do the label mapping to a nicer message myself, but the problem is more in the interface not being a class.

Edit: I just noticed the errors in auth do have nice labels for mapping, but the firestore ones have just a number.

hiranya911 commented 4 years ago

So far the new error handling model has been released in Python and .NET. We plan to bring it to Java soon. I think we can look into implementing it in Node.js towards the 2nd half of 2020.

driescroons commented 4 years ago

Would you mind sharing a link of the node sdk roadmap? Cannot seem to find it 😀

hiranya911 commented 4 years ago

@driescroons we don't publish our roadmaps publicly.

angelhodar commented 4 years ago

Just in case anyone is interesed, instead of using instanceof to check if error comes from Firebase, I have made this workaround to check if error is from the auth system:

function isFirebaseError(err) {
  if (err.code)
    return err.code.startsWith('auth/');
  return false;
}

Hope it helps ;)

jefersonla commented 3 years ago

Any update on this @hiranya911?

hiranya911 commented 3 years ago

Not yet. We have some other high priority work scheduled for the first half of this year. We can revisit this once that's done.

abierbaum commented 3 years ago

Here is a method I am using for now that uses a custom type guard.

interface IFirebaseError extends Error {
   code: string;
   message: string;
   stack?: string;
}

/** Check if we have a firebase error. */
export function isFirebaseError(err: any): err is IFirebaseError {
   return err.code && err.code.startsWith('auth/');
}
jukbot commented 3 years ago

Alternately, in latest version of firebase sdk, now it's can import global FirebaseError from '@firebase/util' package and using with type guard as below.

import { FirebaseError } from '@firebase/util'

try {
    // Some firebase functions
    await signInWithEmailAndPassword(auth, email, password)
} catch (error: unknown) {
   if (error instanceof FirebaseError) {
      console.error(error.code)
   }
}
tettoffensive commented 3 years ago

@jukbot For me using firebase-admin I had to do:

import { FirebaseError } from 'firebase-admin';

export default function isFirebaseError(error: unknown): error is FirebaseError {
  return (error as FirebaseError).code !== undefined;
}

Then:

try {
      ...
    } catch (error) {
      if (isFirebaseError(error)) {
        res.status(error.code === 'auth/id-token-expired' ? 401 : 100).end('Not authenticated properly');
      }
    }

Now if only I can find a way to get at the error code variables. I see them in the utils subdirectory, but I'm not sure they're exposed.

@hiranya911 Are you still trying to phase out error codes as your comment from 2018 mentioned? I think it would be better to expose a bunch of error code enums instead of me doing things like error.code === 'auth/id-token-expired'

Leo-Newie commented 3 years ago

@jukbot For me using firebase-admin I had to do:

import { FirebaseError } from 'firebase-admin';

export default function isFirebaseError(error: unknown): error is FirebaseError {
  return (error as FirebaseError).code !== undefined;
}

Then:

try {
      ...
    } catch (error) {
      if (isFirebaseError(error)) {
        res.status(error.code === 'auth/id-token-expired' ? 401 : 100).end('Not authenticated properly');
      }
    }

Now if only I can find a way to get at the error code variables. I see them in the utils subdirectory, but I'm not sure they're exposed.

@hiranya911 Are you still trying to phase out error codes as your comment from 2018 mentioned? I think it would be better to expose a bunch of error code enums instead of me doing things like error.code === 'auth/id-token-expired'

I also tried doing exactly what @tettoffensive have done by importing firebase-admin but for somewhat reason the error code that I get is a number(ex. 5) rather than "service/string-code"(ex. "auth/invalid-uid"). Does anyone knows how to deal with this?

abakucz commented 2 years ago

Well, https://github.com/firebase/firebase-admin-node/issues/403#issuecomment-930641443 is certainly helpful, but keep in mind that isFirebaseError returns true for any error that has code property, which many Errors from other libraries do as well. So you'll might end up with false positives.

haggaikaunda commented 1 year ago

Alternately, in latest version of firebase sdk, now it's can import global FirebaseError from '@firebase/util' package and using with type guard as below.

import { FirebaseError } from '@firebase/util'

try {
    // Some firebase functions
    await signInWithEmailAndPassword(auth, email, password)
} catch (error: unknown) {
   if (error instanceof FirebaseError) {
      console.error(error.code)
   }
}

Amazing! thank you! This is super helpful.