Skip to content

Conversation

@delicb
Copy link
Contributor

@delicb delicb commented Oct 7, 2021

Not really sure what was wrong with the original code and why it did not work, but I could reproduce it consistently. This change simplifies typing.py and it passes my local tests. I have tried mypy with following combinations (results in the comments):

from werkzeug.exceptions import HTTPException

class MyHTTPException(HTTPException): pass


# PASS - base case
@app.errorhandler(Exception)
def handle_any_exception(e: Exception) -> tuple[str, int]:
    return "generic error", 500


# PASS - non-base exception, using werkzeug.exceptions.HTTPExceptions as example
@app.errorhandler(HTTPException)
def handle_http_exception(e: HTTPException) -> ResponseReturnValue:
    log.exception("unknown error", exc_info=e)
    return "ERROR", e.code


# PASS - handling more specific exception, accepting more general one as parameter, should work
@app.errorhandler(HTTPException)
def handle_http_exception_lower_type(e: Exception) -> ResponseReturnValue:
    return str(e), 500


# PASS - custom defined exception in error handler, base exception as parameter
@app.errorhandler(MyHTTPException)
def handle_my_http_exception(e: HTTPException) -> ResponseReturnValue:
    return "error", e.code


# FAIL - since handler is accepting broader exception than declared in errorhandler parameter
@app.errorhandler(Exception)
def handle_any_exception_wrong_parameter(e: HTTPException) -> ResponseReturnValue:
    return str(e), e.code


# FAIL - unrelated exceptions
@app.errorhandler(KeyError)
def handler_value_error(e: ValueError) -> tuple[str, int]:
    return "value error", 500


# PASS - works as before, no changes
@app.errorhandler(404)
def handle_404(error: Exception) -> tuple[str, int]:
    return "not found", 404

Equivalent calls to app.register_error_handler behave the same as a decorator usage, as expected.

I did not find any tests of framework to tests typing, that is why I performed manually local testing. If I should change the code to test this change, please do let me know.

Please let me know if I should have done something different regarding CHANGES.rst.
Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@delicb delicb mentioned this pull request Oct 7, 2021
6 tasks
@davidism davidism linked an issue Oct 7, 2021 that may be closed by this pull request
@lucaswerkmeister
Copy link
Contributor

This also fixes a regression I’ve observed, for Python code like the following:

import flask
import pymysql.err  # type: ignore
from typing import Tuple

app = flask.Flask(__name__)

@app.errorhandler(pymysql.error.OperationalError)
def h(e: pymysql.error.OperationalError) -> Tuple[str, int]:
    return '', 503
Version details

Frozen requirements.txt for Flask 2.0.1 (passes):

click==8.0.3
Flask==2.0.1
itsdangerous==2.0.1
Jinja2==3.0.2
MarkupSafe==2.0.1
mypy==0.910
mypy-extensions==0.4.3
toml==0.10.2
typing-extensions==3.10.0.2
Werkzeug==2.0.2

Frozen requirements.txt for Flask 2.0.2 (regressed):

click==8.0.3
Flask==2.0.2
itsdangerous==2.0.1
Jinja2==3.0.2
MarkupSafe==2.0.1
mypy==0.910
mypy-extensions==0.4.3
PyMySQL==1.0.2
toml==0.10.2
typing-extensions==3.10.0.2
Werkzeug==2.0.2

Frozen requirements.txt for this pull request (fixed):

click==8.0.3
git+https://github.com/delicb/flask@7e12132ae19a44a2d6b6eff5ef21b63053a9d46e
itsdangerous==2.0.1
Jinja2==3.0.2
MarkupSafe==2.0.1
mypy==0.910
mypy-extensions==0.4.3
PyMySQL==1.0.2
toml==0.10.2
typing-extensions==3.10.0.2
Werkzeug==2.0.2

@davidism davidism added this to the 2.0.3 milestone Nov 15, 2021
@davidism davidism merged commit b831e85 into pallets:2.0.x Nov 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 30, 2021
@davidism
Copy link
Member

In #4487, I relaxed the type for the callable argument to Any.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

error handler type check fails

3 participants