From 970eb55a707a877c1aeaf5eb272ca8f6f694c635 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Tue, 15 Sep 2026 08:43:21 -0700 Subject: [PATCH] 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 | 78 ++++++++++---------- src/documents/tests/test_api_bulk_edit.py | 90 +++++++++++++++++++++-- 2 files changed, 119 insertions(+), 49 deletions(-) diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index 9cd8d187a..c13cf23d8 100644 --- a/src/documents/serialisers.py +++ b/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.", ) diff --git a/src/documents/tests/test_api_bulk_edit.py b/src/documents/tests/test_api_bulk_edit.py index ebde4ff1f..cdced37c6 100644 --- a/src/documents/tests/test_api_bulk_edit.py +++ b/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")