feat: add async engine support for storage_engine#35
Merged
Conversation
Reviewer's GuideAdds support for both sync and async storage_engine by updating session creation and table management methods to detect AsyncEngine and run DDL operations in an async context via asyncio.run. Class diagram for updated Radar initialization and table managementclassDiagram
class Radar {
- storage_engine: Engine | AsyncEngine
- SessionLocal
- _is_async_storage: bool
+ create_tables()
+ drop_tables()
}
class Engine
class AsyncEngine
Radar --> Engine : uses as storage_engine
Radar --> AsyncEngine : uses as storage_engine
Radar --> SessionLocal : creates
class sessionmaker
class async_sessionmaker
Radar --> sessionmaker : uses for sync engine
Radar --> async_sessionmaker : uses for async engine
Flow diagram for session creation based on storage_engine typeflowchart TD
A["Radar.__init__"] --> B{Is storage_engine AsyncEngine?}
B -- Yes --> C["SessionLocal = async_sessionmaker(...)"]
B -- No --> D["SessionLocal = sessionmaker(...)"]
C --> E["_is_async_storage = True"]
D --> F["_is_async_storage = False"]
Flow diagram for create_tables and drop_tables supporting async enginesflowchart TD
A["create_tables() or drop_tables() called"] --> B{Is storage_engine AsyncEngine?}
B -- Yes --> C["Run DDL in async context using asyncio.run"]
B -- No --> D["Run DDL synchronously"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Avoid using asyncio.run inside FastAPI context since it will fail if there’s already a running loop; consider using the existing event loop or an async startup event to run DDL commands.
- The
_is_async_storageattribute is set but not used anywhere else—either leverage it in your logic or remove it to keep the code clean. - There’s duplicated async create/drop table logic; consider extracting a helper method to reduce redundancy and improve maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid using asyncio.run inside FastAPI context since it will fail if there’s already a running loop; consider using the existing event loop or an async startup event to run DDL commands.
- The `_is_async_storage` attribute is set but not used anywhere else—either leverage it in your logic or remove it to keep the code clean.
- There’s duplicated async create/drop table logic; consider extracting a helper method to reduce redundancy and improve maintainability.
## Individual Comments
### Comment 1
<location> `fastapi_radar/radar.py:395` </location>
<code_context>
+ async with self.storage_engine.begin() as conn:
+ await conn.run_sync(Base.metadata.create_all)
+
+ asyncio.run(_create_tables())
+ else:
+ Base.metadata.create_all(bind=self.storage_engine)
</code_context>
<issue_to_address>
**issue (bug_risk):** Using asyncio.run may cause issues if already inside an event loop.
If called from an async context, asyncio.run will raise RuntimeError. Consider checking for an active event loop or providing an async alternative.
</issue_to_address>
### Comment 2
<location> `fastapi_radar/radar.py:414` </location>
<code_context>
+ async with self.storage_engine.begin() as conn:
+ await conn.run_sync(Base.metadata.drop_all)
+
+ asyncio.run(_drop_tables())
+ else:
+ Base.metadata.drop_all(bind=self.storage_engine)
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential event loop conflict with asyncio.run in drop_tables.
Refactor to allow async usage or clearly document that this function cannot be called from within an active event loop.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="fastapi_radar/radar.py">
<violation number="1" location="fastapi_radar/radar.py:400">
Calling `asyncio.run(_create_tables())` will crash when `create_tables()` is triggered from within an active event loop (e.g., a FastAPI startup hook). This regresses legitimate usage because `asyncio.run` raises `RuntimeError` if the loop is already running.</violation>
<violation number="2" location="fastapi_radar/radar.py:419">
The new `asyncio.run(_drop_tables())` call will raise `RuntimeError` if `drop_tables()` is executed while an event loop is active (typical in async FastAPI lifecycle code), causing the operation to fail.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
Add support for SQLAlchemy AsyncEngine in storage_engine parameter. Update create_tables() and drop_tables() to handle both sync and async engines. Use async_sessionmaker for async engines, sessionmaker for sync engines. Maintain backward compatibility with existing sync engine usage. Fixes #32
Use sync_engine from AsyncEngine for session operations to keep the existing synchronous session usage pattern throughout the codebase (middleware, capture, api, etc.) This approach: - Uses AsyncEngine for DDL operations (create_tables, drop_tables) - Uses sync_engine for session operations (middleware, capture, etc.) - Maintains type safety and passes mypy checks
Detect if an event loop is already running (e.g., FastAPI startup hooks) and run async DDL operations in a thread pool to avoid RuntimeError. This ensures create_tables() and drop_tables() work correctly: - When called directly (no event loop) - When called from async context (active event loop) - When called from FastAPI startup hooks Fixes the RuntimeError regression for legitimate async usage patterns.
doganarif
force-pushed
the
support-async-storage-engine
branch
from
45f4ba7 to
604601e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add support for AsyncEngine in storage backend by adapting session creation and DDL operations for both synchronous and asynchronous SQLAlchemy engines
New Features:
Enhancements:
Summary by cubic
Add support for SQLAlchemy AsyncEngine in storage_engine to enable async databases while keeping existing sync setups working.
New Features
Bug Fixes
Written for commit 911dfb0. Summary will update automatically on new commits.