diff --git a/app/api/client.ts b/app/api/client.ts index ae8299c0cd..0324f2c015 100644 --- a/app/api/client.ts +++ b/app/api/client.ts @@ -82,13 +82,27 @@ export type ResultsPage = { items: TItem[]; nextPage?: string | null } type HandledResult = { type: 'success'; data: T } | { type: 'error'; data: ApiError } +type ExpectedError = { + /** + * Why this error response is expected at the call site. + * Logged after "This error is expected: ", so this should be a sentence + * fragment starting lowercase and ending with punctuation, e.g., + * "404 means the image name is not taken." + */ + explanation: string + /** Expected HTTP status for the handled error case */ + statusCode: number +} + +const expectedErrorLabel = ({ statusCode }: ExpectedError) => `status ${statusCode}` + // method: keyof Api would be strictly more correct, but making it a string // means we can call this directly in all the spots below instead of having to // make it generic over Api, which requires passing it as an argument to // getUseApiQuery, etc. This is fine because it is only being called inside // functions where `method` is already required to be an API method. const handleResult = - (method: string) => + ({ method, errorsExpected }: { method: string; errorsExpected?: ExpectedError }) => (result: ApiResult): HandledResult => { if (result.type === 'success') return { type: 'success', data: result.data } @@ -111,15 +125,40 @@ const handleResult = const consolePage = window.location.pathname + window.location.search // TODO: need to change oxide.ts to put the HTTP method on the result in // order to log it here - console.error( - `More info about API ${error.statusCode || 'error'} on ${consolePage} - -API URL: ${result.response.url} + const matchesExpectedError = + !!errorsExpected && error.statusCode === errorsExpected.statusCode + const logFn = matchesExpectedError ? console.info : console.error + const details = `API URL: ${result.response.url} Request ID: ${error.requestId} Error code: ${error.errorCode} -Error message: ${error.message.replace(/\n/g, '\n' + ' '.repeat('Error message: '.length))} +Error message: ${error.message.replace(/\n/g, '\n' + ' '.repeat('Error message: '.length))}` + if (matchesExpectedError) { + logFn( + `API ${error.statusCode || 'error'} on ${consolePage} + +%cThis error is expected: %c${errorsExpected.explanation} +Expected: ${expectedErrorLabel(errorsExpected)} + +${details} +`, + 'font-weight: bold', + 'font-weight: normal' + ) + } else { + const mismatchDetails = errorsExpected + ? ` +Expected: ${expectedErrorLabel(errorsExpected)} +Reason: ${errorsExpected.explanation} ` - ) + : '' + logFn( + `More info about API ${error.statusCode || 'error'} on ${consolePage} + +${mismatchDetails}${details} + +` + ) + } } return { type: 'error', data: error } @@ -234,9 +273,17 @@ export function ensurePrefetched( export const q = ( f: (p: Params) => Promise>, params: Params, - options: UseQueryOtherOptions = {} -) => - queryOptions({ + options: UseQueryOtherOptions & { + /** + * Expected error details. Matching errors are logged as `console.info` + * with the explanation; mismatches remain `console.error`. + * The explanation is rendered after "This error is expected: ". + */ + errorsExpected?: ExpectedError + } = {} +) => { + const { errorsExpected, ...rqOptions } = options + return queryOptions({ // method name first means we can invalidate all calls to this endpoint by // invalidating [f.name] (see invalidateEndpoint) queryKey: [f.name, params], @@ -248,7 +295,7 @@ export const q = ( // in the middle of prefetching queryFn: () => f(params) - .then(handleResult(f.name)) + .then(handleResult({ method: f.name, errorsExpected })) .then((result) => { if (result.type === 'success') return result.data throw result.data @@ -258,8 +305,9 @@ export const q = ( // up as `error` state instead, pass `throwOnError: false` as an // option from the calling component and it will override this throwOnError: (err) => err.statusCode === 404, - ...options, + ...rqOptions, }) +} const ERRORS_ALLOWED = 'errors-allowed' @@ -282,21 +330,30 @@ const ERRORS_ALLOWED = 'errors-allowed' export const qErrorsAllowed = ( f: (p: Params) => Promise>, params: Params, - options: UseQueryOtherOptions> = {} -) => - queryOptions({ + options: UseQueryOtherOptions> & { + /** + * Expected error details. Matching errors are logged as `console.info` + * with the explanation; mismatches remain `console.error`. + * The explanation is rendered after "This error is expected: ". + */ + errorsExpected: ExpectedError + } +) => { + const { errorsExpected, ...rqOptions } = options + return queryOptions({ // extra bit of key is important to distinguish from normal query. if we // hit a given endpoint twice on the same page, once the normal way and // once with errors allowed the responses have different shapes, so we do // not want to share the cache and mix them up queryKey: [f.name, params, ERRORS_ALLOWED], - queryFn: () => f(params).then(handleResult(f.name)), + queryFn: () => f(params).then(handleResult({ method: f.name, errorsExpected })), // No point having throwOnError because errors do not throw. Worth // considering still throwing for particular errors: sometimes we expect // a 403, other times we expect 404s. We could take a list of acceptable // status codes. - ...options, + ...rqOptions, }) +} // Unlike the query one, we don't need this to go through an options object // because we are not calling the mutation in two spots and sharing the options @@ -326,7 +383,7 @@ export const useApiMutation = ( // us back Params, but TS can't prove Omit // === Params structurally. f(params as Params, { signal: __signal }) - .then(handleResult(f.name)) + .then(handleResult({ method: f.name })) .then((result) => { if (result.type === 'success') return result.data throw result.data diff --git a/app/components/AttachFloatingIpModal.tsx b/app/components/AttachFloatingIpModal.tsx index de8d063473..2e48b5f7da 100644 --- a/app/components/AttachFloatingIpModal.tsx +++ b/app/components/AttachFloatingIpModal.tsx @@ -27,7 +27,16 @@ import { ModalForm } from './form/ModalForm' function IpPoolName({ ipPoolId }: { ipPoolId: string }) { const { data: result } = useQuery( - qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } }) + qErrorsAllowed( + api.ipPoolView, + { path: { pool: ipPoolId } }, + { + errorsExpected: { + explanation: 'the referenced IP pool may have been deleted.', + statusCode: 404, + }, + } + ) ) // As with IpPoolCell, this should never happen, but to be safe … if (!result || result.type === 'error') return null diff --git a/app/forms/image-upload.tsx b/app/forms/image-upload.tsx index a38732ef03..69ad3b79d3 100644 --- a/app/forms/image-upload.tsx +++ b/app/forms/image-upload.tsx @@ -517,19 +517,20 @@ export default function ImageCreate() { // check that image name isn't taken before starting the whole thing const image = await queryClient .fetchQuery( - q(api.imageView, { - path: { image: values.imageName }, - query: { project }, - }) + q( + api.imageView, + { path: { image: values.imageName }, query: { project } }, + { + errorsExpected: { + explanation: 'the image name may not exist yet.', + statusCode: 404, + }, + } + ) ) .catch((e) => { // eat a 404 since that's what we want. anything else should still blow up - if (e.statusCode === 404) { - console.info( - '/v1/images 404 is expected. It means the image name is not taken.' - ) - return null - } + if (e.statusCode === 404) return null throw e }) if (image) { diff --git a/app/pages/project/instances/NetworkingTab.tsx b/app/pages/project/instances/NetworkingTab.tsx index 994836927b..f7dfc6aa73 100644 --- a/app/pages/project/instances/NetworkingTab.tsx +++ b/app/pages/project/instances/NetworkingTab.tsx @@ -130,11 +130,18 @@ export async function clientLoader({ params }: LoaderFunctionArgs) { .fetchQuery(q(api.ipPoolList, { query: { limit: ALL_ISH } })) .then((pools) => { for (const pool of pools.items) { - // both IpPoolCell and the fetch in the model use errors-allowed + // both IpPoolCell and the fetch in the modal use errors-allowed // versions to avoid blowing up in the unlikely event of an error - const { queryKey } = qErrorsAllowed(api.ipPoolView, { - path: { pool: pool.id }, - }) + const { queryKey } = qErrorsAllowed( + api.ipPoolView, + { path: { pool: pool.id } }, + { + errorsExpected: { + explanation: 'the referenced IP pool may have been deleted.', + statusCode: 404, + }, + } + ) queryClient.setQueryData(queryKey, { type: 'success', data: pool }) } }), diff --git a/app/pages/project/snapshots/SnapshotsPage.tsx b/app/pages/project/snapshots/SnapshotsPage.tsx index bcbe981112..fbf4c2a57d 100644 --- a/app/pages/project/snapshots/SnapshotsPage.tsx +++ b/app/pages/project/snapshots/SnapshotsPage.tsx @@ -41,6 +41,18 @@ import { TableActions } from '~/ui/lib/Table' import { docLinks } from '~/util/links' import { pb } from '~/util/path-builder' +const diskViewErrorsAllowedQ = (disk: string) => + qErrorsAllowed( + api.diskView, + { path: { disk } }, + { + errorsExpected: { + explanation: 'the source disk may have been deleted.', + statusCode: 404, + }, + } + ) + const DiskNameFromId = ({ value, onClick, @@ -48,7 +60,7 @@ const DiskNameFromId = ({ value: string onClick: (disk: Disk) => void }) => { - const { data } = useQuery(qErrorsAllowed(api.diskView, { path: { disk: value } })) + const { data } = useQuery(diskViewErrorsAllowedQ(value)) if (!data) return if (data.type === 'error') return Deleted @@ -85,10 +97,10 @@ export async function clientLoader({ params }: LoaderFunctionArgs) { .fetchQuery(q(api.diskList, { query: { project, limit: 200 } })) .then((disks) => { for (const disk of disks.items) { - queryClient.setQueryData( - qErrorsAllowed(api.diskView, { path: { disk: disk.id } }).queryKey, - { type: 'success', data: disk } - ) + queryClient.setQueryData(diskViewErrorsAllowedQ(disk.id).queryKey, { + type: 'success', + data: disk, + }) } }), ]) diff --git a/app/pages/system/silos/SiloScimTab.tsx b/app/pages/system/silos/SiloScimTab.tsx index e6b5b62333..5aeeee375b 100644 --- a/app/pages/system/silos/SiloScimTab.tsx +++ b/app/pages/system/silos/SiloScimTab.tsx @@ -44,6 +44,18 @@ import { docLinks } from '~/util/links' export const handle = makeCrumb('SCIM') +const scimTokenListErrorsAllowedQ = (silo: string) => + qErrorsAllowed( + api.scimTokenList, + { query: { silo } }, + { + errorsExpected: { + explanation: 'the current user may not have permission to list SCIM tokens.', + statusCode: 403, + }, + } + ) + const colHelper = createColumnHelper() const staticColumns = [ colHelper.accessor('id', { @@ -74,7 +86,7 @@ const EmptyState = () => ( export async function clientLoader({ params }: LoaderFunctionArgs) { const { silo } = getSiloSelector(params) // Use errors-allowed approach so 403s don't throw and break the loader - await queryClient.prefetchQuery(qErrorsAllowed(api.scimTokenList, { query: { silo } })) + await queryClient.prefetchQuery(scimTokenListErrorsAllowedQ(silo)) return null } @@ -86,7 +98,7 @@ type ModalState = export default function SiloScimTab() { const siloSelector = useSiloSelector() const { data: tokensResult } = usePrefetchedQuery( - qErrorsAllowed(api.scimTokenList, { query: siloSelector }) + scimTokenListErrorsAllowedQ(siloSelector.silo) ) const [modalState, setModalState] = useState(false) diff --git a/app/table/cells/IpPoolCell.tsx b/app/table/cells/IpPoolCell.tsx index 58383e0f8d..e4cb6ba4da 100644 --- a/app/table/cells/IpPoolCell.tsx +++ b/app/table/cells/IpPoolCell.tsx @@ -14,7 +14,16 @@ import { EmptyCell, SkeletonCell } from './EmptyCell' export const IpPoolCell = ({ ipPoolId }: { ipPoolId: string }) => { const { data: result } = useQuery( - qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } }) + qErrorsAllowed( + api.ipPoolView, + { path: { pool: ipPoolId } }, + { + errorsExpected: { + explanation: 'the referenced IP pool may have been deleted.', + statusCode: 404, + }, + } + ) ) if (!result) return // this should essentially never happen, but it's probably better than blowing diff --git a/app/table/cells/IpVersionCell.tsx b/app/table/cells/IpVersionCell.tsx index 0b66e12bea..7961341354 100644 --- a/app/table/cells/IpVersionCell.tsx +++ b/app/table/cells/IpVersionCell.tsx @@ -14,7 +14,16 @@ import { EmptyCell, SkeletonCell } from './EmptyCell' export const IpVersionCell = ({ ipPoolId }: { ipPoolId: string }) => { const { data: result } = useQuery( - qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } }) + qErrorsAllowed( + api.ipPoolView, + { path: { pool: ipPoolId } }, + { + errorsExpected: { + explanation: 'the referenced IP pool may have been deleted.', + statusCode: 404, + }, + } + ) ) if (!result) return if (result.type === 'error') return diff --git a/test/e2e/error-pages.e2e.ts b/test/e2e/error-pages.e2e.ts index c92d743048..79d21a6691 100644 --- a/test/e2e/error-pages.e2e.ts +++ b/test/e2e/error-pages.e2e.ts @@ -5,9 +5,7 @@ * * Copyright Oxide Computer Company */ -import { expect, test } from '@playwright/test' - -import { getPageAsUser, hasConsoleMessage } from './utils' +import { expect, expectConsoleMessage, getPageAsUser, test } from './utils' test('Shows 404 page when a resource is not found', async ({ page }) => { await page.goto('/nonexistent') @@ -19,22 +17,17 @@ test('Shows 404 page when a resource is not found', async ({ page }) => { await expect(page.getByRole('button', { name: 'Sign out' })).toBeVisible() }) -test('Shows something went wrong page on other errors', async ({ page, browserName }) => { +test('Shows something went wrong page on other errors', async ({ page }) => { await page.goto('/projects/error-503') // specially handled in mock server await expect(page.getByText('Something went wrong')).toBeVisible() // Invariant failed doesn't show up in the page... await expect(page.getByText('Invariant failed')).toBeHidden() - // But we do see it in the browser console. Skip Firefox because it handles - // these errors differently and it's hard to get the error text out. - // eslint-disable-next-line playwright/no-conditional-in-test - if (browserName !== 'firefox') { - const error = - 'Expected query to be prefetched.\nKey: ["projectView",{"path":{"project":"error-503"}}]' - // eslint-disable-next-line playwright/no-conditional-expect - expect(await hasConsoleMessage(page, error)).toBeTruthy() - } + // But we do see it in the browser console + const error = + 'Expected query to be prefetched.\nKey: ["projectView",{"path":{"project":"error-503"}}]' + await expectConsoleMessage(page, error) // test clicking sign out await page.getByRole('button', { name: 'Sign out' }).click() diff --git a/test/e2e/image-upload.e2e.ts b/test/e2e/image-upload.e2e.ts index cc98bd0e4e..cfb3f97e03 100644 --- a/test/e2e/image-upload.e2e.ts +++ b/test/e2e/image-upload.e2e.ts @@ -9,6 +9,7 @@ import { expect, test, type Page } from '@playwright/test' import { chooseFile, + expectConsoleMessage, expectNotVisible, expectRowVisible, expectVisible, @@ -69,6 +70,10 @@ test.describe('Image upload', () => { // now the modal pops open and the thing starts going await expectUploadProcess(page) + // the image name check 404 should be logged as expected-info, with context + await expectConsoleMessage(page, 'This error is expected', 'info') + await expectConsoleMessage(page, 'the image name may not exist yet.', 'info') + await expect(page).toHaveURL('/projects/mock-project/images') await expectRowVisible(page.locator('role=table'), { name: 'new-image', diff --git a/test/e2e/instance-metrics.e2e.ts b/test/e2e/instance-metrics.e2e.ts index effaecc081..5047bb6eb9 100644 --- a/test/e2e/instance-metrics.e2e.ts +++ b/test/e2e/instance-metrics.e2e.ts @@ -10,7 +10,7 @@ import { expect, test } from '@playwright/test' import { OXQL_GROUP_BY_ERROR } from '~/api' -import { getPageAsUser, hasConsoleMessage } from './utils' +import { expectConsoleMessage, expectNoConsoleMessage, getPageAsUser } from './utils' test('Click through instance metrics', async ({ page }) => { await page.goto('/projects/mock-project/instances/db1/metrics/cpu') @@ -67,14 +67,14 @@ test('empty and loading states', async ({ page }) => { await expect(noData).toBeVisible() // idle state returns group_by must be aligned error, treated as empty - expect(await hasConsoleMessage(page, OXQL_GROUP_BY_ERROR)).toBe(false) // error not in console + await expectNoConsoleMessage(page, OXQL_GROUP_BY_ERROR) await statePicker.click() await page.getByRole('option', { name: 'State: Idle' }).click() await expect(loading).toBeVisible() await expect(loading).toBeHidden() await expect(page.getByText('Something went wrong')).toBeHidden() await expect(noData).toBeVisible() - expect(await hasConsoleMessage(page, OXQL_GROUP_BY_ERROR)).toBe(true) // error present in console + await expectConsoleMessage(page, OXQL_GROUP_BY_ERROR) // make sure empty state goes away again for the first one await statePicker.click() diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index 77dc01a329..e573ef51b6 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -5,7 +5,7 @@ * * Copyright Oxide Computer Company */ -import { expect, type Browser, type Locator, type Page } from '@playwright/test' +import { expect, test, type Browser, type Locator, type Page } from '@playwright/test' import { MiB } from '~/util/units' @@ -286,7 +286,25 @@ export async function addTlsCert(page: Page) { await page.getByRole('button', { name: 'Add Certificate' }).click() } -export async function hasConsoleMessage(page: Page, msg: string) { +/** + * Assert that a console message matching `msg` was logged (optionally at a + * given level). Skips on Firefox because Playwright's `page.consoleMessages()` + * drops real console output there and only returns React profiling entries. + */ +export async function expectConsoleMessage(page: Page, msg: string, type?: string) { + // eslint-disable-next-line playwright/no-conditional-in-test + if (test.info().project.name === 'firefox') return + const messages = await page.consoleMessages() + const match = messages.find((m) => m.text().includes(msg)) + expect(match, `expected console message containing "${msg}"`).toBeTruthy() + if (type) expect(match!.type()).toBe(type) +} + +/** Assert that no console message matching `msg` was logged. Skips on Firefox. */ +export async function expectNoConsoleMessage(page: Page, msg: string) { + // eslint-disable-next-line playwright/no-conditional-in-test + if (test.info().project.name === 'firefox') return const messages = await page.consoleMessages() - return messages.some((m) => m.text().includes(msg)) + const match = messages.find((m) => m.text().includes(msg)) + expect(match, `expected no console message containing "${msg}"`).toBeFalsy() }