Fix isolate db sessions #150
Conversation
Reviewer's Guide by SourceryThis pull request focuses on improving database session management by implementing isolated database sessions. The changes involve refactoring the database connection handling, updating service and repository classes to use these isolated sessions, and modifying various parts of the application to work with the new session management approach. Sequence diagram for transaction creation with isolated sessionssequenceDiagram
actor User
participant API as API
participant Service as TransactionService
participant Repo as Repository
participant DB as Database
participant Session as SessionLocal
User->>API: Create Transaction
API->>Service: create_transaction(user, data)
Service->>Repo: save(transaction)
Repo->>Session: async with SessionLocal()
Session->>DB: Execute transaction
DB-->>Session: Transaction result
Session-->>Repo: Commit
Repo-->>Service: Transaction saved
Service-->>API: Transaction created
API-->>User: Transaction created response
Sequence diagram for user login process with UserServicesequenceDiagram
actor User
participant API as API
participant UserService as UserService
participant UserManager as UserManager
User->>API: Login request
API->>UserService: authenticate(credentials)
UserService->>UserManager: authenticate(credentials)
UserManager-->>UserService: User object
UserService-->>API: User object
API-->>User: Login success
Updated class diagram for database session managementclassDiagram
class Database {
-url: str
-engine: AsyncEngine
-_session: AsyncSession
+init()
+get_session() AsyncSession
+session() AsyncSession
}
class Repository {
-session: AsyncSession
+get_all()
+get()
+filter_by()
+filter_by_multiple()
+save()
+update()
+delete()
+refresh()
+refresh_all()
}
class SessionLocal {
<<async_sessionmaker>>
}
Database --> SessionLocal
Repository --> SessionLocal
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 @EduardSchwarzkopf - I've reviewed your changes - here's some feedback:
Overall Comments:
- These changes look good overall. Consider adding monitoring for database connection usage in production to ensure the new SessionLocal approach doesn't lead to connection exhaustion under high load.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -12,11 +11,10 @@ class BaseTransactionService(BaseService): | |||
| def __init__( | |||
| self, | |||
| service_model: Type[Union[models.Transaction, models.TransactionScheduled]], | |||
| repository: Optional[Repository] = None, | |||
There was a problem hiding this comment.
suggestion: Consider keeping repository as a parameter for better testability
While simplifying the constructor is good, allowing repository injection can make unit testing easier by enabling mocking of the repository.
| repository: Optional[Repository] = None, | |
| def __init__( | |
| self, | |
| service_model: Type[Union[models.Transaction, models.TransactionScheduled]], | |
| repository: Optional[Repository] = None, | |
| ): | |
| self.service_model = service_model | |
| self.repository = repository |
| @@ -60,7 +64,8 @@ async def fixture_user_service(): | |||
| UserService: The user service fixture. | |||
|
|
|||
| """ | |||
| yield UserService() | |||
| user_manager = await get_user_manager().__anext__() | |||
| yield UserService(user_manager) | |||
There was a problem hiding this comment.
suggestion (testing): Update user service fixture to use user manager
This change ensures that the user service is initialized with a user manager, which is more aligned with the actual implementation. Consider adding a test to verify that the user service is correctly initialized with the user manager.
| yield UserService(user_manager) | |
| @pytest.fixture(name="fixture_user_service") | |
| async def fixture_user_service(): | |
| """Fixture for UserService initialized with UserManager.""" | |
| user_manager = await anext(get_user_manager()) | |
| return UserService(user_manager) |
| wallet_list = await asyncio.gather(*create_task_list) | ||
|
|
||
| yield wallet_list | ||
| yield await asyncio.gather(*create_task_list) |
There was a problem hiding this comment.
issue (testing): Remove transaction context for fixture creation
The removal of the transaction context might affect the isolation of tests. Consider adding a cleanup step to ensure that any created objects are properly removed after the tests.
| engine = create_async_engine(settings.db_url, future=True) | ||
| TestSessionLocal = async_sessionmaker( | ||
| expire_on_commit=False, | ||
| bind=engine, | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Add test-specific database engine and session
This change introduces a separate database engine for tests, which is a good practice. Consider adding a fixture to provide this test session to individual tests that need direct database access.
test_engine = create_async_engine(settings.test_db_url, future=True)
TestSessionLocal = async_sessionmaker(
expire_on_commit=False,
bind=test_engine,
)
@pytest.fixture
async def test_db_session():
async with TestSessionLocal() as session:
yield session
| @@ -54,7 +50,8 @@ async def get_all( | |||
| """ | |||
| q = select(cls) | |||
| q = self._load_relationships(q, load_relationships_list) | |||
| result = await self.session.execute(q) | |||
| async with SessionLocal() as session: | |||
There was a problem hiding this comment.
issue (complexity): Consider implementing a helper method for session management.
While the changes introduce more explicit session management, they also add unnecessary repetition. Consider introducing a helper method to encapsulate the session creation and execution pattern. This will maintain explicit control over sessions while reducing code duplication. Here's an example:
from contextlib import asynccontextmanager
class Repository:
@asynccontextmanager
async def session_scope(self):
async with SessionLocal() as session:
try:
yield session
await session.commit()
except Exception:
await session.rollback()
raise
async def get_all(self, cls: Type[ModelT], load_relationships_list: Optional[list[InstrumentedAttribute]] = None) -> list[ModelT]:
q = select(cls)
q = self._load_relationships(q, load_relationships_list)
async with self.session_scope() as session:
result = await session.execute(q)
return result.unique().scalars().all()
# Apply similar pattern to other methodsThis approach:
- Centralizes session management logic
- Ensures consistent error handling and session cleanup
- Reduces code duplication
- Maintains explicit session control
- Improves readability and maintainability
Apply this pattern across all methods in the Repository class to significantly reduce complexity while preserving the benefits of explicit session management.
|
|
||
| from app.date_manager import get_iso_timestring | ||
| from app.models import User, Wallet | ||
| from tests.utils import make_http_request |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
Summary by Sourcery
Improve database session management by using
async_sessionmakerfor session creation, ensuring session isolation. Refactor services to remove unnecessary repository dependencies and streamline user management. Enhance transaction and wallet operations for better consistency. Add tests to verify the new session management approach.Bug Fixes:
Enhancements:
async_sessionmakerfor creating sessions, improving session management and isolation.UserServiceto directly use aUserManagerinstance, streamlining user management operations.Tests: