feat: Add support for Trusted connections for MSSQL #57
Conversation
📝 WalkthroughWalkthroughThis PR adds support for MS SQL Server trusted connections (Windows Authentication). A new handler 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
📦 Python package built successfully!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/unit/test_sql_execution.py`:
- Around line 997-1009: In test_trusted_connection_false, after calling
_handle_mssql_trusted_connection(sql_alchemy_dict) add an assertion to verify
the flag is retained: assert that "mssql_trusted_connection" is present in
sql_alchemy_dict["params"] (i.e., self.assertIn("mssql_trusted_connection",
sql_alchemy_dict["params"])) so the test confirms the parameter was not removed
when False.
- Around line 1011-1027: The test lacks an assertion that the
mssql_trusted_connection flag remains in params when a warning is logged; update
the test_trusted_connection_non_mssql_url_warns test to assert that
"mssql_trusted_connection" is still present in sql_alchemy_dict["params"] after
calling _handle_mssql_trusted_connection, in addition to the existing warning
and URL-equality checks.
📜 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)
_handle_mssql_trusted_connection(355-384)
🪛 Ruff (0.14.11)
tests/unit/test_sql_execution.py
960-960: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
961-961: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
962-962: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
963-963: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
964-964: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
967-967: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
981-981: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
995-995: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1009-1009: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1027-1027: 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.10
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - 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.9
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.11
🔇 Additional comments (5)
deepnote_toolkit/sql/sql_execution.py (2)
120-120: LGTM!Correct placement after federated auth handling.
355-385: Clean implementation.Logic correctly removes credentials for Windows Authentication. The early returns keep it readable.
tests/unit/test_sql_execution.py (3)
948-967: Good coverage of the happy path.Consider adding explicit port check (
self.assertIn("1433", ...)) to verify port preservation.
969-981: LGTM!
983-995: LGTM!
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def test_trusted_connection_false(self): | ||
| """Test that no action is taken when mssql_trusted_connection is False.""" | ||
| from deepnote_toolkit.sql.sql_execution import _handle_mssql_trusted_connection | ||
|
|
||
| sql_alchemy_dict = { | ||
| "url": "mssql+pymssql://user:password@myserver/mydb", | ||
| "params": {"mssql_trusted_connection": False}, | ||
| } | ||
| original_url = sql_alchemy_dict["url"] | ||
|
|
||
| _handle_mssql_trusted_connection(sql_alchemy_dict) | ||
|
|
||
| self.assertEqual(sql_alchemy_dict["url"], original_url) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify flag retention.
When mssql_trusted_connection is False, the flag remains in params. Add assertion to confirm:
self.assertIn("mssql_trusted_connection", sql_alchemy_dict["params"])🧰 Tools
🪛 Ruff (0.14.11)
1009-1009: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
🤖 Prompt for AI Agents
In `@tests/unit/test_sql_execution.py` around lines 997 - 1009, In
test_trusted_connection_false, after calling
_handle_mssql_trusted_connection(sql_alchemy_dict) add an assertion to verify
the flag is retained: assert that "mssql_trusted_connection" is present in
sql_alchemy_dict["params"] (i.e., self.assertIn("mssql_trusted_connection",
sql_alchemy_dict["params"])) so the test confirms the parameter was not removed
when False.
| @mock.patch("deepnote_toolkit.sql.sql_execution.logger") | ||
| def test_trusted_connection_non_mssql_url_warns(self, mock_logger): | ||
| """Test that a warning is logged for non-mssql URLs.""" | ||
| from deepnote_toolkit.sql.sql_execution import _handle_mssql_trusted_connection | ||
|
|
||
| sql_alchemy_dict = { | ||
| "url": "postgresql://user:password@localhost/db", | ||
| "params": {"mssql_trusted_connection": True}, | ||
| } | ||
| original_url = sql_alchemy_dict["url"] | ||
|
|
||
| _handle_mssql_trusted_connection(sql_alchemy_dict) | ||
|
|
||
| mock_logger.warning.assert_called_once_with( | ||
| "mssql_trusted_connection is only supported for MS SQL Server connections" | ||
| ) | ||
| self.assertEqual(sql_alchemy_dict["url"], original_url) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing assertion for flag retention.
When warning triggers, flag should remain in params. Add:
self.assertIn("mssql_trusted_connection", sql_alchemy_dict["params"])🧰 Tools
🪛 Ruff (0.14.11)
1027-1027: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
🤖 Prompt for AI Agents
In `@tests/unit/test_sql_execution.py` around lines 1011 - 1027, The test lacks an
assertion that the mssql_trusted_connection flag remains in params when a
warning is logged; update the test_trusted_connection_non_mssql_url_warns test
to assert that "mssql_trusted_connection" is still present in
sql_alchemy_dict["params"] after calling _handle_mssql_trusted_connection, in
addition to the existing warning and URL-equality checks.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 73.27% 73.33% +0.06%
==========================================
Files 93 93
Lines 5193 5205 +12
Branches 757 759 +2
==========================================
+ Hits 3805 3817 +12
Misses 1144 1144
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. |
|
🚀 Review App Deployment Started
|
When
mssql_trusted_connection: trueis set in theconnection params, the handler removes the username and password from the URL, enabling Windows Authentication viapymssql:{ "url": "mssql+pymssql://user:pass@server/database", "params": { "mssql_trusted_connection": true } }Transforms to:
mssql+pymssql://@server/databaseFixes #56
Reference: sqlalchemy/sqlalchemy#11425