chore(ibis): add kwargs and security-related arguments for connection info#1205
chore(ibis): add kwargs and security-related arguments for connection info#1205douenergy merged 6 commits intoCanner:mainfrom
Conversation
|
""" WalkthroughThis change adds an optional Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 Pylint (3.3.7)ibis-server/app/model/__init__.py[convention] 234-234: Line too long (134/100) (C0301) [convention] 212-212: Missing class docstring (C0115) [refactor] 212-212: Too few public methods (1/2) (R0903) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ 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 (
|
goldmedal
changed the title
goldmedal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
ibis-server/app/model/data_source.py (3)
182-182: Improve consistency with other connection methods.The kwargs handling is correct, but for consistency with other methods, consider using
{}instead ofdict().- **info.kwargs if info.kwargs else dict(), + **(info.kwargs if info.kwargs else {}),
203-203: Improve consistency with other connection methods.Same consistency improvement as suggested for Postgres.
- **info.kwargs if info.kwargs else dict(), + **(info.kwargs if info.kwargs else {}),
215-215: Improve consistency with other connection methods.Same consistency improvement as suggested for other connection methods.
- **info.kwargs if info.kwargs else dict(), + **(info.kwargs if info.kwargs else {}),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ibis-server/app/model/__init__.py(4 hunks)ibis-server/app/model/data_source.py(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: ibis CI
ibis-server/app/model/data_source.py
[error] 1-1: Ruff formatting check failed. File would be reformatted. Run 'ruff --fix' to fix code style issues.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
ibis-server/app/model/__init__.py (4)
134-136: Type annotation is too restrictive for kwargs field.The
kwargsfield usesdict[str, str]which is too restrictive for database connection parameters that may include integers, booleans, and other types.
206-209: Type annotation is too restrictive for kwargs field.Same issue as with ClickHouse - the
dict[str, str]type annotation should be more flexible to accommodate various parameter types.
258-261: Type annotation is too restrictive for kwargs field.Same type annotation issue as other database connection classes - should use a more flexible type to accommodate non-string parameter values.
283-286: Type annotation is too restrictive for kwargs field.Consistent with other connection info classes, this should also use a more flexible type annotation for the kwargs field.
🧹 Nitpick comments (5)
ibis-server/app/model/__init__.py (1)
226-237: Oracle connection fields implementation looks good.The new Oracle connection fields (
sid,service_name,dsn) are well-designed with clear descriptions and proper precedence handling. The implementation correctly usesSecretStr | Nonefor security.However, consider breaking long description lines to improve readability:
- description="Unique name of an Oracle Instance, used to construct a DSN if provided.", + description=( + "Unique name of an Oracle Instance, used to construct a DSN if provided." + ),- description="Oracle service name, used to construct a DSN if provided. Only one of database and service_name should be provided.", + description=( + "Oracle service name, used to construct a DSN if provided. " + "Only one of database and service_name should be provided." + ),- description="An Oracle Data Source Name. If provided, overrides all other connection arguments except username and password.", + description=( + "An Oracle Data Source Name. If provided, overrides all other " + "connection arguments except username and password." + ),🧰 Tools
🪛 Pylint (3.3.7)
[convention] 232-232: Line too long (138/100)
(C0301)
[convention] 236-236: Line too long (134/100)
(C0301)
ibis-server/app/model/data_source.py (4)
138-138: Consider using empty dict literal for better readability.The logic is correct for safely unpacking kwargs, but consider using an empty dict literal for better readability and performance.
- **info.kwargs if info.kwargs else dict(), + **info.kwargs if info.kwargs else {},🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 138-138: Consider using '{}' instead of a call to 'dict'.
(R1735)
181-181: Consider using empty dict literal for better readability.Same improvement as with ClickHouse - use empty dict literal instead of
dict().- **info.kwargs if info.kwargs else dict(), + **info.kwargs if info.kwargs else {},🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 181-181: Consider using '{}' instead of a call to 'dict'.
(R1735)
207-207: Consider using empty dict literal for better readability.Consistent with other connection methods, use empty dict literal for the fallback.
- **info.kwargs if info.kwargs else dict(), + **info.kwargs if info.kwargs else {},🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 207-207: Consider using '{}' instead of a call to 'dict'.
(R1735)
219-219: Consider using empty dict literal for better readability.Same style improvement as other connection methods.
- **info.kwargs if info.kwargs else dict(), + **info.kwargs if info.kwargs else {},🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 219-219: Consider using '{}' instead of a call to 'dict'.
(R1735)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ibis-server/app/model/__init__.py(5 hunks)ibis-server/app/model/data_source.py(5 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
ibis-server/app/model/__init__.py
[convention] 232-232: Line too long (138/100)
(C0301)
[convention] 236-236: Line too long (134/100)
(C0301)
ibis-server/app/model/data_source.py
[refactor] 138-138: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 181-181: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 207-207: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 219-219: Consider using '{}' instead of a call to 'dict'.
(R1735)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (1)
ibis-server/app/model/data_source.py (1)
192-197: Oracle connection enhancement implemented correctly.The new Oracle connection parameters (
sid,service_name,dsn) are properly extracted and passed to the connection. The implementation correctly handles optional fields by checking for None values before extracting secret values.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
ibis-server/app/model/__init__.py (4)
134-136: Type annotation for kwargs field is too restrictive.This is consistent with previous feedback - the
dict[str, str]type annotation should bedict[str, Any]to accommodate various parameter types like integers for timeouts, booleans for flags, etc.
206-209: Type annotation for kwargs field is too restrictive.Same issue as other connection info classes -
dict[str, str]should bedict[str, Any]for flexibility.
257-260: Type annotation for kwargs field is too restrictive.Consistent with other connection info classes, this should use
dict[str, Any]instead ofdict[str, str].
282-285: Type annotation for kwargs field is too restrictive.Same type annotation issue - should be
dict[str, Any]for consistency and flexibility.
🧹 Nitpick comments (6)
ibis-server/app/model/__init__.py (1)
232-236: Consider shortening the field description to address line length.The description is comprehensive but exceeds the 100-character line limit flagged by static analysis.
Consider splitting the description:
- dsn: SecretStr | None = Field( - default=None, - description="An Oracle Data Source Name. If provided, overrides all other connection arguments except username and password.", - examples=["localhost:1521/orcl"], - ) + dsn: SecretStr | None = Field( + default=None, + description="An Oracle Data Source Name. If provided, overrides all other " + "connection arguments except username and password.", + examples=["localhost:1521/orcl"], + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 234-234: Line too long (134/100)
(C0301)
ibis-server/app/model/data_source.py (4)
138-138: Consider using empty dict literal for better performance.The static analysis tool suggests using
{}instead ofdict()for better performance and readability.- **info.kwargs if info.kwargs else dict(), + **info.kwargs if info.kwargs else {},🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 138-138: Consider using '{}' instead of a call to 'dict'.
(R1735)
181-181: Consider using empty dict literal for better performance.Same suggestion as for ClickHouse - use
{}instead ofdict().- **info.kwargs if info.kwargs else dict(), + **info.kwargs if info.kwargs else {},🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 181-181: Consider using '{}' instead of a call to 'dict'.
(R1735)
210-210: Consider using empty dict literal for better performance.Consistent with other connection methods - use
{}instead ofdict().- **info.kwargs if info.kwargs else dict(), + **info.kwargs if info.kwargs else {},🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 210-210: Consider using '{}' instead of a call to 'dict'.
(R1735)
222-222: Consider using empty dict literal for better performance.Same suggestion as other connection methods - use
{}instead ofdict().- **info.kwargs if info.kwargs else dict(), + **info.kwargs if info.kwargs else {},🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 222-222: Consider using '{}' instead of a call to 'dict'.
(R1735)
ibis-server/tests/routers/v3/connector/oracle/test_query.py (1)
137-160: Well-implemented test for DSN-based Oracle connection.The test correctly:
- Constructs DSN from individual connection parameters
- Uses only required fields (dsn, user, password) as documented
- Follows the same validation pattern as existing tests
- Provides coverage for the new DSN functionality
Consider adding a docstring to improve documentation:
+async def test_query_with_dsn(client, manifest_str, connection_info): + """Test Oracle connection using DSN format instead of individual parameters.""" -async def test_query_with_dsn(client, manifest_str, connection_info):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 137-137: Missing function or method docstring
(C0116)
[warning] 137-137: Redefining name 'manifest_str' from outer scope (line 71)
(W0621)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ibis-server/app/model/__init__.py(4 hunks)ibis-server/app/model/data_source.py(4 hunks)ibis-server/tests/routers/v3/connector/oracle/test_query.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ibis-server/tests/routers/v3/connector/oracle/test_query.py (3)
ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/oracle/test_function.py (1)
manifest_str(31-32)ibis-server/tests/routers/v3/connector/oracle/conftest.py (1)
connection_info(56-65)
🪛 Pylint (3.3.7)
ibis-server/tests/routers/v3/connector/oracle/test_query.py
[convention] 137-137: Missing function or method docstring
(C0116)
[warning] 137-137: Redefining name 'manifest_str' from outer scope (line 71)
(W0621)
ibis-server/app/model/__init__.py
[convention] 234-234: Line too long (134/100)
(C0301)
[convention] 212-212: Missing class docstring
(C0115)
[refactor] 212-212: Too few public methods (1/2)
(R0903)
ibis-server/app/model/data_source.py
[refactor] 138-138: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 181-181: Consider using '{}' instead of a call to 'dict'.
(R1735)
[convention] 185-185: Missing function or method docstring
(C0116)
[refactor] 210-210: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 222-222: Consider using '{}' instead of a call to 'dict'.
(R1735)
🔇 Additional comments (2)
ibis-server/app/model/__init__.py (1)
214-225: Oracle connection info enhancements look good.The addition of default values and the
dsnfield is well-implemented:
- Sensible defaults for host ("default"), port ("1521"), and database ("orcl")
- Clear documentation that DSN overrides other connection arguments except username/password
- Proper typing with
SecretStr | Noneibis-server/app/model/data_source.py (1)
186-200: Oracle DSN implementation is well-designed.The logic correctly prioritizes DSN when available and falls back to individual connection parameters. The implementation properly:
- Checks for DSN existence using
hasattrand value check- Passes through username and password as documented
- Maintains backward compatibility with existing connection parameters
ibis-server/app/model/__init__.py
Outdated
| examples=["localhost"], description="the hostname of your database" | ||
| examples=["localhost"], | ||
| description="the hostname of your database", | ||
| default="default", |
There was a problem hiding this comment.
I think the default host should be "localhost" ?
|
Thanks @goldmedal |
2 participants
Changed
Add the security-related arguments for the following data source:
dsnSummary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Tests