diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index c8e4e2e3df968002bcac4e46e7ac82da9d7aa278..5ba90a272c944be129819991da5b643ad8118126 100644 --- a/app/helpers/application_settings_helper.rb +++ b/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 diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 88746375c6702138ac7bab92efe6b50cd467f318..c658211d12d48c796fbeafacee62582f7de81f5e 100644 --- a/app/models/application_setting.rb +++ b/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 diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 5fa6f79bdaa241e615dc1c112ba029eac59f63bc..1a8570b80c368202a7dfe71a2bba4b70541e68d5 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/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 diff --git a/changelogs/unreleased/local-markdown-version-bkp3.yml b/changelogs/unreleased/local-markdown-version-bkp3.yml new file mode 100644 index 0000000000000000000000000000000000000000..ce5bff6ae6bb2bf059ec0a215fc27c68cfee8b2b --- /dev/null +++ b/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 diff --git a/db/migrate/20190130091630_add_local_cached_markdown_version.rb b/db/migrate/20190130091630_add_local_cached_markdown_version.rb new file mode 100644 index 0000000000000000000000000000000000000000..00570e6458c7f893edf1761941f22c2fd29c3a42 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 20c8dab4c3eebcf3ce52ebcde1c28da5351d8fc9..97ce8d216455def0bd56733882607954ae568d7c 100644 --- a/db/schema.rb +++ b/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 diff --git a/doc/administration/index.md b/doc/administration/index.md index 184754cd467482281f78552b2088394f501dbf76..12fec2753bf2f484165a02f29bbf6c4c5ab1ae3b 100644 --- a/doc/administration/index.md +++ b/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 diff --git a/doc/administration/invalidate_markdown_cache.md b/doc/administration/invalidate_markdown_cache.md new file mode 100644 index 0000000000000000000000000000000000000000..ad64cb077c1d19a51e245347bb463450df327355 --- /dev/null +++ b/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: " https://gitlab.example.com/api/v4/application/settings?local_markdown_version= +``` diff --git a/doc/api/settings.md b/doc/api/settings.md index c329e3cdf241ec56c7ce6037a4d1a908c497c955..287e48c2a42feb5233a04b5653194e1cb424efaf 100644 --- a/doc/api/settings.md +++ b/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. | diff --git a/lib/api/settings.rb b/lib/api/settings.rb index 953719613980a3143b6edf4fb0af1d6063eaddae..b16faffe335214275c94c708819a8c3febfca591 100644 --- a/lib/api/settings.rb +++ b/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", diff --git a/spec/features/snippets/show_spec.rb b/spec/features/snippets/show_spec.rb index eb974c7c7fd5a1fbc3939e9ad604eab499d76a04..74fdfcf492ebdcc1c751b9a17f3cb4e6599de83f 100644 --- a/spec/features/snippets/show_spec.rb +++ b/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 diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index 7ae45c40c28320c2d6abeeeb7f1b81ecf9d3edf1..348743081eb5d688107014f2c323e399aa463065 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/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 = { diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index 209a547c3b38fd78ee5de03bf6e9b1e62be41a3e..3b52f6666d0fbbd38bf6b9ee87d2de3a8354fc81 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -11,7 +11,7 @@ describe Banzai::ObjectRenderer do ) end - let(:object) { Note.new(note: 'hello', note_html: '

hello

', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) } + let(:object) { Note.new(note: 'hello', note_html: '

hello

', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) } describe '#render' do context 'with cache' do diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 96aa9a82b718dc1811a3f9781e7f3a7fbb41e33e..789e14e8a20fad02bfecc5a55202c0130a17513c 100644 --- a/spec/models/application_setting_spec.rb +++ b/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) diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 29197ef372ec239b33c8e8b513f4efd6e3de97c4..447279f19a8eea789c6d426f4ed5d292009850ac 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/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) { '

Bar

' } - 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 diff --git a/spec/models/resource_label_event_spec.rb b/spec/models/resource_label_event_spec.rb index 3797960ac3d3b1e2267c417474b8d810c48c9076..7eeb2fae57d84ef4cfa75b53e2282e52099c524e 100644 --- a/spec/models/resource_label_event_spec.rb +++ b/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 diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index 45fb1562e84732f11d8dc94acb4ac241d2128c34..f33eb5b9e0208401d457a7d7940724e527ed1611 100644 --- a/spec/requests/api/settings_spec.rb +++ b/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