Merge branch 'local-markdown-version' into 'master'

Add local markdown version

Closes #32789

See merge request gitlab-org/gitlab-ce!24822

app/helpers/application_settings_helper.rb

@@ -231,7 +231,8 @@ module ApplicationSettingsHelper
       :web_ide_clientside_preview_enabled,
       :diff_max_patch_bytes,
       :commit_email_hostname,
-      :protected_ci_variables
+      :protected_ci_variables,
+      :local_markdown_version
     ]
   end
 

app/models/application_setting.rb

@@ -193,6 +193,10 @@ class ApplicationSetting < ActiveRecord::Base
             allow_nil: true,
             numericality: { only_integer: true, greater_than_or_equal_to: 1.day.seconds }
 
+  validates :local_markdown_version,
+            allow_nil: true,
+            numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than: 65536 }
+
   SUPPORTED_KEY_TYPES.each do |type|
     validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
   end
@@ -303,7 +307,8 @@ class ApplicationSetting < ActiveRecord::Base
       usage_stats_set_by_user_id: nil,
       diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
       commit_email_hostname: default_commit_email_hostname,
-      protected_ci_variables: false
+      protected_ci_variables: false,
+      local_markdown_version: 0
     }
   end
 

app/models/concerns/cache_markdown_field.rb

@@ -115,7 +115,28 @@ module CacheMarkdownField
   end
 
   def latest_cached_markdown_version
-    CacheMarkdownField::CACHE_COMMONMARK_VERSION
+    @latest_cached_markdown_version ||= (CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) | local_version
+  end
+
+  def local_version
+    # because local_markdown_version is stored in application_settings which
+    # uses cached_markdown_version too, we check explicitly to avoid
+    # endless loop
+    return local_markdown_version if has_attribute?(:local_markdown_version)
+
+    settings = Gitlab::CurrentSettings.current_application_settings
+
+    # Following migrations are not properly isolated and
+    # use real models (by calling .ghost method), in these migrations
+    # local_markdown_version attribute doesn't exist yet, so we
+    # use a default value:
+    # db/migrate/20170825104051_migrate_issues_to_ghost_user.rb
+    # db/migrate/20171114150259_merge_requests_author_id_foreign_key.rb
+    if settings.respond_to?(:local_markdown_version)
+      settings.local_markdown_version
+    else
+      0
+    end
   end
 
   included do

changelogs/unreleased/local-markdown-version-bkp3.yml

@@ -0,0 +1,5 @@
+---
+title: Allow admins to invalidate markdown texts by setting local markdown version.
+merge_request:
+author:
+type: added

db/migrate/20190130091630_add_local_cached_markdown_version.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class AddLocalCachedMarkdownVersion < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :local_markdown_version, :integer, default: 0, null: false
+  end
+end

db/schema.rb

@@ -168,6 +168,7 @@ ActiveRecord::Schema.define(version: 20190131122559) do
     t.string "commit_email_hostname"
     t.boolean "protected_ci_variables", default: false, null: false
     t.string "runners_registration_token_encrypted"
+    t.integer "local_markdown_version", default: 0, null: false
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree
   end
 

doc/administration/index.md

@@ -65,6 +65,7 @@ Learn how to install, configure, update, and maintain your GitLab instance.
   - [Backup and restore](../raketasks/backup_restore.md): Backup and restore your GitLab instance.
 - [Operations](operations/index.md): Keeping GitLab up and running (clean up Redis sessions, moving repositories, Sidekiq MemoryKiller, Unicorn).
 - [Restart GitLab](restart_gitlab.md): Learn how to restart GitLab and its components.
+- [Invalidate markdown cache](invalidate_markdown_cache.md): Invalidate any cached markdown.
 
 #### Updating GitLab
 

doc/administration/invalidate_markdown_cache.md

@@ -0,0 +1,16 @@
+# Invalidate Markdown Cache
+
+For performance reasons, GitLab caches the HTML version of markdown text
+(e.g. issue and merge request descriptions, comments). It's possible
+that these cached versions become outdated, for example
+when the `external_url` configuration option is changed - causing links
+in the cached text to refer to the old URL.
+
+To avoid this problem, the administrator can invalidate the existing cache by
+increasing the `local_markdown_version` setting in application settings.  This can
+be done by [changing the application settings through
+the API](../api/settings.md#change-application-settings):
+
+```bash
+curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/application/settings?local_markdown_version=<increased_number>
+```

doc/api/settings.md

@@ -61,7 +61,8 @@ Example response:
    "terms": "Hello world!",
    "performance_bar_allowed_group_id": 42,
    "instance_statistics_visibility_private": false,
-   "user_show_add_ssh_key_message": true
+   "user_show_add_ssh_key_message": true,
+   "local_markdown_version": 0
 }
 ```
 
@@ -117,7 +118,8 @@ Example response:
   "terms": "Hello world!",
   "performance_bar_allowed_group_id": 42,
   "instance_statistics_visibility_private": false,
-  "user_show_add_ssh_key_message": true
+  "user_show_add_ssh_key_message": true,
+  "local_markdown_version": 0
 }
 ```
 
@@ -235,3 +237,4 @@ are listed in the descriptions of the relevant settings.
 | `user_oauth_applications`                | boolean          | no                                   | Allow users to register any application to use GitLab as an OAuth provider. |
 | `user_show_add_ssh_key_message`          | boolean          | no                                   | When set to `false` disable the "You won't be able to pull or push project code via SSH" warning shown to users with no uploaded SSH key. |
 | `version_check_enabled`                  | boolean          | no                                   | Let GitLab inform you when an update is available. |
+| `local_markdown_version`                 | integer          | no                                   | Increase this value when any cached markdown should be invalidated. |

lib/api/settings.rb

@@ -121,6 +121,7 @@ module API
       optional :terminal_max_session_time, type: Integer, desc: 'Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time.'
       optional :usage_ping_enabled, type: Boolean, desc: 'Every week GitLab will report license usage back to GitLab, Inc.'
       optional :instance_statistics_visibility_private, type: Boolean, desc: 'When set to `true` Instance statistics will only be available to admins'
+      optional :local_markdown_version, type: Integer, desc: "Local markdown version, increase this value when any cached markdown should be invalidated"
 
       ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type|
         optional :"#{type}_key_restriction",

spec/features/snippets/show_spec.rb

@@ -79,14 +79,6 @@ describe 'Snippet', :js do
             expect(page).not_to have_xpath("//ol//li//ul")
           end
         end
-
-        context 'with cached CommonMark html' do
-          let(:snippet) { create(:personal_snippet, :public, file_name: file_name, content: content, cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
-
-          it 'renders correctly' do
-            expect(page).not_to have_xpath("//ol//li//ul")
-          end
-        end
       end
 
       context 'switching to the simple viewer' do

spec/javascripts/notes/mock_data.js

@@ -165,7 +165,6 @@ export const note = {
   report_abuse_path:
     '/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_546&user_id=1',
   path: '/gitlab-org/gitlab-ce/notes/546',
-  cached_markdown_version: 11,
 };
 
 export const discussionMock = {

spec/lib/banzai/object_renderer_spec.rb

@@ -11,7 +11,7 @@ describe Banzai::ObjectRenderer do
     )
   end
 
-  let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
+  let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) }
 
   describe '#render' do
     context 'with cache' do

spec/models/application_setting_spec.rb

@@ -70,6 +70,13 @@ describe ApplicationSetting do
         .is_greater_than(0)
     end
 
+    it do
+      is_expected.to validate_numericality_of(:local_markdown_version)
+        .only_integer
+        .is_greater_than_or_equal_to(0)
+        .is_less_than(65536)
+    end
+
     context 'key restrictions' do
       it 'supports all key types' do
         expect(described_class::SUPPORTED_KEY_TYPES).to contain_exactly(:rsa, :dsa, :ecdsa, :ed25519)

spec/models/concerns/cache_markdown_field_spec.rb

@@ -60,6 +60,10 @@ describe CacheMarkdownField do
         changes_applied
       end
     end
+
+    def has_attribute?(attr_name)
+      attribute_names.include?(attr_name)
+    end
   end
 
   def thing_subclass(new_attr)
@@ -72,8 +76,8 @@ describe CacheMarkdownField do
   let(:updated_markdown) { '`Bar`' }
   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) }
-  let(:cache_version) { CacheMarkdownField::CACHE_COMMONMARK_VERSION }
+  let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+  let(:cache_version) { CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16 }
 
   before do
     stub_commonmark_sourcepos_disabled
@@ -94,11 +98,11 @@ describe CacheMarkdownField do
     it { expect(thing.foo).to eq(markdown) }
     it { expect(thing.foo_html).to eq(html) }
     it { expect(thing.foo_html_changed?).not_to be_truthy }
-    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
+    it { expect(thing.cached_markdown_version).to eq(cache_version) }
   end
 
   context 'a changed markdown field' do
-    let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+    let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version - 1) }
 
     before do
       thing.foo = updated_markdown
@@ -139,7 +143,7 @@ describe CacheMarkdownField do
   end
 
   context 'a non-markdown field changed' do
-    let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+    let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version - 1) }
 
     before do
       thing.bar = 'OK'
@@ -160,7 +164,7 @@ describe CacheMarkdownField do
     end
 
     it { expect(thing.foo_html).to eq(updated_html) }
-    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
+    it { expect(thing.cached_markdown_version).to eq(cache_version) }
   end
 
   describe '#cached_html_up_to_date?' do
@@ -174,21 +178,35 @@ describe CacheMarkdownField do
       is_expected.to be_falsy
     end
 
-    it 'returns false when the version is too early' do
-      thing.cached_markdown_version -= 1
+    it 'returns false when the cached version is too old' do
+      thing.cached_markdown_version = cache_version - 1
 
       is_expected.to be_falsy
     end
 
-    it 'returns false when the version is too late' do
-      thing.cached_markdown_version += 1
+    it 'returns false when the cached version is in future' do
+      thing.cached_markdown_version = cache_version + 1
 
       is_expected.to be_falsy
     end
 
-    it 'returns true when the version is just right' do
+    it 'returns false when the local version was bumped' do
+      allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2)
       thing.cached_markdown_version = cache_version
 
+      is_expected.to be_falsy
+    end
+
+    it 'returns true when the local version is default' do
+      thing.cached_markdown_version = cache_version
+
+      is_expected.to be_truthy
+    end
+
+    it 'returns true when the cached version is just right' do
+      allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2)
+      thing.cached_markdown_version = cache_version + 2
+
       is_expected.to be_truthy
     end
 
@@ -221,14 +239,9 @@ describe CacheMarkdownField do
   describe '#latest_cached_markdown_version' do
     subject { thing.latest_cached_markdown_version }
 
-    it 'returns commonmark version' do
-      thing.cached_markdown_version = CacheMarkdownField::CACHE_COMMONMARK_VERSION_START + 1
-      is_expected.to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
-    end
-
-    it 'returns default version when version is nil' do
+    it 'returns default version' do
       thing.cached_markdown_version = nil
-      is_expected.to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
+      is_expected.to eq(cache_version)
     end
   end
 
@@ -255,7 +268,7 @@ describe CacheMarkdownField do
       thing.cached_markdown_version = nil
       thing.refresh_markdown_cache
 
-      expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
+      expect(thing.cached_markdown_version).to eq(cache_version)
     end
   end
 
@@ -336,7 +349,7 @@ describe CacheMarkdownField do
 
         expect(thing.foo_html).to eq(updated_html)
         expect(thing.baz_html).to eq(updated_html)
-        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
+        expect(thing.cached_markdown_version).to eq(cache_version)
       end
     end
 
@@ -356,7 +369,7 @@ describe CacheMarkdownField do
 
         expect(thing.foo_html).to eq(updated_html)
         expect(thing.baz_html).to eq(updated_html)
-        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
+        expect(thing.cached_markdown_version).to eq(cache_version)
       end
     end
   end

spec/models/resource_label_event_spec.rb

@@ -81,14 +81,14 @@ RSpec.describe ResourceLabelEvent, type: :model do
       expect(subject.outdated_markdown?).to be true
     end
 
-    it 'returns true markdown is outdated' do
-      subject.attributes = { cached_markdown_version: 0 }
+    it 'returns true if markdown is outdated' do
+      subject.attributes = { cached_markdown_version: ((CacheMarkdownField::CACHE_COMMONMARK_VERSION - 1) << 16) | 0 }
 
       expect(subject.outdated_markdown?).to be true
     end
 
     it 'returns false if label and reference are set' do
-      subject.attributes = { reference: 'whatever', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION }
+      subject.attributes = { reference: 'whatever', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16 }
 
       expect(subject.outdated_markdown?).to be false
     end

spec/requests/api/settings_spec.rb

@@ -64,7 +64,8 @@ describe API::Settings, 'Settings' do
             performance_bar_allowed_group_path: group.full_path,
             instance_statistics_visibility_private: true,
             diff_max_patch_bytes: 150_000,
-            default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE
+            default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE,
+            local_markdown_version: 3
           }
 
         expect(response).to have_gitlab_http_status(200)
@@ -90,6 +91,7 @@ describe API::Settings, 'Settings' do
         expect(json_response['instance_statistics_visibility_private']).to be(true)
         expect(json_response['diff_max_patch_bytes']).to eq(150_000)
         expect(json_response['default_branch_protection']).to eq(Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+        expect(json_response['local_markdown_version']).to eq(3)
       end
     end