Add text and json fallback error handlers #1937

ahopkins · 2020-09-29T08:59:18Z

Replaces #1905

PR #1768 added HTML default exception pages. This PR adds two alternative fallback exception pages: text and JSON.

This is NOT meant to replace custom error handlers, but is meant to be for fallback.

This is configurable with:

app.config.FALLBACK_ERROR_FORMAT = "text"
# or
app.config.FALLBACK_ERROR_FORMAT = "json"

The default will still be HTML. And, the HTML page is untouched (except for some fixes to make valid HTML.

Just like in HTML, there are two handlers: one for debuggable exception responses, the other for minimal responses.

Text

⚠️ 500 — Internal Server Error
==============================
foobar

Exception: foobar while handling path /hello
Traceback of __BASE__ (most recent call last):

  Exception: foobar
    File /path/to/sanic/app.py, line 938, in handle_request
    response = await response

    File server.py, line 12, in foo
    raise Exception("foobar")

⚠️ 500 — Internal Server Error
==============================
The server encountered an internal error and cannot complete your request.

JSON

{
  "description": "Internal Server Error",
  "status": 500,
  "message": "foobar",
  "path": "/hello",
  "args": {
    "name": [
      "world"
    ]
  },
  "exceptions": [
    {
      "type": "Exception",
      "exception": "foobar",
      "frames": [
        {
          "file": "/path/to/sanic/app.py",
          "line": 938,
          "name": "handle_request",
          "src": "response = await response"
        },
        {
          "file": "server.py",
          "line": 12,
          "name": "foo",
          "src": "raise Exception(\"foobar\")"
        }
      ]
    }
  ]
}

{
  "description": "Internal Server Error",
  "status": 500,
  "message": "The server encountered an internal error and cannot complete your request."
}

ahopkins · 2020-09-29T09:01:14Z

I am still working on the tests, but I wanted to get this out there for some feedback ASAP so we can try and include it in upcoming release. It is not a breaking change, and a relatively minor change.

ashleysommer

Looks great!

FWIW, the old HTML responses are actually valid HTML5.
The <!DOCTYPE html> hint tag puts the document in HTML5-mode. In HTML5 the <html> document wrapper tag is not needed (though most people put it in anyway for html4 backward compatibility). Also the <head> tag is technically optional, you can just put your meta, script, title, and style tags before your content. Along the same lines, it doesn't need a <body> tag if there's no head before it.
<p> tags never need to be closed (some linters even generate an error for a closing </p> tag) and finally </body> and </html> closing tags don't need to be present at the end of the file (they are assumed present if the HTML just ends).

ahopkins · 2020-09-29T13:19:34Z

Looks great!

FWIW, the old HTML responses are actually valid HTML5.

The <!DOCTYPE html> hint tag puts the document in HTML5-mode. In HTML5 the <html> document wrapper tag is not needed (though most people put it in anyway for html4 backward compatibility). Also the <head> tag is technically optional, you can just put your meta, script, title, and style tags before your content. Along the same lines, it doesn't need a <body> tag if there's no head before it.

<p> tags never need to be closed (some linters even generate an error for a closing </p> tag) and finally </body> and </html> closing tags don't need to be present at the end of the file (they are assumed present if the HTML just ends).

I put it in a linter and it yelled at me 🤷‍♂️

vltr · 2020-09-29T13:20:24Z

long time no see, but I really like this PR. I was just wondering: what if we read the Content-Type header from the request (in case it exists) and provide the error properly? let's say, text/plain would return the text error, application/json would return the JSON error and anything else not declared would return the default fallback? it's just an initial thought, it's open to ideas 😅

ahopkins · 2020-09-29T13:24:10Z

long time no see, but I really like this PR. I was just wondering: what if we read the Content-Type header from the request (in case it exists) and provide the error properly? let's say, text/plain would return the text error, application/json would return the JSON error and anything else not declared would return the default fallback? it's just an initial thought, it's open to ideas 😅

I like this idea. Maybe this can be the behavior for

app.config.FALLBACK_ERROR_FORMAT = "auto"

I would suggest that would be a sensible default. But since that would be a breaking change, maybe we don't make that default until after LTS.

vltr · 2020-09-29T13:30:53Z

app.config.FALLBACK_ERROR_FORMAT = "auto"

hey, I like that as well! 😎 I don't know if this will add more complexity to the code (after all, "gotta go fast"), but this is just in case of an error, which is not what any developer is looking on their app (except during development, sure), so ... but, even so, I'm pretty sure that could be passed to a constructor / partial to eliminate any unnecessary ifs and / or elses from the code (not that I'm concerned, but I had to bring this up before this could become a concern).

ahopkins · 2020-09-29T13:45:48Z

@vltr, I think the main usage of this is really for development purposes. They should be safe enough to not leak in production, but this is not intended to be a production-ready replacement for error handling. Primarily because (as mentioned in the other thread), there are too many considerations for us to be able to account for. With that said, I will keep this in mind.

vltr · 2020-09-29T18:37:57Z

@ahopkins yeap, being just for development I think it's fine ... I just mentioned about performance because, well, never mind, that should not be the case 👍

ahopkins · 2020-09-29T19:21:25Z

Looks great!

FWIW, the old HTML responses are actually valid HTML5.
The <!DOCTYPE html> hint tag puts the document in HTML5-mode. In HTML5 the <html> document wrapper tag is not needed (though most people put it in anyway for html4 backward compatibility). Also the <head> tag is technically optional, you can just put your meta, script, title, and style tags before your content. Along the same lines, it doesn't need a <body> tag if there's no head before it.
<p> tags never need to be closed (some linters even generate an error for a closing </p> tag) and finally </body> and </html> closing tags don't need to be present at the end of the file (they are assumed present if the HTML just ends).

I discovered the issue. Needed a single closing </div>.

vltr · 2020-09-29T19:27:48Z

I discovered the issue. Needed a single closing </div>.

"The missing </div>". Sorry, but I had to ... 🤣

ashleysommer · 2020-09-30T01:14:31Z

Ok, I think we can still push this out in this release, with default format as "html", to keep compatibility. But we'll change it to default to "auto" in v21.03.

ashleysommer · 2020-09-30T01:16:56Z

There are still some type-hinting mypy errors

sanic/errorpages.py:310: error: Incompatible types in assignment (expression has type "Type[HTMLRenderer]", variable has type "Optional[BaseRenderer]")

sanic/errorpages.py:315: error: Incompatible types in assignment (expression has type "Union[Type[JSONRenderer], Type[HTMLRenderer]]", variable has type "Optional[BaseRenderer]")

sanic/errorpages.py:317: error: Incompatible types in assignment (expression has type "Type[HTMLRenderer]", variable has type "Optional[BaseRenderer]")

sanic/errorpages.py:322: error: Incompatible types in assignment (expression has type "Union[Type[BaseRenderer], BaseRenderer, None]", variable has type "Optional[BaseRenderer]")

sanic/errorpages.py:327: error: Incompatible types in assignment (expression has type "Union[Type[BaseRenderer], BaseRenderer, None]", variable has type "Optional[BaseRenderer]")

sanic/errorpages.py:329: error: "BaseRenderer" not callable

sanic/errorpages.py:329: error: "None" not callable

codecov · 2020-09-30T06:34:42Z

Codecov Report

Merging #1937 into master will increase coverage by 0.28%.
The diff coverage is 99.24%.

@@            Coverage Diff             @@
##           master    #1937      +/-   ##
==========================================
+ Coverage   92.45%   92.73%   +0.28%     
==========================================
  Files          27       27              
  Lines        3100     3192      +92     
  Branches      555      563       +8     
==========================================
+ Hits         2866     2960      +94     
+ Misses        157      155       -2     
  Partials       77       77

Impacted Files	Coverage Δ
sanic/config.py	`100.00% <ø> (ø)`
sanic/errorpages.py	`99.25% <99.24%> (-0.75%)`	⬇️
sanic/server.py	`81.61% <0.00%> (+0.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c4a9b1...fdfc129. Read the comment docs.

ashleysommer

Go ahead and merge

Tronic · 2020-10-04T08:56:37Z

If I understood the discussion and the code correctly, only the request Content-Type header is used for determining which format to return in auto mode. Content-Type on request specifies the format of the request itself, and is normally not provided in GET requests which carry no body.

It might be a good idea to use the Accept request header if available, and see if it explicitly lists text/html (I think all browsers put that at the very beginning). Curl and wget by default send Accept: */* in which case a text response is more appropriate. Some JSON APIs only work when Accept: application/json is specified on the request, so despite the widespread misuse of Content-Type for this, it might be enough to only examine the Accept header.

ahopkins · 2020-10-04T15:05:22Z

I thought about that, but as you said, most browsers list a bunch of different options in Accept headers.

Before this becomes default, we need a much smarter logic switch on this.

Tronic · 2020-10-05T07:28:01Z

I just tested Chrome, Firefox, Safari, Edge, IE11, Android-Chrome, Android-Firefox, iOS-Safari and Opera Mini, which all can be handled by accept.startswith("text/html"), really fast with no need for further parsing. The fetch API uses */* by default, as do wget and curl. (except for IE11 which doesn't support fetch and Opera Mini which uses */*;image/webp for whatever the reason).

The results and a quickly hacked testing page at https://browser.zi.fi/

ahopkins · 2020-10-05T19:13:31Z

Nice. @Tronic can you move this to a new issue for further development?

Add text and json fallback error handlers

8b36ea3

ahopkins mentioned this pull request Sep 29, 2020

making default reponse text, with optional formatting to be passed #1905

Closed

ahopkins requested review from ashleysommer and Tronic September 29, 2020 09:01

squash

713c17e

ashleysommer reviewed Sep 29, 2020

View reviewed changes

ahopkins added 2 commits September 29, 2020 23:56

Add tests and auto-detect error fallback type

6c25022

Add tests and auto-detect error fallback type

360180a

ahopkins requested review from ashleysommer, vltr and sjsadowski September 29, 2020 20:56

ahopkins added 2 commits September 29, 2020 23:59

squash

2f589e4

squash

9726a79

ashleysommer previously approved these changes Sep 30, 2020

View reviewed changes

squash

fdfc129

ahopkins dismissed ashleysommer’s stale review via fdfc129 September 30, 2020 06:17

ahopkins requested a review from ashleysommer September 30, 2020 06:20

ashleysommer approved these changes Sep 30, 2020

View reviewed changes

ahopkins merged commit 066df2c into master Sep 30, 2020

ahopkins deleted the default-error-handlers branch September 30, 2020 12:11

ahopkins mentioned this pull request Sep 30, 2020

Version 20.9 #1940

Merged

ahopkins mentioned this pull request Jan 11, 2021

Exception formatting in handlers.py #1230

Closed

This was referenced Jun 14, 2021

Smarter auto fallback #2162

Merged

RFC: Accept header parsing #2171

Closed

Tronic mentioned this pull request Jan 29, 2023

Error page rendering format selection #2668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add text and json fallback error handlers #1937

Add text and json fallback error handlers #1937

ahopkins commented Sep 29, 2020 •

edited

ahopkins commented Sep 29, 2020

ashleysommer left a comment •

edited

ahopkins commented Sep 29, 2020

vltr commented Sep 29, 2020

ahopkins commented Sep 29, 2020 •

edited

vltr commented Sep 29, 2020

ahopkins commented Sep 29, 2020

vltr commented Sep 29, 2020

ahopkins commented Sep 29, 2020

vltr commented Sep 29, 2020

ashleysommer commented Sep 30, 2020

ashleysommer commented Sep 30, 2020 •

edited

codecov bot commented Sep 30, 2020 •

edited

ashleysommer left a comment

Tronic commented Oct 4, 2020

ahopkins commented Oct 4, 2020 •

edited

Tronic commented Oct 5, 2020 •

edited

ahopkins commented Oct 5, 2020

Add text and json fallback error handlers #1937

Add text and json fallback error handlers #1937

Conversation

ahopkins commented Sep 29, 2020 • edited

Text

JSON

ahopkins commented Sep 29, 2020

ashleysommer left a comment • edited

Choose a reason for hiding this comment

ahopkins commented Sep 29, 2020

vltr commented Sep 29, 2020

ahopkins commented Sep 29, 2020 • edited

vltr commented Sep 29, 2020

ahopkins commented Sep 29, 2020

vltr commented Sep 29, 2020

ahopkins commented Sep 29, 2020

vltr commented Sep 29, 2020

ashleysommer commented Sep 30, 2020

ashleysommer commented Sep 30, 2020 • edited

codecov bot commented Sep 30, 2020 • edited

Codecov Report

ashleysommer left a comment

Choose a reason for hiding this comment

Tronic commented Oct 4, 2020

ahopkins commented Oct 4, 2020 • edited

Tronic commented Oct 5, 2020 • edited

ahopkins commented Oct 5, 2020

ahopkins commented Sep 29, 2020 •

edited

ashleysommer left a comment •

edited

ahopkins commented Sep 29, 2020 •

edited

ashleysommer commented Sep 30, 2020 •

edited

codecov bot commented Sep 30, 2020 •

edited

ahopkins commented Oct 4, 2020 •

edited

Tronic commented Oct 5, 2020 •

edited