aminghadersohi commented on code in PR #43479:
URL: https://github.com/apache/superset/pull/43479#discussion_r3872929612
##########
superset/daos/base.py:
##########
@@ -433,6 +443,11 @@ def find_by_ids(
try:
results = query.all()
+ except OperationalError:
+ # A transient connection-level failure (e.g. the server dropping
the
+ # connection mid-query) surfaces as OperationalError. Let it
propagate
+ # as a 5xx instead of masking it as a 400 "record doesn't exist".
+ raise
Review Comment:
> **Error-message content on the new 500 path** — worth confirming that
connection strings / hostnames inside an `OperationalError` can't leak through
this new clause on guest-token routes.
Checked, and the redaction does hold — but it holds indirectly, which is
exactly why it was worth asking. Tests added in 9d37390e so it can't silently
regress.
**Why it's safe.** The new clause returns
`json_error_response(utils.error_msg_from_exception(ex), status=500)`.
`error_msg_from_exception` finds no `.message` attribute on a SQLAlchemy
`DBAPIError`, so it falls through to `str(ex)` — the full driver text, which
for a connection failure does quote host, IP and port:
```
(psycopg2.OperationalError) could not connect to server: Connection refused
Is the server running on host "analytics-prod.internal" (10.0.4.17) and
accepting
TCP/IP connections on port 5432?
```
That string then reaches `json_error_response`, whose
`isinstance(error_details, str)` branch routes it through
`sanitize_error_message(error_details, status)`. For a guest that returns
`GENERIC_ERROR_MESSAGE` — 500 is not in `ACCESS_STATUSES`, so it takes the
data-error wording rather than the denial wording. The protection is inherited
from `json_error_response` and is identical to what the adjacent 422 clause
already relies on; the new clause introduces no path around it.
**What's now pinned down**
(`tests/unit_tests/views/test_error_handling.py`), driven through
`handle_api_exception` rather than through `json_error_response` directly, so
the assertion covers the real clause:
- `test_operational_error_message_is_redacted_for_guest_users` — a driver
message containing a hostname, an IP and a port returns 500 with
`GENERIC_ERROR_MESSAGE`, and asserts none of those three substrings appear
anywhere in the response body.
- `test_operational_error_message_is_kept_for_regular_users` — the redaction
is guest-only; an operator still gets the driver detail, which is the whole
point of the fix.
`logger.exception(ex)` keeps the unredacted message server-side, matching
every other clause in the decorator.
On your point 1 (analytic-DB routes): agreed it needs an explicit maintainer
call rather than an author assertion, so I've left it as an open question in
the PR body rather than treating it as settled. @eschutho — that one is a
judgement call on intended semantics, not something the tests can decide.
--
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]
