From cfa3bba59f750957cbfdd46d027517e0c6105dd4 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Sat, 18 Jul 2026 15:36:10 +0200 Subject: [PATCH] FEATURE: Add an AST normalization pass for target-format nesting rules This moves nesting-rule enforcement out of the migrations-tooling custom Url tag and into Markbridge. The default only fixes target-format legality and stays a general converter; the Discourse-specific policy, like moving an image out of a link, stays on the migrations side, but shrinks from a whole custom tag to a single rule. Real markup nests elements in ways Markdown cannot express: a link inside a link, an image inside a link, a block element inside a link label or inside bold. The renderer printed whatever the tree held, so these came out broken. The inner link wins and the outer one turns into plain text; a block's blank lines cut the [..](url) construct in half. migrations-tooling worked around the link cases in its own Url tag, but that is knowledge in one consumer that every other consumer would have to rediscover, and two copies of "what may nest in what" that drift. Markbridge::Normalizer walks the AST once, between the parse-time yield hook and rendering, and rewrites it so the renderer only gets markup the target format can express. The tags stay simple string emitters; the nesting rules live one level up, in one place. The pass runs for all four source formats and for Markbridge.render, so a consumer's own AST changes made in the yield hook get normalized too. The default rules are target-format legality only: no link inside a link (CommonMark forbids it at any depth), no block element inside an inline container (its blank lines would break the container), and a code span that stays inline only while it is on one line. Discourse policy, like moving an image out of a link, is not built in. A consumer adds it with one #rule call, which is what migrations-tooling does. Each match resolves to a strategy: :keep, :hoist_after, :unwrap, :textify, :drop, or a callable that decides at apply time. Everything the pass changes is reported through diagnostics[:normalization], next to unknown_tags. #violations lists what would change without touching the tree. Hoisting is the tricky part. There are no parent pointers, so the walker keeps the ancestor stack, takes the offending node out, and puts it back as a sibling right after its outermost offending ancestor. The moved subtree is walked against the stack it will have after the move, so a node that leaves a link never sees the link while its own inside is normalized: a legal quote inside a quote stays nested, and an image inside a quote inside a link stays in the quote. A wrapper left empty by a hoist is removed, so there are no empty ** ** markers, except an empty Url, which still renders as a bare link. The rules are built once. Normalizer.shared_default is a frozen instance reused across conversions; Normalizer.default returns a fresh one to customize with #rule. #normalize and #violations keep no state on the instance, so the shared frozen instance is safe to reuse, also across threads. It runs by default. normalize: false skips it, normalize: swaps in a customized one. I benchmarked the violation-free path at about 2 microseconds per post, which does not show against render time, so I left it on by default. --- UPGRADING.md | 52 ++ bench/corpus_bench.rb | 51 ++ docs/architecture.md | 6 + docs/normalization.md | 123 ++++ docs/renderers/discourse.md | 9 + lib/markbridge.rb | 72 +- lib/markbridge/ast/element.rb | 28 + lib/markbridge/normalizer.rb | 180 +++++ lib/markbridge/normalizer/report.rb | 41 ++ lib/markbridge/normalizer/rule_set.rb | 89 +++ lib/markbridge/normalizer/text_projection.rb | 37 + lib/markbridge/normalizer/walker.rb | 250 +++++++ .../renderers/discourse/tags/event_tag.rb | 6 +- .../renderers/discourse/tags/poll_tag.rb | 6 +- mutant.yml | 15 + .../integration/markbridge/normalizer_spec.rb | 77 ++ spec/markbridge_spec.rb | 113 +++ spec/unit/markbridge/ast/element_spec.rb | 44 ++ .../unit/markbridge/normalizer/report_spec.rb | 51 ++ .../markbridge/normalizer/rule_set_spec.rb | 199 +++++ .../normalizer/text_projection_spec.rb | 48 ++ .../unit/markbridge/normalizer/walker_spec.rb | 697 ++++++++++++++++++ spec/unit/markbridge/normalizer_spec.rb | 398 ++++++++++ .../discourse/tags/event_tag_spec.rb | 45 +- .../renderers/discourse/tags/poll_tag_spec.rb | 37 +- 25 files changed, 2611 insertions(+), 63 deletions(-) create mode 100644 docs/normalization.md create mode 100644 lib/markbridge/normalizer.rb create mode 100644 lib/markbridge/normalizer/report.rb create mode 100644 lib/markbridge/normalizer/rule_set.rb create mode 100644 lib/markbridge/normalizer/text_projection.rb create mode 100644 lib/markbridge/normalizer/walker.rb create mode 100644 spec/integration/markbridge/normalizer_spec.rb create mode 100644 spec/unit/markbridge/normalizer/report_spec.rb create mode 100644 spec/unit/markbridge/normalizer/rule_set_spec.rb create mode 100644 spec/unit/markbridge/normalizer/text_projection_spec.rb create mode 100644 spec/unit/markbridge/normalizer/walker_spec.rb create mode 100644 spec/unit/markbridge/normalizer_spec.rb diff --git a/UPGRADING.md b/UPGRADING.md index f983c8cc..02f7cd8d 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,5 +1,57 @@ # Upgrading Markbridge +## Unreleased — AST normalization runs by default + +A new `Markbridge::Normalizer` pass runs between the parse-time `yield` hook +and rendering. It rewrites the AST so the renderer only gets markup the target +format can express. It is **on by default** for every `*_to_markdown` call, +`convert`, and `render`. + +What changes in the output, with no code change on your side: + +- A link inside a link (`[url][url]…[/url][/url]`) collapses to a single link. + CommonMark does not allow nested links. +- A block element inside an inline container — a quote, list, table, or a + `Poll`/`Event` node inside a link, bold, or a heading — is moved out, so the + inline element does not break. This is not link-specific. +- A fenced or multi-line code block inside an inline container is moved out; a + one-line code span stays. +- A formatting wrapper left empty by the above is removed (no empty `**` `**`). + An empty link is kept, because it renders as a plain URL. + +Each change is reported under `conversion.diagnostics[:normalization]`, next +to `unknown_tags`. + +The default rules are legality only. Discourse policy is not built in. A +linked image (`[![alt](src)](url)`) is valid CommonMark, so the default leaves +it alone. To move image-likes out of links, add the rules yourself. This is +the pattern that replaces hoisting logic a consumer used to have in a custom +`Url` tag: + +```ruby +NORMALIZER = + Markbridge::Normalizer + .default + .rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :hoist_after) + .rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Upload, strategy: :hoist_after) + .rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Attachment, strategy: :hoist_after) + .freeze + +Markbridge.convert(input, format: :bbcode, normalize: NORMALIZER) +``` + +Build the normalizer once (a constant) and reuse it. `#normalize` keeps no +state on the instance, so one frozen instance is safe for every conversion, +also across threads, and passing it is as fast as the default path. + +To skip normalization: + +```ruby +Markbridge.convert(input, format: :bbcode, normalize: false) +``` + +See [docs/normalization.md](docs/normalization.md). + ## 0.3.0 — quote attribution fields and URL rendering ### `AST::Quote` attribution fields renamed and typed diff --git a/bench/corpus_bench.rb b/bench/corpus_bench.rb index b1b440db..ebec8ccf 100644 --- a/bench/corpus_bench.rb +++ b/bench/corpus_bench.rb @@ -43,9 +43,22 @@ def report(name, corpus, &work) ) end +# A violation-heavy variant of a corpus: prepends a few degenerate +# constructs (linked image, nested links, block-in-link) to each post so +# the normalizer's worst case — including the destination-stack rewalk on +# moved subtrees — is exercised on realistic surrounding text. +def build_heavy(corpus) + violating = + "[url=https://ex.com/a][img]https://ex.com/i.png[/img][/url] " \ + "[url=https://a.com][url=https://b.com]x[/url][/url] " \ + "[url=https://ex.com][quote]q[/quote][/url] " + corpus.map { |post| violating + post } +end + # ASCII/multibyte corpus pair per source format. CORPORA = { bbcode: -> { [Corpus.ascii, Corpus.multibyte] }, + bbcode_heavy: -> { [build_heavy(Corpus.ascii), build_heavy(Corpus.multibyte)] }, mediawiki: -> { [Corpus.mediawiki, Corpus.mediawiki_multibyte] }, html: -> { [Corpus.html, Corpus.html_multibyte] }, text_formatter: -> { [Corpus.text_formatter, Corpus.text_formatter_multibyte] }, @@ -98,6 +111,44 @@ def report(name, corpus, &work) end end, ], + # The default-on gate: the normalization walk over violation-free trees + # in isolation (pre-parsed ASTs; the clean corpus never mutates, so every + # round measures the same zero-violation traversal). Compare against zero. + "norm_only" => [ + :bbcode, + lambda do |corpus, tag| + parser = Markbridge::Parsers::BBCode::Parser.new + asts = corpus.map { |post| parser.parse(post) } + normalizer = Markbridge::Normalizer.shared_for(:discourse) + report("norm_only/#{tag}", asts) { |ast| normalizer.normalize(ast) } + end, + ], + # End-to-end baseline with normalization off; `fresh` minus this is the + # zero-violation overhead of default-on normalization. + "fresh_no_norm" => [ + :bbcode, + lambda do |corpus, tag| + report("fresh_no_norm/#{tag}", corpus) do |post| + Markbridge.bbcode_to_markdown(post, normalize: false) + end + end, + ], + # Worst-case bound: end-to-end over a violation-heavy corpus, with and + # without normalization (the difference is the mutation + rewalk cost). + "norm_heavy" => [ + :bbcode_heavy, + lambda do |corpus, tag| + report("norm_heavy/#{tag}", corpus) { |post| Markbridge.bbcode_to_markdown(post) } + end, + ], + "norm_heavy_off" => [ + :bbcode_heavy, + lambda do |corpus, tag| + report("norm_heavy_off/#{tag}", corpus) do |post| + Markbridge.bbcode_to_markdown(post, normalize: false) + end + end, + ], "mw_fresh" => [ :mediawiki, lambda do |corpus, tag| diff --git a/docs/architecture.md b/docs/architecture.md index 5922c27b..f1c60300 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -26,6 +26,12 @@ Markbridge follows a **Parse → AST → Render** architecture: Registry Library ``` +Between the AST and Render, an optional `Markbridge::Normalizer` pass +rewrites the tree so the renderer is only handed markup the target format +can express (no link inside a link, no block inside an inline container, +etc.). It runs by default at the conversion level; the three components +above are unchanged by it. See [AST Normalization](normalization.md). + ### Phase 1: Parsing (BBCode → AST) **Purpose:** Convert BBCode text into a structured tree diff --git a/docs/normalization.md b/docs/normalization.md new file mode 100644 index 00000000..a92cf5e5 --- /dev/null +++ b/docs/normalization.md @@ -0,0 +1,123 @@ +# AST Normalization + +Markup can nest elements in ways Markdown cannot express: a link inside a +link, a block element inside an inline container (a link label, but also bold +or a heading), a fenced code block inside emphasis. If the renderer prints +these as-is, the Markdown breaks — the inner link wins and the outer one turns +into text, a block's blank lines break out of the emphasis around it. + +`Markbridge::Normalizer` walks the AST once, between the parse-time `yield` +hook and rendering, and rewrites it so the renderer only gets markup the +target format can express. It runs **by default**. The renderer's tags stay +simple string emitters; the rules about what may nest in what live here +instead. + +## Where it runs + +``` +parse → yield(ast) → normalize → render +``` + +Because it runs after the `yield` hook, changes you make to the AST in that +block are normalized too. It runs for every source format and for +`Markbridge.render`, because normalization is about the *target* format, not +the source. + +## The default rules + +The default rules are CommonMark legality. Break one and the Markdown does +not parse back as the tree meant: + +- No link inside a link, at any depth (§6.3). The inner link is unwrapped. +- An inline container holds inline content only, so a block element inside one + is moved out. This is not link-specific: emphasis (`Bold`, `Italic`, …) and + headings are inline containers too, so a poll inside bold or a list inside a + heading is handled the same way as a block inside a link. The lists are + `Normalizer::INLINE_CONTAINERS` and `Normalizer::BLOCK_NODES` (which covers + `List`, `Table`, `Quote`, `Details`, `HorizontalRule`, `Align`, and the + Discourse `Poll`/`Event` nodes). +- A code span inside an inline container is fine while it stays on one line. A + fenced or multi-line block is moved out. + +Discourse-specific policy is **not** built in. Moving an image out of a link, +for example, is a rule you add yourself (see below). A linked image +(`[![alt](src)](url)`) is valid CommonMark, so the default leaves it alone. + +## Strategies + +Each match resolves to one strategy: + +| Strategy | Effect | +|----------|--------| +| `:keep` | Allow it. This records a decision and keeps it out of the report. | +| `:hoist_after` | Move the node out and put it right after the outermost matching ancestor, keeping the document order. An image in a bold that sits in a link is moved after the whole link (out of both), because the bold is inside the link. The walker only moves a node out to a sibling; it never puts one into a wrapper it was not already in. | +| `:unwrap` | Remove the element and put its children in its place. The built-in case is a link inside a link: `[[text](inner)](outer)` becomes `[text](outer)`. The inner link and its href are dropped; its text stays under the outer link. | +| `:textify` | Replace the subtree with its plain text (`@name` for a mention, the joined text otherwise). | +| `:drop` | Remove it. | +| callable | `->(boundary, node) { … }` that returns a strategy symbol, an `Array` to put in its place, or `nil` to drop it. Use this for anything the built-in strategies do not cover. | + +A formatting wrapper (bold, italic, color, …) that ends up empty after a +hoist or drop is removed, so no empty `**` `**` markers are left. A link is +the exception: an empty link is kept, because it renders as a plain URL. + +## Diagnostics + +Every change is reported through the same channel as `unknown_tags`: + +```ruby +conversion = Markbridge.convert(input, format: :bbcode) +conversion.diagnostics[:normalization] +# => [{ parent: "Url", child: "Url", strategy: :unwrap, count: 1 }] +``` + +For a migration this feeds per-post warnings and shows which sources produce +broken trees. The key is absent when nothing changed. + +## Opting out and customizing + +`normalize:` takes `true` (default, the shared normalizer), `false` (skip), or +a `Normalizer` instance: + +```ruby +# Skip normalization +Markbridge.convert(input, format: :bbcode, normalize: false) + +# Add your own rules on top of the defaults +normalizer = Markbridge::Normalizer.default +normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :hoist_after) +Markbridge.convert(input, format: :bbcode, normalize: normalizer) +``` + +Build a customized normalizer once and reuse it. `#normalize` and +`#violations` keep no state on the instance, so one instance (freeze it if you +like) is safe to use for every conversion, also across threads. Passing your +own instance is as fast as the default path — there is no per-call rule +build. + +A rule for a `(parent, child)` pair that already exists is replaced, so your +`#rule` calls override the defaults. Matching is by exact class +(`instance_of?`), so a rule for `AST::Url` does not catch a subclass. + +`Markbridge::Normalizer.shared_default` is the default normalizer, built once +and frozen; the `normalize: true` path uses it. Do not change it — call +`.default` for a fresh, customizable one. + +## Validation + +The same rules, without changing the tree: + +```ruby +Markbridge::Normalizer.default.violations(ast) +# => [{ parent: "Url", child: "Url", strategy: :unwrap }] +``` + +Two uses: check in your own test suite that the trees your parsers and tag +fixtures build have no violations, or run it as a lint over a corpus without +changing any output. After a `normalize`, `violations` returns nothing — +normalization is done in a single pass. + +## Adding a target format + +`Normalizer.default` builds the rule table; the engine (`RuleSet`, `Walker`) +does not care about the format. A second target would add another builder and +a matching class method next to `default`; nothing else changes. diff --git a/docs/renderers/discourse.md b/docs/renderers/discourse.md index fecc903c..9b2a5b7a 100644 --- a/docs/renderers/discourse.md +++ b/docs/renderers/discourse.md @@ -958,8 +958,17 @@ doc = AST::Document.new([ renderer.render(doc) # => "content" (Unknown wrapper ignored) ``` +## AST normalization + +The renderer's tags are simple string emitters. They assume the tree is +already legal for the target format (no link inside a link, no block element +inside an inline container like bold or a heading). Making it legal is the job +of a separate pass, `Markbridge::Normalizer`, which runs by default between +parse and render. See **[AST Normalization](../normalization.md)**. + ## Next Steps +- **[AST Normalization](../normalization.md)** - Target-format nesting rules applied before rendering - **[BBCode Parser Guide](../parsers/bbcode.md)** - Learn how to build AST from BBCode - **[Extending Markbridge](../extending.md)** - Add custom tags and renderers - **[Architecture Overview](../architecture.md)** - Understand the full pipeline diff --git a/lib/markbridge.rb b/lib/markbridge.rb index df1753c9..6b565088 100644 --- a/lib/markbridge.rb +++ b/lib/markbridge.rb @@ -5,6 +5,7 @@ require_relative "markbridge/conversion" require_relative "markbridge/ast" +require_relative "markbridge/normalizer" require_relative "markbridge/renderers/discourse" module Markbridge @@ -44,11 +45,21 @@ def parse_bbcode(input, handlers: nil) # {Conversion} with an empty +markdown+ string, and surface the # exceptions via {Conversion#errors}. # @yieldparam ast [AST::Document] mutate before rendering (optional) + # @param normalize [Boolean, Normalizer] apply target-format nesting + # rules between the +yield+ hook and render. +true+ (default) uses the + # shared default normalizer; a {Normalizer} is used as-is; +false+ + # skips normalization. See {Normalizer}. # @return [Conversion] - def bbcode_to_markdown(input, handlers: nil, renderer: nil, raise_on_error: true) + def bbcode_to_markdown( + input, + handlers: nil, + renderer: nil, + raise_on_error: true, + normalize: true + ) parse = parse_bbcode(input, handlers:) yield(parse.ast) if block_given? - build_conversion(parse, renderer:, raise_on_error:) + build_conversion(parse, renderer:, raise_on_error:, normalize:) end # Parse HTML to AST. @@ -85,11 +96,12 @@ def parse_html(input, handlers: nil) # {Conversion} with an empty +markdown+ string, and surface the # exceptions via {Conversion#errors}. # @yieldparam ast [AST::Document] mutate before rendering (optional) + # @param normalize [Boolean, Normalizer] see {.bbcode_to_markdown} # @return [Conversion] - def html_to_markdown(input, handlers: nil, renderer: nil, raise_on_error: true) + def html_to_markdown(input, handlers: nil, renderer: nil, raise_on_error: true, normalize: true) parse = parse_html(input, handlers:) yield(parse.ast) if block_given? - build_conversion(parse, renderer:, raise_on_error:) + build_conversion(parse, renderer:, raise_on_error:, normalize:) end # Parse s9e/TextFormatter XML to AST. @@ -122,11 +134,18 @@ def parse_text_formatter_xml(input, handlers: nil) # @param renderer [Renderers::Discourse::Renderer, nil] custom renderer # @param raise_on_error [Boolean] see {.bbcode_to_markdown} # @yieldparam ast [AST::Document] mutate before rendering (optional) + # @param normalize [Boolean, Normalizer] see {.bbcode_to_markdown} # @return [Conversion] - def text_formatter_xml_to_markdown(input, handlers: nil, renderer: nil, raise_on_error: true) + def text_formatter_xml_to_markdown( + input, + handlers: nil, + renderer: nil, + raise_on_error: true, + normalize: true + ) parse = parse_text_formatter_xml(input, handlers:) yield(parse.ast) if block_given? - build_conversion(parse, renderer:, raise_on_error:) + build_conversion(parse, renderer:, raise_on_error:, normalize:) end # Parse MediaWiki wikitext to AST. @@ -155,11 +174,18 @@ def parse_mediawiki(input, handlers: nil) # @param renderer [Renderers::Discourse::Renderer, nil] custom renderer # @param raise_on_error [Boolean] see {.bbcode_to_markdown} # @yieldparam ast [AST::Document] mutate before rendering (optional) + # @param normalize [Boolean, Normalizer] see {.bbcode_to_markdown} # @return [Conversion] - def mediawiki_to_markdown(input, handlers: nil, renderer: nil, raise_on_error: true) + def mediawiki_to_markdown( + input, + handlers: nil, + renderer: nil, + raise_on_error: true, + normalize: true + ) parse = parse_mediawiki(input, handlers:) yield(parse.ast) if block_given? - build_conversion(parse, renderer:, raise_on_error:) + build_conversion(parse, renderer:, raise_on_error:, normalize:) end # Convert input in the given format. Thin dispatcher over the @@ -211,8 +237,17 @@ def convert(input, format:, **kwargs, &block) # @param format [Symbol] :discourse (only renderer currently shipped) # @param renderer [Renderers::Discourse::Renderer, nil] # @param raise_on_error [Boolean] + # @param normalize [Boolean, Normalizer] see {.bbcode_to_markdown}. + # Normalization is idempotent, so re-rendering an already-normalized + # {Parse} is a no-op. # @return [Conversion] - def render(parse_or_ast, format: :discourse, renderer: nil, raise_on_error: true) + def render( + parse_or_ast, + format: :discourse, + renderer: nil, + raise_on_error: true, + normalize: true + ) raise ArgumentError, "unknown render format #{format.inspect}" unless format == :discourse parse = @@ -228,7 +263,7 @@ def render(parse_or_ast, format: :discourse, renderer: nil, raise_on_error: true raise ArgumentError, "expected Parse or AST::Node, got #{parse_or_ast.class}" end - build_conversion(parse, renderer:, raise_on_error:) + build_conversion(parse, renderer:, raise_on_error:, normalize:) end # Build a configured Discourse {Renderers::Discourse::Renderer} @@ -297,13 +332,28 @@ def bbcode_diagnostics(parser) } end - def build_conversion(parse, renderer:, raise_on_error:) + def build_conversion(parse, renderer:, raise_on_error:, normalize:) + parse = apply_normalization(parse, normalize) renderer ||= Renderers::Discourse::Renderer.new markdown, errors = render_through(renderer, parse.ast, raise_on_error:) Conversion.new(parsed: parse, markdown:, errors:) end + # Normalize +parse.ast+ in place (target-format nesting rules) and fold + # any report into +diagnostics[:normalization]+. Returns the +parse+ to + # render — a new one carrying the report when something changed, else + # the original unchanged. + def apply_normalization(parse, normalize) + return parse unless normalize + + normalizer = normalize.is_a?(Normalizer) ? normalize : Normalizer.shared_default + report = normalizer.normalize(parse.ast) + return parse if report.empty? + + parse.with(diagnostics: parse.diagnostics.merge(normalization: report)) + end + def build_escaper(escape:, escape_hard_line_breaks:, allow:) if escape == false if escape_hard_line_breaks || allow diff --git a/lib/markbridge/ast/element.rb b/lib/markbridge/ast/element.rb index 99fe7934..73479a84 100644 --- a/lib/markbridge/ast/element.rb +++ b/lib/markbridge/ast/element.rb @@ -105,6 +105,34 @@ def replace_child(old_child, new_child) @children[index] = new_child self end + + # Replace this element's entire child list in one shot. + # + # A plain validated setter for tree-rewriting passes (e.g. the + # {Markbridge::Normalizer}) that rebuild an element's children out + # of band and need to commit the result without re-running the + # per-append logic of {#<<}. Every entry must be a {Node}. + # + # NOTE: unlike {#<<}, this does *not* merge adjacent {Text} nodes — + # the auto-merge invariant is a property of {#<<} only. Callers that + # build the array themselves own that coalescing (the Normalizer's + # walker merges adjacent text as it assembles the list, so it never + # hands a state {#<<} would not have produced). + # + # @param new_children [Array] the replacement children + # @return [Element] +self+ + # @raise [TypeError] when any entry is not a {Node} + def replace_children(new_children) + new_children.each do |child| + next if child.is_a?(Node) + + actual = child.nil? ? "nil" : child.class + raise TypeError, "replace_children on #{self.class} expected #{Node}s, got #{actual}" + end + + @children = new_children + self + end end end end diff --git a/lib/markbridge/normalizer.rb b/lib/markbridge/normalizer.rb new file mode 100644 index 00000000..3b3e66b1 --- /dev/null +++ b/lib/markbridge/normalizer.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +require_relative "normalizer/rule_set" +require_relative "normalizer/report" +require_relative "normalizer/text_projection" +require_relative "normalizer/walker" + +module Markbridge + # Rewrites an AST so the renderer only gets markup the target format can + # express. It runs once, between parse and render. The default rules are + # CommonMark legality: no link inside a link, no block element inside an + # inline container, and an inline-only code span. Each match resolves to a + # strategy (+:keep+, +:hoist_after+, +:unwrap+, +:textify+, +:drop+, or a + # callable) that the {Walker} applies. + # + # Discourse-specific policy (for example, moving an image out of a link) is + # not built in. A consumer adds those with {#rule}. + # + # @example The default, reused across conversions + # Markbridge::Normalizer.shared_default + # + # @example A customized normalizer + # n = Markbridge::Normalizer.default + # n.rule(parent: AST::Url, child: AST::Image, strategy: :hoist_after) + # Markbridge.convert(input, format: :bbcode, normalize: n) + # + # @example List what would change, without changing it + # Markbridge::Normalizer.default.violations(ast) # => [...] + class Normalizer + # The strategy symbols a rule may resolve to. + STRATEGIES = %i[keep hoist_after unwrap textify drop].freeze + + EMPTY_STACK = [].freeze + private_constant :EMPTY_STACK + + # Containers that hold inline content only: a link's text (CommonMark + # §6.3), and emphasis and heading content. + INLINE_CONTAINERS = [ + AST::Url, + AST::Bold, + AST::Italic, + AST::Strikethrough, + AST::Underline, + AST::Superscript, + AST::Subscript, + AST::Heading, + ].freeze + + # AST nodes the Discourse renderer prints as block-level Markdown (their + # output has blank lines around it). One inside an inline container breaks + # that container, so it is moved out. Spoiler and single-line Code stay + # inline and are not listed (Code is handled by {KEEP_INLINE_CODE}). + BLOCK_NODES = [ + AST::Quote, + AST::Heading, + AST::List, + AST::ListItem, + AST::Table, + AST::TableRow, + AST::TableCell, + AST::Details, + AST::Paragraph, + AST::HorizontalRule, + AST::Align, + AST::Poll, + AST::Event, + ].freeze + + # A code span may stay inside an inline container while it is on one line. + # A fenced or multi-line block is moved out. This matches + # +RenderingInterface#block_context?+: Code prints as a fenced block when a + # Text child has a newline (the language alone does not make it a block). + KEEP_INLINE_CODE = + lambda do |_boundary, node| + block = node.children.any? { |c| c.instance_of?(AST::Text) && c.text.include?("\n") } + block ? :hoist_after : :keep + end + + class << self + # A fresh, customizable normalizer with the default rules. Add more with + # {#rule}. + # @return [Normalizer] + def default + new(build_rules) + end + + # The default normalizer, built once and frozen, reused across + # conversions. +#normalize+ and +#violations+ keep no state on the + # instance, so one frozen instance is safe to reuse, also across threads. + # @return [Normalizer] the same frozen instance on every call + def shared_default + @shared_default ||= default.freeze + end + + private + + def build_rules + rules = RuleSet.new + + # §6.3 A link may not contain another link, at any depth. Unwrap the + # inner link and keep its text. + rules.add(parent: AST::Url, child: AST::Url, strategy: :unwrap) + + INLINE_CONTAINERS.each do |container| + rules.add(parent: container, child: AST::Code, strategy: KEEP_INLINE_CODE) + + BLOCK_NODES.each do |block| + next if container == block + + rules.add(parent: container, child: block, strategy: :hoist_after) + end + end + + rules + end + end + + # @param rule_set [RuleSet] + def initialize(rule_set) + @rules = rule_set + end + + # Add or override a rule. Chainable. A rule for a +(parent, child)+ pair + # that already exists is replaced. Raises on a frozen ({shared_default}) + # instance; build a fresh one with {.default}. + # + # @param parent [Class] ancestor AST class + # @param child [Class] contained AST class + # @param strategy [Symbol, #call] one of {STRATEGIES} or a callable + # @return [self] + def rule(parent:, child:, strategy:) + @rules.add(parent:, child:, strategy:) + self + end + + # Rewrite +ast+ in place so it satisfies the rules. + # + # @param ast [AST::Document, AST::Element] + # @return [Array] a report of what changed (empty when nothing did), + # one +{parent:, child:, strategy:, count:}+ row per distinct change. + def normalize(ast) + report = Report.new + Walker.new(@rules, report).call(ast) + report.to_a + end + + # List the violations in +ast+ without changing it. + # + # @param ast [AST::Document, AST::Element] + # @return [Array] +{parent:, child:, strategy:}+ per occurrence + def violations(ast) + found = [] + collect_violations(ast, EMPTY_STACK, found) + found + end + + def freeze + @rules.freeze + super + end + + private + + def collect_violations(element, ancestors, found) + stack = ancestors + [element] + element.children.each do |child| + strategy, boundary = @rules.resolve(child, stack) + strategy = strategy.call(boundary, child) if strategy.respond_to?(:call) + unless strategy.nil? || strategy == :keep + found << { parent: demodulize(boundary.class), child: demodulize(child.class), strategy: } + end + collect_violations(child, stack, found) if child.is_a?(AST::Element) + end + end + + def demodulize(klass) + klass.name.split("::").last + end + end +end diff --git a/lib/markbridge/normalizer/report.rb b/lib/markbridge/normalizer/report.rb new file mode 100644 index 00000000..4f34e9a7 --- /dev/null +++ b/lib/markbridge/normalizer/report.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Markbridge + class Normalizer + # Tally of the transformations a single {Normalizer#normalize} pass + # applied. Held as a local per call (never on the Normalizer), so a + # frozen shared instance stays reusable and thread-safe. + class Report + def initialize + @counts = Hash.new(0) + end + + # @param parent_class [Class] the offending ancestor's class + # @param child_class [Class] the moved/removed node's class + # @param strategy [Symbol] the strategy actually applied + def record(parent_class, child_class, strategy) + @counts[[demodulize(parent_class), demodulize(child_class), strategy]] += 1 + end + + # @return [Boolean] + def empty? + @counts.empty? + end + + # One +{parent:, child:, strategy:, count:}+ row per distinct + # transformation, e.g. + # +{parent: "Url", child: "Image", strategy: :hoist_after, count: 3}+. + # + # @return [Array] + def to_a + @counts.map { |(parent, child, strategy), count| { parent:, child:, strategy:, count: } } + end + + private + + def demodulize(klass) + klass.name.split("::").last + end + end + end +end diff --git a/lib/markbridge/normalizer/rule_set.rb b/lib/markbridge/normalizer/rule_set.rb new file mode 100644 index 00000000..c18222e6 --- /dev/null +++ b/lib/markbridge/normalizer/rule_set.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module Markbridge + class Normalizer + # Maps a node and its ancestor stack to a strategy. + # + # Matching is exact-class (equivalent to +instance_of?+): rules are + # keyed by +Class+ and looked up via +node.class+, so an anonymous + # +Class.new(AST::Element)+ or any future subclass never accidentally + # matches a rule written for the base class. Registering a rule for a + # +(parent, child)+ pair that already has one replaces it, so later + # layers (Discourse, a consumer's +#rule+) override earlier ones. + class RuleSet + NO_MATCH = [nil, nil].freeze + + def initialize + @by_parent = {} # parent_class => { child_class => strategy } + @child_classes = Set.new + end + + # Register (or replace) a rule. + # + # @param parent [Class] ancestor AST class + # @param child [Class] contained AST class + # @param strategy [Symbol, #call] a strategy symbol or callable + # @return [self] + def add(parent:, child:, strategy:) + validate_strategy!(strategy) + (@by_parent[parent] ||= {})[child] = strategy + @child_classes << child + self + end + + # Resolve the strategy for +child+ given its ancestor stack (root + # first). Returns +[strategy, boundary]+ where +boundary+ is the + # *outermost* ancestor whose class has a rule for +child+'s class, or + # {NO_MATCH} (+[nil, nil]+) when nothing matches. + # + # @param child [AST::Node] + # @param ancestors [Array] root-first ancestor stack + # @return [Array(Object, AST::Element), Array(nil, nil)] + def resolve(child, ancestors) + child_class = child.class + # Skip the ancestor scan for a class no rule targets (most nodes, for + # example plain text). The scan below returns the same result for such + # a class, so this only saves work. + return NO_MATCH unless @child_classes.include?(child_class) + + scan_ancestors(child_class, ancestors) + end + + # Freeze so a shared instance raises if something tries to change it. + # Freezing +@by_parent+ and its inner hashes is enough: {#add} writes + # there before it touches +@child_classes+, so a frozen instance raises + # on the +@by_parent+ write first. +@child_classes+ is never reached, so + # it does not need freezing. + def freeze + @by_parent.each_value(&:freeze) + @by_parent.freeze + super + end + + private + + # The ancestor scan behind {#resolve}: the outermost matching ancestor + # wins. It is split from the skip check in {#resolve} so the scan can be + # tested on its own. + def scan_ancestors(child_class, ancestors) + ancestors.each do |ancestor| + strategies = @by_parent[ancestor.class] + next unless strategies + + strategy = strategies[child_class] + return strategy, ancestor if strategy + end + NO_MATCH + end + + def validate_strategy!(strategy) + return if strategy.respond_to?(:call) + return if STRATEGIES.include?(strategy) + + raise ArgumentError, + "unknown strategy #{strategy.inspect} " \ + "(expected one of #{STRATEGIES.inspect} or a callable)" + end + end + end +end diff --git a/lib/markbridge/normalizer/text_projection.rb b/lib/markbridge/normalizer/text_projection.rb new file mode 100644 index 00000000..7d6552c8 --- /dev/null +++ b/lib/markbridge/normalizer/text_projection.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Markbridge + class Normalizer + # Best-effort plain-text projection of a subtree, used by the + # +:textify+ strategy. Concatenates text content; renders a {AST::Mention} + # as its literal +@name+, and opaque leaf nodes as their alt/raw text + # when they carry one, otherwise the empty string. + module TextProjection + class << self + # @param node [AST::Node] + # @return [String] + def call(node) + case node + when AST::Text, AST::MarkdownText + node.text + when AST::Mention + "@#{node.name}" + when AST::Element + node.children.map { |child| call(child) }.join + else + leaf_text(node) + end + end + + # @param node [AST::Node] an opaque leaf (Upload, Attachment, …) + # @return [String] + def leaf_text(node) + return node.alt if node.respond_to?(:alt) && node.alt + return node.raw if node.respond_to?(:raw) && node.raw + + "" + end + end + end + end +end diff --git a/lib/markbridge/normalizer/walker.rb b/lib/markbridge/normalizer/walker.rb new file mode 100644 index 00000000..fb993652 --- /dev/null +++ b/lib/markbridge/normalizer/walker.rb @@ -0,0 +1,250 @@ +# frozen_string_literal: true + +module Markbridge + class Normalizer + # The parent-aware tree-rewriting engine. One {Walker} is built per + # {Normalizer#normalize} call (holding that call's {RuleSet} and + # {Report}; no state is kept on the Normalizer). It changes the tree + # in place via {AST::Element#replace_children}, touching only elements + # whose children actually changed. + # + # The main rule is resolve-before-descend: a child's strategy is resolved + # against its current ancestor stack first, and a moved subtree (hoist or + # unwrap) is then walked against the ancestor stack it will have in its + # new place. So a node that leaves a link does not see the link while its + # own inside is normalized. That keeps a legally nested quote-in-quote or + # image-in-quote intact, and it means a second normalize reports nothing. + # + # Hoisting: a node taken out of an inline container is moved up, tagged + # with its boundary (the outermost matching ancestor, compared by + # identity), and placed right after that boundary, at the boundary's + # parent. A wrapper left empty by a hoist or drop is removed (see + # {PRUNE_WHEN_EMPTY}), so no empty +**+ +**+ markers stay. + # + # For speed, the ancestor stack is one shared array that is pushed and + # popped, and an element's children list is copied only when a child + # first changes (copy-on-write). A subtree with no violations allocates + # nothing and is left as it was, so the pass can run by default. + class Walker + EMPTY = [].freeze + + # Wrappers that mean nothing once they are empty, so an empty one is + # removed instead of kept. +AST::Url+ is not here on purpose: an empty + # link still renders as a bare URL, so a hoist that empties a link keeps + # the link. + PRUNE_WHEN_EMPTY = [ + AST::Bold, + AST::Italic, + AST::Underline, + AST::Strikethrough, + AST::Superscript, + AST::Subscript, + AST::Color, + AST::Size, + AST::Align, + AST::Email, + ].freeze + + # @param rule_set [RuleSet] + # @param report [Report] + def initialize(rule_set, report) + @rules = rule_set + @report = report + end + + # Normalize +document+'s subtree in place. + # @param document [AST::Document] + def call(document) + _element, bubble = normalize_element(document, []) + # Defensive: anything that never reached its boundary becomes a + # trailing sibling rather than being lost. Well-formed rule tables + # do not get here. + bubble.each { |node, _boundary| document << node } + end + + private + + # Copy-on-write check: a kept child that came back as the same object + # (so normalizing it changed nothing) with no bubble, while nothing + # earlier changed, needs no rebuilt +out+. This only saves work. If it + # is wrong, the code still rebuilds the same child list, so its + # mutations do not change the output. + def unchanged?(out, normalized, child, child_bubble) + out.nil? && normalized.equal?(child) && child_bubble.empty? + end + + # Normalize a node's descendants. Elements recurse; leaves are + # returned untouched. Returns +[node_or_nil, bubble]+ — +nil+ when the + # element was pruned, and +bubble+ is the list of +[node, boundary]+ + # pairs to hoist above this node. + def normalize_node(node, stack) + return node, EMPTY unless node.is_a?(AST::Element) + + normalize_element(node, stack) + end + + # +stack+ is a shared, mutable ancestor stack (root first): +element+ + # is pushed for the duration of its children's processing and popped + # after. +out+ (the rebuilt child list) and +bubble+ stay +nil+ until + # a child actually changes, so the clean path allocates nothing. + def normalize_element(element, stack) + stack.push(element) + children = element.children + out = nil + bubble = nil + + children.each_with_index do |child, index| + strategy, boundary = @rules.resolve(child, stack) + strategy = strategy.call(boundary, child) if strategy.respond_to?(:call) + + if strategy.nil? || strategy == :keep + normalized, child_bubble = normalize_node(child, stack) + next if unchanged?(out, normalized, child, child_bubble) + + out ||= children[0, index] + bubble = append_kept(normalized, child_bubble, child, out, bubble) + else + out ||= children[0, index] + bubble = emit(child, strategy, boundary, stack, out, bubble) + end + end + + stack.pop + element.replace_children(coalesce(out)) if out + + raised = bubble || EMPTY + return nil, raised if prune?(element, out, children) + + [element, raised] + end + + # An element is removed when it ends up with no children and is one of + # the wrappers in {PRUNE_WHEN_EMPTY}. + def prune?(element, out, children) + empty = out ? out.empty? : children.empty? + empty && PRUNE_WHEN_EMPTY.include?(element.class) + end + + # Resolve one child (against +stack+) and place it into an existing + # +out+ — the shared entry point used by {#emit}'s +:unwrap+ recursion, + # where +out+ already exists. + def resolve_into(child, stack, out, bubble) + strategy, boundary = @rules.resolve(child, stack) + strategy = strategy.call(boundary, child) if strategy.respond_to?(:call) + + if strategy.nil? || strategy == :keep + child2, child_bubble = normalize_node(child, stack) + append_kept(child2, child_bubble, child, out, bubble) + else + emit(child, strategy, boundary, stack, out, bubble) + end + end + + # Append a kept child that is already normalized, then place any of its + # bubbles whose boundary is this child. ({#land} does nothing when + # +child_bubble+ is empty, so there is no separate check for that.) + def append_kept(normalized, child_bubble, child, out, bubble) + out << normalized unless normalized.nil? + land(child_bubble, child, out, bubble) + end + + # Apply a non-keep strategy for +child+, appending to +out+ and + # returning the (possibly newly allocated) +bubble+. + def emit(child, strategy, boundary, stack, out, bubble) + case strategy + when :hoist_after + hoist(child, boundary, stack, bubble) + when :unwrap + unwrap(child, boundary, stack, out, bubble) + when :textify + @report.record(boundary.class, child.class, :textify) + out << AST::Text.new(TextProjection.call(child)) + bubble + when :drop + @report.record(boundary.class, child.class, :drop) + bubble + when Array + # A callable returned replacement nodes to splice in place. + @report.record(boundary.class, child.class, :replace) + strategy.each { |node| out << node } + bubble + else + raise ArgumentError, "strategy resolved to #{strategy.inspect}" + end + end + + def hoist(child, boundary, stack, bubble) + @report.record(boundary.class, child.class, :hoist_after) + # Walk the relocated subtree against its destination stack (the + # ancestors strictly above the boundary) so its interior never sees + # the boundary it is leaving. + child2, child_bubble = normalize_node(child, ancestors_above(boundary, stack)) + bubble ||= [] + bubble << [child2, boundary] unless child2.nil? + # For the built-in tables a hoisted subtree yields no escaping + # bubbles; carry any (from a custom rule) up as a best effort. + child_bubble.each { |entry| bubble << entry } + bubble + end + + def unwrap(child, boundary, stack, out, bubble) + # Unwrap means "promote the element's children"; a leaf has none. A + # rule that targets one is a misconfiguration, so keep the node in + # place rather than silently dropping it (and don't report a no-op). + unless child.is_a?(AST::Element) + out << child + return bubble + end + + @report.record(boundary.class, child.class, :unwrap) + # Unwrap: run the child's children through the current +out+, resolved + # against the current stack. Resolving them again in the same pass is + # what fixes deeply nested links in one go. + child.children.each { |grandchild| bubble = resolve_into(grandchild, stack, out, bubble) } + bubble + end + + # Land inbound bubbles whose boundary is +child+ (this level is the + # boundary's parent), each after the previous to preserve order; + # propagate the rest upward. + def land(child_bubble, child, out, bubble) + child_bubble.each do |node, boundary| + if boundary.equal?(child) + out << node + else + bubble ||= [] + bubble << [node, boundary] + end + end + bubble + end + + # Ancestors strictly above +boundary+ in +stack+ — the stack the + # hoisted node inherits (its parent becomes the boundary's parent). + def ancestors_above(boundary, stack) + index = stack.index { |ancestor| ancestor.equal?(boundary) } + stack.first(index) + end + + # Coalesce adjacent text (textify can create neighbours that +#<<+ + # would have merged) just before committing a changed child list. + def coalesce(nodes) + nodes.each_with_object([]) do |node, acc| + last = acc.last + # mergeable? is false when +last+ is nil (the first node), so no + # separate nil-guard is needed. + if mergeable?(last, node) + last.merge(node) + else + acc << node + end + end + end + + def mergeable?(left, right) + (left.instance_of?(AST::Text) && right.instance_of?(AST::Text)) || + (left.instance_of?(AST::MarkdownText) && right.instance_of?(AST::MarkdownText)) + end + end + end +end diff --git a/lib/markbridge/renderers/discourse/tags/event_tag.rb b/lib/markbridge/renderers/discourse/tags/event_tag.rb index 070202be..2e586a07 100644 --- a/lib/markbridge/renderers/discourse/tags/event_tag.rb +++ b/lib/markbridge/renderers/discourse/tags/event_tag.rb @@ -18,11 +18,9 @@ module Tags # end # end class EventTag < Tag - def render(element, interface) + def render(element, _interface) body = element.raw || build_event_bbcode(element) - return "\n\n#{body}\n\n" if interface.html_mode? - - "#{body}\n\n" + "\n\n#{body}\n\n" end private diff --git a/lib/markbridge/renderers/discourse/tags/poll_tag.rb b/lib/markbridge/renderers/discourse/tags/poll_tag.rb index 560e1b27..fd5c3359 100644 --- a/lib/markbridge/renderers/discourse/tags/poll_tag.rb +++ b/lib/markbridge/renderers/discourse/tags/poll_tag.rb @@ -18,11 +18,9 @@ module Tags # end # end class PollTag < Tag - def render(element, interface) + def render(element, _interface) body = element.raw || build_poll_bbcode(element) - return "\n\n#{body}\n\n" if interface.html_mode? - - "#{body}\n\n" + "\n\n#{body}\n\n" end private diff --git a/mutant.yml b/mutant.yml index 89133131..eaf41335 100644 --- a/mutant.yml +++ b/mutant.yml @@ -282,6 +282,21 @@ matcher: # path runs only for the Document branch. - Markbridge::Parsers::HTML::Parser#parse + # Normalizer fast-path guards. Both methods exist ONLY to skip work; + # when a mutation defeats the guard, the fallback recomputes the same + # result, so every surviving mutation is output-equivalent (byte-for- + # byte). The real behaviour they gate is pinned elsewhere: + # RuleSet#resolve — the ancestor scan is extracted into + # #scan_ancestors, which the resolve/normalize specs cover; the + # guard only decides whether to call it, and a non-candidate class + # yields NO_MATCH from the scan anyway. + # Walker#unchanged? — the copy-on-write skip; when it wrongly returns + # false, normalize_element just rebuilds an identical child list + # (the "leaves a violation-free tree untouched" and COW specs pin + # the observable behaviour). + - Markbridge::Normalizer::RuleSet#resolve + - Markbridge::Normalizer::Walker#unchanged? + mutation: ignore_patterns: # Bucket A bound-check equivalences. @current_pos, pos, and diff --git a/spec/integration/markbridge/normalizer_spec.rb b/spec/integration/markbridge/normalizer_spec.rb new file mode 100644 index 00000000..ca967a8c --- /dev/null +++ b/spec/integration/markbridge/normalizer_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +# End-to-end: the default normalization runs between parse and render, so +# these assertions show the raw Markdown you get. The default fixes legality +# (a link inside a link, a block inside an inline container). Discourse policy +# like moving an image out of a link is a rule the consumer adds. +RSpec.describe "Markbridge normalization (end-to-end)" do + def convert(input, **kwargs) = Markbridge.convert(input, format: :bbcode, **kwargs) + + describe "default: a link inside a link" do + let(:input) { "[url=https://a.com][url=https://b.com]click[/url][/url]" } + + it "collapses to a single link and reports it" do + conv = convert(input) + + expect(conv.markdown).to eq("[click](https://a.com)") + expect(conv.diagnostics[:normalization]).to contain_exactly( + { parent: "Url", child: "Url", strategy: :unwrap, count: 1 }, + ) + end + + it "leaves the tree alone when normalize: false" do + expect(convert(input, normalize: false).diagnostics[:normalization]).to be_nil + end + end + + describe "an image inside a link" do + let(:input) { "[url=https://ex.com][img]https://ex.com/i.png[/img][/url]" } + + it "is left as a linked image by default (not a default rule)" do + expect(convert(input).markdown).to eq("[![](https://ex.com/i.png)](https://ex.com)") + expect(convert(input).diagnostics[:normalization]).to be_nil + end + + it "is hoisted out by a consumer rule (how migrations-tooling does it)" do + normalizer = Markbridge::Normalizer.default + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + + conv = convert(input, normalize: normalizer) + expect(conv.markdown).not_to include("[![") # not a linked image + expect(conv.markdown).to include("![](https://ex.com/i.png)") + expect(conv.diagnostics[:normalization]).to contain_exactly( + { parent: "Url", child: "Image", strategy: :hoist_after, count: 1 }, + ) + end + end + + describe "clean content" do + it "is untouched and reports nothing" do + conv = convert("[b]hello[/b] and [i]world[/i]") + + expect(conv.markdown).to eq("**hello** and *world*") + expect(conv.diagnostics[:normalization]).to be_nil + end + end + + describe "validation property: the default leaves no default violation" do + inputs = [ + "[url=https://a.com][url=https://b.com]x[/url][/url]", + "plain text with [b]bold[/b] and a [url=https://ex.com]link[/url]", + "[quote]a quote[/quote] then [url=https://ex.com]link[/url]", + ] + + inputs.each do |input| + it "leaves no violation for #{input.inspect}" do + ast = Markbridge.parse_bbcode(input).ast + Markbridge::Normalizer.shared_default.normalize(ast) + + expect(Markbridge::Normalizer.default.violations(ast)).to eq([]) + end + end + end +end diff --git a/spec/markbridge_spec.rb b/spec/markbridge_spec.rb index 1ee5d88e..0579978b 100644 --- a/spec/markbridge_spec.rb +++ b/spec/markbridge_spec.rb @@ -1,6 +1,27 @@ # frozen_string_literal: true RSpec.describe Markbridge do + # Append a link inside a link — a default violation (the inner link is + # unwrapped) — so a test can prove the +normalize:+ pass ran, whatever the + # source format. + def append_nested_link(ast) + outer = Markbridge::AST::Url.new(href: "https://a.example.com") + inner = Markbridge::AST::Url.new(href: "https://b.example.com") + inner << Markbridge::AST::Text.new("x") + outer << inner + ast << outer + end + + # Append a link wrapping an image. Moving it out is not a default rule, so + # only a normalizer that adds the rule changes this. + def append_linked_image(ast) + url = Markbridge::AST::Url.new(href: "https://example.com") + url << Markbridge::AST::Image.new(src: "https://example.com/pic.png") + ast << url + end + + UNWRAPPED = [{ parent: "Url", child: "Url", strategy: :unwrap, count: 1 }].freeze + it "has a version number" do expect(Markbridge::VERSION).not_to be_nil end @@ -66,6 +87,19 @@ end describe ".bbcode_to_markdown" do + it "normalizes the AST by default" do + conversion = described_class.bbcode_to_markdown("hi") { |ast| append_nested_link(ast) } + + expect(conversion.diagnostics[:normalization]).to eq(UNWRAPPED) + end + + it "does not normalize when normalize: false" do + conversion = + described_class.bbcode_to_markdown("hi", normalize: false) { |ast| append_nested_link(ast) } + + expect(conversion.diagnostics[:normalization]).to be_nil + end + it "returns a Conversion whose markdown reflects the input" do result = described_class.bbcode_to_markdown("[b]hi[/b]") @@ -205,6 +239,21 @@ def render(_e, _i) end describe ".html_to_markdown" do + it "normalizes the AST by default" do + conversion = described_class.html_to_markdown("hi") { |ast| append_nested_link(ast) } + + expect(conversion.diagnostics[:normalization]).to eq(UNWRAPPED) + end + + it "does not normalize when normalize: false" do + conversion = + described_class.html_to_markdown("hi", normalize: false) do |ast| + append_nested_link(ast) + end + + expect(conversion.diagnostics[:normalization]).to be_nil + end + it "renders HTML to a Conversion" do result = described_class.html_to_markdown("hi") @@ -304,6 +353,22 @@ def render(_e, _i) describe ".text_formatter_xml_to_markdown" do let(:xml) { "[b]hi[/b]" } + it "normalizes the AST by default" do + conversion = + described_class.text_formatter_xml_to_markdown(xml) { |ast| append_nested_link(ast) } + + expect(conversion.diagnostics[:normalization]).to eq(UNWRAPPED) + end + + it "does not normalize when normalize: false" do + conversion = + described_class.text_formatter_xml_to_markdown(xml, normalize: false) do |ast| + append_nested_link(ast) + end + + expect(conversion.diagnostics[:normalization]).to be_nil + end + it "renders TextFormatter XML to a Conversion" do result = described_class.text_formatter_xml_to_markdown(xml) @@ -393,6 +458,22 @@ def render(_e, _i) end describe ".mediawiki_to_markdown" do + it "normalizes the AST by default" do + conversion = + described_class.mediawiki_to_markdown("'''hi'''") { |ast| append_nested_link(ast) } + + expect(conversion.diagnostics[:normalization]).to eq(UNWRAPPED) + end + + it "does not normalize when normalize: false" do + conversion = + described_class.mediawiki_to_markdown("'''hi'''", normalize: false) do |ast| + append_nested_link(ast) + end + + expect(conversion.diagnostics[:normalization]).to be_nil + end + it "renders MediaWiki to a Conversion" do expect(described_class.mediawiki_to_markdown("'''hi'''").markdown).to eq("**hi**") end @@ -616,6 +697,38 @@ def render(_e, _i) end describe ".render" do + it "normalizes the AST by default" do + ast = Markbridge::AST::Document.new + append_nested_link(ast) + + expect(described_class.render(ast).diagnostics[:normalization]).to eq(UNWRAPPED) + end + + it "does not normalize when normalize: false" do + ast = Markbridge::AST::Document.new + append_nested_link(ast) + + expect(described_class.render(ast, normalize: false).diagnostics[:normalization]).to be_nil + end + + it "uses a passed Normalizer, including a subclass instance (is_a?, not instance_of?)" do + subclass = Class.new(Markbridge::Normalizer) + normalizer = subclass.default + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :drop) + ast = Markbridge::AST::Document.new + append_linked_image(ast) + + conversion = described_class.render(ast, normalize: normalizer) + + # The subclass instance is accepted and its :drop rule runs. With + # instance_of? it would be rejected, shared_default would run instead, + # and shared_default has no image rule, so the image would stay. + expect(conversion.diagnostics[:normalization]).to eq( + [{ parent: "Url", child: "Image", strategy: :drop, count: 1 }], + ) + expect(conversion.ast.descendants(Markbridge::AST::Image)).to be_empty + end + it "renders a Document AST through the default Discourse renderer" do doc = described_class.parse_bbcode("[b]hi[/b]").ast diff --git a/spec/unit/markbridge/ast/element_spec.rb b/spec/unit/markbridge/ast/element_spec.rb index a4e147a1..e4d5dd0b 100644 --- a/spec/unit/markbridge/ast/element_spec.rb +++ b/spec/unit/markbridge/ast/element_spec.rb @@ -272,4 +272,48 @@ ) end end + + describe "#replace_children" do + let(:element) { test_element_class.new } + + before { element << Markbridge::AST::Text.new("old") } + + it "swaps the entire child list" do + replacement = [Markbridge::AST::Bold.new, Markbridge::AST::Italic.new] + element.replace_children(replacement) + + expect(element.children).to eq(replacement) + end + + it "returns self for chaining" do + expect(element.replace_children([])).to be(element) + end + + it "accepts an empty list" do + element.replace_children([]) + expect(element.children).to eq([]) + end + + it "does NOT merge adjacent Text nodes (the merge invariant is #<< only)" do + first = Markbridge::AST::Text.new("hello") + second = Markbridge::AST::Text.new(" world") + element.replace_children([first, second]) + + expect(element.children).to eq([first, second]) + end + + it "raises TypeError when any entry is not a Node, naming the offending value" do + expect { element.replace_children([Markbridge::AST::Bold.new, "nope"]) }.to raise_error( + TypeError, + /replace_children on #{Regexp.escape(test_element_class.to_s)} expected Markbridge::AST::Nodes, got String/, + ) + end + + it "raises TypeError for a nil entry" do + expect { element.replace_children([nil]) }.to raise_error( + TypeError, + /replace_children on #{Regexp.escape(test_element_class.to_s)} expected Markbridge::AST::Nodes, got nil/, + ) + end + end end diff --git a/spec/unit/markbridge/normalizer/report_spec.rb b/spec/unit/markbridge/normalizer/report_spec.rb new file mode 100644 index 00000000..4137a916 --- /dev/null +++ b/spec/unit/markbridge/normalizer/report_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +RSpec.describe Markbridge::Normalizer::Report do + subject(:report) { described_class.new } + + describe "#empty?" do + it "is true for a fresh report" do + expect(report.empty?).to be(true) + end + + it "is false after a record" do + report.record(Markbridge::AST::Url, Markbridge::AST::Image, :hoist_after) + expect(report.empty?).to be(false) + end + end + + describe "#to_a" do + it "is empty for a fresh report" do + expect(report.to_a).to eq([]) + end + + it "returns one row per transformation with demodulized class names" do + report.record(Markbridge::AST::Url, Markbridge::AST::Image, :hoist_after) + expect(report.to_a).to eq( + [{ parent: "Url", child: "Image", strategy: :hoist_after, count: 1 }], + ) + end + + it "tallies repeats of the same transformation" do + 3.times { report.record(Markbridge::AST::Url, Markbridge::AST::Image, :hoist_after) } + expect(report.to_a).to eq( + [{ parent: "Url", child: "Image", strategy: :hoist_after, count: 3 }], + ) + end + + it "keeps transformations that differ in parent or child as separate rows" do + report.record(Markbridge::AST::Url, Markbridge::AST::Image, :hoist_after) + report.record(Markbridge::AST::Url, Markbridge::AST::Url, :unwrap) + expect(report.to_a).to contain_exactly( + { parent: "Url", child: "Image", strategy: :hoist_after, count: 1 }, + { parent: "Url", child: "Url", strategy: :unwrap, count: 1 }, + ) + end + + it "distinguishes rows that differ only by strategy" do + report.record(Markbridge::AST::Url, Markbridge::AST::Image, :hoist_after) + report.record(Markbridge::AST::Url, Markbridge::AST::Image, :drop) + expect(report.to_a.size).to eq(2) + end + end +end diff --git a/spec/unit/markbridge/normalizer/rule_set_spec.rb b/spec/unit/markbridge/normalizer/rule_set_spec.rb new file mode 100644 index 00000000..aa7b30fb --- /dev/null +++ b/spec/unit/markbridge/normalizer/rule_set_spec.rb @@ -0,0 +1,199 @@ +# frozen_string_literal: true + +RSpec.describe Markbridge::Normalizer::RuleSet do + subject(:rule_set) { described_class.new } + + describe "#resolve" do + it "returns [nil, nil] when the child class is not in any rule" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + bold = Markbridge::AST::Bold.new + expect(rule_set.resolve(bold, [Markbridge::AST::Url.new])).to eq([nil, nil]) + end + + it "returns [strategy, boundary] for the matching ancestor" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + url = Markbridge::AST::Url.new + image = Markbridge::AST::Image.new + + strategy, boundary = rule_set.resolve(image, [url]) + expect(strategy).to eq(:hoist_after) + expect(boundary).to be(url) + end + + it "picks the OUTERMOST matching ancestor when several match" do + rule_set.add(parent: Markbridge::AST::Url, child: Markbridge::AST::Url, strategy: :unwrap) + outer = Markbridge::AST::Url.new + inner = Markbridge::AST::Url.new + target = Markbridge::AST::Url.new + + _strategy, boundary = rule_set.resolve(target, [outer, inner]) + expect(boundary).to be(outer) + end + + it "matches by exact class, not is_a? — an anonymous subclass does not match" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + subclass_image = Class.new(Markbridge::AST::Image).new(src: "x") + + expect(rule_set.resolve(subclass_image, [Markbridge::AST::Url.new])).to eq([nil, nil]) + end + + it "does not match an anonymous subclass of the parent either" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + subclass_url = Class.new(Markbridge::AST::Url).new + + expect(rule_set.resolve(Markbridge::AST::Image.new, [subclass_url])).to eq([nil, nil]) + end + + it "skips an ancestor whose class has no rules at all" do + rule_set.add(parent: Markbridge::AST::Bold, child: Markbridge::AST::Image, strategy: :drop) + bold = Markbridge::AST::Bold.new + + # Url has no rules; Bold does — resolution must not stop at Url. + strategy, boundary = + rule_set.resolve(Markbridge::AST::Image.new, [Markbridge::AST::Url.new, bold]) + expect(strategy).to eq(:drop) + expect(boundary).to be(bold) + end + + it "skips an ancestor that has rules but none for this child class" do + rule_set.add(parent: Markbridge::AST::Url, child: Markbridge::AST::Url, strategy: :unwrap) + rule_set.add(parent: Markbridge::AST::Bold, child: Markbridge::AST::Image, strategy: :drop) + bold = Markbridge::AST::Bold.new + + # Url has a rules hash, but not one for Image — must fall through to Bold. + strategy, boundary = + rule_set.resolve(Markbridge::AST::Image.new, [Markbridge::AST::Url.new, bold]) + expect(strategy).to eq(:drop) + expect(boundary).to be(bold) + end + end + + describe "#add override" do + it "replaces an earlier rule for the same (parent, child) pair" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + rule_set.add(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :drop) + + strategy, = rule_set.resolve(Markbridge::AST::Image.new, [Markbridge::AST::Url.new]) + expect(strategy).to eq(:drop) + end + + it "returns self for chaining" do + expect( + rule_set.add(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :drop), + ).to be(rule_set) + end + + it "accepts a callable strategy" do + callable = ->(_boundary, _node) { :keep } + expect { + rule_set.add(parent: Markbridge::AST::Url, child: Markbridge::AST::Code, strategy: callable) + }.not_to raise_error + end + + it "raises for an unknown symbol strategy" do + expect { + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :explode, + ) + }.to raise_error(ArgumentError, /unknown strategy :explode/) + end + end + + describe "#freeze" do + it "freezes the receiver (via super)" do + rule_set.freeze + expect(rule_set).to be_frozen + end + + it "makes adding to an EXISTING parent raise (inner hash deep-frozen)" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + rule_set.freeze + + expect { + rule_set.add(parent: Markbridge::AST::Url, child: Markbridge::AST::Url, strategy: :unwrap) + }.to raise_error(FrozenError) + end + + it "makes adding a NEW parent raise (top-level hash frozen)" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + rule_set.freeze + + expect { + rule_set.add(parent: Markbridge::AST::Bold, child: Markbridge::AST::Image, strategy: :drop) + }.to raise_error(FrozenError) + end + + # The raise alone doesn't prove *which* collection stopped the write — + # a later-frozen collection would still raise while an earlier unfrozen + # one silently accepted the rule. These re-add an ALREADY-registered + # child class (Image) so the child-class fast-reject can't mask a sneak, + # then check the strategy did not change. + it "deep-freezes inner hashes so a raised add cannot mutate shared state" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + rule_set.freeze + + begin + rule_set.add(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :drop) + rescue FrozenError + # expected + end + + strategy, = rule_set.resolve(Markbridge::AST::Image.new, [Markbridge::AST::Url.new]) + expect(strategy).to eq(:hoist_after) # unchanged; :drop never sneaked in + end + + it "freezes the top-level hash so a raised new-parent add cannot mutate shared state" do + rule_set.add( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + rule_set.freeze + + begin + rule_set.add(parent: Markbridge::AST::Bold, child: Markbridge::AST::Image, strategy: :drop) + rescue FrozenError + # expected + end + + # No (Bold, *) parent bucket was created before the raise. + expect(rule_set.resolve(Markbridge::AST::Image.new, [Markbridge::AST::Bold.new])).to eq( + [nil, nil], + ) + end + end +end diff --git a/spec/unit/markbridge/normalizer/text_projection_spec.rb b/spec/unit/markbridge/normalizer/text_projection_spec.rb new file mode 100644 index 00000000..5a08544c --- /dev/null +++ b/spec/unit/markbridge/normalizer/text_projection_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +RSpec.describe Markbridge::Normalizer::TextProjection do + def project(node) = described_class.call(node) + + it "returns a Text node's own text" do + expect(project(Markbridge::AST::Text.new("hello"))).to eq("hello") + end + + it "returns a MarkdownText node's own text" do + expect(project(Markbridge::AST::MarkdownText.new("**hi**"))).to eq("**hi**") + end + + it "renders a Mention as its literal @name" do + expect(project(Markbridge::AST::Mention.new(name: "alice"))).to eq("@alice") + end + + it "concatenates the text of an Element's descendants" do + bold = Markbridge::AST::Bold.new + bold << Markbridge::AST::Text.new("a") + bold << Markbridge::AST::Text.new("b") + inner = Markbridge::AST::Italic.new + inner << Markbridge::AST::Text.new("c") + bold << inner + + expect(project(bold)).to eq("abc") + end + + it "uses an opaque leaf's alt text when present" do + upload = Markbridge::AST::Upload.new(sha1: "x", type: :image, alt: "a cat") + expect(project(upload)).to eq("a cat") + end + + it "falls back to raw when there is no alt" do + upload = Markbridge::AST::Upload.new(sha1: "x", type: :image, raw: "![](upload://x)") + expect(project(upload)).to eq("![](upload://x)") + end + + it "projects to the empty string for an opaque leaf carrying no text" do + expect(project(Markbridge::AST::HorizontalRule.new)).to eq("") + end + + it "projects to the empty string when alt and raw are both nil" do + # Upload responds to both :alt and :raw, but here they are nil — the + # guards must fall through to "" rather than returning nil. + expect(project(Markbridge::AST::Upload.new(sha1: "x"))).to eq("") + end +end diff --git a/spec/unit/markbridge/normalizer/walker_spec.rb b/spec/unit/markbridge/normalizer/walker_spec.rb new file mode 100644 index 00000000..8af46442 --- /dev/null +++ b/spec/unit/markbridge/normalizer/walker_spec.rb @@ -0,0 +1,697 @@ +# frozen_string_literal: true + +# The Walker is exercised through the public Normalizer#normalize (no private +# calls), but lives under a describe matching the subject so mutant selects +# these as the engine's tests. +RSpec.describe Markbridge::Normalizer::Walker do + def el(klass, *children, **kwargs) + node = kwargs.empty? ? klass.new : klass.new(**kwargs) + children.each { |child| node << child } + node + end + + def doc(*children) = el(Markbridge::AST::Document, *children) + def text(string) = Markbridge::AST::Text.new(string) + def url(*children, href: "https://ex.com") = el(Markbridge::AST::Url, *children, href:) + def image(src: "https://ex.com/i.png") = Markbridge::AST::Image.new(src:) + + # The default rules do not move an image out of a link. These engine + # examples add that rule so they have a simple violation to use. + let(:normalizer) do + Markbridge::Normalizer.default.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + end + + describe "hoist_after" do + it "moves an image out of a link, reports it, and leaves the empty link" do + img = image + tree = doc(url(img)) + report = normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Image]) + expect(tree.children.last).to be(img) # the same node, moved + expect(tree.children.first.children).to eq([]) + expect(report).to eq([{ parent: "Url", child: "Image", strategy: :hoist_after, count: 1 }]) + end + + it "does not push a bubble entry when the hoisted node is pruned to nil" do + # An empty prunable wrapper hoisted out normalizes to nil — it must not + # be pushed as a [nil, boundary] bubble (which would splice a nil child). + custom = Markbridge::Normalizer.default + custom.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Bold, + strategy: :hoist_after, + ) + tree = doc(url(Markbridge::AST::Bold.new)) # empty bold + + expect { custom.normalize(tree) }.not_to raise_error + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url]) + expect(tree.children.first.children).to eq([]) + end + + it "lands a hoisted node inside the boundary's (non-root) parent, in position" do + img = image + italic = el(Markbridge::AST::Italic, text("after")) + # image in url in bold: image hoists to right after the url, INSIDE the bold + tree = doc(el(Markbridge::AST::Bold, url(img), italic)) + normalizer.normalize(tree) + + bold = tree.children.first + expect(bold).to be_a(Markbridge::AST::Bold) + expect(bold.children.map(&:class)).to eq( + [Markbridge::AST::Url, Markbridge::AST::Image, Markbridge::AST::Italic], + ) + expect(bold.children[1]).to be(img) # landed between url and italic, not at root + end + + it "hoists to the outermost offending ancestor across nested formatting" do + img = image + tree = doc(url(el(Markbridge::AST::Bold, img))) + normalizer.normalize(tree) + + # after the link, not after the bold + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Image]) + expect(tree.children.last).to be(img) + # bold emptied → pruned + expect(tree.descendants(Markbridge::AST::Bold)).to be_empty + end + + it "preserves document order for several hoists to one boundary" do + img1 = Markbridge::AST::Image.new(src: "one") + img2 = Markbridge::AST::Image.new(src: "two") + tree = doc(url(img1, text("mid"), img2)) + normalizer.normalize(tree) + + link, first, second = tree.children + expect(link.children.map(&:text)).to eq(["mid"]) + expect([first, second]).to eq([img1, img2]) # order preserved + end + + it "keeps a relocated subtree's interior intact (destination-stack walk)" do + inner_quote = el(Markbridge::AST::Quote, text("deep")) + tree = doc(url(el(Markbridge::AST::Quote, inner_quote))) + normalizer.normalize(tree) + + hoisted = tree.children.last + expect(hoisted).to be_a(Markbridge::AST::Quote) + expect(hoisted.children).to eq([inner_quote]) # inner quote NOT flattened out + end + + it "hoists a block-level Poll out of an inline container (not just a link)" do + # Poll renders block-level, so it breaks emphasis with blank lines just + # like it breaks a link label. + poll = Markbridge::AST::Poll.new(name: "p") + tree = doc(el(Markbridge::AST::Bold, text("x"), poll)) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Bold, Markbridge::AST::Poll]) + expect(tree.children.last).to be(poll) + expect(tree.children.first.children.map(&:class)).to eq([Markbridge::AST::Text]) + end + + it "does not rip an image out of a quote that is itself hoisted from a link" do + img = image + tree = doc(url(el(Markbridge::AST::Quote, img))) + normalizer.normalize(tree) + + hoisted_quote = tree.children.last + expect(hoisted_quote.children).to eq([img]) # image stays in the quote + end + end + + describe "unwrap" do + it "replaces the inner link with its label, keeping the outer link, and reports it" do + label = text("click") + tree = doc(url(url(label, href: "https://b.com"), href: "https://a.com")) + report = normalizer.normalize(tree) + + outer = tree.children.first + expect(outer.href).to eq("https://a.com") + expect(outer.children).to eq([label]) + expect(report).to eq([{ parent: "Url", child: "Url", strategy: :unwrap, count: 1 }]) + end + + it "re-resolves the dissolved children (image inside inner link hoists out)" do + img = image + inner = url(text("x"), img, href: "https://b.com") + tree = doc(url(inner, href: "https://a.com")) + normalizer.normalize(tree) + + outer, hoisted = tree.children + expect(outer.href).to eq("https://a.com") + expect(outer.children.map(&:class)).to eq([Markbridge::AST::Text]) + expect(hoisted).to be(img) + end + + it "reaches a fixpoint for triple-nested links in one pass" do + tree = doc(url(url(url(text("deep"), href: "c"), href: "b"), href: "a")) + first = normalizer.normalize(tree) + second = normalizer.normalize(tree) + + expect(first).not_to be_empty + expect(second).to eq([]) + expect(tree.children.size).to eq(1) + expect(tree.children.first.href).to eq("a") + end + end + + describe "textify / drop / splice (custom rules)" do + it "textifies a subtree to its plain-text projection" do + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Mention, + strategy: :textify, + ) + tree = doc(url(text("hi "), Markbridge::AST::Mention.new(name: "alice"))) + report = normalizer.normalize(tree) + + link = tree.children.first + expect(link.children.size).to eq(1) + expect(link.children.first.text).to eq("hi @alice") + expect(report).to eq([{ parent: "Url", child: "Mention", strategy: :textify, count: 1 }]) + end + + it "drops a node entirely and reports it" do + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :drop) + tree = doc(url(text("label"), image)) + report = normalizer.normalize(tree) + + expect(tree.children.first.children.map(&:class)).to eq([Markbridge::AST::Text]) + expect(report).to eq([{ parent: "Url", child: "Image", strategy: :drop, count: 1 }]) + end + + it "splices in replacement nodes when a callable returns an Array, reported as :replace" do + replacement = Markbridge::AST::Text.new("[img]") + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: ->(_boundary, _node) { [replacement] }, + ) + tree = doc(url(text("a "), image)) + report = normalizer.normalize(tree) + + # "a " + "[img]" coalesced into one Text + expect(tree.children.first.children.map(&:text)).to eq(["a [img]"]) + expect(report).to eq([{ parent: "Url", child: "Image", strategy: :replace, count: 1 }]) + end + + it "passes the boundary to a callable strategy" do + seen = [] + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: + lambda do |boundary, _node| + seen << boundary + :drop + end, + ) + link = url(image) + tree = doc(link) + normalizer.normalize(tree) + + expect(seen).to eq([link]) # the offending Url, not nil + end + + it "raises when a callable resolves to an unknown strategy" do + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: ->(_boundary, _node) { :bogus }, + ) + tree = doc(url(image)) + + expect { normalizer.normalize(tree) }.to raise_error( + ArgumentError, + /strategy resolved to :bogus/, + ) + end + end + + describe "bubble preservation across non-hoist strategies" do + # A pending hoist (the leading image) must survive when the NEXT sibling + # is handled by a strategy that returns the accumulated bubble. + def link_with(strategy) + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Mention, strategy:) + img = image + tree = doc(url(img, Markbridge::AST::Mention.new(name: "m"))) + normalizer.normalize(tree) + [tree, img] + end + + it "keeps a pending hoist across a :textify sibling" do + tree, img = link_with(:textify) + expect(tree.children.last).to be(img) + end + + it "keeps a pending hoist across a :drop sibling" do + tree, img = link_with(:drop) + expect(tree.children.last).to be(img) + end + + it "keeps a pending hoist across a spliced (callable) sibling" do + tree, img = link_with(->(_b, _n) { [Markbridge::AST::Text.new("m")] }) + expect(tree.children.last).to be(img) + end + + it "keeps a pending hoist across an :unwrap of a leaf sibling" do + tree, img = link_with(:unwrap) # Mention is a leaf → dissolves to nothing + expect(tree.children.last).to be(img) + end + end + + describe "pruning" do + # Every entry in PRUNE_WHEN_EMPTY, emptied via a custom hoist rule so + # even the non-inline-container wrappers (Color/Size/Align/Email) are + # exercised. A missing entry would leave an empty wrapper and fail here. + prunable = { + Markbridge::AST::Bold => { + }, + Markbridge::AST::Italic => { + }, + Markbridge::AST::Underline => { + }, + Markbridge::AST::Strikethrough => { + }, + Markbridge::AST::Superscript => { + }, + Markbridge::AST::Subscript => { + }, + Markbridge::AST::Color => { + color: "red", + }, + Markbridge::AST::Size => { + size: "5", + }, + Markbridge::AST::Align => { + alignment: "center", + }, + Markbridge::AST::Email => { + address: "a@b.c", + }, + } + + prunable.each do |wrapper, kwargs| + it "prunes an emptied #{wrapper.name.split("::").last}" do + custom = Markbridge::Normalizer.default + custom.rule(parent: wrapper, child: Markbridge::AST::Image, strategy: :hoist_after) + node = kwargs.empty? ? wrapper.new : wrapper.new(**kwargs) + node << image + tree = doc(node) + custom.normalize(tree) + + expect(tree.descendants(wrapper)).to be_empty + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Image]) + end + end + + it "keeps an emptied Url (not prunable) as a bare link" do + tree = doc(url(image)) + normalizer.normalize(tree) + + expect(tree.children.first).to be_a(Markbridge::AST::Url) + expect(tree.children.first.children).to eq([]) + end + end + + describe "copy-on-write" do + it "leaves unchanged children as the same objects and only rewrites what moved" do + keep_a = el(Markbridge::AST::Bold, text("a")) + keep_b = el(Markbridge::AST::Italic, text("b")) + img = image + link = url(keep_a, img, keep_b) # image between two non-mergeable wrappers + tree = doc(link) + normalizer.normalize(tree) + + expect(tree.children.first).to be(link) + expect(tree.children.first.children).to eq([keep_a, keep_b]) + expect(tree.children.first.children.first).to be(keep_a) + expect(tree.children.first.children.last).to be(keep_b) + expect(tree.children.last).to be(img) + end + + it "keeps the unchanged prefix when divergence happens after the first child" do + # text at index 0 is kept (no divergence yet); the image at index 1 + # hoists, so the copied prefix must be exactly [text]. + keep = text("keep") + img = image + link = url(keep, img) + tree = doc(link) + normalizer.normalize(tree) + + expect(link.children).to eq([keep]) + expect(link.children.first).to be(keep) + expect(tree.children.last).to be(img) + end + + it "keeps the unchanged prefix when a kept child diverges after the first child" do + # text is kept at index 0 (no divergence); the bold at index 1 is a kept + # child that diverges (its image hoists, emptying it), so the copied + # prefix must be exactly [text] and the emptied bold pruned away. + keep = text("keep") + img = image + link = url(keep, el(Markbridge::AST::Bold, img)) + tree = doc(link) + normalizer.normalize(tree) + + expect(link.children).to eq([keep]) + expect(link.children.first).to be(keep) + expect(tree.children.last).to be(img) + end + + it "pops each element off the shared stack so a later sibling sees no stale ancestor" do + # url_a is a sibling of the bold, not an ancestor of url_b. If url_a were + # left on the stack, url_b would resolve (Url, Url) and be unwrapped. + inner = url(text("b"), href: "https://b.com") + bold = el(Markbridge::AST::Bold, inner) + tree = doc(url(text("a"), href: "https://a.com"), bold) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Bold]) + expect(bold.children.first).to be(inner) # url_b intact, not unwrapped + end + + it "does not touch a violation-free tree at all (same child arrays)" do + inner = text("hello") + bold = el(Markbridge::AST::Bold, inner) + tree = doc(bold) + before = tree.children + report = normalizer.normalize(tree) + + expect(report).to eq([]) + expect(tree.children).to be(before) # array identity: never replaced + expect(bold.children.first).to be(inner) + end + end + + describe "coalescing" do + it "merges adjacent MarkdownText produced by an unwrap" do + md1 = Markbridge::AST::MarkdownText.new("**a**") + md2 = Markbridge::AST::MarkdownText.new("**b**") + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + tree = doc(url(el(Markbridge::AST::Bold, md1, md2))) + normalizer.normalize(tree) + + link = tree.children.first + expect(link.children.size).to eq(1) + expect(link.children.first).to be_a(Markbridge::AST::MarkdownText) + expect(link.children.first.text).to eq("**a****b**") + end + + it "does not merge a Text next to a MarkdownText" do + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + tree = + doc( + url( + el(Markbridge::AST::Bold, text("plain"), Markbridge::AST::MarkdownText.new("**md**")), + ), + ) + normalizer.normalize(tree) + + link = tree.children.first + expect(link.children.map(&:class)).to eq( + [Markbridge::AST::Text, Markbridge::AST::MarkdownText], + ) + end + + it "does not merge a MarkdownText followed by a plain Text" do + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + tree = + doc( + url( + el(Markbridge::AST::Bold, Markbridge::AST::MarkdownText.new("**md**"), text("plain")), + ), + ) + normalizer.normalize(tree) + + expect(tree.children.first.children.map(&:class)).to eq( + [Markbridge::AST::MarkdownText, Markbridge::AST::Text], + ) + end + + it "does not merge (or crash on) a non-text node adjacent to a Text" do + # common_mark has no (Url, Image) rule, so the image survives the unwrap + # and lands next to the text — mergeable? must reject the pair. + unwrapper = Markbridge::Normalizer.default + unwrapper.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + img = image + tree = doc(url(el(Markbridge::AST::Bold, img, text("x")))) + + expect { unwrapper.normalize(tree) }.not_to raise_error + expect(tree.children.first.children.map(&:class)).to eq( + [Markbridge::AST::Image, Markbridge::AST::Text], + ) + end + end + + describe "ancestor stack ordering and destination" do + it "picks the outermost boundary when a node is inside two matching ancestors" do + # quote inside a link inside a heading: (Heading, Quote) and (Url, Quote) + # both match; the outermost (Heading) wins, so the quote lands at the + # document level after the heading — not inside it after the link. + heading = el(Markbridge::AST::Heading, url(el(Markbridge::AST::Quote, text("q"))), level: 1) + tree = doc(heading) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Heading, Markbridge::AST::Quote]) + end + + it "walks a hoisted subtree against the ancestors above its boundary" do + # (Url, Quote) hoists the quote out of the link; a custom (Color, Image) + # rule must still fire on the image inside that quote, which is only + # possible if the destination stack still contains the Color ancestor. + # (Color is used because, unlike an inline container, it has no block + # rule of its own, so the quote's boundary stays at the Url.) + custom = Markbridge::Normalizer.default + custom.rule( + parent: Markbridge::AST::Color, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + img = image + color = el(Markbridge::AST::Color, url(el(Markbridge::AST::Quote, img)), color: "red") + tree = doc(color) + custom.normalize(tree) + + # image escaped the quote entirely (destination stack retained Color) + quote = tree.descendants(Markbridge::AST::Quote).first + expect(quote.descendants(Markbridge::AST::Image)).to be_empty + expect(tree.descendants(Markbridge::AST::Image)).to contain_exactly(img) + end + + it "retains the root in the destination stack (a document-boundary rule fires on deep content)" do + # (Url, Quote) hoists the quote out of the link; a custom (Document, Image) + # rule must still fire on the image inside that quote, which requires the + # destination stack to still contain the Document (root) ancestor. + custom = Markbridge::Normalizer.default + custom.rule( + parent: Markbridge::AST::Document, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + img = image + tree = doc(url(el(Markbridge::AST::Quote, img))) + custom.normalize(tree) + + quote = tree.descendants(Markbridge::AST::Quote).first + expect(quote.descendants(Markbridge::AST::Image)).to be_empty + expect(tree.children.last).to be(img) # escaped all the way to the document + end + + it "lands a propagated hoist after its boundary, not at the very end" do + # image → boundary is the link; a trailing sibling must stay after it. + img = image + tail = el(Markbridge::AST::Italic, text("tail")) + tree = doc(url(el(Markbridge::AST::Bold, img)), tail) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq( + [Markbridge::AST::Url, Markbridge::AST::Image, Markbridge::AST::Italic], + ) + expect(tree.children[1]).to be(img) + end + end + + describe "inline-code predicate through the walker" do + it "keeps an inline code span in a link" do + code = el(Markbridge::AST::Code, text("x")) + tree = doc(url(code)) + normalizer.normalize(tree) + + expect(tree.children.first.children).to eq([code]) + end + + it "hoists a multi-line code block out of a link" do + code = el(Markbridge::AST::Code, text("a\nb")) + tree = doc(url(code)) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Code]) + end + + it "hoists a multi-line code block out of an inline container that is not a link" do + code = el(Markbridge::AST::Code, text("a\nb")) + tree = doc(el(Markbridge::AST::Bold, code)) + normalizer.normalize(tree) + + # bold emptied → pruned; the block lands at the top level + expect(tree.descendants(Markbridge::AST::Bold)).to be_empty + expect(tree.children).to eq([code]) + end + + it "keeps an inline code span inside bold" do + code = el(Markbridge::AST::Code, text("x")) + tree = doc(el(Markbridge::AST::Bold, code)) + normalizer.normalize(tree) + + expect(tree.children.first).to be_a(Markbridge::AST::Bold) + expect(tree.children.first.children).to eq([code]) + end + end + + describe "unwrap edge cases" do + it "keeps a leaf targeted by an unwrap rule and does not report a no-op" do + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Mention, + strategy: :unwrap, + ) + mention = Markbridge::AST::Mention.new(name: "x") + tree = doc(url(text("a"), mention)) + report = normalizer.normalize(tree) + + expect(tree.children.first.children.map(&:class)).to eq( + [Markbridge::AST::Text, Markbridge::AST::Mention], + ) + expect(tree.children.first.children.last).to be(mention) # kept, not dropped + expect(report).to eq([]) # unwrap of a leaf is a silent no-op + end + + it "resolves a callable rule on a dissolved (unwrapped) child, reporting the boundary" do + # unwrap the bold; the code block it held must then be hoisted out of the + # link by the (Url, Code) callable — exercised via the unwrap recursion. + # The reported parent must be the Url boundary, not nil. + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + tree = doc(url(el(Markbridge::AST::Bold, el(Markbridge::AST::Code, text("a\nb"))))) + report = normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Code]) + expect(report).to include({ parent: "Url", child: "Code", strategy: :hoist_after, count: 1 }) + end + + it "passes the real boundary to a callable resolved on a dissolved child" do + seen = [] + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: + lambda do |boundary, _node| + seen << boundary + :drop + end, + ) + link = url(el(Markbridge::AST::Bold, image)) + normalizer.normalize(doc(link)) + + expect(seen).to eq([link]) # the Url boundary, not nil + end + + it "lands a hoist raised inside a dissolved child after that child" do + # (Color, Bold) unwrap dissolves the bold; the inner link keeps its + # image hoist landing right after the link, inside the Color — the + # dissolved child (the link) is itself the boundary. + custom = Markbridge::Normalizer.default + custom.rule(parent: Markbridge::AST::Color, child: Markbridge::AST::Bold, strategy: :unwrap) + custom.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + img = image + color = el(Markbridge::AST::Color, el(Markbridge::AST::Bold, url(img)), color: "red") + custom.normalize(doc(color)) + + expect(color.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Image]) + expect(color.children.last).to be(img) + end + + it "preserves multiple hoists raised while dissolving one child" do + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + img1 = Markbridge::AST::Image.new(src: "one") + img2 = Markbridge::AST::Image.new(src: "two") + tree = doc(url(el(Markbridge::AST::Bold, img1, img2))) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq( + [Markbridge::AST::Url, Markbridge::AST::Image, Markbridge::AST::Image], + ) + expect(tree.children[1, 2]).to eq([img1, img2]) # both images, in order + end + + it "keeps a dissolved child that resolves to :keep (not routed to emit)" do + # unwrap the bold; the mention it held resolves to an explicit :keep and + # must be appended, not sent through emit (which would raise on :keep). + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Mention, + strategy: :keep, + ) + tree = doc(url(el(Markbridge::AST::Bold, Markbridge::AST::Mention.new(name: "x")))) + + expect { normalizer.normalize(tree) }.not_to raise_error + expect(tree.children.first.children.map(&:class)).to eq([Markbridge::AST::Mention]) + end + + it "preserves an earlier sibling's hoist across a later unwrap" do + # image hoists first; then the bold unwraps — the pending [image] bubble + # must survive into the unwrap, not be dropped. + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + img = image + tree = doc(url(img, el(Markbridge::AST::Bold, text("k")))) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Image]) + expect(tree.children.last).to be(img) + expect(tree.children.first.children.map(&:text)).to eq(["k"]) + end + + it "preserves a hoist raised while dissolving, across the remaining children" do + # image (first dissolved child) hoists; the following text must not drop + # its pending bubble. + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Bold, strategy: :unwrap) + img = image + tree = doc(url(el(Markbridge::AST::Bold, img, text("k")))) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Image]) + expect(tree.children.last).to be(img) + expect(tree.children.first.children.map(&:text)).to eq(["k"]) + end + end + + describe "root-level bubble (defensive)" do + it "appends a node hoisted with the document as its boundary" do + # A (Document, Image) rule makes the document the hoist boundary; the + # bubble reaches the root and Walker#call must append it (not drop it). + custom = Markbridge::Normalizer.default + custom.rule( + parent: Markbridge::AST::Document, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + img = image + tree = doc(img, text("b")) + custom.normalize(tree) + + # image removed from the front, appended at the end by the root fallback + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Text, Markbridge::AST::Image]) + expect(tree.children.last).to be(img) + end + end +end diff --git a/spec/unit/markbridge/normalizer_spec.rb b/spec/unit/markbridge/normalizer_spec.rb new file mode 100644 index 00000000..f6131ee7 --- /dev/null +++ b/spec/unit/markbridge/normalizer_spec.rb @@ -0,0 +1,398 @@ +# frozen_string_literal: true + +RSpec.describe Markbridge::Normalizer do + # Concise tree builders. `el` wraps a klass with children; the helpers + # name the common nodes. + def el(klass, *children, **kwargs) + node = kwargs.empty? ? klass.new : klass.new(**kwargs) + children.each { |child| node << child } + node + end + + def doc(*children) = el(Markbridge::AST::Document, *children) + def text(string) = Markbridge::AST::Text.new(string) + def url(*children, href: "https://ex.com") = el(Markbridge::AST::Url, *children, href:) + def image(src: "https://ex.com/i.png") = Markbridge::AST::Image.new(src:) + + # Any valid instance of the class; the rules only look at the class. + def instance(klass) + case klass.name.split("::").last + when "Heading" + klass.new(level: 1) + when "Align" + klass.new(alignment: "center") + when "Url" + klass.new(href: "u") + when "Image" + klass.new(src: "s") + when "Poll" + klass.new(name: "p") + when "Event" + klass.new(name: "e", starts_at: "2026-01-01") + else + klass.new + end + end + + # An instance of +container_klass+ holding +child+. + def wrap(container_klass, child) = instance(container_klass).tap { |c| c << child } + + def short(klass) = klass.name.split("::").last + + # The default rules do not move an image out of a link (that is Discourse + # policy a consumer adds). The examples below add that rule so they have a + # simple case to work with. + subject(:normalizer) do + described_class.default.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :hoist_after, + ) + end + + describe ".default" do + it "builds a fresh, customizable instance" do + one = described_class.default + two = described_class.default + expect(one).to be_a(described_class) + expect(one).not_to be(two) + expect(one).not_to be_frozen + end + + it "carries the default rules (a link inside a link is unwrapped)" do + tree = doc(url(url(text("x"), href: "b"), href: "a")) + described_class.default.normalize(tree) + + expect(tree.children.size).to eq(1) + expect(tree.children.first.href).to eq("a") + end + end + + describe ".shared_default" do + it "returns the same frozen instance on every call" do + expect(described_class.shared_default).to be_a(described_class) + expect(described_class.shared_default).to be(described_class.shared_default) + expect(described_class.shared_default).to be_frozen + end + + it "normalizes and can be reused, even though it is frozen" do + shared = described_class.shared_default + 2.times do + tree = doc(url(url(text("x"), href: "b"), href: "a")) + expect(shared.normalize(tree)).to eq( + [{ parent: "Url", child: "Url", strategy: :unwrap, count: 1 }], + ) + end + end + end + + describe "default rules" do + it "unwraps a link inside a link" do + expect(described_class.default.violations(doc(url(url(text("x")))))).to contain_exactly( + { parent: "Url", child: "Url", strategy: :unwrap }, + ) + end + + it "flags every block node inside every inline container" do + described_class::INLINE_CONTAINERS.each do |container| + described_class::BLOCK_NODES.each do |block| + next if container == block + + found = described_class.default.violations(doc(wrap(container, instance(block)))) + expect(found).to include( + { parent: short(container), child: short(block), strategy: :hoist_after }, + ), + "(#{container}, #{block}) not flagged" + end + end + end + + it "keeps an inline code span but flags a fenced one, in any inline container" do + described_class::INLINE_CONTAINERS.each do |container| + inline = el(Markbridge::AST::Code, text("x")) + fenced = el(Markbridge::AST::Code, text("a\nb")) + + expect(described_class.default.violations(doc(wrap(container, inline)))).to eq([]), + "inline code in #{container}" + expect(described_class.default.violations(doc(wrap(container, fenced)))).to contain_exactly( + { parent: short(container), child: "Code", strategy: :hoist_after }, + ), + "fenced code in #{container}" + end + end + + it "does not flag a node that is both an inline container and a block against itself" do + # Heading is in INLINE_CONTAINERS and in BLOCK_NODES. build_rules must not + # add a (Heading, Heading) hoist rule, or a heading in a heading would move. + heading = Markbridge::AST::Heading + tree = doc(wrap(heading, instance(heading))) + + expect(described_class.default.violations(tree)).to eq([]) + end + + it "does not add Discourse policy: an image or mention in a link is not flagged" do + expect(described_class.default.violations(doc(url(image)))).to eq([]) + mention = Markbridge::AST::Mention.new(name: "a") + expect(described_class.default.violations(doc(url(mention)))).to eq([]) + end + end + + describe "#rule" do + it "is chainable" do + expect( + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Mention, + strategy: :textify, + ), + ).to be(normalizer) + end + + it "replaces an existing rule for the same (parent, child) pair" do + tree = doc(url(image)) + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :drop) + normalizer.normalize(tree) + + # was :hoist_after, now :drop — image gone, nothing hoisted + expect(tree.children).to eq([tree.children.first]) + expect(tree.children.first.children).to eq([]) + end + + it "raises on a frozen (shared) instance" do + expect { + described_class.shared_default.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: :drop, + ) + }.to raise_error(FrozenError) + end + end + + describe "#normalize strategies" do + it "hoists an image out of a link, leaves the (empty) link, and reports it" do + tree = doc(url(image)) + report = normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Image]) + expect(tree.children.first.children).to eq([]) # empty Url survives (bare link) + expect(report).to contain_exactly( + { parent: "Url", child: "Image", strategy: :hoist_after, count: 1 }, + ) + end + + it "hoists to the OUTERMOST offending ancestor, across nested formatting" do + # image inside bold inside link → hoist after the LINK, not the bold + tree = doc(url(el(Markbridge::AST::Bold, image))) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Image]) + # bold emptied by the hoist → removed (no empty ** **) + expect(tree.children.first.children).to eq([]) + end + + it "prunes an emptied formatting wrapper but never an emptied Url" do + tree = doc(url(el(Markbridge::AST::Bold, image))) + normalizer.normalize(tree) + + expect(tree.descendants(Markbridge::AST::Bold)).to be_empty + expect(tree.children.first).to be_a(Markbridge::AST::Url) # kept as bare link + end + + it "unwraps an inner link, keeping its label text" do + inner = url(text("click"), href: "https://b.com") + tree = doc(url(inner, href: "https://a.com")) + report = normalizer.normalize(tree) + + expect(tree.children.size).to eq(1) + outer = tree.children.first + expect(outer).to be_a(Markbridge::AST::Url) + expect(outer.href).to eq("https://a.com") + expect(outer.children.map { |c| [c.class, c.respond_to?(:text) ? c.text : nil] }).to eq( + [[Markbridge::AST::Text, "click"]], + ) + expect(report).to contain_exactly( + { parent: "Url", child: "Url", strategy: :unwrap, count: 1 }, + ) + end + + it "textifies a node via a custom rule, projecting its plain text" do + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Mention, + strategy: :textify, + ) + tree = doc(url(text("hi "), Markbridge::AST::Mention.new(name: "alice"))) + normalizer.normalize(tree) + + link = tree.children.first + expect(link.children.size).to eq(1) # "hi " + "@alice" coalesced + expect(link.children.first.text).to eq("hi @alice") + end + + it "drops a node via a custom rule" do + normalizer.rule(parent: Markbridge::AST::Url, child: Markbridge::AST::Image, strategy: :drop) + tree = doc(url(text("label"), image)) + normalizer.normalize(tree) + + link = tree.children.first + expect(link.children.map(&:class)).to eq([Markbridge::AST::Text]) + end + + it "keeps a mention in a link silently (no report row)" do + tree = doc(url(Markbridge::AST::Mention.new(name: "alice"))) + report = normalizer.normalize(tree) + + expect(tree.children.first.children.map(&:class)).to eq([Markbridge::AST::Mention]) + expect(report).to be_empty + end + end + + describe "#normalize ordering (moved subtrees walk against their new-place stack)" do + it "keeps a legally-nested quote-in-quote intact when the outer quote is hoisted" do + inner_quote = el(Markbridge::AST::Quote, text("deep")) + outer_quote = el(Markbridge::AST::Quote, inner_quote) + tree = doc(url(outer_quote)) + normalizer.normalize(tree) + + # outer quote hoisted after the (empty) link; inner quote STILL nested + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Quote]) + hoisted = tree.children.last + expect(hoisted.children).to contain_exactly(be_a(Markbridge::AST::Quote)) + expect(hoisted.children.first.children.first.text).to eq("deep") + end + + it "leaves an image inside a quote inside a link where it is (in the quote)" do + quote = el(Markbridge::AST::Quote, image) + tree = doc(url(quote)) + normalizer.normalize(tree) + + # quote hoists out of the link; image stays IN the quote (not ripped out) + hoisted_quote = tree.children.last + expect(hoisted_quote).to be_a(Markbridge::AST::Quote) + expect(hoisted_quote.children).to contain_exactly(be_a(Markbridge::AST::Image)) + end + + it "preserves document order for multiple hoists to one link" do + img1 = Markbridge::AST::Image.new(src: "one") + img2 = Markbridge::AST::Image.new(src: "two") + tree = doc(url(img1, text("mid"), img2)) + normalizer.normalize(tree) + + # link keeps "mid"; images land after the link in original order + link, first, second = tree.children + expect(link).to be_a(Markbridge::AST::Url) + expect(link.children.map(&:text)).to eq(["mid"]) + expect([first.src, second.src]).to eq(%w[one two]) + end + end + + describe "#normalize fixpoint" do + it "reaches a fixpoint: a second normalize reports nothing" do + trees = [ + doc(url(el(Markbridge::AST::Bold, image))), + doc(url(url(text("x"), href: "b"), href: "a")), + doc(url(el(Markbridge::AST::Quote, el(Markbridge::AST::Quote, image)))), + doc(url(url(url(text("deep"), href: "c"), href: "b"), href: "a")), + ] + + trees.each do |tree| + first = normalizer.normalize(tree) + second = normalizer.normalize(tree) + expect(first).not_to be_empty + expect(second).to eq([]), "expected empty second report, got #{second.inspect}" + end + end + end + + describe "#normalize inline-code predicate (Url, Code)" do + it "keeps an inline code span in a link label" do + code = el(Markbridge::AST::Code, text("x")) + tree = doc(url(code)) + normalizer.normalize(tree) + + expect(tree.children.first).to be_a(Markbridge::AST::Url) + expect(tree.children.first.children).to contain_exactly(be_a(Markbridge::AST::Code)) + end + + it "hoists a multi-line (fenced) code block out of a link label" do + code = el(Markbridge::AST::Code, text("line1\nline2")) + tree = doc(url(code)) + normalizer.normalize(tree) + + expect(tree.children.map(&:class)).to eq([Markbridge::AST::Url, Markbridge::AST::Code]) + end + end + + describe "#normalize early exit" do + it "returns an empty report and leaves a clean tree untouched" do + tree = doc(el(Markbridge::AST::Bold, text("hi"))) + before = tree.children.first + report = normalizer.normalize(tree) + + expect(report).to eq([]) + expect(tree.children.first).to be(before) + end + end + + describe "#violations" do + it "reports would-be violations without mutating the tree" do + tree = doc(url(image)) + found = normalizer.violations(tree) + + expect(found).to contain_exactly({ parent: "Url", child: "Image", strategy: :hoist_after }) + # unchanged + expect(tree.children.first).to be_a(Markbridge::AST::Url) + expect(tree.children.first.children).to contain_exactly(be_a(Markbridge::AST::Image)) + end + + it "omits explicitly-kept nodes (a mention in a link)" do + tree = doc(url(Markbridge::AST::Mention.new(name: "alice"))) + expect(normalizer.violations(tree)).to eq([]) + end + + it "recurses into nested elements, finding violations at every depth" do + # image nested two elements deep inside a link, plus the quote itself + tree = doc(url(el(Markbridge::AST::Quote, el(Markbridge::AST::Bold, image)))) + found = normalizer.violations(tree) + + expect(found).to contain_exactly( + { parent: "Url", child: "Quote", strategy: :hoist_after }, + { parent: "Url", child: "Image", strategy: :hoist_after }, + ) + end + + it "passes the boundary to a callable rule" do + seen = [] + normalizer.rule( + parent: Markbridge::AST::Url, + child: Markbridge::AST::Image, + strategy: + lambda do |boundary, _node| + seen << boundary + :drop + end, + ) + link = url(image) + normalizer.violations(doc(link)) + + expect(seen).to eq([link]) # the offending Url, not nil + end + + it "resolves callable rules — inline code is kept (omitted), a code block reported" do + inline = doc(url(el(Markbridge::AST::Code, text("x")))) + block = doc(url(el(Markbridge::AST::Code, text("a\nb")))) + + expect(normalizer.violations(inline)).to eq([]) + expect(normalizer.violations(block)).to contain_exactly( + { parent: "Url", child: "Code", strategy: :hoist_after }, + ) + end + + it "is empty once the tree has been normalized (validation property)" do + tree = doc(url(el(Markbridge::AST::Bold, image))) + normalizer.normalize(tree) + expect(described_class.default.violations(tree)).to eq([]) + end + end +end diff --git a/spec/unit/markbridge/renderers/discourse/tags/event_tag_spec.rb b/spec/unit/markbridge/renderers/discourse/tags/event_tag_spec.rb index 69bc5d76..433f12d7 100644 --- a/spec/unit/markbridge/renderers/discourse/tags/event_tag_spec.rb +++ b/spec/unit/markbridge/renderers/discourse/tags/event_tag_spec.rb @@ -15,14 +15,14 @@ raw: "[event]ORIGINAL[/event]", ) - expect(tag.render(element, interface)).to eq("[event]ORIGINAL[/event]\n\n") + expect(tag.render(element, interface)).to eq("\n\n[event]ORIGINAL[/event]\n\n") end it "reconstructs BBCode with name and start when raw is missing" do element = Markbridge::AST::Event.new(name: "Meeting", starts_at: "2026-01-01 14:00") expect(tag.render(element, interface)).to eq( - %([event name="Meeting" start="2026-01-01 14:00"]\n[/event]\n\n), + %(\n\n[event name="Meeting" start="2026-01-01 14:00"]\n[/event]\n\n), ) end @@ -31,7 +31,7 @@ Markbridge::AST::Event.new(name: "Meeting", starts_at: "2026-01-01", ends_at: "2026-01-02") expect(tag.render(element, interface)).to eq( - %([event name="Meeting" start="2026-01-01" end="2026-01-02"]\n[/event]\n\n), + %(\n\n[event name="Meeting" start="2026-01-01" end="2026-01-02"]\n[/event]\n\n), ) end @@ -40,7 +40,7 @@ Markbridge::AST::Event.new(name: "Meeting", starts_at: "2026-01-01", status: "public") expect(tag.render(element, interface)).to eq( - %([event name="Meeting" start="2026-01-01" status="public"]\n[/event]\n\n), + %(\n\n[event name="Meeting" start="2026-01-01" status="public"]\n[/event]\n\n), ) end @@ -53,7 +53,7 @@ ) expect(tag.render(element, interface)).to eq( - %([event name="Meeting" start="2026-01-01" timezone="Europe/Vienna"]\n[/event]\n\n), + %(\n\n[event name="Meeting" start="2026-01-01" timezone="Europe/Vienna"]\n[/event]\n\n), ) end @@ -67,13 +67,14 @@ expect(result).not_to include("timezone=") end - it "emits a trailing blank line after a reconstructed event" do + it "brackets a reconstructed event with leading and trailing blank lines" do element = Markbridge::AST::Event.new(name: "Meeting", starts_at: "2026-04-24 10:00") + expect(tag.render(element, interface)).to start_with("\n\n[event") expect(tag.render(element, interface)).to end_with("[/event]\n\n") end - it "emits a trailing blank line after a raw-passthrough event" do + it "brackets a raw-passthrough event the same way" do element = Markbridge::AST::Event.new( name: "Meeting", @@ -81,26 +82,22 @@ raw: %([event name="Meeting" start="2026-04-24 10:00"]\n[/event]), ) + expect(tag.render(element, interface)).to start_with("\n\n[event") expect(tag.render(element, interface)).to end_with("[/event]\n\n") end - context "in html_mode" do - let(:context) { Markbridge::Renderers::Discourse::RenderContext.new([], html_mode: true) } - - it "wraps the BBCode in leading + trailing blank lines so CommonMark re-enters Markdown parsing" do - element = Markbridge::AST::Event.new(name: "Meeting", starts_at: "2026-01-01") - - expect(tag.render(element, interface)).to eq( - %(\n\n[event name="Meeting" start="2026-01-01"]\n[/event]\n\n), - ) - end - - it "wraps a raw-passthrough event in blank lines too" do - element = - Markbridge::AST::Event.new(name: "x", starts_at: "y", raw: "[event]ORIGINAL[/event]") - - expect(tag.render(element, interface)).to eq("\n\n[event]ORIGINAL[/event]\n\n") - end + # The stub is mode-agnostic: the same blank-line-bracketed island serves + # both a standalone block in Markdown and the html_mode contract (which + # is enforced by html_mode_contract_spec). + it "emits the same island form in html_mode" do + html_context = Markbridge::Renderers::Discourse::RenderContext.new([], html_mode: true) + html_interface = + Markbridge::Renderers::Discourse::RenderingInterface.new(renderer, html_context) + element = Markbridge::AST::Event.new(name: "Meeting", starts_at: "2026-01-01") + + expect(tag.render(element, html_interface)).to eq( + %(\n\n[event name="Meeting" start="2026-01-01"]\n[/event]\n\n), + ) end end end diff --git a/spec/unit/markbridge/renderers/discourse/tags/poll_tag_spec.rb b/spec/unit/markbridge/renderers/discourse/tags/poll_tag_spec.rb index cb31e45f..51959014 100644 --- a/spec/unit/markbridge/renderers/discourse/tags/poll_tag_spec.rb +++ b/spec/unit/markbridge/renderers/discourse/tags/poll_tag_spec.rb @@ -7,16 +7,16 @@ let(:interface) { Markbridge::Renderers::Discourse::RenderingInterface.new(renderer, context) } describe "#render" do - it "returns the raw BBCode verbatim when present" do + it "returns the raw BBCode verbatim when present, as a standalone block" do element = Markbridge::AST::Poll.new(raw: "[poll]ORIGINAL[/poll]") - expect(tag.render(element, interface)).to eq("[poll]ORIGINAL[/poll]\n\n") + expect(tag.render(element, interface)).to eq("\n\n[poll]ORIGINAL[/poll]\n\n") end it "reconstructs BBCode with options when raw is missing" do element = Markbridge::AST::Poll.new(options: %w[A B]) - expect(tag.render(element, interface)).to eq("[poll]\n* A\n* B\n[/poll]\n\n") + expect(tag.render(element, interface)).to eq("\n\n[poll]\n* A\n* B\n[/poll]\n\n") end it "omits the name attribute when it equals the default 'poll'" do @@ -70,35 +70,32 @@ it "produces an empty options block when there are no options" do element = Markbridge::AST::Poll.new - expect(tag.render(element, interface)).to eq("[poll]\n\n[/poll]\n\n") + expect(tag.render(element, interface)).to eq("\n\n[poll]\n\n[/poll]\n\n") end - it "emits a trailing blank line after a reconstructed poll" do + it "brackets a reconstructed poll with leading and trailing blank lines" do element = Markbridge::AST::Poll.new(name: "fav", type: "regular", options: %w[A B]) + expect(tag.render(element, interface)).to start_with("\n\n[poll") expect(tag.render(element, interface)).to end_with("[/poll]\n\n") end - it "emits a trailing blank line after a raw-passthrough poll" do + it "brackets a raw-passthrough poll the same way" do element = Markbridge::AST::Poll.new(raw: "[poll]\n* A\n* B\n[/poll]") - expect(tag.render(element, interface)).to eq("[poll]\n* A\n* B\n[/poll]\n\n") + expect(tag.render(element, interface)).to eq("\n\n[poll]\n* A\n* B\n[/poll]\n\n") end - context "in html_mode" do - let(:context) { Markbridge::Renderers::Discourse::RenderContext.new([], html_mode: true) } - - it "wraps the BBCode in leading + trailing blank lines so CommonMark re-enters Markdown parsing" do - element = Markbridge::AST::Poll.new(options: %w[A B]) - - expect(tag.render(element, interface)).to eq("\n\n[poll]\n* A\n* B\n[/poll]\n\n") - end - - it "wraps a raw-passthrough poll in blank lines too" do - element = Markbridge::AST::Poll.new(raw: "[poll]ORIGINAL[/poll]") + # The stub is mode-agnostic: the same blank-line-bracketed island serves + # both a standalone block in Markdown and the html_mode contract (which + # is enforced by html_mode_contract_spec). + it "emits the same island form in html_mode" do + html_context = Markbridge::Renderers::Discourse::RenderContext.new([], html_mode: true) + html_interface = + Markbridge::Renderers::Discourse::RenderingInterface.new(renderer, html_context) + element = Markbridge::AST::Poll.new(options: %w[A B]) - expect(tag.render(element, interface)).to eq("\n\n[poll]ORIGINAL[/poll]\n\n") - end + expect(tag.render(element, html_interface)).to eq("\n\n[poll]\n* A\n* B\n[/poll]\n\n") end end end