Rework conversion handling to prevent subtle errors (#80)

Fixes #63

Error on invalid formats for both global & local config, for the drawio valid formats and valid builder formats.

Changes imgconverter tests to verify no unnecessary conversions and no erroring when using that extension.

sphinxcontrib/drawio/__init__.py

@@ -16,6 +16,7 @@
 from docutils.parsers.rst import directives
 from docutils.parsers.rst.directives.images import Image
 from sphinx.application import Sphinx
+from sphinx.builders import Builder
 from sphinx.config import Config, ENUM
 from sphinx.directives.patches import Figure
 from sphinx.errors import SphinxError
@@ -24,12 +25,16 @@
 from sphinx.util.docutils import SphinxDirective
 from sphinx.util.fileutil import copy_asset
 
-
 __version__ = "0.0.16"
 
 logger = logging.getLogger(__name__)
 
-VALID_OUTPUT_FORMATS = ("png", "jpg", "svg", "pdf")
+VALID_OUTPUT_FORMATS = {
+    "png": "image/png",
+    "jpg": "image/jpeg",
+    "svg": "image/svg+xml",
+    "pdf": "application/pdf",
+}
 
 
 def is_headless(config: Config):
@@ -49,7 +54,22 @@ class DrawIOError(SphinxError):
 
 
 def format_spec(argument: Any) -> str:
-    return directives.choice(argument, VALID_OUTPUT_FORMATS)
+    return directives.choice(argument, list(VALID_OUTPUT_FORMATS.keys()))
+
+
+def is_valid_format(format: str, builder: Builder) -> str:
+    mimetype = VALID_OUTPUT_FORMATS.get(format, None)
+
+    if format is None:
+        return None
+    elif mimetype is None:
+        raise DrawIOError(f"export format '{format}' is unsupported by draw.io")
+    elif mimetype not in builder.supported_image_types:
+        raise DrawIOError(
+            f"invalid export format '{format}' specified for builder '{builder.name}'"
+        )
+    else:
+        return format
 
 
 def boolean_spec(argument: Any) -> bool:
@@ -120,14 +140,9 @@ class DrawIOConverter(ImageConverter):
 
     def __init__(self, *args: Any, **kwargs: Any) -> None:
         super().__init__(*args, **kwargs)
-        builder_name = self.app.builder.name
-        format = self.config.drawio_builder_export_format.get(builder_name)
-        if format and format not in VALID_OUTPUT_FORMATS:
-            raise DrawIOError(
-                f"Invalid export format '{format}' specified for builder"
-                f" '{builder_name}'"
-            )
-        self._default_export_format = format
+        format = self.config.drawio_builder_export_format.get(self.app.builder.name)
+
+        self._default_export_format = is_valid_format(format, self.app.builder)
 
     @property
     def imagedir(self) -> str:
@@ -139,14 +154,17 @@ def is_available(self) -> bool:
 
     def guess_mimetypes(self, node: nodes.image) -> List[str]:
         if "drawio" in node["classes"]:
-            format = node.get("format") or self._default_export_format
+            node_format = is_valid_format(node.get("format"), self.app.builder)
+            format = node_format or self._default_export_format
             extra = "-{}".format(format) if format else ""
             return ["application/x-drawio" + extra]
-        return [None]
+        else:
+            return []
 
     def handle(self, node: nodes.image) -> None:
         """Render drawio file into an output image file."""
         _from, _to = self.get_conversion_rule(node)
+
         if _from in node["candidates"]:
             srcpath = node["candidates"][_from]
         else:
@@ -183,6 +201,11 @@ def page_name_to_index(input_abspath: str, name: str):
             "draw.io file {} has no diagram named: {}".format(input_abspath, name)
         )
 
+    @staticmethod
+    def num_pages_in_file(input_abspath: Path) -> int:
+        # Each diagram/page is a direct child of the root element
+        return len(ET.parse(input_abspath).getroot())
+
     def _drawio_export(self, input_abspath, options, out_filename):
         builder = self.app.builder
         input_relpath = input_abspath.relative_to(builder.srcdir)
@@ -195,6 +218,12 @@ def _drawio_export(self, input_abspath, options, out_filename):
 
         if page_name:
             page_index = self.page_name_to_index(input_abspath, page_name)
+        elif page_index:
+            max_index = self.num_pages_in_file(input_abspath) - 1
+            if page_index > max_index:
+                logger.warning(
+                    f"selected page {page_index} is out of range [0,{max_index}]"
+                )
         elif page_index is None:
             page_index = 0
 

tests/roots/test-bad-config2/conf.py

@@ -0,0 +1,9 @@
+extensions = ["sphinxcontrib.drawio"]
+
+master_doc = "index"
+exclude_patterns = ["_build"]
+
+# removes most of the HTML
+html_theme = "basic"
+
+drawio_builder_export_format = {"html": "pdf"}

tests/roots/test-bad-config2/index.rst

@@ -0,0 +1 @@
+.. drawio-image:: box.drawio

tests/roots/test-imgconverter/conf.py

@@ -5,3 +5,5 @@
 
 # removes most of the HTML
 html_theme = "basic"
+
+drawio_builder_export_format = {"html": "svg", "latex": "pdf"}

tests/roots/test-page-index-out-of-range/conf.py

@@ -0,0 +1,7 @@
+extensions = ["sphinxcontrib.drawio"]
+
+master_doc = "index"
+exclude_patterns = ["_build"]
+
+# removes most of the HTML
+html_theme = "basic"

tests/roots/test-page-index-out-of-range/index.rst

@@ -0,0 +1,4 @@
+.. drawio-image:: pages.drawio
+    :format: png
+    :alt: out of range
+    :page-index: 6

tests/roots/test-page-index-out-of-range/pages.drawio

@@ -0,0 +1 @@
+<mxfile host="Electron" modified="2022-12-21T12:52:56.968Z" agent="5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) draw.io/20.6.2 Chrome/106.0.5249.199 Electron/21.3.3 Safari/537.36" etag="5HcsiU377iumTImNEKXf" version="20.6.2" type="device" pages="6"><diagram id="GZmhYcr-ncgRq0jOcgJH" name="A">rZRNc4IwEIZ/DcfOKIhtjwp+dEbb6TjWTm8pWSFtyDIhFPHXN5EgUu20B09snt1Ndt9NcLwg3c0kyZIlUuCO26M7xwsd1+17w57+GFLV5M4b1iCWjNqgFqzYHiy0eXHBKOSdQIXIFcu6MEIhIFIdRqTEshu2Rd49NSMxnIFVRPg53TCqEtuFe9vyObA4aU7uD+9rT0qaYNtJnhCK5QnyJo4XSERVW+kuAG7Ea3Sp86a/eI+FSRDqPwlDdxo/pbheTGj6OsgGD2/P1Y3d5YvwwjZsi1VVo4DEQlAwm/Qcb1wmTMEqI5HxlnrmmiUq5XrV1+Z5Uc0JIBXsTpAtcgaYgpKVDrHeRq+quyxb9fsNS06Uby4asQOPjxu3mmjDynJZoiScFOvBxyMPXpbzzSIcsNH6gkSjM410a6orhISc7cn7IcDoRgqFeX27jZtwFgttR1onkBoYfZi+diPrSBmlJnmcIRPq0JM/dvxQky3jPECOOi0UKExQriR+wg/YHdwWhbLPSz/Bq0zK9/+alH+dQell+04OvpO/jTf5Bg==</diagram><diagram name="B" id="6fUARE8VIy0xdAgN1t-w">tVTfT4MwEP5reFyyQWD6KGxDYzQxM9l8MpXeoFp6WLrB9tfbjjLAmaiJPnH97kd733eH40V5HUtSZHdIgTvumNaON3Ncd+IFY/0xyL5BLrygAVLJqA3qgCU7gAVtXrplFMpBoELkihVDMEEhIFEDjEiJ1TBsg3x4a0FSOAOWCeHn6IpRldku3GmHXwNLs/bmSXDZeHLSBttOyoxQrHqQN3e8SCKqxsrrCLghr+Uluo/99eLpcL2cBu/lnD5vHsioKbb4TcqpBQlC/W1ptym9I3xr+bK9qn1LoMStoGCKjB0vrDKmYFmQxHgrPTIay1TO9WmizR++1Ha0A6mg7ulkXx4D5qDkXodYb6vBfnisOkXbOc16YrYYsTOUnup2PGnDUvU1bQHm6+Cu5mG2isvLx5vX8nZkme7TFp7xpjtTQ3IklOxAXo4BhkuyVVg2C2PchLNUaDvR3IHUgKGH6Um+so6cUWqSwwKZUMee/NDxZxrZMM4j5KjTZgKFCSqVxDf4BA7F3KBQdmP1Vv+jev536vl/o54+dvt49PX+at78Aw==</diagram><diagram id="1kS-xlAtLlubi-TgpoV-" name="C">jZNNb4MwDIZ/DcdJQARbj4N2H9K6S6VOO6bEQLQQoxBa2l+/UEwpqyrthPPYjh37xWNp1b0aXpdrFKC80Bedx5ZeGAYs9t2nJ8eBPLF4AIWRgoImsJEnIEh5RSsFNLNAi6isrOcwQ60hszPGjcHDPCxHNa9a8wJuwCbj6pZ+SWFLekX4OPE3kEU5Vg7ixeCp+BhML2lKLvBwhdjKY6lBtINVdSmofnjjXIa8lzveS2MGtP1PQsy3ebw4fVSf799tlmzXvr97oFv2XLX04JS6tcdxBBY6VyApbaUcCJxpoJEnvjsH+O7MW4vNsLrezZUstLMz1xkYB/ZgrHQzfSZHJYXok5MapbbnDUWJFy0dyaVSKSp0aUuNug9qrMEf+AMNtlqAoAZy1Ja04/TFEnqVKwvd3XEFlyU49QJWYM3RhVBCGA0ZpNtxi4dJBBGh8mr/o9w5ya643Dttxhm0nPE4ieDsu/qV2OoX</diagram><diagram id="PmvFIfAPNmRKBYWDG9Gn" name="D">jZNNb4MwDIZ/DcdJhYx+HAt026W7dB/HKW1ciBZiFkIL/fULxZSyqtJOOI/t2LFfPBbn9bPhRbZGAcoLJqL2WOIFgc+mE/dpSdOROZt2IDVSUNAANvIEBCkvraSAchRoEZWVxRjuUGvY2RHjxuBxHLZHNa5a8BRuwGbH1S39lMJm9IpgNvAXkGnWV/ani86T8z6YXlJmXODxCrGVx2KDaDsrr2NQ7fD6uXR5T3e8l8YMaPufhPfZOvx5+/havZp4uV0Ey/lj80C3HLiq6MEJdWubfgQWalcgymyuHPCdaaCUJ749B0zcmVcWy251rZsrmWpn71xnYBw4gLHSzXRJjlwK0SZHBUptzxsKIy9MHNlLpWJU6NISjboNKq3Bb/gDDVZagKAG9qgtacfpi0X0KlcW6rvj8i9LcOoFzMGaxoVQQhB2GaTbfovHQQQhoexq/73cOckuvdw7bMYZtJz+OIjg7Lv6ldjqFw==</diagram><diagram id="WuyL8PAHfq40Bik2V_-U" name="E">jZPBboMwDIafhuOkQkTXHVfoWmnaoeqh54y4EC3EKISW9ulniillVaWdcD7bsWP/BCIp27WTVfGFCkwQzVQbiDSIolDMZ/TpyLknCzHvQe604qAR7PQFGHJe3mgF9STQIxqvqynM0FrI/IRJ5/A0DTugmVatZA4PYJdJ80j3WvmCXxG9jnwDOi+GyuH8rfeUcgjml9SFVHi6Q2IViMQh+t4q2wRMN7xhLn3exxPvrTEH1v8nQdtokX5ui83eqmR7zKt1Zl74lqM0DT94xd368zACDy0VWBa+NARCMh3U+iK/rwEzOsvGY92vrnNLo3NLdkadgSNwBOc1zfSdHaVWqkteVqitv24oXgZxSuSgjUnQIKWlFm0XVHuHP/AHOmysAsUNHNB61g7pSyz5VVQW2qfjCm9LIPUCluDdmUI4IYr7DNbtsMXTKIKYUXG3/0HukmWX3+4dN0MGL2c4jiK4+u5+JbH6BQ==</diagram><diagram id="eSY2IZH2ICmWwmnWPC8n" name="F">jZNNb4MwDIZ/DcdJQATbjoN+HbbLepi0W0pciBbiKqQt7a+fKaaUVZV2wnlsx479Eoi8bpdO7qoPVGCCOFRtIGZBHEciDenTkVNPXkTag9JpxUEjWOszMOS8cq8VNJNAj2i83k1hgdZC4SdMOofHadgWzbTqTpZwB9aFNPf0Sytf8Svi55GvQJfVUDlKX3tPLYdgfklTSYXHGyTmgcgdou+tus3BdMMb5tLnLR54r405sP4/CUkoNut6tvz8FkWzSlXl37MnvuUgzZ4fvOBu/WkYgYeWCmSVrw2BiEwHjT7LzSUgpLPce2z61XVuaXRpyS6oM3AEDuC8ppm+saPWSnXJ2Q619ZcNJVmQzIhstTE5GqS0mUXbBTXe4Q/8gQ73VoHiBrZoPWuH9CUyfhWVhfbhuKLrEki9gDV4d6IQToiTPoN1O2zxOIogYVTd7H+Qu2TZldd7x82QwcsZjqMILr6bX0nMfwE=</diagram></mxfile>

tests/test_compat.py

@@ -5,14 +5,21 @@
 
 from bs4 import Tag
 
+# This tests two things:
+# - That it doesn't convert when unnecessary
+# - That it doesn't error.
+
+
+@pytest.mark.sphinx("latex", testroot="imgconverter")
+def test_pdfnoconvert(tex_images: List[Path]):
+    (image,) = tex_images
+    # It should not convert a PDF into another format.
+    assert image.basename() == "box.pdf"
+
 
-@pytest.mark.skip(reason="Somehow sphinx doesn't convert the svg to png images.")
 @pytest.mark.sphinx("html", testroot="imgconverter")
-def test_imgconverter(directives: List[Tag]):
-    (img,) = directives
-    assert img.name == "img"
-    # This will have been converted from our exported
-    # SVG to PNG by sphinx.ext.imgconverter
-    assert img["src"] == "_images/box.png"
-    assert img["alt"] == "_images/box.png"
-    assert img["class"] == ["drawio"]
+def test_noconvert(directives: List[Tag]):
+    # it should not convert an SVG output from sphinx into another format
+    (image,) = directives
+    assert image["src"] == "_images/box.svg"
+    assert image["alt"] == "_images/box.svg"

tests/test_options.py

@@ -21,6 +21,14 @@ def test_page_index(images: List[Path]):
     assert get_image_size(images[3]) == (125, 65)
 
 
+@pytest.mark.sphinx("html", testroot="page-index-out-of-range")
+def test_page_index_out_of_range(content: Sphinx, directives: List[Tag]):
+    assert len(directives) == 1
+
+    warnings = content._warning.getvalue()
+    assert "selected page 6 is out of range [0,5]" in warnings
+
+
 @pytest.mark.sphinx("html", testroot="page-name")
 def test_page_name(images: List[Path]):
     assert images[0].name == "pages.png"
@@ -115,7 +123,15 @@ def test_bad_config(app_with_local_user_config):
     with pytest.raises(DrawIOError) as exc:
         app_with_local_user_config.build()
     (message,) = exc.value.args
-    assert message == "Invalid export format 'bmp' specified for builder 'html'"
+    assert message == "export format 'bmp' is unsupported by draw.io"
+
+
+@pytest.mark.sphinx("html", testroot="bad-config2")
+def test_bad_config2(app_with_local_user_config):
+    with pytest.raises(DrawIOError) as exc:
+        app_with_local_user_config.build()
+    (message,) = exc.value.args
+    assert message == "invalid export format 'pdf' specified for builder 'html'"
 
 
 @pytest.mark.sphinx("html", testroot="page-name-not-exist")