Improve error parsing for billing errors#10639
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the API client to gracefully handle non-JSON error responses (status >= 400) by falling back to the raw response text instead of throwing a JSON parsing error, and adds a corresponding unit test. The reviewer suggested wrapping the raw text in a standard error object structure ({ error: { message: text } }) to prevent potential runtime TypeErrors for callers expecting an object structure.
| if (res.status >= 400) { | ||
| body = text as unknown as ResT; | ||
| } else { | ||
| throw new FirebaseError(`Unable to parse JSON: ${err}`); | ||
| } |
There was a problem hiding this comment.
When resolveOnHTTPError is true, the caller expects the returned body to be an object (matching the generic ResT type). If we assign the raw non-JSON text string directly to body, any caller attempting to access properties on it (e.g., body.error.message) will encounter a runtime TypeError (e.g., Cannot read properties of undefined).
To prevent this and maintain type safety/compatibility, we can wrap the raw text in a standard error object structure: { error: { message: text } }. This structure is fully compatible with responseToError (which expects this shape or a string) and ensures that callers using resolveOnHTTPError: true can safely inspect the error message without throwing runtime exceptions.
| if (res.status >= 400) { | |
| body = text as unknown as ResT; | |
| } else { | |
| throw new FirebaseError(`Unable to parse JSON: ${err}`); | |
| } | |
| if (res.status >= 400) { | |
| body = { error: { message: text } } as unknown as ResT; | |
| } else { | |
| throw new FirebaseError(`Unable to parse JSON: ${err}`); | |
| } |
Description
Scenarios Tested
Sample Commands