fix(ibis): Restore support for Snowflake password authentication#1236
fix(ibis): Restore support for Snowflake password authentication#1236goldmedal merged 3 commits intoCanner:mainfrom
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThe changes introduce optional password-based authentication for Snowflake connections in addition to the existing private key method. The Snowflake connection info model is updated to allow optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DataSource
participant Snowflake
Client->>DataSource: Request connection (with info)
alt info has private_key
DataSource->>Snowflake: Connect using private_key
else info has password
DataSource->>Snowflake: Connect using password
end
Snowflake-->>DataSource: Connection established
DataSource-->>Client: Return connection/result
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
douenergy
force-pushed
the
snowflake-password
branch
from
df23ded to
ce58328
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ibis-server/app/model/data_source.py (1)
227-227: Address static analysis hints for cleaner code.The static analysis tools correctly identified opportunities for improvement:
- Remove unnecessary
elseafterreturn- Use
{}instead ofdict()callsAlso applies to: 238-238
ibis-server/tests/routers/v2/connector/test_snowflake.py (1)
121-156: Consider adding edge case tests for authentication.While the main password authentication test is comprehensive, consider adding tests for edge cases such as:
- Missing password when private key is also missing
- Invalid password scenarios
- Mixed authentication parameters
Would you like me to generate additional test cases to cover these authentication edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ibis-server/app/model/__init__.py(2 hunks)ibis-server/app/model/data_source.py(1 hunks)ibis-server/tests/routers/v2/connector/test_snowflake.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: goldmedal
PR: Canner/wren-engine#1053
File: ibis-server/app/model/__init__.py:187-196
Timestamp: 2025-02-04T12:08:44.141Z
Learning: In GcsFileConnectionInfo, both secret-based (key_id/secret_key) and credentials-based (service account) authentication methods are required fields.
🪛 Pylint (3.3.7)
ibis-server/app/model/data_source.py
[refactor] 221-239: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 229-229: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 238-238: Consider using '{}' instead of a call to 'dict'.
(R1735)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (4)
ibis-server/app/model/__init__.py (2)
321-323: LGTM! Password field correctly made optional for dual authentication support.The change to make the
passwordfield optional withSecretStr | Noneand defaultNonecorrectly supports both password-based and private key authentication methods.
335-339: LGTM! Warehouse field appropriately made optional.Making the
warehousefield optional aligns with the authentication method branching, where password-based connections may not require specifying a warehouse.ibis-server/tests/routers/v2/connector/test_snowflake.py (2)
22-28: LGTM! Password connection info correctly configured.The password-based connection configuration appropriately uses environment variables and omits the warehouse field, aligning with the connection logic changes.
121-156: ```shell
#!/bin/bashLocate the test file path
echo "Locating test_snowflake.py:"
fd -e py test_snowflake.pyShow snippet around private key authentication test
echo -e "\nPrivate key test snippet:"
rg -n -C5 "private_key_connection_info" ibis-server/tests/routers/v2/connector/test_snowflake.py || echo "No private_key fixture found"Show all test_query_with_ occurrences for context
echo -e "\nAll test_query_with_ occurrences:"
rg -n "test_query_with_" ibis-server/tests/routers/v2/connector/test_snowflake.py</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| if hasattr(info, "private_key") and info.private_key: | ||
| return ibis.snowflake.connect( | ||
| user=info.user.get_secret_value(), | ||
| account=info.account.get_secret_value(), | ||
| database=info.database.get_secret_value(), | ||
| schema=info.sf_schema.get_secret_value(), | ||
| warehouse=info.warehouse.get_secret_value(), | ||
| private_key=info.private_key.get_secret_value(), | ||
| **info.kwargs if info.kwargs else dict(), | ||
| ) | ||
| else: | ||
| return ibis.snowflake.connect( | ||
| user=info.user.get_secret_value(), | ||
| password=info.password.get_secret_value(), | ||
| account=info.account.get_secret_value(), | ||
| database=info.database.get_secret_value(), | ||
| schema=info.sf_schema.get_secret_value(), | ||
| **info.kwargs if info.kwargs else dict(), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify authentication credential validation and improve code style.
The authentication branching logic is sound, but consider these improvements:
- Add explicit validation for missing credentials to provide clearer error messages
- Simplify the else clause and improve dictionary usage per static analysis hints
Apply this diff to improve the implementation:
@staticmethod
def get_snowflake_connection(info: SnowflakeConnectionInfo) -> BaseBackend:
- if hasattr(info, "private_key") and info.private_key:
+ if hasattr(info, "private_key") and info.private_key:
return ibis.snowflake.connect(
user=info.user.get_secret_value(),
account=info.account.get_secret_value(),
database=info.database.get_secret_value(),
schema=info.sf_schema.get_secret_value(),
warehouse=info.warehouse.get_secret_value(),
private_key=info.private_key.get_secret_value(),
- **info.kwargs if info.kwargs else dict(),
+ **info.kwargs if info.kwargs else {},
)
- else:
+
+ if not info.password:
+ raise ValueError("Either private_key or password must be provided for Snowflake authentication")
+
+ return ibis.snowflake.connect(
+ user=info.user.get_secret_value(),
+ password=info.password.get_secret_value(),
+ account=info.account.get_secret_value(),
+ database=info.database.get_secret_value(),
+ schema=info.sf_schema.get_secret_value(),
+ **info.kwargs if info.kwargs else {},
+ )
- return ibis.snowflake.connect(
- user=info.user.get_secret_value(),
- password=info.password.get_secret_value(),
- account=info.account.get_secret_value(),
- database=info.database.get_secret_value(),
- schema=info.sf_schema.get_secret_value(),
- **info.kwargs if info.kwargs else dict(),
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if hasattr(info, "private_key") and info.private_key: | |
| return ibis.snowflake.connect( | |
| user=info.user.get_secret_value(), | |
| account=info.account.get_secret_value(), | |
| database=info.database.get_secret_value(), | |
| schema=info.sf_schema.get_secret_value(), | |
| warehouse=info.warehouse.get_secret_value(), | |
| private_key=info.private_key.get_secret_value(), | |
| **info.kwargs if info.kwargs else dict(), | |
| ) | |
| else: | |
| return ibis.snowflake.connect( | |
| user=info.user.get_secret_value(), | |
| password=info.password.get_secret_value(), | |
| account=info.account.get_secret_value(), | |
| database=info.database.get_secret_value(), | |
| schema=info.sf_schema.get_secret_value(), | |
| **info.kwargs if info.kwargs else dict(), | |
| ) | |
| @staticmethod | |
| def get_snowflake_connection(info: SnowflakeConnectionInfo) -> BaseBackend: | |
| if hasattr(info, "private_key") and info.private_key: | |
| return ibis.snowflake.connect( | |
| user=info.user.get_secret_value(), | |
| account=info.account.get_secret_value(), | |
| database=info.database.get_secret_value(), | |
| schema=info.sf_schema.get_secret_value(), | |
| warehouse=info.warehouse.get_secret_value(), | |
| private_key=info.private_key.get_secret_value(), | |
| **info.kwargs if info.kwargs else {}, | |
| ) | |
| if not info.password: | |
| raise ValueError( | |
| "Either private_key or password must be provided for Snowflake authentication" | |
| ) | |
| return ibis.snowflake.connect( | |
| user=info.user.get_secret_value(), | |
| password=info.password.get_secret_value(), | |
| account=info.account.get_secret_value(), | |
| database=info.database.get_secret_value(), | |
| schema=info.sf_schema.get_secret_value(), | |
| **info.kwargs if info.kwargs else {}, | |
| ) |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 221-239: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 229-229: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 238-238: Consider using '{}' instead of a call to 'dict'.
(R1735)
🤖 Prompt for AI Agents
In ibis-server/app/model/data_source.py around lines 221 to 239, add explicit
checks to validate that required authentication credentials (like user,
password, or private_key) are present before attempting connection, raising
clear errors if missing. Simplify the else clause by using a single dictionary
unpacking with a default empty dict for info.kwargs, avoiding redundant
conditional expressions to improve code clarity and comply with static analysis
recommendations.
2 participants
We will support password login until Snowflake sunsets it.
related to #1230
Summary by CodeRabbit
New Features
Bug Fixes
Tests