From b7875d02e15f931c329959b4e3490dbe69fa92ed Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Fri, 26 Jul 2024 22:11:02 +0100 Subject: [PATCH 1/2] Add MR mergeability check for locked paths This change introduces a new check. This check will prevent merging MRs which include locked paths. We also have a check which runs when a user pushes code that prevents pushing paths that are locked by other users. This change will allow us to enable the fix for https://gitlab.com/gitlab-org/gitlab/-/issues/23625 Related to https://gitlab.com/gitlab-org/gitlab/-/issues/428247 Changelog: added # Conflicts: # app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js # app/graphql/types/merge_requests/detailed_merge_status_enum.rb # app/models/merge_request.rb # doc/api/graphql/reference/index.md # doc/api/merge_requests.md # ee/app/models/ee/merge_request.rb --- .../components/checks/constants.js | 1 + .../detailed_merge_status_enum.rb | 3 + app/models/merge_request.rb | 2 + doc/api/graphql/reference/index.md | 2 + doc/api/merge_requests.md | 1 + ee/app/models/ee/merge_request.rb | 3 +- ee/app/models/path_lock.rb | 1 + .../mergeability/check_path_locks_service.rb | 59 ++++++++ .../beta/locked_paths_mergeability_check.yml | 9 ++ ee/spec/models/merge_request_spec.rb | 8 +- .../check_path_locks_service_spec.rb | 142 ++++++++++++++++++ locale/gitlab.pot | 3 + 12 files changed, 229 insertions(+), 5 deletions(-) create mode 100644 ee/app/services/merge_requests/mergeability/check_path_locks_service.rb create mode 100644 ee/config/feature_flags/beta/locked_paths_mergeability_check.yml create mode 100644 ee/spec/services/merge_requests/mergeability/check_path_locks_service_spec.rb diff --git a/app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js b/app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js index 27a9c492d1dd9d..5a69476b56eabf 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js @@ -24,6 +24,7 @@ export const FAILURE_REASONS = { jira_association_missing: __('Either the title or description must reference a Jira issue.'), requested_changes: __('The change requests must be completed or resolved.'), approvals_syncing: __('The merge request approvals are currently syncing.'), + locked_paths: __('All paths must be unlocked'), locked_lfs_files: __('All LFS files must be unlocked.'), security_policy_evaluation: __('All security policies must be evaluated.'), }; diff --git a/app/graphql/types/merge_requests/detailed_merge_status_enum.rb b/app/graphql/types/merge_requests/detailed_merge_status_enum.rb index 1d17f38cc6fff8..ce934458aec842 100644 --- a/app/graphql/types/merge_requests/detailed_merge_status_enum.rb +++ b/app/graphql/types/merge_requests/detailed_merge_status_enum.rb @@ -57,6 +57,9 @@ class DetailedMergeStatusEnum < BaseEnum value 'APPROVALS_SYNCING', value: :approvals_syncing, description: 'Merge request approvals currently syncing.' + value 'LOCKED_PATHS', + value: :locked_paths, + description: 'Merge request includes locked paths.' value 'LOCKED_LFS_FILES', value: :locked_lfs_files, description: 'Merge request includes locked LFS files.' diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f7a4e80cceee1b..1ccac0ffaaac83 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1278,6 +1278,7 @@ def skipped_mergeable_checks(options = {}) skip_discussions_check: merge_when_checks_pass_strat, skip_external_status_check: merge_when_checks_pass_strat, skip_requested_changes_check: merge_when_checks_pass_strat, + skip_locked_paths_check: merge_when_checks_pass_strat, skip_jira_check: merge_when_checks_pass_strat, skip_locked_lfs_files_check: merge_when_checks_pass_strat, skip_security_policy_check: merge_when_checks_pass_strat @@ -1292,6 +1293,7 @@ def skipped_mergeable_checks(options = {}) # skip_blocked_check # skip_external_status_check # skip_requested_changes_check + # skip_locked_paths_check # skip_jira_check # skip_locked_lfs_files_check def mergeable?(check_mergeability_retry_lease: false, skip_rebase_check: false, **mergeable_state_check_params) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 12be1235faa7cd..9136b784dfacc8 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -35338,6 +35338,7 @@ Detailed representation of whether a GitLab merge request can be merged. | `EXTERNAL_STATUS_CHECKS` | Status checks must pass. | | `JIRA_ASSOCIATION` | Either the title or description must reference a Jira issue. | | `LOCKED_LFS_FILES` | Merge request includes locked LFS files. | +| `LOCKED_PATHS` | Merge request includes locked paths. | | `MERGEABLE` | Branch can be merged. | | `NEED_REBASE` | Merge request needs to be rebased. | | `NOT_APPROVED` | Merge request must be approved before merging. | @@ -36115,6 +36116,7 @@ Representation of mergeability check identifier. | `DRAFT_STATUS` | Checks whether the merge request is draft. | | `JIRA_ASSOCIATION_MISSING` | Checks whether the title or description references a Jira issue. | | `LOCKED_LFS_FILES` | Checks whether the merge request contains locked LFS files that are locked by users other than the merge request author. | +| `LOCKED_PATHS` | Checks whether the merge request contains locked paths. | | `MERGE_REQUEST_BLOCKED` | Checks whether the merge request is blocked. | | `NEED_REBASE` | Checks whether the merge request needs to be rebased. | | `NOT_APPROVED` | Checks whether the merge request is approved. | diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 8e406cd4e9288f..937e3492d714ba 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -909,6 +909,7 @@ Use `detailed_merge_status` instead of `merge_status` to account for all potenti - `not_open`: The merge request must be open before merge. - `requested_changes`: The merge request has reviewers who have requested changes. - `unchecked`: Git has not yet tested if a valid merge is possible. + - `locked_paths`: Paths locked by other users must be unlocked before merging to default branch. - `locked_lfs_files`: LFS files locked by other users must be unlocked before merge. ### Preparation steps diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index c1b06fdd08d327..c018ad9c9d8a4b 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -210,7 +210,8 @@ def mergeable_state_checks ::MergeRequests::Mergeability::CheckBlockedByOtherMrsService, ::MergeRequests::Mergeability::CheckJiraStatusService, ::MergeRequests::Mergeability::CheckSecurityPolicyEvaluationService, - ::MergeRequests::Mergeability::CheckExternalStatusChecksPassedService + ::MergeRequests::Mergeability::CheckExternalStatusChecksPassedService, + ::MergeRequests::Mergeability::CheckPathLocksService ] + super end end diff --git a/ee/app/models/path_lock.rb b/ee/app/models/path_lock.rb index 9837a339ec62a0..724f9928d65f2f 100644 --- a/ee/app/models/path_lock.rb +++ b/ee/app/models/path_lock.rb @@ -10,6 +10,7 @@ class PathLock < ApplicationRecord validate :path_unique_validation scope :for_paths, ->(paths) { where(path: paths) } + scope :not_for_users, ->(user_ids) { where.not(user_id: user_ids) } def downstream?(path) self.path.start_with?(path) && !exact?(path) diff --git a/ee/app/services/merge_requests/mergeability/check_path_locks_service.rb b/ee/app/services/merge_requests/mergeability/check_path_locks_service.rb new file mode 100644 index 00000000000000..4941fdbc0633e7 --- /dev/null +++ b/ee/app/services/merge_requests/mergeability/check_path_locks_service.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module MergeRequests + module Mergeability + class CheckPathLocksService < CheckBaseService + include ::Gitlab::Utils::StrongMemoize + + identifier :locked_paths + description 'Checks whether the merge request contains locked paths' + + CACHE_KEY = 'merge_request:%{id}:%{sha}:path_locks_mergeability:%{epoch}' + + def execute + return inactive if check_inactive? + return failure if contains_locked_paths? + + success + end + + def skip? + params[:skip_locked_paths_check].present? + end + + def cacheable? + true + end + + def cache_key + # If the feature is disabled we will return inactive so we don't need + # to link the cache key to a specific MR. + return 'inactive_path_locks_mergeability_check' if check_inactive? + + # Cache is linked to a specific MR + id = merge_request.id + # Cache is invalidated when new changes are added + sha = merge_request.diff_head_sha + # Cache is invalidated when path_locks are added or removed + epoch = project.path_locks_changed_epoch + + format(CACHE_KEY, id: id, sha: sha, epoch: epoch) + end + + private + + delegate :project, :changed_paths, :author_id, to: :merge_request + + def contains_locked_paths? + project.path_locks.for_paths(changed_paths.map(&:path)).not_for_users(author_id).exists? + end + + def check_inactive? + ::Feature.disabled?(:locked_paths_mergeability_check, project) || + !project.licensed_feature_available?(:file_locks) || + merge_request.target_branch != project.default_branch + end + strong_memoize_attr :check_inactive? + end + end +end diff --git a/ee/config/feature_flags/beta/locked_paths_mergeability_check.yml b/ee/config/feature_flags/beta/locked_paths_mergeability_check.yml new file mode 100644 index 00000000000000..3f81db81aa4c07 --- /dev/null +++ b/ee/config/feature_flags/beta/locked_paths_mergeability_check.yml @@ -0,0 +1,9 @@ +--- +name: locked_paths_mergeability_check +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/428247 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/160929 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/474705 +milestone: '17.3' +group: group::source code +type: beta +default_enabled: false diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index a53596359bc6d1..50f6a7fbbc2294 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -1588,10 +1588,10 @@ let(:options) { { auto_merge_strategy: auto_merge_strategy } } - where(:auto_merge_strategy, :skip_approved_check, :skip_draft_check, :skip_blocked_check, :skip_discussions_check, :skip_external_status_check) do - '' | false | false | false | false | false - AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS | false | false | false | false | false - AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS | true | true | true | true | true + where(:auto_merge_strategy, :skip_approved_check, :skip_draft_check, :skip_blocked_check, :skip_discussions_check, :skip_external_status_check, :skip_locked_paths_check) do + '' | false | false | false | false | false | false + AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS | false | false | false | false | false | false + AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS | true | true | true | true | true | true end with_them do diff --git a/ee/spec/services/merge_requests/mergeability/check_path_locks_service_spec.rb b/ee/spec/services/merge_requests/mergeability/check_path_locks_service_spec.rb new file mode 100644 index 00000000000000..b2f05d8ac0ab3b --- /dev/null +++ b/ee/spec/services/merge_requests/mergeability/check_path_locks_service_spec.rb @@ -0,0 +1,142 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckPathLocksService, feature_category: :code_review_workflow do + subject(:check_path_locks) { described_class.new(merge_request: merge_request, params: params) } + + let_it_be(:project) { create(:project) } + let_it_be(:merge_request) { build(:merge_request, source_project: project) } + let(:params) { { skip_locked_paths_check: skip_check } } + let(:skip_check) { false } + let(:feature_flag_enabled) { true } + let(:file_locks_enabled) { true } + let(:target_branch) { project.default_branch } + + before do + stub_feature_flags(locked_paths_mergeability_check: feature_flag_enabled) + stub_licensed_features(file_locks: file_locks_enabled) + allow(merge_request).to receive(:target_branch).and_return(target_branch) + end + + it_behaves_like 'mergeability check service', + :locked_paths, 'Checks whether the merge request contains locked paths' + + describe '#execute' do + subject(:execute) { check_path_locks.execute } + + context 'when file locks is enabled' do + let(:only_allow_merge_if_pipeline_succeeds) { true } + let(:changed_path) { instance_double('Gitlab::Git::ChangedPath', path: 'README.md') } + + before do + allow(merge_request).to receive(:changed_paths).and_return([changed_path]) + end + + context 'when there are no path locks for this project' do + it 'returns a check result with status success' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when there are paths locked by the merge request author' do + let(:user) { create(:user) } + + before do + allow(merge_request).to receive(:author_id).and_return(user.id) + create(:path_lock, project: project, path: changed_path.path, user: user) + end + + it 'returns a check result with status success' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when there are paths locked by another user' do + before do + allow(merge_request).to receive(:author_id).and_return(0) + create(:path_lock, project: project, path: changed_path.path) + end + + it 'returns a check result with status failure' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + end + + context 'when file locks is not enabled' do + let(:file_locks_enabled) { false } + + it 'returns a check result with inactive status' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::INACTIVE_STATUS + end + end + + context 'when the target branch is not the default branch' do + let(:target_branch) { 'not_the_default_branch' } + + it 'returns a check result with inactive status' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::INACTIVE_STATUS + end + end + end + + describe '#skip?' do + subject(:skip) { check_path_locks.skip? } + + context 'when skip check is true' do + let(:skip_check) { true } + + it { expect(skip).to eq(true) } + end + + context 'when skip check is false' do + let(:skip_check) { false } + + it { expect(skip).to eq(false) } + end + end + + describe '#cacheable?' do + subject(:cacheable) { check_path_locks.cacheable? } + + it { expect(cacheable).to eq(true) } + end + + describe '#cache_key' do + subject(:cache_key) { check_path_locks.cache_key } + + context 'when the feature flag is enabled' do + let(:id) { 'id' } + let(:sha) { 'sha' } + let(:epoch) { 'epoch' } + let(:expected_cache_key) { format(described_class::CACHE_KEY, id: id, sha: sha, epoch: epoch) } + + before do + allow(merge_request).to receive(:id).and_return(id) + allow(merge_request).to receive(:diff_head_sha).and_return(sha) + allow(project).to receive(:path_locks_changed_epoch).and_return(epoch) + end + + it { expect(cache_key).to eq(expected_cache_key) } + end + + context 'when the feature flag is disabled' do + let(:feature_flag_enabled) { false } + + it { expect(cache_key).to eq('inactive_path_locks_mergeability_check') } + end + + context 'when file locks is disabled' do + let(:file_locks_enabled) { false } + + it { expect(cache_key).to eq('inactive_path_locks_mergeability_check') } + end + + context 'when target_branch is not the default_branch' do + let(:target_branch) { 'not_the_default_branch' } + + it { expect(cache_key).to eq('inactive_path_locks_mergeability_check') } + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7f9b64e4f7a613..616aac21fee4ce 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5440,6 +5440,9 @@ msgstr "" msgid "All paths are relative to the GitLab URL. Do not include %{relative_url_link_start}relative URLs%{relative_url_link_end}." msgstr "" +msgid "All paths must be unlocked" +msgstr "" + msgid "All project members" msgstr "" -- GitLab From d43ae84e3efa728f2f4ad941824ddd7ce8bec96b Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Thu, 8 Aug 2024 11:55:52 +1000 Subject: [PATCH 2/2] Adjust N+1 tolerance by an additional query --- ee/spec/graphql/ee/types/merge_request_type_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ee/spec/graphql/ee/types/merge_request_type_spec.rb b/ee/spec/graphql/ee/types/merge_request_type_spec.rb index 6536a514024594..5ba3b01a2a7b1b 100644 --- a/ee/spec/graphql/ee/types/merge_request_type_spec.rb +++ b/ee/spec/graphql/ee/types/merge_request_type_spec.rb @@ -110,8 +110,10 @@ # Clear batch loader cache to ensure there's no N+1 if batch loading isn't cached. BatchLoader::Executor.clear_current - # +1 as we do an extra query for setting retreival - expect { GitlabSchema.execute(query, context: { current_user: user }) }.not_to exceed_query_limit(control).with_threshold(1) + # +2 as we do extra queries for: + # MergeRequests::Mergeability::CheckSecurityPolicyEvaluationService + # MergeRequests::Mergeability::CheckPathLocksService + expect { GitlabSchema.execute(query, context: { current_user: user }) }.not_to exceed_query_limit(control).with_threshold(2) end end -- GitLab