Merge branch 'separate-validator-files' into 'master'

Separate custom validators into own class files

Closes #209876

See merge request gitlab-org/gitlab!28266

changelogs/unreleased/separate-validator-files.yml

+---
+title: Separate validators into own class files
+merge_request: 28266
+author: Rajendra Kadam
+type: added

config/initializers/grape_validators.rb

+# frozen_string_literal: true
+
+Grape::Validations.register_validator(:absence, ::API::Validations::Validators::Absence)
+Grape::Validations.register_validator(:file_path, ::API::Validations::Validators::FilePath)
+Grape::Validations.register_validator(:git_ref, ::API::Validations::Validators::GitRef)
+Grape::Validations.register_validator(:git_sha, ::API::Validations::Validators::GitSha)
+Grape::Validations.register_validator(:integer_none_any, ::API::Validations::Validators::IntegerNoneAny)
+Grape::Validations.register_validator(:array_none_any, ::API::Validations::Validators::ArrayNoneAny)
+Grape::Validations.register_validator(:check_assignees_count, ::API::Validations::Validators::CheckAssigneesCount)

ee/lib/ee/api/validations/check_assignees_count.rb

-# frozen_string_literal: true
-
-module EE
-  module API
-    module Validations
-      module CheckAssigneesCount
-        extend ActiveSupport::Concern
-        extend ::Gitlab::Utils::Override
-
-        private
-
-        override :param_allowed?
-        def param_allowed?(attr_name, params)
-          super || License.feature_available?(:multiple_issue_assignees)
-        end
-      end
-    end
-  end
-end

ee/lib/ee/api/validations/validators/check_assignees_count.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Validations
+      module Validators
+        module CheckAssigneesCount
+          extend ActiveSupport::Concern
+          extend ::Gitlab::Utils::Override
+
+          private
+
+          override :param_allowed?
+          def param_allowed?(attr_name, params)
+            super || License.feature_available?(:multiple_issue_assignees)
+          end
+        end
+      end
+    end
+  end
+end

lib/api/environments.rb

@@ -3,7 +3,6 @@
 module API
   # Environments RESTfull API endpoints
   class Environments < Grape::API
-    include ::API::Helpers::CustomValidators
     include PaginationParams
 
     before { authenticate! }

lib/api/helpers/merge_requests_helpers.rb

@@ -4,7 +4,6 @@ module API
   module Helpers
     module MergeRequestsHelpers
       extend Grape::API::Helpers
-      include ::API::Helpers::CustomValidators
 
       params :merge_requests_base_params do
         optional :state,

lib/api/issues.rb

@@ -23,7 +23,7 @@ module API
         optional :assignee_id, types: [Integer, String], integer_none_any: true,
                  desc: 'Return issues which are assigned to the user with the given ID'
         optional :assignee_username, type: Array[String], check_assignees_count: true,
-                 coerce_with: Validations::CheckAssigneesCount.coerce,
+                 coerce_with: Validations::Validators::CheckAssigneesCount.coerce,
                  desc: 'Return issues which are assigned to the user with the given username'
         mutually_exclusive :assignee_id, :assignee_username
       end

lib/api/validations/check_assignees_count.rb

-# frozen_string_literal: true
-
-module API
-  module Validations
-    class CheckAssigneesCount < Grape::Validations::Base
-      def self.coerce
-        lambda do |value|
-          case value
-          when String, Array
-            Array.wrap(value)
-          else
-            []
-          end
-        end
-      end
-
-      def validate_param!(attr_name, params)
-        return if param_allowed?(attr_name, params)
-
-        raise Grape::Exceptions::Validation,
-              params: [@scope.full_name(attr_name)],
-              message: "allows one value, but found #{params[attr_name].size}: #{params[attr_name].join(", ")}"
-      end
-
-      private
-
-      def param_allowed?(attr_name, params)
-        params[attr_name].size <= 1
-      end
-    end
-  end
-end
-
-API::Validations::CheckAssigneesCount.prepend_if_ee('EE::API::Validations::CheckAssigneesCount')

lib/api/validations/validators/absence.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Validators
+      class Absence < Grape::Validations::Base
+        def validate_param!(attr_name, params)
+          return if params.respond_to?(:key?) && !params.key?(attr_name)
+
+          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:absence)
+        end
+      end
+    end
+  end
+end

lib/api/validations/validators/array_none_any.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Validators
+      class ArrayNoneAny < Grape::Validations::Base
+        def validate_param!(attr_name, params)
+          value = params[attr_name]
+
+          return if value.is_a?(Array) ||
+              [IssuableFinder::Params::FILTER_NONE, IssuableFinder::Params::FILTER_ANY].include?(value.to_s.downcase)
+
+          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
+                                               message: "should be an array, 'None' or 'Any'"
+        end
+      end
+    end
+  end
+end

lib/api/validations/validators/check_assignees_count.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Validators
+      class CheckAssigneesCount < Grape::Validations::Base
+        def self.coerce
+          lambda do |value|
+            case value
+            when String, Array
+              Array.wrap(value)
+            else
+              []
+            end
+          end
+        end
+
+        def validate_param!(attr_name, params)
+          return if param_allowed?(attr_name, params)
+
+          raise Grape::Exceptions::Validation,
+                params: [@scope.full_name(attr_name)],
+                message: "allows one value, but found #{params[attr_name].size}: #{params[attr_name].join(", ")}"
+        end
+
+        private
+
+        def param_allowed?(attr_name, params)
+          params[attr_name].size <= 1
+        end
+      end
+    end
+  end
+end
+
+API::Validations::Validators::CheckAssigneesCount.prepend_if_ee('EE::API::Validations::Validators::CheckAssigneesCount')

lib/api/validations/validators/file_path.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Validators
+      class FilePath < Grape::Validations::Base
+        def validate_param!(attr_name, params)
+          path = params[attr_name]
+
+          Gitlab::Utils.check_path_traversal!(path)
+        rescue StandardError
+          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
+                                               message: "should be a valid file path"
+        end
+      end
+    end
+  end
+end

lib/api/helpers/custom_validators.rb → lib/api/validations/validators/git_ref.rb

 # frozen_string_literal: true
 
 module API
-  module Helpers
-    module CustomValidators
-      class FilePath < Grape::Validations::Base
-        def validate_param!(attr_name, params)
-          path = params[attr_name]
-
-          Gitlab::Utils.check_path_traversal!(path)
-        rescue StandardError
-          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
-                                               message: "should be a valid file path"
-        end
-      end
-
-      class GitSha < Grape::Validations::Base
-        def validate_param!(attr_name, params)
-          sha = params[attr_name]
-
-          return if Commit::EXACT_COMMIT_SHA_PATTERN.match?(sha)
-
-          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
-                                                message: "should be a valid sha"
-        end
-      end
-
-      class Absence < Grape::Validations::Base
-        def validate_param!(attr_name, params)
-          return if params.respond_to?(:key?) && !params.key?(attr_name)
-
-          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:absence)
-        end
-      end
-
-      class IntegerNoneAny < Grape::Validations::Base
-        def validate_param!(attr_name, params)
-          value = params[attr_name]
-
-          return if value.is_a?(Integer) ||
-              [IssuableFinder::Params::FILTER_NONE, IssuableFinder::Params::FILTER_ANY].include?(value.to_s.downcase)
-
-          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
-                                               message: "should be an integer, 'None' or 'Any'"
-        end
-      end
-
-      class ArrayNoneAny < Grape::Validations::Base
-        def validate_param!(attr_name, params)
-          value = params[attr_name]
-
-          return if value.is_a?(Array) ||
-              [IssuableFinder::Params::FILTER_NONE, IssuableFinder::Params::FILTER_ANY].include?(value.to_s.downcase)
-
-          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
-                                               message: "should be an array, 'None' or 'Any'"
-        end
-      end
-
+  module Validations
+    module Validators
       class GitRef < Grape::Validations::Base
         # There are few checks that a Git reference should pass through to be valid reference.
         # The link contains some rules that have been added to this validator.
@@ -88,10 +34,3 @@ module API
     end
   end
 end
-
-Grape::Validations.register_validator(:file_path, ::API::Helpers::CustomValidators::FilePath)
-Grape::Validations.register_validator(:git_sha, ::API::Helpers::CustomValidators::GitSha)
-Grape::Validations.register_validator(:absence, ::API::Helpers::CustomValidators::Absence)
-Grape::Validations.register_validator(:integer_none_any, ::API::Helpers::CustomValidators::IntegerNoneAny)
-Grape::Validations.register_validator(:array_none_any, ::API::Helpers::CustomValidators::ArrayNoneAny)
-Grape::Validations.register_validator(:git_ref, ::API::Helpers::CustomValidators::GitRef)

lib/api/validations/validators/git_sha.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Validators
+      class GitSha < Grape::Validations::Base
+        def validate_param!(attr_name, params)
+          sha = params[attr_name]
+
+          return if Commit::EXACT_COMMIT_SHA_PATTERN.match?(sha)
+
+          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
+                                                message: "should be a valid sha"
+        end
+      end
+    end
+  end
+end

lib/api/validations/validators/integer_none_any.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Validators
+      class IntegerNoneAny < Grape::Validations::Base
+        def validate_param!(attr_name, params)
+          value = params[attr_name]
+
+          return if value.is_a?(Integer) ||
+              [IssuableFinder::Params::FILTER_NONE, IssuableFinder::Params::FILTER_ANY].include?(value.to_s.downcase)
+
+          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
+                                               message: "should be an integer, 'None' or 'Any'"
+        end
+      end
+    end
+  end
+end

spec/lib/api/helpers/custom_validators_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe API::Helpers::CustomValidators do
-  let(:scope) do
-    Struct.new(:opts) do
-      def full_name(attr_name)
-        attr_name
-      end
-    end
-  end
-
-  describe API::Helpers::CustomValidators::Absence do
-    subject do
-      described_class.new(['test'], {}, false, scope.new)
-    end
-
-    context 'empty param' do
-      it 'does not raise a validation error' do
-        expect_no_validation_error({})
-      end
-    end
-
-    context 'invalid parameters' do
-      it 'raises a validation error' do
-        expect_validation_error('test' => 'some_value')
-      end
-    end
-  end
-
-  describe API::Helpers::CustomValidators::GitSha do
-    let(:sha) { RepoHelpers.sample_commit.id }
-    let(:short_sha) { sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH] }
-    let(:too_short_sha) { sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH - 1] }
-
-    subject do
-      described_class.new(['test'], {}, false, scope.new)
-    end
-
-    context 'valid sha' do
-      it 'does not raise a validation error' do
-        expect_no_validation_error('test' => sha)
-        expect_no_validation_error('test' => short_sha)
-      end
-    end
-
-    context 'empty params' do
-      it 'raises a validation error' do
-        expect_validation_error('test' => nil)
-        expect_validation_error('test' => '')
-      end
-    end
-
-    context 'invalid sha' do
-      it 'raises a validation error' do
-        expect_validation_error('test' => "#{sha}2") # Sha length > 40
-        expect_validation_error('test' => 'somestring')
-        expect_validation_error('test' => too_short_sha) # sha length < MIN_SHA_LENGTH (7)
-      end
-    end
-  end
-
-  describe API::Helpers::CustomValidators::GitRef do
-    subject do
-      described_class.new(['test'], {}, false, scope.new)
-    end
-
-    context 'valid revision param' do
-      it 'does not raise a validation error' do
-        expect_no_validation_error('test' => '4e963fe')
-        expect_no_validation_error('test' => 'foo/bar/baz')
-        expect_no_validation_error('test' => "heads/fu\303\237")
-        expect_no_validation_error('test' => 'a' * 1024)
-      end
-    end
-
-    context "revision param contains invalid chars" do
-      it 'raises a validation error' do
-        expect_validation_error('test' => '-4e963fe')
-        expect_validation_error('test' => '4e963fe..ed4ef')
-        expect_validation_error('test' => '4e96\3fe')
-        expect_validation_error('test' => '4e96@3fe')
-        expect_validation_error('test' => '4e9@{63fe')
-        expect_validation_error('test' => '4e963 fe')
-        expect_validation_error('test' => '4e96~3fe')
-        expect_validation_error('test' => '^4e963fe')
-        expect_validation_error('test' => '4:e963fe')
-        expect_validation_error('test' => '4e963fe.')
-        expect_validation_error('test' => 'heads/foo..bar')
-        expect_validation_error('test' => 'foo/bar/.')
-        expect_validation_error('test' => 'heads/v@{ation')
-        expect_validation_error('test' => 'refs/heads/foo.')
-        expect_validation_error('test' => 'heads/foo\bar')
-        expect_validation_error('test' => 'heads/f[/bar')
-        expect_validation_error('test' => "heads/foo\t")
-        expect_validation_error('test' => "heads/foo\177")
-        expect_validation_error('test' => "#{'a' * 1025}")
-        expect_validation_error('test' => nil)
-        expect_validation_error('test' => '')
-      end
-    end
-  end
-
-  describe API::Helpers::CustomValidators::FilePath do
-    subject do
-      described_class.new(['test'], {}, false, scope.new)
-    end
-
-    context 'valid file path' do
-      it 'does not raise a validation error' do
-        expect_no_validation_error('test' => './foo')
-        expect_no_validation_error('test' => './bar.rb')
-        expect_no_validation_error('test' => 'foo%2Fbar%2Fnew%2Ffile.rb')
-        expect_no_validation_error('test' => 'foo%2Fbar%2Fnew')
-        expect_no_validation_error('test' => 'foo%252Fbar%252Fnew%252Ffile.rb')
-      end
-    end
-
-    context 'invalid file path' do
-      it 'raise a validation error' do
-        expect_validation_error('test' => '../foo')
-        expect_validation_error('test' => '../')
-        expect_validation_error('test' => 'foo/../../bar')
-        expect_validation_error('test' => 'foo/../')
-        expect_validation_error('test' => 'foo/..')
-        expect_validation_error('test' => '../')
-        expect_validation_error('test' => '..\\')
-        expect_validation_error('test' => '..\/')
-        expect_validation_error('test' => '%2e%2e%2f')
-        expect_validation_error('test' => '/etc/passwd')
-      end
-    end
-  end
-
-  describe API::Helpers::CustomValidators::IntegerNoneAny do
-    subject do
-      described_class.new(['test'], {}, false, scope.new)
-    end
-
-    context 'valid parameters' do
-      it 'does not raise a validation error' do
-        expect_no_validation_error('test' => 2)
-        expect_no_validation_error('test' => 100)
-        expect_no_validation_error('test' => 'None')
-        expect_no_validation_error('test' => 'Any')
-        expect_no_validation_error('test' => 'none')
-        expect_no_validation_error('test' => 'any')
-      end
-    end
-
-    context 'invalid parameters' do
-      it 'raises a validation error' do
-        expect_validation_error({ 'test' => 'some_other_string' })
-      end
-    end
-  end
-
-  describe API::Helpers::CustomValidators::ArrayNoneAny do
-    subject do
-      described_class.new(['test'], {}, false, scope.new)
-    end
-
-    context 'valid parameters' do
-      it 'does not raise a validation error' do
-        expect_no_validation_error('test' => [])
-        expect_no_validation_error('test' => [1, 2, 3])
-        expect_no_validation_error('test' => 'None')
-        expect_no_validation_error('test' => 'Any')
-        expect_no_validation_error('test' => 'none')
-        expect_no_validation_error('test' => 'any')
-      end
-    end
-
-    context 'invalid parameters' do
-      it 'raises a validation error' do
-        expect_validation_error('test' => 'some_other_string')
-      end
-    end
-  end
-
-  def expect_no_validation_error(params)
-    expect { validate_test_param!(params) }.not_to raise_error
-  end
-
-  def expect_validation_error(params)
-    expect { validate_test_param!(params) }.to raise_error(Grape::Exceptions::Validation)
-  end
-
-  def validate_test_param!(params)
-    subject.validate_param!('test', params)
-  end
-end

spec/lib/api/validations/validators/absence_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Validations::Validators::Absence do
+  include ApiValidatorsHelpers
+
+  subject do
+    described_class.new(['test'], {}, false, scope.new)
+  end
+
+  context 'empty param' do
+    it 'does not raise a validation error' do
+      expect_no_validation_error({})
+    end
+  end
+
+  context 'invalid parameters' do
+    it 'raises a validation error' do
+      expect_validation_error('test' => 'some_value')
+    end
+  end
+end

spec/lib/api/validations/validators/array_none_any_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Validations::Validators::ArrayNoneAny do
+  include ApiValidatorsHelpers
+
+  subject do
+    described_class.new(['test'], {}, false, scope.new)
+  end
+
+  context 'valid parameters' do
+    it 'does not raise a validation error' do
+      expect_no_validation_error('test' => [])
+      expect_no_validation_error('test' => [1, 2, 3])
+      expect_no_validation_error('test' => 'None')
+      expect_no_validation_error('test' => 'Any')
+      expect_no_validation_error('test' => 'none')
+      expect_no_validation_error('test' => 'any')
+    end
+  end
+
+  context 'invalid parameters' do
+    it 'raises a validation error' do
+      expect_validation_error('test' => 'some_other_string')
+    end
+  end
+end

spec/lib/api/validations/validators/file_path_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Validations::Validators::FilePath do
+  include ApiValidatorsHelpers
+
+  subject do
+    described_class.new(['test'], {}, false, scope.new)
+  end
+
+  context 'valid file path' do
+    it 'does not raise a validation error' do
+      expect_no_validation_error('test' => './foo')
+      expect_no_validation_error('test' => './bar.rb')
+      expect_no_validation_error('test' => 'foo%2Fbar%2Fnew%2Ffile.rb')
+      expect_no_validation_error('test' => 'foo%2Fbar%2Fnew')
+      expect_no_validation_error('test' => 'foo%252Fbar%252Fnew%252Ffile.rb')
+    end
+  end
+
+  context 'invalid file path' do
+    it 'raise a validation error' do
+      expect_validation_error('test' => '../foo')
+      expect_validation_error('test' => '../')
+      expect_validation_error('test' => 'foo/../../bar')
+      expect_validation_error('test' => 'foo/../')
+      expect_validation_error('test' => 'foo/..')
+      expect_validation_error('test' => '../')
+      expect_validation_error('test' => '..\\')
+      expect_validation_error('test' => '..\/')
+      expect_validation_error('test' => '%2e%2e%2f')
+      expect_validation_error('test' => '/etc/passwd')
+    end
+  end
+end

spec/lib/api/validations/validators/git_ref_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Validations::Validators::GitRef do
+  include ApiValidatorsHelpers
+
+  subject do
+    described_class.new(['test'], {}, false, scope.new)
+  end
+
+  context 'valid revision param' do
+    it 'does not raise a validation error' do
+      expect_no_validation_error('test' => '4e963fe')
+      expect_no_validation_error('test' => 'foo/bar/baz')
+      expect_no_validation_error('test' => "heads/fu\303\237")
+      expect_no_validation_error('test' => 'a' * 1024)
+    end
+  end
+
+  context "revision param contains invalid chars" do
+    it 'raises a validation error' do
+      expect_validation_error('test' => '-4e963fe')
+      expect_validation_error('test' => '4e963fe..ed4ef')
+      expect_validation_error('test' => '4e96\3fe')
+      expect_validation_error('test' => '4e96@3fe')
+      expect_validation_error('test' => '4e9@{63fe')
+      expect_validation_error('test' => '4e963 fe')
+      expect_validation_error('test' => '4e96~3fe')
+      expect_validation_error('test' => '^4e963fe')
+      expect_validation_error('test' => '4:e963fe')
+      expect_validation_error('test' => '4e963fe.')
+      expect_validation_error('test' => 'heads/foo..bar')
+      expect_validation_error('test' => 'foo/bar/.')
+      expect_validation_error('test' => 'heads/v@{ation')
+      expect_validation_error('test' => 'refs/heads/foo.')
+      expect_validation_error('test' => 'heads/foo\bar')
+      expect_validation_error('test' => 'heads/f[/bar')
+      expect_validation_error('test' => "heads/foo\t")
+      expect_validation_error('test' => "heads/foo\177")
+      expect_validation_error('test' => "#{'a' * 1025}")
+      expect_validation_error('test' => nil)
+      expect_validation_error('test' => '')
+    end
+  end
+end

spec/lib/api/validations/validators/git_sha_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Validations::Validators::GitSha do
+  include ApiValidatorsHelpers
+
+  let(:sha) { RepoHelpers.sample_commit.id }
+  let(:short_sha) { sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH] }
+  let(:too_short_sha) { sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH - 1] }
+
+  subject do
+    described_class.new(['test'], {}, false, scope.new)
+  end
+
+  context 'valid sha' do
+    it 'does not raise a validation error' do
+      expect_no_validation_error('test' => sha)
+      expect_no_validation_error('test' => short_sha)
+    end
+  end
+
+  context 'empty params' do
+    it 'raises a validation error' do
+      expect_validation_error('test' => nil)
+      expect_validation_error('test' => '')
+    end
+  end
+
+  context 'invalid sha' do
+    it 'raises a validation error' do
+      expect_validation_error('test' => "#{sha}2") # Sha length > 40
+      expect_validation_error('test' => 'somestring')
+      expect_validation_error('test' => too_short_sha) # sha length < MIN_SHA_LENGTH (7)
+    end
+  end
+end

spec/lib/api/validations/validators/integer_none_any_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Validations::Validators::IntegerNoneAny do
+  include ApiValidatorsHelpers
+
+  subject do
+    described_class.new(['test'], {}, false, scope.new)
+  end
+
+  context 'valid parameters' do
+    it 'does not raise a validation error' do
+      expect_no_validation_error('test' => 2)
+      expect_no_validation_error('test' => 100)
+      expect_no_validation_error('test' => 'None')
+      expect_no_validation_error('test' => 'Any')
+      expect_no_validation_error('test' => 'none')
+      expect_no_validation_error('test' => 'any')
+    end
+  end
+
+  context 'invalid parameters' do
+    it 'raises a validation error' do
+      expect_validation_error({ 'test' => 'some_other_string' })
+    end
+  end
+end

spec/support/helpers/api_validators_helpers.rb

+# frozen_string_literal: true
+
+module ApiValidatorsHelpers
+  def scope
+    Struct.new(:opts) do
+      def full_name(attr_name)
+        attr_name
+      end
+    end
+  end
+
+  def expect_no_validation_error(params)
+    expect { validate_test_param!(params) }.not_to raise_error
+  end
+
+  def expect_validation_error(params)
+    expect { validate_test_param!(params) }.to raise_error(Grape::Exceptions::Validation)
+  end
+
+  def validate_test_param!(params)
+    subject.validate_param!('test', params)
+  end
+end