diff --git a/.rubocop_todo/gitlab/bounded_contexts.yml b/.rubocop_todo/gitlab/bounded_contexts.yml index 793be23d4cb1a921692514958bb6f2edf8360c69..986fd6ea660fa4071c757f6b390aabeb904cd9e6 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 0e52bfa1a2f29810c5046fbea7734cf8bc2339e6..90eeace7530970e85c677db75c0b4060d7e2f259 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 41611517cd8dc725c1dffa1c91d965f1cf1fac95..7819e81f082fe43c54261afbc3cdf391de2050f9 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 7cc93e725a156cf963919ad2e093dbd40a5355e1..e4b6e21ecb4eaf3d90cb2cf97960034221d87cd5 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 401361e4f5dc22d7ac10adca66d9228b4914e233..c82f87a9716a3511f59870783972ad96a35cd286 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/lib/extracts_path.rb b/lib/extracts_path.rb index db5c3bb1d4aac16879be3dd5f090169931777d7c..f833295bd56e4401fe07908e5d74e587b7d88a31 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -3,23 +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 - - # 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 + 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 # 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. @@ -31,10 +18,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 +55,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 @@ -56,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 @@ -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 80d3f3888fe57302c16748f700169474a0103c58..0000000000000000000000000000000000000000 --- 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 c95f0073bca8ebff30eff7d87d3744407104220b..fb9587933991747b9108738a409f8feaa9680f8d 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 559a6544285877248c9a418cefc728a5d7a4c6a5..0000000000000000000000000000000000000000 --- 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 diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index fbbfc76e89d292fc865c2558d49b0d5ed074d813..96cc5d93b8d8f0450893a29e24f13f3ae2270d97 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'