From 5b75aae20d8fca5910cb2465caa86215f944e41f Mon Sep 17 00:00:00 2001 From: GareArc Date: Wed, 10 Jun 2026 19:06:59 -0700 Subject: [PATCH] test: dedupe error-path test helpers in cli and api --- .../openapi/test_error_contract.py | 16 ++-- cli/src/errors/format.test.ts | 58 ++++++--------- cli/src/http/error-mapper.test.ts | 32 ++++---- cli/src/http/orpc.test.ts | 74 ++++++++----------- 4 files changed, 75 insertions(+), 105 deletions(-) diff --git a/api/tests/unit_tests/controllers/openapi/test_error_contract.py b/api/tests/unit_tests/controllers/openapi/test_error_contract.py index 0d5c223ef9..193c894a2d 100644 --- a/api/tests/unit_tests/controllers/openapi/test_error_contract.py +++ b/api/tests/unit_tests/controllers/openapi/test_error_contract.py @@ -42,6 +42,11 @@ from controllers.service_api.app.error import ( from controllers.web.error import InvokeRateLimitError as InvokeRateLimitHttpError +@pytest.fixture +def fmt() -> OpenApiErrorFormatter: + return OpenApiErrorFormatter() + + class TestErrorBodyModel: def test_minimal_body_serializes_without_optional_fields(self): body = ErrorBody(code=OpenApiErrorCode.NOT_FOUND, message="app not found", status=404) @@ -73,10 +78,6 @@ class TestErrorBodyModel: class TestOpenApiErrorFormatter: - @pytest.fixture - def fmt(self): - return OpenApiErrorFormatter() - def test_plain_werkzeug_exception_maps_code_from_status(self, fmt): e = NotFound("app not found") data = {"code": "not_found", "message": "app not found", "status": 404} @@ -222,10 +223,6 @@ class TestOpenApiErrorFormatter: class TestQuotaExceptions: - @pytest.fixture - def fmt(self): - return OpenApiErrorFormatter() - @pytest.mark.parametrize("exc_class", [MemberLimitExceeded, MemberLicenseExceeded]) def test_quota_exception_carries_declared_code_and_message(self, fmt, exc_class): # Single source: assertions read the class attributes, no re-typed strings. @@ -329,8 +326,7 @@ class TestErrorMatrix: ERROR_MATRIX, ids=lambda v: type(v).__name__ if isinstance(v, Exception) else str(v), ) - def test_every_known_error_path_yields_canonical_code(self, exc, status, expected_code): - fmt = OpenApiErrorFormatter() + def test_every_known_error_path_yields_canonical_code(self, fmt, exc, status, expected_code): data = dict(getattr(exc, "data", None) or {"message": str(exc), "status": status}) wire = fmt.finalize(exc, data, status) diff --git a/cli/src/errors/format.test.ts b/cli/src/errors/format.test.ts index 33591a6da3..89ccf6d1ff 100644 --- a/cli/src/errors/format.test.ts +++ b/cli/src/errors/format.test.ts @@ -1,29 +1,41 @@ +import type { ErrorBody } from '@dify/contracts/api/openapi/types.gen' import { describe, expect, it } from 'vitest' + import { HttpClientError } from './base' import { ErrorCode } from './codes' import { formatErrorForCli } from './format' -function validationError(): HttpClientError { +type ValidationErrorOverrides = { + readonly cliHint?: string + readonly serverHint?: string + readonly details?: ErrorBody['details'] +} + +function validationError(overrides: ValidationErrorOverrides = {}): HttpClientError { + const details + = overrides.details + ?? [ + { type: 'int_parsing', loc: ['page'], msg: 'must be >= 1' }, + { type: 'missing', loc: ['inputs', 'query'], msg: 'field required' }, + ] return new HttpClientError({ code: ErrorCode.Server4xxOther, message: 'Request validation failed', httpStatus: 422, + hint: overrides.cliHint, serverError: { code: 'invalid_param', message: 'Request validation failed', status: 422, - hint: 'check the page parameter', - details: [ - { type: 'int_parsing', loc: ['page'], msg: 'must be >= 1' }, - { type: 'missing', loc: ['inputs', 'query'], msg: 'field required' }, - ], + hint: overrides.serverHint, + details, }, }) } describe('formatErrorForCli — human', () => { it('prints server code, message, and details without verbose', () => { - const out = formatErrorForCli(validationError(), { isErrTTY: false }) + const out = formatErrorForCli(validationError({ serverHint: 'check the page parameter' }), { isErrTTY: false }) expect(out).toContain('invalid_param: Request validation failed') expect(out).toContain('- page: must be >= 1 (int_parsing)') @@ -41,20 +53,7 @@ describe('formatErrorForCli — human', () => { }) it('server hint wins over cli hint; cli hint fills when server sent none', () => { - // validationError has server hint "check the page parameter"; cli hint is ignored - const withCliHint = new HttpClientError({ - code: ErrorCode.Server4xxOther, - message: 'Request validation failed', - httpStatus: 422, - hint: 'cli fallback hint', - serverError: { - code: 'invalid_param', - message: 'Request validation failed', - status: 422, - hint: 'check the page parameter', - details: [], - }, - }) + const withCliHint = validationError({ cliHint: 'cli fallback hint', serverHint: 'check the page parameter', details: [] }) expect(formatErrorForCli(withCliHint, { isErrTTY: false })).toContain('check the page parameter') expect(formatErrorForCli(withCliHint, { isErrTTY: false })).not.toContain('cli fallback hint') @@ -68,19 +67,10 @@ describe('formatErrorForCli — human', () => { }) it('omits the loc prefix when a detail has no loc', () => { - const err = new HttpClientError({ - code: ErrorCode.Server4xxOther, - message: 'Request validation failed', - httpStatus: 422, - serverError: { - code: 'invalid_param', - message: 'Request validation failed', - status: 422, - details: [{ type: 'invalid', loc: [], msg: 'body required' }], - }, - }) - - const out = formatErrorForCli(err, { isErrTTY: false }) + const out = formatErrorForCli( + validationError({ details: [{ type: 'invalid', loc: [], msg: 'body required' }] }), + { isErrTTY: false }, + ) expect(out).toContain('- body required (invalid)') expect(out).not.toContain('- : body required') diff --git a/cli/src/http/error-mapper.test.ts b/cli/src/http/error-mapper.test.ts index 33ec34dd2c..ab0397cce7 100644 --- a/cli/src/http/error-mapper.test.ts +++ b/cli/src/http/error-mapper.test.ts @@ -1,5 +1,5 @@ +import type { HttpClientError } from '@/errors/base' import { describe, expect, it } from 'vitest' -import { isHttpClientError } from '@/errors/base' import { ErrorCode } from '@/errors/codes' import { classifyResponse } from './error-mapper' @@ -12,6 +12,10 @@ function res(status: number, body: unknown): Response { const req = new Request('https://dify.test/openapi/v1/apps') +function classified(status: number, body: unknown): Promise { + return classifyResponse(req, res(status, body)) +} + describe('classifyResponse — canonical ErrorBody', () => { it('attaches the parsed body whole as serverError', async () => { const body = { @@ -22,54 +26,48 @@ describe('classifyResponse — canonical ErrorBody', () => { details: [{ type: 'int_parsing', loc: ['page'], msg: 'must be >= 1' }], } - const err = await classifyResponse(req, res(422, body)) + const err = await classified(422, body) - expect(isHttpClientError(err)).toBe(true) - if (!isHttpClientError(err)) - return expect(err.serverError).toEqual(body) expect(err.message).toBe('Request validation failed') expect(err.code).toBe(ErrorCode.Server4xxOther) }) it('401 classifies by status as AuthExpired with CLI login hint', async () => { - const err = await classifyResponse(req, res(401, { + const err = await classified(401, { code: 'unauthorized', message: 'session expired or revoked', status: 401, - })) + }) expect(err.code).toBe(ErrorCode.AuthExpired) expect(err.hint).toBe('run \'difyctl auth login\' to sign in again') }) it('unknown future server code is data, not behavior — status bucket decides', async () => { - const err = await classifyResponse(req, res(409, { + const err = await classified(409, { code: 'some_future_code', message: 'nope', status: 409, - })) + }) expect(err.code).toBe(ErrorCode.Server4xxOther) - if (isHttpClientError(err)) - expect(err.serverError?.code).toBe('some_future_code') + expect(err.serverError?.code).toBe('some_future_code') }) }) describe('classifyResponse — non-conforming bodies (no fallback by design)', () => { it('non-JSON body yields no serverError, classification by status', async () => { - const err = await classifyResponse(req, res(502, 'bad gateway')) + const err = await classified(502, 'bad gateway') expect(err.code).toBe(ErrorCode.Server5xx) - if (isHttpClientError(err)) - expect(err.serverError).toBeUndefined() + expect(err.serverError).toBeUndefined() }) it('RFC 8628 string error field yields no serverError and a generic message', async () => { - const err = await classifyResponse(req, res(400, { error: 'slow_down' })) + const err = await classified(400, { error: 'slow_down' }) expect(err.message).toBe('request failed (HTTP 400)') - if (isHttpClientError(err)) - expect(err.serverError).toBeUndefined() + expect(err.serverError).toBeUndefined() }) }) diff --git a/cli/src/http/orpc.test.ts b/cli/src/http/orpc.test.ts index 85023792ee..c99232b975 100644 --- a/cli/src/http/orpc.test.ts +++ b/cli/src/http/orpc.test.ts @@ -1,4 +1,5 @@ import type { StubServer } from '@test/fixtures/stub-server' +import type { HttpClientError } from '@/errors/base' import { jsonResponder, startStubServer } from '@test/fixtures/stub-server' import { afterEach, describe, expect, it } from 'vitest' import { isHttpClientError } from '@/errors/base' @@ -33,64 +34,49 @@ describe('createOpenApiClient error mapping', () => { await stub?.stop() }) - it('recovers Dify message from a canonical ErrorBody 4xx response', async () => { - stub = await startStubServer(cap => jsonResponder(403, { code: 'access_denied', message: 'no access', status: 403 }, cap)) + async function classifiedError(status: number, body: unknown): Promise { + stub = await startStubServer(cap => jsonResponder(status, body, cap)) const orpc = orpcClient(stub.url) - const caught = await catchErr(() => orpc.account.get()) + if (!isHttpClientError(caught)) + throw new Error(`expected HttpClientError, got: ${String(caught)}`) + return caught + } - expect(isHttpClientError(caught)).toBe(true) - if (isHttpClientError(caught)) { - expect(caught.code).toBe(ErrorCode.Server4xxOther) - expect(caught.httpStatus).toBe(403) - expect(caught.message).toBe('no access') - // Parity with the transport path: the migrated endpoint's error keeps the request - // method/url and the raw body, so formatted errors still print the `request:` line - // and the raw-response dump (not just message/hint). - expect(caught.method).toBe('GET') - expect(caught.url).toContain('/account') - expect(caught.rawResponse).toContain('no access') - } + it('recovers Dify message from a canonical ErrorBody 4xx response', async () => { + const caught = await classifiedError(403, { code: 'access_denied', message: 'no access', status: 403 }) + + expect(caught.code).toBe(ErrorCode.Server4xxOther) + expect(caught.httpStatus).toBe(403) + expect(caught.message).toBe('no access') + // Parity with the transport path: the migrated endpoint's error keeps the request + // method/url and the raw body, so formatted errors still print the `request:` line + // and the raw-response dump (not just message/hint). + expect(caught.method).toBe('GET') + expect(caught.url).toContain('/account') + expect(caught.rawResponse).toContain('no access') }) it('reads server message from canonical ErrorBody on 401 and keeps the auth code', async () => { - stub = await startStubServer(cap => jsonResponder(401, { code: 'unauthorized', message: 'expired', status: 401 }, cap)) - const orpc = orpcClient(stub.url) + const caught = await classifiedError(401, { code: 'unauthorized', message: 'expired', status: 401 }) - const caught = await catchErr(() => orpc.account.get()) - - expect(isHttpClientError(caught)).toBe(true) - if (isHttpClientError(caught)) { - expect(caught.code).toBe(ErrorCode.AuthExpired) - expect(caught.httpStatus).toBe(401) - expect(caught.message).toBe('expired') - } + expect(caught.code).toBe(ErrorCode.AuthExpired) + expect(caught.httpStatus).toBe(401) + expect(caught.message).toBe('expired') }) it('uses CLI default auth-login hint for non-conforming 401 body', async () => { - stub = await startStubServer(cap => jsonResponder(401, { error: 'expired' }, cap)) - const orpc = orpcClient(stub.url) + const caught = await classifiedError(401, { error: 'expired' }) - const caught = await catchErr(() => orpc.account.get()) - - expect(isHttpClientError(caught)).toBe(true) - if (isHttpClientError(caught)) { - expect(caught.code).toBe(ErrorCode.AuthExpired) - expect(caught.hint).toContain('difyctl auth login') - } + expect(caught.code).toBe(ErrorCode.AuthExpired) + expect(caught.hint).toContain('difyctl auth login') }) it('maps 5xx to Server5xx with message from canonical ErrorBody', async () => { - stub = await startStubServer(cap => jsonResponder(503, { code: 'service_unavailable', message: 'down for maintenance', status: 503 }, cap)) - const orpc = orpcClient(stub.url) + const caught = await classifiedError(503, { code: 'service_unavailable', message: 'down for maintenance', status: 503 }) - const caught = await catchErr(() => orpc.account.get()) - - expect(isHttpClientError(caught)).toBe(true) - if (isHttpClientError(caught)) { - expect(caught.code).toBe(ErrorCode.Server5xx) - expect(caught.httpStatus).toBe(503) - expect(caught.message).toBe('down for maintenance') - } + expect(caught.code).toBe(ErrorCode.Server5xx) + expect(caught.httpStatus).toBe(503) + expect(caught.message).toBe('down for maintenance') }) })