Merge pull request #318 from open-dynaMIX/validate_image_placeholders

fix(validation): correctly validate image placeholders

USAGE.md

@@ -184,6 +184,18 @@ As you can see, the validation went through this time, as our sample data
 covers all placeholders used in the template. Of course, the template
 isn't required to use all placeholders available!
 
+If you use a Template with the DocxTpl syntax that uses [inline images](#inline-images),
+you also need to include the corresponding files along the `sample_data`. So the `files`
+in the example above would become something like:
+
+```python
+...     files=(
+...         ("template", open('docx-mailmerge.docx', 'rb'))),
+...         ("files", ("sunset1.png", open('sunset1.png', 'rb'))),
+...         ("files", ("sunset2.png", open('sunset2.png', 'rb'))),
+...     ),
+```
+
 
 ## Merging templates
 

document_merge_service/api/engines.py

@@ -1,4 +1,5 @@
 import io
+import re
 import zipfile
 
 from docx import Document
@@ -7,6 +8,8 @@
 from mailmerge import MailMerge
 from rest_framework import exceptions
 
+from document_merge_service.api.data import django_file
+
 from . import models
 from .jinja import get_jinja_env
 
@@ -106,6 +109,15 @@ def validate_template_syntax(self, available_placeholders=None, sample_data=None
             ph = {
                 name: root[name] for name in doc.get_undeclared_template_variables(env)
             }
+
+            xml = doc.get_xml()
+            xml = doc.patch_xml(xml)
+            image_match = re.match(r".*{{\s?(\S*)\s?\|\s?image\(.*", xml)
+            images = image_match.groups() if image_match else []
+            for image in images:
+                cleaned_image = image.strip('"').strip("'")
+                ph[root[cleaned_image]] = django_file("black.png").file
+
             ph["_tpl"] = doc
 
             doc.render(ph, env)

document_merge_service/api/serializers.py

@@ -7,6 +7,18 @@
 from . import engines, models
 
 
+class CustomFileField(serializers.FileField):
+    """
+    Custom FileField.
+
+    `to_representation()` of this FileField returns the file object instead of just the
+    filename.
+    """
+
+    def to_representation(self, value):
+        return value or None
+
+
 class CurrentGroupDefault:
     requires_context = True
 
@@ -18,6 +30,9 @@ class TemplateSerializer(serializers.ModelSerializer):
     group = serializers.CharField(allow_null=True, default=CurrentGroupDefault())
     available_placeholders = serializers.ListField(allow_null=True, required=False)
     sample_data = serializers.JSONField(allow_null=True, required=False)
+    files = serializers.ListField(
+        child=CustomFileField(write_only=True, allow_empty_file=False), required=False
+    )
 
     def validate_group(self, group):
         request = self.context["request"]
@@ -53,13 +68,27 @@ def validate(self, data):
 
         available_placeholders = data.pop("available_placeholders", None)
         sample_data = data.pop("sample_data", None)
+        files = data.pop("files", None)
 
         if sample_data and available_placeholders:
             raise exceptions.ValidationError(
                 "Only one of available_placeholders and sample_data is allowed"
             )
+        elif files and engine != models.Template.DOCX_TEMPLATE:
+            raise exceptions.ValidationError(
+                f'Files are only accepted with the "{models.Template.DOCX_TEMPLATE}"'
+                f" engine"
+            )
         elif sample_data:
+            if files:
+                for file in files:
+                    sample_data[file.name] = file
+
             available_placeholders = self._sample_to_placeholders(sample_data)
+        elif files:
+            raise exceptions.ValidationError(
+                "Files are only accepted when also providing sample_data"
+            )
 
         engine = engines.get_engine(engine, template)
         engine.validate(
@@ -78,21 +107,10 @@ class Meta:
             "group",
             "available_placeholders",
             "sample_data",
+            "files",
         )
 
 
-class CustomFileField(serializers.FileField):
-    """
-    Custom FileField.
-
-    `to_representation()` of this FileField returns the file object instead of just the
-    filename.
-    """
-
-    def to_representation(self, value):
-        return value or None
-
-
 class TemplateMergeSerializer(serializers.Serializer):
     data = serializers.JSONField(
         required=True, help_text="Data as json used for merging"

document_merge_service/api/tests/test_template.py

@@ -7,8 +7,9 @@
 from lxml import etree
 from rest_framework import status
 
+from document_merge_service.api.data import django_file
+
 from .. import models, serializers
-from .data import django_file
 
 
 @pytest.mark.parametrize(
@@ -174,24 +175,54 @@ def test_template_create(
 
 
 @pytest.mark.parametrize(
-    "template_name,available_placeholders,sample_data,expect_missing_placeholders,engine,status_code",
+    "template_name,available_placeholders,sample_data,files,expect_missing_placeholders,engine,status_code",
     [
         (
             "docx-template-placeholdercheck.docx",
             ["foo", "bar", "baz"],
             None,
-            ["bar.some_attr", "list", "list[]", "list[].attribute"],
+            [],
+            [
+                "bar.some_attr",
+                "black.png",
+                "list",
+                "list[]",
+                "list[].attribute",
+            ],
             models.Template.DOCX_TEMPLATE,
             status.HTTP_400_BAD_REQUEST,
         ),
         (
             "docx-template-placeholdercheck.docx",
-            ["foo", "bar", "baz", "bar.some_attr", "list[].attribute"],
+            [
+                "foo",
+                "bar",
+                "baz",
+                "bar.some_attr",
+                "list[].attribute",
+                "black.png",
+            ],
             None,
             [],
+            [],
             models.Template.DOCX_TEMPLATE,
             status.HTTP_201_CREATED,
         ),
+        (
+            "docx-template-placeholdercheck.docx",
+            [
+                "foo",
+                "bar",
+                "baz",
+                "bar.some_attr",
+                "list[].attribute",
+            ],
+            None,
+            [],
+            ["black.png"],
+            models.Template.DOCX_TEMPLATE,
+            status.HTTP_400_BAD_REQUEST,
+        ),
         (
             "docx-template-placeholdercheck.docx",
             None,
@@ -204,10 +235,37 @@ def test_template_create(
                 "baz": "1234",
                 "list": [{"attribute": "value"}],
             },
+            [django_file("black.png").file],
             [],
             models.Template.DOCX_TEMPLATE,
             status.HTTP_201_CREATED,
         ),
+        (
+            "docx-template-placeholdercheck.docx",
+            None,
+            {},
+            [django_file("black.png").file],
+            [],
+            models.Template.DOCX_TEMPLATE,
+            status.HTTP_400_BAD_REQUEST,
+        ),
+        (
+            "docx-template-placeholdercheck.docx",
+            None,
+            {
+                "foo": "hello",
+                "bar": {
+                    "some_attr": True,
+                    "list": [{"attribute": "value"}, {"attribute": "value2"}],
+                },
+                "baz": "1234",
+                "list": [{"attribute": "value"}],
+            },
+            [],
+            ["black.png"],
+            models.Template.DOCX_TEMPLATE,
+            status.HTTP_400_BAD_REQUEST,
+        ),
         (
             "docx-template-placeholdercheck.docx",
             None,
@@ -218,6 +276,7 @@ def test_template_create(
                     "list": [{"attribute": "value"}, {"attribute": "value2"}],
                 },
             },
+            [django_file("black.png").file],
             ["baz", "list", "list[]", "list[].attribute"],
             models.Template.DOCX_TEMPLATE,
             status.HTTP_400_BAD_REQUEST,
@@ -232,6 +291,22 @@ def test_template_create(
                     "list": [{"attribute": "value"}, {"attribute": "value2"}],
                 },
             },
+            [],
+            ["test"],
+            models.Template.DOCX_MAILMERGE,
+            status.HTTP_400_BAD_REQUEST,
+        ),
+        (
+            "docx-mailmerge.docx",
+            None,
+            {
+                "foo": "hello",
+                "bar": {
+                    "some_attr": True,
+                    "list": [{"attribute": "value"}, {"attribute": "value2"}],
+                },
+            },
+            [],
             ["test"],
             models.Template.DOCX_MAILMERGE,
             status.HTTP_400_BAD_REQUEST,
@@ -241,6 +316,7 @@ def test_template_create(
             None,
             {"test": "hello"},
             [],
+            [],
             models.Template.DOCX_MAILMERGE,
             status.HTTP_201_CREATED,
         ),
@@ -249,6 +325,16 @@ def test_template_create(
             ["test", "blah"],
             {"test": "hello"},
             [],
+            [],
+            models.Template.DOCX_MAILMERGE,
+            status.HTTP_400_BAD_REQUEST,
+        ),
+        (
+            "docx-mailmerge.docx",
+            [],
+            {"test": "hello"},
+            [django_file("black.png").file],
+            [],
             models.Template.DOCX_MAILMERGE,
             status.HTTP_400_BAD_REQUEST,
         ),
@@ -261,6 +347,7 @@ def test_template_create_with_available_placeholders(
     template_name,
     available_placeholders,
     sample_data,
+    files,
     status_code,
     settings,
     expect_missing_placeholders,
@@ -270,7 +357,12 @@ def test_template_create_with_available_placeholders(
     url = reverse("template-list")
 
     template_file = django_file(template_name)
-    data = {"slug": "test-slug", "template": template_file.file, "engine": engine}
+    data = {
+        "slug": "test-slug",
+        "template": template_file.file,
+        "files": files,
+        "engine": engine,
+    }
     if sample_data:
         data["sample_data"] = json.dumps(sample_data)
     if available_placeholders:
@@ -289,6 +381,16 @@ def test_template_create_with_available_placeholders(
                 resp["non_field_errors"][0]
                 == "Only one of available_placeholders and sample_data is allowed"
             )
+        elif engine == models.Template.DOCX_MAILMERGE and files:
+            assert (
+                resp["non_field_errors"][0]
+                == 'Files are only accepted with the "docx-template" engine'
+            )
+        elif not sample_data and files:
+            assert (
+                resp["non_field_errors"][0]
+                == "Files are only accepted when also providing sample_data"
+            )
         else:
             # we expect some missing placeholders
             assert (
@@ -521,11 +623,11 @@ def test_template_merge_jinja_filters_docx(
     }
 
     if not missing_file:
-        file = django_file("python-powered.png").file
+        file = django_file("black.png").file
         if wrong_mime:
-            # create a file with the correct filename (python-powered.png) but with
+            # create a file with the correct filename (black.png) but with
             # the contents of a docx.
-            file = tmp_path / "python-powered.png"
+            file = tmp_path / "black.png"
             for line in template.template.file:
                 file.write_bytes(line)
             file = file.open("rb")

document_merge_service/conftest.py

@@ -8,9 +8,10 @@
 from pytest_factoryboy import register
 from rest_framework.test import APIClient
 
+from document_merge_service.api.data import django_file
+
 from .api import engines
 from .api.authentication import AnonymousUser
-from .api.tests.data import django_file
 
 
 def register_module(module):