-
Notifications
You must be signed in to change notification settings - Fork 46.2k
feat(backend): add SQLAlchemy infrastructure for database operations #11419
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: dev
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
✅ Deploy Preview for auto-gpt-docs canceled.
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Here's the code health analysis summary for commits Analysis Summary
|
682f01f to
8ae5cbe
Compare
|
Thank you for this well-structured PR that adds SQLAlchemy infrastructure to the backend. The code looks well-designed with comprehensive test coverage and clear documentation. A few items to address before merging:
Overall, this is a well-structured foundation for the gradual migration from Prisma to SQLAlchemy. Once you address the checklist issue, this should be ready for merging. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| """ | ||
| prisma_url = Config().database_url | ||
|
|
||
| # Replace postgresql:// with postgresql+asyncpg:// | ||
| async_url = prisma_url.replace("postgresql://", "postgresql+asyncpg://") | ||
|
|
||
| # Remove ALL query parameters (schema, connect_timeout, etc.) | ||
| # We'll handle these through connect_args instead | ||
| async_url = re.sub(r"\?.*$", "", async_url) | ||
|
|
||
| return async_url |
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.
Bug: Empty DATABASE_URL with enable_sqlalchemy=true causes unhandled ArgumentError during startup.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
When enable_sqlalchemy=true is set and the DATABASE_URL environment variable is not provided, the Config().database_url defaults to an empty string. The get_database_url() function then attempts to create an engine with this empty URL, causing SQLAlchemy to raise an ArgumentError. This ArgumentError is not caught by the existing exception handlers in database.py or rest_api.py, leading to an unhandled exception and application crash during startup.
💡 Suggested Fix
Validate that database_url is not empty before calling create_async_engine(), or add ArgumentError to the list of caught exceptions, or set a sensible fallback for database_url.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: autogpt_platform/backend/backend/data/sqlalchemy.py#L31-L49
Potential issue: When `enable_sqlalchemy=true` is set and the `DATABASE_URL` environment
variable is not provided, the `Config().database_url` defaults to an empty string. The
`get_database_url()` function then attempts to create an engine with this empty URL,
causing SQLAlchemy to raise an `ArgumentError`. This `ArgumentError` is not caught by
the existing exception handlers in `database.py` or `rest_api.py`, leading to an
unhandled exception and application crash during startup.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2841823
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.
@Swiftyos this seems valid ?
| @@ -0,0 +1,233 @@ | |||
| """ | |||
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.
Let's make data_v2 or something equivalent to data folder for the sqlalchemy like sql or database or anything ?
The intention is to not mix sqlalchemy vs prisma code, and it will ease the migration by a simple switch, we will also know how fare are we from fully completing the migration by comparing the folder side-to-side.
| # ============================================================================ | ||
|
|
||
|
|
||
| def get_database_url() -> str: |
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.
Then this file can be a db.py similar to the one we have on data folder
| """ | ||
| prisma_url = Config().database_url | ||
|
|
||
| # Replace postgresql:// with postgresql+asyncpg:// | ||
| async_url = prisma_url.replace("postgresql://", "postgresql+asyncpg://") | ||
|
|
||
| # Remove ALL query parameters (schema, connect_timeout, etc.) | ||
| # We'll handle these through connect_args instead | ||
| async_url = re.sub(r"\?.*$", "", async_url) | ||
|
|
||
| return async_url |
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.
@Swiftyos this seems valid ?
| # ============================================================================ | ||
|
|
||
|
|
||
| def create_engine() -> AsyncEngine: |
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.
For starter, can we use this on scheduler.py and compare its current implementation in there (in case we miss something here or vice versa) ?
This way we are sure the change is working and tested on an already running important component (which we already has been using sqlalchemy).
| logger.info(f"[{self.service_name}] ⏳ Connecting to Database...") | ||
| await db.connect() | ||
|
|
||
| # Initialize SQLAlchemy if enabled (for gradual migration from Prisma) |
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.
This code can live in db.py (sqlalchemy.py) entirely, so we do the exact same thing as the above code, e.g: await (sql/database/whatever).db.connect()
| logger.info(f"[{self.service_name}] ⏳ Disconnecting Database...") | ||
|
|
||
| # Dispose SQLAlchemy if it was enabled | ||
| if config.enable_sqlalchemy: |
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.
same thing here. But I thought we have created a context/session_manager? why do we still do manual connect/disconnect here.
|
|
||
| await backend.data.db.connect() | ||
|
|
||
| # Initialize SQLAlchemy if enabled (for gradual migration from Prisma) |
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.
Is this replicated code as above?
|
You are above your monthly Qodo Merge usage quota. For more information, please visit here. |
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
Summary
Adds SQLAlchemy infrastructure to the backend as foundation for incrementally replacing Prisma for runtime database operations, while maintaining Prisma for migration generation.
Changes
Core Infrastructure
backend/data/sqlalchemy.py: Async SQLAlchemy engine with connection pooling
get_session()FastAPI dependencyinitialize(),dispose())backend/data/sqlalchemy_test.py: Comprehensive test suite
Configuration
backend/util/settings.py: SQLAlchemy settings
backend/.env.default: Default environment variables
Service Integration
Both services now initialize SQLAlchemy on startup and dispose on shutdown.
Dependencies
sqlalchemy[asyncio]andasyncpgTechnical Details
Connection Pool:
Schema Handling:
DATABASE_URLsearch_pathparameterSession Lifecycle:
Migration Approach
This PR establishes infrastructure only. Both Prisma and SQLAlchemy will coexist during incremental migration:
Testing
All tests passing with coverage of:
Breaking Changes
None - purely additive. Prisma continues to work unchanged.
Checklist 📋
For code changes: