diff options
author | Robert Speicher <rspeicher@gmail.com> | 2019-01-28 20:19:23 +0300 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2019-01-28 20:19:23 +0300 |
commit | a24551964180614bb9b19417763996043edba25f (patch) | |
tree | 543fd1d2b641ab19d2d5c72264c57efbfd737915 | |
parent | 4f8bdb01c7e5f02a0ae79cd2641465e45350faf6 (diff) | |
parent | 7bc0fbe22f0dd2c96b596b591ef5dbf3eaae8dd3 (diff) |
Merge branch 'bw-enable-sourcepos' into 'master'
Enable CommonMark source line position information
See merge request gitlab-org/gitlab-ce!23971
21 files changed, 113 insertions, 17 deletions
diff --git a/lib/banzai/filter/markdown_engines/common_mark.rb b/lib/banzai/filter/markdown_engines/common_mark.rb index e52c0d15b31..d3af776db05 100644 --- a/lib/banzai/filter/markdown_engines/common_mark.rb +++ b/lib/banzai/filter/markdown_engines/common_mark.rb @@ -32,8 +32,13 @@ module Banzai :DEFAULT # default rendering system. Nothing special. ].freeze - def initialize - @renderer = Banzai::Renderer::CommonMark::HTML.new(options: RENDER_OPTIONS) + RENDER_OPTIONS_SOURCEPOS = RENDER_OPTIONS + [ + :SOURCEPOS # enable embedding of source position information + ].freeze + + def initialize(context) + @context = context + @renderer = Banzai::Renderer::CommonMark::HTML.new(options: render_options) end def render(text) @@ -41,6 +46,12 @@ module Banzai @renderer.render(doc) end + + private + + def render_options + @context[:no_sourcepos] ? RENDER_OPTIONS : RENDER_OPTIONS_SOURCEPOS + end end end end diff --git a/lib/banzai/filter/markdown_engines/redcarpet.rb b/lib/banzai/filter/markdown_engines/redcarpet.rb index ec150d041ff..5b3f75096b1 100644 --- a/lib/banzai/filter/markdown_engines/redcarpet.rb +++ b/lib/banzai/filter/markdown_engines/redcarpet.rb @@ -20,7 +20,7 @@ module Banzai tables: true }.freeze - def initialize + def initialize(context = nil) html_renderer = Banzai::Renderer::Redcarpet::HTML.new @renderer = ::Redcarpet::Markdown.new(html_renderer, OPTIONS) end diff --git a/lib/banzai/filter/markdown_filter.rb b/lib/banzai/filter/markdown_filter.rb index cdf758472c1..242e39f5495 100644 --- a/lib/banzai/filter/markdown_filter.rb +++ b/lib/banzai/filter/markdown_filter.rb @@ -6,7 +6,7 @@ module Banzai def initialize(text, context = nil, result = nil) super(text, context, result) - @renderer = renderer(context[:markdown_engine]).new + @renderer = renderer(context[:markdown_engine]).new(context) @text = @text.delete("\r") end diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index edc053638a8..a4a06eae7b7 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -41,6 +41,9 @@ module Banzai whitelist[:elements].push('abbr') whitelist[:attributes]['abbr'] = %w(title) + # Allow the 'data-sourcepos' from CommonMark on all elements + whitelist[:attributes][:all].push('data-sourcepos') + # Disallow `name` attribute globally, allow on `a` whitelist[:attributes][:all].delete('name') whitelist[:attributes]['a'].push('name') diff --git a/lib/banzai/filter/spaced_link_filter.rb b/lib/banzai/filter/spaced_link_filter.rb index c6a3a763c23..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/atom_pipeline.rb b/lib/banzai/pipeline/atom_pipeline.rb index 13a342351b6..c632910585d 100644 --- a/lib/banzai/pipeline/atom_pipeline.rb +++ b/lib/banzai/pipeline/atom_pipeline.rb @@ -6,7 +6,8 @@ module Banzai def self.transform_context(context) super(context).merge( only_path: false, - xhtml: true + xhtml: true, + no_sourcepos: true ) end end 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/lib/gitlab/gfm/reference_rewriter.rb b/lib/gitlab/gfm/reference_rewriter.rb index 08d7db49ad7..4d82acd9d87 100644 --- a/lib/gitlab/gfm/reference_rewriter.rb +++ b/lib/gitlab/gfm/reference_rewriter.rb @@ -93,7 +93,7 @@ module Gitlab end def markdown(text) - Banzai.render(text, project: @source_parent, no_original_data: true) + Banzai.render(text, project: @source_parent, no_original_data: true, no_sourcepos: true) end end end diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb index 412ec910f3a..03e3a72a82f 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 diff --git a/spec/lib/banzai/filter/markdown_filter_spec.rb b/spec/lib/banzai/filter/markdown_filter_spec.rb index cf49249756a..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><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><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><code lang=\"日\">") + expect(result).to start_with('<pre><code lang="日">') end end @@ -67,6 +67,38 @@ describe Banzai::Filter::MarkdownFilter do end end + describe 'source line position' do + context 'using CommonMark' do + before do + stub_const('Banzai::Filter::MarkdownFilter::DEFAULT_ENGINE', :common_mark) + end + + it 'defaults to add data-sourcepos' do + result = filter('test') + + expect(result).to eq '<p data-sourcepos="1:1-1:4">test</p>' + end + + it 'disables data-sourcepos' do + result = filter('test', no_sourcepos: true) + + expect(result).to eq '<p>test</p>' + end + end + + context 'using Redcarpet' do + before do + stub_const('Banzai::Filter::MarkdownFilter::DEFAULT_ENGINE', :redcarpet) + end + + it 'does not support data-sourcepos' do + result = filter('test') + + expect(result).to eq '<p>test</p>' + end + end + end + describe 'footnotes in tables' do it 'processes footnotes in table cells' do text = <<-MD.strip_heredoc @@ -77,7 +109,7 @@ describe Banzai::Filter::MarkdownFilter do [^1]: a footnote MD - result = filter(text) + result = filter(text, no_sourcepos: true) expect(result).to include('<td>foot <sup') expect(result).to include('<section class="footnotes">') diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb index 836af18e0b6..f2a5d7b2c9f 100644 --- a/spec/lib/banzai/filter/sanitization_filter_spec.rb +++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb @@ -301,6 +301,13 @@ describe Banzai::Filter::SanitizationFilter do expect(act.to_html).to eq exp end + it 'allows the `data-sourcepos` attribute globally' do + exp = %q{<p data-sourcepos="1:1-1:10">foo/bar.md</p>} + act = filter(exp) + + expect(act.to_html).to eq exp + end + describe 'footnotes' do it 'allows correct footnote id property on links' do exp = %q{<a href="#fn1" id="fnref1">foo/bar.md</a>} diff --git a/spec/lib/banzai/pipeline/description_pipeline_spec.rb b/spec/lib/banzai/pipeline/description_pipeline_spec.rb index 8cce1b96698..77cb1954ea3 100644 --- a/spec/lib/banzai/pipeline/description_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/description_pipeline_spec.rb @@ -13,6 +13,10 @@ describe Banzai::Pipeline::DescriptionPipeline do 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 f8d50e89d40..ef6af232999 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -67,13 +67,17 @@ describe CacheMarkdownField do end let(:markdown) { '`Foo`' } - let(:html) { '<p dir="auto"><code>Foo</code></p>' } + let(:html) { '<p dir="auto"><code>Foo</code></p>' } let(:updated_markdown) { '`Bar`' } - let(:updated_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]) diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb index 689e7d3058f..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! diff --git a/spec/models/concerns/redactable_spec.rb b/spec/models/concerns/redactable_spec.rb index 7d320edd492..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' diff --git a/spec/requests/api/markdown_spec.rb b/spec/requests/api/markdown_spec.rb index e82ef002d32..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 diff --git a/spec/serializers/group_child_entity_spec.rb b/spec/serializers/group_child_entity_spec.rb index dbc40bddc30..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 diff --git a/spec/support/helpers/stub_gitlab_calls.rb b/spec/support/helpers/stub_gitlab_calls.rb index 370d20df444..4cb3b18df85 100644 --- a/spec/support/helpers/stub_gitlab_calls.rb +++ b/spec/support/helpers/stub_gitlab_calls.rb @@ -62,6 +62,12 @@ module StubGitlabCalls end 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_content |