diff --git a/.gitignore b/.gitignore index faab1cb..cb85322 100644 --- a/.gitignore +++ b/.gitignore @@ -37,3 +37,8 @@ docs/glpi_api_contract.json # Coverage data: rewritten by every test run (and by the venv .pth hook). .coverage .coverage.* + +# One-shot live-instance probe scripts. They are standalone investigations +# (a main() run by hand against preprod), not collected tests -- the findings +# get written up in CHANGELOG.md, the scripts stay local. +integration_tests/probe_*.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c6f1c90..b1dc065 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,361 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## 0.5.0 — 2026-09-08 + +### Fixed + +- **Deeply nested HTML raised `RecursionError` while a model was being + validated.** `markdownify` walks the parsed document recursively and + spends about two CPython frames per nesting level, so roughly 494 levels + exhausted the default 1000-frame limit — measured, and the same 494 for + `
`, `

`, `

` and `
`, 495 for + `
  • `, which is what identifies the cost as per-level. An + unclosed tag counts too: `html.parser` does not auto-close `

    ` or + `

  • `, so `"

    " * 5000` really is 5000 levels. + + Because the converter was wired as a Pydantic `BeforeValidator`, the + failure landed inside `model_validate` — that is, inside `get_ticket` — + as a bare builtin from a library whose whole error surface is supposed + to derive from `GlpiError`. + + `from_transport` now *attempts* the conversion and answers the + `RecursionError` by stripping the document to its text instead. + **It degrades, it never truncates, and it does not raise for depth**: + every character of prose the converting path would have produced also + appears in the degraded rendering. + + Attempting it rather than predicting it is the whole design, and it + replaced a fixed `MAX_HTML_DEPTH = 200` bound that was wrong in both + directions. Too low, because the budget is not 1000 frames but whatever + is left of the stack when the conversion starts, and that belongs to + the caller — so the bound had to assume the worst and flattened every + body between 200 and the real cliff of about 494. Measured, a + 300-level and a 400-level body now come back as **Markdown with their + links, emphasis and lists intact** where they used to come back as + plain text, with no error to notice and no way to ask for better. And + too fragile, because predicting the depth meant reproducing the + parser's idea of the tree: three rounds of adversarial review found + seven ways for that estimate to land *under* the real depth, each of + which sent a document to `markdownify` and into the very + `RecursionError` the bound existed to prevent. + + Trying the conversion cannot be wrong about whether the conversion + fits. `MAX_HTML_DEPTH` and the scan behind it are gone; the constant + was introduced in this same unreleased cycle and never shipped. + + Two consequences worth knowing. The outcome now depends on the caller's + remaining stack, so the same body can convert from one call site and + degrade from a deeper one — nothing is lost either way, but a caller + comparing two renderings of one body should know which knob moved it. + And a body too deep to convert now pays the failed attempt before it + degrades: measured, 2.0x to 2.6x the old cost at 600 and 5000 levels. + Ordinary bodies got *faster*, at 0.87x to 0.90x, because the scan they + used to pay for on every read is gone. + + A document `html.parser` refuses outright — `

    " * 600` really is 600 + deep where a naive counter says 1. + - An attribute value may contain `<` and `>`, so + `'
    ' * 600` also measured 0 against a real 600 + until the scan learned to skip quoted values. + - A quote opens a value only as the first character after the `=`, + which is the parser's own rule, so `

    ` carries the + value `don't`. Reading that apostrophe as a quote printed the opening + tag verbatim at the reader — and an apostrophe needs no malice to + reach a French ticket body. + - `tagfind_tolerant` runs a tag *name* to whitespace, `/` or `>`, so + `` is an element named `style=` and never enters raw-text + mode; a self-closed `", True, id="script-body"), + pytest.param("", True, id="style-body"), + pytest.param("", True, id="marked-section"), + pytest.param("", True, id="unterminated-marked-section"), + pytest.param("", True, id="processing-instruction"), + ], +) +def test_the_degraded_path_keeps_exactly_what_the_converter_keeps( + construct: str, kept: bool +) -> None: + """Parity, construct by construct, and not one of these was a guess. + + Each expectation here was read off the converting path rather than + reasoned about, and three came back the opposite way round from the + obvious answer -- a ``x", id="in-a-script-body"), + pytest.param("x", id="in-a-comment"), + pytest.param("

    a
    b

    ", id="slash-not-abutting-gt"), + ], +) +def test_the_void_rewrite_leaves_everything_else_alone(html: str) -> None: + """The rewrite is confined to void tags in real tag position. + + ``
    `` is left as it is -- rewriting it would change what the + document means, and it cannot be affected anyway, since only a void + name is ever recorded as already closed. The last case is the one + worth pinning: ``
    `` reaches the parser as an ordinary start + tag, because its ``/`` does not abut the ``>``, so it never takes the + path that loses text and needs no rewriting. + """ + + assert conversion._canonicalise_void_elements(html) == html + + +def test_the_void_rewrite_changes_nothing_for_one_spelling_alone() -> None: + """A body that picks a spelling and keeps it converts exactly as before. + + The rewrite exists to remove an asymmetry between two spellings of the + same node, so it must be invisible to every body that does not mix + them. Measured over 4000 fuzzed documents of each spelling: not one + output moved. + """ + + bare = "

    a
    b


    c

    " + slashed = "

    a
    b


    c

    " + + assert GlpiContentConverter.from_transport(bare) == "a \nb![]()\n\n---\n\nc" + assert GlpiContentConverter.from_transport(slashed) == "a \nb![]()\n\n---\n\nc" + + +def test_both_paths_agree_on_a_body_using_both_spellings() -> None: + """The degraded path already kept this text; now the converting one does. + + This body was the one place where the fallback said *more* than the + conversion it stands in for, which is the wrong way round for a + fallback and was how the defect was noticed at all. + """ + + shallow = "

    one
    two
    three

    " + deep = "
    " * 600 + shallow + "
    " * 600 + + converted = GlpiContentConverter.from_transport(shallow) + degraded = GlpiContentConverter.from_transport(deep) + + for word in ("one", "two", "three"): + assert word in converted + assert word in degraded + + +@pytest.mark.parametrize( + "fragment", + [ + pytest.param('', id="end-tag-with-a-quoted-attribute"), + pytest.param('
    ', id="name-less-equals-quote"), + pytest.param('

    ', id="quoted-gt-then-tag"), + ], +) +def test_a_misread_tag_end_does_not_swallow_the_body_after_it(fragment: str) -> None: + """Scaled past the cliff: degrades quietly, and keeps its words. + + Reading an end tag with start-tag rules consumed everything up to the + next quote, which deleted prose at any depth and under-counted the + nesting 1:1 with the repetition back when the nesting was predicted. + """ + + html = fragment * 600 + "the printer is offline" + + assert "the printer is offline" in GlpiContentConverter.from_transport(html) + + +def test_a_misread_tag_end_does_not_delete_prose() -> None: + """The other half of the same defect, and it needs no depth at all. + + Reading an end tag with attribute rules consumed everything between + the opening quote and its partner, so a degraded body said less than + the converting one -- the divergence the parity test forbids. + """ + + body = '

    Bonjour

    fin' + + assert_the_degraded_path_says_no_less(body) + assert "Bonjour" in GlpiContentConverter.from_transport(body) + + +def test_a_document_with_no_closing_bracket_is_answered_without_scanning() -> None: + """No ``>`` means no element, and saying so keeps a bad shape cheap. + + ``html.parser`` cannot finish a tag that never closes, so ``close()`` + flushes it one character at a time and rescans the tail at each step: + measured, 32 KB of ``'
    None: + """The give-up point is not the end of the body. + + The scan stops where the parser stopped, so everything past that + construct would go missing unless it is handed back explicitly -- + and a body is far more likely to carry the marked section in the + middle than at the end. + """ + + html = "

    avant

    apres SECRET

    " + + assert "avant" in _strip_tags(html) + assert "SECRET" in _strip_tags(html) + assert "SECRET" in GlpiContentConverter.from_transport(html) + assert "avant" in GlpiContentConverter.from_transport(html) + + +def test_stripping_a_document_with_no_closing_bracket_keeps_all_of_it() -> None: + """The degraded path needs the same guard the depth scan needs. + + ``html.parser`` cannot complete a tag that never closes, so + ``close()`` flushes it one character at a time and rescans the tail + at each step: measured, 32 KB of ``'

    " * 60 + "kept", id="stray-close-p"), + pytest.param("
    " * 60 + "kept", id="stray-close-span"), + pytest.param("

    " * 60 + "kept", id="stray-close-b"), + pytest.param("x", id="interleaved"), + pytest.param("

    " * 100 + "
    " + "
    " * 100, id="void-leaf"), + pytest.param("
    " * 100 + "" + "
    " * 100, id="self-closed-leaf"), + pytest.param("
    " * 5000, id="void-only"), + pytest.param("
    x
    ", id="close-of-a-void"), + pytest.param("

    The printer is offline.

    ", id="realistic"), + pytest.param("

    x

    ", id="tags-in-a-comment"), + pytest.param( + "
    x
    ", id="tags-in-js" + ), + pytest.param("
    " * 30 + "x", id="tables"), + pytest.param("
    " * 100 + "x", id="blockquotes"), + pytest.param("

    a

    " * 100, id="siblings"), + ], +) +def test_both_paths_agree_about_what_is_markup(html: str) -> None: + """The two renderings of one body must not disagree about its markup. + + This corpus was built against a flat scan that predicted the nesting + depth, and it caught the scan reading markup differently from the + parser -- a stray close popping an element the parser keeps, a void + element counted as a parent, a tag inside a comment or a script body + counted at all. The prediction is gone; the corpus is not, because + the same disagreements would now show up as the fallback deleting or + inventing text relative to the converting path. + """ + + assert_the_degraded_path_says_no_less(html) + + +@pytest.mark.parametrize( + "html", + [ + # A comment with no ``-->`` is a *bogus comment*: the parser gives up + # at the first ``>``, so the ```` inside it is text and the + # ``
    `` lands inside ````. Read that ```` as a + # real close and the count comes back one level short -- which is + # how a document that needed degrading reached the converter. + pytest.param("", id="pi-overlapping-a-comment"), + pytest.param("
    ", id="terminated-comment"), + pytest.param("

    x

    ", id="doctype"), + pytest.param("
    b]]>

    x

    ", id="marked-section"), + pytest.param("

    x

    ", id="raw-text"), + pytest.param("