Deprecate should_ignore_error method
The `should_ignore_error()` method was added in f191809 to support
context preservation for debugging, but it no longer serves its
original purpose and adds unnecessary overhead.
Issues with the current implementation:
- Always returns False by default
- Called on every single request with no benefit
- The original intention for error ignoring during debugging is not
how context preservation works anymore
- No documentation beyond API reference
- No tests for the functionality
- No evidence of real-world usage
Changes:
- Add deprecation warning to App.should_ignore_error() that will be
removed in Flask 4.0
- Optimize call site to only invoke the method if it's been overridden
by a subclass, eliminating the function call overhead for 99.9% of
requests
- Add comprehensive tests for the deprecation behavior
- Update CHANGES.rst with deprecation notice
Teardown handlers should manage their own error handling instead of
relying on this method.
Fixes #5816
This commit is contained in:
parent
2579ce9f18
commit
04a5a04140
4 changed files with 103 additions and 2 deletions
|
|
@ -16,6 +16,9 @@ Unreleased
|
||||||
deprecation period. :issue:`5815`
|
deprecation period. :issue:`5815`
|
||||||
- ``template_filter``, ``template_test``, and ``template_global`` decorators
|
- ``template_filter``, ``template_test``, and ``template_global`` decorators
|
||||||
can be used without parentheses. :issue:`5729`
|
can be used without parentheses. :issue:`5729`
|
||||||
|
- Deprecated ``App.should_ignore_error()``. This method always returned ``False``
|
||||||
|
and added an unnecessary function call on every request. Teardown handlers should
|
||||||
|
manage their own error handling instead. :issue:`5816`
|
||||||
|
|
||||||
|
|
||||||
Version 3.1.2
|
Version 3.1.2
|
||||||
|
|
|
||||||
|
|
@ -1576,7 +1576,12 @@ class Flask(App):
|
||||||
if "werkzeug.debug.preserve_context" in environ:
|
if "werkzeug.debug.preserve_context" in environ:
|
||||||
environ["werkzeug.debug.preserve_context"](ctx)
|
environ["werkzeug.debug.preserve_context"](ctx)
|
||||||
|
|
||||||
if error is not None and self.should_ignore_error(error):
|
# Check if should_ignore_error is overridden (deprecated)
|
||||||
|
if (
|
||||||
|
error is not None
|
||||||
|
and type(self).should_ignore_error is not App.should_ignore_error
|
||||||
|
):
|
||||||
|
if self.should_ignore_error(error):
|
||||||
error = None
|
error = None
|
||||||
|
|
||||||
ctx.pop(error)
|
ctx.pop(error)
|
||||||
|
|
|
||||||
|
|
@ -928,8 +928,21 @@ class App(Scaffold):
|
||||||
function returns ``True`` then the teardown handlers will not be
|
function returns ``True`` then the teardown handlers will not be
|
||||||
passed the error.
|
passed the error.
|
||||||
|
|
||||||
|
.. deprecated:: 3.1
|
||||||
|
This method is deprecated and will be removed in Flask 4.0.
|
||||||
|
Teardown functions should manage their own error handling.
|
||||||
|
|
||||||
.. versionadded:: 0.10
|
.. versionadded:: 0.10
|
||||||
"""
|
"""
|
||||||
|
import warnings
|
||||||
|
|
||||||
|
warnings.warn(
|
||||||
|
"'should_ignore_error' is deprecated and will be removed in"
|
||||||
|
" Flask 4.0. Teardown functions should manage error handling"
|
||||||
|
" directly.",
|
||||||
|
DeprecationWarning,
|
||||||
|
stacklevel=2,
|
||||||
|
)
|
||||||
return False
|
return False
|
||||||
|
|
||||||
def redirect(self, location: str, code: int = 302) -> BaseResponse:
|
def redirect(self, location: str, code: int = 302) -> BaseResponse:
|
||||||
|
|
|
||||||
80
tests/test_deprecations.py
Normal file
80
tests/test_deprecations.py
Normal file
|
|
@ -0,0 +1,80 @@
|
||||||
|
"""Tests for deprecated features."""
|
||||||
|
|
||||||
|
import warnings
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
import flask
|
||||||
|
|
||||||
|
|
||||||
|
def test_should_ignore_error_deprecated():
|
||||||
|
"""Test that should_ignore_error emits a deprecation warning."""
|
||||||
|
app = flask.Flask(__name__)
|
||||||
|
|
||||||
|
with warnings.catch_warnings(record=True) as w:
|
||||||
|
warnings.simplefilter("always")
|
||||||
|
result = app.should_ignore_error(ValueError("test"))
|
||||||
|
|
||||||
|
assert len(w) == 1
|
||||||
|
assert issubclass(w[0].category, DeprecationWarning)
|
||||||
|
assert "should_ignore_error" in str(w[0].message)
|
||||||
|
assert "Flask 4.0" in str(w[0].message)
|
||||||
|
assert result is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_should_ignore_error_not_called_by_default():
|
||||||
|
"""Test that should_ignore_error is not called on normal requests."""
|
||||||
|
app = flask.Flask(__name__)
|
||||||
|
|
||||||
|
@app.route("/")
|
||||||
|
def index():
|
||||||
|
return "hello"
|
||||||
|
|
||||||
|
client = app.test_client()
|
||||||
|
|
||||||
|
with warnings.catch_warnings(record=True) as w:
|
||||||
|
warnings.simplefilter("always")
|
||||||
|
response = client.get("/")
|
||||||
|
|
||||||
|
# Should not have any deprecation warnings
|
||||||
|
dep_warnings = [x for x in w if issubclass(x.category, DeprecationWarning)]
|
||||||
|
assert len(dep_warnings) == 0
|
||||||
|
assert response.status_code == 200
|
||||||
|
|
||||||
|
|
||||||
|
def test_should_ignore_error_override_is_called():
|
||||||
|
"""Test that overriding should_ignore_error still works for backward compatibility."""
|
||||||
|
|
||||||
|
called = []
|
||||||
|
|
||||||
|
class MyApp(flask.Flask):
|
||||||
|
def should_ignore_error(self, error):
|
||||||
|
# Track that this was called and call parent to get deprecation warning
|
||||||
|
called.append(error)
|
||||||
|
super().should_ignore_error(error)
|
||||||
|
return False
|
||||||
|
|
||||||
|
app = MyApp(__name__)
|
||||||
|
|
||||||
|
@app.route("/")
|
||||||
|
def index():
|
||||||
|
raise ValueError("test error")
|
||||||
|
|
||||||
|
client = app.test_client()
|
||||||
|
|
||||||
|
with warnings.catch_warnings(record=True) as w:
|
||||||
|
warnings.simplefilter("always")
|
||||||
|
# Request will return 500 error page
|
||||||
|
response = client.get("/")
|
||||||
|
|
||||||
|
# Should have been called with the error
|
||||||
|
assert len(called) == 1
|
||||||
|
assert isinstance(called[0], ValueError)
|
||||||
|
|
||||||
|
# Should have deprecation warning
|
||||||
|
dep_warnings = [x for x in w if issubclass(x.category, DeprecationWarning)]
|
||||||
|
assert len(dep_warnings) > 0
|
||||||
|
assert any("should_ignore_error" in str(x.message) for x in dep_warnings)
|
||||||
|
|
||||||
|
# Should return internal server error
|
||||||
|
assert response.status_code == 500
|
||||||
Loading…
Add table
Add a link
Reference in a new issue