From c93135b338006800657aec30f2036860d15ff31b Mon Sep 17 00:00:00 2001 From: Felix Martin Date: Wed, 10 Jun 2026 19:07:49 -0400 Subject: [PATCH] refactor: per-operation DB sessions in Project/Mcp services Co-Authored-By: Claude Fable 5 --- src/hqt/mcp/service.py | 80 ++++++++++++++++++++----------------- src/hqt/projects/service.py | 68 +++++++++++++++++-------------- tests/test_services.py | 58 +++++++++++++++++---------- 3 files changed, 118 insertions(+), 88 deletions(-) diff --git a/src/hqt/mcp/service.py b/src/hqt/mcp/service.py index 48fc8e5..2639400 100644 --- a/src/hqt/mcp/service.py +++ b/src/hqt/mcp/service.py @@ -1,13 +1,15 @@ import json -from sqlalchemy.orm import Session as DBSession +from sqlalchemy.orm import sessionmaker from hqt.db.models import McpServer, ProjectMcpServer class McpService: - def __init__(self, db: DBSession): - self.db = db + def __init__(self, factory: sessionmaker): + # factory must be created with expire_on_commit=False so rows returned + # from these methods stay readable after their session closes. + self.factory = factory def create( self, @@ -17,49 +19,53 @@ class McpService: args: list[str] | None = None, url: str | None = None, ) -> McpServer: - server = McpServer( - name=name, - transport=transport, - command=command, - args_json=json.dumps(args) if args else None, - url=url, - ) - self.db.add(server) - self.db.commit() - self.db.refresh(server) - return server + with self.factory() as db: + server = McpServer( + name=name, + transport=transport, + command=command, + args_json=json.dumps(args) if args else None, + url=url, + ) + db.add(server) + db.commit() + return server def list_all(self) -> list[McpServer]: - return list(self.db.query(McpServer).all()) + with self.factory() as db: + return list(db.query(McpServer).all()) def get(self, name: str) -> McpServer | None: - return self.db.query(McpServer).filter_by(name=name).first() + with self.factory() as db: + return db.query(McpServer).filter_by(name=name).first() def delete(self, name: str) -> None: - server = self.get(name) - if server: - self.db.delete(server) - self.db.commit() + with self.factory() as db: + server = db.query(McpServer).filter_by(name=name).first() + if server: + db.delete(server) + db.commit() def bind_to_project(self, project_id: int, mcp_server_id: int) -> None: - self.db.add( - ProjectMcpServer(project_id=project_id, mcp_server_id=mcp_server_id) - ) - self.db.commit() + with self.factory() as db: + db.add(ProjectMcpServer(project_id=project_id, mcp_server_id=mcp_server_id)) + db.commit() def unbind_from_project(self, project_id: int, mcp_server_id: int) -> None: - self.db.query(ProjectMcpServer).filter_by( - project_id=project_id, mcp_server_id=mcp_server_id - ).delete() - self.db.commit() + with self.factory() as db: + db.query(ProjectMcpServer).filter_by( + project_id=project_id, mcp_server_id=mcp_server_id + ).delete() + db.commit() def get_project_mcps(self, project_id: int) -> list[McpServer]: - ids = [ - r.mcp_server_id - for r in self.db.query(ProjectMcpServer) - .filter_by(project_id=project_id) - .all() - ] - if not ids: - return [] - return list(self.db.query(McpServer).filter(McpServer.id.in_(ids)).all()) + with self.factory() as db: + ids = [ + r.mcp_server_id + for r in db.query(ProjectMcpServer) + .filter_by(project_id=project_id) + .all() + ] + if not ids: + return [] + return list(db.query(McpServer).filter(McpServer.id.in_(ids)).all()) diff --git a/src/hqt/projects/service.py b/src/hqt/projects/service.py index 608a94f..cfd2286 100644 --- a/src/hqt/projects/service.py +++ b/src/hqt/projects/service.py @@ -1,45 +1,55 @@ from sqlalchemy.exc import IntegrityError -from sqlalchemy.orm import Session as DBSession +from sqlalchemy.orm import sessionmaker from hqt.db.models import Project +from hqt.errors import ServiceError class ProjectService: - def __init__(self, db: DBSession): - self.db = db + def __init__(self, factory: sessionmaker): + # factory must be created with expire_on_commit=False so rows returned + # from these methods stay readable after their session closes. + self.factory = factory def create(self, name: str, path: str) -> Project: - project = Project(name=name, path=path) - self.db.add(project) - self.db.commit() - self.db.refresh(project) - return project + with self.factory() as db: + project = Project(name=name, path=path) + db.add(project) + try: + db.commit() + except IntegrityError as err: + db.rollback() + raise ServiceError(f"Another project already uses path {path}") from err + return project def list_all(self, include_archived: bool = False) -> list[Project]: - q = self.db.query(Project) - if not include_archived: - q = q.filter_by(archived=False) - return list(q.all()) + with self.factory() as db: + q = db.query(Project) + if not include_archived: + q = q.filter_by(archived=False) + return list(q.all()) def get(self, project_id: int) -> Project | None: - return self.db.get(Project, project_id) + with self.factory() as db: + return db.get(Project, project_id) def update(self, project_id: int, name: str, path: str) -> Project: - project = self.get(project_id) - if project is None: - raise ValueError(f"Project {project_id} not found") - project.name = name - project.path = path - try: - self.db.commit() - except IntegrityError as err: - self.db.rollback() - raise ValueError(f"Another project already uses path {path}") from err - self.db.refresh(project) - return project + with self.factory() as db: + project = db.get(Project, project_id) + if project is None: + raise ServiceError(f"Project {project_id} not found") + project.name = name + project.path = path + try: + db.commit() + except IntegrityError as err: + db.rollback() + raise ServiceError(f"Another project already uses path {path}") from err + return project def archive(self, project_id: int) -> None: - project = self.get(project_id) - if project: - project.archived = True - self.db.commit() + with self.factory() as db: + project = db.get(Project, project_id) + if project: + project.archived = True + db.commit() diff --git a/tests/test_services.py b/tests/test_services.py index 58b2bfc..989c7da 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -1,62 +1,76 @@ import pytest from sqlalchemy import create_engine -from sqlalchemy.orm import Session +from sqlalchemy.orm import sessionmaker +from sqlalchemy.pool import StaticPool from hqt.db.models import Base +from hqt.errors import ServiceError from hqt.mcp.service import McpService from hqt.projects.service import ProjectService @pytest.fixture -def db(): - engine = create_engine("sqlite:///:memory:") +def factory(): + # StaticPool + check_same_thread=False: every session created by the + # factory shares the single in-memory connection, so per-operation + # sessions all see the same database. + engine = create_engine( + "sqlite://", + connect_args={"check_same_thread": False}, + poolclass=StaticPool, + ) Base.metadata.create_all(engine) - with Session(engine) as session: - yield session + return sessionmaker(bind=engine, expire_on_commit=False) class TestProjectService: - def test_create_and_get(self, db): - svc = ProjectService(db) + def test_create_and_get(self, factory): + svc = ProjectService(factory) p = svc.create("test", "/tmp/test") assert p.id is not None assert svc.get(p.id).name == "test" - def test_list_excludes_archived(self, db): - svc = ProjectService(db) + def test_list_excludes_archived(self, factory): + svc = ProjectService(factory) svc.create("a", "/a") p2 = svc.create("b", "/b") svc.archive(p2.id) assert len(svc.list_all()) == 1 assert len(svc.list_all(include_archived=True)) == 2 - def test_update_name_and_path(self, db): - svc = ProjectService(db) + def test_update_name_and_path(self, factory): + svc = ProjectService(factory) p = svc.create("old", "/old/path") updated = svc.update(p.id, "new", "/new/path") assert updated.name == "new" assert updated.path == "/new/path" assert svc.get(p.id).path == "/new/path" - def test_update_duplicate_path_raises(self, db): - svc = ProjectService(db) + def test_update_duplicate_path_raises(self, factory): + svc = ProjectService(factory) svc.create("a", "/a") p2 = svc.create("b", "/b") - with pytest.raises(ValueError, match="already uses path"): + with pytest.raises(ServiceError, match="already uses path"): svc.update(p2.id, "b", "/a") # DB session must remain usable after rollback, values unchanged assert svc.get(p2.id).path == "/b" assert svc.get(p2.id).name == "b" - def test_update_unknown_id_raises(self, db): - svc = ProjectService(db) - with pytest.raises(ValueError, match="not found"): + def test_update_unknown_id_raises(self, factory): + svc = ProjectService(factory) + with pytest.raises(ServiceError, match="not found"): svc.update(9999, "x", "/x") + def test_create_duplicate_path_raises_service_error(self, factory): + svc = ProjectService(factory) + svc.create("a", "/dup") + with pytest.raises(ServiceError, match="already uses path"): + svc.create("b", "/dup") + class TestMcpService: - def test_crud(self, db): - svc = McpService(db) + def test_crud(self, factory): + svc = McpService(factory) server = svc.create("test-mcp", "stdio", command="node", args=["server.js"]) assert server.id is not None assert svc.get("test-mcp") is not None @@ -64,9 +78,9 @@ class TestMcpService: svc.delete("test-mcp") assert svc.get("test-mcp") is None - def test_bind_unbind(self, db): - psvc = ProjectService(db) - msvc = McpService(db) + def test_bind_unbind(self, factory): + psvc = ProjectService(factory) + msvc = McpService(factory) project = psvc.create("proj", "/proj") server = msvc.create("s1", "stdio", command="cmd") msvc.bind_to_project(project.id, server.id)