Skip to content

add node depth limit#422

Open
spirit-at-canva wants to merge 4 commits intocommonmark:mainfrom
spirit-at-canva:spirit-add-node-depth-limit
Open

add node depth limit#422
spirit-at-canva wants to merge 4 commits intocommonmark:mainfrom
spirit-at-canva:spirit-add-node-depth-limit

Conversation

@spirit-at-canva
Copy link
Copy Markdown

@spirit-at-canva spirit-at-canva commented Mar 30, 2026

Why

I want to limit the max depth of node traversal.

What

Apply an optional limit to the depth of node parsing.

We make a conscious design choice to drop nodes that are too deep, instead of failing fast.

We apply the limit when constructing the AST, by dropping deeper node generation (we collapse it into plain text).

When visiting the AST, we apply a depth limit again and drop deep nodes altogether.

The two depths correspond to different meaning, and aren't semantically the same.

@spirit-at-canva spirit-at-canva force-pushed the spirit-add-node-depth-limit branch 2 times, most recently from 137fc23 to bfe2c5a Compare March 30, 2026 09:23
@spirit-at-canva spirit-at-canva force-pushed the spirit-add-node-depth-limit branch from bfe2c5a to f8f7684 Compare March 30, 2026 09:23
@spirit-at-canva
Copy link
Copy Markdown
Author

cc @robinst

Copy link
Copy Markdown
Collaborator

@robinst robinst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to DocumentParser, Parser, ParserTest LGTM, with some minor comments to address.

The visitor changes: Can you take them out of this PR? I'm not sure I want to include this by default on every visitor, and it's something that's pretty straightforward to add in subclasses (overriding the protected visitChildren method).

}

/**
* Limit how many non-document block parsers may be open at once while parsing.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top-level block parser is really an implementation detail, no need to mention it prominently:

Suggested change
* Limit how many non-document block parsers may be open at once while parsing.
* Limit how many block parsers may be open at once while parsing.

}

private String renderText(Node node) {
return TextContentRenderer.builder().build().render(node).trim();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a MarkdownRenderer instead (TextContentRenderer is an arbitrary plain text format, we don't want to rely on it for tests):

Suggested change
return TextContentRenderer.builder().build().render(node).trim();
return MarkdownRenderer.builder().build().render(node).trim();

node.accept(this);
} finally {
currentDepth--;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incrementing and decrementing should probably be done outside the while loop, just once?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants