Merge branch '336202-add-field-validations' into 'master'

Add field validations for Finding Evidence models

See merge request gitlab-org/gitlab!66394

.rubocop_manual_todo.yml

@@ -1685,6 +1685,7 @@ Gitlab/NamespacedClass:
     - 'app/uploaders/personal_file_uploader.rb'
     - 'app/validators/abstract_path_validator.rb'
     - 'app/validators/addressable_url_validator.rb'
+    - 'app/validators/any_field_validator.rb'
     - 'app/validators/array_members_validator.rb'
     - 'app/validators/branch_filter_validator.rb'
     - 'app/validators/certificate_fingerprint_validator.rb'

app/models/concerns/any_field_validation.rb

-# frozen_string_literal: true
-
-# This module enables a record to be valid if any field is present
-#
-# Overwrite one_of_required_fields to set one of which fields must be present
-module AnyFieldValidation
-  extend ActiveSupport::Concern
-
-  included do
-    validate :any_field_present
-  end
-
-  private
-
-  def any_field_present
-    return unless one_of_required_fields.all? { |field| self[field].blank? }
-
-    errors.add(:base, _("At least one field of %{one_of_required_fields} must be present") %
-      { one_of_required_fields: one_of_required_fields })
-  end
-
-  def one_of_required_fields
-    raise NotImplementedError
-  end
-end

app/validators/any_field_validator.rb

+# frozen_string_literal: true
+
+# AnyFieldValidator
+#
+# Custom validator that checks if any of the provided
+# fields are present to ensure creation of a non-empty
+# record
+#
+# Example:
+#
+#   class MyModel < ApplicationRecord
+#     validates_with AnyFieldValidator, fields: %w[type name url]
+#   end
+class AnyFieldValidator < ActiveModel::Validator
+  def initialize(*args)
+    super
+
+    if options[:fields].blank?
+      raise 'Provide the fields options'
+    end
+  end
+
+  def validate(record)
+    return unless one_of_required_fields.all? { |field| record[field].blank? }
+
+    record.errors.add(:base, _("At least one field of %{one_of_required_fields} must be present") %
+      { one_of_required_fields: one_of_required_fields })
+  end
+
+  private
+
+  def one_of_required_fields
+    options[:fields]
+  end
+end

ee/app/models/vulnerabilities/finding/evidence/asset.rb

@@ -4,10 +4,10 @@ module Vulnerabilities
   class Finding
     class Evidence
       class Asset < ApplicationRecord
-        include AnyFieldValidation
-
         self.table_name = 'vulnerability_finding_evidence_assets'
 
+        DATA_FIELDS = %w[type name url].freeze
+
         belongs_to :evidence,
                    class_name: 'Vulnerabilities::Finding::Evidence',
                    inverse_of: :assets,
@@ -17,14 +17,7 @@ module Vulnerabilities
         validates :type, length: { maximum: 2048 }
         validates :name, length: { maximum: 2048 }
         validates :url, length: { maximum: 2048 }
-
-        validate :any_field_present
-
-        private
-
-        def one_of_required_fields
-          [:type, :name, :url]
-        end
+        validates_with AnyFieldValidator, fields: DATA_FIELDS
       end
     end
   end

ee/app/models/vulnerabilities/finding/evidence/header.rb

@@ -9,8 +9,8 @@ module Vulnerabilities
         belongs_to :request, class_name: 'Vulnerabilities::Finding::Evidence::Request', inverse_of: :headers, foreign_key: 'vulnerability_finding_evidence_request_id'
         belongs_to :response, class_name: 'Vulnerabilities::Finding::Evidence::Response', inverse_of: :headers, foreign_key: 'vulnerability_finding_evidence_response_id'
 
-        validates :name, length: { maximum: 255 }
-        validates :value, length: { maximum: 8192 }
+        validates :name, length: { maximum: 255 }, presence: true
+        validates :value, length: { maximum: 8192 }, presence: true
         validate :request_or_response_is_set
         validate :request_and_response_cannot_be_set
 

ee/app/models/vulnerabilities/finding/evidence/request.rb

@@ -8,6 +8,8 @@ module Vulnerabilities
 
         self.table_name = 'vulnerability_finding_evidence_requests'
 
+        DATA_FIELDS = %w[method url].freeze
+
         belongs_to :evidence,
                    class_name: 'Vulnerabilities::Finding::Evidence',
                    inverse_of: :request,
@@ -26,6 +28,7 @@ module Vulnerabilities
 
         validates :method, length: { maximum: 32 }
         validates :url, length: { maximum: 2048 }
+        validates_with AnyFieldValidator, fields: DATA_FIELDS
       end
     end
   end

ee/app/models/vulnerabilities/finding/evidence/response.rb

@@ -24,7 +24,7 @@ module Vulnerabilities
                  inverse_of: :response,
                  foreign_key: 'vulnerability_finding_evidence_response_id'
 
-        validates :reason_phrase, length: { maximum: 2048 }
+        validates :reason_phrase, length: { maximum: 2048 }, presence: true
       end
     end
   end

ee/app/models/vulnerabilities/finding/evidence/source.rb

@@ -6,10 +6,13 @@ module Vulnerabilities
       class Source < ApplicationRecord
         self.table_name = 'vulnerability_finding_evidence_sources'
 
+        DATA_FIELDS = %w[name url].freeze
+
         belongs_to :evidence, class_name: 'Vulnerabilities::Finding::Evidence', inverse_of: :source, foreign_key: 'vulnerability_finding_evidence_id', optional: false
 
         validates :name, length: { maximum: 2048 }
         validates :url, length: { maximum: 2048 }
+        validates_with AnyFieldValidator, fields: DATA_FIELDS
       end
     end
   end

ee/app/models/vulnerabilities/finding/evidence/supporting_message.rb

@@ -11,7 +11,7 @@ module Vulnerabilities
         has_one :request, class_name: 'Vulnerabilities::Finding::Evidence::Request', inverse_of: :supporting_message, foreign_key: 'vulnerability_finding_evidence_supporting_message_id'
         has_one :response, class_name: 'Vulnerabilities::Finding::Evidence::Response', inverse_of: :supporting_message, foreign_key: 'vulnerability_finding_evidence_supporting_message_id'
 
-        validates :name, length: { maximum: 2048 }
+        validates :name, length: { maximum: 2048 }, presence: true
       end
     end
   end

ee/spec/models/vulnerabilities/finding/evidence/asset_spec.rb

@@ -9,27 +9,5 @@ RSpec.describe Vulnerabilities::Finding::Evidence::Asset do
   it { is_expected.to validate_length_of(:name).is_at_most(2048) }
   it { is_expected.to validate_length_of(:url).is_at_most(2048) }
 
-  describe '.any_field_present' do
-    let_it_be(:evidence) { build(:vulnerabilties_finding_evidence) }
-    let_it_be(:asset) { Vulnerabilities::Finding::Evidence::Asset.new(evidence: evidence) }
-
-    it 'is invalid if there are no fields present' do
-      expect(asset).not_to be_valid
-    end
-
-    it 'validates if there is only a type' do
-      asset.type = 'asset-type'
-      expect(asset).to be_valid
-    end
-
-    it 'validates if there is only a name' do
-      asset.type = 'asset-name'
-      expect(asset).to be_valid
-    end
-
-    it 'validates if there is only a url' do
-      asset.type = 'asset-url.example'
-      expect(asset).to be_valid
-    end
-  end
+  it_behaves_like 'validates presence of any field'
 end

ee/spec/models/vulnerabilities/finding/evidence/header_spec.rb

@@ -7,29 +7,7 @@ RSpec.describe Vulnerabilities::Finding::Evidence::Header do
   it { is_expected.to belong_to(:response).class_name('Vulnerabilities::Finding::Evidence::Response').inverse_of(:headers).optional }
 
   it { is_expected.to validate_length_of(:name).is_at_most(255) }
+  it { is_expected.to validate_presence_of(:name) }
   it { is_expected.to validate_length_of(:value).is_at_most(8192) }
-
-  describe '.request_or_response_is_set' do
-    let(:header) { build(:vulnerabilties_finding_evidence_header) }
-
-    it 'is invalid if there is no request or response' do
-      expect(header).not_to be_valid
-    end
-
-    it 'validates if there is a response' do
-      header.response = build(:vulnerabilties_finding_evidence_response)
-      expect(header).to be_valid
-    end
-
-    it 'validates if there is a request' do
-      header.request = build(:vulnerabilties_finding_evidence_request)
-      expect(header).to be_valid
-    end
-
-    it 'is invalid if there is a request and a response' do
-      header.request = build(:vulnerabilties_finding_evidence_request)
-      header.response = build(:vulnerabilties_finding_evidence_response)
-      expect(header).not_to be_valid
-    end
-  end
+  it { is_expected.to validate_presence_of(:value) }
 end

ee/spec/models/vulnerabilities/finding/evidence/request_spec.rb

@@ -11,4 +11,6 @@ RSpec.describe Vulnerabilities::Finding::Evidence::Request do
   it { is_expected.to validate_length_of(:url).is_at_most(2048) }
 
   it_behaves_like 'body shared examples', :vulnerabilties_finding_evidence_request
+
+  it_behaves_like 'validates presence of any field'
 end

ee/spec/models/vulnerabilities/finding/evidence/response_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe Vulnerabilities::Finding::Evidence::Response do
   it { is_expected.to have_many(:headers).class_name('Vulnerabilities::Finding::Evidence::Header').with_foreign_key('vulnerability_finding_evidence_response_id').inverse_of(:response) }
 
   it { is_expected.to validate_length_of(:reason_phrase).is_at_most(2048) }
+  it { is_expected.to validate_presence_of(:reason_phrase) }
 
   it_behaves_like 'body shared examples', :vulnerabilties_finding_evidence_response
 end

ee/spec/models/vulnerabilities/finding/evidence/source_spec.rb

@@ -7,4 +7,6 @@ RSpec.describe Vulnerabilities::Finding::Evidence::Source do
 
   it { is_expected.to validate_length_of(:name).is_at_most(2048) }
   it { is_expected.to validate_length_of(:url).is_at_most(2048) }
+
+  it_behaves_like 'validates presence of any field'
 end

ee/spec/models/vulnerabilities/finding/evidence/supporting_message_spec.rb

@@ -8,4 +8,5 @@ RSpec.describe Vulnerabilities::Finding::Evidence::SupportingMessage do
   it { is_expected.to have_one(:response).class_name('Vulnerabilities::Finding::Evidence::Response').with_foreign_key('vulnerability_finding_evidence_supporting_message_id').inverse_of(:supporting_message) }
 
   it { is_expected.to validate_length_of(:name).is_at_most(2048) }
+  it { is_expected.to validate_presence_of(:name) }
 end

ee/spec/support/shared_examples/models/concerns/any_field_validation_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'validates presence of any field' do
+  describe '.any_field_present' do
+    let_it_be(:evidence) { build(:vulnerabilties_finding_evidence) }
+    let_it_be(:test_object) { described_class.new(evidence: evidence) }
+
+    it 'is invalid if there are no fields present' do
+      expect(test_object).not_to be_valid
+    end
+
+    described_class::DATA_FIELDS.each do |field|
+      it "validates on a single field present when #{field} is set" do
+        test_object[field] = "test-object-#{field}"
+        expect(test_object).to be_valid
+      end
+    end
+  end
+end

spec/validators/any_field_validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AnyFieldValidator do
+  context 'when validation is instantiated correctly' do
+    let(:validated_class) do
+      Class.new(ApplicationRecord) do
+        self.table_name = 'vulnerabilities'
+
+        validates_with AnyFieldValidator, fields: %w(title description)
+      end
+    end
+
+    it 'raises an error if no fields are defined' do
+      validated_object = validated_class.new
+
+      expect(validated_object.valid?).to be_falsey
+      expect(validated_object.errors.messages)
+      .to eq(base: ["At least one field of %{one_of_required_fields} must be present" %
+        { one_of_required_fields: %w(title description) }])
+    end
+
+    it 'validates if only one field is present' do
+      validated_object = validated_class.new(title: 'Vulnerability title')
+
+      expect(validated_object.valid?).to be_truthy
+    end
+  end
+
+  context 'when validation is missing the fields parameter' do
+    let(:invalid_class) do
+      Class.new(ApplicationRecord) do
+        self.table_name = 'vulnerabilities'
+
+        validates_with AnyFieldValidator
+      end
+    end
+
+    it 'raises an error' do
+      expect { invalid_class.new }.to raise_error(RuntimeError)
+    end
+  end
+end