fix(big-query): Fix BigQuery federated auth token refresh#54
Conversation
📝 WalkthroughWalkthroughAuthentication parameter handling is reorganized in SQL execution. Federated authentication and IAM parameter processing now occur at the start of 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (.cursorrules)
Files:
**/test_*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
deepnote_toolkit/**/*.py📄 CodeRabbit inference engine (.cursorrules)
Files:
🧬 Code graph analysis (1)tests/unit/test_sql_execution.py (1)
🪛 Ruff (0.14.11)tests/unit/test_sql_execution.py636-636: Use a regular Replace (PT009) 637-637: Use a regular Replace (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)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting Comment |
|
📦 Python package built successfully!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
+ Coverage 73.14% 73.27% +0.13%
==========================================
Files 93 93
Lines 5194 5194
Branches 758 758
==========================================
+ Hits 3799 3806 +7
+ Misses 1151 1144 -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. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
deepnote_toolkit/sql/sql_execution.py (2)
337-343: TypeError not caught for BigQuery.If
sql_alchemy_dict["params"]isNone, line 339 raisesTypeError, notKeyError. The except block won't catch it.Proposed fix
elif federated_auth.integrationType == "big-query": try: + if sql_alchemy_dict.get("params") is None: + raise KeyError("params") sql_alchemy_dict["params"]["access_token"] = federated_auth.accessToken except KeyError: logger.exception( "Invalid federated auth params, try updating toolkit version" )
280-282: Use dictionary unpacking for headers instead of in-place mutation.
get_project_auth_headers()returns a fresh dict on each call, so the mutation is safe. However, follow the dictionary unpacking pattern from the coding guidelines:headers = { "UserPodAuthContextToken": user_pod_auth_context_token, **get_project_auth_headers(), }tests/unit/test_sql_execution.py (1)
905-948: Test doesn't cover TypeError case.Test
test_federated_auth_params_bigquery_missing_paramsvalidates whenparamskey is absent. But ifparamsisNone, production code raisesTypeError, notKeyError. Consider adding a test case for"params": None.
📜 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 (2)
deepnote_toolkit/sql/sql_execution.pytests/unit/test_sql_execution.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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.pydeepnote_toolkit/sql/sql_execution.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name test files with
test_*.pypattern
Files:
tests/unit/test_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
🧬 Code graph analysis (1)
tests/unit/test_sql_execution.py (1)
deepnote_toolkit/sql/sql_execution.py (1)
_get_federated_auth_credentials(271-290)
🪛 Ruff (0.14.11)
tests/unit/test_sql_execution.py
636-636: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
637-637: 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.11
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.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.13
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.12
🔇 Additional comments (2)
deepnote_toolkit/sql/sql_execution.py (1)
118-120: LGTM - handlers moved to correct position.Credential augmentation now completes before URL construction and SSH handling. Clean refactor.
tests/unit/test_sql_execution.py (1)
592-637: Good coverage of federated auth flow.Test validates URL construction, headers composition, status check, and response parsing. Solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
🚀 Review App Deployment Started
|
Summary by CodeRabbit
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.