-
Notifications
You must be signed in to change notification settings - Fork 1
feat(backend): add sqlalchemy backend #20
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
Conversation
📝 WalkthroughWalkthroughAdds a new SQLAlchemy-backed DatabaseBackend and ORM models, multi-database config and types, optional DB dependency groups, example Postgres demo, docker-compose Postgres service, and updates app/backend exports and backend selection to support Postgres/MySQL/SQLite/MSSQL. Changes
Sequence Diagram(s)sequenceDiagram
actor App
participant Backend as DatabaseBackend
participant Engine as SQLAlchemy Engine
participant DB as Database
Note over App,DB: DatabaseBackend connect / store / retrieve / disconnect flows
rect rgb(220,235,255)
Note over App,Backend: Connect phase
App->>Backend: connect()
Backend->>Engine: create_async_engine(_get_engine_kwargs)
Engine->>DB: establish connection
Engine-->>Backend: engine ready
Backend->>Engine: create async sessionmaker
Backend->>DB: create_all(metadata)
Backend-->>App: connected
end
rect rgb(210,255,220)
Note over App,Backend: Store result
App->>Backend: store_result(task_id, result)
Backend->>Engine: acquire session
Engine->>DB: checkout connection
Backend->>DB: upsert TaskResultModel (INSERT/UPDATE)
DB-->>Backend: persisted
Engine->>DB: release connection
Backend-->>App: ack
end
rect rgb(255,245,220)
Note over App,Backend: Retrieve result
App->>Backend: get_result(task_id)
Backend->>Engine: acquire session
Engine->>DB: query TaskResultModel
DB-->>Backend: record
Backend->>Backend: deserialize JSON -> TaskResult
Engine->>DB: release connection
Backend-->>App: TaskResult | None
end
rect rgb(255,220,230)
Note over App,Backend: Disconnect
App->>Backend: disconnect()
Backend->>Engine: dispose()
Engine->>DB: close connections
Backend-->>App: disconnected
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)src/chicory/backend/__init__.py (1)
src/chicory/app.py (2)
⏰ 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). (12)
🔇 Additional comments (2)
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.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/chicory/app.py (1)
101-116: Consider consolidating the DatabaseBackend cases to reduce duplication.The four database backend cases share the same import statement. You could consolidate them while maintaining lazy loading:
🔎 Proposed refactor
case BackendType.POSTGRES: from chicory.backend import DatabaseBackend return DatabaseBackend(config=self.config.backend.postgres) - case BackendType.MYSQL: - from chicory.backend import DatabaseBackend - - return DatabaseBackend(config=self.config.backend.mysql) - case BackendType.SQLITE: - from chicory.backend import DatabaseBackend - - return DatabaseBackend(config=self.config.backend.sqlite) - case BackendType.MSSQL: - from chicory.backend import DatabaseBackend - - return DatabaseBackend(config=self.config.backend.mssql) + case BackendType.MYSQL: + from chicory.backend import DatabaseBackend + + return DatabaseBackend(config=self.config.backend.mysql) + case BackendType.SQLITE: + from chicory.backend import DatabaseBackend + + return DatabaseBackend(config=self.config.backend.sqlite) + case BackendType.MSSQL: + from chicory.backend import DatabaseBackend + + return DatabaseBackend(config=self.config.backend.mssql)Alternatively, for more significant consolidation:
case BackendType.POSTGRES | BackendType.MYSQL | BackendType.SQLITE | BackendType.MSSQL: from chicory.backend import DatabaseBackend config_map = { BackendType.POSTGRES: self.config.backend.postgres, BackendType.MYSQL: self.config.backend.mysql, BackendType.SQLITE: self.config.backend.sqlite, BackendType.MSSQL: self.config.backend.mssql, } return DatabaseBackend(config=config_map[backend_type])The current implementation is functionally correct; this is purely an optional cleanup for maintainability.
tests/unit/test_optional_imports.py (1)
138-178: Remove unused# noqa: E501directives.Static analysis indicates these
noqadirectives are unused becauseE501(line-too-long) is not enabled in the ruff configuration. Remove them to keep the code clean.🔎 Lines to update
Remove
# noqa: E501from lines: 138, 147, 156, 165, 171, 172, 175, 176, 177, 178For example:
- "from chicory import backend; print('DatabaseBackend' in backend.__all__)", # noqa: E501 + "from chicory import backend; print('DatabaseBackend' in backend.__all__)",src/chicory/config.py (1)
457-460: Security consideration:trust_server_certificatedefaults toTrue.Defaulting to
Truebypasses SSL certificate validation, which is insecure in production environments. Consider defaulting toFalseto enforce secure connections by default, requiring explicit opt-in for development scenarios.🔎 Suggested change
trust_server_certificate: bool = Field( - default=True, + default=False, description="Whether to trust the server certificate (for SSL connections)", )src/chicory/backend/models.py (1)
55-66: Consider adding an index onupdated_at.The
cleanup_old_resultsmethod indatabase.pyqueries byupdated_at < cutoff. Without an index, this could become slow with large tables.🔎 Proposed fix
updated_at: Mapped[datetime] = mapped_column( DateTime(timezone=True), nullable=False, default=lambda: datetime.now(UTC), onupdate=lambda: datetime.now(UTC), + index=True, )src/chicory/backend/database.py (1)
160-162: Unused session variable in connection test.The session is created but not used. Consider adding a simple query or using
_to clarify intent.🔎 Proposed fix
# Test connection - async with self._get_db_session_from_pool() as session: - pass + async with self._get_db_session_from_pool() as session: + await session.execute(select(1))examples/postgres_example.py (1)
316-330: Consider documenting that this is a workaround.Manually constructing
TaskMessageand publishing bypasses the standarddelay()API. If ETA support is planned for the public API, consider adding a comment or TODO noting this workaround can be simplified once that's available.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
docker-compose.ymlexamples/postgres_example.pypyproject.tomlsrc/chicory/__init__.pysrc/chicory/app.pysrc/chicory/backend/__init__.pysrc/chicory/backend/database.pysrc/chicory/backend/models.pysrc/chicory/config.pysrc/chicory/exceptions.pysrc/chicory/types.pytests/unit/test_optional_imports.py
🧰 Additional context used
🧬 Code graph analysis (6)
src/chicory/__init__.py (3)
src/chicory/app.py (1)
backend(127-128)tests/unit/test_worker.py (1)
backend(44-49)src/chicory/backend/database.py (1)
DatabaseBackend(40-342)
src/chicory/backend/__init__.py (1)
src/chicory/backend/database.py (1)
DatabaseBackend(40-342)
src/chicory/backend/models.py (2)
src/chicory/config.py (4)
MSSQLBackendConfig(436-524)MySQLBackendConfig(365-433)PostgresBackendConfig(294-362)SQLiteBackendConfig(527-590)src/chicory/result.py (1)
state(55-61)
examples/postgres_example.py (5)
src/chicory/result.py (2)
AsyncResult(13-71)get(28-53)src/chicory/types.py (7)
BackendType(239-244)BrokerType(234-236)DeliveryMode(24-26)RetryBackoff(36-41)RetryPolicy(44-120)ValidationMode(29-33)TaskMessage(123-147)src/chicory/app.py (6)
Chicory(26-170)broker(123-124)backend(127-128)task(130-152)connect(160-164)disconnect(166-170)src/chicory/context.py (4)
TaskContext(13-84)remaining_retries(79-84)retry(22-64)fail(66-68)src/chicory/task.py (2)
delay(108-130)send(132-139)
src/chicory/app.py (2)
src/chicory/types.py (1)
BackendType(239-244)src/chicory/backend/database.py (1)
DatabaseBackend(40-342)
src/chicory/backend/database.py (5)
src/chicory/config.py (11)
MSSQLBackendConfig(436-524)MySQLBackendConfig(365-433)PostgresBackendConfig(294-362)SQLiteBackendConfig(527-590)dsn(100-113)dsn(223-236)dsn(278-291)dsn(349-362)dsn(420-433)dsn(507-524)dsn(581-590)src/chicory/exceptions.py (1)
DbPoolExhaustedException(58-64)src/chicory/types.py (4)
BackendStatus(202-202)TaskResult(150-163)TaskState(15-21)WorkerStats(205-231)src/chicory/backend/base.py (1)
Backend(10-68)src/chicory/backend/models.py (2)
TaskResultModel(47-66)WorkerHeartbeatModel(69-86)
🪛 Ruff (0.14.10)
examples/postgres_example.py
110-110: Avoid specifying long messages outside the exception class
(TRY003)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
168-168: Avoid specifying long messages outside the exception class
(TRY003)
179-179: Unused function argument: scheduled_time
(ARG001)
231-231: Unused function argument: metadata
(ARG001)
259-259: Avoid specifying long messages outside the exception class
(TRY003)
src/chicory/config.py
497-497: Avoid specifying long messages outside the exception class
(TRY003)
577-577: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/test_optional_imports.py
138-138: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
147-147: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
156-156: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
165-165: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
171-171: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
172-172: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
175-175: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
177-177: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
178-178: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
src/chicory/backend/database.py
81-81: Avoid specifying long messages outside the exception class
(TRY003)
88-88: Avoid specifying long messages outside the exception class
(TRY003)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
161-161: Local variable session is assigned to but never used
Remove assignment to unused variable session
(F841)
301-301: Unused method argument: stale_seconds
(ARG002)
322-322: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (11)
- GitHub Check: run (windows-latest, 3.11)
- GitHub Check: run (macos-latest, 3.12)
- GitHub Check: run (macos-latest, 3.11)
- GitHub Check: run (macos-latest, 3.14)
- GitHub Check: run (windows-latest, 3.13)
- GitHub Check: run (windows-latest, 3.14)
- GitHub Check: run (ubuntu-latest, 3.14)
- GitHub Check: run (windows-latest, 3.12)
- GitHub Check: run (ubuntu-latest, 3.13)
- GitHub Check: run (ubuntu-latest, 3.11)
- GitHub Check: run (ubuntu-latest, 3.12)
🔇 Additional comments (13)
src/chicory/__init__.py (1)
85-90: LGTM!The guarded import pattern for
DatabaseBackendis consistent with the existingRedisBackendandRabbitMQBrokerpatterns, correctly exposing the backend only when optional dependencies are installed.src/chicory/backend/__init__.py (1)
15-20: LGTM!The guarded export of
DatabaseBackendfollows the same pattern asRedisBackend, ensuring the class is only exposed when the database dependencies are installed.src/chicory/types.py (1)
239-244: LGTM!The
BackendTypeenum is cleanly extended with the four new database backend types. The naming convention is consistent and the string values align with the optional dependency group names inpyproject.toml.pyproject.toml (1)
44-69: LGTM! Well-organized optional dependencies.The optional dependency groups are cleanly structured, with each database backend being self-contained. The
allgroup correctly includes all database drivers.Consider using group references in the
allgroup for easier maintenance:all = [ "chicory[redis]", "chicory[cli]", "chicory[rabbitmq]", "chicory[postgres]", "chicory[mysql]", "chicory[sqlite]", "chicory[mssql]", ]This would automatically include any future additions to individual groups. Verify that your build system supports this syntax.
tests/unit/test_optional_imports.py (1)
132-167: LGTM! Comprehensive test coverage for all database backends.The test matrix properly covers all four new database backends (postgres, mssql, mysql, sqlite), verifying both direct
DatabaseBackendimports and__all__membership.src/chicory/config.py (3)
294-362: LGTM!The PostgreSQL backend configuration is well-structured with appropriate defaults, DSN validation, and connection pooling parameters. The use of
postgresql+asyncpgscheme correctly targets the async driver.
365-433: LGTM!The MySQL backend configuration follows the same consistent pattern as PostgreSQL, with proper DSN validation and the correct
mysql+aiomysqlasync driver scheme.
662-665: LGTM!The new backend configuration fields are added consistently with the existing
redisfield pattern.src/chicory/backend/models.py (2)
29-44: LGTM!The metadata naming convention is a good practice for database migrations, ensuring consistent and predictable constraint names across different databases.
69-86: LGTM!The worker heartbeat model is well-designed with an appropriate index on
expires_atfor efficient TTL-based queries.src/chicory/backend/database.py (3)
95-132: LGTM!The engine configuration correctly handles database-specific requirements:
StaticPoolfor in-memory SQLite,check_same_thread=Falsefor SQLite async usage, and PostgreSQL'sapplication_namefor connection identification.
171-199: LGTM with note on pending TODO.The implementation correctly handles both existing and new records. The TODO comment (Lines 171-172) references PR #17 for a design decision about whether
set_stateshould only update existing records.
225-241: LGTM!The result retrieval handles missing records gracefully, and the delete operation is idempotent.
…add indexes to TaskResultModel and WorkerHeartbeatModel
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.