Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions console/src/api/cloudGlobalApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export async function getCostsBreakdownDaily(
requestOptions: OpenApiRequestOptions = {},
) {
const { headers, ...options } = requestOptions;
const { data, response } = await getClient().GET(
const { data, error, response } = await getClient().GET(
"/api/costs/breakdown/daily",
{
params: {
Expand All @@ -173,7 +173,7 @@ export async function getCostsBreakdownDaily(
...options,
},
);
return handleOpenApiResponseWithBody(data, response);
return handleOpenApiResponseWithBody(data, response, error);
}

export async function createStripeSetupIntent(
Expand Down
16 changes: 16 additions & 0 deletions console/src/api/mocks/cloudGlobalApiHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
Region,
Regions,
} from "~/api/cloudGlobalApi";
import { ApiError } from "~/api/openApiUtils";

export const buildCloudRegionsReponse = (
options: {
Expand Down Expand Up @@ -52,6 +53,21 @@ export const buildDailyCostBreakdownResponse = (
return HttpResponse.json(payload, { status: options.status ?? 200 });
});

/** A structured `ApiError` error response (SAS-172), e.g. for asserting the
* console renders the real message/request id instead of a generic fallback. */
export const buildDailyCostBreakdownErrorResponse = (
options: { apiError?: Partial<ApiError>; status?: number } = {},
) =>
http.get("*/api/costs/breakdown/daily", () => {
const payload: ApiError = {
reason: "internal_error",
message: "There was an error servicing this request.",
requestId: "11111111-1111-1111-1111-111111111111",
...options.apiError,
};
return HttpResponse.json(payload, { status: options.status ?? 500 });
});

export const buildInvoicesResponse = (
options: { invoices?: Invoice[]; status?: number } = {},
) =>
Expand Down
53 changes: 48 additions & 5 deletions console/src/api/openApiUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,71 @@
// by the Apache License, Version 2.0.

import { OpenApiFetchError } from "./OpenApiFetchError";
import { components } from "./schemas/global-api";

export async function handleOpenApiResponse<T>(
export type ApiError = components["schemas"]["ApiError"];

/**
* Extracts the structured `ApiError` body from a caught error, when the
* failing endpoint has been migrated to return one (SAS-172). Endpoints
* that haven't yet return a plain-text or otherwise-shaped body, so callers
* must still have a generic fallback for when this returns `null`.
*/
export function getApiError(error: unknown): ApiError | null {
if (
error instanceof OpenApiFetchError &&
typeof error.body === "object" &&
error.body !== null &&
"reason" in error.body &&
"message" in error.body &&
"requestId" in error.body
) {
return error.body as ApiError;
}
return null;
}

// `openapi-fetch` splits a response into `data` (success schema) or `error`
// (error-status schema), never both: `data` is always `undefined` on a
// non-2xx response, `error` is always `undefined` on a 2xx one. Callers that
// only ever passed `data` here could never surface a JSON error body, no
// matter what the backend sent, which is the deeper cause behind SAS-149's
// "Empty response" fallback masking real backend messages. `error` is
// optional so existing call sites that don't pass it keep working (they fall
// back to the generic "Empty response" string exactly as before); pass it
// through for endpoints migrated to a structured error body (SAS-172).
export async function handleOpenApiResponse<T, E = unknown>(
data: T | undefined,
response: Response,
error?: E,
) {
if (!response.ok) {
throw new OpenApiFetchError(response.status, data ?? "Empty response");
throw new OpenApiFetchError(
response.status,
error ?? data ?? "Empty response",
);
}
return {
...response,
data,
};
}
export async function handleOpenApiResponseWithBody<T>(
export async function handleOpenApiResponseWithBody<T, E = unknown>(
data: T | undefined,
response: Response,
error?: E,
) {
if (!response.ok) {
throw new OpenApiFetchError(response.status, data ?? "Empty response");
throw new OpenApiFetchError(
response.status,
error ?? data ?? "Empty response",
);
}
if (!data) {
throw new OpenApiFetchError(response.status, data ?? "Empty response");
throw new OpenApiFetchError(
response.status,
error ?? data ?? "Empty response",
);
}
return {
...response,
Expand Down
39 changes: 37 additions & 2 deletions console/src/api/schemas/global-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,29 @@ export interface components {
/** @description Storage resource costs. */
storage: components["schemas"]["Cost_StoragePrice"];
};
/**
* @description A structured JSON error response.
*
* `reason` is a closed, safe-by-construction category (see [`ErrorReason`]).
* `message` is curated text safe to show a customer: for `Validation`, it
* may describe the problem specifically, since it only concerns the
* caller's own request; for every other reason it is a fixed, generic
* string, chosen by the constructor, never raw upstream or internal error
* text. `request_id` correlates this response to the full, unredacted
* error in server-side logs.
*
* Construct via the `ApiError::validation`/`forbidden`/`not_found`/
* `upstream_error`/`upstream_limit_exceeded`/`internal` functions, not by
* hand, so every call site logs the real cause exactly once (except
* `validation`, which has no separate "real cause": the message *is* the
* cause) and gets a request id for free.
*/
ApiError: {
reason: components["schemas"]["ErrorReason"];
message: string;
/** Format: uuid */
requestId: string;
};
Card: {
/** @description The last 4 digits of the card number. */
last4: string;
Expand Down Expand Up @@ -494,6 +517,14 @@ export interface components {
DetachPaymentMethodRequest: {
paymentMethodId: string;
};
/**
* @description A small, closed set of safe-to-expose error categories. Never add a
* variant, or a call site, that could put upstream (Orb, Stripe, ...) error
* text, internal service/host names, or infra topology into a response —
* see `ApiError`.
* @enum {string}
*/
ErrorReason: "validation" | "forbidden" | "not_found" | "upstream_error" | "upstream_limit_exceeded" | "internal_error";
GetSelfManagedSubscriptionResponse: {
/** Format: date-time */
endDate: string;
Expand Down Expand Up @@ -823,14 +854,18 @@ export interface operations {
headers: {
[name: string]: unknown;
};
content?: never;
content: {
"application/json": components["schemas"]["ApiError"];
};
};
/** @description Insufficient permissions */
403: {
headers: {
[name: string]: unknown;
};
content?: never;
content: {
"application/json": components["schemas"]["ApiError"];
};
};
};
};
Expand Down
20 changes: 19 additions & 1 deletion console/src/platform/billing/AccountSpendBreakdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
CostBreakdownCluster,
CostBreakdownDay,
} from "~/api/cloudGlobalApi";
import { getApiError } from "~/api/openApiUtils";
import ErrorBox from "~/components/ErrorBox";
import { GraphEventOverlay, GraphTooltip } from "~/components/graphComponents";
import { LoadingContainer } from "~/components/LoadingContainer";
Expand Down Expand Up @@ -625,6 +626,7 @@ const AccountSpendBreakdown = ({
days,
isLoading,
isError,
error,
regionFilter,
setRegionFilter,
timeRange,
Expand All @@ -634,6 +636,7 @@ const AccountSpendBreakdown = ({
days,
regionFilter,
);
const apiError = getApiError(error);

return (
<Box data-testid="account-spend-breakdown">
Expand All @@ -650,7 +653,22 @@ const AccountSpendBreakdown = ({
// data lands.
<LoadingContainer minHeight={`${chartHeightPx}px`} />
) : isError ? (
<ErrorBox message={ACCOUNT_SPEND_FETCH_ERROR_MESSAGE} />
<ErrorBox
message={apiError?.message ?? ACCOUNT_SPEND_FETCH_ERROR_MESSAGE}
>
{/* A validation error describes the caller's own request (e.g. an
out-of-range date), so there's nothing for support to look up;
every other reason gets a reference for support correlation. */}
{apiError && apiError.reason !== "validation" && (
<Text
fontSize="sm"
color="gray.500"
data-testid="api-error-request-id"
>
Reference: {apiError.requestId}
</Text>
)}
</ErrorBox>
) : !days || accountIds.length === 0 ? (
<EmptyState />
) : (
Expand Down
49 changes: 49 additions & 0 deletions console/src/platform/billing/UsagePage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
buildCloudOrganizationsResponse,
buildCloudRegionsReponse,
buildCreditsResponse,
buildDailyCostBreakdownErrorResponse,
buildDailyCostBreakdownResponse,
buildInvoicesResponse,
} from "~/api/mocks/cloudGlobalApiHandlers";
Expand Down Expand Up @@ -250,6 +251,54 @@ describe("UsagePage", () => {
).toBeVisible();
});

it("shows the backend's real message and a request id for a structured API error (SAS-172)", async () => {
server.use(
buildDailyCostBreakdownErrorResponse({
apiError: {
reason: "upstream_limit_exceeded",
message:
"This account has more billing configuration than we can currently summarize. Contact support with the reference below.",
requestId: "22222222-2222-2222-2222-222222222222",
},
}),
);
renderComponent(<UsagePage />);
expect(
await screen.findByText(
"This account has more billing configuration than we can currently summarize. Contact support with the reference below.",
),
).toBeVisible();
expect(await screen.findByTestId("api-error-request-id")).toHaveTextContent(
"22222222-2222-2222-2222-222222222222",
);
// The generic fallback must not also render alongside the real message.
expect(
screen.queryByText("An error occurred loading your usage"),
).not.toBeInTheDocument();
});

it("does not show a request id for a validation error (the caller's own request, not something support needs to look up)", async () => {
server.use(
buildDailyCostBreakdownErrorResponse({
status: 400,
apiError: {
reason: "validation",
message: "startDate cannot be more than 100 days in the past",
requestId: "33333333-3333-3333-3333-333333333333",
},
}),
);
renderComponent(<UsagePage />);
expect(
await screen.findByText(
"startDate cannot be more than 100 days in the past",
),
).toBeVisible();
expect(
screen.queryByTestId("api-error-request-id"),
).not.toBeInTheDocument();
});

it("shows an empty state when the window has no usage", async () => {
// beforeEach's default handler already returns { days: [] }.
renderComponent(<UsagePage />);
Expand Down
Loading