From b6ce2d6dc9db16227c523b2d0a39a231e5d38945 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 27 Nov 2024 00:46:02 +0800 Subject: Refactor markup render system (#32645) This PR mainly removes some global variables, moves some code and renames some functions to make code clearer. This PR also removes a testing-only option ForceHardLineBreak during refactoring since the behavior is clear now. --- modules/markup/html.go | 121 +++++++--------- modules/markup/html_internal_test.go | 4 +- modules/markup/html_test.go | 14 +- modules/markup/markdown/goldmark.go | 4 +- modules/markup/markdown/markdown_test.go | 240 +++++++++++++++---------------- modules/markup/render.go | 10 +- tests/fuzz/fuzz_test.go | 2 +- 7 files changed, 179 insertions(+), 216 deletions(-) diff --git a/modules/markup/html.go b/modules/markup/html.go index 0b1e9b3224..04b768bb8e 100644 --- a/modules/markup/html.go +++ b/modules/markup/html.go @@ -5,9 +5,9 @@ package markup import ( "bytes" + "fmt" "io" "regexp" - "slices" "strings" "sync" @@ -133,75 +133,49 @@ func CustomLinkURLSchemes(schemes []string) { common.GlobalVars().LinkRegex, _ = xurls.StrictMatchingScheme(strings.Join(withAuth, "|")) } -type postProcessError struct { - context string - err error -} - -func (p *postProcessError) Error() string { - return "PostProcess: " + p.context + ", " + p.err.Error() -} - type processor func(ctx *RenderContext, node *html.Node) -var defaultProcessors = []processor{ - fullIssuePatternProcessor, - comparePatternProcessor, - codePreviewPatternProcessor, - fullHashPatternProcessor, - shortLinkProcessor, - linkProcessor, - mentionProcessor, - issueIndexPatternProcessor, - commitCrossReferencePatternProcessor, - hashCurrentPatternProcessor, - emailAddressProcessor, - emojiProcessor, - emojiShortCodeProcessor, -} - -// PostProcess does the final required transformations to the passed raw HTML +// PostProcessDefault does the final required transformations to the passed raw HTML // data, and ensures its validity. Transformations include: replacing links and // emails with HTML links, parsing shortlinks in the format of [[Link]], like // MediaWiki, linking issues in the format #ID, and mentions in the format // @user, and others. -func PostProcess(ctx *RenderContext, input io.Reader, output io.Writer) error { - return postProcess(ctx, defaultProcessors, input, output) -} - -var commitMessageProcessors = []processor{ - fullIssuePatternProcessor, - comparePatternProcessor, - fullHashPatternProcessor, - linkProcessor, - mentionProcessor, - issueIndexPatternProcessor, - commitCrossReferencePatternProcessor, - hashCurrentPatternProcessor, - emailAddressProcessor, - emojiProcessor, - emojiShortCodeProcessor, +func PostProcessDefault(ctx *RenderContext, input io.Reader, output io.Writer) error { + procs := []processor{ + fullIssuePatternProcessor, + comparePatternProcessor, + codePreviewPatternProcessor, + fullHashPatternProcessor, + shortLinkProcessor, + linkProcessor, + mentionProcessor, + issueIndexPatternProcessor, + commitCrossReferencePatternProcessor, + hashCurrentPatternProcessor, + emailAddressProcessor, + emojiProcessor, + emojiShortCodeProcessor, + } + return postProcess(ctx, procs, input, output) } // RenderCommitMessage will use the same logic as PostProcess, but will disable -// the shortLinkProcessor and will add a defaultLinkProcessor if defaultLink is -// set, which changes every text node into a link to the passed default link. +// the shortLinkProcessor. func RenderCommitMessage(ctx *RenderContext, content string) (string, error) { - procs := commitMessageProcessors - return renderProcessString(ctx, procs, content) -} - -var commitMessageSubjectProcessors = []processor{ - fullIssuePatternProcessor, - comparePatternProcessor, - fullHashPatternProcessor, - linkProcessor, - mentionProcessor, - issueIndexPatternProcessor, - commitCrossReferencePatternProcessor, - hashCurrentPatternProcessor, - emojiShortCodeProcessor, - emojiProcessor, + procs := []processor{ + fullIssuePatternProcessor, + comparePatternProcessor, + fullHashPatternProcessor, + linkProcessor, + mentionProcessor, + issueIndexPatternProcessor, + commitCrossReferencePatternProcessor, + hashCurrentPatternProcessor, + emailAddressProcessor, + emojiProcessor, + emojiShortCodeProcessor, + } + return postProcessString(ctx, procs, content) } var emojiProcessors = []processor{ @@ -214,7 +188,18 @@ var emojiProcessors = []processor{ // emailAddressProcessor, will add a defaultLinkProcessor if defaultLink is set, // which changes every text node into a link to the passed default link. func RenderCommitMessageSubject(ctx *RenderContext, defaultLink, content string) (string, error) { - procs := slices.Clone(commitMessageSubjectProcessors) + procs := []processor{ + fullIssuePatternProcessor, + comparePatternProcessor, + fullHashPatternProcessor, + linkProcessor, + mentionProcessor, + issueIndexPatternProcessor, + commitCrossReferencePatternProcessor, + hashCurrentPatternProcessor, + emojiShortCodeProcessor, + emojiProcessor, + } procs = append(procs, func(ctx *RenderContext, node *html.Node) { ch := &html.Node{Parent: node, Type: html.TextNode, Data: node.Data} node.Type = html.ElementNode @@ -223,19 +208,19 @@ func RenderCommitMessageSubject(ctx *RenderContext, defaultLink, content string) node.Attr = []html.Attribute{{Key: "href", Val: defaultLink}, {Key: "class", Val: "muted"}} node.FirstChild, node.LastChild = ch, ch }) - return renderProcessString(ctx, procs, content) + return postProcessString(ctx, procs, content) } // RenderIssueTitle to process title on individual issue/pull page func RenderIssueTitle(ctx *RenderContext, title string) (string, error) { // do not render other issue/commit links in an issue's title - which in most cases is already a link. - return renderProcessString(ctx, []processor{ + return postProcessString(ctx, []processor{ emojiShortCodeProcessor, emojiProcessor, }, title) } -func renderProcessString(ctx *RenderContext, procs []processor, content string) (string, error) { +func postProcessString(ctx *RenderContext, procs []processor, content string) (string, error) { var buf strings.Builder if err := postProcess(ctx, procs, strings.NewReader(content), &buf); err != nil { return "", err @@ -246,7 +231,7 @@ func renderProcessString(ctx *RenderContext, procs []processor, content string) // RenderDescriptionHTML will use similar logic as PostProcess, but will // use a single special linkProcessor. func RenderDescriptionHTML(ctx *RenderContext, content string) (string, error) { - return renderProcessString(ctx, []processor{ + return postProcessString(ctx, []processor{ descriptionLinkProcessor, emojiShortCodeProcessor, emojiProcessor, @@ -256,7 +241,7 @@ func RenderDescriptionHTML(ctx *RenderContext, content string) (string, error) { // RenderEmoji for when we want to just process emoji and shortcodes // in various places it isn't already run through the normal markdown processor func RenderEmoji(ctx *RenderContext, content string) (string, error) { - return renderProcessString(ctx, emojiProcessors, content) + return postProcessString(ctx, emojiProcessors, content) } func postProcess(ctx *RenderContext, procs []processor, input io.Reader, output io.Writer) error { @@ -276,7 +261,7 @@ func postProcess(ctx *RenderContext, procs []processor, input io.Reader, output strings.NewReader(""), )) if err != nil { - return &postProcessError{"invalid HTML", err} + return fmt.Errorf("markup.postProcess: invalid HTML: %w", err) } if node.Type == html.DocumentNode { @@ -308,7 +293,7 @@ func postProcess(ctx *RenderContext, procs []processor, input io.Reader, output // Render everything to buf. for _, node := range newNodes { if err := html.Render(output, node); err != nil { - return &postProcessError{"error rendering processed HTML", err} + return fmt.Errorf("markup.postProcess: html.Render: %w", err) } } return nil diff --git a/modules/markup/html_internal_test.go b/modules/markup/html_internal_test.go index 7f2057a343..651e674108 100644 --- a/modules/markup/html_internal_test.go +++ b/modules/markup/html_internal_test.go @@ -277,12 +277,12 @@ func TestRender_AutoLink(t *testing.T) { test := func(input, expected string) { var buffer strings.Builder - err := PostProcess(NewTestRenderContext(localMetas), strings.NewReader(input), &buffer) + err := PostProcessDefault(NewTestRenderContext(localMetas), strings.NewReader(input), &buffer) assert.Equal(t, err, nil) assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(buffer.String())) buffer.Reset() - err = PostProcess(NewTestRenderContext(localMetas), strings.NewReader(input), &buffer) + err = PostProcessDefault(NewTestRenderContext(localMetas), strings.NewReader(input), &buffer) assert.Equal(t, err, nil) assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(buffer.String())) } diff --git a/modules/markup/html_test.go b/modules/markup/html_test.go index f806f66d11..54bd91f3b3 100644 --- a/modules/markup/html_test.go +++ b/modules/markup/html_test.go @@ -445,14 +445,14 @@ func Test_ParseClusterFuzz(t *testing.T) { data := "
` var res strings.Builder - err := markup.PostProcess(markup.NewTestRenderContext(localMetas), strings.NewReader(data), &res) + err := markup.PostProcessDefault(markup.NewTestRenderContext(localMetas), strings.NewReader(data), &res) assert.NoError(t, err) assert.Equal(t, data, res.String()) } @@ -514,7 +514,7 @@ func BenchmarkEmojiPostprocess(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { var res strings.Builder - err := markup.PostProcess(markup.NewTestRenderContext(localMetas), strings.NewReader(data), &res) + err := markup.PostProcessDefault(markup.NewTestRenderContext(localMetas), strings.NewReader(data), &res) assert.NoError(b, err) } } @@ -522,7 +522,7 @@ func BenchmarkEmojiPostprocess(b *testing.B) { func TestFuzz(t *testing.T) { s := "t/l/issues/8#/../../a" renderContext := markup.NewTestRenderContext() - err := markup.PostProcess(renderContext, strings.NewReader(s), io.Discard) + err := markup.PostProcessDefault(renderContext, strings.NewReader(s), io.Discard) assert.NoError(t, err) } @@ -530,7 +530,7 @@ func TestIssue18471(t *testing.T) { data := `http://domain/org/repo/compare/783b039...da951ce` var res strings.Builder - err := markup.PostProcess(markup.NewTestRenderContext(localMetas), strings.NewReader(data), &res) + err := markup.PostProcessDefault(markup.NewTestRenderContext(localMetas), strings.NewReader(data), &res) assert.NoError(t, err) assert.Equal(t, `783b039...da951ce`, res.String()) diff --git a/modules/markup/markdown/goldmark.go b/modules/markup/markdown/goldmark.go index ed95cecf8b..620a39ebfd 100644 --- a/modules/markup/markdown/goldmark.go +++ b/modules/markup/markdown/goldmark.go @@ -80,9 +80,7 @@ func (g *ASTTransformer) Transform(node *ast.Document, reader text.Reader, pc pa // many places render non-comment contents with no mode=document, then these contents also use comment's hard line break setting // especially in many tests. markdownLineBreakStyle := ctx.RenderOptions.Metas["markdownLineBreakStyle"] - if markup.RenderBehaviorForTesting.ForceHardLineBreak { - v.SetHardLineBreak(true) - } else if markdownLineBreakStyle == "comment" { + if markdownLineBreakStyle == "comment" { v.SetHardLineBreak(setting.Markdown.EnableHardLineBreakInComments) } else if markdownLineBreakStyle == "document" { v.SetHardLineBreak(setting.Markdown.EnableHardLineBreakInDocuments) diff --git a/modules/markup/markdown/markdown_test.go b/modules/markup/markdown/markdown_test.go index aac5ccbb1f..22ab39ebfa 100644 --- a/modules/markup/markdown/markdown_test.go +++ b/modules/markup/markdown/markdown_test.go @@ -85,8 +85,89 @@ func TestRender_Images(t *testing.T) { `

`+title+`

`) } -func testAnswers(baseURL string) []string { - return []string{ +func TestTotal_RenderString(t *testing.T) { + defer test.MockVariableValue(&markup.RenderBehaviorForTesting.DisableAdditionalAttributes, true)() + + // Test cases without ambiguous links (It is not right to copy a whole file here, instead it should clearly test what is being tested) + sameCases := []string{ + // dear imgui wiki markdown extract: special wiki syntax + `Wiki! Enjoy :) +- [[Links, Language bindings, Engine bindings|Links]] +- [[Tips]] + +See commit 65f1bf27bc + +Ideas and codes + +- Bezier widget (by @r-lyeh) ` + AppURL + `ocornut/imgui/issues/786 +- Bezier widget (by @r-lyeh) ` + FullURL + `issues/786 +- Node graph editors https://github.com/ocornut/imgui/issues/306 +- [[Memory Editor|memory_editor_example]] +- [[Plot var helper|plot_var_example]]`, + // wine-staging wiki home extract: tables, special wiki syntax, images + `## What is Wine Staging? +**Wine Staging** on website [wine-staging.com](http://wine-staging.com). + +## Quick Links +Here are some links to the most important topics. You can find the full list of pages at the sidebar. + +| [[images/icon-install.png]] | [[Installation]] | +|--------------------------------|----------------------------------------------------------| +| [[images/icon-usage.png]] | [[Usage]] | +`, + // libgdx wiki page: inline images with special syntax + `[Excelsior JET](http://www.excelsiorjet.com/) allows you to create native executables for Windows, Linux and Mac OS X. + +1. [Package your libGDX application](https://github.com/libgdx/libgdx/wiki/Gradle-on-the-Commandline#packaging-for-the-desktop) +[[images/1.png]] +2. Perform a test run by hitting the Run! button. +[[images/2.png]] + +## More tests {#custom-id} + +(from https://www.markdownguide.org/extended-syntax/) + +### Checkboxes + +- [ ] unchecked +- [x] checked +- [ ] still unchecked + +### Definition list + +First Term +: This is the definition of the first term. + +Second Term +: This is one definition of the second term. +: This is another definition of the second term. + +### Footnotes + +Here is a simple footnote,[^1] and here is a longer one.[^bignote] + +[^1]: This is the first footnote. + +[^bignote]: Here is one with multiple paragraphs and code. + + Indent paragraphs to include them in the footnote. + + ` + "`{ my code }`" + ` + + Add as many paragraphs as you like. +`, + ` +- [ ] If you want to rebase/retry this PR, click this checkbox. + +--- + +This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). + +`, + } + + baseURL := "" + testAnswers := []string{ `

Wiki! Enjoy :)

  • Links, Language bindings, Engine bindings
    • @@ -123,9 +204,9 @@ func testAnswers(baseURL string) []string { `, `

    Excelsior JET allows you to create native executables for Windows, Linux and Mac OS X.

      -
    1. Package your libGDX application
      +
    2. Package your libGDX application images/1.png
      3. -
    3. Perform a test run by hitting the Run! button.
      +
    4. Perform a test run by hitting the Run! button. images/2.png

    More tests

    @@ -160,106 +241,24 @@ func testAnswers(baseURL string) []string { -`, `
      +`, + `
      • If you want to rebase/retry this PR, click this checkbox.

      This PR has been generated by Renovate Bot.

      `, } -} - -// Test cases without ambiguous links -var sameCases = []string{ - // dear imgui wiki markdown extract: special wiki syntax - `Wiki! Enjoy :) -- [[Links, Language bindings, Engine bindings|Links]] -- [[Tips]] -See commit 65f1bf27bc - -Ideas and codes - -- Bezier widget (by @r-lyeh) ` + AppURL + `ocornut/imgui/issues/786 -- Bezier widget (by @r-lyeh) ` + FullURL + `issues/786 -- Node graph editors https://github.com/ocornut/imgui/issues/306 -- [[Memory Editor|memory_editor_example]] -- [[Plot var helper|plot_var_example]]`, - // wine-staging wiki home extract: tables, special wiki syntax, images - `## What is Wine Staging? -**Wine Staging** on website [wine-staging.com](http://wine-staging.com). - -## Quick Links -Here are some links to the most important topics. You can find the full list of pages at the sidebar. - -| [[images/icon-install.png]] | [[Installation]] | -|--------------------------------|----------------------------------------------------------| -| [[images/icon-usage.png]] | [[Usage]] | -`, - // libgdx wiki page: inline images with special syntax - `[Excelsior JET](http://www.excelsiorjet.com/) allows you to create native executables for Windows, Linux and Mac OS X. - -1. [Package your libGDX application](https://github.com/libgdx/libgdx/wiki/Gradle-on-the-Commandline#packaging-for-the-desktop) -[[images/1.png]] -2. Perform a test run by hitting the Run! button. -[[images/2.png]] - -## More tests {#custom-id} - -(from https://www.markdownguide.org/extended-syntax/) - -### Checkboxes - -- [ ] unchecked -- [x] checked -- [ ] still unchecked - -### Definition list - -First Term -: This is the definition of the first term. - -Second Term -: This is one definition of the second term. -: This is another definition of the second term. - -### Footnotes - -Here is a simple footnote,[^1] and here is a longer one.[^bignote] - -[^1]: This is the first footnote. - -[^bignote]: Here is one with multiple paragraphs and code. - - Indent paragraphs to include them in the footnote. - - ` + "`{ my code }`" + ` - - Add as many paragraphs as you like. -`, - ` -- [ ] If you want to rebase/retry this PR, click this checkbox. - ---- - -This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). - -`, -} - -func TestTotal_RenderString(t *testing.T) { - defer test.MockVariableValue(&markup.RenderBehaviorForTesting.ForceHardLineBreak, true)() - defer test.MockVariableValue(&markup.RenderBehaviorForTesting.DisableAdditionalAttributes, true)() markup.Init(&markup.RenderHelperFuncs{ IsUsernameMentionable: func(ctx context.Context, username string) bool { return username == "r-lyeh" }, }) - answers := testAnswers("") for i := 0; i < len(sameCases); i++ { line, err := markdown.RenderString(markup.NewTestRenderContext(localMetas), sameCases[i]) assert.NoError(t, err) - assert.Equal(t, answers[i], string(line)) + assert.Equal(t, testAnswers[i], string(line)) } } @@ -312,10 +311,9 @@ func TestRenderSiblingImages_Issue12925(t *testing.T) { testcase := `![image1](/image1) ![image2](/image2) ` - expected := `

      image1
      + expected := `

      image1 image2

      ` - defer test.MockVariableValue(&markup.RenderBehaviorForTesting.ForceHardLineBreak, true)() res, err := markdown.RenderRawString(markup.NewTestRenderContext(), testcase) assert.NoError(t, err) assert.Equal(t, expected, res) @@ -525,43 +523,33 @@ mail@domain.com space${SPACE}${SPACE} ` input = strings.ReplaceAll(input, "${SPACE}", " ") // replace ${SPACE} with " ", to avoid some editor's auto-trimming - cases := []struct { - Expected string - }{ - { - Expected: `

      space @mention-user
      -/just/a/path.bin
      -https://example.com/file.bin
      -local link
      -remote link
      -local link
      -remote link
      -local image
      -local image
      -local image
      -remote image
      -local image
      -remote link
      -88fc37a3c0...12fc37a3c0 (hash)
      -com 88fc37a3c0a4dda553bdcfc80c178a58247f42fb...12fc37a3c0a4dda553bdcfc80c178a58247f42fb pare
      -88fc37a3c0
      -com 88fc37a3c0a4dda553bdcfc80c178a58247f42fb mit
      -👍
      -mail@domain.com
      -@mention-user test
      -#123
      + expected := `

      space @mention-user
      +/just/a/path.bin +https://example.com/file.bin +local link +remote link +local link +remote link +local image +local image +local image +remote image +local image +remote link +88fc37a3c0...12fc37a3c0 (hash) +com 88fc37a3c0a4dda553bdcfc80c178a58247f42fb...12fc37a3c0a4dda553bdcfc80c178a58247f42fb pare +88fc37a3c0 +com 88fc37a3c0a4dda553bdcfc80c178a58247f42fb mit +👍 +mail@domain.com +@mention-user test +#123 space

      -`, - }, - } - - defer test.MockVariableValue(&markup.RenderBehaviorForTesting.ForceHardLineBreak, true)() +` defer test.MockVariableValue(&markup.RenderBehaviorForTesting.DisableAdditionalAttributes, true)() - for i, c := range cases { - result, err := markdown.RenderString(markup.NewTestRenderContext(localMetas), input) - assert.NoError(t, err, "Unexpected error in testcase: %v", i) - assert.Equal(t, c.Expected, string(result), "Unexpected result in testcase %v", i) - } + result, err := markdown.RenderString(markup.NewTestRenderContext(localMetas), input) + assert.NoError(t, err) + assert.Equal(t, expected, string(result)) } func TestAttention(t *testing.T) { diff --git a/modules/markup/render.go b/modules/markup/render.go index be75d08c8c..3b112b1a14 100644 --- a/modules/markup/render.go +++ b/modules/markup/render.go @@ -28,14 +28,6 @@ const ( ) var RenderBehaviorForTesting struct { - // Markdown line break rendering has 2 default behaviors: - // * Use hard: replace "\n" with "
      " for comments, setting.Markdown.EnableHardLineBreakInComments=true - // * Keep soft: "\n" for non-comments (a.k.a. documents), setting.Markdown.EnableHardLineBreakInDocuments=false - // In history, there was a mess: - // * The behavior was controlled by `Metas["mode"] != "document", - // * However, many places render the content without setting "mode" in Metas, all these places used comment line break setting incorrectly - ForceHardLineBreak bool - // Gitea will emit some additional attributes for various purposes, these attributes don't affect rendering. // But there are too many hard-coded test cases, to avoid changing all of them again and again, we can disable emitting these internal attributes. DisableAdditionalAttributes bool @@ -218,7 +210,7 @@ func render(ctx *RenderContext, renderer Renderer, input io.Reader, output io.Wr eg.Go(func() (err error) { if r, ok := renderer.(PostProcessRenderer); ok && r.NeedPostProcess() { - err = PostProcess(ctx, pr1, pw2) + err = PostProcessDefault(ctx, pr1, pw2) } else { _, err = io.Copy(pw2, pr1) } diff --git a/tests/fuzz/fuzz_test.go b/tests/fuzz/fuzz_test.go index 946f7c46f1..01d562d995 100644 --- a/tests/fuzz/fuzz_test.go +++ b/tests/fuzz/fuzz_test.go @@ -27,6 +27,6 @@ func FuzzMarkdownRenderRaw(f *testing.F) { func FuzzMarkupPostProcess(f *testing.F) { f.Fuzz(func(t *testing.T, data []byte) { setting.AppURL = "http://localhost:3000/" - markup.PostProcess(newFuzzRenderContext(), bytes.NewReader(data), io.Discard) + markup.PostProcessDefault(newFuzzRenderContext(), bytes.NewReader(data), io.Discard) }) } -- cgit v1.2.3