Skip to content
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

Error page rendering format selection #2668

Merged
merged 73 commits into from
Feb 26, 2023
Merged

Error page rendering format selection #2668

merged 73 commits into from
Feb 26, 2023

Conversation

Tronic
Copy link
Member

@Tronic Tronic commented Jan 29, 2023

Error format selection

This PR simplifies the error handling logic in Sanic by making it more dependent on the accept header when available. The goal is to produce more suitable responses for real-world use cases, such as always returning HTML on pages displayed in a browser, even if the handler would normally return text().

Sanic's preferred format is determined by route error_format (set by app, or determined from handler source code if "auto", the default) and by app.config.FALLBACK_ERROR_FORMAT.

If the fallback is set to "auto" (default), all Sanic's supported formats are offered such that client preference takes priority. If the route also had no format determined, Sanic uses heuristics for request JSON format: is it verbatim in accept header or content-type, or does the body parse as JSON (the latter is being DEPRECATED in this PR). If no JSON is found either, text becomes the first preferred format of any remaining formats.

Sanic logs show which accept part matched the format and from where it came (route name, content-type, request.json, FALLBACK, any), or if there was no match with accept at all, in which case the base renderer is used.

Just as before, the base renderer is always text unless set via undocumented functionality Sanic(error_handler=ErrorHandler(SomeOtherRenderer)). It was previously used in many cases but now it is only reached if a client specifically asks only for unsupported format(s), e.g. accept: application/xml.

Typical use cases

  • Viewing in browser
    • If fallback format is auto, always gets html because text/html is the only q=1 match
    • Otherwise, uses Sanic's preferred format from route or fallback (matching */*;q=0.8)
  • Browsers loading img or other resources only match the final */*;q=0.8 and get Sanic's preferred format
  • Curl, requests, fetch(), XHR etc. default to accept: */* and get Sanic's preferred format
  • Clients setting accept to a specific format get that or text

Background

In Sanic 22.12:

  • Text/JSON are returned when viewed in browser (affects text("hello world") type apps)
  • HTML or text is returned to applications expecting JSON but returning empty()
  • The fallback setting has little effect (using route formats or hardcoded text format instead)

There are breaking changes needing careful review, but the PR is still planned for release in 23.3. It addresses the above urgent issues by respecting the q values given in accept and reducing the use of the base renderer. Those who previously received JSON mostly still receive it, and can (as before) force it by accept header or by setting error_format="json" on the route.

The previous large rewrite was in #2162 (patched in #2310), originally stemming from #1937 where text and json were added as alternatives to html error pages. There is useful background and explanation of the existing logic in these issues.

PR #2659 is related to this and fixed some of the issues mentioned above. This PR is based on the new request.accept functionality merged in #2687.

@Tronic
Copy link
Member Author

Tronic commented Jan 29, 2023

  • The client sends accept: application/xml
  • The app sets FALLBACK_ERROR_FORMAT = "json", and
  • The handler returns html(...) but raises an exception.

Which of text/json/html should be used on the error response?

Oh, but text of course, because html and json don't fit the accept header, I'll just send text instead!

This PR still does that but I am not quite sure there is a better answer. Presumably someone could supply XMLRenderer as an alternative base renderer? But this is a niche case, as usually all formats are accepted or at least the right format is.

@Tronic
Copy link
Member Author

Tronic commented Jan 29, 2023

TODO: Refactor _guess_renderer to return a string rather than a renderer object, to make it usable for other things as well (file browser API?). Do the Renderer lookups instead in exception_response that calls the other function.

@Tronic
Copy link
Member Author

Tronic commented Jan 31, 2023

89 failing tests on main branch, 90 failing tests on this one but headers and errorpages are fully passing. Hard to find the last one...

JSON header check now does accept (for literal application/json) as well as content-type (when route and fallback are auto and not found). This only affects the case when multiple supported formats are accepted, where this gives JSON the higher priority regardless of q values (backwards compatibility, hopefully avoids breaking someone's API calls).

@sjsadowski
Copy link
Contributor

Note: I'm tagging myself in here to review and understand the changes

@sjsadowski sjsadowski self-requested a review February 19, 2023 01:53
@ahopkins
Copy link
Member

@Tronic I resolved the merge conflicts in this PR into this branch: #2691

@ahopkins ahopkins changed the title Error page rendering format selection and request.accept rewritten Error page rendering format selection Feb 21, 2023
sanic/errorpages.py Outdated Show resolved Hide resolved
ahopkins
ahopkins previously approved these changes Feb 21, 2023
sanic/errorpages.py Outdated Show resolved Hide resolved
@ahopkins ahopkins merged commit dfc0704 into main Feb 26, 2023
@ahopkins ahopkins deleted the error-format-redux branch February 26, 2023 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants