From aeffa2a9669cd413d1df93f3b824d054d98e6a9c Mon Sep 17 00:00:00 2001 From: Matijs Date: Tue, 27 Nov 2018 11:09:42 +0000 Subject: [PATCH 01/85] Fix wrong example url (`repositories` instead of `repository`) --- doc/api/repository_submodules.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/repository_submodules.md b/doc/api/repository_submodules.md index 2e6797f18f4..11b04c81172 100644 --- a/doc/api/repository_submodules.md +++ b/doc/api/repository_submodules.md @@ -22,7 +22,7 @@ PUT /projects/:id/repository/submodules/:submodule | `commit_message` | string | no | Commit message. If no message is provided, a default one will be set | ```sh -curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/5/repositories/submodules/lib%2Fmodules%2Fexample" +curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/5/repository/submodules/lib%2Fmodules%2Fexample" --data "branch=master&commit_sha=3ddec28ea23acc5caa5d8331a6ecb2a65fc03e88&commit_message=Update submodule reference" ``` -- GitLab From 55a8e88ffcc459c9e6e0e01f0ff00d91f45a2b4a Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 27 Nov 2018 16:02:06 +0100 Subject: [PATCH 02/85] Reenable CODEOWNERS Automatically adding required approvals to merge reqeusts caused a lot of notifications. Since that was fixed, we can re-enable CODEOWNERS --- .gitlab/{CODEOWNERS.disabled => CODEOWNERS} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .gitlab/{CODEOWNERS.disabled => CODEOWNERS} (100%) diff --git a/.gitlab/CODEOWNERS.disabled b/.gitlab/CODEOWNERS similarity index 100% rename from .gitlab/CODEOWNERS.disabled rename to .gitlab/CODEOWNERS -- GitLab From 7fd5dbf9db4e513faaabbe14aff3c10fca603415 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 29 Nov 2018 12:52:48 +0000 Subject: [PATCH 03/85] Add a flag to use a subquery for group issues search We already had a flag to use a CTE, but this broke searching in some cases where we need to sort by a joined table. Disabling the CTE flag makes searches much slower. The new flag, to use a subquery, makes them slightly slower than the CTE, while maintaining correctness. If both it and the CTE flag are enabled, the subquery takes precedence. --- .../concerns/issuable_collections.rb | 2 +- app/finders/issuable_finder.rb | 47 +++++-- ...e-group-issues-search-cte-up-the-chain.yml | 5 + spec/controllers/groups_controller_spec.rb | 3 +- spec/finders/issues_finder_spec.rb | 127 ++++++++++++++++++ 5 files changed, 173 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 34a8c50fcbd..4aeec961f46 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -102,7 +102,7 @@ module IssuableCollections elsif @group options[:group_id] = @group.id options[:include_subgroups] = true - options[:use_cte_for_search] = true + options[:attempt_group_search_optimizations] = true end params.permit(finder_type.valid_params).merge(options) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index e04e3a2a7e0..247f2d373f1 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -27,12 +27,13 @@ # created_before: datetime # updated_after: datetime # updated_before: datetime -# use_cte_for_search: boolean +# attempt_group_search_optimizations: boolean # class IssuableFinder prepend FinderWithCrossProjectAccess include FinderMethods include CreatedAtFilter + include Gitlab::Utils::StrongMemoize requires_cross_project_access unless: -> { project? } @@ -75,8 +76,9 @@ class IssuableFinder items = init_collection items = filter_items(items) - # This has to be last as we may use a CTE as an optimization fence by - # passing the use_cte_for_search param + # This has to be last as we may use a CTE as an optimization fence + # by passing the attempt_group_search_optimizations param and + # enabling the use_cte_for_group_issues_search feature flag # https://www.postgresql.org/docs/current/static/queries-with.html items = by_search(items) @@ -85,6 +87,8 @@ class IssuableFinder def filter_items(items) items = by_project(items) + items = by_group(items) + items = by_subquery(items) items = by_scope(items) items = by_created_at(items) items = by_updated_at(items) @@ -282,12 +286,36 @@ class IssuableFinder end # rubocop: enable CodeReuse/ActiveRecord + def use_subquery_for_search? + strong_memoize(:use_subquery_for_search) do + if attempt_group_search_optimizations? + Feature.enabled?(:use_subquery_for_group_issues_search, default_enabled: false) + else + false + end + end + end + + def use_cte_for_search? + strong_memoize(:use_cte_for_search) do + if attempt_group_search_optimizations? && !use_subquery_for_search? + Feature.enabled?(:use_cte_for_group_issues_search, default_enabled: true) + else + false + end + end + end + private def init_collection klass.all end + def attempt_group_search_optimizations? + search && Gitlab::Database.postgresql? && params[:attempt_group_search_optimizations] + end + def count_key(value) Array(value).last.to_sym end @@ -351,12 +379,13 @@ class IssuableFinder end # rubocop: enable CodeReuse/ActiveRecord - def use_cte_for_search? - return false unless search - return false unless Gitlab::Database.postgresql? - return false unless Feature.enabled?(:use_cte_for_group_issues_search, default_enabled: true) - - params[:use_cte_for_search] + # Wrap projects and groups in a subquery if the conditions are met. + def by_subquery(items) + if use_subquery_for_search? + klass.where(id: items.select(:id)) # rubocop: disable CodeReuse/ActiveRecord + else + items + end end # rubocop: disable CodeReuse/ActiveRecord diff --git a/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml b/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml new file mode 100644 index 00000000000..0269e7b6196 --- /dev/null +++ b/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml @@ -0,0 +1,5 @@ +--- +title: Fix error when searching for group issues with priority or popularity sort +merge_request: 23445 +author: +type: fixed diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index f6c85102830..4b0dc4c9b69 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -226,9 +226,10 @@ describe GroupsController do end context 'searching' do - # Remove as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/52271 before do + # Remove in https://gitlab.com/gitlab-org/gitlab-ce/issues/54643 stub_feature_flags(use_cte_for_group_issues_search: false) + stub_feature_flags(use_subquery_for_group_issues_search: true) end it 'works with popularity sort' do diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 515f6f70b99..80f7232f282 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -640,4 +640,131 @@ describe IssuesFinder do end end end + + describe '#use_subquery_for_search?' do + let(:finder) { described_class.new(nil, params) } + + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + stub_feature_flags(use_subquery_for_group_issues_search: true) + end + + context 'when there is no search param' do + let(:params) { { attempt_group_search_optimizations: true } } + + it 'returns false' do + expect(finder.use_subquery_for_search?).to be_falsey + end + end + + context 'when the database is not Postgres' do + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } + + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + + it 'returns false' do + expect(finder.use_subquery_for_search?).to be_falsey + end + end + + context 'when the attempt_group_search_optimizations param is falsey' do + let(:params) { { search: 'foo' } } + + it 'returns false' do + expect(finder.use_subquery_for_search?).to be_falsey + end + end + + context 'when the use_subquery_for_group_issues_search flag is disabled' do + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } + + before do + stub_feature_flags(use_subquery_for_group_issues_search: false) + end + + it 'returns false' do + expect(finder.use_subquery_for_search?).to be_falsey + end + end + + context 'when all conditions are met' do + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } + + it 'returns true' do + expect(finder.use_subquery_for_search?).to be_truthy + end + end + end + + describe '#use_cte_for_search?' do + let(:finder) { described_class.new(nil, params) } + + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + stub_feature_flags(use_cte_for_group_issues_search: true) + stub_feature_flags(use_subquery_for_group_issues_search: false) + end + + context 'when there is no search param' do + let(:params) { { attempt_group_search_optimizations: true } } + + it 'returns false' do + expect(finder.use_cte_for_search?).to be_falsey + end + end + + context 'when the database is not Postgres' do + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } + + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + + it 'returns false' do + expect(finder.use_cte_for_search?).to be_falsey + end + end + + context 'when the attempt_group_search_optimizations param is falsey' do + let(:params) { { search: 'foo' } } + + it 'returns false' do + expect(finder.use_cte_for_search?).to be_falsey + end + end + + context 'when the use_cte_for_group_issues_search flag is disabled' do + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } + + before do + stub_feature_flags(use_cte_for_group_issues_search: false) + end + + it 'returns false' do + expect(finder.use_cte_for_search?).to be_falsey + end + end + + context 'when use_subquery_for_search? is true' do + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } + + before do + stub_feature_flags(use_subquery_for_group_issues_search: true) + end + + it 'returns false' do + expect(finder.use_cte_for_search?).to be_falsey + end + end + + context 'when all conditions are met' do + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } + + it 'returns true' do + expect(finder.use_cte_for_search?).to be_truthy + end + end + end end -- GitLab From 058de0d40127e20b3068d159e053539f1e353fc3 Mon Sep 17 00:00:00 2001 From: jhampton Date: Mon, 3 Dec 2018 11:56:49 -0500 Subject: [PATCH 04/85] Conditionally send variable values We only want to send trigger variable values if the user is a maintainer. --- app/serializers/trigger_variable_entity.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/serializers/trigger_variable_entity.rb b/app/serializers/trigger_variable_entity.rb index 56203113631..8845540cc30 100644 --- a/app/serializers/trigger_variable_entity.rb +++ b/app/serializers/trigger_variable_entity.rb @@ -3,5 +3,6 @@ class TriggerVariableEntity < Grape::Entity include RequestAwareEntity - expose :key, :value, :public + expose :key, :public + expose :value, if: ->(_, _) { request.project.team.maintainer?(request.current_user) } end -- GitLab From 9a14844ec0e677a5b4644e9b2bb8eec75532917b Mon Sep 17 00:00:00 2001 From: jhampton Date: Mon, 3 Dec 2018 11:57:40 -0500 Subject: [PATCH 05/85] Styles trigger variables block Adds new style to the trigger variables table. --- app/assets/stylesheets/pages/builds.scss | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss index 81cb519883b..39f6bda9520 100644 --- a/app/assets/stylesheets/pages/builds.scss +++ b/app/assets/stylesheets/pages/builds.scss @@ -228,6 +228,21 @@ padding: 16px 0; } + .trigger-variables-table { + td { + font-size: 12px; + line-height: 16px; + border: 1px solid #dfdfdf; + padding: 4px 6px; + width: 50%; + } + } + + .trigger-variables-btn-container { + display: flex; + justify-content: space-between; + } + .trigger-build-variables { margin: 0; overflow-x: auto; @@ -243,7 +258,6 @@ .trigger-build-value { padding: 2px 4px; color: $black; - background-color: $white-light; } .badge.badge-pill { -- GitLab From aaf22b668f5a2489f9729f445eb4eaa769036446 Mon Sep 17 00:00:00 2001 From: jhampton Date: Mon, 3 Dec 2018 14:29:05 -0500 Subject: [PATCH 06/85] Conditionally display variable values We want to hide trigger varialbes values in the UI by default. A toggle button will be available to maintainers. --- .../jobs/components/trigger_block.vue | 76 +++++++++++++------ 1 file changed, 51 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/jobs/components/trigger_block.vue b/app/assets/javascripts/jobs/components/trigger_block.vue index 4a9b2903eec..8aded3192f7 100644 --- a/app/assets/javascripts/jobs/components/trigger_block.vue +++ b/app/assets/javascripts/jobs/components/trigger_block.vue @@ -1,6 +1,12 @@