diff --git a/api/controllers/console/workspace/snippets.py b/api/controllers/console/workspace/snippets.py index 22769ba88a4..2b8f609801e 100644 --- a/api/controllers/console/workspace/snippets.py +++ b/api/controllers/console/workspace/snippets.py @@ -5,7 +5,7 @@ from uuid import UUID from flask import Response, request from flask_restx import Resource from sqlalchemy.orm import Session, sessionmaker -from werkzeug.exceptions import NotFound +from werkzeug.exceptions import BadRequest, NotFound from controllers.common.fields import TextFileResponse from controllers.common.rbac import RBACCheck, Workspace @@ -36,7 +36,13 @@ from controllers.console.wraps import ( from core.plugin.entities.plugin import PluginDependency from extensions.ext_database import db from fields.base import ResponseModel -from fields.snippet_fields import SnippetListItemResponse, SnippetPaginationResponse, SnippetResponse +from fields.snippet_fields import ( + SnippetListItemResponse, + SnippetPaginationResponse, + SnippetResponse, + snippet_list_item_responses, + snippet_response, +) from libs.helper import dump_response from libs.login import login_required from models import Account @@ -112,14 +118,15 @@ class CustomizedSnippetsApi(Resource): @login_required @account_initialization_required @with_current_tenant_id - def get(self, current_tenant_id: str): + @with_session(write=False) + def get(self, session: Session, current_tenant_id: str): """List customized snippets with pagination and search.""" query = _snippet_list_query_from_request() snippet_service = _snippet_service() snippets, total, has_more = snippet_service.get_snippets( tenant_id=current_tenant_id, - session=db.session(), + session=session, page=query.page, limit=query.limit, keyword=query.keyword, @@ -131,7 +138,7 @@ class CustomizedSnippetsApi(Resource): return dump_response( SnippetPaginationResponse, { - "data": snippets, + "data": snippet_list_item_responses(snippets, session=session), "page": query.page, "limit": query.limit, "total": total, @@ -150,8 +157,9 @@ class CustomizedSnippetsApi(Resource): @rbac_permission_required(RBACCheck(RBACPermission.SNIPPETS_CREATE_AND_MODIFY, Workspace())) @with_current_user @with_current_tenant_id + @with_session @model_validate(CreateSnippetPayload) - def post(self, req_data: CreateSnippetPayload, current_tenant_id: str, current_user: Account): + def post(self, req_data: CreateSnippetPayload, session: Session, current_tenant_id: str, current_user: Account): """Create a new customized snippet.""" try: snippet_type = SnippetType(req_data.type) @@ -175,7 +183,7 @@ class CustomizedSnippetsApi(Resource): except ValueError as e: return {"message": str(e)}, 400 - return dump_response(SnippetResponse, snippet), 201 + return dump_response(SnippetResponse, snippet_response(snippet, session=session)), 201 @console_ns.route("/workspaces/current/customized-snippets/") @@ -187,7 +195,8 @@ class CustomizedSnippetDetailApi(Resource): @login_required @account_initialization_required @with_current_tenant_id - def get(self, current_tenant_id: str, snippet_id: UUID): + @with_session(write=False) + def get(self, session: Session, current_tenant_id: str, snippet_id: UUID): """Get customized snippet details.""" snippet_service = _snippet_service() snippet = snippet_service.get_snippet_by_id( @@ -198,7 +207,7 @@ class CustomizedSnippetDetailApi(Resource): if not snippet: raise NotFound("Snippet not found") - return dump_response(SnippetResponse, snippet), 200 + return dump_response(SnippetResponse, snippet_response(snippet, session=session)), 200 @console_ns.doc("update_customized_snippet") @console_ns.expect(console_ns.models.get(UpdateSnippetPayload.__name__)) @@ -212,8 +221,16 @@ class CustomizedSnippetDetailApi(Resource): @rbac_permission_required(RBACCheck(RBACPermission.SNIPPETS_CREATE_AND_MODIFY, Workspace())) @with_current_user @with_current_tenant_id + @with_session @model_validate(UpdateSnippetPayload) - def patch(self, req_data: UpdateSnippetPayload, current_tenant_id: str, current_user: Account, snippet_id: str): + def patch( + self, + req_data: UpdateSnippetPayload, + session: Session, + current_tenant_id: str, + current_user: Account, + snippet_id: str, + ): """Update customized snippet.""" snippet_service = _snippet_service() snippet = snippet_service.get_snippet_by_id( @@ -233,19 +250,22 @@ class CustomizedSnippetDetailApi(Resource): return {"message": "No valid fields to update"}, 400 try: - with Session(db.engine, expire_on_commit=False) as session: - snippet = session.merge(snippet) - snippet = SnippetService.update_snippet( - session=session, - snippet=snippet, - account_id=current_user.id, - data=update_data, - ) - session.commit() + snippet = session.merge(snippet) + snippet = SnippetService.update_snippet( + session=session, + snippet=snippet, + account_id=current_user.id, + data=update_data, + ) + session.commit() except ValueError as e: - return {"message": str(e)}, 400 + # Raise rather than return: `with_session` commits on a normal return, so returning here + # would persist whatever the update wrote before it rejected the payload. Raising routes + # through the decorator's rollback. Status stays 400 and `message` is unchanged; the body + # picks up the standard error envelope, as on every other BadRequest in the console API. + raise BadRequest(str(e)) from e - return dump_response(SnippetResponse, snippet), 200 + return dump_response(SnippetResponse, snippet_response(snippet, session=session)), 200 @console_ns.doc("delete_customized_snippet") @console_ns.response(204, "Snippet deleted successfully") diff --git a/api/fields/snippet_fields.py b/api/fields/snippet_fields.py index 77aba3fe76e..6c6d6469afb 100644 --- a/api/fields/snippet_fields.py +++ b/api/fields/snippet_fields.py @@ -1,12 +1,14 @@ +from collections.abc import Iterable from datetime import datetime from typing import Any from pydantic import Field, field_validator +from sqlalchemy.orm import Session from fields.base import ResponseModel from fields.member_fields import SimpleAccountResponse from libs.helper import to_timestamp -from models.snippet import SnippetType +from models.snippet import CustomizedSnippet, SnippetType class SnippetTagResponse(ResponseModel): @@ -72,3 +74,54 @@ class SnippetPaginationResponse(ResponseModel): limit: int total: int has_more: bool + + +def snippet_response(snippet: CustomizedSnippet, *, session: Session) -> SnippetResponse: + """Build the snippet detail response, resolving session-backed lookups at the request boundary.""" + return SnippetResponse.model_validate( + { + "id": snippet.id, + "name": snippet.name, + "description": snippet.description, + "type": snippet.type, + "version": snippet.version, + "use_count": snippet.use_count, + "is_published": snippet.is_published, + "icon_info": snippet.icon_info, + "graph": snippet.get_graph_dict(session=session), + "input_fields": snippet.input_fields_list, + "tags": snippet.get_tags(session=session), + "created_by": snippet.get_created_by_account(session=session), + "created_at": snippet.created_at, + "updated_by": snippet.get_updated_by_account(session=session), + "updated_at": snippet.updated_at, + } + ) + + +def snippet_list_item_response(snippet: CustomizedSnippet, *, session: Session) -> SnippetListItemResponse: + """Build one snippet list row, resolving session-backed lookups at the request boundary.""" + return SnippetListItemResponse.model_validate( + { + "id": snippet.id, + "name": snippet.name, + "description": snippet.description, + "type": snippet.type, + "version": snippet.version, + "use_count": snippet.use_count, + "is_published": snippet.is_published, + "icon_info": snippet.icon_info, + "tags": snippet.get_tags(session=session), + "created_by": snippet.created_by, + "author_name": snippet.get_author_name(session=session), + "created_at": snippet.created_at, + "updated_by": snippet.updated_by, + "updated_at": snippet.updated_at, + } + ) + + +def snippet_list_item_responses( + snippets: Iterable[CustomizedSnippet], *, session: Session +) -> list[SnippetListItemResponse]: + return [snippet_list_item_response(snippet, session=session) for snippet in snippets] diff --git a/api/models/snippet.py b/api/models/snippet.py index 4a9320e51f6..57c51ea9e3f 100644 --- a/api/models/snippet.py +++ b/api/models/snippet.py @@ -1,17 +1,17 @@ import json +from collections.abc import Sequence from datetime import datetime from enum import StrEnum from typing import Any import sqlalchemy as sa from sqlalchemy import DateTime, String, func -from sqlalchemy.orm import Mapped, mapped_column +from sqlalchemy.orm import Mapped, Session, mapped_column from libs.uuid_utils import uuidv7 from .account import Account from .base import Base -from .engine import db from .model import Tag, TagBinding from .types import AdjustedJSON, LongText, StringUUID @@ -65,13 +65,12 @@ class CustomizedSnippet(Base): DateTime, nullable=False, server_default=func.current_timestamp(), onupdate=func.current_timestamp() ) - @property - def graph_dict(self) -> dict[str, Any]: + def get_graph_dict(self, *, session: Session) -> dict[str, Any]: """Get graph from associated workflow.""" if self.workflow_id: from .workflow import Workflow - workflow = db.session.get(Workflow, self.workflow_id) + workflow = session.get(Workflow, self.workflow_id) if workflow: return json.loads(workflow.graph) if workflow.graph else {} return {} @@ -81,10 +80,9 @@ class CustomizedSnippet(Base): """Parse input_fields JSON to list.""" return json.loads(self.input_fields) if self.input_fields else [] - @property - def tags(self): + def get_tags(self, *, session: Session) -> Sequence[Tag]: """Get snippet tags.""" - tags = db.session.scalars( + tags = session.scalars( sa.select(Tag) .join(TagBinding, Tag.id == TagBinding.tag_id) .where( @@ -97,24 +95,21 @@ class CustomizedSnippet(Base): return tags or [] - @property - def created_by_account(self) -> Account | None: + def get_created_by_account(self, *, session: Session) -> Account | None: """Get the account that created this snippet.""" if self.created_by: - return db.session.get(Account, self.created_by) + return session.get(Account, self.created_by) return None - @property - def author_name(self) -> str | None: + def get_author_name(self, *, session: Session) -> str | None: """Get the creator account name.""" - account = self.created_by_account + account = self.get_created_by_account(session=session) return account.name if account else None - @property - def updated_by_account(self) -> Account | None: + def get_updated_by_account(self, *, session: Session) -> Account | None: """Get the account that last updated this snippet.""" if self.updated_by: - return db.session.get(Account, self.updated_by) + return session.get(Account, self.updated_by) return None @property diff --git a/api/tests/unit_tests/controllers/console/workspace/test_snippets.py b/api/tests/unit_tests/controllers/console/workspace/test_snippets.py index e89aa6a6b57..d664fb3eb9a 100644 --- a/api/tests/unit_tests/controllers/console/workspace/test_snippets.py +++ b/api/tests/unit_tests/controllers/console/workspace/test_snippets.py @@ -1,14 +1,17 @@ from datetime import UTC, datetime from inspect import unwrap from types import SimpleNamespace -from unittest.mock import ANY, Mock +from unittest.mock import Mock import pytest from flask import Flask -from werkzeug.exceptions import NotFound +from pydantic import ValidationError +from sqlalchemy.orm import Session +from werkzeug.exceptions import BadRequest, NotFound from controllers.console.workspace import snippets as snippets_module from models.account import Account, TenantAccountRole +from models.snippet import CustomizedSnippet from services.snippet_dsl_service import ImportStatus, SnippetImportInfo @@ -39,30 +42,31 @@ def _account(account_id: str = "account-1") -> Account: return account -def _snippet(**overrides) -> SimpleNamespace: - data = { - "id": "snippet-1", - "tenant_id": "tenant-1", - "name": "Snippet", - "description": "Description", - "type": snippets_module.SnippetType.NODE, - "version": 1, - "use_count": 0, - "is_published": False, - "icon_info": None, - "graph_dict": {}, - "input_fields_list": [], - "tags": [], - "created_by": None, - "author_name": None, - "created_by_account": None, - "created_at": datetime.fromtimestamp(1_704_067_200, UTC), - "updated_by": None, - "updated_by_account": None, - "updated_at": datetime.fromtimestamp(1_704_153_600, UTC), - } - data.update(overrides) - return SimpleNamespace(**data) +def _snippet(**overrides) -> CustomizedSnippet: + """Build a real ``CustomizedSnippet`` row so session-backed accessors run against the test schema. + + The SQLite fixtures in ``tests/unit_tests/conftest.py`` provide the full schema, so the + ``get_*(session=...)`` accessors resolve through real queries instead of hand-written stubs. + """ + snippet = CustomizedSnippet( + tenant_id="tenant-1", + name="Snippet", + description="Description", + type=snippets_module.SnippetType.NODE, + version=1, + use_count=0, + is_published=False, + icon_info=None, + input_fields=None, + created_by=None, + created_at=datetime.fromtimestamp(1_704_067_200, UTC), + updated_by=None, + updated_at=datetime.fromtimestamp(1_704_153_600, UTC), + ) + snippet.id = "snippet-1" + for name, value in overrides.items(): + setattr(snippet, name, value) + return snippet def test_snippet_list_query_reads_repeated_values(app: Flask): @@ -98,7 +102,7 @@ def test_snippet_list_query_ignores_indexed_values(app: Flask): assert query.creators is None -def test_list_snippets_returns_pagination(app: Flask, monkeypatch: pytest.MonkeyPatch): +def test_list_snippets_returns_pagination(app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session): snippets = [_snippet()] tag_id = "11111111-1111-1111-1111-111111111111" get_snippets = Mock(return_value=(snippets, 1, False)) @@ -110,7 +114,7 @@ def test_list_snippets_returns_pagination(app: Flask, monkeypatch: pytest.Monkey with app.test_request_context( f"/workspaces/current/customized-snippets?page=2&limit=10&tag_ids={tag_id}&creators=account-2" ): - response, status_code = handler(api, "tenant-1") + response, status_code = handler(api, sqlite_session, "tenant-1") assert status_code == 200 assert response == { @@ -139,7 +143,7 @@ def test_list_snippets_returns_pagination(app: Flask, monkeypatch: pytest.Monkey } get_snippets.assert_called_once_with( tenant_id="tenant-1", - session=ANY, + session=sqlite_session, page=2, limit=10, keyword=None, @@ -149,7 +153,9 @@ def test_list_snippets_returns_pagination(app: Flask, monkeypatch: pytest.Monkey ) -def test_create_snippet_defaults_unknown_type_and_returns_created(app: Flask, monkeypatch: pytest.MonkeyPatch): +def test_create_snippet_defaults_unknown_type_and_returns_created( + app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session +): user = _account("account-1") snippet = _snippet() create_snippet = Mock(return_value=snippet) @@ -172,7 +178,7 @@ def test_create_snippet_defaults_unknown_type_and_returns_created(app: Flask, mo method="POST", json={"name": "Snippet", "type": "node", "description": "Description"}, ): - response, status_code = handler(api, req_data, "tenant-1", user) + response, status_code = handler(api, req_data, sqlite_session, "tenant-1", user) assert status_code == 201 assert response["id"] == "snippet-1" @@ -180,7 +186,7 @@ def test_create_snippet_defaults_unknown_type_and_returns_created(app: Flask, mo assert create_snippet.call_args.kwargs["snippet_type"] == snippets_module.SnippetType.NODE -def test_create_snippet_rejects_forbidden_nodes(app: Flask, monkeypatch: pytest.MonkeyPatch): +def test_create_snippet_rejects_forbidden_nodes(app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session): user = _account("account-1") create_snippet = Mock() monkeypatch.setattr(snippets_module.SnippetService, "create_snippet", create_snippet) @@ -213,14 +219,14 @@ def test_create_snippet_rejects_forbidden_nodes(app: Flask, monkeypatch: pytest. }, }, ): - response, status_code = handler(api, req_data, "tenant-1", user) + response, status_code = handler(api, req_data, sqlite_session, "tenant-1", user) assert status_code == 400 assert "knowledge-retrieval" in response["message"] create_snippet.assert_not_called() -def test_get_snippet_detail_raises_when_missing(app: Flask, monkeypatch: pytest.MonkeyPatch): +def test_get_snippet_detail_raises_when_missing(app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session): monkeypatch.setattr(snippets_module.SnippetService, "get_snippet_by_id", Mock(return_value=None)) api = snippets_module.CustomizedSnippetDetailApi() @@ -228,10 +234,10 @@ def test_get_snippet_detail_raises_when_missing(app: Flask, monkeypatch: pytest. with app.test_request_context("/workspaces/current/customized-snippets/snippet-1"): with pytest.raises(NotFound, match="Snippet not found"): - handler(api, "tenant-1", snippet_id="snippet-1") + handler(api, sqlite_session, "tenant-1", snippet_id="snippet-1") -def test_get_snippet_detail_returns_snippet(app: Flask, monkeypatch: pytest.MonkeyPatch): +def test_get_snippet_detail_returns_snippet(app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session): snippet = _snippet() monkeypatch.setattr(snippets_module.SnippetService, "get_snippet_by_id", Mock(return_value=snippet)) @@ -239,14 +245,38 @@ def test_get_snippet_detail_returns_snippet(app: Flask, monkeypatch: pytest.Monk handler = unwrap(api.get) with app.test_request_context("/workspaces/current/customized-snippets/snippet-1"): - response, status_code = handler(api, "tenant-1", snippet_id="snippet-1") + response, status_code = handler(api, sqlite_session, "tenant-1", snippet_id="snippet-1") assert status_code == 200 assert response["id"] == "snippet-1" assert response["name"] == "Snippet" -def test_patch_snippet_returns_400_for_empty_payload(app: Flask, monkeypatch: pytest.MonkeyPatch): +def test_get_snippet_detail_resolves_creator_through_the_request_session( + app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session +): + """The injected session is the one the accessors query, so a persisted creator resolves.""" + author = _account("11111111-1111-1111-1111-111111111111") + sqlite_session.add(author) + sqlite_session.commit() + + snippet = _snippet(created_by=author.id, updated_by=author.id) + monkeypatch.setattr(snippets_module.SnippetService, "get_snippet_by_id", Mock(return_value=snippet)) + + api = snippets_module.CustomizedSnippetDetailApi() + handler = unwrap(api.get) + + with app.test_request_context("/workspaces/current/customized-snippets/snippet-1"): + response, status_code = handler(api, sqlite_session, "tenant-1", snippet_id="snippet-1") + + assert status_code == 200 + assert response["created_by"] == {"id": author.id, "name": "Test User", "email": author.email} + assert response["updated_by"] == {"id": author.id, "name": "Test User", "email": author.email} + + +def test_patch_snippet_returns_400_for_empty_payload( + app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session +): snippet = _snippet() user = _account("user-1") monkeypatch.setattr(snippets_module.SnippetService, "get_snippet_by_id", Mock(return_value=snippet)) @@ -261,27 +291,20 @@ def test_patch_snippet_returns_400_for_empty_payload(app: Flask, monkeypatch: py method="PATCH", json={}, ): - response, status_code = handler(api, req_data, "tenant-1", user, snippet_id="snippet-1") + response, status_code = handler(api, req_data, sqlite_session, "tenant-1", user, snippet_id="snippet-1") assert status_code == 400 assert response == {"message": "No valid fields to update"} -def test_patch_snippet_updates_and_commits(app: Flask, monkeypatch: pytest.MonkeyPatch): +def test_patch_snippet_updates_and_commits(app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session): + _persist_snippet(sqlite_session) user = _account("account-1") snippet = _snippet() - updated_snippet = _snippet(name="New") - session = SimpleNamespace(merge=Mock(return_value=snippet), commit=Mock()) - update_snippet = Mock(return_value=updated_snippet) - - class SessionContext(_SessionContext): - def __init__(self, engine, *args, **kwargs): - super().__init__(engine, *args, session=session, **kwargs) + update_snippet = Mock(side_effect=_apply_update) monkeypatch.setattr(snippets_module.SnippetService, "get_snippet_by_id", Mock(return_value=snippet)) monkeypatch.setattr(snippets_module.SnippetService, "update_snippet", update_snippet) - monkeypatch.setattr(snippets_module, "Session", SessionContext) - monkeypatch.setattr(snippets_module, "db", SimpleNamespace(engine=object())) req_data = snippets_module.UpdateSnippetPayload(name="New", icon_info={"icon": "star"}) @@ -293,17 +316,125 @@ def test_patch_snippet_updates_and_commits(app: Flask, monkeypatch: pytest.Monke method="PATCH", json={"name": "New", "icon_info": {"icon": "star"}}, ): - response, status_code = handler(api, req_data, "tenant-1", user, snippet_id="snippet-1") + response, status_code = handler(api, req_data, sqlite_session, "tenant-1", user, snippet_id="snippet-1") assert status_code == 200 assert response["id"] == "snippet-1" assert response["name"] == "New" update_snippet.assert_called_once() + assert update_snippet.call_args.kwargs["session"] is sqlite_session assert update_snippet.call_args.kwargs["data"] == { "name": "New", "icon_info": {"icon": "star", "icon_background": None, "icon_type": None, "icon_url": None}, } - session.commit.assert_called_once() + assert _persisted_name(sqlite_session) == "New" + + +def test_patch_snippet_does_not_report_a_committed_write_as_a_bad_request( + app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session +): + """The ``except ValueError`` must scope the update call only, never the serialization. + + ``ValidationError`` subclasses ``ValueError``. Both routes end at a 400 either way — the app + registers a ``ValueError`` handler in ``libs/external_api.py`` — so what matters is *where* it + is handled: covering the serialization would blame the client's payload for a write that already + succeeded, and swallow the failure before it ever reaches the error handlers. The committed row + must survive regardless. + """ + _persist_snippet(sqlite_session) + user = _account("account-1") + snippet = _snippet() + + monkeypatch.setattr(snippets_module.SnippetService, "get_snippet_by_id", Mock(return_value=snippet)) + monkeypatch.setattr(snippets_module.SnippetService, "update_snippet", Mock(side_effect=_apply_update)) + monkeypatch.setattr(CustomizedSnippet, "get_graph_dict", _unserializable_graph) + + req_data = snippets_module.UpdateSnippetPayload(name="New") + + api = snippets_module.CustomizedSnippetDetailApi() + handler = unwrap(api.patch) + + with app.test_request_context( + "/workspaces/current/customized-snippets/snippet-1", + method="PATCH", + json={"name": "New"}, + ): + with pytest.raises(ValidationError): + handler(api, req_data, sqlite_session, "tenant-1", user, snippet_id="snippet-1") + + assert _persisted_name(sqlite_session) == "New" + + +def test_patch_snippet_does_not_persist_a_rejected_update( + app: Flask, monkeypatch: pytest.MonkeyPatch, sqlite_session: Session +): + """A rejected update must raise, not return a 400 tuple. + + ``with_session`` commits on any normal return, so returning ``{"message": ...}, 400`` would + durably persist whatever ``update_snippet`` wrote before it rejected the payload; raising routes + through the decorator's rollback instead. That commit lives *in the decorator*, so this test runs + the handler through it rather than through ``unwrap`` — the autouse ``_sqlite_session_factory`` + fixture already points ``with_session`` at the same database as ``sqlite_session``. + """ + _persist_snippet(sqlite_session) + user = _account("account-1") + snippet = _snippet() + + monkeypatch.setattr(snippets_module.SnippetService, "get_snippet_by_id", Mock(return_value=snippet)) + monkeypatch.setattr(snippets_module.SnippetService, "update_snippet", Mock(side_effect=_reject_update)) + + req_data = snippets_module.UpdateSnippetPayload(name="New") + + api = snippets_module.CustomizedSnippetDetailApi() + view = unwrap(api.patch) + + @snippets_module.with_session + def patch_through_decorator(resource, session: Session, snippet_id: str): + return view(resource, req_data, session, "tenant-1", user, snippet_id=snippet_id) + + with app.test_request_context( + "/workspaces/current/customized-snippets/snippet-1", + method="PATCH", + json={"name": "New"}, + ): + with pytest.raises(BadRequest, match="name already in use"): + patch_through_decorator(api, snippet_id="snippet-1") + + assert _persisted_name(sqlite_session) == "Snippet" + + +def _apply_update(*, session: Session, snippet: CustomizedSnippet, account_id: str, data: dict) -> CustomizedSnippet: + """Stand in for ``SnippetService.update_snippet``: write the payload onto the merged row.""" + del session, account_id + for field, value in data.items(): + setattr(snippet, field, value) + return snippet + + +def _reject_update(*, session: Session, snippet: CustomizedSnippet, account_id: str, data: dict) -> CustomizedSnippet: + """Stand in for an ``update_snippet`` that writes some fields and then rejects the payload.""" + _apply_update(session=session, snippet=snippet, account_id=account_id, data=data) + raise ValueError("name already in use") + + +def _unserializable_graph(self: CustomizedSnippet, *, session: Session) -> str: + """Return a non-dict graph so response validation fails after the write is committed.""" + del self, session + return "not-a-dict" + + +def _persist_snippet(session: Session) -> None: + """Persist the baseline row so the handler's ``merge`` takes the UPDATE path, as in production.""" + with Session(bind=session.get_bind()) as setup_session: + setup_session.add(_snippet()) + setup_session.commit() + + +def _persisted_name(session: Session) -> str | None: + """Read the snippet name back through a second session to prove what was committed.""" + with Session(bind=session.get_bind()) as verification_session: + stored = verification_session.get(CustomizedSnippet, "snippet-1") + return stored.name if stored else None def test_delete_snippet_deletes_and_commits(app: Flask, monkeypatch: pytest.MonkeyPatch): diff --git a/api/tests/unit_tests/fields/test_snippet_fields.py b/api/tests/unit_tests/fields/test_snippet_fields.py index f3119012c58..aa46ddd83c3 100644 --- a/api/tests/unit_tests/fields/test_snippet_fields.py +++ b/api/tests/unit_tests/fields/test_snippet_fields.py @@ -1,17 +1,26 @@ +import json from datetime import UTC, datetime import pytest from sqlalchemy.orm import Session -from fields.snippet_fields import SnippetListItemResponse -from libs.helper import dump_response -from models import snippet as snippet_module +from fields.snippet_fields import snippet_list_item_response, snippet_response from models.account import Account -from models.snippet import CustomizedSnippet +from models.enums import TagType +from models.model import Tag, TagBinding +from models.snippet import CustomizedSnippet, SnippetType +from models.workflow import Workflow, WorkflowType + +TENANT_ID = "11111111-1111-1111-1111-111111111111" +WORKFLOW_ID = "22222222-2222-2222-2222-222222222222" +APP_ID = "33333333-3333-3333-3333-333333333333" +SNIPPET_ID = "44444444-4444-4444-4444-444444444444" +ACCOUNT_1_ID = "55555555-5555-5555-5555-555555555555" +ACCOUNT_2_ID = "55555555-5555-5555-5555-555555555556" @pytest.mark.parametrize("sqlite_session", [(CustomizedSnippet, Account)], indirect=True) -def test_snippet_list_fields_include_author_name(sqlite_session: Session, monkeypatch: pytest.MonkeyPatch) -> None: +def test_snippet_list_fields_include_author_name(sqlite_session: Session) -> None: account = Account(name="Alice", email="alice@example.com") account.id = "account-1" snippet = CustomizedSnippet( @@ -31,8 +40,72 @@ def test_snippet_list_fields_include_author_name(sqlite_session: Session, monkey ) sqlite_session.add_all([account, snippet]) sqlite_session.flush() - monkeypatch.setattr(snippet_module.db, "session", sqlite_session) - result = dump_response(SnippetListItemResponse, snippet) + result = snippet_list_item_response(snippet, session=sqlite_session).model_dump(mode="json") assert result["author_name"] == "Alice" + + +@pytest.fixture +def populated_snippet(sqlite_session: Session) -> CustomizedSnippet: + """Persist a snippet plus the workflow, accounts and tag its response resolves.""" + workflow = Workflow( + tenant_id=TENANT_ID, + app_id=APP_ID, + type=WorkflowType.WORKFLOW, + version="1", + graph=json.dumps({"nodes": [{"id": "llm-1"}], "edges": []}), + _features="{}", + created_by=ACCOUNT_1_ID, + ) + workflow.id = WORKFLOW_ID + author = Account(name="Ada", email="ada@example.com") + author.id = ACCOUNT_1_ID + editor = Account(name="Grace", email="grace@example.com") + editor.id = ACCOUNT_2_ID + tag = Tag(tenant_id=TENANT_ID, type=TagType.SNIPPET, name="Reusable", created_by=ACCOUNT_1_ID) + binding = TagBinding(tenant_id=TENANT_ID, tag_id=tag.id, target_id=SNIPPET_ID, created_by=ACCOUNT_1_ID) + sqlite_session.add_all((workflow, author, editor, tag, binding)) + sqlite_session.commit() + + return CustomizedSnippet( + id=SNIPPET_ID, + tenant_id=TENANT_ID, + name="Snippet", + description="Reusable node", + type=SnippetType.NODE, + workflow_id=WORKFLOW_ID, + is_published=True, + version=1, + use_count=0, + icon_info=None, + input_fields=json.dumps([{"variable": "query"}]), + created_by=ACCOUNT_1_ID, + updated_by=ACCOUNT_2_ID, + created_at=datetime.fromtimestamp(1704067200, tz=UTC), + updated_at=datetime.fromtimestamp(1704067201, tz=UTC), + ) + + +def test_snippet_response_resolves_fields_from_the_given_session( + populated_snippet: CustomizedSnippet, sqlite_session: Session +) -> None: + result = snippet_response(populated_snippet, session=sqlite_session).model_dump(mode="json") + + assert result["graph"] == {"nodes": [{"id": "llm-1"}], "edges": []} + assert result["input_fields"] == [{"variable": "query"}] + assert result["created_by"]["name"] == "Ada" + assert result["updated_by"]["name"] == "Grace" + assert [tag["name"] for tag in result["tags"]] == ["Reusable"] + + +def test_snippet_list_item_resolves_author_and_tags_from_the_given_session( + populated_snippet: CustomizedSnippet, sqlite_session: Session +) -> None: + result = snippet_list_item_response(populated_snippet, session=sqlite_session).model_dump(mode="json") + + assert result["author_name"] == "Ada" + assert [tag["name"] for tag in result["tags"]] == ["Reusable"] + # The list row carries the raw audit ids; only the detail response resolves them to accounts. + assert result["created_by"] == ACCOUNT_1_ID + assert result["updated_by"] == ACCOUNT_2_ID diff --git a/api/tests/unit_tests/models/test_snippet.py b/api/tests/unit_tests/models/test_snippet.py index 12500bb2d0c..3a03fbcebc6 100644 --- a/api/tests/unit_tests/models/test_snippet.py +++ b/api/tests/unit_tests/models/test_snippet.py @@ -1,8 +1,7 @@ -"""Snippet model properties backed by the shared SQLite test session.""" +"""Snippet model accessors backed by the shared SQLite test session.""" import json -import pytest from sqlalchemy.orm import Session from models import snippet as snippet_module @@ -18,24 +17,15 @@ APP_ID = "33333333-3333-3333-3333-333333333333" SNIPPET_ID = "44444444-4444-4444-4444-444444444444" ACCOUNT_1_ID = "55555555-5555-5555-5555-555555555555" ACCOUNT_2_ID = "55555555-5555-5555-5555-555555555556" -SQLITE_MODELS = (Workflow, Tag, TagBinding, Account) -@pytest.fixture -def snippet_session(sqlite_session: Session, monkeypatch: pytest.MonkeyPatch) -> Session: - """Expose the shared SQLite session to model properties that use the global Flask session.""" - monkeypatch.setattr(snippet_module.db, "session", sqlite_session) - return sqlite_session - - -def test_graph_dict_returns_empty_without_workflow_id() -> None: +def test_get_graph_dict_returns_empty_without_workflow_id(sqlite_session: Session) -> None: snippet = CustomizedSnippet(workflow_id=None) - assert snippet.graph_dict == {} + assert snippet.get_graph_dict(session=sqlite_session) == {} -@pytest.mark.parametrize("sqlite_session", [SQLITE_MODELS], indirect=True) -def test_graph_dict_loads_published_workflow_graph(snippet_session: Session) -> None: +def test_get_graph_dict_loads_published_workflow_graph(sqlite_session: Session) -> None: workflow = Workflow( tenant_id=TENANT_ID, app_id=APP_ID, @@ -46,18 +36,17 @@ def test_graph_dict_loads_published_workflow_graph(snippet_session: Session) -> created_by=ACCOUNT_1_ID, ) workflow.id = WORKFLOW_ID - snippet_session.add(workflow) - snippet_session.commit() + sqlite_session.add(workflow) + sqlite_session.commit() snippet = CustomizedSnippet(workflow_id=WORKFLOW_ID) - assert snippet.graph_dict == {"nodes": [{"id": "llm-1"}], "edges": []} + assert snippet.get_graph_dict(session=sqlite_session) == {"nodes": [{"id": "llm-1"}], "edges": []} -@pytest.mark.parametrize("sqlite_session", [SQLITE_MODELS], indirect=True) -def test_graph_dict_returns_empty_when_workflow_missing(snippet_session: Session) -> None: +def test_get_graph_dict_returns_empty_when_workflow_missing(sqlite_session: Session) -> None: snippet = CustomizedSnippet(workflow_id=WORKFLOW_ID) - assert snippet.graph_dict == {} + assert snippet.get_graph_dict(session=sqlite_session) == {} def test_input_fields_list_parses_json_or_returns_empty() -> None: @@ -67,45 +56,50 @@ def test_input_fields_list_parses_json_or_returns_empty() -> None: ] -@pytest.mark.parametrize("sqlite_session", [SQLITE_MODELS], indirect=True) -def test_tags_returns_query_results_or_empty(snippet_session: Session) -> None: +def test_get_tags_returns_query_results_or_empty(sqlite_session: Session) -> None: tag = Tag(tenant_id=TENANT_ID, type=TagType.SNIPPET, name="Reusable", created_by=ACCOUNT_1_ID) binding = TagBinding(tenant_id=TENANT_ID, tag_id=tag.id, target_id=SNIPPET_ID, created_by=ACCOUNT_1_ID) - snippet_session.add_all((tag, binding)) - snippet_session.commit() + sqlite_session.add_all((tag, binding)) + sqlite_session.commit() snippet = CustomizedSnippet(id=SNIPPET_ID, tenant_id=TENANT_ID) - assert snippet.tags == [tag] + assert snippet.get_tags(session=sqlite_session) == [tag] - snippet_session.delete(binding) - snippet_session.commit() - assert snippet.tags == [] + sqlite_session.delete(binding) + sqlite_session.commit() + assert snippet.get_tags(session=sqlite_session) == [] -@pytest.mark.parametrize("sqlite_session", [SQLITE_MODELS], indirect=True) -def test_account_properties_and_author_name(snippet_session: Session) -> None: +def test_get_account_accessors_and_author_name(sqlite_session: Session) -> None: account = Account(name="Ada", email="ada@example.com") account.id = ACCOUNT_1_ID updated_account = Account(name="Grace", email="grace@example.com") updated_account.id = ACCOUNT_2_ID - snippet_session.add_all((account, updated_account)) - snippet_session.commit() + sqlite_session.add_all((account, updated_account)) + sqlite_session.commit() snippet = CustomizedSnippet(created_by=ACCOUNT_1_ID, updated_by=ACCOUNT_2_ID) - assert snippet.created_by_account is account - assert snippet.author_name == "Ada" - assert snippet.updated_by_account is updated_account + assert snippet.get_created_by_account(session=sqlite_session) is account + assert snippet.get_author_name(session=sqlite_session) == "Ada" + assert snippet.get_updated_by_account(session=sqlite_session) is updated_account -def test_account_properties_return_none_without_account_ids() -> None: +def test_get_account_accessors_return_none_without_account_ids(sqlite_session: Session) -> None: snippet = CustomizedSnippet(created_by=None, updated_by=None) - assert snippet.created_by_account is None - assert snippet.author_name is None - assert snippet.updated_by_account is None + assert snippet.get_created_by_account(session=sqlite_session) is None + assert snippet.get_author_name(session=sqlite_session) is None + assert snippet.get_updated_by_account(session=sqlite_session) is None def test_version_str_returns_string_value() -> None: snippet = CustomizedSnippet(version=7) assert snippet.version_str == "7" + + +def test_session_backed_lookups_are_not_exposed_as_properties() -> None: + """Callers must pass a session; the model no longer reads the Flask-global ``db.session``.""" + assert not hasattr(snippet_module, "db") + for name in ("graph_dict", "tags", "created_by_account", "author_name", "updated_by_account"): + assert not hasattr(CustomizedSnippet, name)