From 0e975f6305eef9a5ef2dd04f33d41a1ec501745e Mon Sep 17 00:00:00 2001 From: chenshiyang <88581400+a-yeyang@users.noreply.github.com> Date: Tue, 18 Aug 2026 07:23:09 +0000 Subject: [PATCH] refactor(models): dep-inject Session on remaining @property accessors (#40797) Co-authored-by: Claude --- api/models/trigger.py | 13 ++- api/models/web.py | 8 +- .../unit_tests/models/test_trigger_models.py | 89 +++++++++++++++++++ .../unit_tests/models/test_web_models.py | 67 ++++++++++++++ 4 files changed, 164 insertions(+), 13 deletions(-) create mode 100644 api/tests/unit_tests/models/test_trigger_models.py create mode 100644 api/tests/unit_tests/models/test_web_models.py diff --git a/api/models/trigger.py b/api/models/trigger.py index abab8dfb2cb..fe61a479495 100644 --- a/api/models/trigger.py +++ b/api/models/trigger.py @@ -8,7 +8,7 @@ from uuid import uuid4 import sqlalchemy as sa from sqlalchemy import DateTime, Index, Integer, String, UniqueConstraint, func -from sqlalchemy.orm import Mapped, mapped_column +from sqlalchemy.orm import Mapped, Session, mapped_column from core.plugin.entities.plugin_daemon import CredentialType from core.trigger.entities.api_entities import TriggerProviderSubscriptionApiEntity @@ -18,7 +18,6 @@ from libs.datetime_utils import naive_utc_now from libs.uuid_utils import uuidv7 from .base import TypeBase -from .engine import db from .enums import AppTriggerStatus, AppTriggerType, CreatorUserRole, PermissionEnum, WorkflowTriggerStatus from .model import Account from .types import EnumText, LongText, StringUUID @@ -291,17 +290,15 @@ class WorkflowTriggerLog(TypeBase): triggered_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True, default=None) finished_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True, default=None) - @property - def created_by_account(self): + def created_by_account(self, session: Session) -> Account | None: created_by_role = CreatorUserRole(self.created_by_role) - return db.session.get(Account, self.created_by) if created_by_role == CreatorUserRole.ACCOUNT else None + return session.get(Account, self.created_by) if created_by_role == CreatorUserRole.ACCOUNT else None - @property - def created_by_end_user(self): + def created_by_end_user(self, session: Session): from .model import EndUser created_by_role = CreatorUserRole(self.created_by_role) - return db.session.get(EndUser, self.created_by) if created_by_role == CreatorUserRole.END_USER else None + return session.get(EndUser, self.created_by) if created_by_role == CreatorUserRole.END_USER else None def to_dict(self) -> WorkflowTriggerLogDict: """Convert to dictionary for API responses""" diff --git a/api/models/web.py b/api/models/web.py index 1fb37340d73..44843640a49 100644 --- a/api/models/web.py +++ b/api/models/web.py @@ -3,10 +3,9 @@ from uuid import uuid4 import sqlalchemy as sa from sqlalchemy import DateTime, func, select -from sqlalchemy.orm import Mapped, mapped_column +from sqlalchemy.orm import Mapped, Session, mapped_column from .base import TypeBase -from .engine import db from .enums import CreatorUserRole from .model import Message from .types import EnumText, StringUUID @@ -36,9 +35,8 @@ class SavedMessage(TypeBase): init=False, ) - @property - def message(self): - return db.session.scalar(select(Message).where(Message.id == self.message_id)) + def message(self, session: Session) -> Message | None: + return session.scalar(select(Message).where(Message.id == self.message_id)) class PinnedConversation(TypeBase): diff --git a/api/tests/unit_tests/models/test_trigger_models.py b/api/tests/unit_tests/models/test_trigger_models.py new file mode 100644 index 00000000000..749bac5a6f6 --- /dev/null +++ b/api/tests/unit_tests/models/test_trigger_models.py @@ -0,0 +1,89 @@ +"""Regression coverage for ``models.trigger.WorkflowTriggerLog`` account accessors. + +Ensures the ``@property``→session-parameter refactor preserves the role-based dispatch: +``created_by_account`` looks up an Account only when role is ACCOUNT; ``created_by_end_user`` +looks up an EndUser only when role is END_USER. + +Both accessors are exercised against the real ``sqlite_session`` fixture (a genuine +SQLAlchemy ``Session`` bound to a pristine full-schema SQLite database) so the assertions +cover actual query behaviour rather than a mock's recorded call. +""" + +from sqlalchemy.orm import Session + +from models.account import Account +from models.enums import CreatorUserRole, EndUserType, WorkflowTriggerStatus +from models.model import EndUser +from models.trigger import WorkflowTriggerLog + + +def _log(role: CreatorUserRole, created_by: str) -> WorkflowTriggerLog: + """Construct a WorkflowTriggerLog without touching the database.""" + return WorkflowTriggerLog( + tenant_id="00000000-0000-0000-0000-000000000001", + app_id="00000000-0000-0000-0000-000000000002", + workflow_id="00000000-0000-0000-0000-000000000003", + workflow_run_id=None, + root_node_id=None, + trigger_metadata="{}", + trigger_type="manual", + trigger_data="{}", + inputs="{}", + outputs=None, + status=WorkflowTriggerStatus.SUCCEEDED, + error=None, + queue_name="default", + celery_task_id=None, + created_by_role=role, + created_by=created_by, + ) + + +class TestCreatedByAccount: + def test_returns_account_lookup_when_role_is_account(self, sqlite_session: Session) -> None: + account = Account(name="Test Account", email="test@example.com") + sqlite_session.add(account) + sqlite_session.flush() + log = _log(CreatorUserRole.ACCOUNT, created_by=account.id) + + result = log.created_by_account(session=sqlite_session) + + assert result is not None + assert result.id == account.id + + def test_returns_none_when_role_is_end_user(self, sqlite_session: Session) -> None: + account = Account(name="Test Account", email="test@example.com") + sqlite_session.add(account) + sqlite_session.flush() + log = _log(CreatorUserRole.END_USER, created_by=account.id) + + assert log.created_by_account(session=sqlite_session) is None + + +class TestCreatedByEndUser: + def test_returns_end_user_lookup_when_role_is_end_user(self, sqlite_session: Session) -> None: + end_user = EndUser( + tenant_id="00000000-0000-0000-0000-000000000001", + type=EndUserType.BROWSER, + session_id="session-1", + ) + sqlite_session.add(end_user) + sqlite_session.flush() + log = _log(CreatorUserRole.END_USER, created_by=end_user.id) + + result = log.created_by_end_user(session=sqlite_session) + + assert result is not None + assert result.id == end_user.id + + def test_returns_none_when_role_is_account(self, sqlite_session: Session) -> None: + end_user = EndUser( + tenant_id="00000000-0000-0000-0000-000000000001", + type=EndUserType.BROWSER, + session_id="session-1", + ) + sqlite_session.add(end_user) + sqlite_session.flush() + log = _log(CreatorUserRole.ACCOUNT, created_by=end_user.id) + + assert log.created_by_end_user(session=sqlite_session) is None diff --git a/api/tests/unit_tests/models/test_web_models.py b/api/tests/unit_tests/models/test_web_models.py new file mode 100644 index 00000000000..1abd9fb094e --- /dev/null +++ b/api/tests/unit_tests/models/test_web_models.py @@ -0,0 +1,67 @@ +"""Regression coverage for ``models.web.SavedMessage.message`` accessor. + +Ensures the property→method refactor (drop of ``db.session`` in favor of a caller-provided +``Session``) preserves query intent: the accessor forwards ``self.message_id`` to the +supplied session and returns the matching :class:`Message` (or ``None`` when absent). + +The accessor is exercised against the real ``sqlite_session`` fixture (a genuine SQLAlchemy +``Session`` bound to a pristine full-schema SQLite database) so the assertions cover actual +query behaviour rather than a mock's recorded call. +""" + +from decimal import Decimal +from uuid import uuid4 + +from sqlalchemy.orm import Session + +from models.enums import CreatorUserRole +from models.model import ConversationFromSource, Message +from models.web import SavedMessage + + +def _persist_message(session: Session, *, app_id: str) -> Message: + """Persist a minimal valid Message row and return it.""" + message = Message( + app_id=app_id, + conversation_id=str(uuid4()), + inputs={}, + query="hello", + message=[{"role": "user", "text": "hello"}], + message_unit_price=Decimal(0), + message_price_unit=Decimal(0), + answer="hi", + answer_unit_price=Decimal(0), + answer_price_unit=Decimal(0), + currency="USD", + from_source=ConversationFromSource.API, + ) + session.add(message) + session.flush() + return message + + +def _saved_message(*, app_id: str, message_id: str) -> SavedMessage: + """Construct a SavedMessage without touching the database.""" + return SavedMessage( + app_id=app_id, + message_id=message_id, + created_by_role=CreatorUserRole.END_USER, + created_by=str(uuid4()), + ) + + +def test_message_returns_persisted_message(sqlite_session: Session) -> None: + app_id = str(uuid4()) + message = _persist_message(sqlite_session, app_id=app_id) + saved = _saved_message(app_id=app_id, message_id=message.id) + + result = saved.message(session=sqlite_session) + + assert result is not None + assert result.id == message.id + + +def test_message_returns_none_when_message_missing(sqlite_session: Session) -> None: + saved = _saved_message(app_id=str(uuid4()), message_id=str(uuid4())) + + assert saved.message(session=sqlite_session) is None