Conversation
error_handling_router decided between JSON and HTML by looking at the matched URL rule, which is None when no route matched, so an unknown URL under /api fell through to the HTML error page unless the request itself carried a JSON content type. Decide on the request path instead, so every error under /api is answered in JSON. Adds tests on a bare Flask app with only the generic error handler, and entries in the API change log and the changelog. Closes FlexMeasures#2598
Documentation build overview
10 files changed ·
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new test’s HTML fallback handler is misnamed (NotFoundError_handler_html vs NotFound_handler_html), which will prevent the intended HTML-vs-JSON distinction from working as asserted.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR fixes error response negotiation so that unknown URLs under /api consistently return a JSON 404 (instead of the HTML error page when no route matches), aligning behavior for API clients and addressing #2598.
Changes:
- Update
error_handling_routerto decide JSON vs HTML based onrequest.path.startswith("/api")rather thanrequest.url_rule. - Add focused tests using a bare Flask app to ensure unknown
/apipaths return JSON404, while non-API paths keep HTML behavior. - Add changelog entries documenting the behavior change for both general users and API consumers.
| File | Description |
|---|---|
| flexmeasures/utils/error_utils.py | Routes all errors under /api to JSON responses even when no Flask route matched |
| flexmeasures/utils/tests/test_error_utils.py | Adds regression tests for unknown /api URLs and preserves HTML behavior outside /api |
| documentation/changelog.rst | Notes the bugfix in the main changelog under Bugfixes |
| documentation/api/change_log.rst | Documents the API-facing behavior change under v3.0-40 |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+12
to
+16
| app.NotFoundError_handler_html = lambda error: ( | ||
| "<html>not found</html>", | ||
| 404, | ||
| {"Content-Type": "text/html"}, | ||
| ) |
Comment on lines
+65
to
67
| We respond in JSON if the request content-type is JSON, if the request was made under /api | ||
| (whether or not it matched a route, so that an unknown API URL gets a JSON 404, too), | ||
| or if the error is a SecurityError. |
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #2598
error_handling_routerdecided between a JSON and an HTML response by looking atrequest.url_rule, which isNonewhen no route matched, so an unknown URL under/api(a typo, an outdated API version) got the HTML error page unless the request carried a JSON content type. It now checksrequest.path.startswith("/api"), as proposed in the issue, so every error under/apiis answered in JSON.GET /api/v3_0/no-such-endpoint(no content type)404,text/html404,application/jsonGET /api/v3_0/no-such-endpointwithContent-Type: application/json404,application/jsonGET /no-such-page(no content type)How to test
pytest flexmeasures/utils/tests/test_error_utils.py— new tests on a bare Flask app with onlyadd_basic_error_handlers(plus a dummy HTML handler to tell the two apart): three unknown/apipaths get a JSON 404 withmessageandstatus; an unknown non-API path still gets HTML; a JSON request outside/apistill gets JSON. The three/apicases fail onmain. I don't have a Postgres at hand, so the full app tests I could not run here.Further Improvements
None.
Related Items
Change-log entries added under v3.0-40 in
documentation/api/change_log.rstand under Bugfixes indocumentation/changelog.rst.