aminghadersohi commented on code in PR #44666:
URL: https://github.com/apache/superset/pull/44666#discussion_r4113909520
##########
superset/views/error_handling.py:
##########
@@ -248,7 +248,15 @@ def refresh_csrf_token(ex: CSRFError) -> FlaskResponse:
@app.errorhandler(HTTPException)
def show_http_exception(ex: HTTPException) -> FlaskResponse:
- logger.warning("HTTPException", exc_info=True)
+ status = ex.code or 500
+ if status == 404:
+ # Unmatched URLs are a client condition, and scanner traffic makes
+ # them frequent; a traceback here only adds noise.
+ logger.debug("HTTPException: 404 %s", request.path)
+ elif status < 500:
+ logger.warning("HTTPException: %s", ex)
Review Comment:
Addressed in fe34027d8bc51b97d3191d7ae972612566ab7c56. Valid single-line
logging concern: lazy %s interpolation does not escape CR/LF. The 404 path uses
%r, and other client HTTP errors use %r on str(ex), escaping both strings
without changing response bodies or logging levels. Regression cases cover a
percent-encoded CR/LF path and a CR/LF exception description; 404 still
produces one DEBUG record with no traceback. Targeted pytest: 37 passed;
pre-commit passed, including mypy.
##########
tests/unit_tests/views/test_error_handling.py:
##########
@@ -293,6 +303,179 @@ def
test_html_accept_serves_branded_error_page_not_raw_json(self):
mock_send_file.assert_called_once()
+class TestShowHttpException:
+ """
+ Client-side HTTP errors are not server faults and must not log tracebacks.
+ """
+
+ def _build_app_with_handlers(self, error: HTTPException) -> Flask:
+ test_app = Flask(__name__)
+ test_app.config["DEBUG"] = False
+ Babel(test_app)
+ set_app_error_handlers(test_app)
+
+ @test_app.route("/http-error")
+ def http_error_view() -> FlaskResponse:
+ raise error
+
+ return test_app
+
+ @staticmethod
+ def _handler_records(
+ caplog: pytest.LogCaptureFixture,
+ ) -> list[logging.LogRecord]:
+ return [
+ record
+ for record in caplog.records
+ if record.name == "superset.views.error_handling"
+ ]
+
+ def test_routing_404_logs_no_warning_and_no_traceback(
Review Comment:
Addressed in fe34027d8bc51b97d3191d7ae972612566ab7c56. Added concise
scenario/expected-behavior docstrings to all seven test methods. Targeted
pytest: 37 passed; pre-commit passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
