From 7a47cf730a0b783ba4fa87db266d48d3498aab19 Mon Sep 17 00:00:00 2001 From: Eugen Ciur Date: Tue, 14 Oct 2025 07:03:54 +0200 Subject: [PATCH 1/5] document types: fix failing tests --- papermerge/core/dbapi.py | 4 +- papermerge/core/features/conftest.py | 86 +++++++---- .../core/features/document_types/db/api.py | 146 +++++++----------- .../core/features/document_types/router.py | 12 +- .../tests/test_document_type.py | 8 +- .../tests/test_document_type_dbapi.py | 15 +- papermerge/core/features/ownership/db/api.py | 16 +- 7 files changed, 133 insertions(+), 154 deletions(-) diff --git a/papermerge/core/dbapi.py b/papermerge/core/dbapi.py index b1a4b2ec1..a46f542dc 100644 --- a/papermerge/core/dbapi.py +++ b/papermerge/core/dbapi.py @@ -49,7 +49,7 @@ get_document_types, get_document_type, get_document_types_without_pagination, - get_document_types_grouped_by_owner_without_pagination, + get_document_types_by_owner_without_pagination, delete_document_type, update_document_type, document_type_cf_count @@ -95,7 +95,7 @@ "get_document_types", "get_document_type", "get_document_types_without_pagination", - "get_document_types_grouped_by_owner_without_pagination", + "get_document_types_by_owner_without_pagination", "delete_document_type", "update_document_type", "document_type_cf_count", diff --git a/papermerge/core/features/conftest.py b/papermerge/core/features/conftest.py index 6e5503dcb..5d93fd158 100644 --- a/papermerge/core/features/conftest.py +++ b/papermerge/core/features/conftest.py @@ -16,7 +16,8 @@ from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload -from core.utils.tz import utc_now +from papermerge.core import schema +from papermerge.core.utils.tz import utc_now from papermerge.core.tests.resource_file import ResourceFile from papermerge.core.types import OCRStatusEnum from papermerge.core.features.auth.scopes import SCOPES @@ -550,16 +551,38 @@ async def document_type_groceries(db_session: AsyncSession, user, make_custom_fi @pytest.fixture def make_document_type_without_cf(db_session: AsyncSession, user, make_custom_field): async def _make_document_type(name: str): + create_data = schema.CreateDocumentType( + name=name, + custom_field_ids=[], + owner_type=OwnerType.USER, + owner_id=user.id + ) return await dbapi.create_document_type( db_session, - name=name, - custom_field_ids=[], # no custom fields - user_id=user.id, + data=create_data ) return _make_document_type +@pytest.fixture +async def document_type_zdf(db_session: AsyncSession, user, make_custom_field_v2): + cf1 = await make_custom_field_v2(name="Start Date", type_handler=CustomFieldType.date) + cf2 = await make_custom_field_v2(name="End Date", type_handler=CustomFieldType.date) + cf3 = await make_custom_field_v2(name="Total Due", type_handler=CustomFieldType.monetary) + + create_data = schema.CreateDocumentType( + name="ZDF", + custom_field_ids=[cf1.id, cf2.id, cf3.id], + owner_type=OwnerType.USER, + owner_id=user.id + ) + + return await dbapi.create_document_type( + db_session, + data=create_data + ) + @pytest.fixture def make_document_zdf(db_session: AsyncSession, document_type_zdf): @@ -685,10 +708,10 @@ def make_document_type(db_session, user): UPDATED: Create document type with ownership """ async def _maker( - name: str, - custom_fields: list = None, - user: orm.User | None = None, - group_id: UUID | None = None + name: str, + custom_fields: list = None, + user: orm.User | None = None, + group_id: UUID | None = None ): if custom_fields is None: custom_fields = [] @@ -736,24 +759,46 @@ async def _maker( @pytest.fixture def make_document_receipt(db_session: AsyncSession, document_type_groceries): - async def _make_receipt(title: str, user: orm.User, parent=None): + async def _make_receipt( + title: str, + user: orm.User, + parent=None, + group_id: UUID | None = None + ): if parent is None: parent_id = user.home_folder_id else: parent_id = parent.id + # Determine owner + if group_id: + owner_type = OwnerType.GROUP + owner_id = group_id + else: + owner_type = OwnerType.USER + owner_id = user.id if user else user.id + doc_id = uuid.uuid4() doc = orm.Document( id=doc_id, ctype="document", title=title, - user=user, document_type_id=document_type_groceries.id, parent_id=parent_id, lang="deu", ) db_session.add(doc) + await db_session.flush() + + # Set ownership + await ownership_api.set_owner( + session=db_session, + resource_type=ResourceType.NODE, + resource_id=doc.id, + owner_type=owner_type, + owner_id=owner_id + ) await db_session.commit() @@ -898,27 +943,6 @@ def _generator(): return _generator -@pytest.fixture -def make_document_zdf(db_session: AsyncSession, document_type_zdf): - async def _make_receipt(title: str, user: orm.User): - doc = orm.Document( - id=uuid.uuid4(), - ctype="document", - title=title, - user=user, - document_type_id=document_type_zdf.id, - parent_id=user.home_folder_id, - ) - - db_session.add(doc) - - await db_session.commit() - - return doc - - return _make_receipt - - @pytest.fixture def make_custom_field_v2(db_session: AsyncSession, user): """ diff --git a/papermerge/core/features/document_types/db/api.py b/papermerge/core/features/document_types/db/api.py index 34646dfd1..abbb12754 100644 --- a/papermerge/core/features/document_types/db/api.py +++ b/papermerge/core/features/document_types/db/api.py @@ -1,12 +1,11 @@ import logging import math import uuid -from itertools import groupby from typing import Optional, Dict, Any from sqlalchemy.orm import selectinload, aliased from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy.exc import NoResultFound +from sqlalchemy.exc import NoResultFound, IntegrityError from sqlalchemy import select, func, and_, or_ from papermerge.core.db.exceptions import ResourceAccessDenied, \ @@ -14,8 +13,9 @@ from papermerge.core import schema, orm from papermerge.core import constants as const from papermerge.core.tasks import send_task -from papermerge.core.features.document_types import schema as dt_schema +from papermerge.core.features.ownership.db import api as ownership_api from papermerge.core.utils.tz import utc_now +from papermerge.core.types import ResourceType from .orm import DocumentType logger = logging.getLogger(__name__) @@ -46,63 +46,24 @@ async def get_document_types_without_pagination( return items -async def get_document_types_grouped_by_owner_without_pagination( +async def get_document_types_by_owner_without_pagination( db_session: AsyncSession, - user_id: uuid.UUID, -) -> list[dt_schema.GroupedDocumentType]: + owner: schema.Owner, +) -> list[orm.DocumentType]: """ - Returns all document types to which user has access to, grouped - by owner. Results are not paginated. + Returns all document types that belongs to given owner. + Results are not paginated. """ - subquery = select(orm.UserGroup.group_id).where( - orm.UserGroup.user_id == user_id - ) - stmt_base = ( - select( - DocumentType.id, - DocumentType.name, - DocumentType.user_id, - DocumentType.group_id, - orm.Group.name.label("group_name"), - ) - .select_from(DocumentType) - .join(orm.Group, orm.Group.id == DocumentType.group_id, isouter=True) - .order_by( - DocumentType.user_id, - DocumentType.group_id, - DocumentType.name.asc(), - ) + document_type_ids = await ownership_api.get_resources_by_owner( + db_session, owner_type=owner.owner_type, owner_id=owner.owner_id, + resource_type=ResourceType.DOCUMENT_TYPE ) - stmt = stmt_base.where( - or_( - DocumentType.user_id == user_id, - DocumentType.group_id.in_(subquery), - ) - ) - - db_document_types = await db_session.execute(stmt) - - def keyfunc(x): - if x.user_id: - return "My" - - return x.group_name - - results = [] - document_types = list(db_document_types) + stmt = select(orm.DocumentType).where(orm.DocumentType.id.in_(document_type_ids)) + result = (await db_session.execute(stmt)).all() - for key, group in groupby(document_types, keyfunc): - group_items = [] - for item in group: - group_items.append( - dt_schema.GroupedDocumentTypeItem(name=item.name, id=item.id) - ) - - results.append(dt_schema.GroupedDocumentType(name=key, items=group_items)) - - return results + return result async def get_document_types( @@ -297,29 +258,51 @@ async def document_type_cf_count(session: AsyncSession, document_type_id: uuid.U async def create_document_type( session: AsyncSession, - name: str, - user_id: uuid.UUID | None = None, - group_id: uuid.UUID | None = None, - custom_field_ids: list[uuid.UUID] | None = None, - path_template: str | None = None, + data: schema.CreateDocumentType ) -> DocumentType: - if custom_field_ids is None: + + is_unique = await ownership_api.check_name_unique_for_owner( + session=session, + resource_type=ResourceType.CUSTOM_FIELD, + owner_type=data.owner_type, + owner_id=data.owner_id, + name=data.name + ) + + if not is_unique: + raise ValueError( + f"A Document type named '{data.name}' already exists for this {data.owner_type.value}" + ) + + if data.custom_field_ids is None: cf_ids = [] else: - cf_ids = custom_field_ids + cf_ids = data.custom_field_ids stmt = select(orm.CustomField).where(orm.CustomField.id.in_(cf_ids)) custom_fields = (await session.execute(stmt)).scalars().all() dtype = DocumentType( id=uuid.uuid4(), - name=name, + name=data.name, custom_fields=custom_fields, - path_template=path_template, - user_id=user_id, - group_id=group_id, + path_template=data.path_template, ) - session.add(dtype) - await session.commit() + + try: + session.add(dtype) + # Set ownership + await ownership_api.set_owner( + session=session, + resource_type=ResourceType.DOCUMENT_TYPE, + resource_id=dtype.id, + owner_type=data.owner_type, + owner_id=data.owner_id + ) + await session.commit() + except IntegrityError as e: + await session.rollback() + raise ValueError(f"Failed to create document type: {str(e)}") + return dtype @@ -486,7 +469,7 @@ async def delete_document_type( document_type_id: uuid.UUID ): """ - Soft delete a document type with access control and dependency validation. + Soft delete a document type Prevents deletion if: - Document type has associated custom fields (non-deleted) @@ -494,7 +477,7 @@ async def delete_document_type( Args: session: Database session - user_id: Current user ID (for access control) + user_id: Current user ID document_type_id: ID of the document type to delete Raises: @@ -511,32 +494,13 @@ async def delete_document_type( if not exists_result.scalar(): raise NoResultFound(f"Document type with id {document_type_id} not found") - # Subquery to get user's group IDs (for access control) - user_groups_subquery = select(orm.UserGroup.group_id).where( - orm.UserGroup.user_id == user_id - ) - - # Check if user has access to this document type - access_stmt = ( + stmt = ( select(DocumentType) - .where( - and_( - DocumentType.id == document_type_id, - # Access control: user can delete if they own it directly or through group - or_( - DocumentType.user_id == user_id, - DocumentType.group_id.in_(user_groups_subquery) - ) - ) - ) + .where(DocumentType.id == document_type_id) ) - access_result = await session.execute(access_stmt) - document_type = access_result.scalars().first() - - # If no result, user doesn't have access (we know the document type exists) - if not document_type: - raise ResourceAccessDenied(f"User {user_id} does not have permission to delete document type {document_type_id}") + result = await session.execute(stmt) + document_type = result.scalars().first() # Check if already soft deleted if document_type.deleted_at is not None: diff --git a/papermerge/core/features/document_types/router.py b/papermerge/core/features/document_types/router.py index f62b6bf6f..5c8ed3de7 100644 --- a/papermerge/core/features/document_types/router.py +++ b/papermerge/core/features/document_types/router.py @@ -177,23 +177,13 @@ async def create_document_type( Required scope: `{scope}` """ - kwargs = { - "name": dtype.name, - "path_template": dtype.path_template, - "custom_field_ids": dtype.custom_field_ids, - } - if dtype.group_id: - kwargs["group_id"] = dtype.group_id - else: - kwargs["user_id"] = user.id - try: async with AsyncAuditContext( db_session, user_id=user.id, username=user.username ): - document_type = await dbapi.create_document_type(db_session, **kwargs) + document_type = await dbapi.create_document_type(db_session, data=dtype) except Exception as e: error_msg = str(e) if "UNIQUE constraint failed" in error_msg: diff --git a/papermerge/core/features/document_types/tests/test_document_type.py b/papermerge/core/features/document_types/tests/test_document_type.py index 8aae9b300..bf9146cd9 100644 --- a/papermerge/core/features/document_types/tests/test_document_type.py +++ b/papermerge/core/features/document_types/tests/test_document_type.py @@ -13,12 +13,14 @@ async def test_on_delete_document_type_which_has_docs_associated( should stay (they, documents, will have doc.document_type_id set to NULL) """ # Arrange - doc: orm.Document = await make_document_receipt(title="receipt.pdf", user=user) - user_id = doc.document_type.user_id + doc: orm.Document = await make_document_receipt( + title="receipt.pdf", + user=user + ) with pytest.raises(DependenciesExist): await dbapi.delete_document_type( db_session, - user_id=user_id, + user_id=user.id, document_type_id=doc.document_type_id ) diff --git a/papermerge/core/features/document_types/tests/test_document_type_dbapi.py b/papermerge/core/features/document_types/tests/test_document_type_dbapi.py index 23ef7a50e..9cae30e12 100644 --- a/papermerge/core/features/document_types/tests/test_document_type_dbapi.py +++ b/papermerge/core/features/document_types/tests/test_document_type_dbapi.py @@ -1,9 +1,11 @@ from sqlalchemy.ext.asyncio import AsyncSession -from papermerge.core import orm, dbapi +from core.types import OwnerType +from papermerge.core import orm, schema +from papermerge.core.features.document_types.db import api as dt_dbapi -async def test_get_document_types_grouped_by_owner_without_pagination( +async def test_get_document_types_by_owner_without_pagination( db_session: AsyncSession, make_document_type, user, make_group ): family: orm.Group = await make_group("Family") @@ -11,13 +13,14 @@ async def test_get_document_types_grouped_by_owner_without_pagination( await make_document_type(name="Bills", group_id=family.id) await make_document_type(name="My Private", user=user) - # Create UserGroup relationship instead of appending to non-existent groups user_group = orm.UserGroup(user_id=user.id, group_id=family.id) db_session.add(user_group) await db_session.commit() - results = await dbapi.get_document_types_grouped_by_owner_without_pagination( - db_session, user_id=user.id + owner = schema.Owner(owner_type=OwnerType.GROUP, owner_id=family.id) + + results = await dt_dbapi.get_document_types_by_owner_without_pagination( + db_session, owner=owner ) assert len(results) == 2 @@ -26,7 +29,7 @@ async def test_get_document_types_grouped_by_owner_without_pagination( async def test_document_type_cf_count(db_session: AsyncSession, make_document_zdf, user): zdf_doc_instance: orm.Document = await make_document_zdf(title="ZDF Title", user=user) - cf_count = await dbapi.document_type_cf_count( + cf_count = await dt_dbapi.document_type_cf_count( db_session, document_type_id=zdf_doc_instance.document_type_id ) diff --git a/papermerge/core/features/ownership/db/api.py b/papermerge/core/features/ownership/db/api.py index 8c320195b..833d4f185 100644 --- a/papermerge/core/features/ownership/db/api.py +++ b/papermerge/core/features/ownership/db/api.py @@ -294,8 +294,6 @@ async def get_resources_by_owner( owner_type: OwnerType, owner_id: UUID, resource_type: ResourceType | None = None, - limit: int = 100, - offset: int = 0 ) -> list[UUID]: """ Get all resource IDs owned by a specific owner. @@ -314,8 +312,6 @@ async def get_resources_by_owner( if resource_type: stmt = stmt.where(Ownership.resource_type == resource_type.value) - stmt = stmt.limit(limit).offset(offset) - result = await session.execute(stmt) return [row[0] for row in result] @@ -492,12 +488,12 @@ async def delete_node_with_ownership( async def get_nodes_for_owner( - session: AsyncSession, - owner_type: OwnerType, - owner_id: UUID, - parent_id: UUID | None = None, - limit: int = 100, - offset: int = 0 + session: AsyncSession, + owner_type: OwnerType, + owner_id: UUID, + parent_id: UUID | None = None, + limit: int = 100, + offset: int = 0 ) -> list[orm.Node]: """ Get all nodes owned by a specific owner. From c9c6f4cb1b7d4a5463f83520842a11e30ff56091 Mon Sep 17 00:00:00 2001 From: Eugen Ciur Date: Tue, 14 Oct 2025 07:56:12 +0200 Subject: [PATCH 2/5] document types: fix tests --- papermerge/core/features/conftest.py | 5 +- .../core/features/document_types/db/api.py | 57 ++++++++++++------- .../core/features/document_types/router.py | 44 +++++++------- .../core/features/document_types/schema.py | 11 ++-- .../tests/test_router_document_types.py | 21 +++++-- papermerge/core/schema.py | 3 +- 6 files changed, 83 insertions(+), 58 deletions(-) diff --git a/papermerge/core/features/conftest.py b/papermerge/core/features/conftest.py index 5d93fd158..e95d3bdb0 100644 --- a/papermerge/core/features/conftest.py +++ b/papermerge/core/features/conftest.py @@ -710,13 +710,13 @@ def make_document_type(db_session, user): async def _maker( name: str, custom_fields: list = None, + path_template: str | None = None, user: orm.User | None = None, group_id: UUID | None = None ): if custom_fields is None: custom_fields = [] - # Determine owner if group_id: owner_type = OwnerType.GROUP owner_id = group_id @@ -724,15 +724,14 @@ async def _maker( owner_type = OwnerType.USER owner_id = user.id if user else user.id - # Create document type WITHOUT user_id/group_id dt = orm.DocumentType( id=uuid.uuid4(), + path_template=path_template, name=name ) db_session.add(dt) await db_session.flush() - # Set ownership await ownership_api.set_owner( session=db_session, resource_type=ResourceType.DOCUMENT_TYPE, diff --git a/papermerge/core/features/document_types/db/api.py b/papermerge/core/features/document_types/db/api.py index abbb12754..f55fccf61 100644 --- a/papermerge/core/features/document_types/db/api.py +++ b/papermerge/core/features/document_types/db/api.py @@ -9,13 +9,13 @@ from sqlalchemy import select, func, and_, or_ from papermerge.core.db.exceptions import ResourceAccessDenied, \ - DependenciesExist, InvalidRequest + DependenciesExist from papermerge.core import schema, orm from papermerge.core import constants as const from papermerge.core.tasks import send_task from papermerge.core.features.ownership.db import api as ownership_api from papermerge.core.utils.tz import utc_now -from papermerge.core.types import ResourceType +from papermerge.core.types import ResourceType, OwnerType from .orm import DocumentType logger = logging.getLogger(__name__) @@ -259,11 +259,11 @@ async def document_type_cf_count(session: AsyncSession, document_type_id: uuid.U async def create_document_type( session: AsyncSession, data: schema.CreateDocumentType -) -> DocumentType: +) -> orm.DocumentType: is_unique = await ownership_api.check_name_unique_for_owner( session=session, - resource_type=ResourceType.CUSTOM_FIELD, + resource_type=ResourceType.DOCUMENT_TYPE, owner_type=data.owner_type, owner_id=data.owner_id, name=data.name @@ -299,6 +299,7 @@ async def create_document_type( owner_id=data.owner_id ) await session.commit() + await session.refresh(dtype) except IntegrityError as e: await session.rollback() raise ValueError(f"Failed to create document type: {str(e)}") @@ -565,7 +566,7 @@ async def update_document_type( session: AsyncSession, document_type_id: uuid.UUID, attrs: schema.UpdateDocumentType, -) -> schema.DocumentType: +) -> orm.DocumentType: """ Update a document type with proper validation. @@ -625,21 +626,35 @@ async def update_document_type( if attrs.name is not None: doc_type.name = attrs.name - # Update ownership - ensure mutual exclusivity - if attrs.group_id is not None and attrs.user_id is not None: - raise InvalidRequest("Cannot set both user_id and group_id - they are mutually exclusive") + if attrs.owner_type is not None and attrs.owner_id is not None: + owner_type = OwnerType(attrs.owner_type) + owner_id = attrs.owner_id + + # Check name uniqueness under new owner (if name wasn't changed above) + if attrs.name is None: + is_unique = await ownership_api.check_name_unique_for_owner( + session=session, + resource_type=ResourceType.DOCUMENT_TYPE, + owner_type=owner_type, + owner_id=owner_id, + name=doc_type.name, + exclude_id=doc_type.id + ) - if attrs.group_id is not None: - doc_type.user_id = None - doc_type.group_id = attrs.group_id - elif attrs.user_id is not None: - doc_type.user_id = attrs.user_id - doc_type.group_id = None - # If both are None, keep existing ownership + if not is_unique: + raise ValueError( + f"A custom document type '{doc_type.name}' already exists for the target {owner_type.value}" + ) + + # Update ownership + await ownership_api.set_owner( + session=session, + resource_type=ResourceType.DOCUMENT_TYPE, + resource_id=doc_type.id, + owner_type=owner_type, + owner_id=owner_id + ) - # Validate that at least one ownership field is set after update - if doc_type.user_id is None and doc_type.group_id is None: - raise InvalidRequest("Document type must have either user_id or group_id set") # Update path template and track if notification needed notify_path_tmpl_worker = False @@ -650,9 +665,7 @@ async def update_document_type( # Commit changes session.add(doc_type) await session.commit() - - # Convert to schema - result = schema.DocumentType.model_validate(doc_type) + await session.refresh(doc_type) # Send background task if path template changed if notify_path_tmpl_worker: @@ -664,7 +677,7 @@ async def update_document_type( route_name="path_tmpl", ) - return result + return doc_type def _build_document_type_filter_conditions( diff --git a/papermerge/core/features/document_types/router.py b/papermerge/core/features/document_types/router.py index 5c8ed3de7..5458c9683 100644 --- a/papermerge/core/features/document_types/router.py +++ b/papermerge/core/features/document_types/router.py @@ -8,13 +8,15 @@ from papermerge.core.db.exceptions import ResourceAccessDenied, \ DependenciesExist, InvalidRequest -from papermerge.core import utils, schema, dbapi +from papermerge.core import utils, dbapi, orm, schema from papermerge.core.features.auth import get_current_user from papermerge.core.features.auth import scopes from papermerge.core.routers.common import OPEN_API_GENERIC_JSON_DETAIL from papermerge.core.features.users import schema as users_schema from papermerge.core.features.document_types import schema as dt_schema from papermerge.core.db.engine import get_db +from papermerge.core.features.ownership.db import api as ownership_api +from papermerge.core.types import ResourceType from papermerge.core.features.audit.db.audit_context import AsyncAuditContext from .schema import DocumentTypeParams, DocumentTypeEx @@ -169,7 +171,7 @@ async def create_document_type( Security(get_current_user, scopes=[scopes.DOCUMENT_TYPE_CREATE]), ], db_session: AsyncSession = Depends(get_db), -) -> schema.DocumentType: +) -> schema.DocumentTypeShort: """Creates document type If attribute `group_id` is present, document type will be owned @@ -190,7 +192,7 @@ async def create_document_type( raise HTTPException(status_code=400, detail="Document type already exists") raise HTTPException(status_code=400, detail=error_msg) - return document_type + return schema.DocumentTypeShort.model_validate(document_type) @router.delete( @@ -257,7 +259,6 @@ async def delete_document_type( @router.patch( "/{document_type_id}", status_code=200, - response_model=schema.DocumentType, responses={ status.HTTP_403_FORBIDDEN: { "description": """User does not belong to group""", @@ -278,34 +279,31 @@ async def update_document_type( Security(get_current_user, scopes=[scopes.DOCUMENT_TYPE_UPDATE]), ], db_session: AsyncSession = Depends(get_db), -) -> schema.DocumentType: +) -> schema.DocumentTypeShort: """Updates document type Required scope: `{scope}` """ - try: - if attrs.group_id: - group_id = attrs.group_id - ok = await dbapi.user_belongs_to( - db_session, - user_id=cur_user.id, - group_id=group_id - ) - if not ok: - user_id = cur_user.id - detail = f"User {user_id=} does not belong to group {group_id=}" - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, detail=detail - ) - else: - attrs.user_id = cur_user.id + has_access = await ownership_api.user_can_access_resource( + session=db_session, + user_id=cur_user.id, + resource_type=ResourceType.DOCUMENT_TYPE, + resource_id=document_type_id + ) + if not has_access: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, # Use 404 to not leak existence + detail=f"{ResourceType.DOCUMENT_TYPE.value.replace('_', ' ').title()} not found" + ) + + try: async with AsyncAuditContext( db_session, user_id=cur_user.id, username=cur_user.username ): - dtype: schema.DocumentType = await dbapi.update_document_type( + dtype: orm.DocumentType = await dbapi.update_document_type( db_session, document_type_id=document_type_id, attrs=attrs, @@ -315,4 +313,4 @@ async def update_document_type( except InvalidRequest as e: raise HTTPException(status_code=400, detail=str(e)) - return dtype + return schema.DocumentTypeShort.model_validate(dtype) diff --git a/papermerge/core/features/document_types/schema.py b/papermerge/core/features/document_types/schema.py index 1a1a9aeae..b82416e5c 100644 --- a/papermerge/core/features/document_types/schema.py +++ b/papermerge/core/features/document_types/schema.py @@ -10,17 +10,20 @@ from papermerge.core.features.custom_fields.schema import CustomField -class DocumentType(BaseModel): +class DocumentTypeShort(BaseModel): id: UUID name: str path_template: str | None = None - custom_fields: list[CustomField] - owned_by: OwnedBy # Config model_config = ConfigDict(from_attributes=True) +class DocumentType(DocumentTypeShort): + custom_fields: list[CustomField] + owned_by: OwnedBy + + class DocumentTypeDetails(BaseModel): id: UUID name: str @@ -60,7 +63,7 @@ class UpdateDocumentType(BaseModel): name: str | None = None path_template: str | None = None custom_field_ids: list[UUID] | None = None - owner_type: OwnerType + owner_type: OwnerType | None = None class GroupedDocumentTypeItem(BaseModel): diff --git a/papermerge/core/features/document_types/tests/test_router_document_types.py b/papermerge/core/features/document_types/tests/test_router_document_types.py index 1f0f898df..b963949d2 100644 --- a/papermerge/core/features/document_types/tests/test_router_document_types.py +++ b/papermerge/core/features/document_types/tests/test_router_document_types.py @@ -1,13 +1,17 @@ from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession +from core.types import OwnerType from papermerge.core import schema, orm from papermerge.core.tests.types import AuthTestClient async def test_create_document_type_with_path_template( - auth_api_client: AuthTestClient, db_session: AsyncSession + auth_api_client: AuthTestClient, + db_session: AsyncSession, + make_user, ): + user = await make_user(username="momo") count_before = (await db_session.execute( select(func.count(orm.DocumentType.id)) )).scalar() @@ -19,6 +23,8 @@ async def test_create_document_type_with_path_template( "name": "Invoice", "path_template": "/home/My ZDF/", "custom_field_ids": [], + "owner_id": str(user.id), + "owner_type": OwnerType.USER }, ) assert response.status_code == 201, response.json() @@ -28,15 +34,18 @@ async def test_create_document_type_with_path_template( ).scalar() assert count_after == 1 - document_type = schema.DocumentType.model_validate(response.json()) + document_type = schema.DocumentTypeShort.model_validate(response.json()) assert document_type.name == "Invoice" assert document_type.path_template == "/home/My ZDF/" async def test_update_document_type_with_path_template( - make_document_type, auth_api_client: AuthTestClient, db_session: AsyncSession + make_document_type, + auth_api_client: AuthTestClient, + db_session: AsyncSession ): - doc_type = await make_document_type(name="ZDF", path_template="/home/") + user = auth_api_client.user + doc_type = await make_document_type(name="ZDF", path_template="/home/", user=user) response = await auth_api_client.patch( f"/document-types/{doc_type.id}", json={ @@ -46,7 +55,9 @@ async def test_update_document_type_with_path_template( }, ) - document_type = schema.DocumentType.model_validate(response.json()) + assert response.status_code == 200, response.json() + + document_type = schema.DocumentTypeShort.model_validate(response.json()) assert document_type.path_template == "/home/My ZDF/updated/" diff --git a/papermerge/core/schema.py b/papermerge/core/schema.py index 04fc86ded..8ebb6e514 100644 --- a/papermerge/core/schema.py +++ b/papermerge/core/schema.py @@ -48,7 +48,7 @@ UpdateCustomField, CustomFieldType, CustomFieldValue, ) from .features.tags.schema import Tag, TagShort, TagEx, TagDetails, UpdateTag, CreateTag -from .features.document_types.schema import DocumentType, DocumentTypeEx, DocumentTypeDetails, UpdateDocumentType, CreateDocumentType +from .features.document_types.schema import DocumentType, DocumentTypeShort, DocumentTypeEx, DocumentTypeDetails, UpdateDocumentType, CreateDocumentType from .features.groups.schema import Group, GroupEx, GroupDetails, CreateGroup, UpdateGroup from .features.roles.schema import Role, RoleEx, RoleDetails, CreateRole, UpdateRole, Permission from .features.audit.schema import AuditLog, AuditLogDetails @@ -117,6 +117,7 @@ 'MoveStrategy', 'PaginatedResponse', 'DocumentType', + 'DocumentTypeShort', 'DocumentTypeEx', 'DocumentTypeDetails', 'CreateDocumentType', From aef238b26d54f06169f35049639688283b5b185e Mon Sep 17 00:00:00 2001 From: Eugen Ciur Date: Tue, 14 Oct 2025 08:05:11 +0200 Subject: [PATCH 3/5] document types: fix tests --- .../tests/test_router_document_types.py | 45 +++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/papermerge/core/features/document_types/tests/test_router_document_types.py b/papermerge/core/features/document_types/tests/test_router_document_types.py index b963949d2..4909a7083 100644 --- a/papermerge/core/features/document_types/tests/test_router_document_types.py +++ b/papermerge/core/features/document_types/tests/test_router_document_types.py @@ -1,9 +1,10 @@ from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession -from core.types import OwnerType +from papermerge.core.types import OwnerType, ResourceType from papermerge.core import schema, orm from papermerge.core.tests.types import AuthTestClient +from papermerge.core.features.ownership.db import api as ownership_dbapi async def test_create_document_type_with_path_template( @@ -62,10 +63,13 @@ async def test_update_document_type_with_path_template( async def test_create_document_type_owned_by_user( - make_custom_field_v2, auth_api_client: AuthTestClient, db_session: AsyncSession + make_custom_field_v2, + auth_api_client: AuthTestClient, + db_session: AsyncSession ): cf1: schema.CustomField = await make_custom_field_v2(name="shop", type_handler="text") cf2: schema.CustomField = await make_custom_field_v2(name="total", type_handler="monetary") + user = auth_api_client.user count_before = (await db_session.execute( select(func.count(orm.DocumentType.id)) @@ -74,7 +78,12 @@ async def test_create_document_type_owned_by_user( response = await auth_api_client.post( "/document-types/", - json={"name": "Invoice", "custom_field_ids": [str(cf1.id), str(cf2.id)]}, + json={ + "name": "Invoice", + "custom_field_ids": [str(cf1.id), str(cf2.id)], + "owner_type": OwnerType.USER, + "owner_id": str(user.id) + }, ) assert response.status_code == 201, response.json() @@ -82,12 +91,17 @@ async def test_create_document_type_owned_by_user( count_after = (await db_session.execute(select(func.count(orm.DocumentType.id)))).scalar() assert count_after == 1 - document_type = schema.DocumentType.model_validate(response.json()) - db_document_type = await db_session.get(orm.DocumentType, document_type.id) + document_type = schema.DocumentTypeShort.model_validate(response.json()) + assert document_type.name == "Invoice" + owner_type, owner_id = await ownership_dbapi.get_owner_info( + db_session, + resource_type=ResourceType.DOCUMENT_TYPE, + resource_id=document_type.id + ) + assert document_type.name == "Invoice" - assert db_document_type.user_id == auth_api_client.user.id - assert len(document_type.custom_fields) == 2 - assert set([cf.name for cf in document_type.custom_fields]) == {"shop", "total"} + assert owner_type == OwnerType.USER + assert owner_id == user.id async def test_create_document_type_owned_by_group( @@ -110,7 +124,8 @@ async def test_create_document_type_owned_by_group( json={ "name": "Invoice", "custom_field_ids": [str(cf1.id), str(cf2.id)], - "group_id": str(group.id), + "owner_id": str(group.id), + "owner_type": OwnerType.GROUP }, ) @@ -119,12 +134,16 @@ async def test_create_document_type_owned_by_group( count_after = (await db_session.execute(select(func.count(orm.DocumentType.id)))).scalar() assert count_after == 1 - document_type = schema.DocumentType.model_validate(response.json()) - db_document_type = await db_session.get(orm.DocumentType, document_type.id) + document_type = schema.DocumentTypeShort.model_validate(response.json()) + owner_type, owner_id = await ownership_dbapi.get_owner_info( + db_session, + resource_type=ResourceType.DOCUMENT_TYPE, + resource_id=document_type.id + ) assert document_type.name == "Invoice" - assert db_document_type.user_id == None - assert db_document_type.group_id == group.id + assert owner_type == OwnerType.GROUP + assert owner_id == group.id async def test_list_document_types(make_document_type, auth_api_client: AuthTestClient): From 832e2e2a42befc42be5253de5499f138340db467 Mon Sep 17 00:00:00 2001 From: Eugen Ciur Date: Wed, 15 Oct 2025 06:36:50 +0200 Subject: [PATCH 4/5] document types: fix tests --- .../core/features/document_types/db/api.py | 124 +++++++++++------- .../core/features/document_types/router.py | 26 +++- .../core/features/document_types/schema.py | 1 + .../tests/test_router_document_types.py | 39 +++--- papermerge/core/features/ownership/schema.py | 12 ++ 5 files changed, 136 insertions(+), 66 deletions(-) diff --git a/papermerge/core/features/document_types/db/api.py b/papermerge/core/features/document_types/db/api.py index f55fccf61..c10973d75 100644 --- a/papermerge/core/features/document_types/db/api.py +++ b/papermerge/core/features/document_types/db/api.py @@ -6,7 +6,7 @@ from sqlalchemy.orm import selectinload, aliased from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.exc import NoResultFound, IntegrityError -from sqlalchemy import select, func, and_, or_ +from sqlalchemy import select, func, and_, or_, Sequence from papermerge.core.db.exceptions import ResourceAccessDenied, \ DependenciesExist @@ -23,27 +23,25 @@ async def get_document_types_without_pagination( db_session: AsyncSession, - user_id: uuid.UUID | None = None, - group_id: uuid.UUID | None = None, -) -> list[schema.DocumentType]: - stmt_base = select(DocumentType).options( - selectinload(orm.DocumentType.custom_fields) - ).order_by(DocumentType.name.asc()) - - if group_id: - stmt = stmt_base.where(DocumentType.group_id == group_id) - elif user_id: - stmt = stmt_base.where(DocumentType.user_id == user_id) - else: - raise ValueError("Both: group_id and user_id are missing") - - db_document_types = (await db_session.scalars(stmt)).all() - items = [ - schema.DocumentType.model_validate(db_document_type) - for db_document_type in db_document_types - ] + owner: schema.Owner, +) -> Sequence[orm.DocumentType]: + stmt = ( + select(orm.DocumentType) + .join( + orm.Ownership, + and_( + orm.Ownership.resource_type == ResourceType.DOCUMENT_TYPE.value, + orm.Ownership.resource_id == orm.DocumentType.id + ) + ) + .where( + orm.Ownership.owner_type == owner.owner_type, + orm.Ownership.owner_id == owner.owner_id + ) + .order_by(orm.DocumentType.name.asc()) + ) - return items + return (await db_session.execute(stmt)).scalars().all() async def get_document_types_by_owner_without_pagination( @@ -110,20 +108,51 @@ async def get_document_types( # Create user alias for owner owner_user = aliased(orm.User, name='owner_user') + owner_group = aliased(orm.Group, name='owner_group') # Build base query with joins for all audit user data and group/owner info base_query = ( select(orm.DocumentType) + .join( + orm.Ownership, + and_( + orm.Ownership.resource_type == ResourceType.DOCUMENT_TYPE.value, + orm.Ownership.resource_id == orm.DocumentType.id + ) + ) .join(created_user, orm.DocumentType.created_by == created_user.id, isouter=True) .join(updated_user, orm.DocumentType.updated_by == updated_user.id, isouter=True) - .join(orm.Group, orm.Group.id == orm.DocumentType.group_id, isouter=True) - .join(owner_user, orm.DocumentType.user_id == owner_user.id, isouter=True) + # Join to owner_user when owner is a user + .join( + owner_user, + and_( + orm.Ownership.owner_type == OwnerType.USER, + orm.Ownership.owner_id == owner_user.id + ), + isouter=True + ) + # Join to owner_group when owner is a group + .join( + owner_group, + and_( + orm.Ownership.owner_type == OwnerType.GROUP, + orm.Ownership.owner_id == owner_group.id + ), + isouter=True + ) + ) # Apply access control - user can see document types they own or from their groups access_control_condition = or_( - orm.DocumentType.user_id == user_id, - orm.DocumentType.group_id.in_(user_groups_subquery) + and_( + orm.Ownership.owner_type == OwnerType.USER, + orm.Ownership.owner_id == user_id + ), + and_( + orm.Ownership.owner_type == OwnerType.GROUP, + orm.Ownership.owner_id.in_(user_groups_subquery) + ) ) where_conditions = [access_control_condition] @@ -142,10 +171,15 @@ async def get_document_types( # Count total items (using the same filters) count_query = ( select(func.count(orm.DocumentType.id)) + .join( + orm.Ownership, + and_( + orm.Ownership.resource_type == ResourceType.DOCUMENT_TYPE.value, + orm.Ownership.resource_id == orm.DocumentType.id + ) + ) .join(created_user, orm.DocumentType.created_by == created_user.id, isouter=True) .join(updated_user, orm.DocumentType.updated_by == updated_user.id, isouter=True) - .join(orm.Group, orm.Group.id == orm.DocumentType.group_id, isouter=True) - .join(owner_user, orm.DocumentType.user_id == owner_user.id, isouter=True) .where(and_(*where_conditions)) ) @@ -169,9 +203,12 @@ async def get_document_types( paginated_query_with_users = ( base_query .add_columns( - # Group info - orm.Group.id.label('group_id'), - orm.Group.name.label('group_name'), + # Ownership info + orm.Ownership.owner_type.label('owner_type'), + orm.Ownership.owner_id.label('owner_id'), + # Owner group info + owner_group.id.label('owner_group_id'), # Changed from orm.Group + owner_group.name.label('owner_group_name'), # Changed from orm.Group # Owner user info owner_user.id.label('owner_user_id'), owner_user.username.label('owner_username'), @@ -194,6 +231,19 @@ async def get_document_types( for row in results: document_type = row[0] # The DocumentType object + if row.owner_type == OwnerType.USER: + owned_by = schema.OwnedBy( + id=row.owner_user_id, + name=row.owner_username, + type=OwnerType.USER + ) + elif row.owner_type == OwnerType.GROUP: + owned_by = schema.OwnedBy( + id=row.owner_group_id, + name=row.owner_group_name, + type=OwnerType.GROUP + ) + # Build audit user objects (handle None values) created_by = None if row.created_by_id: @@ -209,20 +259,6 @@ async def get_document_types( username=row.updated_by_username ) - # Determine owner based on which ID exists - if document_type.user_id: - owned_by = schema.OwnedBy( - id=document_type.user_id, - name=row.owner_username, - type="user" - ) - else: # group_id is not null (enforced by check constraint) - owned_by = schema.OwnedBy( - id=row.group_id, - name=row.group_name, - type="group" - ) - document_type_data = { "id": document_type.id, "name": document_type.name, diff --git a/papermerge/core/features/document_types/router.py b/papermerge/core/features/document_types/router.py index 5458c9683..b0ed1f227 100644 --- a/papermerge/core/features/document_types/router.py +++ b/papermerge/core/features/document_types/router.py @@ -30,7 +30,7 @@ @router.get( "/all", - response_model=list[schema.DocumentType], + response_model=list[schema.DocumentTypeShort], responses={ status.HTTP_403_FORBIDDEN: { "description": """User does not belong to group""", @@ -60,11 +60,18 @@ async def get_document_types_without_pagination( Required scope: `{scope}` """ + owner = schema.Owner.create_from( + user_id=user.id, + group_id=group_id + ) + result = await dbapi.get_document_types_without_pagination( - db_session, user_id=user.id, group_id=group_id + db_session, owner=owner ) - return result + items = [schema.DocumentTypeShort.model_validate(item) for item in result] + + return items @router.get( @@ -228,6 +235,19 @@ async def delete_document_type( Required scope: `{scope}` """ + has_access = await ownership_api.user_can_access_resource( + session=db_session, + user_id=user.id, + resource_type=ResourceType.DOCUMENT_TYPE, + resource_id=document_type_id + ) + + if not has_access: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, # Use 404 to not leak existence + detail=f"{ResourceType.DOCUMENT_TYPE.value.replace('_', ' ').title()} not found" + ) + try: async with AsyncAuditContext( db_session, diff --git a/papermerge/core/features/document_types/schema.py b/papermerge/core/features/document_types/schema.py index b82416e5c..57919e6bc 100644 --- a/papermerge/core/features/document_types/schema.py +++ b/papermerge/core/features/document_types/schema.py @@ -64,6 +64,7 @@ class UpdateDocumentType(BaseModel): path_template: str | None = None custom_field_ids: list[UUID] | None = None owner_type: OwnerType | None = None + owner_id: UUID | None = None class GroupedDocumentTypeItem(BaseModel): diff --git a/papermerge/core/features/document_types/tests/test_router_document_types.py b/papermerge/core/features/document_types/tests/test_router_document_types.py index 4909a7083..7dc4c4da3 100644 --- a/papermerge/core/features/document_types/tests/test_router_document_types.py +++ b/papermerge/core/features/document_types/tests/test_router_document_types.py @@ -147,8 +147,9 @@ async def test_create_document_type_owned_by_group( async def test_list_document_types(make_document_type, auth_api_client: AuthTestClient): - await make_document_type(name="Invoice") - response = await auth_api_client.get("/document-types/") + user = auth_api_client.user + await make_document_type(name="Invoice", user=user) + response = await auth_api_client.get("/document-types/all") assert response.status_code == 200, response.json() @@ -161,7 +162,8 @@ async def test_update_document_type( ): cf1 = await make_custom_field_v2(name="cf1", type_handler="text") cf2 = await make_custom_field_v2(name="cf2", type_handler="boolean") - doc_type = await make_document_type(name="Invoice") + user = auth_api_client.user + doc_type = await make_document_type(name="Invoice", user=user) response = await auth_api_client.patch( f"/document-types/{doc_type.id}", @@ -171,9 +173,8 @@ async def test_update_document_type( }, ) assert response.status_code == 200 - updated_dtype = schema.DocumentType(**response.json()) + updated_dtype = schema.DocumentTypeShort(**response.json()) assert updated_dtype.name == "Invoice-updated" - assert set([cf.name for cf in updated_dtype.custom_fields]) == {"cf1", "cf2"} async def test_update_group_id_field_in_document_type( @@ -187,9 +188,10 @@ async def test_update_group_id_field_in_document_type( DocumentType is owned by user and user can transfer ownership to the group. """ - doc_type = await make_document_type(name="Invoice") - group: orm.Group = await make_group("Familly", with_special_folders=True) user = auth_api_client.user + doc_type = await make_document_type(name="Invoice", user=user) + group: orm.Group = await make_group("Familly", with_special_folders=True) + user_group = orm.UserGroup(user=user, group=group) db_session.add(user_group) @@ -199,18 +201,17 @@ async def test_update_group_id_field_in_document_type( response = await auth_api_client.patch( f"/document-types/{doc_type.id}", json={ - "group_id": str(group.id), + "owner_id": str(group.id), + "owner_type": OwnerType.GROUP }, ) - result = await db_session.execute( - select(orm.DocumentType).where(orm.DocumentType.id == doc_type.id) - ) - fresh_document_type = result.scalar_one() assert response.status_code == 200 - assert fresh_document_type.group_id == group.id - assert fresh_document_type.user_id == None - + owner_type, owner_id = await ownership_dbapi.get_owner_info( + db_session, ResourceType.DOCUMENT_TYPE, resource_id=doc_type.id + ) + assert owner_type == OwnerType.GROUP + assert owner_id == group.id async def test_delete_document_type( auth_api_client: AuthTestClient, @@ -222,8 +223,8 @@ async def test_delete_document_type( assert count_before == 1 response = await auth_api_client.delete(f"/document-types/{doc_type.id}") - # cannot delete document type which has custom fields associated! - assert response.status_code == 409, response.json() + + assert response.status_code == 204, response.json() async def test_paginated_result__9_items_first_page( @@ -300,7 +301,7 @@ async def test_document_types_all_route( assert response.status_code == 200, response.json() - items = [schema.DocumentType(**kw) for kw in response.json()] + items = [schema.DocumentTypeShort(**kw) for kw in response.json()] assert len(items) == total_doc_type_items @@ -333,5 +334,5 @@ async def test__positive__document_types_all_route_with_group_id_param( ) assert response.status_code == 200, response.json() - dtype_names = {schema.DocumentType(**kw).name for kw in response.json()} + dtype_names = {schema.DocumentTypeShort(**kw).name for kw in response.json()} assert dtype_names == {"Research 1", "Research 2"} diff --git a/papermerge/core/features/ownership/schema.py b/papermerge/core/features/ownership/schema.py index a5245a693..39158b3a1 100644 --- a/papermerge/core/features/ownership/schema.py +++ b/papermerge/core/features/ownership/schema.py @@ -11,6 +11,18 @@ class Owner(BaseModel): owner_type: OwnerType owner_id: UUID + @staticmethod + def create_from( + user_id: UUID | None = None, + group_id: UUID | None = None + ) -> "Owner": + if group_id is not None: + return Owner(owner_type=OwnerType.GROUP, owner_id=group_id) + elif user_id is not None: + return Owner(owner_type=OwnerType.USER, owner_id=user_id) + else: + raise ValueError("Either user_id or group_id must be provided") + class OwnerInfo(BaseModel): """Basic owner information""" From 94b74412d5c22148a229c9ce5ef2284d0b29e164 Mon Sep 17 00:00:00 2001 From: Eugen Ciur Date: Wed, 15 Oct 2025 06:39:39 +0200 Subject: [PATCH 5/5] minor annotation fix --- papermerge/core/features/document_types/db/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/papermerge/core/features/document_types/db/api.py b/papermerge/core/features/document_types/db/api.py index c10973d75..c2155f42d 100644 --- a/papermerge/core/features/document_types/db/api.py +++ b/papermerge/core/features/document_types/db/api.py @@ -47,7 +47,7 @@ async def get_document_types_without_pagination( async def get_document_types_by_owner_without_pagination( db_session: AsyncSession, owner: schema.Owner, -) -> list[orm.DocumentType]: +) -> Sequence[orm.DocumentType]: """ Returns all document types that belongs to given owner. Results are not paginated.