diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2018-10-25 17:35:31 +0200 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2018-11-05 14:28:29 +0100 |
commit | d171ff60168cd55b6d7b9ee920269f44a26e577e (patch) | |
tree | 3d8885ab8e70f578a57cb168c7533811cf0da241 /spec/models | |
parent | 6d7bf439d6bad4fff6bba0fbdb91edf50974ad4b (diff) | |
download | gitlab-ce-d171ff60168cd55b6d7b9ee920269f44a26e577e.tar.gz |
Rewrite SnippetsFinder to improve performance
This completely rewrites the SnippetsFinder class from the ground up in
order to improve its performance. The old code was beyond salvaging. It
was complex, included various Rails 5 workarounds, comments that
shouldn't be necessary, and most important of all: it produced a really
poorly performing database query.
As a result, I opted for rewriting the finder from scratch, instead of
trying to patch the existing code. Instead of trying to reuse as many
existing methods as possible, I opted for defining new methods
specifically meant for the SnippetsFinder. This requires some extra code
here and there, but allows us to have much more control over the
resulting SQL queries. It is these changes that then allow us to produce
a _much_ more efficient query.
To illustrate how bad the old query was, we will use my own snippets as
an example. Currently I have 52 snippets, most of which are global ones.
To retrieve these, you would run the following Ruby code:
user = User.find_by(username: 'yorickpeterse')
SnippetsFinder.new(user, author: user).execute
On GitLab.com the resulting query will take between 10 and 15 seconds to
run, producing the query plan found at
https://explain.depesz.com/s/Y5IX. Apart from the long execution time,
the total number of buffers (the sum of all shared hits) is around 185
GB, though the real number is probably (hopefully) much lower as I doubt
simply summing these numbers produces the true total number of buffers
used.
The new query's plan can be found at https://explain.depesz.com/s/wHdN,
and this query takes between 10 and 100-ish milliseconds to run. The
total number of buffers used is only about 30 MB.
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/52639
Diffstat (limited to 'spec/models')
-rw-r--r-- | spec/models/project_spec.rb | 22 | ||||
-rw-r--r-- | spec/models/snippet_spec.rb | 211 |
2 files changed, 233 insertions, 0 deletions
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index d4b9a4c8cd6..be2aff73c0a 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3963,6 +3963,28 @@ describe Project do end end + describe '#snippets_visible?' do + it 'returns true when a logged in user can read snippets' do + project = create(:project, :public) + user = create(:user) + + expect(project.snippets_visible?(user)).to eq(true) + end + + it 'returns true when an anonymous user can read snippets' do + project = create(:project, :public) + + expect(project.snippets_visible?).to eq(true) + end + + it 'returns false when a user can not read snippets' do + project = create(:project, :private) + user = create(:user) + + expect(project.snippets_visible?(user)).to eq(false) + end + end + def rugged_config rugged_repo(project.repository).config end diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index e09d89d235d..7a7272ccb60 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -131,6 +131,217 @@ describe Snippet do end end + describe '.with_optional_visibility' do + context 'when a visibility level is provided' do + it 'returns snippets with the given visibility' do + create(:snippet, :private) + + snippet = create(:snippet, :public) + snippets = described_class + .with_optional_visibility(Gitlab::VisibilityLevel::PUBLIC) + + expect(snippets).to eq([snippet]) + end + end + + context 'when a visibility level is not provided' do + it 'returns all snippets' do + snippet1 = create(:snippet, :public) + snippet2 = create(:snippet, :private) + snippets = described_class.with_optional_visibility + + expect(snippets).to include(snippet1, snippet2) + end + end + end + + describe '.only_global_snippets' do + it 'returns snippets not associated with any projects' do + create(:project_snippet) + + snippet = create(:snippet) + snippets = described_class.only_global_snippets + + expect(snippets).to eq([snippet]) + end + end + + describe '.only_include_projects_visible_to' do + let!(:project1) { create(:project, :public) } + let!(:project2) { create(:project, :internal) } + let!(:project3) { create(:project, :private) } + let!(:snippet1) { create(:project_snippet, project: project1) } + let!(:snippet2) { create(:project_snippet, project: project2) } + let!(:snippet3) { create(:project_snippet, project: project3) } + + context 'when a user is provided' do + it 'returns snippets visible to the user' do + user = create(:user) + + snippets = described_class.only_include_projects_visible_to(user) + + expect(snippets).to include(snippet1, snippet2) + expect(snippets).not_to include(snippet3) + end + end + + context 'when a user is not provided' do + it 'returns snippets visible to anonymous users' do + snippets = described_class.only_include_projects_visible_to + + expect(snippets).to include(snippet1) + expect(snippets).not_to include(snippet2, snippet3) + end + end + end + + describe 'only_include_projects_with_snippets_enabled' do + context 'when the include_private option is enabled' do + it 'includes snippets for projects with snippets set to private' do + project = create(:project) + + project.project_feature + .update(snippets_access_level: ProjectFeature::PRIVATE) + + snippet = create(:project_snippet, project: project) + + snippets = described_class + .only_include_projects_with_snippets_enabled(include_private: true) + + expect(snippets).to eq([snippet]) + end + end + + context 'when the include_private option is not enabled' do + it 'does not include snippets for projects that have snippets set to private' do + project = create(:project) + + project.project_feature + .update(snippets_access_level: ProjectFeature::PRIVATE) + + create(:project_snippet, project: project) + + snippets = described_class.only_include_projects_with_snippets_enabled + + expect(snippets).to be_empty + end + end + + it 'includes snippets for projects with snippets enabled' do + project = create(:project) + + project.project_feature + .update(snippets_access_level: ProjectFeature::ENABLED) + + snippet = create(:project_snippet, project: project) + snippets = described_class.only_include_projects_with_snippets_enabled + + expect(snippets).to eq([snippet]) + end + end + + describe '.only_include_authorized_projects' do + it 'only includes snippets for projects the user is authorized to see' do + user = create(:user) + project1 = create(:project, :private) + project2 = create(:project, :private) + + project1.team.add_developer(user) + + create(:project_snippet, project: project2) + + snippet = create(:project_snippet, project: project1) + snippets = described_class.only_include_authorized_projects(user) + + expect(snippets).to eq([snippet]) + end + end + + describe '.for_project_with_user' do + context 'when a user is provided' do + it 'returns an empty collection if the user can not view the snippets' do + project = create(:project, :private) + user = create(:user) + + project.project_feature + .update(snippets_access_level: ProjectFeature::ENABLED) + + create(:project_snippet, :public, project: project) + + expect(described_class.for_project_with_user(project, user)).to be_empty + end + + it 'returns the snippets if the user is a member of the project' do + project = create(:project, :private) + user = create(:user) + snippet = create(:project_snippet, project: project) + + project.team.add_developer(user) + + snippets = described_class.for_project_with_user(project, user) + + expect(snippets).to eq([snippet]) + end + + it 'returns public snippets for a public project the user is not a member of' do + project = create(:project, :public) + + project.project_feature + .update(snippets_access_level: ProjectFeature::ENABLED) + + user = create(:user) + snippet = create(:project_snippet, :public, project: project) + + create(:project_snippet, :private, project: project) + + snippets = described_class.for_project_with_user(project, user) + + expect(snippets).to eq([snippet]) + end + end + + context 'when a user is not provided' do + it 'returns an empty collection for a private project' do + project = create(:project, :private) + + project.project_feature + .update(snippets_access_level: ProjectFeature::ENABLED) + + create(:project_snippet, :public, project: project) + + expect(described_class.for_project_with_user(project)).to be_empty + end + + it 'returns public snippets for a public project' do + project = create(:project, :public) + snippet = create(:project_snippet, :public, project: project) + + project.project_feature + .update(snippets_access_level: ProjectFeature::PUBLIC) + + create(:project_snippet, :private, project: project) + + snippets = described_class.for_project_with_user(project) + + expect(snippets).to eq([snippet]) + end + end + end + + describe '.visible_to_or_authored_by' do + it 'returns snippets visible to the user' do + user = create(:user) + snippet1 = create(:snippet, :public) + snippet2 = create(:snippet, :private, author: user) + snippet3 = create(:snippet, :private) + + snippets = described_class.visible_to_or_authored_by(user) + + expect(snippets).to include(snippet1, snippet2) + expect(snippets).not_to include(snippet3) + end + end + describe '#participants' do let(:project) { create(:project, :public) } let(:snippet) { create(:snippet, content: 'foo', project: project) } |