codeant-ai-for-open-source[bot] commented on code in PR #41421:
URL: https://github.com/apache/superset/pull/41421#discussion_r3484725005
##########
superset/db_engine_specs/databricks.py:
##########
@@ -277,6 +284,135 @@ class
DatabricksDynamicBaseEngineSpec(BasicParametersMixin, DatabricksBaseEngine
"port": "port",
}
+ # The Databricks SQL driver has no dedicated authentication exception, so
an
+ # expired or missing token surfaces as a generic driver error. These case-
+ # insensitive substrings flag the errors that should bootstrap a re-auth.
+ oauth2_auth_failure_signals = (
+ "401",
+ "unauthorized",
+ "unauthenticated",
+ "invalid access token",
+ "invalid token",
+ "expired token",
+ "token expired",
Review Comment:
**Suggestion:** Matching the bare substring `401` is too broad and will
misclassify unrelated driver errors as authentication failures (for example
line numbers, IDs, or payload values containing 401), causing unnecessary
OAuth2 redirects and masking real query errors. Tighten this signal to an
auth-specific pattern such as `401 unauthorized`/`http 401` (or structured
exception metadata) instead of any occurrence of `401`. [incorrect condition
logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Non-auth Databricks errors can incorrectly trigger OAuth2 re-auth.
- ⚠️ Real connection failures may be masked as auth issues.
- ⚠️ Database test connection may misreport auth instead of error.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the Databricks OAuth2 error-signalling configuration in
`superset/db_engine_specs/databricks.py` lines 18-29, where
`DatabricksDynamicBaseEngineSpec.oauth2_auth_failure_signals` includes the
bare substring
`"401"` as its first entry.
2. Inspect the Databricks OAuth2 detection logic in
`superset/db_engine_specs/databricks.py` lines 50-64: `needs_oauth2` ensures
a user is
present in `flask.g`, then checks `if isinstance(ex, cls.oauth2_exception)`
and finally
lowercases the exception message and returns `any(signal in message for
signal in
cls.oauth2_auth_failure_signals)`.
3. In a unit-style reproduction (mirroring
`test_needs_oauth2_detects_auth_failure_from_message` in
`tests/unit_tests/db_engine_specs/test_databricks.py` lines 21-33), patch
`superset.db_engine_specs.databricks.g` with a `user` attribute, and call
`DatabricksNativeEngineSpec.needs_oauth2(Exception("Query failed at line
401: syntax
error"))`. The message is not an authentication error but contains the
substring `"401"`.
4. During this call, `isinstance(ex, cls.oauth2_exception)` is `False`, but
`message.lower()` contains `"401"`, which matches the first entry in
`oauth2_auth_failure_signals`, so `needs_oauth2` returns `True`. Upstream
callers like
`check_for_oauth2` in `superset/utils/oauth2.py` lines 311-319 and the
database test
connection logic in `superset/commands/database/test_connection.py` lines
156-163 and
245-248 treat this as an OAuth2 auth failure and invoke `start_oauth2_dance`,
misclassifying a non-auth Databricks error as requiring OAuth2
re-authentication.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ef1261c57b3c46b69197ea4077abf763&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ef1261c57b3c46b69197ea4077abf763&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/databricks.py
**Line:** 290:297
**Comment:**
*Incorrect Condition Logic: Matching the bare substring `401` is too
broad and will misclassify unrelated driver errors as authentication failures
(for example line numbers, IDs, or payload values containing 401), causing
unnecessary OAuth2 redirects and masking real query errors. Tighten this signal
to an auth-specific pattern such as `401 unauthorized`/`http 401` (or
structured exception metadata) instead of any occurrence of `401`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41421&comment_hash=479411ae6c27709f1e9fb97f06fc9e0f43e6b9611c9f3b87000ee97ceab10f8a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41421&comment_hash=479411ae6c27709f1e9fb97f06fc9e0f43e6b9611c9f3b87000ee97ceab10f8a&reaction=dislike'>👎</a>
--
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]
