feat(core): Error Handling Revamp by jonathanedey · Pull Request #3102 · firebase/firebase-admin-node
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the error handling across the Firebase Admin SDK by introducing a structured ErrorInfo interface and updating various error classes to accept this object. This allows for better inclusion of httpResponse and cause (underlying error) information, improving debuggability. I have provided feedback regarding missing semicolons in auth-api-request.ts and suggested simplifying error messages by removing redundant low-level error details, as these are now captured by the cause property.
| * @internal | ||
| */ | ||
| export const FIREBASE_AUTH_UPLOAD_ACCOUNT = new ApiSettings('/accounts:batchCreate', 'POST'); | ||
| export const FIREBASE_AUTH_UPLOAD_ACCOUNT = new ApiSettings('/accounts:batchCreate', 'POST') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semicolon was removed from this line, which is inconsistent with the rest of the file and the project's coding style. Please restore it.
| export const FIREBASE_AUTH_UPLOAD_ACCOUNT = new ApiSettings('/accounts:batchCreate', 'POST') | |
| export const FIREBASE_AUTH_UPLOAD_ACCOUNT = new ApiSettings('/accounts:batchCreate', 'POST'); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semicolon was removed from the end of this statement. Please restore it to maintain consistency.
| 'INTERNAL ASSERT FAILED: Server request is missing user identifier'); | ||
| } | ||
| }); | ||
| }) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semicolon was removed from the end of this statement. Please restore it.
| 'INTERNAL ASSERT FAILED: Server request is missing user identifier'); | ||
| } | ||
| }); | ||
| }) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semicolon was removed from the end of this statement. Please restore it.
| ); | ||
| throw new FirebaseAppError({ | ||
| code: AppErrorCodes.INVALID_CREDENTIAL, | ||
| message: `Failed to parse service account json file: ${(error as Error).message}`, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including the low-level error message directly in the main error message string is redundant now that the cause property is available. It's better to keep the main message clean and rely on the cause for the detailed error information. This also avoids potential issues if error is not a standard Error object.
| message: `Failed to parse service account json file: ${(error as Error).message}`, | |
| message: 'Failed to parse service account json file.', |
| throw new FirebaseAppError({ | ||
| code: AppErrorCodes.INVALID_APP_OPTIONS, | ||
| message: `Failed to parse app options file: ${(error as Error).message}`, | ||
| cause: error as Error |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the change in credential-internal.ts, it's better to keep the main error message concise and use the cause property to provide the underlying error details.
| cause: error as Error | |
| message: 'Failed to parse app options file.', |