8.7.0 RC1 seemingly broke Markdown rendering

mentioned in issue gitlab-com/operations#42 (closed)

Confirmed this was introduced by https://gitlab.com/gitlab-org/gitlab-ce/commit/141148057703048f5c409c040c80c277f7747273, checking into it a bit more.

Markdown headers get auto-generated anchor links from TableOfContentsFilter. For our monthly release post, one of those anchors might look like this: #31st-16-working-days-before-the-22nd.

Here in AbstractReferenceFilter, we're checking if link =~ /\A#{ref_pattern}/, which is truthy in these cases.

@yorickpeterse Can the fix really be this simple?

diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb
index f21dbef..007113a 100644
--- a/lib/banzai/filter/abstract_reference_filter.rb
+++ b/lib/banzai/filter/abstract_reference_filter.rb
@@ -119,7 +119,7 @@ module Banzai
 
           elsif element_node?(node)
             yield_valid_link(node) do |link, text|
-              if ref_pattern && link =~ /\A#{ref_pattern}/
+              if ref_pattern && text =~ /\A#{ref_pattern}/
                 replace_link_node_with_href(node, link) do
                   object_link_filter(link, ref_pattern, link_text: text)
                 end

Will leave to you to verify.

Reassigned to @yorickpeterse

It appears this problem has existed in some shape or form before. For example, this script:

Rails.cache.clear

filters = [
  Banzai::Filter::MarkdownFilter,
  Banzai::Filter::TableOfContentsFilter,
  Banzai::Filter::AutolinkFilter,
  Banzai::Filter::IssueReferenceFilter
]

project  = Project.find_with_namespace('gitlab-org/gitlab-ce')
pipeline = HTML::Pipeline.new(filters, project: project)
input    = "### #{project.issues.first.iid}"
document = pipeline.to_document(input)

puts 'Partial pipeline:'
puts

puts document

puts
puts 'Full pipeline:'
puts
puts Banzai::Renderer.cacheless_render(input, project: project)

Spits out (both when using 8-6-stable and master):

Partial pipeline:

<h3>
<a href="http://localhost:3000/gitlab-org/gitlab-ce/issues/2875" data-original="" data-project="13083" data-issue="566948" data-reference-filter="IssueReferenceFilter" title="Issue: Branch labels incorrect in me
rge request creation form w/ 8.0.2" class="gfm gfm-issue"></a>2875</h3>

Full pipeline:

<h3>
<a href="/gitlab-org/gitlab-ce/issues/2875" data-original="" data-project="13083" data-issue="566948" data-reference-filter="IssueReferenceFilter" title="Issue: Branch labels incorrect in merge request creation 
form w/ 8.0.2" class="gfm gfm-issue"></a>2875</h3>

However, the particular problem with 31st and such appears to indeed be caused by my refactoring. If we change the script to the following:

Rails.cache.clear

filters = [
  Banzai::Filter::MarkdownFilter,
  Banzai::Filter::TableOfContentsFilter,
  Banzai::Filter::AutolinkFilter,
  Banzai::Filter::IssueReferenceFilter
]

project  = Project.find_with_namespace('gitlab-org/gitlab-ce')
pipeline = HTML::Pipeline.new(filters, project: project)
input    = "### #{project.issues.first.iid}st"
document = pipeline.to_document(input)

puts 'Partial pipeline:'
puts

puts document

puts
puts 'Full pipeline:'
puts
puts Banzai::Renderer.cacheless_render(input, project: project)

We get this output on 8.6:

Partial pipeline:

<h3>
<a id="2875st" class="anchor" href="#2875st" aria-hidden="true"></a>2875st</h3>

Full pipeline:

<h3>
<a id="2875st" class="anchor" href="#2875st" aria-hidden="true"></a>2875st</h3>

But on the current master branch we get this instead:

Partial pipeline:

<h3>
<a href="http://localhost:3000/gitlab-org/gitlab-ce/issues/2875" data-original="" data-project="13083" data-issue="566948" data-reference-filter="IssueReferenceFilter" title="Issue: Branch labels incorrect in merge request creation form w/ 8.0.2" class="gfm gfm-issue"></a>st2875st</h3>

Full pipeline:

<h3>
<a href="/gitlab-org/gitlab-ce/issues/2875" data-original="" data-project="13083" data-issue="566948" data-reference-filter="IssueReferenceFilter" title="Issue: Branch labels incorrect in merge request creation form w/ 8.0.2" class="gfm gfm-issue"></a>st2875st</h3>

The suggestion of @rspeicher does solve this particular case but breaks a bunch of other tests. Looking back at the old code this also isn't correct as the old code used to compare the link URL and not the link text.

OK so there are two problems:

### 1st gets turned into a link pointing to issue 1, this can be fixed by using if ref_pattern && link =~ /\A#{ref_pattern}\z/ which is actually the old regex (I forgot to port over the \z part)
### 1 gets turned into an issue link, this seems to have always been the case

@rspeicher @DouweM Is it correct that # 1 gets turned into an issue link? I'd expect this to only happen when using #1.

mentioned in commit 507cbca3

mentioned in merge request !3568 (merged)

FYI I reported this in #14930 (closed) (I will close it since you started digging the issue here).

mentioned in issue #14939 (closed)

Added regression label

mentioned in issue #14930 (closed)

Is it correct that # 1 gets turned into an issue link? I'd expect this to only happen when using #1.

@yorickpeterse No, it should never match on the version with a space.

I'm surprised you're having trouble recreating it in a test. Make sure that:

You pass through both MarkdownFilter to convert the headers, then TableOfContentsFilter to add anchor links to the headers
Whatever number you put in the header (e.g., ### 31st) needs to have an issue with that same iid (i.e., 31).

mentioned in commit fe132f52

The initial problem got fixed in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3568, next I'll take a look at # 1 getting turned into issue references.

The problem with # 1 getting turned into a reference link is as follows:

# 1 gets turned into a header for the TOC, something like <h1> <a id="1" class="anchor" href="#1" aria-hidden="true"></a>1</h1>
The IssueReferenceFilter processes this link and runs if ref_pattern && link =~ /\A#{ref_pattern}\z/ (AbstractReferenceFilter line 122) which returns 0
The following code processes this as if it were an issue reference

I'm not entirely sure just yet what the best way of fixing this would be. @rspeicher @DouweM: any thoughts?

To reproduce this problem I wrote the following test:

diff --git a/spec/lib/banzai/filter/issue_reference_filter_spec.rb b/spec/lib/banzai/filter/issue_reference_filter_spec.rb
index 266ebef..3ef77e8 100644
--- a/spec/lib/banzai/filter/issue_reference_filter_spec.rb
+++ b/spec/lib/banzai/filter/issue_reference_filter_spec.rb
@@ -103,6 +103,13 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
 
       expect(link).to eq(href)
     end
+
+    it 'does not process links pointing to issue references' do
+      input = "# #{issue.iid}"
+      doc = reference_filter("<a href='#{reference}'>#{issue.iid}</a>")
+
+      expect(document.to_html).to eq(input)
+    end
   end
 
   context 'cross-project reference' do

If trying to find references in link hrefs causes issues, ww can remove the functionality, I think. See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3568#note_4633504

The use of that feature was pretty far fetched anyway.

@rspeicher @DouweM (and cc-ing @dzaporozhets so he can also give his opinion): so what's the final verdict, do we keep the feature (leading to the somewhat odd linking behaviour described) or do we ditch it entirely?

@yorickpeterse ditch it and fix # 1. It should not be a link but h1

@yorickpeterse Fine with removing.

Status changed to closed

Fixed by !3568 (merged).

@rymai The issue mentioned above has not yet been fixed so I'll re-open this issue for the time being.

Status changed to reopened

@yorickpeterse Sorry about that! Are you referring to this issue?:

### 1 gets turned into an issue link, this seems to have always been the case

If yes, I see it fixed, examples: https://gitlab.com/gitlab-org/gitlab-ce/issues/14935 and https://gitlab.com/gitlab-org/gitlab-ce/blob/v8.6.4/doc/update/8.5-to-8.6.md

Btw, the issue was not here before the first 8.7 RC (as reported in https://gitlab.com/gitlab-org/gitlab-ce/issues/14930).

@rymai The problem only occurs when there's nothing following the number. For example, this:

### 1

Gets turned into the following on GitLab.com:

<h3>
<a href="/gitlab-org/gitlab-ce/issues/1" data-original="" data-project="13083" data-issue="12379" data-reference-filter="IssueReferenceFilter" title="Issue: GitLab is only useable in English" class="gfm gfm-issue"></a>1</h3>

Here's an example (use Chrome's inspector to see the actual HTML):