-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restore custom error views (but return JSON where desired) #11722 #11784
Conversation
def looks_like_api_call(request): | ||
known_api_paths_without_api_signposting = [ | ||
# Until these are renamed for consistency, just list them. | ||
"arches.app.views.workflow_history", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add more as we find them, but I'm considering it out of scope to audit everything right now: that's a lot of views to analyze. See next comment for why this one is here.
self.assertEqual(response.status_code, 403) | ||
self.assertIn(b"Forbidden", response.content) | ||
self.assertContains( | ||
response, "Permission Denied", status_code=HTTPStatus.FORBIDDEN | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this error condition was already tested, we caught an example of HTML where we should have sent JSON.
Before:
\n<!doctype html>\n<html lang="en">\n<head>\n <title>403 Forbidden</title>\n</head>\n<body>\n <h1>403 Forbidden</h1><p></p>\n</body>\n</html>\n
After:
{"message": "Permission Denied", "status": "false", "success": false, "title": "Request Failed"}
@@ -781,9 +781,13 @@ | |||
api.SpatialView.as_view(), | |||
name="spatialview_api", | |||
), | |||
re_path("^api", api.API404.as_view(), name="api_404"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we don't need this anymore to handle errors, which is good, because it was too fragile in practice (sneaky failures depending on the order in which you included your other URLs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Types of changes
Description of Change
This PR addresses two issues:
.json()
on a response body won't throw when they get an HTML response from the generic (or customized) error view.Issues Solved
Closes #11722
Checklist
Ticket Background