Fix: type edit_pdf operations via a nested serializer (#14116)

operations was a ListField with no child, so both EditPdfDocumentsSerializer
and BulkEditSerializer._validate_parameters_edit_pdf hand-checked each entry
with isinstance(). That accepted booleans (isinstance(True, int) is true) and
passed through any extra keys.

Adds PdfEditOperationSerializer (page >= 1, doc >= 0, rotate a multiple of
90) and uses it as the ListField child on both paths, dropping the manual
type checks and the now-redundant negative/page < 1 bounds checks. Rotations
that are not a multiple of 90 previously passed validation and then failed
inside the task when QPDF refused them.

src/documents/serialisers.py

@@ -1749,8 +1749,23 @@ 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)
+
+    def validate_rotate(self, value: int) -> int:
+        if value % 90 != 0:
+            raise serializers.ValidationError("rotate must be a multiple of 90")
+        return value
+
+
 class EditPdfDocumentsSerializer(DocumentListSerializer, SourceModeValidationMixin):
-    operations = serializers.ListField(required=True, allow_empty=False)
+    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 +1783,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)
@@ -1788,16 +1794,10 @@ class EditPdfDocumentsSerializer(DocumentListSerializer, SourceModeValidationMix
                     "update_document only allowed with a single output document",
                 )
 
-        if any(
-            op.get("doc", 0) < 0 or op.get("doc", 0) >= len(operations)
-            for op in operations
-        ):
-            raise serializers.ValidationError("doc index is out of bounds")
-
         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.",
                     )
@@ -2128,19 +2128,18 @@ 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")
-        if not parameters["operations"]:
-            raise serializers.ValidationError("operations must not be empty")
-        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,
+        )
+        try:
+            operations = operations_field.run_validation(parameters["operations"])
+        except serializers.ValidationError as e:
+            # Key the errors under "operations" so they match what the
+            # dedicated edit_pdf endpoint returns
+            raise serializers.ValidationError({"operations": e.detail}) from e
+        parameters["operations"] = operations
+
         if "update_document" in parameters:
             if not isinstance(parameters["update_document"], bool):
                 raise serializers.ValidationError("update_document must be a boolean")
@@ -2152,24 +2151,21 @@ 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",
                 )
 
-        if any(
-            op.get("doc", 0) < 0 or op.get("doc", 0) >= len(parameters["operations"])
-            for op in parameters["operations"]
-        ):
-            raise serializers.ValidationError("doc index is out of bounds")
-
         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/tests/test_api_bulk_edit.py

@@ -1681,7 +1681,10 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
         )
 
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertIn(b"operations must not be empty", response.content)
+        self.assertEqual(
+            response.json(),
+            {"operations": ["This list may not be empty."]},
+        )
 
     @mock.patch("documents.views.bulk_edit.edit_pdf")
     def test_edit_pdf(self, m) -> None:
@@ -1751,7 +1754,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/",
@@ -1764,7 +1767,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/",
@@ -1777,7 +1780,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/",
@@ -1790,9 +1793,14 @@ 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)
 
-        for doc_index in (-1, 2**32):
+        # A negative index fails the field's min_value before the
+        # object-level bound against len(operations) is checked
+        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/",
@@ -1805,7 +1813,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
                     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)
+                self.assertIn(expected_message, response.content)
 
         response = self.client.post(
             "/api/documents/edit_pdf/",
@@ -1813,7 +1821,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",
@@ -1838,6 +1846,72 @@ 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_invalid_operation_values(self, m) -> None:
+        """
+        GIVEN:
+            - An edit_pdf operation with a non-positive page, a boolean in
+              place of an integer, or a rotation that is not a multiple of 90
+        WHEN:
+            - API to edit the PDF is called
+        THEN:
+            - API returns HTTP 400 with the field error
+            - edit_pdf is not called
+        """
+        self.setup_mock(m, "edit_pdf")
+        for operation, expected_message in (
+            ({"page": 0}, b"greater than or equal to 1"),
+            ({"page": True}, b"valid integer is required"),
+            ({"page": 1, "rotate": True}, b"valid integer is required"),
+            ({"page": 1, "doc": True}, b"valid integer is required"),
+            ({"page": 1, "rotate": 45}, b"rotate must be a multiple of 90"),
+        ):
+            with self.subTest(operation=operation):
+                response = self.client.post(
+                    "/api/documents/edit_pdf/",
+                    json.dumps(
+                        {
+                            "documents": [self.doc2.id],
+                            "operations": [operation],
+                        },
+                    ),
+                    content_type="application/json",
+                )
+                self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+                self.assertIn(expected_message, response.content)
+        m.assert_not_called()
+
+    def test_legacy_bulk_edit_keys_pdf_operation_errors_under_operations(
+        self,
+    ) -> None:
+        """
+        GIVEN:
+            - A legacy bulk_edit edit_pdf request with an invalid operation
+        WHEN:
+            - API to bulk edit is called
+        THEN:
+            - API returns HTTP 400
+            - The error is keyed under operations and the operation index,
+              matching the edit_pdf endpoint
+        """
+        response = self.client.post(
+            "/api/documents/bulk_edit/",
+            json.dumps(
+                {
+                    "documents": [self.doc2.id],
+                    "method": "edit_pdf",
+                    "parameters": {"operations": [{"page": 1, "rotate": 45}]},
+                },
+            ),
+            content_type="application/json",
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(
+            response.json(),
+            {"operations": {"0": {"rotate": ["rotate must be a multiple of 90"]}}},
+        )
+
     @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")