Skip to content

Add common contract schemas and standardized error handling (#543)#575

Open
abhishek-8081 wants to merge 1 commit into
fireform-core:developmentfrom
abhishek-8081:issue-543-fix
Open

Add common contract schemas and standardized error handling (#543)#575
abhishek-8081 wants to merge 1 commit into
fireform-core:developmentfrom
abhishek-8081:issue-543-fix

Conversation

@abhishek-8081

Copy link
Copy Markdown
Collaborator

Closes #543.

Adds the shared contract schemas and standardizes error handling across the app.
Per the discussion on the issue, this is the Option A approach — the new error format
is applied everywhere, including the existing routes, rather than a v1-only split.

New error envelope (per contracts/schemas/common.yaml):

  • AppError now carries an error_code and optional detail, falling back to a
    status-derived code so existing callers keep working.
  • The handler emits {error_code, message, detail?, retry_after_seconds?}. A new 422
    handler maps FastAPI validation errors to the contract's validation_errors array
    (field / issue / value).
  • retry_after_seconds on 503 is a named advisory constant (30s), commented as a
    client hint — not a measured queue depth or backpressure value.

Common schemas: added ErrorResponse, ValidationErrorItem, Pagination,
AsyncJobResponse, and Job (all using ConfigDict), plus all 11 enums from
enums.yaml as str enums in a new enums.py. The job models use the enums
directly rather than plain strings.

Migration of existing routes:

  • The 4 AppError raises in forms.py now pass explicit codes (TEMPLATE_NOT_FOUND,
    FORM_FILL_ERROR, STT_UNAVAILABLE, TRANSCRIPTION_FAILED). templates.py raises
    no AppError, so it needed no changes.
  • The old {"error": message} shape and the dead SuccessResponse/ErrorDetail/old
    ErrorResponse classes in common.py are removed.
  • The two /schema stubs from Health endpoints #542 already matched the envelope, so their PROVISIONAL
    comments were dropped.

Tests: 36 passing. Updated the one test asserting the old shape, and the 422 test now
verifies the full contract envelope (real validation_errors entries, not just a
status code).

@marcvergees marcvergees left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a couple of my thoughts through my code, could you doublecheck it, so that we can therefore merge it?

Comment thread app/api/schemas/enums.py
report_generation = "report_generation"


class FormType(str, Enum):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, we're gonna keep it, but Idk if it's useful in a close future. I know that it was specified in the .yaml by @chetanr25, but I need to doublecheck with the firefighters if they indeed need a field like that.


# Advisory retry hint on 503 responses — not a measured queue depth or backpressure
# value; just a safe default so clients know when to try again.
_RETRY_AFTER_SECONDS = 30

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we feeling about moving it in the app/core/config.py where we have all the global variables? Feel free to say disagree if you keep thinking that should be here, just lmk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Common Pydantic schemas supporting the OpenAPI contract

2 participants