From e51b7ab33e3c302e7617cd3bbb1cc90d0ff2605b Mon Sep 17 00:00:00 2001 From: Ramon Bartl Date: Wed, 7 Oct 2026 08:30:49 +0200 Subject: [PATCH] Carry an error's status instead of applying it on construction 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. --- docs/changelog.rst | 1 + src/senaite/jsonapi/exceptions.py | 16 ++-- src/senaite/jsonapi/tests/test_exceptions.py | 99 ++++++++++++++++++++ 3 files changed, 110 insertions(+), 6 deletions(-) create mode 100644 src/senaite/jsonapi/tests/test_exceptions.py diff --git a/docs/changelog.rst b/docs/changelog.rst index 3478b8d..1dc6586 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -15,6 +15,7 @@ Changelog applied. Uninstalling from the same panel removes the PAS plugin and the per-user JWT signing secrets. +- #113 Carry an error's status instead of applying it on construction - #109 Add a partition operation endpoint - #106 Fix single-valued UID reference fields not settable through the JSON API - #112 Allow updating the Laboratory through the API diff --git a/src/senaite/jsonapi/exceptions.py b/src/senaite/jsonapi/exceptions.py index 88e62b8..a499530 100644 --- a/src/senaite/jsonapi/exceptions.py +++ b/src/senaite/jsonapi/exceptions.py @@ -30,6 +30,12 @@ Backward compatibility: every typed error inherits `APIError`, so any code that catches `APIError` still catches all of them, and the legacy `api.fail(status, msg)` helper still raises a plain `APIError`. + +The status is carried, not applied. It reaches the response when +plone.jsonapi.core renders the error envelope, which is the moment the +error becomes the answer. Setting it in the constructor instead meant +an error that was caught, and never answered with, still left its +status on whatever the request went on to return. """ from senaite.jsonapi import request as req @@ -39,9 +45,6 @@ class APIError(Exception): """Base class for every JSON API error. Instances carry an HTTP status code and a human-facing message. - Instantiating one sets the response status on the current request - as a side effect (matches the legacy behavior that route code and - the error-envelope decorator both rely on). """ status = 500 @@ -49,13 +52,12 @@ def __init__(self, message, status=None): if status is not None: self.status = status self.message = message - self._set_response_status(self.status) def _set_response_status(self, status): request = req.getRequest() # req.getRequest() may return None outside a real request # context (unit tests, setup handlers). Skip silently in that - # case so raising an APIError never crashes ancillary code. + # case so setting a status never crashes ancillary code. if request is None: return response = getattr(request, "response", None) @@ -64,8 +66,10 @@ def _set_response_status(self, status): response.setStatus(status) # Legacy alias, retained so existing callers of `err.setStatus(x)` - # keep working. + # keep working. Nothing in this package calls it: the status of an + # error reaches the response when the envelope is rendered. def setStatus(self, status): + self.status = status self._set_response_status(status) def __str__(self): diff --git a/src/senaite/jsonapi/tests/test_exceptions.py b/src/senaite/jsonapi/tests/test_exceptions.py new file mode 100644 index 0000000..5f4d747 --- /dev/null +++ b/src/senaite/jsonapi/tests/test_exceptions.py @@ -0,0 +1,99 @@ +# -*- coding: utf-8 -*- +# +# This file is part of SENAITE.JSONAPI. +# +# SENAITE.JSONAPI is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by the +# Free Software Foundation, version 2. +# +# This program is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General +# Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# +# Copyright 2017-2026 by it's authors. +# Some rights reserved, see README and LICENSE. + +"""The status a typed error carries, and when it is applied.""" + +import unittest + +from senaite.jsonapi import exceptions + + +class FakeResponse(object): + def __init__(self): + self.status = None + + def setStatus(self, status): + self.status = status + + +class FakeRequest(object): + def __init__(self): + self.response = FakeResponse() + + +class APIErrorStatusTestCase(unittest.TestCase): + """An error carries its status; it does not apply it. + + The status reaches the response when plone.jsonapi.core renders the + error envelope, which is the moment the error becomes the answer. + Applied in the constructor instead, an error that was caught, and + never answered with, left its status on whatever the request went + on to return: a route that catches a ForbiddenError and then + succeeds answered its complete result under a 403. + """ + + def setUp(self): + self.request = FakeRequest() + self._get_request = exceptions.req.getRequest + exceptions.req.getRequest = lambda: self.request + + def tearDown(self): + exceptions.req.getRequest = self._get_request + + def test_each_error_carries_its_own_status(self): + self.assertEqual(exceptions.BadRequestError("x").status, 400) + self.assertEqual(exceptions.UnauthorizedError("x").status, 401) + self.assertEqual(exceptions.ForbiddenError("x").status, 403) + self.assertEqual(exceptions.NotFoundError("x").status, 404) + self.assertEqual(exceptions.APIError("x").status, 500) + + def test_an_explicit_status_wins(self): + self.assertEqual(exceptions.APIError("x", status=418).status, 418) + + def test_constructing_one_leaves_the_response_alone(self): + exceptions.ForbiddenError("not for you") + self.assertIsNone(self.request.response.status) + + def test_the_message_is_what_str_gives(self): + self.assertEqual(str(exceptions.NotFoundError("gone")), "gone") + + # The legacy alias is the one way to apply a status by hand, kept + # for callers outside this package that still do. + def test_set_status_applies_and_records_it(self): + error = exceptions.APIError("x") + error.setStatus(409) + self.assertEqual(error.status, 409) + self.assertEqual(self.request.response.status, 409) + + def test_set_status_outside_a_request_is_harmless(self): + exceptions.req.getRequest = lambda: None + exceptions.APIError("x").setStatus(409) + + def test_every_typed_error_is_an_api_error(self): + for name in ("BadRequestError", "UnauthorizedError", + "ForbiddenError", "NotFoundError"): + self.assertTrue( + issubclass(getattr(exceptions, name), exceptions.APIError)) + + +def test_suite(): + suite = unittest.TestSuite() + suite.addTest(unittest.makeSuite(APIErrorStatusTestCase)) + return suite