From 433bcf9b0439771b1e2661a62fec115e44922232 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Thu, 31 Jan 2019 12:28:31 +0100 Subject: [PATCH] Add local markdown version Cached markdown version is composed both from global and local markdown version. This allows admins to bump version locally when needed (e.g. when external URL is changed). --- app/helpers/application_settings_helper.rb | 3 +- app/models/application_setting.rb | 7 ++- app/models/concerns/cache_markdown_field.rb | 23 +++++++- .../local-markdown-version-bkp3.yml | 5 ++ ...91630_add_local_cached_markdown_version.rb | 11 ++++ db/schema.rb | 1 + doc/administration/index.md | 1 + .../invalidate_markdown_cache.md | 16 ++++++ doc/api/settings.md | 7 ++- lib/api/settings.rb | 1 + spec/features/snippets/show_spec.rb | 8 --- spec/javascripts/notes/mock_data.js | 1 - spec/lib/banzai/object_renderer_spec.rb | 2 +- spec/models/application_setting_spec.rb | 7 +++ .../concerns/cache_markdown_field_spec.rb | 55 ++++++++++++------- spec/models/resource_label_event_spec.rb | 6 +- spec/requests/api/settings_spec.rb | 4 +- 17 files changed, 118 insertions(+), 40 deletions(-) create mode 100644 changelogs/unreleased/local-markdown-version-bkp3.yml create mode 100644 db/migrate/20190130091630_add_local_cached_markdown_version.rb create mode 100644 doc/administration/invalidate_markdown_cache.md diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index c8e4e2e3df9..5ba90a272c9 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 88746375c67..c658211d12d 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 5fa6f79bdaa..1a8570b80c3 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 00000000000..ce5bff6ae6b --- /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 00000000000..00570e6458c --- /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 4b6e4992056..095c2fbab4a 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 0b673d61139..38f746ca5bc 100644 --- a/doc/administration/index.md +++ b/doc/administration/index.md @@ -64,6 +64,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 00000000000..ad64cb077c1 --- /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 c329e3cdf24..287e48c2a42 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 95371961398..b16faffe335 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 eb974c7c7fd..74fdfcf492e 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 7ae45c40c28..348743081eb 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 209a547c3b3..3b52f6666d0 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 96aa9a82b71..789e14e8a20 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 29197ef372e..447279f19a8 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 3797960ac3d..7eeb2fae57d 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 45fb1562e84..f33eb5b9e02 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