feat: Add support to refresh federated auth access token #46
Conversation
📝 WalkthroughWalkthroughAdds federated authentication support to SQL execution. Introduces Pydantic models IntegrationFederatedAuthParams and FederatedAuthResponseData, plus a helper _get_federated_auth_credentials to request federated access tokens. Adds handlers _handle_federated_auth_params and _handle_iam_params to apply federated and IAM credentials to sql_alchemy_dict in-place. _query_data_source is updated to call these handlers. HTTP responses now call raise_for_status(); invalid or unsupported federatedAuthParams are logged and left unmodified. Unit tests covering Trino, BigQuery, Snowflake, missing/invalid params, and unsupported integrations were added. Sequence Diagram(s)mermaid SQL->>Handler: inspect sql_alchemy_dict for federatedAuthParams Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
📦 Python package built successfully!
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 73.04% 73.13% +0.09%
==========================================
Files 93 93
Lines 5149 5193 +44
Branches 754 758 +4
==========================================
+ Hits 3761 3798 +37
- Misses 1144 1151 +7
Partials 244 244
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deepnote_toolkit/sql/sql_execution.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
deepnote_toolkit/sql/sql_execution.py
deepnote_toolkit/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
deepnote_toolkit/**/*.py: Use dictionary unpacking for headers: headers = {"Content-Type": "application/json", **auth_headers}
Use appropriate exception types, log errors with context, and handle Jupyter/IPython specific exceptions properly
Document functions and classes with docstrings
Files:
deepnote_toolkit/sql/sql_execution.py
🪛 Ruff (0.14.8)
deepnote_toolkit/sql/sql_execution.py
310-312: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
311-311: Logging statement uses f-string
(G004)
319-319: Cannot use match statement on Python 3.9 (syntax was added in Python 3.10)
(invalid-syntax)
328-330: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test - Python 3.12
- GitHub Check: Typecheck - 3.13
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.11
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Typecheck - 3.9
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.13
🔇 Additional comments (4)
deepnote_toolkit/sql/sql_execution.py (4)
10-13:Neverimport looks correct for version compatibility.Proper conditional import for Python 3.9 support.
48-56: Pydantic models defined correctly.Clean structure for federated auth validation.
321-323: Potential KeyError if nested keys don't exist.If
params,connect_args, orhttp_headersare missing, this will raise KeyError.Verify that callers always provide the expected structure, or add defensive checks:
sql_alchemy_dict.setdefault("params", {}).setdefault("connect_args", {}).setdefault("http_headers", {})["Authorization"] = f"Bearer {access_token}"
432-434: Clean refactor to helper functions.Inline IAM/federated logic is now properly encapsulated.
|
🚀 Review App Deployment Started
|
…ype hints - Refactored `_get_federated_auth_credentials` to return `FederatedAuthResponseData` instead of just the access token. - Updated `IntegrationFederatedAuthParams` to include `userPodAuthContextToken` and changed the handling of integration types. - Improved error logging for unsupported integration types. - Cleaned up imports and ensured consistent use of type hints.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
deepnote_toolkit/sql/sql_execution.py (2)
265-278: Still missing docstring and error handling.Per previous review, this function needs:
- A docstring describing purpose, parameters, and return value
response.raise_for_status()to fail fast on non-2xx responses- Error handling for
response.json()and validation failuresWithout these, silent failures will cause confusing downstream errors.
🔎 Proposed fix
def _get_federated_auth_credentials(integration_id: str, user_pod_auth_context_token: str) -> FederatedAuthResponseData: + """Fetch federated auth credentials from the userpod API. + + Args: + integration_id: The integration ID to fetch credentials for + user_pod_auth_context_token: Auth context token for the user pod + + Returns: + FederatedAuthResponseData containing access token and integration type + """ url = get_absolute_userpod_api_url( f"integrations/federated-auth-token/{integration_id}" ) # Add project credentials in detached mode headers = get_project_auth_headers() headers["UserPodAuthContextToken"] = user_pod_auth_context_token response = requests.post(url, timeout=10, headers=headers) + response.raise_for_status() data = FederatedAuthResponseData.model_validate(response.json()) return data
309-311: Uselogger.exceptionfor automatic traceback.Per static analysis and previous review, replace
logger.error(..., exc_info=e)withlogger.exception(...)for cleaner traceback inclusion.🔎 Proposed fix
except ValidationError as e: - logger.error( - "Invalid federated auth params, try updating toolkit version:", exc_info=e - ) + logger.exception("Invalid federated auth params, try updating toolkit version") return
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deepnote_toolkit/sql/sql_execution.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
deepnote_toolkit/sql/sql_execution.py
deepnote_toolkit/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
deepnote_toolkit/**/*.py: Use dictionary unpacking for headers: headers = {"Content-Type": "application/json", **auth_headers}
Use appropriate exception types, log errors with context, and handle Jupyter/IPython specific exceptions properly
Document functions and classes with docstrings
Files:
deepnote_toolkit/sql/sql_execution.py
🪛 Ruff (0.14.8)
deepnote_toolkit/sql/sql_execution.py
309-311: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.9
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Test - Python 3.10
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Typecheck - 3.13
🔇 Additional comments (3)
deepnote_toolkit/sql/sql_execution.py (3)
4-4: LGTM.Imports and model setup are clean. The camelCase field names in Pydantic models align with typical API contracts.
Also applies to: 8-8, 19-19, 39-49
281-295: LGTM.Docstring added and variable naming fixed per previous review.
429-431: LGTM.Clean refactoring to use the new helper functions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
deepnote_toolkit/sql/sql_execution.py (1)
309-311: Fix logging: useexc_info=Truenotexc_info=e.Should be
logger.exception(...)orlogger.error(..., exc_info=True).Based on static analysis hint TRY400.
🔎 Proposed fix
except ValidationError as e: - logger.error( - "Invalid federated auth params, try updating toolkit version:", exc_info=e - ) + logger.exception("Invalid federated auth params, try updating toolkit version") return
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deepnote_toolkit/sql/sql_execution.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
deepnote_toolkit/sql/sql_execution.py
deepnote_toolkit/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
deepnote_toolkit/**/*.py: Use dictionary unpacking for headers: headers = {"Content-Type": "application/json", **auth_headers}
Use appropriate exception types, log errors with context, and handle Jupyter/IPython specific exceptions properly
Document functions and classes with docstrings
Files:
deepnote_toolkit/sql/sql_execution.py
🪛 Ruff (0.14.8)
deepnote_toolkit/sql/sql_execution.py
309-311: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.10
- GitHub Check: Typecheck - 3.13
- GitHub Check: Test - Python 3.9
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.11
🔇 Additional comments (1)
deepnote_toolkit/sql/sql_execution.py (1)
429-431: Clean integration of credential handlers.The in-place mutation approach keeps the flow straightforward.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
deepnote_toolkit/sql/sql_execution.py (2)
317-319: Uselogger.exceptionfor automatic traceback capture.Per static analysis hint TRY400 and coding guidelines.
except ValidationError as e: - logger.error( - "Invalid federated auth params, try updating toolkit version:", exc_info=e - ) + logger.exception("Invalid federated auth params, try updating toolkit version") return
326-329: Missing KeyError protection for nested dict access.Lines 327-329 assume
sql_alchemy_dict["params"]["connect_args"]["http_headers"]exists. Will raiseKeyErrorif the dict structure is incomplete.🔎 Proposed fix
if federated_auth.integrationType == "trino": - sql_alchemy_dict["params"]["connect_args"]["http_headers"][ - "Authorization" - ] = f"Bearer {federated_auth.accessToken}" + params = sql_alchemy_dict.setdefault("params", {}) + connect_args = params.setdefault("connect_args", {}) + http_headers = connect_args.setdefault("http_headers", {}) + http_headers["Authorization"] = f"Bearer {federated_auth.accessToken}" elif federated_auth.integrationType == "big-query": - sql_alchemy_dict["params"]["access_token"] = federated_auth.accessToken + params = sql_alchemy_dict.setdefault("params", {}) + params["access_token"] = federated_auth.accessToken
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deepnote_toolkit/sql/sql_execution.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
deepnote_toolkit/sql/sql_execution.py
deepnote_toolkit/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
deepnote_toolkit/**/*.py: Use dictionary unpacking for headers: headers = {"Content-Type": "application/json", **auth_headers}
Use appropriate exception types, log errors with context, and handle Jupyter/IPython specific exceptions properly
Document functions and classes with docstrings
Files:
deepnote_toolkit/sql/sql_execution.py
🪛 Ruff (0.14.8)
deepnote_toolkit/sql/sql_execution.py
317-319: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Typecheck - 3.13
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.9
- GitHub Check: Test - Python 3.10
🔇 Additional comments (4)
deepnote_toolkit/sql/sql_execution.py (4)
4-4: LGTM: Imports and logger setup.Standard imports and logger initialization for new federated auth functionality.
Also applies to: 8-8, 19-19, 39-40
260-260: LGTM: HTTP error handling added.Good addition of
raise_for_status()for early failure detection.
289-304: LGTM: Clean IAM credential handling.Proper early return, clear logic, follows guidelines.
442-442: LGTM: Clean integration of credential handlers.Proper placement and ordering of new helper functions.
Also applies to: 444-444
…ption instead of logger.error
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
deepnote_toolkit/sql/sql_execution.py (1)
267-287: Add error handling for JSON decode and validation failures.
response.json()andmodel_validate()can raise exceptions without helpful context. Wrap them and logresponse.texton failure.🔎 Proposed fix
def _get_federated_auth_credentials( integration_id: str, user_pod_auth_context_token: str ) -> FederatedAuthResponseData: """Get federated auth credentials for the given integration ID and user pod auth context token.""" url = get_absolute_userpod_api_url( f"integrations/federated-auth-token/{integration_id}" ) # Add project credentials in detached mode headers = get_project_auth_headers() headers["UserPodAuthContextToken"] = user_pod_auth_context_token response = requests.post(url, timeout=10, headers=headers) response.raise_for_status() + try: - data = FederatedAuthResponseData.model_validate(response.json()) + data = FederatedAuthResponseData.model_validate(response.json()) + except (ValueError, ValidationError) as e: + logger.error( + "Failed to parse federated auth response from %s: %s. Response: %s", + url, + e, + response.text, + ) + raise return data
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deepnote_toolkit/sql/sql_execution.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
deepnote_toolkit/sql/sql_execution.py
deepnote_toolkit/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
deepnote_toolkit/**/*.py: Use dictionary unpacking for headers: headers = {"Content-Type": "application/json", **auth_headers}
Use appropriate exception types, log errors with context, and handle Jupyter/IPython specific exceptions properly
Document functions and classes with docstrings
Files:
deepnote_toolkit/sql/sql_execution.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.13
🔇 Additional comments (4)
deepnote_toolkit/sql/sql_execution.py (4)
4-4: LGTM: Clean imports.Standard library and third-party imports are appropriate for federated auth support.
Also applies to: 8-8, 19-19, 39-39
260-260: Good addition: raise_for_status.Fails fast on HTTP errors.
289-304: LGTM: Clean IAM handler.Early return, snake_case, and clear structure.
450-452: LGTM: Clean integration.Helper calls are well-placed and modify connection params before engine creation.
…r.exception - Changed the assertion in the TestFederatedAuth class to verify that an exception was logged instead of an error. - Updated the test to check for the correct logging of invalid federated auth parameters.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/unit/test_sql_execution.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
tests/unit/test_sql_execution.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name test files with
test_*.pypattern
Files:
tests/unit/test_sql_execution.py
🧬 Code graph analysis (1)
tests/unit/test_sql_execution.py (1)
deepnote_toolkit/sql/sql_execution.py (2)
FederatedAuthResponseData(47-49)_handle_federated_auth_params(306-348)
🪛 Ruff (0.14.8)
tests/unit/test_sql_execution.py
630-630: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
671-671: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
718-718: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
740-740: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
764-764: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
766-766: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
806-806: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Combine and Upload Coverage
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.10
…thub.com:deepnote/deepnote-toolkit into tomaskislan/support-federated-auth-token-refresh
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/unit/test_sql_execution.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
tests/unit/test_sql_execution.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name test files with
test_*.pypattern
Files:
tests/unit/test_sql_execution.py
🧬 Code graph analysis (1)
tests/unit/test_sql_execution.py (1)
deepnote_toolkit/sql/sql_execution.py (2)
FederatedAuthResponseData(47-49)_handle_federated_auth_params(306-348)
🪛 Ruff (0.14.8)
tests/unit/test_sql_execution.py
631-631: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
672-672: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
719-719: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
741-741: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
765-765: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
767-767: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
807-807: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
853-853: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
856-856: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
898-898: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
901-901: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test - Python 3.9
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.11
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.13
🔇 Additional comments (8)
tests/unit/test_sql_execution.py (8)
1-2: Good: addressed previous review feedback.Using
copy.deepcopy()instead of JSON round-trip as previously suggested.
591-634: LGTM.Trino test correctly verifies Authorization header update with Bearer token.
636-675: LGTM.BigQuery test correctly verifies
access_tokenparam update.
721-741: LGTM.Early return path tested correctly.
743-767: LGTM.Validation error path tested correctly.
769-807: LGTM.Unsupported integration type error logging verified.
809-856: Good: addresses past review feedback.KeyError coverage for Trino malformed structure now included.
858-901: Good: addresses past review feedback.KeyError coverage for BigQuery missing params now included.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/unit/test_sql_execution.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
tests/unit/test_sql_execution.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name test files with
test_*.pypattern
Files:
tests/unit/test_sql_execution.py
🧬 Code graph analysis (1)
tests/unit/test_sql_execution.py (1)
deepnote_toolkit/sql/sql_execution.py (2)
FederatedAuthResponseData(47-49)_handle_federated_auth_params(306-348)
🪛 Ruff (0.14.8)
tests/unit/test_sql_execution.py
631-631: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
672-672: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
719-719: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
741-741: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
765-765: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
767-767: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
807-807: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
853-853: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
856-856: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
898-898: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
901-901: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.9
🔇 Additional comments (1)
tests/unit/test_sql_execution.py (1)
2-2: LGTM - proper import for deepcopy.Correctly added and used throughout the test suite.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.