Adds server error messages to form actions bar #788
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| import { navToLogin } from './nav-to-login' | ||
| import type { ApiMethods, ErrorResponse } from '.' | ||
| import { capitalize } from '@oxide/util' | ||
|
|
||
| // Generic messages that work anywhere. There will probably be few or none of | ||
| // these, but it's convenient for now. | ||
| const globalCodeMap: Record<string, string> = { | ||
| Forbidden: 'Action not authorized', | ||
| } | ||
|
|
||
| const methodCodeMap: { [key in keyof Partial<ApiMethods>]: Record<string, string> } = { | ||
| organizationsPost: { | ||
| ObjectAlreadyExists: 'An organization with that name already exists', | ||
| }, | ||
| projectInstancesPost: { | ||
| ObjectAlreadyExists: 'An instance with that name already exists in this project', | ||
| }, | ||
| projectDisksPost: { | ||
| ObjectAlreadyExists: 'A disk with that name already exists in this project', | ||
| }, | ||
| projectVpcsPost: { | ||
| ObjectAlreadyExists: 'A VPC with that name already exists in this project', | ||
| }, | ||
| vpcSubnetsPost: { | ||
| ObjectAlreadyExists: 'A Subnet with that name already exists in this project', | ||
| }, | ||
| } | ||
|
There was a problem hiding this comment. You can imagine how this could get incredibly verbose. Definitely need to come up with a better way to break it down. There was a problem hiding this comment. We could easily put this together dynamically if each endpoint was tagged with the name of the resource and the name of the parent, but that really feels like API logic. At least for this specific error, I don't see why the API couldn't produce this sentence for us. To me the fact that this mapping feels like it belongs in the API layer of the console suggests it could really go in the API itself. That doesn't solve the problem in general, because I'm sure there will be cases where we want to say something more specific than would be appropriate for an API message. But for those cases maybe we don't need to think of them as API-layer errors but rather as part of the logic of the calling form, like I had before at the page level: console/app/pages/ProjectCreatePage.tsx Lines 18 to 21 in 3505cc3 Maybe the solution is to get as many of these messages as we can from the API, and for the remaining ones we encode them at the form level? There was a problem hiding this comment. I agree this should come more from the API. Any transformations or mapping that we have to do on the client are a bit of a smell. One clear way we could resolve this is to change the I'm not necessarily convinced that spreading the errors out makes any real difference. I'm afraid that would lead to inconsistent implementation of error messaging because the only context you have is what's already in the form you're looking at (or another form you reference). Having them centralized makes it easy to see all the custom errors at a glance. From an architectural perspective I also don't feel like the forms should have any specific knowledge of an error. Ideally it just displays whatever is given. There was a problem hiding this comment. I think spreading them out into the callers would make sense only if there are just a few very context-specific cases we can't get the API to handle, in which case we would think of them as UI copy. That's a good point about sagas. With composite endpoints like instance create, we really do need more info from the API no matter what, and I'm inclined toward having it just write a nice message instead of trying to come up with an abstract scheme of all the bits of info we might need to come up with such a message. There was a problem hiding this comment. Looking at the |
||
|
|
||
| export const handleErrors = | ||
| <M>(method: M) => | ||
zephraph marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| (resp: ErrorResponse) => { | ||
| // TODO is this a valid failure condition? | ||
| if (!resp) throw 'unknown server error' | ||
|
|
||
| // if logged out, hit /login to trigger login redirect | ||
| if (resp.status === 401) { | ||
| // TODO-usability: for background requests, a redirect to login without | ||
| // warning could come as a surprise to the user, especially because | ||
| // sometimes background requests are not directly triggered by a user | ||
| // action, e.g., polling or refetching when window regains focus | ||
| navToLogin({ includeCurrent: true }) | ||
| } | ||
| // we need to rethrow because that's how react-query knows it's an error | ||
| throw formatServerError(resp, methodCodeMap[method as unknown as keyof ApiMethods]) | ||
| } | ||
|
|
||
| function formatServerError( | ||
| resp: ErrorResponse, | ||
| codeMap: Record<string, string> = {} | ||
| ): ErrorResponse { | ||
| const code = resp.error.errorCode | ||
| const codeMsg = code && (codeMap[code] || globalCodeMap[code]) | ||
| const serverMsg = resp.error.message | ||
|
|
||
| resp.error.message = | ||
| codeMsg || getParseError(serverMsg) || serverMsg || 'Unknown server error' | ||
|
|
||
| return resp | ||
| } | ||
|
|
||
| function getParseError(message: string | undefined): string | undefined { | ||
| if (!message) return undefined | ||
| const inner = /^unable to parse body: (.+) at line \d+ column \d+$/.exec(message)?.[1] | ||
| return inner && capitalize(inner) | ||
| } | ||
|
|
||
| // -- TESTS ---------------- | ||
|
|
||
| if (import.meta.vitest) { | ||
| const { describe, it, expect } = import.meta.vitest | ||
| const parseError = { | ||
| error: { | ||
| requestId: '1', | ||
| errorCode: null, | ||
| message: 'unable to parse body: hello there, you have an error at line 129 column 4', | ||
| }, | ||
| } as ErrorResponse | ||
|
|
||
| const alreadyExists = { | ||
| error: { | ||
| requestId: '2', | ||
| errorCode: 'ObjectAlreadyExists', | ||
| message: 'whatever', | ||
| }, | ||
| } as ErrorResponse | ||
|
|
||
| const unauthorized = { | ||
| error: { | ||
| requestId: '3', | ||
| errorCode: 'Forbidden', | ||
| message: "I'm afraid you can't do that, Dave", | ||
| }, | ||
| } as ErrorResponse | ||
|
|
||
| describe('getParseError', () => { | ||
| it('extracts nice part of error message', () => { | ||
| expect(getParseError(parseError.error.message)).toEqual( | ||
| 'Hello there, you have an error' | ||
| ) | ||
| }) | ||
|
|
||
| it('returns undefined if error does not match pattern', () => { | ||
| expect(getParseError('some nonsense')).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe('getServerError', () => { | ||
| it('extracts message from parse errors', () => { | ||
| expect(formatServerError(parseError, {}).error.message).toEqual( | ||
| 'Hello there, you have an error' | ||
| ) | ||
| }) | ||
|
|
||
| it('uses message from code map if error code matches', () => { | ||
| expect( | ||
| formatServerError(alreadyExists, { | ||
| ObjectAlreadyExists: 'that already exists', | ||
| }).error.message | ||
| ).toEqual('that already exists') | ||
| }) | ||
|
|
||
| it('falls back to server error message if code not found', () => { | ||
| expect( | ||
| formatServerError(alreadyExists, { NotACode: 'stop that' }).error.message | ||
| ).toEqual('whatever') | ||
| }) | ||
|
|
||
| it('uses global map of generic codes for, e.g., 403s', () => { | ||
| expect(formatServerError(unauthorized, {}).error.message).toEqual( | ||
| 'Action not authorized' | ||
| ) | ||
| }) | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll go away once we convert this form