From 56ebd15e2965297d6989ab8fe2b7a668f12817be Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Tue, 4 Jun 2024 11:17:25 +0200 Subject: [PATCH 1/3] Merge ExtractsRef into ExtractsPath Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/425379 **Problem** ExtractsRef is used only by ExtractsPath. The separation of the code between two modules complicates the logic. **Solution** Merge ExtractsRef code into ExtractsPath. It should simplify further refactoring attempts to get rid of ExtractsPath module. Changelog: other --- lib/extracts_path.rb | 40 +++++++++++-- lib/extracts_ref.rb | 61 -------------------- spec/lib/extracts_path_spec.rb | 65 ++++++++++++++++++++- spec/lib/extracts_ref_spec.rb | 101 --------------------------------- 4 files changed, 99 insertions(+), 168 deletions(-) delete mode 100644 lib/extracts_ref.rb delete mode 100644 spec/lib/extracts_ref_spec.rb diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb index db5c3bb1d4aac1..ef2122c03617df 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -3,8 +3,10 @@ # Module providing methods for dealing with separating a tree-ish string and a # file path string when combined in a request parameter module ExtractsPath - extend ::Gitlab::Utils::Override - include ExtractsRef + InvalidPathError = ExtractsRef::RefExtractor::InvalidPathError + BRANCH_REF_TYPE = ExtractsRef::RefExtractor::BRANCH_REF_TYPE + TAG_REF_TYPE = ExtractsRef::RefExtractor::TAG_REF_TYPE + REF_TYPES = ExtractsRef::RefExtractor::REF_TYPES # If we have an ID of 'foo.atom', and the controller provides Atom and HTML # formats, then we have to check if the request was for the Atom version of @@ -31,10 +33,29 @@ def extract_ref_without_atom(id) # Automatically redirects to the current default branch if the ref matches a # previous default branch that has subsequently been deleted. # + # Assignments are: + # + # - @id - A string representing the joined ref and path + # - @ref - A string representing the ref (e.g., the branch, tag, or commit SHA) + # - @path - A string representing the filesystem path + # - @commit - A Commit representing the commit from the given ref + # + # If the :id parameter appears to be requesting a specific response format, + # that will be handled as well. # rubocop:disable Gitlab/ModuleWithInstanceVariables - override :assign_ref_vars def assign_ref_vars - super + ref_extractor = ExtractsRef::RefExtractor.new(repository_container, params.permit(:id, :ref, :path, :ref_type)) + ref_extractor.extract! + + @id = ref_extractor.id + @ref = ref_extractor.ref + @path = ref_extractor.path + @repo = ref_extractor.repo + + return unless @ref.present? + + @commit = ref_extractor.commit + @fully_qualified_ref = ref_extractor.fully_qualified_ref rectify_atom! @@ -49,6 +70,10 @@ def assign_ref_vars end # rubocop:enable Gitlab/ModuleWithInstanceVariables + def ref_type + ExtractsRef::RefExtractor.ref_type(params[:ref_type]) + end + private # Override in controllers to determine which actions are subject to the redirect @@ -87,7 +112,12 @@ def rectify_renamed_default_branch! end # rubocop:enable Gitlab/ModuleWithInstanceVariables - override :repository_container + def ref_names + return [] unless repository_container + + @ref_names ||= repository_container.repository.ref_names # rubocop:disable Gitlab/ModuleWithInstanceVariables -- will be fixed in https://gitlab.com/gitlab-org/gitlab/-/issues/425379 + end + def repository_container @project end diff --git a/lib/extracts_ref.rb b/lib/extracts_ref.rb deleted file mode 100644 index 80d3f3888fe573..00000000000000 --- a/lib/extracts_ref.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -# TOOD: https://gitlab.com/gitlab-org/gitlab/-/issues/425379 -# WARNING: This module has been deprecated. -# The module solely exists because ExtractsPath depends on this module (ExtractsPath is the only user.) -# ExtractsRef::RefExtractor class is a refactored version of this module and provides -# the same functionalities. You should use the class instead. -# -# Module providing methods for dealing with separating a tree-ish string and a -# file path string when combined in a request parameter -# Can be extended for different types of repository object, e.g. Project or Snippet -module ExtractsRef - InvalidPathError = ExtractsRef::RefExtractor::InvalidPathError - BRANCH_REF_TYPE = ExtractsRef::RefExtractor::BRANCH_REF_TYPE - TAG_REF_TYPE = ExtractsRef::RefExtractor::TAG_REF_TYPE - REF_TYPES = ExtractsRef::RefExtractor::REF_TYPES - - # Assigns common instance variables for views working with Git tree-ish objects - # - # Assignments are: - # - # - @id - A string representing the joined ref and path - # - @ref - A string representing the ref (e.g., the branch, tag, or commit SHA) - # - @path - A string representing the filesystem path - # - @commit - A Commit representing the commit from the given ref - # - # If the :id parameter appears to be requesting a specific response format, - # that will be handled as well. - # rubocop:disable Gitlab/ModuleWithInstanceVariables - def assign_ref_vars - ref_extractor = ExtractsRef::RefExtractor.new(repository_container, params.permit(:id, :ref, :path, :ref_type)) - ref_extractor.extract! - - @id = ref_extractor.id - @ref = ref_extractor.ref - @path = ref_extractor.path - @repo = ref_extractor.repo - - return unless @ref.present? - - @commit = ref_extractor.commit - @fully_qualified_ref = ref_extractor.fully_qualified_ref - end - # rubocop:enable Gitlab/ModuleWithInstanceVariables - - def ref_type - ExtractsRef::RefExtractor.ref_type(params[:ref_type]) - end - - private - - def ref_names - return [] unless repository_container - - @ref_names ||= repository_container.repository.ref_names # rubocop:disable Gitlab/ModuleWithInstanceVariables - end - - def repository_container - raise NotImplementedError - end -end diff --git a/spec/lib/extracts_path_spec.rb b/spec/lib/extracts_path_spec.rb index c95f0073bca8eb..fb958793399174 100644 --- a/spec/lib/extracts_path_spec.rb +++ b/spec/lib/extracts_path_spec.rb @@ -36,6 +36,28 @@ def default_url_options it_behaves_like 'assigns ref vars' + context 'when ref and path have incorrect format' do + let(:ref) { { wrong: :format } } + let(:path) { { also: :wrong } } + + it 'does not raise an exception' do + expect { assign_ref_vars }.not_to raise_error + end + end + + context 'when a ref_type parameter is provided' do + let(:params) { ActionController::Parameters.new(path: path, ref: ref, ref_type: 'tags') } + + it 'sets a fully_qualified_ref variable' do + fully_qualified_ref = "refs/tags/#{ref}" + expect(container.repository).to receive(:commit).with(fully_qualified_ref) + expect(self).to receive(:render_404) + + assign_ref_vars + expect(@fully_qualified_ref).to eq(fully_qualified_ref) + end + end + it 'log tree path has no escape sequences' do assign_ref_vars @@ -134,7 +156,6 @@ def default_url_options it 'does not set commit' do expect(container.repository).not_to receive(:commit).with('') - expect(self).to receive(:render_404) assign_ref_vars @@ -203,6 +224,48 @@ def default_url_options end end + describe '#ref_type' do + subject { ref_type } + + let(:params) { ActionController::Parameters.new(ref_type: ref) } + + context 'when ref_type is nil' do + let(:ref) { nil } + + it { is_expected.to eq(nil) } + end + + context 'when ref_type is heads' do + let(:ref) { 'heads' } + + it { is_expected.to eq('heads') } + end + + context 'when ref_type is tags' do + let(:ref) { 'tags' } + + it { is_expected.to eq('tags') } + end + + context 'when case does not match' do + let(:ref) { 'tAgS' } + + it { is_expected.to(eq('tags')) } + end + + context 'when ref_type is invalid' do + let(:ref) { 'invalid' } + + it { is_expected.to eq(nil) } + end + + context 'when ref_type is a hash' do + let(:ref) { { 'just' => 'hash' } } + + it { is_expected.to eq(nil) } + end + end + describe '#extract_ref_without_atom' do it 'ignores any matching refs suffixed with atom' do expect(extract_ref_without_atom('master.atom')).to eq('master') diff --git a/spec/lib/extracts_ref_spec.rb b/spec/lib/extracts_ref_spec.rb deleted file mode 100644 index 559a6544285877..00000000000000 --- a/spec/lib/extracts_ref_spec.rb +++ /dev/null @@ -1,101 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ExtractsRef, feature_category: :source_code_management do - include described_class - include RepoHelpers - - let_it_be(:owner) { create(:user) } - let_it_be(:container) { create(:snippet, :repository, author: owner) } - - let(:ref) { sample_commit[:id] } - let(:path) { sample_commit[:line_code_path] } - let(:params) { ActionController::Parameters.new(path: path, ref: ref) } - - before do - ref_names = ['master', 'foo/bar/baz', 'v1.0.0', 'v2.0.0', 'release/app', 'release/app/v1.0.0'] - - allow(container.repository).to receive(:ref_names).and_return(ref_names) - allow_any_instance_of(described_class).to receive(:repository_container).and_return(container) - end - - describe '#assign_ref_vars' do - it_behaves_like 'assigns ref vars' - - context 'ref and path are nil' do - let(:ref) { nil } - let(:path) { nil } - - it 'does not set commit' do - expect(container.repository).not_to receive(:commit).with('') - - assign_ref_vars - - expect(@commit).to be_nil - end - end - - context 'when ref and path have incorrect format' do - let(:ref) { { wrong: :format } } - let(:path) { { also: :wrong } } - - it 'does not raise an exception' do - expect { assign_ref_vars }.not_to raise_error - end - end - - context 'when a ref_type parameter is provided' do - let(:params) { ActionController::Parameters.new(path: path, ref: ref, ref_type: 'tags') } - - it 'sets a fully_qualified_ref variable' do - fully_qualified_ref = "refs/tags/#{ref}" - expect(container.repository).to receive(:commit).with(fully_qualified_ref) - assign_ref_vars - expect(@fully_qualified_ref).to eq(fully_qualified_ref) - end - end - end - - describe '#ref_type' do - subject { ref_type } - - let(:params) { ActionController::Parameters.new(ref_type: ref) } - - context 'when ref_type is nil' do - let(:ref) { nil } - - it { is_expected.to eq(nil) } - end - - context 'when ref_type is heads' do - let(:ref) { 'heads' } - - it { is_expected.to eq('heads') } - end - - context 'when ref_type is tags' do - let(:ref) { 'tags' } - - it { is_expected.to eq('tags') } - end - - context 'when case does not match' do - let(:ref) { 'tAgS' } - - it { is_expected.to(eq('tags')) } - end - - context 'when ref_type is invalid' do - let(:ref) { 'invalid' } - - it { is_expected.to eq(nil) } - end - - context 'when ref_type is a hash' do - let(:ref) { { 'just' => 'hash' } } - - it { is_expected.to eq(nil) } - end - end -end -- GitLab From 987be2889ddf8def3d206250a77e6fcff2fd555f Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Tue, 4 Jun 2024 11:24:38 +0200 Subject: [PATCH 2/3] Update rubocop rules --- .rubocop_todo/gitlab/bounded_contexts.yml | 1 - .rubocop_todo/rspec/any_instance_of.yml | 1 - .rubocop_todo/rspec/context_wording.yml | 1 - .rubocop_todo/rspec/instance_variable.yml | 1 - .rubocop_todo/style/inline_disable_annotation.yml | 1 - spec/support/rspec_order_todo.yml | 1 - 6 files changed, 6 deletions(-) diff --git a/.rubocop_todo/gitlab/bounded_contexts.yml b/.rubocop_todo/gitlab/bounded_contexts.yml index 793be23d4cb1a9..986fd6ea660fa4 100644 --- a/.rubocop_todo/gitlab/bounded_contexts.yml +++ b/.rubocop_todo/gitlab/bounded_contexts.yml @@ -4207,7 +4207,6 @@ Gitlab/BoundedContexts: - 'lib/event_filter.rb' - 'lib/expand_variables.rb' - 'lib/extracts_path.rb' - - 'lib/extracts_ref.rb' - 'lib/extracts_ref/ref_extractor.rb' - 'lib/extracts_ref/requested_ref.rb' - 'lib/feature.rb' diff --git a/.rubocop_todo/rspec/any_instance_of.yml b/.rubocop_todo/rspec/any_instance_of.yml index 0e52bfa1a2f298..90eeace7530970 100644 --- a/.rubocop_todo/rspec/any_instance_of.yml +++ b/.rubocop_todo/rspec/any_instance_of.yml @@ -120,7 +120,6 @@ RSpec/AnyInstanceOf: - 'spec/lib/banzai/filter/references/issue_reference_filter_spec.rb' - 'spec/lib/banzai/filter/repository_link_filter_spec.rb' - 'spec/lib/banzai/pipeline/gfm_pipeline_spec.rb' - - 'spec/lib/extracts_ref_spec.rb' - 'spec/lib/feature_spec.rb' - 'spec/lib/gitlab/app_logger_spec.rb' - 'spec/lib/gitlab/asciidoc_spec.rb' diff --git a/.rubocop_todo/rspec/context_wording.yml b/.rubocop_todo/rspec/context_wording.yml index 41611517cd8dc7..7819e81f082fe4 100644 --- a/.rubocop_todo/rspec/context_wording.yml +++ b/.rubocop_todo/rspec/context_wording.yml @@ -1490,7 +1490,6 @@ RSpec/ContextWording: - 'spec/lib/error_tracking/sentry_client/projects_spec.rb' - 'spec/lib/expand_variables_spec.rb' - 'spec/lib/extracts_path_spec.rb' - - 'spec/lib/extracts_ref_spec.rb' - 'spec/lib/feature_spec.rb' - 'spec/lib/gitlab/alert_management/fingerprint_spec.rb' - 'spec/lib/gitlab/analytics/cycle_analytics/average_spec.rb' diff --git a/.rubocop_todo/rspec/instance_variable.yml b/.rubocop_todo/rspec/instance_variable.yml index 7cc93e725a156c..e4b6e21ecb4eaf 100644 --- a/.rubocop_todo/rspec/instance_variable.yml +++ b/.rubocop_todo/rspec/instance_variable.yml @@ -77,7 +77,6 @@ RSpec/InstanceVariable: - 'spec/lib/api/helpers/authentication_spec.rb' - 'spec/lib/banzai/filter/asset_proxy_filter_spec.rb' - 'spec/lib/extracts_path_spec.rb' - - 'spec/lib/extracts_ref_spec.rb' - 'spec/lib/gitlab/auth/auth_finders_spec.rb' - 'spec/lib/gitlab/auth/ldap/person_spec.rb' - 'spec/lib/gitlab/chat_name_token_spec.rb' diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml index 401361e4f5dc22..c82f87a9716a35 100644 --- a/.rubocop_todo/style/inline_disable_annotation.yml +++ b/.rubocop_todo/style/inline_disable_annotation.yml @@ -2058,7 +2058,6 @@ Style/InlineDisableAnnotation: - 'lib/declarative_enum.rb' - 'lib/event_filter.rb' - 'lib/extracts_path.rb' - - 'lib/extracts_ref.rb' - 'lib/feature.rb' - 'lib/file_size_validator.rb' - 'lib/gem_extensions/active_record/association.rb' diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index fbbfc76e89d292..96cc5d93b8d8f0 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -5091,7 +5091,6 @@ - './spec/lib/event_filter_spec.rb' - './spec/lib/expand_variables_spec.rb' - './spec/lib/extracts_path_spec.rb' -- './spec/lib/extracts_ref_spec.rb' - './spec/lib/feature/definition_spec.rb' - './spec/lib/feature/gitaly_spec.rb' - './spec/lib/feature_spec.rb' -- GitLab From 5808457a6dfabee86a7ed75122543756a334f1e9 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Fri, 7 Jun 2024 17:23:51 +0200 Subject: [PATCH 3/3] Move "extract_ref_without_atom" into private scope --- lib/extracts_path.rb | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb index ef2122c03617df..f833295bd56e44 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -8,21 +8,6 @@ module ExtractsPath TAG_REF_TYPE = ExtractsRef::RefExtractor::TAG_REF_TYPE REF_TYPES = ExtractsRef::RefExtractor::REF_TYPES - # If we have an ID of 'foo.atom', and the controller provides Atom and HTML - # formats, then we have to check if the request was for the Atom version of - # the ID without the '.atom' suffix, or the HTML version of the ID including - # the suffix. We only check this if the version including the suffix doesn't - # match, so it is possible to create a branch which has an unroutable Atom - # feed. - def extract_ref_without_atom(id) - id_without_atom = id.sub(/\.atom$/, '') - valid_refs = ref_names.select { |v| "#{id_without_atom}/".start_with?("#{v}/") } - - raise InvalidPathError if valid_refs.blank? - - valid_refs.max_by(&:length) - end - # Extends the method to handle if there is no path and the ref doesn't # exist in the repo, try to resolve the ref without an '.atom' suffix. # If _that_ ref is found, set the request's format to Atom manually. @@ -81,6 +66,21 @@ def redirect_renamed_default_branch? false end + # If we have an ID of 'foo.atom', and the controller provides Atom and HTML + # formats, then we have to check if the request was for the Atom version of + # the ID without the '.atom' suffix, or the HTML version of the ID including + # the suffix. We only check this if the version including the suffix doesn't + # match, so it is possible to create a branch which has an unroutable Atom + # feed. + def extract_ref_without_atom(id) + id_without_atom = id.sub(/\.atom$/, '') + valid_refs = ref_names.select { |v| "#{id_without_atom}/".start_with?("#{v}/") } + + raise InvalidPathError if valid_refs.blank? + + valid_refs.max_by(&:length) + end + # rubocop:disable Gitlab/ModuleWithInstanceVariables def rectify_atom! return if @commit -- GitLab