fix: fix sqlalchemy.orm.exc.DetachedInstanceError#34845
fix: fix sqlalchemy.orm.exc.DetachedInstanceError#34845asukaminato0721 merged 1 commit intolanggenius:mainfrom
Conversation
dosubot
Bot
added
the
size:XS
Pyrefly DiffNo changes detected. |
dosubot
Bot
added
size:M
Pyrefly DiffNo changes detected. |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix sqlalchemy.orm.exc.DetachedInstanceError seen when reading plugin auto-upgrade strategy preferences by changing how SQLAlchemy sessions are created/managed in the plugin auto-upgrade service.
Changes:
- Switched
PluginAutoUpgradeServicefromsessionmaker(bind=db.engine).begin()tosession_factory.create_session(). - Updated unit tests to patch
session_factoryinstead of patchingsessionmaker/db.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
api/services/plugin/plugin_auto_upgrade_service.py |
Refactors DB session creation for strategy reads/updates to use the shared session_factory. |
api/tests/unit_tests/services/plugin/test_plugin_auto_upgrade_service.py |
Updates mocks to match the new session factory usage. |
Comments suppressed due to low confidence (1)
api/services/plugin/plugin_auto_upgrade_service.py:30
change_strategy()performs inserts/updates but this session scope never starts a transaction or commits.Sessionused as a context manager will close/rollback on exit, so changes won’t be persisted. Use an explicit transaction (with session_factory.create_session() as session, session.begin():) or callsession.commit()before returning.
with session_factory.create_session() as session:
exist_strategy = session.scalar(
select(TenantPluginAutoUpgradeStrategy)
.where(TenantPluginAutoUpgradeStrategy.tenant_id == tenant_id)
.limit(1)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def exclude_plugin(tenant_id: str, plugin_id: str) -> bool: | ||
| with sessionmaker(bind=db.engine).begin() as session: | ||
| with session_factory.create_session() as session: | ||
| exist_strategy = session.scalar( | ||
| select(TenantPluginAutoUpgradeStrategy) | ||
| .where(TenantPluginAutoUpgradeStrategy.tenant_id == tenant_id) |
There was a problem hiding this comment.
exclude_plugin() mutates exist_strategy but doesn’t commit. Without session.begin() / session.commit(), updates to mode and include/exclude lists won’t be saved. Wrap this session scope in an explicit transaction (or commit before returning).
| def _patched_session(): | ||
| """Patch sessionmaker(bind=db.engine).begin() to return a mock session as context manager.""" | ||
| """Patch session_factory.create_session() to return a mock session as context manager.""" | ||
| session = MagicMock() | ||
| mock_sessionmaker = MagicMock() | ||
| mock_sessionmaker.return_value.begin.return_value.__enter__ = MagicMock(return_value=session) | ||
| mock_sessionmaker.return_value.begin.return_value.__exit__ = MagicMock(return_value=False) | ||
| patcher = patch(f"{MODULE}.sessionmaker", mock_sessionmaker) | ||
| db_patcher = patch(f"{MODULE}.db") | ||
| return patcher, db_patcher, session | ||
| session.__enter__ = MagicMock(return_value=session) | ||
| session.__exit__ = MagicMock(return_value=False) |
There was a problem hiding this comment.
These tests patch session_factory.create_session() to act as a context manager, but they don’t model/verify the transaction boundary (session.begin() / session.commit()) that production code should use for writes. After fixing the service to commit, update this helper to also mock the transaction context and assert it was invoked so the tests catch missing commits.
Important
Fixes #<issue number>.Summary
fix #34844
Screenshots
Checklist
make lintandmake type-check(backend) andcd web && pnpm exec vp staged(frontend) to appease the lint gods