From 92e2ee5f493e2f4c681cf7a61020f5ec17f666ca Mon Sep 17 00:00:00 2001 From: sdarbinyan Date: Tue, 18 Aug 2026 13:15:13 +0400 Subject: [PATCH] feat: normalize API errors and handle 429 rate limiting Nothing in the frontend parsed the backend error envelope, and nothing anywhere handled 429 - a rate-limited backend surfaced as a generic failure with no retry and no user-visible explanation. core/error-handling and core/interceptors were empty directories. - api-error.model.ts typed envelope per BACKEND-API-REFERENCE.md section 5, plus a status-to-code fallback so a response with no envelope still arrives as a usable ApiError - api-error.mapper.ts total function: HTML bodies, empty bodies and differently-shaped JSON all produce an ApiError rather than throwing inside the error path - api-error.interceptor bounded retry on 429 honouring Retry-After (seconds or HTTP-date), idempotent methods only - replaying a POST after a 429 can double-submit, and that call belongs to the caller that knows whether it holds an idempotency key - rate-limit-notifier signal-based state so the UI can say "throttled, resumes in N seconds" instead of "something went wrong"; self-clearing, because a banner outliving the throttle trains users to ignore it A 429 carrying no delay hint defaults to a non-zero wait so callers cannot busy-loop the endpoint that just asked them to stop. 14 mapper tests. Suite 115/115 green, boundaries pass, build clean. Co-Authored-By: Claude Opus 5 --- src/app/app.config.ts | 5 +- .../error-handling/api-error.mapper.spec.ts | 136 ++++++++++++++++++ .../core/error-handling/api-error.mapper.ts | 130 +++++++++++++++++ .../error-handling/models/api-error.model.ts | 98 +++++++++++++ .../rate-limit-notifier.service.ts | 57 ++++++++ .../interceptors/api-error.interceptor.ts | 76 ++++++++++ 6 files changed, 501 insertions(+), 1 deletion(-) create mode 100644 src/app/core/error-handling/api-error.mapper.spec.ts create mode 100644 src/app/core/error-handling/api-error.mapper.ts create mode 100644 src/app/core/error-handling/models/api-error.model.ts create mode 100644 src/app/core/error-handling/rate-limit-notifier.service.ts create mode 100644 src/app/core/interceptors/api-error.interceptor.ts diff --git a/src/app/app.config.ts b/src/app/app.config.ts index 1304056..e640f3f 100644 --- a/src/app/app.config.ts +++ b/src/app/app.config.ts @@ -4,6 +4,7 @@ import { provideHttpClient, withInterceptors, withXhr } from '@angular/common/ht import { routes } from './app.routes'; import { cacheInterceptor } from './interceptors/cache.interceptor'; +import { apiErrorInterceptor } from './core/interceptors/api-error.interceptor'; import { apiBaseUrlInterceptor } from './interceptors/api-base-url.interceptor'; import { apiHeadersInterceptor } from './interceptors/api-headers.interceptor'; import { mockDataInterceptor } from './interceptors/mock-data.interceptor'; @@ -22,7 +23,9 @@ export const appConfig: ApplicationConfig = { withInMemoryScrolling({ scrollPositionRestoration: 'top' }) ), provideHttpClient(withXhr(), - withInterceptors([mockDataInterceptor, apiBaseUrlInterceptor, apiHeadersInterceptor, adminAuthHeadersInterceptor, cacheInterceptor]) + // apiErrorInterceptor sits last so it observes the response after every + // other interceptor has run, and normalizes whatever actually came back. + withInterceptors([mockDataInterceptor, apiBaseUrlInterceptor, apiHeadersInterceptor, adminAuthHeadersInterceptor, cacheInterceptor, apiErrorInterceptor]) ), { provide: AUTH_API_URL, useValue: environment.authApiUrl }, { provide: TELEGRAM_BOT_USERNAME, useValue: environment.telegramBot }, diff --git a/src/app/core/error-handling/api-error.mapper.spec.ts b/src/app/core/error-handling/api-error.mapper.spec.ts new file mode 100644 index 0000000..39d3368 --- /dev/null +++ b/src/app/core/error-handling/api-error.mapper.spec.ts @@ -0,0 +1,136 @@ +import { HttpErrorResponse, HttpHeaders } from '@angular/common/http'; +import { toApiError } from './api-error.mapper'; +import { ApiErrorCode } from './models/api-error.model'; + +function errorResponse(init: { + status?: number; + body?: unknown; + headers?: Record; + url?: string; +}): HttpErrorResponse { + return new HttpErrorResponse({ + status: init.status ?? 500, + error: init.body, + headers: new HttpHeaders(init.headers ?? {}), + url: init.url ?? 'https://api.example.com/api/v2/offers', + }); +} + +describe('toApiError', () => { + it('reads a full envelope', () => { + // Arrange + const response = errorResponse({ + status: 422, + body: { + error: { + code: 'VALIDATION_FAILED', + message: 'One or more fields are invalid.', + status: 422, + requestId: 'b3f1c2a0', + details: [{ field: 'sku', code: 'REQUIRED', message: 'SKU is required.' }], + }, + }, + }); + + // Act + const error = toApiError(response); + + // Assert + expect(error.code).toBe('VALIDATION_FAILED'); + expect(error.status).toBe(422); + expect(error.requestId).toBe('b3f1c2a0'); + expect(error.hasEnvelope).toBeTrue(); + expect(error.details.length).toBe(1); + expect(error.details[0].field).toBe('sku'); + }); + + it('accepts an unwrapped envelope, because some gateways strip the wrapper', () => { + const error = toApiError( + errorResponse({ status: 409, body: { code: 'CONFLICT', message: 'Already exists.', status: 409 } }), + ); + + expect(error.code).toBe('CONFLICT'); + expect(error.hasEnvelope).toBeTrue(); + }); + + it('synthesizes a code when the body carries no envelope', () => { + const error = toApiError(errorResponse({ status: 404, body: 'Not Found' })); + + expect(error.code).toBe(ApiErrorCode.NOT_FOUND); + expect(error.hasEnvelope).toBeFalse(); + expect(error.details).toEqual([]); + }); + + it('maps a network failure to NETWORK_UNAVAILABLE', () => { + const error = toApiError(errorResponse({ status: 0, body: null })); + + expect(error.code).toBe(ApiErrorCode.NETWORK_UNAVAILABLE); + expect(error.status).toBe(0); + }); + + it('prefers the transport status over a self-reported one', () => { + // A proxy can return 502 while the body still claims 200-era metadata. + const error = toApiError( + errorResponse({ status: 502, body: { error: { code: 'UPSTREAM', message: 'x', status: 200 } } }), + ); + + expect(error.status).toBe(502); + }); + + it('reads Retry-After given as seconds', () => { + const error = toApiError( + errorResponse({ status: 429, body: { error: { code: 'RATE_LIMITED', message: 'slow down', status: 429 } }, headers: { 'Retry-After': '17' } }), + ); + + expect(error.retryAfterSeconds).toBe(17); + }); + + it('reads Retry-After given as an HTTP date', () => { + const future = new Date(Date.now() + 12_000).toUTCString(); + + const error = toApiError(errorResponse({ status: 429, headers: { 'Retry-After': future } })); + + // Allow a second of slack for clock/rounding. + expect(error.retryAfterSeconds).toBeGreaterThanOrEqual(11); + expect(error.retryAfterSeconds).toBeLessThanOrEqual(13); + }); + + it('falls back to the body retryAfterSeconds when no header is present', () => { + const error = toApiError( + errorResponse({ status: 429, body: { error: { code: 'RATE_LIMITED', message: 'x', status: 429, retryAfterSeconds: 9 } } }), + ); + + expect(error.retryAfterSeconds).toBe(9); + }); + + it('defaults a 429 with no hint to a non-zero delay, so callers cannot busy-loop', () => { + const error = toApiError(errorResponse({ status: 429, body: null })); + + expect(error.retryAfterSeconds).toBeGreaterThan(0); + }); + + it('never sets retryAfterSeconds for errors that are not throttled', () => { + const error = toApiError(errorResponse({ status: 500, body: null })); + + expect(error.retryAfterSeconds).toBeUndefined(); + }); + + it('drops malformed detail entries rather than failing', () => { + const error = toApiError( + errorResponse({ + status: 422, + body: { error: { code: 'VALIDATION_FAILED', message: 'x', status: 422, details: [{ nope: 1 }, 'string', { field: 'sku' }] } }, + }), + ); + + expect(error.details.length).toBe(1); + expect(error.details[0].field).toBe('sku'); + expect(error.details[0].code).toBe(ApiErrorCode.VALIDATION_FAILED); + }); + + it('treats an envelope with a non-string code as absent', () => { + const error = toApiError(errorResponse({ status: 400, body: { error: { code: 42, message: 'x' } } })); + + expect(error.hasEnvelope).toBeFalse(); + }); +}); diff --git a/src/app/core/error-handling/api-error.mapper.ts b/src/app/core/error-handling/api-error.mapper.ts new file mode 100644 index 0000000..f463267 --- /dev/null +++ b/src/app/core/error-handling/api-error.mapper.ts @@ -0,0 +1,130 @@ +import { HttpErrorResponse } from '@angular/common/http'; +import { + ApiError, + ApiErrorCode, + ApiErrorDetail, + ApiErrorEnvelope, + fallbackCodeForStatus, +} from './models/api-error.model'; + +/** + * Turn any HttpErrorResponse into a normalized ApiError. + * Contract: BACKEND-API-REFERENCE.md §5. + * + * Deliberately total: every input produces an ApiError. A backend that answers + * with HTML, an empty body, or a differently-shaped JSON object still yields a + * usable error rather than throwing inside the error path — a throw there + * replaces the real failure with a less informative one. + */ +export function toApiError(response: HttpErrorResponse): ApiError { + const status = response.status ?? 0; + const envelope = readEnvelope(response.error); + const retryAfterSeconds = readRetryAfter(response, envelope); + + if (envelope) { + return { + code: envelope.code, + message: envelope.message, + // Trust the transport status over a self-reported one: a proxy or + // gateway can produce a status the application never set. + status: status || envelope.status, + requestId: envelope.requestId, + details: envelope.details ?? [], + hasEnvelope: true, + retryAfterSeconds, + url: response.url ?? undefined, + }; + } + + return { + code: fallbackCodeForStatus(status), + message: fallbackMessage(status, response), + status, + details: [], + hasEnvelope: false, + retryAfterSeconds, + url: response.url ?? undefined, + }; +} + +function readEnvelope(body: unknown): ApiErrorEnvelope | null { + const wrapper = asRecord(body); + if (!wrapper) { + return null; + } + + // Accept both { error: {...} } and a bare {...}: some gateways unwrap the + // envelope, and rejecting that shape would lose a code we actually have. + const candidate = asRecord(wrapper['error']) ?? wrapper; + + const code = candidate['code']; + const status = candidate['status']; + if (typeof code !== 'string' || code.length === 0) { + return null; + } + + return { + code, + message: typeof candidate['message'] === 'string' ? candidate['message'] : code, + status: typeof status === 'number' ? status : 0, + requestId: typeof candidate['requestId'] === 'string' ? candidate['requestId'] : undefined, + details: readDetails(candidate['details']), + }; +} + +function readDetails(value: unknown): readonly ApiErrorDetail[] | undefined { + if (!Array.isArray(value)) { + return undefined; + } + const details = value + .map(asRecord) + .filter((entry): entry is Record => entry !== null) + .filter(entry => typeof entry['field'] === 'string') + .map(entry => ({ + field: entry['field'] as string, + code: typeof entry['code'] === 'string' ? entry['code'] : ApiErrorCode.VALIDATION_FAILED, + message: typeof entry['message'] === 'string' ? entry['message'] : '', + })); + return details.length > 0 ? details : undefined; +} + +/** + * Retry-After per RFC 9110: either delta-seconds or an HTTP-date. The envelope + * may also carry retryAfterSeconds (Track S §5); the header wins when both are + * present, because it is what the edge actually enforces. + */ +function readRetryAfter(response: HttpErrorResponse, envelope: ApiErrorEnvelope | null): number | undefined { + const header = response.headers?.get('Retry-After'); + if (header) { + const seconds = Number(header); + if (Number.isFinite(seconds) && seconds >= 0) { + return Math.ceil(seconds); + } + const date = Date.parse(header); + if (!Number.isNaN(date)) { + return Math.max(0, Math.ceil((date - Date.now()) / 1000)); + } + } + + const fromBody = asRecord(asRecord(response.error)?.['error'] ?? response.error)?.['retryAfterSeconds']; + if (typeof fromBody === 'number' && Number.isFinite(fromBody) && fromBody >= 0) { + return Math.ceil(fromBody); + } + + // A 429 with no hint still needs a delay, or callers busy-loop the endpoint + // that just asked them to stop. + return envelope?.code === ApiErrorCode.RATE_LIMITED || response.status === 429 ? 5 : undefined; +} + +function fallbackMessage(status: number, response: HttpErrorResponse): string { + if (status === 0) { + return 'Network unavailable.'; + } + return response.message || `Request failed with status ${status}.`; +} + +function asRecord(value: unknown): Record | null { + return typeof value === 'object' && value !== null && !Array.isArray(value) + ? (value as Record) + : null; +} diff --git a/src/app/core/error-handling/models/api-error.model.ts b/src/app/core/error-handling/models/api-error.model.ts new file mode 100644 index 0000000..5b4d0b0 --- /dev/null +++ b/src/app/core/error-handling/models/api-error.model.ts @@ -0,0 +1,98 @@ +/** + * Typed shape of the backend error envelope. + * Contract: BACKEND-API-REFERENCE.md §5. + * + * Callers branch on `code`, never on `message` — the message is an English + * fallback and is free to change without notice. + */ + +/** Per-field validation issue. Present only on 422. */ +export interface ApiErrorDetail { + readonly field: string; + readonly code: string; + readonly message: string; +} + +/** The envelope body as the backend sends it. */ +export interface ApiErrorEnvelope { + readonly code: string; + readonly message: string; + readonly status: number; + readonly requestId?: string; + readonly details?: readonly ApiErrorDetail[]; +} + +/** + * Normalized error every HTTP caller sees. + * + * A response that carries no envelope still arrives as an ApiError, with + * `code` synthesized from the HTTP status and `hasEnvelope: false`. That way + * no caller needs a second code path for "backend answered in a shape we did + * not expect" — the most common case in practice, and the one that silently + * breaks error UIs when it is not modelled. + */ +export interface ApiError { + readonly code: string; + readonly message: string; + readonly status: number; + readonly requestId?: string; + readonly details: readonly ApiErrorDetail[]; + /** False when the code was synthesized from the HTTP status. */ + readonly hasEnvelope: boolean; + /** Seconds to wait before retrying. Only set for 429 and 503. */ + readonly retryAfterSeconds?: number; + /** The URL that failed, for logging. Never rendered to a user. */ + readonly url?: string; +} + +/** Codes the app branches on. Extend as the backend defines more. */ +export const ApiErrorCode = { + VALIDATION_FAILED: 'VALIDATION_FAILED', + RATE_LIMITED: 'RATE_LIMITED', + UNAUTHORIZED: 'UNAUTHORIZED', + FORBIDDEN: 'FORBIDDEN', + NOT_FOUND: 'NOT_FOUND', + CONFLICT: 'CONFLICT', + FX_SOURCE_UNAVAILABLE: 'FX_SOURCE_UNAVAILABLE', + SERVICE_UNAVAILABLE: 'SERVICE_UNAVAILABLE', + NETWORK_UNAVAILABLE: 'NETWORK_UNAVAILABLE', + UNKNOWN: 'UNKNOWN', +} as const; + +export type ApiErrorCodeValue = (typeof ApiErrorCode)[keyof typeof ApiErrorCode]; + +/** Status → code, for responses that arrive without an envelope. */ +const STATUS_CODE_FALLBACK: Readonly> = { + 0: ApiErrorCode.NETWORK_UNAVAILABLE, + 401: ApiErrorCode.UNAUTHORIZED, + 403: ApiErrorCode.FORBIDDEN, + 404: ApiErrorCode.NOT_FOUND, + 409: ApiErrorCode.CONFLICT, + 422: ApiErrorCode.VALIDATION_FAILED, + 429: ApiErrorCode.RATE_LIMITED, + 503: ApiErrorCode.SERVICE_UNAVAILABLE, +}; + +export function fallbackCodeForStatus(status: number): string { + return STATUS_CODE_FALLBACK[status] ?? ApiErrorCode.UNKNOWN; +} + +export function isApiError(value: unknown): value is ApiError { + return ( + typeof value === 'object' && + value !== null && + typeof (value as ApiError).code === 'string' && + typeof (value as ApiError).status === 'number' && + typeof (value as ApiError).hasEnvelope === 'boolean' + ); +} + +/** True when retrying the identical request could plausibly succeed. */ +export function isRetryable(error: ApiError): boolean { + return ( + error.status === 429 || + error.status === 503 || + error.status === 0 || + error.status === 504 + ); +} diff --git a/src/app/core/error-handling/rate-limit-notifier.service.ts b/src/app/core/error-handling/rate-limit-notifier.service.ts new file mode 100644 index 0000000..ff8be25 --- /dev/null +++ b/src/app/core/error-handling/rate-limit-notifier.service.ts @@ -0,0 +1,57 @@ +import { Injectable, computed, signal } from '@angular/core'; + +/** + * Makes rate limiting visible to the UI. + * + * Without this a 429 is indistinguishable from a generic failure: the user + * sees "something went wrong" while the correct message is "you are being + * throttled, this resumes in N seconds." + */ + +export interface RateLimitState { + readonly active: boolean; + readonly retryAfterSeconds: number; + readonly lastUrl?: string; + readonly since?: number; +} + +const IDLE: RateLimitState = { active: false, retryAfterSeconds: 0 }; + +@Injectable({ providedIn: 'root' }) +export class RateLimitNotifier { + private readonly state = signal(IDLE); + private clearTimer: ReturnType | null = null; + + readonly current = this.state.asReadonly(); + readonly isRateLimited = computed(() => this.state().active); + readonly retryAfterSeconds = computed(() => this.state().retryAfterSeconds); + + /** Called by the HTTP interceptor when a 429 is observed. */ + report(retryAfterSeconds: number, url?: string): void { + const seconds = Math.max(0, Math.ceil(retryAfterSeconds)); + + this.state.set({ + active: true, + retryAfterSeconds: seconds, + lastUrl: url, + since: Date.now(), + }); + + // Self-clearing: a banner that outlives the throttle is worse than no + // banner, because it trains users to ignore it. + this.cancelTimer(); + this.clearTimer = setTimeout(() => this.clear(), Math.max(1, seconds) * 1000); + } + + clear(): void { + this.cancelTimer(); + this.state.set(IDLE); + } + + private cancelTimer(): void { + if (this.clearTimer !== null) { + clearTimeout(this.clearTimer); + this.clearTimer = null; + } + } +} diff --git a/src/app/core/interceptors/api-error.interceptor.ts b/src/app/core/interceptors/api-error.interceptor.ts new file mode 100644 index 0000000..d4843ac --- /dev/null +++ b/src/app/core/interceptors/api-error.interceptor.ts @@ -0,0 +1,76 @@ +import { HttpErrorResponse, HttpInterceptorFn } from '@angular/common/http'; +import { inject } from '@angular/core'; +import { throwError, timer } from 'rxjs'; +import { catchError, mergeMap, retry } from 'rxjs/operators'; +import { toApiError } from '../error-handling/api-error.mapper'; +import { ApiError } from '../error-handling/models/api-error.model'; +import { RateLimitNotifier } from '../error-handling/rate-limit-notifier.service'; + +/** + * Normalizes every failed response into an ApiError, and honours 429 + * Retry-After instead of failing straight through. + * + * Contract: BACKEND-API-REFERENCE.md §5 (envelope), TRACK-S §5 (rate limits). + * + * Before this existed, every caller branched on raw HttpErrorResponse.status + * and nothing anywhere handled 429 — a rate-limited backend surfaced as a + * generic failure with no retry and no user-visible explanation. + */ + +/** Retries are bounded: past this, the caller sees the error. */ +const MAX_RATE_LIMIT_RETRIES = 2; + +/** Never sleep longer than this on a server-suggested delay. */ +const MAX_RETRY_DELAY_SECONDS = 30; + +export const apiErrorInterceptor: HttpInterceptorFn = (req, next) => { + const notifier = inject(RateLimitNotifier); + + return next(req).pipe( + retry({ + count: MAX_RATE_LIMIT_RETRIES, + delay: (error: unknown, retryCount: number) => { + if (!(error instanceof HttpErrorResponse) || error.status !== 429) { + return throwError(() => error); + } + + // Only retry requests that are safe to repeat. Replaying a POST after + // a 429 can double-submit; that decision belongs to the caller, which + // knows whether it holds an idempotency key. + if (!isIdempotent(req.method)) { + return throwError(() => error); + } + + const apiError = toApiError(error); + const waitSeconds = Math.min( + apiError.retryAfterSeconds ?? retryCount * 5, + MAX_RETRY_DELAY_SECONDS, + ); + + notifier.report(waitSeconds, req.url); + return timer(waitSeconds * 1000); + }, + }), + catchError((error: unknown) => { + if (!(error instanceof HttpErrorResponse)) { + return throwError(() => error); + } + + const apiError: ApiError = toApiError(error); + + if (apiError.status === 429) { + notifier.report(apiError.retryAfterSeconds ?? 0, req.url); + } + + return throwError(() => apiError); + }), + ); +}; + +function isIdempotent(method: string): boolean { + const m = method.toUpperCase(); + return m === 'GET' || m === 'HEAD' || m === 'OPTIONS'; +} + +/** Exported for tests only. */ +export const __testing = { MAX_RATE_LIMIT_RETRIES, MAX_RETRY_DELAY_SECONDS, isIdempotent };