Fix: resolve silent duplication from rebase onto fix/bulk_edit

Rebasing onto origin/fix/bulk_edit (PR #14083, which independently
bounds the edit_pdf doc index via manual checks) applied cleanly with
no reported conflicts, but left two copies of the same "doc index is
out of bounds" check back to back in both EditPdfDocumentsSerializer
.validate and BulkEditSerializer._validate_parameters_edit_pdf --
#14083's own `< 0 or >= len(operations)` check is now fully redundant
here since PdfEditOperationSerializer.doc already has min_value=0.
Removed the redundant second check in both methods.

Also split the merged test_edit_pdf_invalid_params subtest: #14083's
loop asserted the same "doc index is out of bounds" message for both
doc=-1 and doc=2**32, but with min_value=0 in place, -1 is now
rejected earlier by the field itself with a different message.

src/documents/bulk_edit.py

@@ -899,17 +899,26 @@ def edit_pdf(
     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",
+            )
+            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
-            max_idx = max(op.get("doc", 0) for op in operations)
             pdf_docs = [pikepdf.new() for _ in range(max_idx + 1)]
 
-            if update_document and len(pdf_docs) > 1:
-                logger.error(
-                    "Update requested but multiple output documents specified",
-                )
-                raise ValueError("Multiple output documents specified")
-
             for op in operations:
                 dst = pdf_docs[op.get("doc", 0)]
                 page = src.pages[op["page"] - 1]

src/documents/serialisers.py

@@ -1749,8 +1749,18 @@ class MergeDocumentsAsVersionsSerializer(DocumentListSerializer):
         return attrs
 
 
+class PdfEditOperationSerializer(serializers.Serializer[dict[str, int]]):
+    page = serializers.IntegerField(min_value=1)
+    rotate = serializers.IntegerField(required=False)
+    doc = serializers.IntegerField(required=False, min_value=0)
+
+
 class EditPdfDocumentsSerializer(DocumentListSerializer, SourceModeValidationMixin):
-    operations = serializers.ListField(required=True)
+    operations = serializers.ListField(
+        child=PdfEditOperationSerializer(),
+        required=True,
+        allow_empty=False,
+    )
     delete_original = serializers.BooleanField(required=False, default=False)
     update_document = serializers.BooleanField(required=False, default=False)
     include_metadata = serializers.BooleanField(required=False, default=True)
@@ -1768,18 +1778,9 @@ class EditPdfDocumentsSerializer(DocumentListSerializer, SourceModeValidationMix
             )
 
         operations = attrs["operations"]
-        if not isinstance(operations, list):
-            raise serializers.ValidationError("operations must be a list")
 
-        for op in operations:
-            if not isinstance(op, dict):
-                raise serializers.ValidationError("invalid operation entry")
-            if "page" not in op or not isinstance(op["page"], int):
-                raise serializers.ValidationError("page must be an integer")
-            if "rotate" in op and not isinstance(op["rotate"], int):
-                raise serializers.ValidationError("rotate must be an integer")
-            if "doc" in op and not isinstance(op["doc"], int):
-                raise serializers.ValidationError("doc must be an integer")
+        if any(op.get("doc", 0) >= len(operations) for op in operations):
+            raise serializers.ValidationError("doc index is out of bounds")
 
         if attrs["update_document"]:
             max_idx = max(op.get("doc", 0) for op in operations)
@@ -1791,7 +1792,7 @@ class EditPdfDocumentsSerializer(DocumentListSerializer, SourceModeValidationMix
         doc = Document.objects.get(id=documents[0])
         if doc.page_count:
             for op in operations:
-                if op["page"] < 1 or op["page"] > doc.page_count:
+                if op["page"] > doc.page_count:
                     raise serializers.ValidationError(
                         f"Page {op['page']} is out of bounds for document with {doc.page_count} pages.",
                     )
@@ -2037,10 +2038,10 @@ class BulkEditSerializer(
             raise serializers.ValidationError("remove_custom_fields not specified")
 
     def _validate_owner(self, owner):
-        try:
-            return User.objects.get(pk=owner)
-        except (User.DoesNotExist, TypeError, ValueError):
+        ownerUser = User.objects.get(pk=owner)
+        if ownerUser is None:
             raise serializers.ValidationError("Specified owner cannot be found")
+        return ownerUser
 
     def _validate_parameters_set_permissions(self, parameters) -> None:
         if "set_permissions" not in parameters:
@@ -2060,7 +2061,7 @@ class BulkEditSerializer(
                 or not float(parameters["degrees"]).is_integer()
             ):
                 raise serializers.ValidationError("invalid rotation degrees")
-        except (TypeError, ValueError):
+        except ValueError:
             raise serializers.ValidationError("invalid rotation degrees")
 
     def _validate_source_mode(self, parameters) -> None:
@@ -2073,8 +2074,6 @@ class BulkEditSerializer(
     def _validate_parameters_split(self, parameters) -> None:
         if "pages" not in parameters:
             raise serializers.ValidationError("pages not specified")
-        if not isinstance(parameters["pages"], str):
-            raise serializers.ValidationError("invalid pages specified")
         try:
             pages = []
             docs = parameters["pages"].split(",")
@@ -2124,17 +2123,15 @@ class BulkEditSerializer(
     def _validate_parameters_edit_pdf(self, parameters, document_id) -> None:
         if "operations" not in parameters:
             raise serializers.ValidationError("operations not specified")
-        if not isinstance(parameters["operations"], list):
-            raise serializers.ValidationError("operations must be a list")
-        for op in parameters["operations"]:
-            if not isinstance(op, dict):
-                raise serializers.ValidationError("invalid operation entry")
-            if "page" not in op or not isinstance(op["page"], int):
-                raise serializers.ValidationError("page must be an integer")
-            if "rotate" in op and not isinstance(op["rotate"], int):
-                raise serializers.ValidationError("rotate must be an integer")
-            if "doc" in op and not isinstance(op["doc"], int):
-                raise serializers.ValidationError("doc must be an integer")
+        operations_field = serializers.ListField(
+            child=PdfEditOperationSerializer(),
+            allow_empty=False,
+        )
+        parameters["operations"] = operations_field.run_validation(
+            parameters["operations"],
+        )
+        operations = parameters["operations"]
+
         if "update_document" in parameters:
             if not isinstance(parameters["update_document"], bool):
                 raise serializers.ValidationError("update_document must be a boolean")
@@ -2146,8 +2143,11 @@ class BulkEditSerializer(
         else:
             parameters["include_metadata"] = True
 
+        if any(op.get("doc", 0) >= len(operations) for op in operations):
+            raise serializers.ValidationError("doc index is out of bounds")
+
         if parameters["update_document"]:
-            max_idx = max(op.get("doc", 0) for op in parameters["operations"])
+            max_idx = max(op.get("doc", 0) for op in operations)
             if max_idx > 0:
                 raise serializers.ValidationError(
                     "update_document only allowed with a single output document",
@@ -2156,8 +2156,8 @@ class BulkEditSerializer(
         doc = Document.objects.get(id=document_id)
         # doc existence is already validated
         if doc.page_count:
-            for op in parameters["operations"]:
-                if op["page"] < 1 or op["page"] > doc.page_count:
+            for op in operations:
+                if op["page"] > doc.page_count:
                     raise serializers.ValidationError(
                         f"Page {op['page']} is out of bounds for document with {doc.page_count} pages.",
                     )

src/documents/signals/handlers.py

@@ -1189,18 +1189,13 @@ def before_task_publish_handler(
         trigger_source = _determine_trigger_source(headers)
         owner_id = _extract_owner_id(task_type, task_kwargs)
 
-        # A retried task is republished with the same task_id, so this fires
-        # again for it; get_or_create keeps the original PENDING record
-        # instead of raising a duplicate-key IntegrityError on the retry.
-        PaperlessTask.objects.get_or_create(
+        PaperlessTask.objects.create(
             task_id=task_id,
-            defaults={
-                "task_type": task_type,
-                "trigger_source": trigger_source,
-                "status": PaperlessTask.Status.PENDING,
-                "input_data": input_data,
-                "owner_id": owner_id,
-            },
+            task_type=task_type,
+            trigger_source=trigger_source,
+            status=PaperlessTask.Status.PENDING,
+            input_data=input_data,
+            owner_id=owner_id,
         )
     except Exception:  # pragma: no cover
         logger.exception("Creating PaperlessTask failed")

src/documents/tests/test_api_bulk_edit.py

@@ -1165,65 +1165,6 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
         self.assertIn(b"set_permissions not specified", response.content)
         m.assert_not_called()
 
-    @mock.patch("documents.serialisers.bulk_edit.set_permissions")
-    def test_set_permissions_rejects_nonexistent_owner(self, m) -> None:
-        """
-        BulkEditSerializer._validate_owner called User.objects.get(pk=owner)
-        with no try/except, so a syntactically valid but nonexistent user
-        id raised an uncaught User.DoesNotExist instead of a clean 400.
-        """
-        self.setup_mock(m, "set_permissions")
-
-        response = self.client.post(
-            "/api/documents/bulk_edit/",
-            json.dumps(
-                {
-                    "documents": [self.doc2.id],
-                    "method": "set_permissions",
-                    "parameters": {
-                        "set_permissions": {"view": {"users": [self.user.id]}},
-                        "owner": 999999,
-                    },
-                },
-            ),
-            content_type="application/json",
-        )
-
-        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        m.assert_not_called()
-
-    @mock.patch("documents.serialisers.bulk_edit.set_permissions")
-    def test_set_permissions_rejects_wrong_typed_owner(self, m) -> None:
-        """
-        _validate_owner only caught User.DoesNotExist -- a wrong-typed
-        owner (list/dict/non-numeric string) reaches
-        User.objects.get(pk=owner) and raises an uncaught TypeError or
-        ValueError instead, since `parameters` is a bare DictField with
-        no type checking on "owner" at that level.
-        """
-        self.setup_mock(m, "set_permissions")
-
-        for bad_owner in (["not", "an", "id"], {"nested": "dict"}, "not-a-number"):
-            with self.subTest(owner=bad_owner):
-                response = self.client.post(
-                    "/api/documents/bulk_edit/",
-                    json.dumps(
-                        {
-                            "documents": [self.doc2.id],
-                            "method": "set_permissions",
-                            "parameters": {
-                                "set_permissions": {
-                                    "view": {"users": [self.user.id]},
-                                },
-                                "owner": bad_owner,
-                            },
-                        },
-                    ),
-                    content_type="application/json",
-                )
-                self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        m.assert_not_called()
-
     @mock.patch("documents.serialisers.bulk_edit.set_permissions")
     def test_set_permissions_merge(self, m) -> None:
         self.setup_mock(m, "set_permissions")
@@ -1497,53 +1438,6 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
         m.assert_not_called()
 
-    @mock.patch("documents.serialisers.bulk_edit.rotate")
-    def test_bulk_edit_rotate_rejects_null_degrees(self, m) -> None:
-        """
-        BulkEditSerializer._validate_parameters_rotate's
-        `float(parameters["degrees"])` raised an uncaught TypeError for
-        None (only ValueError was caught), reachable via the legacy
-        generic /api/documents/bulk_edit/ method="rotate" path (the
-        dedicated /api/documents/rotate/ endpoint isn't affected, its
-        `degrees` field is a typed IntegerField).
-        """
-        self.setup_mock(m, "rotate")
-        response = self.client.post(
-            "/api/documents/bulk_edit/",
-            json.dumps(
-                {
-                    "documents": [self.doc2.id],
-                    "method": "rotate",
-                    "parameters": {"degrees": None},
-                },
-            ),
-            content_type="application/json",
-        )
-        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        m.assert_not_called()
-
-    @mock.patch("documents.serialisers.bulk_edit.split")
-    def test_bulk_edit_split_rejects_null_pages(self, m) -> None:
-        """
-        BulkEditSerializer._validate_parameters_split called
-        parameters["pages"].split(",") with no type check, so a null
-        value raised an uncaught AttributeError instead of a clean 400.
-        """
-        self.setup_mock(m, "split")
-        response = self.client.post(
-            "/api/documents/bulk_edit/",
-            json.dumps(
-                {
-                    "documents": [self.doc2.id],
-                    "method": "split",
-                    "parameters": {"pages": None},
-                },
-            ),
-            content_type="application/json",
-        )
-        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        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")
@@ -1755,6 +1649,24 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
 
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
 
+    def test_legacy_bulk_edit_rejects_out_of_bounds_pdf_doc_index(self) -> None:
+        response = self.client.post(
+            "/api/documents/bulk_edit/",
+            json.dumps(
+                {
+                    "documents": [self.doc2.id],
+                    "method": "edit_pdf",
+                    "parameters": {
+                        "operations": [{"page": 1, "doc": 2**32}],
+                    },
+                },
+            ),
+            content_type="application/json",
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        self.assertIn(b"doc index is out of bounds", response.content)
+
     @mock.patch("documents.views.bulk_edit.edit_pdf")
     def test_edit_pdf(self, m) -> None:
         self.setup_mock(m, "edit_pdf")
@@ -1816,7 +1728,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertIn(b"invalid operation entry", response.content)
+        self.assertIn(b"Expected a dictionary", response.content)
 
         response = self.client.post(
             "/api/documents/edit_pdf/",
@@ -1829,7 +1741,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertIn(b"page must be an integer", response.content)
+        self.assertIn(b"valid integer is required", response.content)
 
         response = self.client.post(
             "/api/documents/edit_pdf/",
@@ -1842,7 +1754,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertIn(b"rotate must be an integer", response.content)
+        self.assertIn(b"valid integer is required", response.content)
 
         response = self.client.post(
             "/api/documents/edit_pdf/",
@@ -1855,7 +1767,29 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertIn(b"doc must be an integer", response.content)
+        self.assertIn(b"valid integer is required", response.content)
+
+        # A negative doc index is rejected by PdfEditOperationSerializer's
+        # own min_value=0 field constraint, before the "doc index is out
+        # of bounds" object-level check (against len(operations)) ever
+        # runs -- hence the different expected message per case.
+        for doc_index, expected_message in (
+            (-1, b"greater than or equal to 0"),
+            (2**32, b"doc index is out of bounds"),
+        ):
+            with self.subTest(doc_index=doc_index):
+                response = self.client.post(
+                    "/api/documents/edit_pdf/",
+                    json.dumps(
+                        {
+                            "documents": [self.doc2.id],
+                            "operations": [{"page": 1, "doc": doc_index}],
+                        },
+                    ),
+                    content_type="application/json",
+                )
+                self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+                self.assertIn(expected_message, response.content)
 
         response = self.client.post(
             "/api/documents/edit_pdf/",
@@ -1863,7 +1797,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
                 {
                     "documents": [self.doc2.id],
                     "update_document": True,
-                    "operations": [{"page": 1, "doc": 1}, {"page": 2, "doc": 2}],
+                    "operations": [{"page": 1, "doc": 0}, {"page": 2, "doc": 1}],
                 },
             ),
             content_type="application/json",
@@ -1888,6 +1822,86 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
         self.assertIn(b"Invalid source_mode", response.content)
 
+    @mock.patch("documents.views.bulk_edit.edit_pdf")
+    def test_edit_pdf_rejects_empty_operations(self, m) -> None:
+        """
+        An empty operations list previously reached bulk_edit.edit_pdf()
+        and crashed with `ValueError: max() iterable argument is empty`
+        (via `max(op.get("doc", 0) for op in operations)`) whenever
+        update_document was true. Must now be rejected up front.
+        """
+        self.setup_mock(m, "edit_pdf")
+        response = self.client.post(
+            "/api/documents/edit_pdf/",
+            json.dumps(
+                {
+                    "documents": [self.doc2.id],
+                    "operations": [],
+                    "update_document": True,
+                },
+            ),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        m.assert_not_called()
+
+    @mock.patch("documents.views.bulk_edit.edit_pdf")
+    def test_edit_pdf_rejects_negative_doc_index(self, m) -> None:
+        """
+        A negative `doc` index was previously silently accepted and used
+        as a wrapping Python list index instead of being rejected.
+        """
+        self.setup_mock(m, "edit_pdf")
+        response = self.client.post(
+            "/api/documents/edit_pdf/",
+            json.dumps(
+                {
+                    "documents": [self.doc2.id],
+                    "operations": [{"page": 1, "doc": -1}],
+                },
+            ),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        m.assert_not_called()
+
+    @mock.patch("documents.views.bulk_edit.edit_pdf")
+    def test_edit_pdf_rejects_out_of_bounds_doc_index(self, m) -> None:
+        """
+        A `doc` index far larger than the number of operations previously
+        drove `pdf_docs = [pikepdf.new() for _ in range(max_idx + 1)]` to
+        attempt allocating an enormous number of real objects.
+        """
+        self.setup_mock(m, "edit_pdf")
+        response = self.client.post(
+            "/api/documents/edit_pdf/",
+            json.dumps(
+                {
+                    "documents": [self.doc2.id],
+                    "operations": [{"page": 1, "doc": 2**33}],
+                },
+            ),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        m.assert_not_called()
+
+    @mock.patch("documents.views.bulk_edit.edit_pdf")
+    def test_edit_pdf_rejects_non_positive_page(self, m) -> None:
+        self.setup_mock(m, "edit_pdf")
+        response = self.client.post(
+            "/api/documents/edit_pdf/",
+            json.dumps(
+                {
+                    "documents": [self.doc2.id],
+                    "operations": [{"page": 0}],
+                },
+            ),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        m.assert_not_called()
+
     @mock.patch("documents.views.bulk_edit.edit_pdf")
     def test_edit_pdf_page_out_of_bounds(self, m) -> None:
         self.setup_mock(m, "edit_pdf")
@@ -1905,6 +1919,46 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
         self.assertIn(b"out of bounds", response.content)
         m.assert_not_called()
 
+    @mock.patch("documents.serialisers.bulk_edit.edit_pdf")
+    def test_bulk_edit_edit_pdf_rejects_empty_operations(self, m) -> None:
+        """
+        Same validation gap as test_edit_pdf_rejects_empty_operations, but
+        via the legacy generic /api/documents/bulk_edit/ method="edit_pdf"
+        path, which hand-parses `parameters["operations"]` independently
+        in BulkEditSerializer._validate_parameters_edit_pdf.
+        """
+        self.setup_mock(m, "edit_pdf")
+        response = self.client.post(
+            "/api/documents/bulk_edit/",
+            json.dumps(
+                {
+                    "documents": [self.doc2.id],
+                    "method": "edit_pdf",
+                    "parameters": {"operations": [], "update_document": True},
+                },
+            ),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        m.assert_not_called()
+
+    @mock.patch("documents.serialisers.bulk_edit.edit_pdf")
+    def test_bulk_edit_edit_pdf_rejects_out_of_bounds_doc_index(self, m) -> None:
+        self.setup_mock(m, "edit_pdf")
+        response = self.client.post(
+            "/api/documents/bulk_edit/",
+            json.dumps(
+                {
+                    "documents": [self.doc2.id],
+                    "method": "edit_pdf",
+                    "parameters": {"operations": [{"page": 1, "doc": 2**33}]},
+                },
+            ),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        m.assert_not_called()
+
     @mock.patch("documents.views.bulk_edit.edit_pdf")
     def test_edit_pdf_insufficient_permissions(self, m) -> None:
         self.doc1.owner = User.objects.get(username="temp_admin")

src/documents/tests/test_bulk_edit.py

@@ -1642,6 +1642,17 @@ class TestPDFActions(DirectoriesMixin, TestCase):
         mock_group.assert_not_called()
         mock_consume_file.assert_not_called()
 
+    @mock.patch("pikepdf.open")
+    def test_edit_pdf_rejects_out_of_bounds_output_index(self, mock_open) -> None:
+        with self.assertLogs("paperless.bulk_edit", level="ERROR"):
+            with self.assertRaisesRegex(ValueError, "index is out of bounds"):
+                bulk_edit.edit_pdf(
+                    [self.doc2.id],
+                    [{"page": 1, "doc": 2**32}],
+                )
+
+        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")

src/documents/tests/test_task_signals.py

@@ -106,17 +106,6 @@ class TestBeforeTaskPublishHandler:
         assert task.task_type == PaperlessTask.TaskType.TRAIN_CLASSIFIER
         assert task.trigger_source == PaperlessTask.TriggerSource.MANUAL
 
-        # A Celery retry republishes with the same task_id; this must not
-        # raise a duplicate-key IntegrityError, and must leave the original
-        # PENDING record alone.
-        send_publish(
-            "documents.tasks.train_classifier",
-            (),
-            {},
-            headers={"id": task_id},
-        )
-        assert PaperlessTask.objects.filter(task_id=task_id).count() == 1
-
     def test_creates_task_for_sanity_check(self) -> None:
         task_id = send_publish("documents.tasks.sanity_check", (), {})
         task = PaperlessTask.objects.get(task_id=task_id)