Fix review comments

including refactoring, disabling sourcepos for pipelines that
don't need it, and minimizing spec changes by disabling
sourcepos when not testing for it explicitly.

16 files changed, 65 insertions, 32 deletions

diff --git a/lib/banzai/filter/markdown_engines/common_mark.rb b/lib/banzai/filter/markdown_engines/common_mark.rb
index 81fdf0b2652..d3af776db05 100644
--- a/lib/banzai/filter/markdown_engines/common_mark.rb
+++ b/lib/banzai/filter/markdown_engines/common_mark.rb
@@ -50,7 +50,7 @@ module Banzai
         private
 
         def render_options
-          @context&.dig(:no_sourcepos) ? RENDER_OPTIONS : RENDER_OPTIONS_SOURCEPOS
+          @context[:no_sourcepos] ? RENDER_OPTIONS : RENDER_OPTIONS_SOURCEPOS
         end
       end
     end
diff --git a/lib/banzai/filter/spaced_link_filter.rb b/lib/banzai/filter/spaced_link_filter.rb
index de76e3f9df4..00dbf2d3130 100644
--- a/lib/banzai/filter/spaced_link_filter.rb
+++ b/lib/banzai/filter/spaced_link_filter.rb
@@ -73,7 +73,8 @@ module Banzai
         html = Banzai::Filter::MarkdownFilter.call(transform_markdown(match), context)
 
         # link is wrapped in a <p>, so strip that off
-        html.sub(/<p[^>]*>/, '').chomp('</p>')
+        p_node = Nokogiri::HTML.fragment(html).at_css('p')
+        p_node ? p_node.children.to_html : html
       end
 
       def spaced_link_filter(text)
diff --git a/lib/banzai/pipeline/broadcast_message_pipeline.rb b/lib/banzai/pipeline/broadcast_message_pipeline.rb
index a3d63e0aaf5..580b5b72474 100644
--- a/lib/banzai/pipeline/broadcast_message_pipeline.rb
+++ b/lib/banzai/pipeline/broadcast_message_pipeline.rb
@@ -14,6 +14,12 @@ module Banzai
           Filter::ExternalLinkFilter
         ]
       end
+
+      def self.transform_context(context)
+        super(context).merge(
+          no_sourcepos: true
+        )
+      end
     end
   end
 end
diff --git a/lib/banzai/pipeline/email_pipeline.rb b/lib/banzai/pipeline/email_pipeline.rb
index 2c08581ce0d..0f4dd9d143d 100644
--- a/lib/banzai/pipeline/email_pipeline.rb
+++ b/lib/banzai/pipeline/email_pipeline.rb
@@ -11,7 +11,8 @@ module Banzai
 
       def self.transform_context(context)
         super(context).merge(
-          only_path: false
+          only_path: false,
+          no_sourcepos: true
         )
       end
     end
diff --git a/lib/banzai/pipeline/single_line_pipeline.rb b/lib/banzai/pipeline/single_line_pipeline.rb
index 61ff7b0bcce..72374207a8f 100644
--- a/lib/banzai/pipeline/single_line_pipeline.rb
+++ b/lib/banzai/pipeline/single_line_pipeline.rb
@@ -27,6 +27,12 @@ module Banzai
           Filter::CommitReferenceFilter
         ]
       end
+
+      def self.transform_context(context)
+        super(context).merge(
+          no_sourcepos: true
+        )
+      end
     end
   end
 end
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 963d3029830..a0fb3641899 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -173,6 +173,7 @@ describe IssuablesHelper do
     before do
       allow(helper).to receive(:current_user).and_return(user)
       allow(helper).to receive(:can?).and_return(true)
+      stub_commonmark_sourcepos_disabled
     end
 
     it 'returns the correct json for an issue' do
@@ -193,7 +194,7 @@ describe IssuablesHelper do
         projectNamespace: @project.namespace.path,
         initialTitleHtml: issue.title,
         initialTitleText: issue.title,
-        initialDescriptionHtml: '<p data-sourcepos="1:1-1:10" dir="auto">issue text</p>',
+        initialDescriptionHtml: '<p dir="auto">issue text</p>',
         initialDescriptionText: 'issue text',
         initialTaskStatus: '0 of 0 tasks completed'
       }
diff --git a/spec/lib/banzai/filter/markdown_filter_spec.rb b/spec/lib/banzai/filter/markdown_filter_spec.rb
index 62f68b5f0cb..4c4e821deab 100644
--- a/spec/lib/banzai/filter/markdown_filter_spec.rb
+++ b/spec/lib/banzai/filter/markdown_filter_spec.rb
@@ -30,21 +30,21 @@ describe Banzai::Filter::MarkdownFilter do
       end
 
       it 'adds language to lang attribute when specified' do
-        result = filter("```html\nsome code\n```")
+        result = filter("```html\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with('<pre data-sourcepos="1:1-3:3"><code lang="html">')
+        expect(result).to start_with('<pre><code lang="html">')
       end
 
       it 'does not add language to lang attribute when not specified' do
-        result = filter("```\nsome code\n```")
+        result = filter("```\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with('<pre data-sourcepos="1:1-3:3"><code>')
+        expect(result).to start_with('<pre><code>')
       end
 
       it 'works with utf8 chars in language' do
-        result = filter("```日\nsome code\n```")
+        result = filter("```日\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with('<pre data-sourcepos="1:1-3:3"><code lang="日">')
+        expect(result).to start_with('<pre><code lang="日">')
       end
     end
 
@@ -80,7 +80,7 @@ describe Banzai::Filter::MarkdownFilter do
       end
 
       it 'disables data-sourcepos' do
-        result = filter('test', { no_sourcepos: true })
+        result = filter('test', no_sourcepos: true)
 
         expect(result).to eq '<p>test</p>'
       end
@@ -109,9 +109,9 @@ describe Banzai::Filter::MarkdownFilter do
       [^1]: a footnote
       MD
 
-      result = filter(text)
+      result = filter(text, no_sourcepos: true)
 
-      expect(result).to include('<td data-sourcepos="3:2-3:12">foot <sup')
+      expect(result).to include('<td>foot <sup')
       expect(result).to include('<section class="footnotes">')
     end
   end
diff --git a/spec/lib/banzai/pipeline/description_pipeline_spec.rb b/spec/lib/banzai/pipeline/description_pipeline_spec.rb
index 1f9d53c6345..77cb1954ea3 100644
--- a/spec/lib/banzai/pipeline/description_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/description_pipeline_spec.rb
@@ -8,11 +8,15 @@ describe Banzai::Pipeline::DescriptionPipeline do
 
     output = described_class.to_html(html, project: spy)
 
-    output.gsub!(%r{\A<p #{MarkdownFeature::SOURCEPOS_REGEX} dir="auto">(.*)</p>(.*)\z}, '\1\2') if unwrap
+    output.gsub!(%r{\A<p dir="auto">(.*)</p>(.*)\z}, '\1\2') if unwrap
 
     output
   end
 
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   it 'uses a limited whitelist' do
     doc = parse('# Description')
 
diff --git a/spec/lib/banzai/pipeline/full_pipeline_spec.rb b/spec/lib/banzai/pipeline/full_pipeline_spec.rb
index 3634655c6a5..aa503b6e1d5 100644
--- a/spec/lib/banzai/pipeline/full_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/full_pipeline_spec.rb
@@ -54,6 +54,8 @@ describe Banzai::Pipeline::FullPipeline do
     end
 
     it 'properly adds the necessary ids and classes' do
+      stub_commonmark_sourcepos_disabled
+
       expect(html.lines.map(&:strip).join("\n")).to eq filtered_footnote
     end
   end
diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb
index da5f184af06..ef6af232999 100644
--- a/spec/models/concerns/cache_markdown_field_spec.rb
+++ b/spec/models/concerns/cache_markdown_field_spec.rb
@@ -67,14 +67,17 @@ describe CacheMarkdownField do
   end
 
   let(:markdown) { '`Foo`' }
-  let(:html)     { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Foo</code></p>' }
+  let(:html)     { '<p dir="auto"><code>Foo</code></p>' }
 
   let(:updated_markdown) { '`Bar`' }
-  let(:updated_html)     { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Bar</code></p>' }
-  let(:updated_redcarpet_html) { '<p dir="auto"><code>Bar</code></p>' }
+  let(:updated_html)     { '<p dir="auto"><code>Bar</code></p>' }
 
   let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
 
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   describe '.attributes' do
     it 'excludes cache attributes' do
       expect(thing.attributes.keys.sort).to eq(%w[bar baz foo])
@@ -96,16 +99,13 @@ describe CacheMarkdownField do
   context 'a changed markdown field' do
     shared_examples 'with cache version' do |cache_version|
       let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
-      let(:updated_version_html) do
-        cache_version == CacheMarkdownField::CACHE_REDCARPET_VERSION ? updated_redcarpet_html : updated_html
-      end
 
       before do
         thing.foo = updated_markdown
         thing.save
       end
 
-      it { expect(thing.foo_html).to eq(updated_version_html) }
+      it { expect(thing.foo_html).to eq(updated_html) }
       it { expect(thing.cached_markdown_version).to eq(cache_version) }
     end
 
@@ -272,9 +272,6 @@ describe CacheMarkdownField do
   describe '#refresh_markdown_cache!' do
     shared_examples 'with cache version' do |cache_version|
       let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
-      let(:updated_version_html) do
-        cache_version == CacheMarkdownField::CACHE_REDCARPET_VERSION ? updated_redcarpet_html : updated_html
-      end
 
       before do
         thing.foo = updated_markdown
@@ -283,7 +280,7 @@ describe CacheMarkdownField do
       it 'fills all html fields' do
         thing.refresh_markdown_cache!
 
-        expect(thing.foo_html).to eq(updated_version_html)
+        expect(thing.foo_html).to eq(updated_html)
         expect(thing.foo_html_changed?).to be_truthy
         expect(thing.baz_html_changed?).to be_truthy
       end
@@ -298,7 +295,7 @@ describe CacheMarkdownField do
       it 'saves the changes using #update_columns' do
         expect(thing).to receive(:persisted?).and_return(true)
         expect(thing).to receive(:update_columns)
-          .with("foo_html" => updated_version_html, "baz_html" => "", "cached_markdown_version" => cache_version)
+          .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => cache_version)
 
         thing.refresh_markdown_cache!
       end
diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb
index 9bc381694d6..43a544cfe26 100644
--- a/spec/models/concerns/cacheable_attributes_spec.rb
+++ b/spec/models/concerns/cacheable_attributes_spec.rb
@@ -159,6 +159,10 @@ describe CacheableAttributes do
 
     describe 'edge cases' do
       describe 'caching behavior', :use_clean_rails_memory_store_caching do
+        before do
+          stub_commonmark_sourcepos_disabled
+        end
+
         it 'retrieves upload fields properly' do
           ar_record = create(:appearance, :with_logo)
           ar_record.cache!
@@ -177,7 +181,7 @@ describe CacheableAttributes do
           cache_record = Appearance.current
 
           expect(cache_record.description).to eq('**Hello**')
-          expect(cache_record.description_html).to eq('<p data-sourcepos="1:1-1:9" dir="auto"><strong>Hello</strong></p>')
+          expect(cache_record.description_html).to eq('<p dir="auto"><strong>Hello</strong></p>')
         end
       end
     end
diff --git a/spec/models/concerns/redactable_spec.rb b/spec/models/concerns/redactable_spec.rb
index fe17970d75f..7feeaa54069 100644
--- a/spec/models/concerns/redactable_spec.rb
+++ b/spec/models/concerns/redactable_spec.rb
@@ -1,6 +1,10 @@
 require 'spec_helper'
 
 describe Redactable do
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   shared_examples 'model with redactable field' do
     it 'redacts unsubscribe token' do
       model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
@@ -35,7 +39,7 @@ describe Redactable do
 
       expected = 'some text /sent_notifications/REDACTED/unsubscribe more text'
       expect(model[field]).to eq expected
-      expect(model["#{field}_html"]).to eq "<p data-sourcepos=\"1:1-1:60\" dir=\"auto\">#{expected}</p>"
+      expect(model["#{field}_html"]).to eq "<p dir=\"auto\">#{expected}</p>"
     end
   end
 
diff --git a/spec/requests/api/markdown_spec.rb b/spec/requests/api/markdown_spec.rb
index 386679d9dfd..0cf5c5677b9 100644
--- a/spec/requests/api/markdown_spec.rb
+++ b/spec/requests/api/markdown_spec.rb
@@ -7,6 +7,8 @@ describe API::Markdown do
     let(:user) {} # No-op. It gets overwritten in the contexts below.
 
     before do
+      stub_commonmark_sourcepos_disabled
+
       post api("/markdown", user), params: params
     end
 
@@ -15,7 +17,7 @@ describe API::Markdown do
         expect(response).to have_http_status(201)
         expect(response.headers["Content-Type"]).to eq("application/json")
         expect(json_response).to be_a(Hash)
-        expect(json_response["html"]).to eq("<p data-sourcepos=\"1:1-1:28\">#{text}</p>")
+        expect(json_response["html"]).to eq("<p>#{text}</p>")
       end
     end
 
diff --git a/spec/serializers/group_child_entity_spec.rb b/spec/serializers/group_child_entity_spec.rb
index 7f2ddc9546a..d02b4c554b1 100644
--- a/spec/serializers/group_child_entity_spec.rb
+++ b/spec/serializers/group_child_entity_spec.rb
@@ -10,6 +10,7 @@ describe GroupChildEntity do
 
   before do
     allow(request).to receive(:current_user).and_return(user)
+    stub_commonmark_sourcepos_disabled
   end
 
   shared_examples 'group child json' do
@@ -102,7 +103,7 @@ describe GroupChildEntity do
       let(:description) { ':smile:' }
 
       it 'has the correct markdown_description' do
-        expect(json[:markdown_description]).to eq('<p data-sourcepos="1:1-1:7" dir="auto"><gl-emoji title="smiling face with open mouth and smiling eyes" data-name="smile" data-unicode-version="6.0">😄</gl-emoji></p>')
+        expect(json[:markdown_description]).to eq('<p dir="auto"><gl-emoji title="smiling face with open mouth and smiling eyes" data-name="smile" data-unicode-version="6.0">😄</gl-emoji></p>')
       end
     end
 
diff --git a/spec/support/helpers/markdown_feature.rb b/spec/support/helpers/markdown_feature.rb
index e60e5ee6783..96401379cf0 100644
--- a/spec/support/helpers/markdown_feature.rb
+++ b/spec/support/helpers/markdown_feature.rb
@@ -10,8 +10,6 @@
 class MarkdownFeature
   include FactoryBot::Syntax::Methods
 
-  SOURCEPOS_REGEX = 'data-sourcepos="\d*:\d*-\d*:\d*"'.freeze
-
   attr_reader :fixture_path
 
   def initialize(fixture_path = Rails.root.join('spec/fixtures/markdown.md.erb'))
diff --git a/spec/support/helpers/stub_gitlab_calls.rb b/spec/support/helpers/stub_gitlab_calls.rb
index 2933f2c78dc..f2f128cc8dc 100644
--- a/spec/support/helpers/stub_gitlab_calls.rb
+++ b/spec/support/helpers/stub_gitlab_calls.rb
@@ -52,6 +52,12 @@ module StubGitlabCalls
       .and_return(stub_container_registry_blob)
   end
 
+  def stub_commonmark_sourcepos_disabled
+    allow_any_instance_of(Banzai::Filter::MarkdownEngines::CommonMark)
+      .to receive(:render_options)
+      .and_return(Banzai::Filter::MarkdownEngines::CommonMark::RENDER_OPTIONS)
+  end
+
   private
 
   def stub_container_registry_tag_manifest