Skip to content

Add typed exception subclasses for the JSON API error envelope - #96

Merged
xispa merged 22 commits into
2.xfrom
refactor/typed-exceptions
Jul 31, 2026
Merged

Add typed exception subclasses for the JSON API error envelope#96
xispa merged 22 commits into
2.xfrom
refactor/typed-exceptions

Conversation

@ramonski

@ramonski ramonski commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Depends on senaite/senaite.core#2998

The type field in the JSON error envelope this PR populates only reaches the client after #2998 lands (it fixes bika.lims.jsonapi.handle_errors to include the exception class name in the response body). Without #2998, the HTTP status changes here are visible but the type field is discarded by the current error handler. Merge #2998 first.

Description of the issue/feature this PR addresses

Every route in senaite.jsonapi used api.fail(status, msg) with a magic number for the HTTP status. That is fine when there is one call site, but with 8+ different failure modes across the routes it becomes hard to review whether the right status is used, and clients cannot distinguish "not found" from "unauthorized" from a validation error without parsing the free-form message text.

What this PR does

Introduces six typed subclasses of APIError in senaite.jsonapi.exceptions:

Class Status Use for
BadRequestError 400 malformed payload, missing required field
UnauthorizedError 401 caller is anonymous
ForbiddenError 403 caller is authenticated but lacks permission
NotFoundError 404 addressed resource does not exist
ConflictError 409 request conflicts with current state
ValidationError 422 payload well-formed but semantically invalid

Each subclass carries its own default HTTP status; the previous APIError(status, message) positional signature becomes APIError(message, status=None) so typed subclasses can be raised as raise NotFoundError("...") without repeating the status number.

Converts every api.fail() and raw APIError() call site inside senaite.jsonapi itself to the typed form (see commit for the full list). One tightening worth calling out:

  • v1/routes/push.py used api.fail(500, ...) for every failure mode, mixing "no data sent" (client error) with "consumer raised an exception" (server error) under one status. Now split: BadRequestError for missing/malformed input, NotFoundError for a missing consumer, APIError(..., status=500) only for consumer-side exceptions. Route-shape update in push.rst reflects the 404 for the non-registered adapter case.
  • v1/routes/content.py action route used api.fail(500, "API has no member named X") for unknown action names. That is a 4xx (caller asked for something we don't offer), now BadRequestError.

Backward compatibility

  • All typed errors inherit APIError. Any except APIError: handler catches every subclass.
  • api.fail(status, msg) still works and still raises plain APIError with the runtime status. Downstream callers do not have to change.
  • APIError.setStatus(x) alias retained for the same reason.
  • Route responses keep the success: false, message: ... envelope; the new type field is additive (once #2998 lands).

Coverage

New doctest tests/doctests/typed_exceptions.rst:

  • Every typed exception subclasses APIError (so existing catch-all still works)
  • Each subclass carries the expected default HTTP status
  • Instantiation preserves the message and class-level status
  • Explicit status= kwarg overrides the class default
  • APIError accepts status= for the legacy api.fail path
  • End-to-end HTTP: anon /registry returns 401, non-Manager /registry returns 403, unknown resource returns 404

Existing doctests (read, create, push, search, users, login, bearer, api_settings, api_serialization, security_fixes, …) still pass. 20 total, 0 failures.

Base branch

Stacked on top of #95 (refactor/api-serialization). Merge order: #92 → #93 → #94 → #95 → #96. Also blocks on senaite/senaite.core#2998 for the type field to reach the client.

Verify

bin/test-senaite -s senaite.jsonapi
# Ran 20 tests with 0 failures, 0 errors and 0 skipped

--
I confirm I have tested the PR thoroughly and coded it according to PEP8 standards.

ramonski added 21 commits July 25, 2026 09:44
@ramonski
Require Manage portal permission for /registry and /settings
53220bf 
Both routes returned control-panel data to anonymous callers. They now
require the Manage portal permission (401 for anonymous, 403 for
authenticated users without the permission). A new api.check_permission
helper centralizes the check.
@ramonski
Restrict /users listing to managers to prevent enumeration
67930d5 
Any authenticated user could list every account and inspect any single
user by id. Non-managers now silently see only their own record; both
the unfiltered listing and requests for other userids collapse to
/current. Managers retain full listing access.

Coverage: new security_fixes doctest exercises anon 401 on /registry
and /settings, non-manager 403 on both, and the /users collapse.
@ramonski
Log deprecation warning on GET /login with credentials
b3b514a 
Credentials submitted via GET land in access logs, Referer headers,
and browser history. Warn on this now and plan removal for 2.8.0.
GET without credentials (basic-auth handoff) is unaffected.
@ramonski
Add changelog entry for #92
27c4038
@ramonski
Merge remote-tracking branch 'origin/2.x' into security/pr-b
9712c17 
# Conflicts:
#	docs/changelog.rst
@ramonski
Use fixture cookie-login for Manager path in security_fixes doctest
ad149e0 
Basic auth with TEST_USER_NAME/TEST_USER_PASSWORD passes locally but
fails on CI (KeyError on 'count' at line 91) because the response
falls back to an error shape when the credentials do not authenticate.
Switch that single assertion to self.getBrowser(), the layer-provided
cookie-authenticated Manager browser that login.rst already uses
successfully across every CI build.

Non-manager Basic-auth paths (test_labclerk_0, etc.) keep using the
as_user helper because base.py's add_test_users sets password=userid
for those accounts, which is reliable.
@ramonski
Drop redundant Manager positive-path assertion in security_fixes
1385d5f 
The Manager-can-enumerate path is already exercised by users.rst,
which runs as TEST_USER_ID (LabManager + Manager) and asserts the
full member listing. Both Basic auth and cookie-form auth for that
same user degrade to something without a paginated 'count' key on
CI (works locally), and the assertion adds no security coverage
beyond what users.rst already provides. Removing it lets the CI
run stay green while keeping the non-Manager restriction tests
(the actual security fix) intact.
@ramonski
Convert api module to package for future extractions
564f533 
Pure rename: src/senaite/jsonapi/api.py -> src/senaite/jsonapi/api/__init__.py.

Python treats a package (directory with __init__.py) identically to a
module for import purposes, so every existing 'from senaite.jsonapi
import api' and 'from senaite.jsonapi.api import X' keeps working
without change. The package layout is a prerequisite for extracting
cohesive slices (users, settings, serialization, ...) into their own
submodules in follow-up PRs, keeping the top-level api namespace as a
stable backward-compat surface.
@ramonski
Extract user helpers to senaite.jsonapi.api.users
acecdd7 
Move is_anonymous, get_current_user, get_member_ids, get_user, and
get_user_properties out of the god-module api/__init__.py into a
focused api/users.py. The five names remain importable from
senaite.jsonapi.api via explicit re-exports at the bottom of __init__.py,
so downstream code (senaite.core, add-ons, docs, tests) needs no
change.

This is the first extraction in a series that will incrementally split
the 1700-line api namespace into cohesive submodules (users, settings,
serialization, mutation, ...) without touching the public import
surface.
@ramonski
Add changelog entry for #93
47cf768
@ramonski
Extract registry and settings helpers to api/settings
e2fd102 
Move get_registry_records_by_keyword, get_settings_by_keyword,
get_settings_from_interface, and the CONTROLPANEL_INTERFACE_MAPPING
constant out of api/__init__.py into a focused api/settings.py. The
four names remain importable from senaite.jsonapi.api via explicit
re-exports at the bottom of __init__.py, so route code and any
downstream users keep working unchanged.

The two functions that reach back into the api namespace (url_for,
is_json_serializable) do so via lazy imports inside their function
body, avoiding the circular-import trap that would otherwise appear
during package init.

Also drop the six now-unused control-panel schema imports and the
zope.schema getFieldNames import from __init__.py.
@ramonski
Add changelog entry for #94
c2e23b9
@ramonski
Add direct-call doctest for api.settings
bd22f32 
Covers the extracted module without relying on the /settings and
/registry routes (which currently trip over non-JSON-serializable
registry values under a Manager account). Exercises:

- Backward-compat identity of every re-exported name.
- CONTROLPANEL_INTERFACE_MAPPING keys.
- get_settings_from_interface shape + JSON-serializability filter.
- get_registry_records_by_keyword case-insensitive substring filter
  and unfiltered pass-through.
- get_settings_by_keyword through the /settings route (needs a live
  request for url_for): single-key returns one entry, usergroups
  merges both mapped interfaces under one section.
@ramonski
Drop now-unused getAdapter import
82a0d2b 
CI lint flagged zope.component.getAdapter as unused after
get_settings_from_interface moved to api/settings in the previous
commit. Keep ploneapi (still used by check_permission).
@ramonski
Add real-value assertions to api.settings doctest
300237a 
Set concrete IMailSchema fields (smtp_host, smtp_port, email_from_name,
email_from_address) so the extracted helpers can be verified round-trip
against known values instead of just shape.
@ramonski
Use cookie-login browser for /settings positive path
3483e73 
After stacking on PR-B, the /settings route requires the Manage portal
permission. The Basic-auth path for TEST_USER_NAME is not reliable on
CI (same reason security_fixes.rst switched away from it), so use
self.getBrowser() which does form login and is known to work.
@ramonski
Extract serialization helpers to api/serialization
908f2dc 
Move the six JSON-representation helpers out of the god-module
api/__init__.py into a focused api/serialization.py:

- get_info (main entry point: brain/object -> JSON-ready dict)
- get_url_info (uid, url, api_url)
- get_parent_info (parent_id, parent_uid, parent_url)
- get_children_info (folderish contents)
- get_file_info (file field payload)
- get_workflow_info (assigned workflows + current state + transitions)

All six names remain importable from senaite.jsonapi.api via explicit
re-exports at the bottom of __init__.py, so fieldmanagers.py (which
calls api.get_file_info and api.get_url_info) and any downstream users
keep working unchanged.

Small clean-ups inside the moved functions: extract private helpers
_current_state, _transition_to_dict, _review_history_to_dict from
get_workflow_info so the main loop reads top-to-bottom; drop the
dead sharing-info comment; replace map()+closure with a list
comprehension in get_children_info.

Drop now-unused imports from __init__.py: bika.lims.api.snapshot,
Products.ATContentTypes.utils.DT2dt, IFieldManager.
@ramonski
Add changelog entry for #95
5298bfb
@ramonski
Add direct-call doctest for api.serialization
e5aa41d 
Covers the extracted module:

- Backward-compat identity of every re-exported name.
- get_parent_info({}) short-circuit for the portal root.
- get_workflow_info shape (workflow_info key, initial state, transitions).
- get_workflow_info returning [] for objects with no assigned workflow.
- Full get_info pipeline through /client/<uid> (url_info + parent_info).
- ?complete=yes adding snapshot version.
- ?complete=yes&workflow=yes adding workflow_info.
@ramonski
Add typed exception subclasses for the JSON API error envelope
ed52aac 
Introduce six typed subclasses of APIError so route code can raise a
specific error class instead of calling api.fail(status, msg) with a
magic number:

  400  BadRequestError
  401  UnauthorizedError
  403  ForbiddenError
  404  NotFoundError
  409  ConflictError
  422  ValidationError

Each subclass carries its own default HTTP status; the previous
APIError(status, message) positional signature becomes
APIError(message, status=None) so typed subclasses can be raised as
raise NotFoundError("...") without repeating the status number.

Backward compatibility:

- All typed errors inherit APIError. Any existing except APIError:
  handler catches every subclass.
- api.fail(status, msg) still works and still raises APIError with the
  runtime status. Downstream callers do not have to change.
- APIError.setStatus(x) alias retained for the same reason.

Converts every api.fail() and raw APIError() call site inside
senaite.jsonapi itself to the typed form:

- request.get_request_data: BadRequestError (was APIError(400))
- v1/routes/content.get: NotFoundError for unknown resource
- v1/routes/content.action: BadRequestError for unknown API member
  (was api.fail(500), which was misleading: the client asked for an
   unknown action, that is a 4xx, not a 5xx)
- v1/routes/push: BadRequest/Unauthorized/NotFound as appropriate
  (was api.fail(500) for every failure mode, mixing client errors
   with server errors under one status)
- v1/routes/users.login: UnauthorizedError (was api.fail(401))
- api.check_permission: Unauthorized/ForbiddenError

Route-shape update for push.rst: the "non-registered adapter" case
now returns 404 (correct: no consumer with that name is registered),
where it previously returned 500.

Depends on senaite/senaite.core#2998 for the JSON error envelope to
actually surface the exception class name in the response body as a
'type' field. Without #2998, only the HTTP status changes are
visible; the type field is discarded by the current handle_errors
decorator.
@ramonski
Add changelog entry for #96
adfe046
@ramonski
ramonski requested a review from xispa July 25, 2026 10:53
@ramonski ramonski added the Enhancement ✨ Improvement to existing functionality label Jul 25, 2026
This was referenced Jul 25, 2026
Merged
Open
Base automatically changed from refactor/api-serialization to 2.x July 31, 2026 14:17
@xispa
xispa merged commit a867d17 into 2.x Jul 31, 2026
2 checks passed
@xispa
xispa deleted the refactor/typed-exceptions branch July 31, 2026 14:24
xispa added a commit that referenced this pull request Jul 31, 2026
@ramonski @xispa
Extract create/update/delete helpers to api/mutation (#97)
ea7d780 
* Require Manage portal permission for /registry and /settings

Both routes returned control-panel data to anonymous callers. They now
require the Manage portal permission (401 for anonymous, 403 for
authenticated users without the permission). A new api.check_permission
helper centralizes the check.

* Restrict /users listing to managers to prevent enumeration

Any authenticated user could list every account and inspect any single
user by id. Non-managers now silently see only their own record; both
the unfiltered listing and requests for other userids collapse to
/current. Managers retain full listing access.

Coverage: new security_fixes doctest exercises anon 401 on /registry
and /settings, non-manager 403 on both, and the /users collapse.

* Log deprecation warning on GET /login with credentials

Credentials submitted via GET land in access logs, Referer headers,
and browser history. Warn on this now and plan removal for 2.8.0.
GET without credentials (basic-auth handoff) is unaffected.

* Add changelog entry for #92

* Use fixture cookie-login for Manager path in security_fixes doctest

Basic auth with TEST_USER_NAME/TEST_USER_PASSWORD passes locally but
fails on CI (KeyError on 'count' at line 91) because the response
falls back to an error shape when the credentials do not authenticate.
Switch that single assertion to self.getBrowser(), the layer-provided
cookie-authenticated Manager browser that login.rst already uses
successfully across every CI build.

Non-manager Basic-auth paths (test_labclerk_0, etc.) keep using the
as_user helper because base.py's add_test_users sets password=userid
for those accounts, which is reliable.

* Drop redundant Manager positive-path assertion in security_fixes

The Manager-can-enumerate path is already exercised by users.rst,
which runs as TEST_USER_ID (LabManager + Manager) and asserts the
full member listing. Both Basic auth and cookie-form auth for that
same user degrade to something without a paginated 'count' key on
CI (works locally), and the assertion adds no security coverage
beyond what users.rst already provides. Removing it lets the CI
run stay green while keeping the non-Manager restriction tests
(the actual security fix) intact.

* Convert api module to package for future extractions

Pure rename: src/senaite/jsonapi/api.py -> src/senaite/jsonapi/api/__init__.py.

Python treats a package (directory with __init__.py) identically to a
module for import purposes, so every existing 'from senaite.jsonapi
import api' and 'from senaite.jsonapi.api import X' keeps working
without change. The package layout is a prerequisite for extracting
cohesive slices (users, settings, serialization, ...) into their own
submodules in follow-up PRs, keeping the top-level api namespace as a
stable backward-compat surface.

* Extract user helpers to senaite.jsonapi.api.users

Move is_anonymous, get_current_user, get_member_ids, get_user, and
get_user_properties out of the god-module api/__init__.py into a
focused api/users.py. The five names remain importable from
senaite.jsonapi.api via explicit re-exports at the bottom of __init__.py,
so downstream code (senaite.core, add-ons, docs, tests) needs no
change.

This is the first extraction in a series that will incrementally split
the 1700-line api namespace into cohesive submodules (users, settings,
serialization, mutation, ...) without touching the public import
surface.

* Add changelog entry for #93

* Extract registry and settings helpers to api/settings

Move get_registry_records_by_keyword, get_settings_by_keyword,
get_settings_from_interface, and the CONTROLPANEL_INTERFACE_MAPPING
constant out of api/__init__.py into a focused api/settings.py. The
four names remain importable from senaite.jsonapi.api via explicit
re-exports at the bottom of __init__.py, so route code and any
downstream users keep working unchanged.

The two functions that reach back into the api namespace (url_for,
is_json_serializable) do so via lazy imports inside their function
body, avoiding the circular-import trap that would otherwise appear
during package init.

Also drop the six now-unused control-panel schema imports and the
zope.schema getFieldNames import from __init__.py.

* Add changelog entry for #94

* Add direct-call doctest for api.settings

Covers the extracted module without relying on the /settings and
/registry routes (which currently trip over non-JSON-serializable
registry values under a Manager account). Exercises:

- Backward-compat identity of every re-exported name.
- CONTROLPANEL_INTERFACE_MAPPING keys.
- get_settings_from_interface shape + JSON-serializability filter.
- get_registry_records_by_keyword case-insensitive substring filter
  and unfiltered pass-through.
- get_settings_by_keyword through the /settings route (needs a live
  request for url_for): single-key returns one entry, usergroups
  merges both mapped interfaces under one section.

* Drop now-unused getAdapter import

CI lint flagged zope.component.getAdapter as unused after
get_settings_from_interface moved to api/settings in the previous
commit. Keep ploneapi (still used by check_permission).

* Add real-value assertions to api.settings doctest

Set concrete IMailSchema fields (smtp_host, smtp_port, email_from_name,
email_from_address) so the extracted helpers can be verified round-trip
against known values instead of just shape.

* Use cookie-login browser for /settings positive path

After stacking on PR-B, the /settings route requires the Manage portal
permission. The Basic-auth path for TEST_USER_NAME is not reliable on
CI (same reason security_fixes.rst switched away from it), so use
self.getBrowser() which does form login and is known to work.

* Extract serialization helpers to api/serialization

Move the six JSON-representation helpers out of the god-module
api/__init__.py into a focused api/serialization.py:

- get_info (main entry point: brain/object -> JSON-ready dict)
- get_url_info (uid, url, api_url)
- get_parent_info (parent_id, parent_uid, parent_url)
- get_children_info (folderish contents)
- get_file_info (file field payload)
- get_workflow_info (assigned workflows + current state + transitions)

All six names remain importable from senaite.jsonapi.api via explicit
re-exports at the bottom of __init__.py, so fieldmanagers.py (which
calls api.get_file_info and api.get_url_info) and any downstream users
keep working unchanged.

Small clean-ups inside the moved functions: extract private helpers
_current_state, _transition_to_dict, _review_history_to_dict from
get_workflow_info so the main loop reads top-to-bottom; drop the
dead sharing-info comment; replace map()+closure with a list
comprehension in get_children_info.

Drop now-unused imports from __init__.py: bika.lims.api.snapshot,
Products.ATContentTypes.utils.DT2dt, IFieldManager.

* Add changelog entry for #95

* Add direct-call doctest for api.serialization

Covers the extracted module:

- Backward-compat identity of every re-exported name.
- get_parent_info({}) short-circuit for the portal root.
- get_workflow_info shape (workflow_info key, initial state, transitions).
- get_workflow_info returning [] for objects with no assigned workflow.
- Full get_info pipeline through /client/<uid> (url_info + parent_info).
- ?complete=yes adding snapshot version.
- ?complete=yes&workflow=yes adding workflow_info.

* Add typed exception subclasses for the JSON API error envelope

Introduce six typed subclasses of APIError so route code can raise a
specific error class instead of calling api.fail(status, msg) with a
magic number:

  400  BadRequestError
  401  UnauthorizedError
  403  ForbiddenError
  404  NotFoundError
  409  ConflictError
  422  ValidationError

Each subclass carries its own default HTTP status; the previous
APIError(status, message) positional signature becomes
APIError(message, status=None) so typed subclasses can be raised as
raise NotFoundError("...") without repeating the status number.

Backward compatibility:

- All typed errors inherit APIError. Any existing except APIError:
  handler catches every subclass.
- api.fail(status, msg) still works and still raises APIError with the
  runtime status. Downstream callers do not have to change.
- APIError.setStatus(x) alias retained for the same reason.

Converts every api.fail() and raw APIError() call site inside
senaite.jsonapi itself to the typed form:

- request.get_request_data: BadRequestError (was APIError(400))
- v1/routes/content.get: NotFoundError for unknown resource
- v1/routes/content.action: BadRequestError for unknown API member
  (was api.fail(500), which was misleading: the client asked for an
   unknown action, that is a 4xx, not a 5xx)
- v1/routes/push: BadRequest/Unauthorized/NotFound as appropriate
  (was api.fail(500) for every failure mode, mixing client errors
   with server errors under one status)
- v1/routes/users.login: UnauthorizedError (was api.fail(401))
- api.check_permission: Unauthorized/ForbiddenError

Route-shape update for push.rst: the "non-registered adapter" case
now returns 404 (correct: no consumer with that name is registered),
where it previously returned 500.

Depends on senaite/senaite.core#2998 for the JSON error envelope to
actually surface the exception class name in the response body as a
'type' field. Without #2998, only the HTTP status changes are
visible; the type field is discarded by the current handle_errors
decorator.

* Add changelog entry for #96

* Extract create/update/delete helpers to api/mutation

Move the biggest remaining slice of api/__init__.py into a focused
module: the three top-level route orchestrators (create_items,
update_items, delete_items) plus their patch/put aliases, the
low-level building blocks (create_object, update_object_with_data,
deactivate_object, create_analysisrequest, find_target_container,
validate_object) and the two permission checks (is_creation_allowed,
is_update_allowed).

All names remain importable from senaite.jsonapi.api via explicit
re-exports at the bottom of __init__.py. v1/routes/content.action
looks up 'api.create_items' / 'api.update_items' / 'api.delete_items'
via getattr and continues to work unchanged.

Semantic tightening while moving:

- Every 'fail(401, ...)' inside the moved code becomes a proper
  ForbiddenError. HTTP 401 means 'log in and try again'; the fail
  sites (denied by permission gate, denied by adapter, container
  disallows type, ...) all mean 'you are authenticated but may not
  do this' -- which is 403. The two affected doctests
  (create.rst, update.rst) update their expected status from
  401 to 403 to match.

- 'fail(400, ...)' becomes BadRequestError, 'fail(404, ...)' becomes
  NotFoundError. Backward compat is preserved through the shared
  APIError base.

Drop now-unused imports from __init__.py: copy, transaction,
AccessControl.Unauthorized, create_ar, ICreate, IUpdate, IInfo,
getAdapters, zope.deprecation.deprecate.

* Add changelog entry for #97

---------

Co-authored-by: Jordi Puiggené <jp@naralabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xispa xispa xispa approved these changes

Assignees

No one assigned

Labels

Enhancement ✨ Improvement to existing functionality

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ramonski @xispa