Skip to content

Carry an error's status instead of applying it on construction (blocked on senaite.core#3038) - #113

Open
ramonski wants to merge 1 commit into
2.xfrom
fix/status-when-rendered
Open

ramonski wants to merge 1 commit into
2.xfrom
fix/status-when-rendered

Conversation

@ramonski

@ramonski ramonski commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Blocked. The CI on this PR is red on purpose and stays red until senaite/senaite.core#3038 is in 2.x. See Why the CI is red below. Do not merge this one first.

APIError.__init__ sets the response status as a side effect of constructing the error. The status of an error belongs to the error reaching the client, not to the moment the object was made, and the two are not the same: an error that is caught is never answered with.

Current behavior before PR

A route that catches a ForbiddenError in order to report something, and then succeeds, answers its complete result under a 403:

403 {"count": 45, "report": {"skipped": [...], "updated": [...], "errors": []}}

That is senaite.composer reporting a field an object will not take any more, which is worth reporting and not worth failing on. Any caller that catches a typed error hits this.

Desired behavior after PR is merged

The error carries its status, and bika.lims.jsonapi.handle_errors sets it when it renders the envelope, which is the moment the error becomes the answer. Nothing else changes: the same status reaches the same responses for every error that is actually raised to the client.

setStatus() stays for callers outside this package that apply one by hand, and now records the status on the error as well, so the two cannot disagree.

Why the CI is red

Against senaite.core 2.x as it is today, the error handler never sets a status. The constructor side effect this PR removes is what stood in for it, so with it gone and nothing yet put in its place, every error answers 200.

The doctests say it plainly:

File ".../typed_exceptions.rst", line 108, in typed_exceptions.rst
Failed example:
    anon.open("{}/registry".format(api_url))
Differences (ndiff with -expected +actual):
    - Traceback (most recent call last):
    - HTTPError: HTTP Error 401: Unauthorized

Expected a 401, got a 200 with a failure envelope. That is the bug this pair of changes is about, seen from the other side.

senaite/senaite.core#3038 teaches the handler to set the status from the exception. With that in, these doctests pass unchanged.

Verification

  • Locally, against a senaite.core whose handler sets the status: bin/test-senaite -s senaite.jsonapi is green, 38 tests including every doctest above.
  • 7 of those are new and cover the exception classes, which had none: what each error carries, that constructing one leaves the response alone, that an explicit status wins, and that setStatus still applies and now records.
  • The 403-on-success above is from a live 2.7 site.
  • flake8 clean.

Constructing a typed error set the response status there and then. The
status of an error belongs to the error reaching the client, not to the
moment the object was made, and the two are not the same: an error that
is caught is never answered with.

A route that catches a ForbiddenError in order to report something, and
then succeeds, answered its complete result under a 403. The composer
does exactly that to report a field an object will not take any more.

plone.jsonapi.core sets the status from the exception when it renders
the envelope, which is the moment the error becomes the answer. The
legacy setStatus alias still applies one by hand, and now records it on
the error too, so the two cannot disagree.
@ramonski ramonski changed the title Carry an error's status instead of applying it on construction Carry an error's status instead of applying it on construction (blocked on senaite.core#3037) Oct 5, 2026
@ramonski ramonski changed the title Carry an error's status instead of applying it on construction (blocked on senaite.core#3037) Carry an error's status instead of applying it on construction (blocked on senaite.core#3038) Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant