-
Notifications
You must be signed in to change notification settings - Fork 1.7k
added connect_args to create_engine method in sqlalchemy #6080
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
closes #6072 |
Greptile OverviewGreptile SummaryThis PR adds support for custom SQLAlchemy connection arguments through a new Changes MadeConfiguration (
Database Engine (
Testing (
Implementation QualityThe implementation is solid with proper handling of edge cases:
Minor ImprovementThe Config class docstring could be updated to mention Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Config
participant Model
participant SQLAlchemy
User->>Config: Set connect_args in rxconfig.py
Note over Config: connect_args = {"application_name": "myapp"}
User->>Model: Request database connection
Model->>Config: get_config()
Config-->>Model: Returns config with connect_args
Model->>Model: get_engine_args(url)
Note over Model: Copy connect_args from config
alt URL is SQLite
Model->>Model: Add check_same_thread: False
Note over Model: Required for admin dashboard
end
Model->>Model: Merge connect_args into kwargs
Model->>SQLAlchemy: create_engine(url, **kwargs)
Note over SQLAlchemy: Receives custom connect_args
SQLAlchemy-->>Model: Returns engine with custom settings
Model-->>User: Database connection ready
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
Additional Comments (1)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: reflex/config.py
Line: 318:318
Comment:
The new `connect_args` field should be documented in the "Key Configuration Areas" section. This field allows users to pass custom connection arguments to SQLAlchemy's create_engine, which is important for advanced database configurations like Snowflake private key authentication.
```suggestion
- **Database**: `db_url`, `async_db_url`, `connect_args`, `redis_url`
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
masenf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the team does not want to take the patch at this time to avoid adding yet another configuration knob for a special case which expands API surface area and testing matrix.
for custom behavior, the current recommendation is to implement your own rx.session-like helper in your app that creates the sqlalchemy connection and use that in your app.
in this particular case, i agree it would be nicer if that didn't involve also reimplementing get_engine and get_engine_args. there's also the wrinkle of how deeply integrated the db subcommands and alembic are in the framework. we're actually moving away from the reflex db * commands and will be recommending the direct use of alembic by end users for managing db migrations prior to our 1.0 release.
Feature added
Added connect_args to rx.Config to allow passing custom arguments to SQLAlchemy create_engine.
Implemented steps
Need for change
Allows supporting private key authentication for Snowflake and other advanced DB configurations