Raise DB-API errors for transport failures and empty access tokens - #961
Open
aminghadersohi wants to merge 1 commit into
Open
aminghadersohi wants to merge 1 commit into
aminghadersohi wants to merge 1 commit into
Conversation
- ThriftDatabricksClient.make_request re-raised urllib3 HTTP errors of every request except GetOperationStatus unchanged, so an unreachable workspace (refused connection, proxy/tunnel failure) surfaced from connect() and execute() as a raw urllib3 MaxRetryError instead of a DB-API error. Report it through the normal non-retryable path as a RequestError (OperationalError). - connect(access_token="") dropped the empty token and fell back to the interactive browser OAuth flow, blocking on a local callback server. An explicitly empty token now raises 'No valid authentication settings!' at once. Signed-off-by: Amin Ghadersohi <amin.ghadersohi@gmail.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Empty-token handling remains inconsistent across OAuth and kernel/recovery paths.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This pull request improves DB-API transport error handling and prevents explicitly empty access tokens from silently starting OAuth login flows.
Changes:
- Wraps transport failures as
RequestError. - Preserves and rejects explicitly empty access tokens.
- Adds regression tests for both behaviors.
| File | Summary |
|---|---|
tests/unit/test_thrift_backend.py |
Tests transport-error wrapping. |
tests/unit/test_auth.py |
Tests empty-token handling. |
src/databricks/sql/client.py |
Preserves explicitly passed tokens. |
src/databricks/sql/backend/thrift_backend.py |
Routes transport failures through DB-API handling. |
src/databricks/sql/auth/auth.py |
Rejects empty access tokens. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| import thrift, errno | ||
| from databricks.sql.thrift_api.TCLIService.TCLIService import Client | ||
| from databricks.sql.exc import RequestError | ||
| from databricks.sql.exc import RequestError, RequestError |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problems (observed with 4.4.0 and 4.6.0 against a SQL warehouse)
ThriftDatabricksClient.make_requestre-raises theurllib3HTTP error of every request exceptGetOperationStatusunchanged. Soconnect()andcursor.execute()raiseurllib3.exceptions.MaxRetryError, which is not a PEP 249 error. SQLAlchemy therefore does not wrap it inDBAPIError, and callers that handle database errors treat it as an unexpected crash.connect(access_token="")silently drops the empty token (if access_token:) and falls back to the default interactive U2M OAuth flow. That flow binds a callback server on 127.0.0.1:8020–8024 and blocks until a browser redirect arrives, which on a server means indefinitely. Applications that clear a stale token to trigger their own OAuth flow (and expect theNo valid authentication settings!error) hang instead.Change
GetOperationStatusurllib3 HTTP errors take the normal non-retryable error path, so they surface asRequestError(OperationalError) with the usual context. urllib3 has already applied its retry policy by then.access_tokenis kept, andget_auth_providerraisesRuntimeError("No valid authentication settings! access_token is empty"). Passing no token at all keeps the documented default (databricks-oauth).Tests
test_make_request_wraps_urllib3_http_error_as_request_errorandtest_get_python_sql_connector_auth_provider_empty_access_token: both fail onmainand pass here.test_thrift_backend.py,test_auth.py,test_client.py,test_retry.pyandtest_session.py: 285 passed.access_token="".