From 4d2f9cb8c1b9e192d4cab3355d0bb9db28bc74f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillermo=20Dols=20Garc=C3=ADa?= Date: Wed, 12 Aug 2026 10:00:37 +0200 Subject: [PATCH] fix: return a placeholder when a chart read fails instead of None _convert_chart_to_markdown returns "[unsupported chart]" from both of its exception handlers, but the ValueError handler returned it only when the message contained "unsupported plot type". Every other ValueError reached the end of that handler and fell off the function, so the method returned None. The caller concatenates the result onto md_content unguarded, so the None raises TypeError: can only concatenate str (not "NoneType") to str. That is caught by MarkItDown._convert and re-raised as FileConversionException, so a single unreadable chart loses the whole presentation rather than one shape. Both handlers produced the same string, so collapsing them into one except Exception preserves the output and removes the None path. The same defect is present in the copy carried by markitdown-ocr. --- .../_pptx_converter_with_ocr.py | 5 +- .../markitdown/converters/_pptx_converter.py | 11 +- .../tests/test_pptx_chart_errors.py | 113 ++++++++++++++++++ 3 files changed, 121 insertions(+), 8 deletions(-) create mode 100644 packages/markitdown/tests/test_pptx_chart_errors.py diff --git a/packages/markitdown-ocr/src/markitdown_ocr/_pptx_converter_with_ocr.py b/packages/markitdown-ocr/src/markitdown_ocr/_pptx_converter_with_ocr.py index 7e91ed6b4..8f0d4a3d5 100644 --- a/packages/markitdown-ocr/src/markitdown_ocr/_pptx_converter_with_ocr.py +++ b/packages/markitdown-ocr/src/markitdown_ocr/_pptx_converter_with_ocr.py @@ -242,8 +242,7 @@ def _convert_chart_to_markdown(self, chart): header = markdown_table[0] separator = "|" + "|".join(["---"] * len(data[0])) + "|" return md + "\\n".join([header, separator] + markdown_table[1:]) - except ValueError as e: - if "unsupported plot type" in str(e): - return "\\n\\n[unsupported chart]\\n\\n" except Exception: + # Every ValueError other than "unsupported plot type" used to fall + # through and return None, which the caller then concatenated. return "\\n\\n[unsupported chart]\\n\\n" diff --git a/packages/markitdown/src/markitdown/converters/_pptx_converter.py b/packages/markitdown/src/markitdown/converters/_pptx_converter.py index 21a12dc81..da62a06bd 100644 --- a/packages/markitdown/src/markitdown/converters/_pptx_converter.py +++ b/packages/markitdown/src/markitdown/converters/_pptx_converter.py @@ -328,10 +328,11 @@ def _convert_chart_to_markdown(self, chart): header = markdown_table[0] separator = "|" + "|".join(["---"] * len(data[0])) + "|" return md + "\n".join([header, separator] + markdown_table[1:]) - except ValueError as e: - # Handle the specific error for unsupported chart types - if "unsupported plot type" in str(e): - return "\n\n[unsupported chart]\n\n" except Exception: - # Catch any other exceptions that might occur + # Any failure to read the chart degrades to a placeholder. This + # includes the ValueError("unsupported plot type ...") that + # python-pptx raises for chart types it cannot model, which used to + # be the only ValueError handled here: every other one fell through + # and returned None, which the caller then concatenated onto + # md_content, failing the whole presentation with a TypeError. return "\n\n[unsupported chart]\n\n" diff --git a/packages/markitdown/tests/test_pptx_chart_errors.py b/packages/markitdown/tests/test_pptx_chart_errors.py new file mode 100644 index 000000000..ad68f2114 --- /dev/null +++ b/packages/markitdown/tests/test_pptx_chart_errors.py @@ -0,0 +1,113 @@ +#!/usr/bin/env python3 -m pytest +"""Tests for chart reads that fail inside PptxConverter. + +``_convert_chart_to_markdown`` wraps its body in two handlers that both return +the same ``[unsupported chart]`` placeholder, but the ``ValueError`` handler +returned it only when the message contained "unsupported plot type". Any other +``ValueError`` reached the end of the handler and fell off the function, so the +method returned ``None``. + +The caller does ``md_content += self._convert_chart_to_markdown(shape.chart)`` +unguarded, so that ``None`` raises ``TypeError: can only concatenate str (not +"NoneType") to str``. ``MarkItDown._convert`` catches it and, with no other +converter accepting a .pptx, re-raises as ``FileConversionException`` -- one +unreadable chart therefore loses the entire presentation rather than a single +shape. +""" + +import io +from unittest.mock import patch + +import pytest +from pptx import Presentation +from pptx.chart.chart import Chart +from pptx.chart.data import CategoryChartData +from pptx.enum.chart import XL_CHART_TYPE +from pptx.util import Inches + +from markitdown import MarkItDown +from markitdown._stream_info import StreamInfo +from markitdown.converters._pptx_converter import PptxConverter + +PLACEHOLDER = "[unsupported chart]" + +# A ValueError python-pptx can raise that is not the special-cased one. +OTHER_VALUE_ERROR = "chart has no categories" + + +def _deck_with_chart() -> bytes: + """A minimal one-slide deck holding a plain clustered column chart.""" + presentation = Presentation() + slide = presentation.slides.add_slide(presentation.slide_layouts[5]) + chart_data = CategoryChartData() + chart_data.categories = ["A", "B"] + chart_data.add_series("S1", (1.0, 2.0)) + slide.shapes.add_chart( + XL_CHART_TYPE.COLUMN_CLUSTERED, + Inches(1), + Inches(1), + Inches(4), + Inches(3), + chart_data, + ) + buffer = io.BytesIO() + presentation.save(buffer) + return buffer.getvalue() + + +def _convert(deck: bytes) -> str: + return ( + MarkItDown() + .convert_stream(io.BytesIO(deck), stream_info=StreamInfo(extension=".pptx")) + .markdown + ) + + +class _ChartRaising: + """Stands in for a chart whose first access raises.""" + + def __init__(self, message: str): + self._message = message + + @property + def has_title(self): + raise ValueError(self._message) + + +@pytest.mark.parametrize("message", [OTHER_VALUE_ERROR, "unsupported plot type xlFoo"]) +def test_chart_read_failure_returns_a_string(message: str) -> None: + """The method must never hand the caller a None to concatenate.""" + result = PptxConverter()._convert_chart_to_markdown(_ChartRaising(message)) + + assert isinstance(result, str) + assert PLACEHOLDER in result + + +def test_deck_survives_an_unreadable_chart() -> None: + """A chart that cannot be read costs the chart, not the presentation.""" + deck = _deck_with_chart() + + def _raise(self): + raise ValueError(OTHER_VALUE_ERROR) + + with patch.object(Chart, "plots", property(_raise)): + markdown = _convert(deck) + + assert PLACEHOLDER in markdown + + +def test_readable_chart_is_still_converted() -> None: + """Control: an ordinary chart must keep converting to a table.""" + markdown = _convert(_deck_with_chart()) + + assert "### Chart" in markdown + assert PLACEHOLDER not in markdown + assert "| Category | S1 |" in markdown + + +if __name__ == "__main__": + test_chart_read_failure_returns_a_string(OTHER_VALUE_ERROR) + test_chart_read_failure_returns_a_string("unsupported plot type xlFoo") + test_deck_survives_an_unreadable_chart() + test_readable_chart_is_still_converted() + print("All tests passed!")