diff --git a/.mypy-baseline.txt b/.mypy-baseline.txt index ef9043fd8..853090d3b 100644 --- a/.mypy-baseline.txt +++ b/.mypy-baseline.txt @@ -38,7 +38,6 @@ src/documents/bulk_edit.py:0: error: Incompatible types in assignment (expressio src/documents/bulk_edit.py:0: error: Invalid index type "str" for "dict[FieldDataType, str]"; expected type "FieldDataType" [index] src/documents/bulk_edit.py:0: error: List comprehension has incompatible type List[tuple[int, Any]]; expected List[int] [misc] src/documents/bulk_edit.py:0: error: List comprehension has incompatible type List[tuple[int, None]]; expected List[int] [misc] -src/documents/bulk_edit.py:0: error: Missing named argument "p" for "remove" of "PageList" [call-arg] src/documents/bulk_edit.py:0: error: Missing type arguments for generic type "dict" [type-arg] src/documents/bulk_edit.py:0: error: Missing type arguments for generic type "dict" [type-arg] src/documents/bulk_edit.py:0: error: Need type annotation for "to_create" (hint: "to_create: list[] = ...") [var-annotated] diff --git a/.pyrefly-baseline.json b/.pyrefly-baseline.json index 9305cfe00..3c09a1099 100644 --- a/.pyrefly-baseline.json +++ b/.pyrefly-baseline.json @@ -91,13 +91,6 @@ "concise_description": "Argument `list[int]` is not assignable to parameter `args` with type `tuple[Any, ...] | None` in function `celery.app.task.Task.apply_async`", "severity": "error" }, - { - "column": 33, - "path": "src/documents/bulk_edit.py", - "name": "missing-argument", - "concise_description": "Missing argument `p` in function `pikepdf._core.PageList.remove`", - "severity": "error" - }, { "column": 25, "path": "src/documents/caching.py", diff --git a/src/documents/bulk_edit.py b/src/documents/bulk_edit.py index ec0d77637..a0f0050c7 100644 --- a/src/documents/bulk_edit.py +++ b/src/documents/bulk_edit.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging import tempfile import uuid +from functools import partial from pathlib import Path from typing import TYPE_CHECKING from typing import Literal @@ -23,6 +24,7 @@ from django.db.models import When from django.db.models.functions import Coalesce from django.utils import timezone +from documents import pdf_ops from documents.data_models import ConsumableDocument from documents.data_models import DocumentMetadataOverrides from documents.data_models import DocumentSource @@ -123,6 +125,11 @@ def _resolve_root_and_source_doc( ) +def _scratch_path(name: str) -> Path: + """A path inside a fresh directory under SCRATCH_DIR.""" + return Path(tempfile.mkdtemp(dir=settings.SCRATCH_DIR)) / name + + def set_correspondent( doc_ids: list[int], correspondent: Correspondent, @@ -499,8 +506,6 @@ def rotate( pair = _resolve_root_and_source_doc(doc, source_mode=source_mode) docs_by_root_id.setdefault(pair.root_doc.id, pair) - import pikepdf - for pair in docs_by_root_id.values(): if pair.source_doc.mime_type != "application/pdf": logger.warning( @@ -513,11 +518,7 @@ def rotate( Path(tempfile.mkdtemp(dir=settings.SCRATCH_DIR)) / f"{pair.root_doc.id}_rotated.pdf" ) - with pikepdf.open(pair.source_doc.source_path) as pdf: - for page in pdf.pages: - page.rotate(degrees, relative=True) - pdf.remove_unreferenced_resources() - pdf.save(filepath) + pdf_ops.rotate_pdf(pair.source_doc.source_path, filepath, degrees) # Preserve metadata/permissions via overrides; mark as new version overrides = DocumentMetadataOverrides().from_document(pair.root_doc) @@ -560,48 +561,45 @@ def merge( qs = Document.objects.select_related("root_document").filter(id__in=doc_ids) docs_by_id = {doc.id: doc for doc in qs} affected_docs: list[int] = [] - import pikepdf - - merged_pdf = pikepdf.new() - version: str = merged_pdf.pdf_version handoff_asn: int | None = None - # use doc_ids to preserve order - for doc_id in doc_ids: - doc = docs_by_id.get(doc_id) - if doc is None: - continue - pair = _resolve_root_and_source_doc(doc, source_mode=source_mode) - try: - doc_path = ( - pair.source_doc.archive_path - if archive_fallback - and pair.source_doc.mime_type != "application/pdf" - and pair.source_doc.has_archive_version - else pair.source_doc.source_path - ) - with pikepdf.open(str(doc_path)) as pdf: - version = max(version, pdf.pdf_version) - merged_pdf.pages.extend(pdf.pages) - affected_docs.append(doc.id) - if handoff_asn is None and doc.archive_serial_number is not None: - handoff_asn = doc.archive_serial_number - except Exception as e: - logger.exception( - f"Error merging document {doc.id}, it will not be included in the merge: {e}", - ) - if len(affected_docs) == 0: - logger.warning("No documents were merged") - return "OK" + with pdf_ops.PdfMerger() as merger: + # use doc_ids to preserve order + for doc_id in doc_ids: + doc = docs_by_id.get(doc_id) + if doc is None: + continue + pair = _resolve_root_and_source_doc(doc, source_mode=source_mode) + try: + # archive_path is None when there is no archive version + archive_path = ( + pair.source_doc.archive_path + if archive_fallback + and pair.source_doc.mime_type != "application/pdf" + else None + ) + merger.add( + archive_path + if archive_path is not None + else pair.source_doc.source_path, + ) + affected_docs.append(doc.id) + if handoff_asn is None and doc.archive_serial_number is not None: + handoff_asn = doc.archive_serial_number + except Exception as e: + logger.exception( + f"Error merging document {doc.id}, it will not be included in the merge: {e}", + ) + if len(affected_docs) == 0: + logger.warning("No documents were merged") + return "OK" - filepath = ( - Path( - tempfile.mkdtemp(dir=settings.SCRATCH_DIR), + filepath = ( + Path( + tempfile.mkdtemp(dir=settings.SCRATCH_DIR), + ) + / f"{'_'.join([str(doc_id) for doc_id in affected_docs])[:100]}_merged.pdf" ) - / f"{'_'.join([str(doc_id) for doc_id in affected_docs])[:100]}_merged.pdf" - ) - merged_pdf.remove_unreferenced_resources() - merged_pdf.save(filepath, min_version=version) - merged_pdf.close() + merger.save(filepath) if metadata_document_id: metadata_document = qs.get(id=metadata_document_id) @@ -777,64 +775,60 @@ def split( ) doc = Document.objects.select_related("root_document").get(id=doc_ids[0]) pair = _resolve_root_and_source_doc(doc, source_mode=source_mode) - import pikepdf consume_tasks = [] try: - with pikepdf.open(pair.source_doc.source_path) as pdf: - for idx, split_doc in enumerate(pages): - dst: pikepdf.Pdf = pikepdf.new() - for page in split_doc: - dst.pages.append(pdf.pages[page - 1]) - filepath: Path = ( - Path( - tempfile.mkdtemp(dir=settings.SCRATCH_DIR), - ) - / f"{doc.id}_{split_doc[0]}-{split_doc[-1]}.pdf" - ) - dst.remove_unreferenced_resources() - dst.save(filepath) - dst.close() + outputs = [ + ( + [pdf_ops.PageSpec(page) for page in split_doc], + partial(_scratch_path, f"{doc.id}_{split_doc[0]}-{split_doc[-1]}.pdf"), + ) + for split_doc in pages + ] + filepaths = pdf_ops.build_pdfs(pair.source_doc.source_path, outputs) - overrides: DocumentMetadataOverrides = ( - DocumentMetadataOverrides().from_document(doc) - ) - overrides.title = f"{doc.title} (split {idx + 1})" - if user is not None: - overrides.owner_id = user.id - if not delete_originals: - overrides.skip_asn_if_exists = True - logger.info( - f"Adding split document with pages {split_doc} to the task queue.", - ) - consume_tasks.append( - consume_file.s( - input_doc=ConsumableDocument( - source=DocumentSource.ConsumeFolder, - original_file=filepath, - ), - overrides=overrides, - ).set(headers={"trigger_source": trigger_source}), - ) + for idx, (split_doc, filepath) in enumerate( + zip(pages, filepaths, strict=True), + ): + overrides: DocumentMetadataOverrides = ( + DocumentMetadataOverrides().from_document(doc) + ) + overrides.title = f"{doc.title} (split {idx + 1})" + if user is not None: + overrides.owner_id = user.id + if not delete_originals: + overrides.skip_asn_if_exists = True + logger.info( + f"Adding split document with pages {split_doc} to the task queue.", + ) + consume_tasks.append( + consume_file.s( + input_doc=ConsumableDocument( + source=DocumentSource.ConsumeFolder, + original_file=filepath, + ), + overrides=overrides, + ).set(headers={"trigger_source": trigger_source}), + ) - if delete_originals: - backup = release_archive_serial_numbers([doc.id]) - logger.info( - "Queueing removal of original document after consumption of the split documents", - ) - try: - chord( - header=consume_tasks, - body=delete.si([doc.id]), - ).on_error( - restore_archive_serial_numbers_task.s(backup), - ).apply_async() - except Exception: - restore_archive_serial_numbers(backup) - raise - else: - group(consume_tasks).delay() + if delete_originals: + backup = release_archive_serial_numbers([doc.id]) + logger.info( + "Queueing removal of original document after consumption of the split documents", + ) + try: + chord( + header=consume_tasks, + body=delete.si([doc.id]), + ).on_error( + restore_archive_serial_numbers_task.s(backup), + ).apply_async() + except Exception: + restore_archive_serial_numbers(backup) + raise + else: + group(consume_tasks).delay() except Exception as e: logger.exception(f"Error splitting document {doc.id}: {e}") @@ -855,8 +849,6 @@ def delete_pages( ) doc = Document.objects.select_related("root_document").get(id=doc_ids[0]) pair = _resolve_root_and_source_doc(doc, source_mode=source_mode) - pages = sorted(pages) # sort pages to avoid index issues - import pikepdf try: # Produce edited PDF to a temp file and create a new version @@ -864,13 +856,7 @@ def delete_pages( Path(tempfile.mkdtemp(dir=settings.SCRATCH_DIR)) / f"{pair.root_doc.id}_pages_deleted.pdf" ) - with pikepdf.open(pair.source_doc.source_path) as pdf: - offset = 1 # pages are 1-indexed - for page_num in pages: - pdf.pages.remove(pdf.pages[page_num - offset]) - offset += 1 # remove() changes the index of the pages - pdf.remove_unreferenced_resources() - pdf.save(filepath) + pdf_ops.remove_pages(pair.source_doc.source_path, filepath, pages) overrides = DocumentMetadataOverrides().from_document(pair.root_doc) if user is not None: @@ -919,47 +905,28 @@ def edit_pdf( ) doc = Document.objects.select_related("root_document").get(id=doc_ids[0]) pair = _resolve_root_and_source_doc(doc, source_mode=source_mode) - import pikepdf - - pdf_docs: list[pikepdf.Pdf] = [] - try: - if not operations: - raise ValueError("Output document index is out of bounds") - - max_idx = max(op.get("doc", 0) for op in operations) - if update_document and max_idx > 0: - logger.error( - "Update requested but multiple output documents specified", + output_count = pdf_ops.validate_page_operations( + operations, + single_output=update_document, + ) + page_specs: list[list[pdf_ops.PageSpec]] = [[] for _ in range(output_count)] + for op in operations: + page_specs[op.get("doc", 0)].append( + pdf_ops.PageSpec(op["page"], op.get("rotate", 0)), ) - raise ValueError("Multiple output documents specified") - - if any( - op.get("doc", 0) < 0 or op.get("doc", 0) >= len(operations) - for op in operations - ): - raise ValueError("Output document index is out of bounds") - - with pikepdf.open(pair.source_doc.source_path) as src: - # prepare output documents - pdf_docs = [pikepdf.new() for _ in range(max_idx + 1)] - - for op in operations: - dst = pdf_docs[op.get("doc", 0)] - page = src.pages[op["page"] - 1] - dst.pages.append(page) - if op.get("rotate"): - dst.pages[-1].rotate(op["rotate"], relative=True) if update_document: # Create a new version from the edited PDF rather than replacing in-place - pdf = pdf_docs[0] - pdf.remove_unreferenced_resources() - filepath: Path = ( - Path(tempfile.mkdtemp(dir=settings.SCRATCH_DIR)) - / f"{pair.root_doc.id}_edited.pdf" + (filepath,) = pdf_ops.build_pdfs( + pair.source_doc.source_path, + [ + ( + page_specs[0], + partial(_scratch_path, f"{pair.root_doc.id}_edited.pdf"), + ), + ], ) - pdf.save(filepath) overrides = ( DocumentMetadataOverrides().from_document(pair.root_doc) if include_metadata @@ -980,6 +947,19 @@ def edit_pdf( headers={"trigger_source": trigger_source}, ) else: + version_filepaths = pdf_ops.build_pdfs( + pair.source_doc.source_path, + [ + ( + specs, + partial( + _scratch_path, + f"{pair.root_doc.id}_edit_{idx}.pdf", + ), + ) + for idx, specs in enumerate(page_specs, start=1) + ], + ) consume_tasks = [] overrides = ( DocumentMetadataOverrides().from_document(pair.root_doc) @@ -991,15 +971,9 @@ def edit_pdf( overrides.actor_id = user.id if not delete_original: overrides.skip_asn_if_exists = True - if delete_original and len(pdf_docs) == 1: + if delete_original and output_count == 1: overrides.asn = pair.root_doc.archive_serial_number - for idx, pdf in enumerate(pdf_docs, start=1): - version_filepath: Path = ( - Path(tempfile.mkdtemp(dir=settings.SCRATCH_DIR)) - / f"{pair.root_doc.id}_edit_{idx}.pdf" - ) - pdf.remove_unreferenced_resources() - pdf.save(version_filepath) + for version_filepath in version_filepaths: consume_tasks.append( consume_file.s( input_doc=ConsumableDocument( @@ -1049,8 +1023,6 @@ def remove_password( """ Remove password protection from PDF documents. """ - import pikepdf - for doc_id in doc_ids: doc = Document.objects.select_related("root_document").get(id=doc_id) pair = _resolve_root_and_source_doc(doc, source_mode=source_mode) @@ -1064,76 +1036,69 @@ def remove_password( doc.id, pair.source_doc.source_path, ) - try: - with pikepdf.open(source_path) as pdf: - if not pdf.is_encrypted: - logger.info( - "Skipping password removal for document %s because the " - "source PDF is not encrypted", - pair.root_doc.id, - ) - continue - except pikepdf.PasswordError: - # Password-protected PDFs need the supplied password below. - pass - - with pikepdf.open(source_path, password=password) as pdf: - filepath: Path = ( - Path(tempfile.mkdtemp(dir=settings.SCRATCH_DIR)) - / f"{pair.root_doc.id}_unprotected.pdf" + if not pdf_ops.needs_decrypt(source_path): + logger.info( + "Skipping password removal for document %s because the " + "source PDF is not encrypted", + pair.root_doc.id, ) - pdf.remove_unreferenced_resources() - pdf.save(filepath) + continue - if update_document: - # Create a new version rather than modifying the root/original in place. - overrides = ( - DocumentMetadataOverrides().from_document(pair.root_doc) - if include_metadata - else DocumentMetadataOverrides() - ) - if user is not None: - overrides.owner_id = user.id - overrides.actor_id = user.id - consume_file.apply_async( - kwargs={ - "input_doc": ConsumableDocument( - source=DocumentSource.ConsumeFolder, - original_file=filepath, - root_document_id=pair.root_doc.id, - ), - "overrides": overrides, - }, - headers={"trigger_source": trigger_source}, - ) + filepath = pdf_ops.decrypt_pdf( + source_path, + partial(_scratch_path, f"{pair.root_doc.id}_unprotected.pdf"), + password, + ) + + if update_document: + # Create a new version rather than modifying the root/original in place. + overrides = ( + DocumentMetadataOverrides().from_document(pair.root_doc) + if include_metadata + else DocumentMetadataOverrides() + ) + if user is not None: + overrides.owner_id = user.id + overrides.actor_id = user.id + consume_file.apply_async( + kwargs={ + "input_doc": ConsumableDocument( + source=DocumentSource.ConsumeFolder, + original_file=filepath, + root_document_id=pair.root_doc.id, + ), + "overrides": overrides, + }, + headers={"trigger_source": trigger_source}, + ) + else: + consume_tasks = [] + overrides = ( + DocumentMetadataOverrides().from_document(pair.root_doc) + if include_metadata + else DocumentMetadataOverrides() + ) + if user is not None: + overrides.owner_id = user.id + overrides.actor_id = user.id + + consume_tasks.append( + consume_file.s( + input_doc=ConsumableDocument( + source=DocumentSource.ConsumeFolder, + original_file=filepath, + ), + overrides=overrides, + ).set(headers={"trigger_source": trigger_source}), + ) + + if delete_original: + chord( + header=consume_tasks, + body=delete.si([doc.id]), + ).delay() else: - consume_tasks = [] - overrides = ( - DocumentMetadataOverrides().from_document(pair.root_doc) - if include_metadata - else DocumentMetadataOverrides() - ) - if user is not None: - overrides.owner_id = user.id - overrides.actor_id = user.id - - consume_tasks.append( - consume_file.s( - input_doc=ConsumableDocument( - source=DocumentSource.ConsumeFolder, - original_file=filepath, - ), - overrides=overrides, - ).set(headers={"trigger_source": trigger_source}), - ) - - if delete_original: - chord( - header=consume_tasks, - body=delete.si([doc.id]), - ).delay() - else: - group(consume_tasks).delay() + group(consume_tasks).delay() except Exception as e: logger.exception( diff --git a/src/documents/pdf_ops.py b/src/documents/pdf_ops.py new file mode 100644 index 000000000..0f5b696f6 --- /dev/null +++ b/src/documents/pdf_ops.py @@ -0,0 +1,194 @@ +""" +Pure PDF page operations used by documents.bulk_edit. + +This module deliberately knows nothing about Django, Celery or the documents +app: callers resolve documents, choose output paths and queue work. Every +function that writes a PDF removes unreferenced resources before saving. + +pikepdf is always called as ``pikepdf.open(...)`` / ``pikepdf.new()`` (never +``from pikepdf import open``) so tests can patch those module attributes. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING +from typing import NamedTuple + +import pikepdf + +if TYPE_CHECKING: + from collections.abc import Callable + from collections.abc import Iterable + from collections.abc import Mapping + from collections.abc import Sequence + from pathlib import Path + from types import TracebackType + + +class PageSpec(NamedTuple): + """One page of an output PDF: a 1-indexed source page, optionally rotated.""" + + page: int + rotate: int = 0 # relative degrees, 0 leaves the page alone + + +def _require_positive(pages: Iterable[int]) -> None: + for page in pages: + if page < 1: + raise ValueError(f"Page numbers start at 1, got {page}") + + +def rotate_pdf(src: Path, dst: Path, degrees: int) -> None: + """ + Rotate every page relatively on the opened document, not a rebuild, so Info, + XMP and outlines are kept. ``src`` is not modified. + """ + with pikepdf.open(src) as pdf: + for page in pdf.pages: + page.rotate(degrees, relative=True) + pdf.remove_unreferenced_resources() + pdf.save(dst) + + +def remove_pages(src: Path, dst: Path, pages: Iterable[int]) -> None: + """ + Remove 1-indexed pages from the opened document, not a rebuild, so Info, XMP + and outlines are kept. ``src`` is not modified. + + Duplicates are ignored. Pages are removed highest first so earlier removals + never shift the index of later ones. + """ + unique = sorted(set(pages)) + _require_positive(unique) + with pikepdf.open(src) as pdf: + for page_num in reversed(unique): + del pdf.pages[page_num - 1] + pdf.remove_unreferenced_resources() + pdf.save(dst) + + +def build_pdfs( + src: Path, + outputs: Sequence[tuple[Sequence[PageSpec], Callable[[], Path]]], +) -> list[Path]: + """ + Build one new PDF per output from pages of ``src``, opening ``src`` once. + + Each output is ``(page_specs, make_dst)``. Every page number is checked against + ``src`` before any output is built, and ``make_dst`` is called after that + output's pages are copied and immediately before it is saved, so a bad page + number in any output never leaves a destination behind. Document-level data + (Info, XMP, outlines) is not carried over. Returns the written paths in output + order. + """ + for specs, _ in outputs: + _require_positive(spec.page for spec in specs) + + written: list[Path] = [] + with pikepdf.open(src) as source: + page_count = len(source.pages) + for specs, _ in outputs: + for spec in specs: + if spec.page > page_count: + raise IndexError( + f"Page {spec.page} is out of range, the PDF has " + f"{page_count} pages", + ) + for specs, make_dst in outputs: + dst = pikepdf.new() + for spec in specs: + dst.pages.append(source.pages[spec.page - 1]) + if spec.rotate: + dst.pages[-1].rotate(spec.rotate, relative=True) + dst.remove_unreferenced_resources() + path = make_dst() + dst.save(path) + dst.close() + written.append(path) + return written + + +def validate_page_operations( + operations: Sequence[Mapping[str, int]], + *, + single_output: bool, +) -> int: + """ + Validate ``edit_pdf`` style operations and return the output document count. + + Each operation has ``page`` and optionally ``rotate`` and ``doc`` (the output + document index, default 0). The bounds rule is kept as it was: a ``doc`` index + must be below the number of operations. + """ + if not operations: + raise ValueError("Output document index is out of bounds") + + max_idx = max(op.get("doc", 0) for op in operations) + if single_output and max_idx > 0: + raise ValueError("Multiple output documents specified") + + if any( + op.get("doc", 0) < 0 or op.get("doc", 0) >= len(operations) for op in operations + ): + raise ValueError("Output document index is out of bounds") + + return max_idx + 1 + + +def needs_decrypt(src: Path) -> bool: + """ + True if ``src`` is encrypted. A PDF that needs a password to open at all + counts as encrypted. + """ + try: + with pikepdf.open(src) as pdf: + return bool(pdf.is_encrypted) + except pikepdf.PasswordError: + return True + + +def decrypt_pdf(src: Path, make_dst: Callable[[], Path], password: str) -> Path: + """ + Write an unencrypted copy of ``src`` and return its path. + + ``make_dst`` is only called once the password has been accepted, so a wrong + password never leaves a destination behind. + """ + with pikepdf.open(src, password=password) as pdf: + pdf.remove_unreferenced_resources() + dst = make_dst() + pdf.save(dst) + return dst + + +class PdfMerger: + """ + Accumulates the pages of several PDFs into one new PDF. + + ``add`` raises if a source cannot be read; deciding whether to skip it is the + caller's policy. Use as a context manager so the merged PDF is closed. + """ + + def __init__(self) -> None: + self._pdf = pikepdf.new() + self._version: str = self._pdf.pdf_version + + def __enter__(self) -> PdfMerger: + return self + + def __exit__( + self, + exc_type: type[BaseException] | None, + exc: BaseException | None, + tb: TracebackType | None, + ) -> None: + self._pdf.close() + + def add(self, path: Path) -> None: + with pikepdf.open(str(path)) as pdf: + self._version = max(self._version, pdf.pdf_version) + self._pdf.pages.extend(pdf.pages) + + def save(self, dst: Path) -> None: + self._pdf.remove_unreferenced_resources() + self._pdf.save(dst, min_version=self._version) diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index c2c875029..1547a81bb 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -2144,6 +2144,8 @@ class BulkEditSerializer( raise serializers.ValidationError("pages must be a list") if not all(isinstance(i, int) for i in parameters["pages"]): raise serializers.ValidationError("pages must be a list of integers") + if any(i < 1 for i in parameters["pages"]): + raise serializers.ValidationError("pages must be positive integers") def _validate_parameters_merge(self, parameters) -> None: if "delete_originals" in parameters: diff --git a/src/documents/tests/test_api_bulk_edit.py b/src/documents/tests/test_api_bulk_edit.py index 208657564..43dfc1064 100644 --- a/src/documents/tests/test_api_bulk_edit.py +++ b/src/documents/tests/test_api_bulk_edit.py @@ -1844,6 +1844,36 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): m.assert_called_once() self.assertEqual(m.call_args.kwargs["pages"], [[1], [2, 3, 4], [5]]) + @mock.patch("documents.serialisers.bulk_edit.delete_pages") + def test_bulk_edit_delete_pages_rejects_pages_below_one(self, m) -> None: + """ + GIVEN: + - A legacy delete_pages bulk edit + WHEN: + - API to bulk edit is called with a page number below 1 + THEN: + - API returns HTTP 400 + - delete_pages is not called + """ + self.setup_mock(m, "delete_pages") + + for pages in ([0], [-1], [1, 0]): + with self.subTest(pages=pages): + response = self.client.post( + "/api/documents/bulk_edit/", + json.dumps( + { + "documents": [self.doc2.id], + "method": "delete_pages", + "parameters": {"pages": pages}, + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn(b"pages must be positive integers", response.content) + m.assert_not_called() + @mock.patch("documents.views.bulk_edit.rotate") def test_rotate_insufficient_permissions(self, m) -> None: self.doc1.owner = User.objects.get(username="temp_admin") diff --git a/src/documents/tests/test_bulk_edit.py b/src/documents/tests/test_bulk_edit.py index 5aa760032..88d021db6 100644 --- a/src/documents/tests/test_bulk_edit.py +++ b/src/documents/tests/test_bulk_edit.py @@ -1,9 +1,9 @@ import shutil +from collections.abc import Callable from datetime import date from pathlib import Path from unittest import mock -import pikepdf import pytest from django.contrib.auth.models import Group from django.contrib.auth.models import Permission @@ -23,6 +23,7 @@ from documents.models import Document from documents.models import DocumentType from documents.models import StoragePath from documents.models import Tag +from documents.pdf_ops import PageSpec from documents.permissions import set_permissions_for_objects from paperless_testing.dirs import DirectoriesMixin from paperless_testing.factories import DocumentFactory @@ -796,16 +797,14 @@ class TestPDFActions(DirectoriesMixin, TestCase): self.img_doc.save() @staticmethod - def mock_password_required_pdf( - mock_open: mock.Mock, - fake_pdf: mock.Mock, - ) -> None: - password_context = mock.MagicMock() - password_context.__enter__.return_value = fake_pdf - mock_open.side_effect = [ - pikepdf.PasswordError("password required"), - password_context, - ] + def fake_decrypt( + src: Path, + make_dst: Callable[[], Path], + password: str, + ) -> Path: + dst = make_dst() + dst.write_bytes(b"password removed") + return dst @mock.patch("documents.tasks.consume_file.s") def test_merge(self, mock_consume_file) -> None: @@ -850,12 +849,12 @@ class TestPDFActions(DirectoriesMixin, TestCase): self.assertEqual(result, "OK") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.PdfMerger") @mock.patch("documents.tasks.consume_file.s") def test_merge_uses_latest_version_source_for_root_selection( self, mock_consume_file, - mock_open_pdf, + mock_merger, ) -> None: version_file = self.dirs.scratch_dir / "sample2_version_merge.pdf" shutil.copy(self.doc2.source_path, version_file) @@ -866,16 +865,14 @@ class TestPDFActions(DirectoriesMixin, TestCase): filename=version_file, mime_type="application/pdf", ) - fake_pdf = mock.MagicMock() - fake_pdf.pdf_version = "1.7" - fake_pdf.pages = [mock.Mock()] - mock_open_pdf.return_value.__enter__.return_value = fake_pdf + merger = mock_merger.return_value.__enter__.return_value + merger.save.side_effect = lambda dst: shutil.copy(version.source_path, dst) result = bulk_edit.merge([self.doc2.id]) self.assertEqual(result, "OK") - mock_open_pdf.assert_called_once_with(str(version.source_path)) - mock_consume_file.assert_not_called() + merger.add.assert_called_once_with(version.source_path) + mock_consume_file.assert_called_once() @mock.patch("documents.bulk_edit.delete.si") @mock.patch("documents.tasks.consume_file.s") @@ -1037,18 +1034,18 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) @mock.patch("documents.tasks.consume_file.delay") - @mock.patch("pikepdf.open") - def test_merge_with_errors(self, mock_open_pdf, mock_consume_file) -> None: + @mock.patch("documents.pdf_ops.PdfMerger.add") + def test_merge_with_errors(self, mock_add, mock_consume_file) -> None: """ GIVEN: - Existing documents WHEN: - Merge action is called with 2 documents - - Error occurs when opening both files + - Error occurs when adding both files THEN: - Consume file should not be called """ - mock_open_pdf.side_effect = Exception("Error opening PDF") + mock_add.side_effect = Exception("Error opening PDF") doc_ids = [self.doc2.id, self.doc3.id] with self.assertLogs("paperless.bulk_edit", level="ERROR") as cm: @@ -1085,12 +1082,12 @@ class TestPDFActions(DirectoriesMixin, TestCase): self.assertEqual(result, "OK") @mock.patch("documents.bulk_edit.group") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.build_pdfs") @mock.patch("documents.tasks.consume_file.s") def test_split_uses_latest_version_source_for_root_selection( self, mock_consume_file, - mock_open_pdf, + mock_build_pdfs, mock_group, ) -> None: version_file = self.dirs.scratch_dir / "sample2_version_split.pdf" @@ -1102,17 +1099,15 @@ class TestPDFActions(DirectoriesMixin, TestCase): filename=version_file, mime_type="application/pdf", ) - fake_pdf = mock.MagicMock() - fake_pdf.pages = [mock.Mock(), mock.Mock()] - mock_open_pdf.return_value.__enter__.return_value = fake_pdf + mock_build_pdfs.return_value = [version.source_path, version.source_path] mock_group.return_value.delay.return_value = None result = bulk_edit.split([self.doc2.id], [[1], [2]]) self.assertEqual(result, "OK") - mock_open_pdf.assert_called_once_with(version.source_path) - mock_consume_file.assert_not_called() - mock_group.return_value.delay.assert_not_called() + self.assertEqual(mock_build_pdfs.call_args.args[0], version.source_path) + self.assertEqual(mock_consume_file.call_count, 2) + mock_group.return_value.delay.assert_called_once() @mock.patch("documents.bulk_edit.delete.si") @mock.patch("documents.tasks.consume_file.s") @@ -1200,18 +1195,18 @@ class TestPDFActions(DirectoriesMixin, TestCase): self.assertEqual(self.doc2.archive_serial_number, 222) @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("pikepdf.Pdf.save") - def test_split_with_errors(self, mock_save_pdf, mock_consume_file) -> None: + @mock.patch("documents.pdf_ops.build_pdfs") + def test_split_with_errors(self, mock_build_pdfs, mock_consume_file) -> None: """ GIVEN: - Existing documents WHEN: - Split action is called with 1 document and 2 page groups - - Error occurs when saving the files + - Error occurs when building the files THEN: - Consume file should not be called """ - mock_save_pdf.side_effect = Exception("Error saving PDF") + mock_build_pdfs.side_effect = Exception("Error building PDFs") doc_ids = [self.doc2.id] pages = [[1, 2], [3]] @@ -1246,10 +1241,10 @@ class TestPDFActions(DirectoriesMixin, TestCase): self.assertEqual(result, "OK") @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("pikepdf.Pdf.save") + @mock.patch("documents.pdf_ops.rotate_pdf") def test_rotate_with_error( self, - mock_pdf_save, + mock_rotate_pdf, mock_consume_delay, ) -> None: """ @@ -1257,11 +1252,11 @@ class TestPDFActions(DirectoriesMixin, TestCase): - Existing documents WHEN: - Rotate action is called with 2 documents - - PikePDF raises an error + - Rotating the PDF raises an error THEN: - Rotate action should be called 0 times """ - mock_pdf_save.side_effect = Exception("Error saving PDF") + mock_rotate_pdf.side_effect = Exception("Error rotating PDF") doc_ids = [self.doc2.id, self.doc3.id] with self.assertLogs("paperless.bulk_edit", level="ERROR") as cm: @@ -1296,10 +1291,10 @@ class TestPDFActions(DirectoriesMixin, TestCase): @mock.patch("documents.data_models.magic.from_file", return_value="application/pdf") @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.rotate_pdf") def test_rotate_explicit_selection_uses_root_source_when_root_selected( self, - mock_open, + mock_rotate_pdf, mock_consume_delay, mock_magic, ) -> None: @@ -1308,9 +1303,6 @@ class TestPDFActions(DirectoriesMixin, TestCase): title="B version 1", root_document=self.doc2, ) - fake_pdf = mock.MagicMock() - fake_pdf.pages = [mock.Mock()] - mock_open.return_value.__enter__.return_value = fake_pdf result = bulk_edit.rotate( [self.doc2.id], @@ -1319,26 +1311,35 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - mock_open.assert_called_once_with(self.doc2.source_path) + self.assertEqual(mock_rotate_pdf.call_args.args[0], self.doc2.source_path) mock_consume_delay.assert_called_once() @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("pikepdf.Pdf.save") + @mock.patch("documents.pdf_ops.remove_pages") @mock.patch("documents.data_models.magic.from_file", return_value="application/pdf") - def test_delete_pages(self, mock_magic, mock_pdf_save, mock_consume_delay) -> None: + def test_delete_pages( + self, + mock_magic, + mock_remove_pages, + mock_consume_delay, + ) -> None: """ GIVEN: - Existing documents WHEN: - Delete pages action is called with 1 document and 2 pages THEN: - - Save should be called once + - The pages are removed from the document's source PDF - A new version should be enqueued via consume_file """ doc_ids = [self.doc2.id] pages = [1, 3] result = bulk_edit.delete_pages(doc_ids, pages) - mock_pdf_save.assert_called_once() + mock_remove_pages.assert_called_once_with( + self.doc2.source_path, + mock.ANY, + [1, 3], + ) mock_consume_delay.assert_called_once() task_kwargs = mock_consume_delay.call_args.kwargs["kwargs"] self.assertEqual(task_kwargs["input_doc"].root_document_id, self.doc2.id) @@ -1350,10 +1351,10 @@ class TestPDFActions(DirectoriesMixin, TestCase): @mock.patch("documents.data_models.magic.from_file", return_value="application/pdf") @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.remove_pages") def test_delete_pages_explicit_selection_uses_root_source_when_root_selected( self, - mock_open, + mock_remove_pages, mock_consume_delay, mock_magic, ) -> None: @@ -1362,9 +1363,6 @@ class TestPDFActions(DirectoriesMixin, TestCase): title="B version 1", root_document=self.doc2, ) - fake_pdf = mock.MagicMock() - fake_pdf.pages = [mock.Mock(), mock.Mock()] - mock_open.return_value.__enter__.return_value = fake_pdf result = bulk_edit.delete_pages( [self.doc2.id], @@ -1373,23 +1371,26 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - mock_open.assert_called_once_with(self.doc2.source_path) + self.assertEqual(mock_remove_pages.call_args.args[0], self.doc2.source_path) mock_consume_delay.assert_called_once() @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("pikepdf.Pdf.save") - def test_delete_pages_with_error(self, mock_pdf_save, mock_consume_delay) -> None: + @mock.patch("documents.pdf_ops.remove_pages") + def test_delete_pages_with_error( + self, + mock_remove_pages, + mock_consume_delay, + ) -> None: """ GIVEN: - Existing documents WHEN: - Delete pages action is called with 1 document and 2 pages - - PikePDF raises an error + - Removing the pages raises an error THEN: - - Save should be called once - No new version should be enqueued """ - mock_pdf_save.side_effect = Exception("Error saving PDF") + mock_remove_pages.side_effect = Exception("Error removing pages") doc_ids = [self.doc2.id] pages = [1, 3] @@ -1419,6 +1420,41 @@ class TestPDFActions(DirectoriesMixin, TestCase): self.assertEqual(result, "OK") mock_group.return_value.delay.assert_called_once() + @mock.patch("documents.bulk_edit.group") + @mock.patch("documents.pdf_ops.build_pdfs") + @mock.patch("documents.tasks.consume_file.s") + def test_edit_pdf_maps_operations_to_outputs( + self, + mock_consume_file: mock.Mock, + mock_build_pdfs: mock.Mock, + mock_group: mock.Mock, + ) -> None: + """ + GIVEN: + - Existing document + WHEN: + - edit_pdf is called with operations interleaved across two outputs, + some of them rotated + THEN: + - Each output is built from its own operations, in operation order, + with the requested rotation + """ + mock_build_pdfs.return_value = [self.doc2.source_path, self.doc2.source_path] + mock_group.return_value.delay.return_value = None + operations = [ + {"page": 3, "doc": 1}, + {"page": 1, "doc": 0, "rotate": 90}, + {"page": 2, "doc": 1, "rotate": 180}, + ] + + bulk_edit.edit_pdf([self.doc2.id], operations) + + outputs = mock_build_pdfs.call_args.args[1] + self.assertEqual( + [specs for specs, _ in outputs], + [[PageSpec(1, 90)], [PageSpec(3), PageSpec(2, 180)]], + ) + @mock.patch("documents.bulk_edit.group") @mock.patch("documents.tasks.consume_file.s") def test_edit_pdf_with_user_override(self, mock_consume_file, mock_group) -> None: @@ -1534,12 +1570,10 @@ class TestPDFActions(DirectoriesMixin, TestCase): @mock.patch("documents.data_models.magic.from_file", return_value="application/pdf") @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("pikepdf.new") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.build_pdfs") def test_edit_pdf_explicit_selection_uses_root_source_when_root_selected( self, - mock_open, - mock_new, + mock_build_pdfs, mock_consume_delay, mock_magic, ) -> None: @@ -1548,12 +1582,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): title="B version 1", root_document=self.doc2, ) - fake_pdf = mock.MagicMock() - fake_pdf.pages = [mock.Mock()] - mock_open.return_value.__enter__.return_value = fake_pdf - output_pdf = mock.MagicMock() - output_pdf.pages = [] - mock_new.return_value = output_pdf + mock_build_pdfs.return_value = [Path("edited.pdf")] result = bulk_edit.edit_pdf( [self.doc2.id], @@ -1563,7 +1592,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - mock_open.assert_called_once_with(self.doc2.source_path) + self.assertEqual(mock_build_pdfs.call_args.args[0], self.doc2.source_path) mock_consume_delay.assert_called_once() @mock.patch("documents.bulk_edit.group") @@ -1589,31 +1618,6 @@ class TestPDFActions(DirectoriesMixin, TestCase): self.assertEqual(result, "OK") mock_group.return_value.delay.assert_called_once() - @mock.patch("documents.bulk_edit.group") - @mock.patch("documents.tasks.consume_file.s") - def test_edit_pdf_open_failure( - self, - mock_consume_file: mock.Mock, - mock_group: mock.Mock, - ) -> None: - """ - GIVEN: - - Existing document - WHEN: - - edit_pdf fails to open PDF - THEN: - - Task group is not called - """ - doc_ids = [self.doc2.id] - operations = [ - {"page": 9999}, # invalid page, forces error during PDF load - ] - with self.assertLogs("paperless.bulk_edit", level="ERROR"): - with self.assertRaises(Exception): - bulk_edit.edit_pdf(doc_ids, operations) - mock_group.assert_not_called() - mock_consume_file.assert_not_called() - @mock.patch("documents.bulk_edit.group") @mock.patch("documents.tasks.consume_file.s") def test_edit_pdf_multiple_outputs_with_update_flag_errors( @@ -1640,23 +1644,15 @@ class TestPDFActions(DirectoriesMixin, TestCase): mock_group.assert_not_called() mock_consume_file.assert_not_called() - @mock.patch("pikepdf.open") - def test_edit_pdf_rejects_invalid_operations(self, mock_open) -> None: - for operations in ([], [{"page": 1, "doc": 2**32}]): - with self.subTest(operations=operations): - with self.assertLogs("paperless.bulk_edit", level="ERROR"): - with self.assertRaisesRegex(ValueError, "index is out of bounds"): - bulk_edit.edit_pdf([self.doc2.id], operations) - - mock_open.assert_not_called() - @mock.patch("documents.bulk_edit.update_document_content_maybe_archive_file.delay") @mock.patch("documents.tasks.consume_file.apply_async") @mock.patch("documents.bulk_edit.tempfile.mkdtemp") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.decrypt_pdf") + @mock.patch("documents.pdf_ops.needs_decrypt", return_value=True) def test_remove_password_update_document( self, - mock_open, + mock_needs_decrypt, + mock_decrypt, mock_mkdtemp, mock_consume_delay, mock_update_document, @@ -1665,16 +1661,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): temp_dir = self.dirs.scratch_dir / "remove-password-update" temp_dir.mkdir(parents=True, exist_ok=True) mock_mkdtemp.return_value = str(temp_dir) - - fake_pdf = mock.MagicMock() - fake_pdf.pages = [mock.Mock(), mock.Mock(), mock.Mock()] - fake_pdf.is_encrypted = True - - def save_side_effect(target_path): - Path(target_path).write_bytes(b"new pdf content") - - fake_pdf.save.side_effect = save_side_effect - mock_open.return_value.__enter__.return_value = fake_pdf + mock_decrypt.side_effect = self.fake_decrypt result = bulk_edit.remove_password( [doc.id], @@ -1683,14 +1670,8 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - self.assertEqual( - mock_open.call_args_list, - [ - mock.call(doc.source_path), - mock.call(doc.source_path, password="secret"), - ], - ) - fake_pdf.remove_unreferenced_resources.assert_called_once() + mock_needs_decrypt.assert_called_once_with(doc.source_path) + mock_decrypt.assert_called_once_with(doc.source_path, mock.ANY, "secret") mock_update_document.assert_not_called() mock_consume_delay.assert_called_once() task_kwargs = mock_consume_delay.call_args.kwargs["kwargs"] @@ -1703,40 +1684,15 @@ class TestPDFActions(DirectoriesMixin, TestCase): self.assertEqual(task_kwargs["input_doc"].root_document_id, doc.id) self.assertIsNotNone(task_kwargs["overrides"]) - @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("documents.bulk_edit.tempfile.mkdtemp") - @mock.patch("pikepdf.open") - def test_remove_password_update_document_skips_unencrypted_pdf( - self, - mock_open, - mock_mkdtemp, - mock_consume_delay, - ) -> None: - doc = self.doc1 - fake_pdf = mock.MagicMock() - fake_pdf.is_encrypted = False - mock_open.return_value.__enter__.return_value = fake_pdf - - result = bulk_edit.remove_password( - [doc.id], - password="secret", - update_document=True, - ) - - self.assertEqual(result, "OK") - mock_open.assert_called_once_with(doc.source_path) - fake_pdf.remove_unreferenced_resources.assert_not_called() - fake_pdf.save.assert_not_called() - mock_mkdtemp.assert_not_called() - mock_consume_delay.assert_not_called() - @mock.patch("documents.bulk_edit.update_document_content_maybe_archive_file.delay") @mock.patch("documents.tasks.consume_file.apply_async") @mock.patch("documents.bulk_edit.tempfile.mkdtemp") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.decrypt_pdf") + @mock.patch("documents.pdf_ops.needs_decrypt", return_value=True) def test_remove_password_update_document_uses_source_paths( self, - mock_open, + mock_needs_decrypt, + mock_decrypt, mock_mkdtemp, mock_consume_delay, mock_update_document, @@ -1747,14 +1703,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): temp_dir = self.dirs.scratch_dir / "remove-password-source-file" temp_dir.mkdir(parents=True, exist_ok=True) mock_mkdtemp.return_value = str(temp_dir) - - fake_pdf = mock.MagicMock() - self.mock_password_required_pdf(mock_open, fake_pdf) - - def save_side_effect(target_path): - Path(target_path).write_bytes(b"new pdf content") - - fake_pdf.save.side_effect = save_side_effect + mock_decrypt.side_effect = self.fake_decrypt result = bulk_edit.remove_password( [doc.id], @@ -1764,22 +1713,19 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - self.assertEqual( - mock_open.call_args_list, - [ - mock.call(source_file), - mock.call(source_file, password="secret"), - ], - ) + mock_needs_decrypt.assert_called_once_with(source_file) + mock_decrypt.assert_called_once_with(source_file, mock.ANY, "secret") mock_update_document.assert_not_called() mock_consume_delay.assert_called_once() @mock.patch("documents.data_models.magic.from_file", return_value="application/pdf") @mock.patch("documents.tasks.consume_file.apply_async") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.decrypt_pdf") + @mock.patch("documents.pdf_ops.needs_decrypt", return_value=True) def test_remove_password_explicit_selection_uses_root_source_when_root_selected( self, - mock_open, + mock_needs_decrypt, + mock_decrypt, mock_consume_delay, mock_magic, ) -> None: @@ -1788,8 +1734,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): title="A version 1", root_document=self.doc1, ) - fake_pdf = mock.MagicMock() - self.mock_password_required_pdf(mock_open, fake_pdf) + mock_decrypt.return_value = Path("unprotected.pdf") result = bulk_edit.remove_password( [self.doc1.id], @@ -1799,12 +1744,11 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - self.assertEqual( - mock_open.call_args_list, - [ - mock.call(self.doc1.source_path), - mock.call(self.doc1.source_path, password="secret"), - ], + mock_needs_decrypt.assert_called_once_with(self.doc1.source_path) + mock_decrypt.assert_called_once_with( + self.doc1.source_path, + mock.ANY, + "secret", ) mock_consume_delay.assert_called_once() @@ -1812,10 +1756,12 @@ class TestPDFActions(DirectoriesMixin, TestCase): @mock.patch("documents.bulk_edit.group") @mock.patch("documents.tasks.consume_file.s") @mock.patch("documents.bulk_edit.tempfile.mkdtemp") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.decrypt_pdf") + @mock.patch("documents.pdf_ops.needs_decrypt", return_value=True) def test_remove_password_creates_consumable_document( self, - mock_open: mock.Mock, + mock_needs_decrypt: mock.Mock, + mock_decrypt: mock.Mock, mock_mkdtemp: mock.Mock, mock_consume_file: mock.Mock, mock_group: mock.Mock, @@ -1825,15 +1771,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): temp_dir = self.dirs.scratch_dir / "remove-password" temp_dir.mkdir(parents=True, exist_ok=True) mock_mkdtemp.return_value = str(temp_dir) - - fake_pdf = mock.MagicMock() - fake_pdf.pages = [mock.Mock(), mock.Mock()] - self.mock_password_required_pdf(mock_open, fake_pdf) - - def save_side_effect(target_path: Path) -> None: - target_path.write_bytes(b"password removed") - - fake_pdf.save.side_effect = save_side_effect + mock_decrypt.side_effect = self.fake_decrypt mock_group.return_value.delay.return_value = None user = User.objects.create(username="owner") @@ -1848,13 +1786,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - self.assertEqual( - mock_open.call_args_list, - [ - mock.call(doc.source_path), - mock.call(doc.source_path, password="secret"), - ], - ) + mock_decrypt.assert_called_once_with(doc.source_path, mock.ANY, "secret") mock_consume_file.assert_called_once() call_kwargs = mock_consume_file.call_args.kwargs consumable_document = call_kwargs["input_doc"] @@ -1876,21 +1808,18 @@ class TestPDFActions(DirectoriesMixin, TestCase): @mock.patch("documents.bulk_edit.chord") @mock.patch("documents.bulk_edit.group") @mock.patch("documents.tasks.consume_file.s") - @mock.patch("documents.bulk_edit.tempfile.mkdtemp") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.decrypt_pdf") + @mock.patch("documents.pdf_ops.needs_decrypt", return_value=False) def test_remove_password_skips_unencrypted_pdf_without_queueing( self, - mock_open: mock.Mock, - mock_mkdtemp: mock.Mock, + mock_needs_decrypt: mock.Mock, + mock_decrypt: mock.Mock, mock_consume_file: mock.Mock, mock_group: mock.Mock, mock_chord: mock.Mock, mock_delete: mock.Mock, ) -> None: doc = self.doc2 - fake_pdf = mock.MagicMock() - fake_pdf.is_encrypted = False - mock_open.return_value.__enter__.return_value = fake_pdf result = bulk_edit.remove_password( [doc.id], @@ -1900,10 +1829,8 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - mock_open.assert_called_once_with(doc.source_path) - fake_pdf.remove_unreferenced_resources.assert_not_called() - fake_pdf.save.assert_not_called() - mock_mkdtemp.assert_not_called() + mock_needs_decrypt.assert_called_once_with(doc.source_path) + mock_decrypt.assert_not_called() mock_consume_file.assert_not_called() mock_group.assert_not_called() mock_chord.assert_not_called() @@ -1914,10 +1841,12 @@ class TestPDFActions(DirectoriesMixin, TestCase): @mock.patch("documents.bulk_edit.group") @mock.patch("documents.tasks.consume_file.s") @mock.patch("documents.bulk_edit.tempfile.mkdtemp") - @mock.patch("pikepdf.open") + @mock.patch("documents.pdf_ops.decrypt_pdf") + @mock.patch("documents.pdf_ops.needs_decrypt", return_value=True) def test_remove_password_deletes_original( self, - mock_open: mock.Mock, + mock_needs_decrypt: mock.Mock, + mock_decrypt: mock.Mock, mock_mkdtemp: mock.Mock, mock_consume_file: mock.Mock, mock_group: mock.Mock, @@ -1928,15 +1857,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): temp_dir = self.dirs.scratch_dir / "remove-password-delete" temp_dir.mkdir(parents=True, exist_ok=True) mock_mkdtemp.return_value = str(temp_dir) - - fake_pdf = mock.MagicMock() - fake_pdf.pages = [mock.Mock(), mock.Mock()] - self.mock_password_required_pdf(mock_open, fake_pdf) - - def save_side_effect(target_path: Path) -> None: - target_path.write_bytes(b"password removed") - - fake_pdf.save.side_effect = save_side_effect + mock_decrypt.side_effect = self.fake_decrypt mock_chord.return_value.delay.return_value = None result = bulk_edit.remove_password( @@ -1948,23 +1869,23 @@ class TestPDFActions(DirectoriesMixin, TestCase): ) self.assertEqual(result, "OK") - self.assertEqual( - mock_open.call_args_list, - [ - mock.call(doc.source_path), - mock.call(doc.source_path, password="secret"), - ], - ) + mock_decrypt.assert_called_once_with(doc.source_path, mock.ANY, "secret") mock_consume_file.assert_called_once() mock_group.assert_not_called() mock_chord.assert_called_once() mock_chord.return_value.delay.assert_called_once() mock_delete.si.assert_called_once_with([doc.id]) - @mock.patch("pikepdf.open") - def test_remove_password_open_failure(self, mock_open: mock.Mock) -> None: - mock_open.side_effect = RuntimeError("wrong password") - + @mock.patch( + "documents.pdf_ops.decrypt_pdf", + side_effect=RuntimeError("wrong password"), + ) + @mock.patch("documents.pdf_ops.needs_decrypt", return_value=True) + def test_remove_password_failure_raises_value_error( + self, + mock_needs_decrypt: mock.Mock, + mock_decrypt: mock.Mock, + ) -> None: with self.assertLogs("paperless.bulk_edit", level="ERROR") as cm: with self.assertRaises(ValueError) as exc: bulk_edit.remove_password([self.doc1.id], password="secret") diff --git a/src/documents/tests/test_pdf_ops.py b/src/documents/tests/test_pdf_ops.py new file mode 100644 index 000000000..eb4cf7005 --- /dev/null +++ b/src/documents/tests/test_pdf_ops.py @@ -0,0 +1,520 @@ +""" +Tests for documents.pdf_ops. + +These use real PDFs from the sample directories. No database, Celery or mocks. +Pages are compared by a hash of their content stream, so page identity and order +are easy to assert. +""" + +import hashlib +from collections.abc import Callable +from pathlib import Path + +import pikepdf +import pytest + +from documents import pdf_ops +from documents.pdf_ops import PageSpec + +SRC_ROOT = Path(__file__).parents[2] +SAMPLES = Path(__file__).parent / "samples" +THREE_PAGES = SAMPLES / "documents" / "originals" / "0000002.pdf" +TWELVE_PAGES = SAMPLES / "barcodes" / "split-by-asn-2.pdf" +ENCRYPTED = SAMPLES / "password-is-test.pdf" +SIGNED = SRC_ROOT / "paperless" / "tests" / "samples" / "tesseract" / "signed.pdf" + + +def _page_fingerprint(page: pikepdf.Page) -> str: + contents = page.obj.get("/Contents") + assert contents is not None, "sample page has no /Contents" + streams = list(contents) if isinstance(contents, pikepdf.Array) else [contents] + return hashlib.sha256(b"".join(s.read_bytes() for s in streams)).hexdigest() + + +def fingerprints(path: Path) -> list[str]: + with pikepdf.open(path) as pdf: + return [_page_fingerprint(page) for page in pdf.pages] + + +def rotations(path: Path) -> list[int]: + with pikepdf.open(path) as pdf: + return [int(page.obj.get("/Rotate", 0)) for page in pdf.pages] + + +def docinfo_keys(path: Path) -> set[str]: + with pikepdf.open(path) as pdf: + return set(pdf.docinfo.keys()) + + +def constant(path: Path) -> Callable[[], Path]: + return lambda: path + + +@pytest.fixture +def source_fingerprints() -> list[str]: + fps = fingerprints(THREE_PAGES) + assert len(set(fps)) == 3, "sample must have three distinct pages" + return fps + + +class TestRotatePdf: + def test_rotation_is_relative_and_applies_to_every_page( + self, + tmp_path: Path, + ) -> None: + """ + GIVEN: + - A three page PDF + WHEN: + - It is rotated by 90 degrees, then the result is rotated by 90 again + THEN: + - Every page is rotated relative to its current rotation + - The page content itself is unchanged + """ + once = tmp_path / "once.pdf" + twice = tmp_path / "twice.pdf" + + pdf_ops.rotate_pdf(THREE_PAGES, once, 90) + pdf_ops.rotate_pdf(once, twice, 90) + + assert rotations(once) == [90, 90, 90] + assert rotations(twice) == [180, 180, 180] + assert fingerprints(twice) == fingerprints(THREE_PAGES) + + def test_keeps_document_info(self, tmp_path: Path) -> None: + """ + GIVEN: + - A PDF with document info + WHEN: + - It is rotated + THEN: + - The document info is still present in the output + """ + dst = tmp_path / "out.pdf" + + pdf_ops.rotate_pdf(THREE_PAGES, dst, 90) + + assert "/Creator" in docinfo_keys(dst) + + +class TestRemovePages: + @pytest.mark.parametrize( + ("pages", "kept"), + [ + pytest.param([2], [0, 2], id="single"), + pytest.param([3, 1], [1], id="unordered"), + pytest.param([2, 2], [0, 2], id="duplicates-remove-once"), + pytest.param([], [0, 1, 2], id="empty-keeps-everything"), + pytest.param([1, 2, 3], [], id="every-page"), + ], + ) + def test_removes_only_the_requested_pages( + self, + tmp_path: Path, + source_fingerprints: list[str], + pages: list[int], + kept: list[int], + ) -> None: + """ + GIVEN: + - A three page PDF + WHEN: + - Pages are removed, in any order and possibly repeated + THEN: + - Exactly the other pages remain, in their original order + """ + dst = tmp_path / "out.pdf" + + pdf_ops.remove_pages(THREE_PAGES, dst, pages) + + assert fingerprints(dst) == [source_fingerprints[i] for i in kept] + + def test_keeps_document_info(self, tmp_path: Path) -> None: + """ + GIVEN: + - A PDF with document info + WHEN: + - A page is removed + THEN: + - The document info is still present in the output + """ + dst = tmp_path / "out.pdf" + + pdf_ops.remove_pages(THREE_PAGES, dst, [1]) + + assert "/Creator" in docinfo_keys(dst) + + @pytest.mark.parametrize( + "bad_page", + [ + pytest.param(0, id="zero"), + pytest.param(-1, id="negative"), + ], + ) + def test_rejects_pages_below_one(self, tmp_path: Path, bad_page: int) -> None: + """ + GIVEN: + - A three page PDF + WHEN: + - Pages are removed and one of them is below 1 + THEN: + - ValueError is raised + - No output file is written + """ + dst = tmp_path / "out.pdf" + + with pytest.raises(ValueError, match="start at 1"): + pdf_ops.remove_pages(THREE_PAGES, dst, [1, bad_page]) + + assert not dst.exists() + + +class TestBuildPdfs: + def test_selects_and_orders_pages( + self, + tmp_path: Path, + source_fingerprints: list[str], + ) -> None: + """ + GIVEN: + - A three page PDF + WHEN: + - One output is built from pages 3 then 1 + THEN: + - The output has those pages in that order + - Its path is returned + """ + dst = tmp_path / "out.pdf" + + written = pdf_ops.build_pdfs( + THREE_PAGES, + [([PageSpec(3), PageSpec(1)], constant(dst))], + ) + + assert written == [dst] + assert fingerprints(dst) == [source_fingerprints[2], source_fingerprints[0]] + + def test_rotates_only_the_requested_pages(self, tmp_path: Path) -> None: + """ + GIVEN: + - A three page PDF + WHEN: + - One output is built with a different rotation on each page + THEN: + - Each output page has exactly the rotation requested for it + """ + dst = tmp_path / "out.pdf" + + pdf_ops.build_pdfs( + THREE_PAGES, + [([PageSpec(1), PageSpec(2, 90), PageSpec(3, 180)], constant(dst))], + ) + + assert rotations(dst) == [0, 90, 180] + + def test_writes_one_file_per_output_in_order(self, tmp_path: Path) -> None: + """ + GIVEN: + - A twelve page PDF + WHEN: + - Two outputs are built from different page ranges + THEN: + - Two files are written and returned in output order + - Each holds exactly its own pages + """ + first = tmp_path / "first.pdf" + second = tmp_path / "second.pdf" + source = fingerprints(TWELVE_PAGES) + + written = pdf_ops.build_pdfs( + TWELVE_PAGES, + [ + ([PageSpec(p) for p in (1, 2, 3)], constant(first)), + ([PageSpec(p) for p in range(4, 13)], constant(second)), + ], + ) + + assert written == [first, second] + assert fingerprints(first) == source[:3] + assert fingerprints(second) == source[3:] + + def test_empty_page_list_writes_a_zero_page_file(self, tmp_path: Path) -> None: + """ + GIVEN: + - A three page PDF + WHEN: + - An output with no pages is built + THEN: + - A PDF with zero pages is written + """ + dst = tmp_path / "out.pdf" + + pdf_ops.build_pdfs(THREE_PAGES, [([], constant(dst))]) + + assert fingerprints(dst) == [] + + def test_destination_is_not_requested_when_a_page_is_out_of_range( + self, + tmp_path: Path, + ) -> None: + """ + GIVEN: + - A three page PDF + WHEN: + - Several outputs are built + - A later output refers to a page past the end + THEN: + - IndexError is raised + - No destination was requested for any output, including earlier valid ones + """ + requested: list[Path] = [] + + def make_dst() -> Path: + requested.append(tmp_path / "out.pdf") + return requested[-1] + + with pytest.raises(IndexError): + pdf_ops.build_pdfs( + THREE_PAGES, + [([PageSpec(1)], make_dst), ([PageSpec(99)], make_dst)], + ) + + assert requested == [] + + @pytest.mark.parametrize( + "bad_page", + [ + pytest.param(0, id="zero"), + pytest.param(-1, id="negative"), + ], + ) + def test_rejects_pages_below_one_before_opening_anything( + self, + tmp_path: Path, + bad_page: int, + ) -> None: + """ + GIVEN: + - A three page PDF + WHEN: + - Several outputs are built and a later one has a page below 1 + THEN: + - ValueError is raised + - No destination was requested for any output + """ + requested: list[Path] = [] + + def make_dst() -> Path: + requested.append(tmp_path / "out.pdf") + return requested[-1] + + with pytest.raises(ValueError, match="start at 1"): + pdf_ops.build_pdfs( + THREE_PAGES, + [([PageSpec(1)], make_dst), ([PageSpec(bad_page)], make_dst)], + ) + + assert requested == [] + + +class TestValidatePageOperations: + def test_returns_the_output_count(self) -> None: + """ + GIVEN: + - Operations that all target the default output + WHEN: + - They are validated + THEN: + - One output document is reported + """ + operations = [{"page": 1}, {"page": 2}, {"page": 3}] + + assert pdf_ops.validate_page_operations(operations, single_output=True) == 1 + + def test_gap_in_output_indices_counts_up_to_the_highest(self) -> None: + """ + GIVEN: + - Operations that target outputs 0 and 2 but never 1 + WHEN: + - They are validated + THEN: + - Three output documents are reported + """ + operations = [ + {"page": 1, "doc": 0}, + {"page": 2, "doc": 2}, + {"page": 3, "doc": 0}, + ] + + count = pdf_ops.validate_page_operations(operations, single_output=False) + + assert count == 3 + + def test_empty_operations_are_rejected(self) -> None: + """ + GIVEN: + - No operations + WHEN: + - They are validated + THEN: + - ValueError is raised + """ + with pytest.raises(ValueError, match="index is out of bounds"): + pdf_ops.validate_page_operations([], single_output=False) + + def test_multiple_outputs_rejected_when_single_output_required(self) -> None: + """ + GIVEN: + - Operations that target two outputs + WHEN: + - They are validated with a single output required + THEN: + - ValueError is raised + """ + operations = [{"page": 1, "doc": 0}, {"page": 2, "doc": 1}] + + with pytest.raises(ValueError, match="Multiple output documents"): + pdf_ops.validate_page_operations(operations, single_output=True) + + @pytest.mark.parametrize( + "doc", + [ + pytest.param(-1, id="negative"), + pytest.param(2, id="equal-to-operation-count"), + pytest.param(2**32, id="huge"), + ], + ) + def test_output_index_out_of_bounds(self, doc: int) -> None: + """ + GIVEN: + - Two operations, one with an output index that is out of bounds + WHEN: + - They are validated + THEN: + - ValueError is raised + """ + operations = [{"page": 1, "doc": 0}, {"page": 2, "doc": doc}] + + with pytest.raises(ValueError, match="index is out of bounds"): + pdf_ops.validate_page_operations(operations, single_output=False) + + +class TestDecrypt: + @pytest.mark.parametrize( + ("path", "expected"), + [ + pytest.param(ENCRYPTED, True, id="password-required"), + pytest.param(SIGNED, True, id="opens-without-password-but-encrypted"), + pytest.param(THREE_PAGES, False, id="not-encrypted"), + ], + ) + def test_needs_decrypt(self, path: Path, *, expected: bool) -> None: + """ + GIVEN: + - A PDF that is encrypted, or encrypted but openable, or plain + WHEN: + - needs_decrypt is asked about it + THEN: + - Only the unencrypted PDF reports False + """ + assert pdf_ops.needs_decrypt(path) is expected + + def test_decrypt_writes_an_unencrypted_copy(self, tmp_path: Path) -> None: + """ + GIVEN: + - A password protected PDF + WHEN: + - It is decrypted with the correct password + THEN: + - The path from make_dst is returned + - The written copy no longer needs decrypting + """ + dst = tmp_path / "out.pdf" + + result = pdf_ops.decrypt_pdf(ENCRYPTED, constant(dst), "test") + + assert result == dst + assert pdf_ops.needs_decrypt(dst) is False + + def test_wrong_password_raises_and_never_requests_a_destination( + self, + tmp_path: Path, + ) -> None: + """ + GIVEN: + - A password protected PDF + WHEN: + - It is decrypted with the wrong password + THEN: + - PasswordError is raised + - No destination was requested + """ + requested: list[Path] = [] + + def make_dst() -> Path: + requested.append(tmp_path / "out.pdf") + return requested[-1] + + with pytest.raises(pikepdf.PasswordError): + pdf_ops.decrypt_pdf(ENCRYPTED, make_dst, "wrong") + + assert requested == [] + + +class TestPdfMerger: + def test_pages_are_appended_in_the_order_added( + self, + tmp_path: Path, + source_fingerprints: list[str], + ) -> None: + """ + GIVEN: + - A reordered PDF and the original three page PDF + WHEN: + - Both are added to a merger in that order and saved + THEN: + - The output holds all pages in the order they were added + """ + reordered = tmp_path / "reordered.pdf" + merged = tmp_path / "merged.pdf" + pdf_ops.build_pdfs( + THREE_PAGES, + [([PageSpec(3), PageSpec(1)], constant(reordered))], + ) + + with pdf_ops.PdfMerger() as merger: + merger.add(reordered) + merger.add(THREE_PAGES) + merger.save(merged) + + assert fingerprints(merged) == [ + source_fingerprints[2], + source_fingerprints[0], + *source_fingerprints, + ] + + def test_output_version_is_at_least_the_highest_source_version( + self, + tmp_path: Path, + ) -> None: + """ + GIVEN: + - Two PDFs with different PDF versions + WHEN: + - Both are added to a merger and saved + THEN: + - The output version is at least the highest source version + """ + merged = tmp_path / "merged.pdf" + with pikepdf.open(TWELVE_PAGES) as pdf: + source_versions = [pdf.pdf_version] + with pikepdf.open(THREE_PAGES) as pdf: + source_versions.append(pdf.pdf_version) + + with pdf_ops.PdfMerger() as merger: + merger.add(TWELVE_PAGES) + merger.add(THREE_PAGES) + merger.save(merged) + + with pikepdf.open(merged) as pdf: + assert pdf.pdf_version >= max(source_versions)