From ff28a7cc3687485ee138d4616a56ba1548a03e7c Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 23 Feb 2016 12:02:59 +0100 Subject: [PATCH 1/8] Moved cache expiration code to Repository hooks This keeps all the cache expiration code in a single file/class instead of spreading it all across the codebase. --- app/models/project.rb | 5 +--- app/models/repository.rb | 38 ++++++++++++++++++++++++ app/services/git_push_service.rb | 6 ++-- app/services/git_tag_push_service.rb | 2 +- app/services/projects/destroy_service.rb | 6 ++-- app/workers/repository_fork_worker.rb | 2 +- app/workers/repository_import_worker.rb | 2 +- 7 files changed, 47 insertions(+), 14 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 95ad88c76ae..6f5d592755a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -801,10 +801,7 @@ class Project < ActiveRecord::Base end def change_head(branch) - # Cached divergent commit counts are based on repository head - repository.expire_branch_cache - repository.expire_root_ref_cache - + repository.before_change_head gitlab_shell.update_repository_head(self.path_with_namespace, branch) reload_default_branch end diff --git a/app/models/repository.rb b/app/models/repository.rb index e050bd45254..e3769451001 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -307,6 +307,44 @@ class Repository cache.expire(:branch_names) end + # Runs code just before a repository is deleted. + def before_delete + # FIXME: a repository not existing shouldn't prevent us from flushing caches. + expire_all_caches! if exists? + end + + # Runs code just before the HEAD of a repository is changed. + def before_change_head + # Cached divergent commit counts are based on repository head + expire_branch_cache + expire_root_ref_cache + end + + # Runs code before creating a new tag. + def before_create_tag + expire_cache + end + + # Runs code after a repository has been forked/imported. + def after_import + expire_emptiness_caches + end + + # Runs code after a new commit has been pushed. + def after_push_commit(branch_name) + expire_cache(branch_name) + end + + # Runs code after a new branch has been created. + def after_create_branch + expire_has_visible_content_cache + end + + # Runs code after an existing branch has been removed. + def after_remove_branch + expire_has_visible_content_cache + end + def method_missing(m, *args, &block) if m == :lookup && !block_given? lookup_cache[m] ||= {} diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index a1711d234ff..9ba200f7bde 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -16,13 +16,13 @@ class GitPushService < BaseService # 5. Executes the project's services # def execute - @project.repository.expire_cache(branch_name) + @project.repository.after_push_commit(branch_name) if push_remove_branch? - @project.repository.expire_has_visible_content_cache + @project.repository.after_remove_branch @push_commits = [] elsif push_to_new_branch? - @project.repository.expire_has_visible_content_cache + @project.repository.after_create_branch # Re-find the pushed commits. if is_default_branch? diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb index 4144c7111d0..a62c5fc4fc4 100644 --- a/app/services/git_tag_push_service.rb +++ b/app/services/git_tag_push_service.rb @@ -2,7 +2,7 @@ class GitTagPushService attr_accessor :project, :user, :push_data def execute(project, user, oldrev, newrev, ref) - project.repository.expire_cache + project.repository.before_create_tag @project, @user = project, user @push_data = build_push_data(oldrev, newrev, ref) diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index f4dcb142850..df5054f08d7 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -76,11 +76,9 @@ module Projects end def flush_caches(project, wiki_path) - project.repository.expire_all_caches! if project.repository.exists? + project.repository.before_delete - wiki_repo = Repository.new(wiki_path, project) - - wiki_repo.expire_all_caches! if wiki_repo.exists? + Repository.new(wiki_path, project).before_delete end end end diff --git a/app/workers/repository_fork_worker.rb b/app/workers/repository_fork_worker.rb index 2572b9d6d98..21d311579e3 100644 --- a/app/workers/repository_fork_worker.rb +++ b/app/workers/repository_fork_worker.rb @@ -27,7 +27,7 @@ class RepositoryForkWorker return end - project.repository.expire_emptiness_caches + project.repository.after_import project.import_finish end end diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb index 0b6f746e118..2937493c614 100644 --- a/app/workers/repository_import_worker.rb +++ b/app/workers/repository_import_worker.rb @@ -18,7 +18,7 @@ class RepositoryImportWorker return end - project.repository.expire_emptiness_caches + project.repository.after_import project.import_finish end end From a39f7b30c5d5fa40408ad572ddbbcbfbe50d8f95 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 23 Feb 2016 12:27:34 +0100 Subject: [PATCH 2/8] Move Repository#expire_all_caches to before_delete --- app/models/repository.rb | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index e3769451001..38438959d40 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -245,15 +245,6 @@ class Repository expire_emptiness_caches if empty? end - # Expires _all_ caches, including those that would normally only be expired - # under specific conditions. - def expire_all_caches! - expire_cache - expire_root_ref_cache - expire_emptiness_caches - expire_has_visible_content_cache - end - def expire_branch_cache(branch_name = nil) # When we push to the root branch we have to flush the cache for all other # branches as their statistics are based on the commits relative to the @@ -310,7 +301,11 @@ class Repository # Runs code just before a repository is deleted. def before_delete # FIXME: a repository not existing shouldn't prevent us from flushing caches. - expire_all_caches! if exists? + if exists? + expire_cache + expire_root_ref_cache + expire_emptiness_caches + end end # Runs code just before the HEAD of a repository is changed. From 3b520efdc7ff00e291464a7c191db12e4d191bfe Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 25 Feb 2016 12:55:37 +0100 Subject: [PATCH 3/8] Moved exists condition in Repository#before_delete This is only needed when calling the "expire_cache" method as the other expiration methods don't depend on Git data. --- app/models/repository.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 38438959d40..a214a69d749 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -300,12 +300,10 @@ class Repository # Runs code just before a repository is deleted. def before_delete - # FIXME: a repository not existing shouldn't prevent us from flushing caches. - if exists? - expire_cache - expire_root_ref_cache - expire_emptiness_caches - end + expire_cache if exists? + + expire_root_ref_cache + expire_emptiness_caches end # Runs code just before the HEAD of a repository is changed. From a10678e7ebd527276f3f66d90676a03264c2ecda Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 25 Feb 2016 12:56:04 +0100 Subject: [PATCH 4/8] Added specs for the various Repository hooks --- spec/models/repository_spec.rb | 108 ++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 2 deletions(-) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index b596782f4e1..51ae2c04ed0 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -362,14 +362,14 @@ describe Repository, models: true do repository.expire_cache('master') end - it 'expires the emptiness cache for an empty repository' do + it 'expires the emptiness caches for an empty repository' do expect(repository).to receive(:empty?).and_return(true) expect(repository).to receive(:expire_emptiness_caches) repository.expire_cache end - it 'does not expire the emptiness cache for a non-empty repository' do + it 'does not expire the emptiness caches for a non-empty repository' do expect(repository).to receive(:empty?).and_return(false) expect(repository).to_not receive(:expire_emptiness_caches) @@ -464,4 +464,108 @@ describe Repository, models: true do expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present end end + + describe '#before_delete' do + describe 'when a repository does not exist' do + before do + allow(repository).to receive(:exists?).and_return(false) + end + + it 'does not flush caches that depend on repository data' do + expect(repository).to_not receive(:expire_cache) + + repository.before_delete + end + + it 'flushes the root ref cache' do + expect(repository).to receive(:expire_root_ref_cache) + + repository.before_delete + end + + it 'flushes the emptiness caches' do + expect(repository).to receive(:expire_emptiness_caches) + + repository.before_delete + end + end + + describe 'when a repository exists' do + before do + allow(repository).to receive(:exists?).and_return(true) + end + + it 'flushes the caches that depend on repository data' do + expect(repository).to receive(:expire_cache) + + repository.before_delete + end + + it 'flushes the root ref cache' do + expect(repository).to receive(:expire_root_ref_cache) + + repository.before_delete + end + + it 'flushes the emptiness caches' do + expect(repository).to receive(:expire_emptiness_caches) + + repository.before_delete + end + end + end + + describe '#before_change_head' do + it 'flushes the branch cache' do + expect(repository).to receive(:expire_branch_cache) + + repository.before_change_head + end + + it 'flushes the root ref cache' do + expect(repository).to receive(:expire_root_ref_cache) + + repository.before_change_head + end + end + + describe '#before_create_tag' do + it 'flushes the cache' do + expect(repository).to receive(:expire_cache) + + repository.before_create_tag + end + end + + describe '#after_import' do + it 'flushes the emptiness cachess' do + expect(repository).to receive(:expire_emptiness_caches) + + repository.after_import + end + end + + describe '#after_push_commit' do + it 'flushes the cache' do + expect(repository).to receive(:expire_cache).with('master') + + repository.after_push_commit('master') + end + end + + describe '#after_create_branch' do + it 'flushes the visible content cache' do + expect(repository).to receive(:expire_has_visible_content_cache) + + repository.after_create_branch + end + end + + describe '#after_remove_branch' do + it 'flushes the visible content cache' do + expect(repository).to receive(:expire_has_visible_content_cache) + + repository.after_remove_branch + end + end end From 4d0e2979b9a17160ad93ff704e7a51f78b4f3b4c Mon Sep 17 00:00:00 2001 From: evuez Date: Mon, 22 Feb 2016 13:55:36 +0100 Subject: [PATCH 5/8] Allow webhooks URL to have leading and trailing spaces --- CHANGELOG | 1 + app/validators/url_validator.rb | 3 +++ spec/models/hooks/project_hook_spec.rb | 4 ++++ spec/models/hooks/web_hook_spec.rb | 20 ++++++++++---------- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 5521c1286a8..9e3fb8db03d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.6.0 (unreleased) - Improve the formatting for the user page bio (Connor Shea) - Fix avatar stretching by providing a cropping feature (Johann Pardanaud) + - Strip leading and trailing spaces in URL validator (evuez) v 8.5.1 - Fix group projects styles diff --git a/app/validators/url_validator.rb b/app/validators/url_validator.rb index 2848b9cd33d..a77beb2683d 100644 --- a/app/validators/url_validator.rb +++ b/app/validators/url_validator.rb @@ -29,8 +29,11 @@ class UrlValidator < ActiveModel::EachValidator end def valid_url?(value) + return false if value.nil? + options = default_options.merge(self.options) + value.strip! value =~ /\A#{URI.regexp(options[:protocols])}\z/ end end diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index 645ee0b929a..983848392b7 100644 --- a/spec/models/hooks/project_hook_spec.rb +++ b/spec/models/hooks/project_hook_spec.rb @@ -19,6 +19,10 @@ require 'spec_helper' describe ProjectHook, models: true do + describe "Associations" do + it { is_expected.to belong_to :project } + end + describe '.push_hooks' do it 'should return hooks for push events only' do hook = create(:project_hook, push_events: true) diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 7070aa4ac62..6ea99952a8f 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -18,20 +18,14 @@ require 'spec_helper' -describe ProjectHook, models: true do - describe "Associations" do - it { is_expected.to belong_to :project } - end - - describe "Mass assignment" do - end - +describe WebHook, models: true do describe "Validations" do it { is_expected.to validate_presence_of(:url) } - context "url format" do + describe 'url' do it { is_expected.to allow_value("http://example.com").for(:url) } - it { is_expected.to allow_value("https://excample.com").for(:url) } + it { is_expected.to allow_value("https://example.com").for(:url) } + it { is_expected.to allow_value(" https://example.com ").for(:url) } it { is_expected.to allow_value("http://test.com/api").for(:url) } it { is_expected.to allow_value("http://test.com/api?key=abc").for(:url) } it { is_expected.to allow_value("http://test.com/api?key=abc&type=def").for(:url) } @@ -39,6 +33,12 @@ describe ProjectHook, models: true do it { is_expected.not_to allow_value("example.com").for(:url) } it { is_expected.not_to allow_value("ftp://example.com").for(:url) } it { is_expected.not_to allow_value("herp-and-derp").for(:url) } + + it 'strips :url before saving it' do + hook = create(:project_hook, url: ' https://example.com ') + + expect(hook.url).to eq('https://example.com') + end end end From 4fcaa7a474295b89b2ed740c179b49c48efbd09c Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 26 Feb 2016 18:14:49 -0500 Subject: [PATCH 6/8] Fix issue milestone filter spec --- spec/features/issues/filter_by_milestone_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/issues/filter_by_milestone_spec.rb b/spec/features/issues/filter_by_milestone_spec.rb index 38c8d343ce3..591866b40d4 100644 --- a/spec/features/issues/filter_by_milestone_spec.rb +++ b/spec/features/issues/filter_by_milestone_spec.rb @@ -13,7 +13,7 @@ feature 'Issue filtering by Milestone', feature: true do visit_issues(project) filter_by_milestone(Milestone::None.title) - expect(page).to have_css('.title', count: 1) + expect(page).to have_css('.issue .title', count: 1) end scenario 'filters by a specific Milestone', js: true do @@ -23,7 +23,7 @@ feature 'Issue filtering by Milestone', feature: true do visit_issues(project) filter_by_milestone(milestone.title) - expect(page).to have_css('.title', count: 1) + expect(page).to have_css('.issue .title', count: 1) end def visit_issues(project) From d324c67db3e50547b6e1a585cb937ead0b20d13e Mon Sep 17 00:00:00 2001 From: Thomas Schmidt Date: Mon, 22 Feb 2016 19:44:13 +0100 Subject: [PATCH 7/8] Adjust documentation of permissions [ci skip] --- CHANGELOG | 1 + doc/permissions/permissions.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 9e3fb8db03d..1e22c125527 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.6.0 (unreleased) - Improve the formatting for the user page bio (Connor Shea) - Fix avatar stretching by providing a cropping feature (Johann Pardanaud) - Strip leading and trailing spaces in URL validator (evuez) + - Update documentation to reflect Guest role not being enforced on internal projects v 8.5.1 - Fix group projects styles diff --git a/doc/permissions/permissions.md b/doc/permissions/permissions.md index f717b30c12e..ac0fd3d1756 100644 --- a/doc/permissions/permissions.md +++ b/doc/permissions/permissions.md @@ -6,7 +6,7 @@ If a user is both in a project group and in the project itself, the highest perm If a user is a GitLab administrator they receive all permissions. -On public projects the Guest role is not enforced. +On public and internal projects the Guest role is not enforced. All users will be able to create issues, leave comments, and pull or download the project code. To add or import a user, you can follow the [project users and members From f838dbe4d13c55fc8796eadf33165f1406612662 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 27 Feb 2016 19:27:11 -0500 Subject: [PATCH 8/8] Update CHANGELOG [ci skip] --- CHANGELOG | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 1e22c125527..9e897644af0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -6,6 +6,10 @@ v 8.6.0 (unreleased) - Strip leading and trailing spaces in URL validator (evuez) - Update documentation to reflect Guest role not being enforced on internal projects +v 8.5.2 + - Fix sidebar overlapping content when screen width was below 1200px + - Fix error 500 when commenting on a commit + v 8.5.1 - Fix group projects styles - Show Crowd login tab when sign in is disabled and Crowd is enabled (Peter Hudec) @@ -25,9 +29,6 @@ v 8.5.1 - Update sentry-raven gem to 0.15.6 - Add build coverage in project's builds page (Steffen Köhler) -v 8.5.2 - - Fix error 500 when commenting on a commit - v 8.5.0 - Fix duplicate "me" in tooltip of the "thumbsup" awards Emoji (Stan Hu) - Cache various Repository methods to improve performance (Yorick Peterse)