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

Smarter auto fallback #2162

Merged
merged 34 commits into from
Sep 29, 2021
Merged

Smarter auto fallback #2162

merged 34 commits into from
Sep 29, 2021

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Jun 14, 2021

This relates back to the error formatting from #1937.

It adds:

  • error_format on @app.route and convenience methods
  • smarter "auto" check

At startup, every route will now have an error_format assigned to it. This can be configured at the route level (perhaps we can think about adding it at Blueprint or BlueprintGroup).

@app.route(..., error_format="text")

If it is not set, then it will use the value of FALLBACK_ERROR_FORMAT, which can be set to "text", "json", "html", or "auto". It will now default to "auto".


What does auto do now? Previously it would try and look at the incoming headers and make a guess by looking at the request content-type and request body (for json).

Now, it at startup it will evaluate the returns of the route handlers and look for text(), json(), html(), etc. It is still not perfect obviously. And, I think we still might want to add a check incoming Accept headers to make sure the selection makes sense.

If auto does not come up with a guess it will fallback HTMLRenderer, which can be reconfigured if you use your own base Config (see #2119).


TODO:

@Tronic
Copy link
Member

Tronic commented Jun 21, 2021

What does auto do now? Previously it would try and look at the incoming headers and make a guess by looking at the request content-type and request body (for json).

Should be using the accept header instead:

  • If it includes (starts with, even) text/html: HTML
  • If it includes application/json: JSON
  • Else (it is probably */* the default of curl, wget and fetch):
    • check content-type for application/json: JSON (Note 1!)
    • otherwise return TEXT.

Note 1: Request body and content-type detection has the drawback that requests with no body "randomly" return different format error responses, so I might even leave out this detection entirely but certainly wouldn't be looking at request.json. Tough luck for those JSON users who do not correctly specify accept.

@Tronic
Copy link
Member

Tronic commented Jun 21, 2021

Other relevant cases for the above: img, script, video, audio etc. tags, where that logic would return a text response. What is returned doesn't matter much because browsers won't show the response even in developer tools; maybe an empty response would be more appropriate.

@ahopkins
Copy link
Member Author

Other relevant cases for the above: img, script, video, audio etc. tags, where that logic would return a text response. What is returned doesn't matter much because browsers won't show the response even in developer tools; maybe an empty response would be more appropriate.

Not sure that I agree that it should be an empty response; you could be accessing the resource directly in a browser.

@ahopkins ahopkins marked this pull request as ready for review August 30, 2021 13:23
@ahopkins ahopkins requested a review from a team as a code owner August 30, 2021 13:23
@ahopkins ahopkins marked this pull request as draft August 30, 2021 13:26
@Tronic
Copy link
Member

Tronic commented Aug 30, 2021

Other relevant cases for the above: img, script, video, audio etc. tags, where that logic would return a text response. What is returned doesn't matter much because browsers won't show the response even in developer tools; maybe an empty response would be more appropriate.

Not sure that I agree that it should be an empty response; you could be accessing the resource directly in a browser.

I believe in that case the accept header lists text/html. It only shows image and other formats if the request is from an HTML element.

@ahopkins
Copy link
Member Author

OK. I'll check that.

@ahopkins ahopkins marked this pull request as ready for review September 26, 2021 08:52
@ahopkins
Copy link
Member Author

This is roughly the logic:

  • Use explicit error_format, if available
  • Use likely intended response type, if available (this will cover the MAJORITY of use cases)
  • Use the fallback
  • If the fallback is auto:
    • Look to see if text/html is first in Accept header, use HTMLRenderer
    • Look to see if application/json is anywhere in Accept or if incoming request has content-type: application/json, use JSONRenderer
    • Check if completely missing Accept header, use TextRenderer (Likely coming from a raw socket)
    • If still nothing matches...
      • Check if its a poorly formatted JSON request, and use JSONRenderer (covers backwards compat)
      • Fallback to TextRenderer if curl
  • In all cases, if there is an Accept header, make sure it is allowed
  • If not, loop thru the Accept and stop at the first match (we may need to add some more potential matches here, but that can be in another iteration)
  • If still nothing, use base renderer (which is HTMLRenderer for compat. Change this to TextRenderer in v22.3)

sanic/errorpages.py Outdated Show resolved Hide resolved
sanic/errorpages.py Outdated Show resolved Hide resolved
tests/test_errorpages.py Outdated Show resolved Hide resolved
tests/test_errorpages.py Outdated Show resolved Hide resolved
Copy link
Member

@vltr vltr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems all good to me! 😎

@codeclimate
Copy link

codeclimate bot commented Sep 29, 2021

Code Climate has analyzed commit 0e0b982 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 97.9% (86% is the threshold).

This pull request will bring the total coverage in the repository to 87.1% (0.0% change).

View more on Code Climate.

@ahopkins ahopkins merged commit cf1d214 into main Sep 29, 2021
@ahopkins ahopkins deleted the enhanced-error-fallback branch September 29, 2021 20:53
@ahopkins ahopkins mentioned this pull request Sep 30, 2021
ahopkins added a commit that referenced this pull request Oct 2, 2021
* Remove unnecessary import in test_constants.py, which also fixes an error on win (#2180)

Co-authored-by: Adam Hopkins <admhpkns@gmail.com>

* Manually reset the buffer when streaming request body (#2183)

* Remove Duplicated Dependencies and PEP 517 Support (#2173)

* Remove duplicated dependencies

* Specify setuptools as the tool for generating distribution (PEP 517)

* Add `isort` to `dev_require`

* manage all dependencies in setup.py

* Execute `make pretty`

* Set usedevelop to true (revert previous change)

* Fix the handling of the end of a chunked request. (#2188)

* Fix the handling of the end of a chunked request.

* Avoid hardcoding final chunk header size.

* Add some unit tests for pipeline body reading

* Decode bytes for json serialization

Co-authored-by: L. Kärkkäinen <tronic@users.noreply.github.com>
Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* Resolve regressions in exceptions (#2181)

* Update sanic-routing to fix path issues plus lookahead / lookbehind support (#2178)

* Update sanic-routing to fix path issues plus lookahead / lookbehind support

* Update setup.py

Co-authored-by: Adam Hopkins <adam@amhopkins.com>
Co-authored-by: Adam Hopkins <admhpkns@gmail.com>

* style(app,blueprints): add some type hints (#2196)

* style(app,blueprints): add some type hints

* style(app): option is Any

* style(blueprints): url prefix default value is ``""``

* style(app): backward compatible

* style(app): backward compatible

* style(blueprints): defult is None

* style(app): apply code style (black)

* Update some CC config (#2199)

* Update README.rst

* raise exception for `_static_request_handler` unknown exception; add test with custom error (#2195)

Co-authored-by: n.feofanov <n.feofanov@visionlabs.ru>
Co-authored-by: Adam Hopkins <admhpkns@gmail.com>

* Change dumps to AnyStr (#2193)

* HTTP tests (#2194)

* Fix issues with after request handling in HTTP pipelining (#2201)

* Clean up after a request is complete, before the next pipelined request.

* Limit the size of request body consumed after handler has finished.

* Linter error.

* Add unit test re: bad headers

Co-authored-by: L. Kärkkäinen <tronic@users.noreply.github.com>
Co-authored-by: Adam Hopkins <admhpkns@gmail.com>
Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* Update CHANGELOG

* Log remote address if available (#2207)

* Log remote address if available

* Add tests

* Fix testing version

Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* Fixed for handling exceptions of asgi app call. (#2211)

@cansarigol3megawatt Thanks for looking into this and getting the quick turnaround on this. I will 🍒 pick this into the 21.6 branch and get it out a little later tonight.

* Signals Integration (#2160)

* Update some tests

* Resolve #2122 route decorator returning tuple

* Use rc sanic-routing version

* Update unit tests to <:str>

* Minimal working version with some signals implemented

* Add more http signals

* Update ASGI and change listeners to signals

* Allow for dynamic ODE signals

* Allow signals to be stacked

* Begin tests

* Prioritize match_info on keyword argument injection

* WIP on tests

* Compat with signals

* Work through some test coverage

* Passing tests

* Post linting

* Setup proper resets

* coverage reporting

* Fixes from vltr comments

* clear delayed tasks

* Fix bad test

* rm pycache

* uncomment windows tests (#2214)

* Add convenience methods to BP groups (#2209)

* Fix bug where ws exceptions not being logged (#2213)

* Fix bug where ws exceptions not being logged

* Fix t\est

* Style: add type hints (#2217)

* style(routes): add_route argument, return typing

* style(listeners): typing

* style(views): typing as_view

* style(routes): change type hint

* style(listeners): change type hint

* style(routes): change type hint

* add some more types

* Change as_view typing

* Add some cleaner type annotations

Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* Add default messages to SanicExceptions (#2216)

* Add default messages to SanicExceptions

* Cleaner exception message setting

* Copy Blueprints Implementation (#2184)

* Accept header parsing (#2200)

* Add some tests

* docstring

* Add accept matching

* Add some more tests on matching

* Add matching flags for wildcards

* Add mathing controls to accept

* Limit uvicorn 14 in testing

* Add convenience for annotated handlers (#2225)

* Split HttpProtocol parts into base SanicProtocol and HTTPProtocol subclass (#2229)

* Split HttpProtocol parts into base SanicProtocol and HTTPProtocol subclass.

* lint fixes

* re-black server.py

* Move server.py into its own module (#2230)

* Move server.py into its own module

* Change monkeypatch path on test_logging.py

* Blueprint specific exception handlers (#2208)

* Call abort() on sockets after close() to prevent dangling sockets (#2231)

* Add ability to return Falsey but not-None from handlers (#2236)

* Adds Blueprint Group exception decorator (#2238)

* Add exception decorator

* Added tests

* Fix line too long

* Static DIR and FILE resource types (#2244)

* Explicit static directive for serving file or dir


Co-authored-by: anbuhckr <36891836+anbuhckr@users.noreply.github.com>
Co-authored-by: anbuhckr <miki.suhendra@gmail.com>

* Close HTTP loop when connection task cancelled (#2245)

* Terminate loop when no transport exists

* Add log when closing HTTP loop because of shutdown

* Add unit test

* New websockets (#2158)

* First attempt at new Websockets implementation based on websockets >= 9.0, with sans-i/o features. Requires more work.

* Update sanic/websocket.py

Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* Update sanic/websocket.py

Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* Update sanic/websocket.py

Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* wip, update websockets code to new Sans/IO API

* Refactored new websockets impl into own modules
Incorporated other suggestions made by team

* Another round of work on the new websockets impl
* Added websocket_timeout support (matching previous/legacy support)
* Lots more comments
* Incorporated suggested changes from previous round of review
* Changed RuntimeError usage to ServerError
* Changed SanicException usage to ServerError
* Removed some redundant asserts
* Change remaining asserts to ServerErrors
* Fixed some timeout handling issues
* Fixed websocket.close() handling, and made it more robust
* Made auto_close task smarter and more error-resilient
* Made fail_connection routine smarter and more error-resilient

* Further new websockets impl fixes
* Update compatibility with Websockets v10
* Track server connection state in a more precise way
* Try to handle the shutdown process more gracefully
* Add a new end_connection() helper, to use as an alterative to close() or fail_connection()
* Kill the auto-close task and keepalive-timeout task when sanic is shutdown
* Deprecate WEBSOCKET_READ_LIMIT and WEBSOCKET_WRITE_LIMIT configs, they are not used in this implementation.

* Change a warning message to debug level
Remove default values for deprecated websocket parameters

* Fix flake8 errors

* Fix a couple of missed failing tests

* remove websocket bench from examples

* Integrate suggestions from code reviews
Use Optional[T] instead of union[T,None]
Fix mypy type logic errors
change "is not None" to truthy checks where appropriate
change "is None" to falsy checks were appropriate
Add more debug logging when debug mode is on
Change to using sanic.logger for debug logging rather than error_logger.

* Fix long line lengths of debug messages
Add some new debug messages when websocket IO is paused and unpaused for flow control
Fix websocket example to use app.static()

* remove unused import in websocket example app

* re-run isort after Flake8 fixes

Co-authored-by: Adam Hopkins <adam@amhopkins.com>
Co-authored-by: Adam Hopkins <admhpkns@gmail.com>

* Account for BP with exception handler but no routes (#2246)

* Don't log "enabled" if auto-reload disabled (#2247)

Fixes #2240

Co-authored-by: Adam Hopkins <admhpkns@gmail.com>

* Smarter auto fallback (#2162)

* Smarter auto fallback

* remove config from blueprints

* Add tests for error formatting

* Add check for proper format

* Fix some tests

* Add some tests

* docstring

* Add accept matching

* Add some more tests on matching

* Fix contains bug, earlier return on MediaType eq

* Add matching flags for wildcards

* Add mathing controls to accept

* Cleanup dev cruft

* Add cleanup and resolve OSError relating to test implementation

* Fix test

* Fix some typos

* Some fixes to the new Websockets impl (#2248)

* First attempt at new Websockets implementation based on websockets >= 9.0, with sans-i/o features. Requires more work.

* Update sanic/websocket.py

Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* Update sanic/websocket.py

Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* Update sanic/websocket.py

Co-authored-by: Adam Hopkins <adam@amhopkins.com>

* wip, update websockets code to new Sans/IO API

* Refactored new websockets impl into own modules
Incorporated other suggestions made by team

* Another round of work on the new websockets impl
* Added websocket_timeout support (matching previous/legacy support)
* Lots more comments
* Incorporated suggested changes from previous round of review
* Changed RuntimeError usage to ServerError
* Changed SanicException usage to ServerError
* Removed some redundant asserts
* Change remaining asserts to ServerErrors
* Fixed some timeout handling issues
* Fixed websocket.close() handling, and made it more robust
* Made auto_close task smarter and more error-resilient
* Made fail_connection routine smarter and more error-resilient

* Further new websockets impl fixes
* Update compatibility with Websockets v10
* Track server connection state in a more precise way
* Try to handle the shutdown process more gracefully
* Add a new end_connection() helper, to use as an alterative to close() or fail_connection()
* Kill the auto-close task and keepalive-timeout task when sanic is shutdown
* Deprecate WEBSOCKET_READ_LIMIT and WEBSOCKET_WRITE_LIMIT configs, they are not used in this implementation.

* Change a warning message to debug level
Remove default values for deprecated websocket parameters

* Fix flake8 errors

* Fix a couple of missed failing tests

* remove websocket bench from examples

* Integrate suggestions from code reviews
Use Optional[T] instead of union[T,None]
Fix mypy type logic errors
change "is not None" to truthy checks where appropriate
change "is None" to falsy checks were appropriate
Add more debug logging when debug mode is on
Change to using sanic.logger for debug logging rather than error_logger.

* Fix long line lengths of debug messages
Add some new debug messages when websocket IO is paused and unpaused for flow control
Fix websocket example to use app.static()

* remove unused import in websocket example app

* re-run isort after Flake8 fixes

* Some fixes to the new Websockets impl
Will throw WebsocketClosed exception instead of ServerException now when attempting to read or write to closed websocket, this makes it easier to catch
The various ws.recv() methods now have the ability to raise CancelledError into your websocket handler
Fix a niche close-socket negotiation bug
Fix bug where http protocol thought the websocket never sent any response.
Allow data to still send in some cases after websocket enters CLOSING state.
Fix some badly formatted and badly placed comments

* allow eof_received to send back data too, if the connection is in CLOSING state

Co-authored-by: Adam Hopkins <adam@amhopkins.com>
Co-authored-by: Adam Hopkins <admhpkns@gmail.com>

* 21.9 release docs (#2218)

* Beging 21.9 release docs

* Add PRs to changelog

* Change deprecation version

* Update logging tests

* Bump version

* Update changelog

* Change dev install command (#2251)

Co-authored-by: Zhiwei <zhi.wei.liang@outlook.com>
Co-authored-by: L. Kärkkäinen <98187+Tronic@users.noreply.github.com>
Co-authored-by: L. Kärkkäinen <tronic@users.noreply.github.com>
Co-authored-by: Robert Palmer <robd003@users.noreply.github.com>
Co-authored-by: Ryu JuHeon <saidbysolo@gmail.com>
Co-authored-by: gluhar2006 <49654448+gluhar2006@users.noreply.github.com>
Co-authored-by: n.feofanov <n.feofanov@visionlabs.ru>
Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
Co-authored-by: Can Sarigol <56863826+cansarigol3megawatt@users.noreply.github.com>
Co-authored-by: Zhiwei <chihwei.public@outlook.com>
Co-authored-by: YongChan Cho <h3236516@gmail.com>
Co-authored-by: Zhiwei <zhiwei@sinatra.ai>
Co-authored-by: Ashley Sommer <ashleysommer@gmail.com>
Co-authored-by: anbuhckr <36891836+anbuhckr@users.noreply.github.com>
Co-authored-by: anbuhckr <miki.suhendra@gmail.com>
@ahopkins ahopkins mentioned this pull request Oct 27, 2021
6 tasks
ChihweiLHBird pushed a commit to ChihweiLHBird/sanic that referenced this pull request Jun 1, 2022
* Smarter auto fallback

* remove config from blueprints

* Add tests for error formatting

* Add check for proper format

* Fix some tests

* Add some tests

* docstring

* Add accept matching

* Add some more tests on matching

* Fix contains bug, earlier return on MediaType eq

* Add matching flags for wildcards

* Add mathing controls to accept

* Cleanup dev cruft

* Add cleanup and resolve OSError relating to test implementation

* Fix test

* Fix some typos
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