diff --git a/api/controllers/openapi/_errors.py b/api/controllers/openapi/_errors.py index 84b76091c3..c910b17623 100644 --- a/api/controllers/openapi/_errors.py +++ b/api/controllers/openapi/_errors.py @@ -14,8 +14,13 @@ mandated by the OAuth spec):: error-handler path funnels through one builder, and it also rewrites ``e.data`` because flask-restx ``Api.handle_error`` lets a pre-existing ``e.data`` override the registered handler's return value. + +The transport-generic enum members, ``_CODE_BY_STATUS`` and the +``OpenApiError``/``OpenApiErrorFormatter`` bases are openapi-only today; +promote them to ``libs`` if a second surface adopts ``ErrorBody``. """ +import logging from enum import StrEnum from typing import Any @@ -96,6 +101,8 @@ _CODE_BY_STATUS: dict[int, OpenApiErrorCode] = { _GENERIC_500_MESSAGE = "Internal Server Error" +logger = logging.getLogger(__name__) + class OpenApiError(HTTPException): """Dedicated throwable for the /openapi/v1 surface. @@ -136,14 +143,24 @@ class OpenApiErrorFormatter: exc_data = getattr(e, "data", None) merged: dict[str, Any] = {**data, **exc_data} if isinstance(exc_data, dict) else dict(data) - body = ErrorBody( - code=self._resolve_code(e, status_code), - message=self._resolve_message(merged, status_code), - status=status_code, - hint=self._resolve_hint(e), - details=self._extract_details(e, merged), - ) - wire = body.model_dump(mode="json", exclude_none=True) + # finalize runs inside the framework error handler: raising here would + # replace the response with an unformatted 500, so fall back instead + try: + body = ErrorBody( + code=self._resolve_code(e, status_code), + message=self._resolve_message(merged, status_code), + status=status_code, + hint=self._resolve_hint(e), + details=self._extract_details(e, merged), + ) + wire = body.model_dump(mode="json", exclude_none=True) + except Exception: + logger.exception("error-body build failed; emitting fallback body") + wire = { + "code": str(_CODE_BY_STATUS.get(status_code, OpenApiErrorCode.UNKNOWN)), + "message": http_status_message(status_code) or "request failed", + "status": status_code, + } # flask-restx Api.handle_error does `data = getattr(e, "data", default_data)` # AFTER our handler returns, so a pre-existing e.data (flask_restx.abort, diff --git a/api/libs/external_api.py b/api/libs/external_api.py index 077edad6b7..fc5286bf80 100644 --- a/api/libs/external_api.py +++ b/api/libs/external_api.py @@ -32,7 +32,9 @@ def register_external_error_handlers(api: Api, body_formatter: ErrorBodyFormatte def handle_http_exception(e: HTTPException): got_request_exception.send(current_app, exception=e) - # If Werkzeug already prepared a Response, just use it. + # If Werkzeug already prepared a Response, just use it. This bypasses + # body_formatter entirely — surfaces with a formatter must not raise + # exceptions carrying a pre-built response. if e.response is not None: return e.response 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 e3ae80ca6d..7da539d264 100644 --- a/api/tests/unit_tests/controllers/openapi/test_error_contract.py +++ b/api/tests/unit_tests/controllers/openapi/test_error_contract.py @@ -126,6 +126,17 @@ class TestOpenApiErrorFormatter: assert second == first + def test_malformed_canonical_details_falls_back_instead_of_raising(self, fmt): + # finalize runs inside the framework error handler; a ValidationError + # escaping it would replace the response with an unformatted 500 + e = UnprocessableEntity() + e.data = {"message": "broken", "details": [{"bad": "shape"}]} + data = {"code": "unprocessable_entity", "message": "broken", "status": 422} + + wire = fmt.finalize(e, data, 422) + + assert wire == {"code": "invalid_param", "message": "Unprocessable Entity", "status": 422} + def test_base_http_exception_error_code_wins_over_status_map(self, fmt): e = ProviderQuotaExceededError() data = dict(e.data) diff --git a/cli/src/errors/format.test.ts b/cli/src/errors/format.test.ts index bebbfb0c73..33591a6da3 100644 --- a/cli/src/errors/format.test.ts +++ b/cli/src/errors/format.test.ts @@ -67,6 +67,25 @@ describe('formatErrorForCli — human', () => { expect(formatErrorForCli(noServerHint, { isErrTTY: false })).toContain('run difyctl auth login') }) + 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 }) + + expect(out).toContain('- body required (invalid)') + expect(out).not.toContain('- : body required') + }) + it('renders request and http_status lines', () => { const err = new HttpClientError({ code: ErrorCode.Server5xx, diff --git a/cli/src/errors/format.ts b/cli/src/errors/format.ts index 63a0a0b28b..281229cb6d 100644 --- a/cli/src/errors/format.ts +++ b/cli/src/errors/format.ts @@ -50,8 +50,10 @@ function renderHuman(env: ErrorEnvelope, isErrTTY: boolean): string { const server = e.server const headerCode = server?.code ?? e.code const lines: string[] = [`${headerCode}: ${e.message}`] - for (const d of server?.details ?? []) - lines.push(` - ${(d.loc ?? []).join('.')}: ${d.msg} (${d.type})`) + for (const d of server?.details ?? []) { + const loc = (d.loc ?? []).join('.') + lines.push(` - ${loc ? `${loc}: ` : ''}${d.msg} (${d.type})`) + } const hint = server?.hint ?? e.hint if (hint !== undefined && hint !== null) lines.push(`${cs.magenta('hint:')} ${cs.cyan(hint)}`)