Merge branch 'improve-uniquness-validator-on-release' into 'master'

Introduce NestedAttributesDuplicatesValidator for release

See merge request gitlab-org/gitlab!51951

.rubocop_manual_todo.yml

@@ -2205,7 +2205,7 @@ Gitlab/NamespacedClass:
     - 'app/validators/system_hook_url_validator.rb'
     - 'app/validators/top_level_group_validator.rb'
     - 'app/validators/untrusted_regexp_validator.rb'
-    - 'app/validators/variable_duplicates_validator.rb'
+    - 'app/validators/nested_attributes_duplicates_validator.rb'
     - 'app/validators/x509_certificate_credentials_validator.rb'
     - 'app/validators/zoom_url_validator.rb'
     - 'app/workers/admin_email_worker.rb'

app/models/ci/pipeline_schedule.rb

@@ -21,7 +21,7 @@ module Ci
     validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? }
     validates :ref, presence: { unless: :importing? }
     validates :description, presence: true
-    validates :variables, variable_duplicates: true
+    validates :variables, nested_attributes_duplicates: true
 
     strip_attributes :cron
 

app/models/group.rb

@@ -84,7 +84,7 @@ class Group < Namespace
   validate :visibility_level_allowed_by_sub_groups
   validate :visibility_level_allowed_by_parent
   validate :two_factor_authentication_allowed
-  validates :variables, variable_duplicates: true
+  validates :variables, nested_attributes_duplicates: true
 
   validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
 

app/models/project.rb

@@ -456,7 +456,7 @@ class Project < ApplicationRecord
   validates :repository_storage,
     presence: true,
     inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
-  validates :variables, variable_duplicates: { scope: :environment_scope }
+  validates :variables, nested_attributes_duplicates: { scope: :environment_scope }
   validates :bfg_object_map, file_size: { maximum: :max_attachment_size }
   validates :max_artifacts_size, numericality: { only_integer: true, greater_than: 0, allow_nil: true }
 

app/models/release.rb

@@ -24,6 +24,7 @@ class Release < ApplicationRecord
 
   validates :project, :tag, presence: true
   validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") }
+  validates :links, nested_attributes_duplicates: { scope: :release, child_attributes: %i[name url filepath] }
 
   scope :sorted, -> { order(released_at: :desc) }
   scope :preloaded, -> { includes(:evidences, :milestones, project: [:project_feature, :route, { namespace: :route }]) }

app/validators/variable_duplicates_validator.rb → app/validators/nested_attributes_duplicates_validator.rb

 # frozen_string_literal: true
 
-# VariableDuplicatesValidator
+# NestedAttributesDuplicates
 #
 # This validator is designed for especially the following condition
 # - Use `accepts_nested_attributes_for :xxx` in a parent model
 # - Use `validates :xxx, uniqueness: { scope: :xxx_id }` in a child model
-class VariableDuplicatesValidator < ActiveModel::EachValidator
+class NestedAttributesDuplicatesValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
-    return if record.errors.include?(:"#{attribute}.key")
+    return if child_attributes.any? { |child_attribute| record.errors.include?(:"#{attribute}.#{child_attribute}") }
 
     if options[:scope]
       scoped = value.group_by do |variable|
@@ -23,12 +23,18 @@ class VariableDuplicatesValidator < ActiveModel::EachValidator
 
   # rubocop: disable CodeReuse/ActiveRecord
   def validate_duplicates(record, attribute, values)
-    duplicates = values.reject(&:marked_for_destruction?).group_by(&:key).select { |_, v| v.many? }.map(&:first)
+    child_attributes.each do |child_attribute|
+      duplicates = values.reject(&:marked_for_destruction?).group_by(&:"#{child_attribute}").select { |_, v| v.many? }.map(&:first)
       if duplicates.any?
         error_message = +"have duplicate values (#{duplicates.join(", ")})"
         error_message << " for #{values.first.send(options[:scope])} scope" if options[:scope] # rubocop:disable GitlabSecurity/PublicSend
         record.errors.add(attribute, error_message)
       end
     end
+  end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  def child_attributes
+    options[:child_attributes] || %i[key]
+  end
 end

changelogs/unreleased/improve-uniquness-validator-on-release.yml

+---
+title: Improve duplication validation on Release Links
+merge_request: 51951
+author:
+type: fixed

spec/models/release_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe Release do
   let_it_be(:user)    { create(:user) }
   let_it_be(:project) { create(:project, :public, :repository) }
-  let_it_be(:release) { create(:release, project: project, author: user) }
+  let(:release) { create(:release, project: project, author: user) }
 
   it { expect(release).to be_valid }
 
@@ -89,6 +89,61 @@ RSpec.describe Release do
     end
   end
 
+  describe '#update' do
+    subject { release.update(params) }
+
+    context 'when links do not exist' do
+      context 'when params are specified for creation' do
+        let(:params) do
+          { links_attributes: [{ name: 'test', url: 'https://www.google.com/' }] }
+        end
+
+        it 'creates a link successfuly' do
+          is_expected.to eq(true)
+
+          expect(release.links.count).to eq(1)
+          expect(release.links.first.name).to eq('test')
+          expect(release.links.first.url).to eq('https://www.google.com/')
+        end
+      end
+    end
+
+    context 'when a link exists' do
+      let!(:link1) { create(:release_link, release: release, name: 'test1', url: 'https://www.google1.com/') }
+      let!(:link2) { create(:release_link, release: release, name: 'test2', url: 'https://www.google2.com/') }
+
+      before do
+        release.reload
+      end
+
+      context 'when params are specified for update' do
+        let(:params) do
+          { links_attributes: [{ id: link1.id, name: 'new' }] }
+        end
+
+        it 'updates the link successfully' do
+          is_expected.to eq(true)
+
+          expect(release.links.count).to eq(2)
+          expect(release.links.first.name).to eq('new')
+        end
+      end
+
+      context 'when params are specified for deletion' do
+        let(:params) do
+          { links_attributes: [{ id: link1.id, _destroy: true }] }
+        end
+
+        it 'removes the link successfuly' do
+          is_expected.to eq(true)
+
+          expect(release.links.count).to eq(1)
+          expect(release.links.first.name).to eq(link2.name)
+        end
+      end
+    end
+  end
+
   describe '#sources' do
     subject { release.sources }
 

spec/requests/api/graphql/mutations/releases/create_spec.rb

@@ -309,10 +309,7 @@ RSpec.describe 'Creation of a new release' do
         let(:asset_link_2) { { name: 'My link', url: 'https://example.com/2' } }
         let(:assets) { { links: [asset_link_1, asset_link_2] } }
 
-        # Right now the raw Postgres error message is sent to the user as the validation message.
-        # We should catch this validation error and return a nicer message:
-        # https://gitlab.com/gitlab-org/gitlab/-/issues/277087
-        it_behaves_like 'errors-as-data with message', 'PG::UniqueViolation'
+        it_behaves_like 'errors-as-data with message', %r{Validation failed: Links have duplicate values \(My link\)}
       end
 
       context 'when two release assets share the same URL' do
@@ -320,8 +317,7 @@ RSpec.describe 'Creation of a new release' do
         let(:asset_link_2) { { name: 'My second link', url: 'https://example.com' } }
         let(:assets) { { links: [asset_link_1, asset_link_2] } }
 
-        # Same note as above about the ugly error message
-        it_behaves_like 'errors-as-data with message', 'PG::UniqueViolation'
+        it_behaves_like 'errors-as-data with message', %r{Validation failed: Links have duplicate values \(https://example.com\)}
       end
 
       context 'when the provided tag name is HEAD' do

spec/requests/api/graphql/project/release_spec.rb

@@ -283,7 +283,7 @@ RSpec.describe 'Query.project(fullPath).release(tagName)' do
       let_it_be(:project) { create(:project, :repository, :private) }
       let_it_be(:milestone_1) { create(:milestone, project: project) }
       let_it_be(:milestone_2) { create(:milestone, project: project) }
-      let_it_be(:release) { create(:release, :with_evidence, project: project, milestones: [milestone_1, milestone_2], released_at: released_at) }
+      let_it_be(:release, reload: true) { create(:release, :with_evidence, project: project, milestones: [milestone_1, milestone_2], released_at: released_at) }
       let_it_be(:release_link_1) { create(:release_link, release: release) }
       let_it_be(:release_link_2) { create(:release_link, release: release, filepath: link_filepath) }
 
@@ -324,7 +324,7 @@ RSpec.describe 'Query.project(fullPath).release(tagName)' do
       let_it_be(:project) { create(:project, :repository, :public) }
       let_it_be(:milestone_1) { create(:milestone, project: project) }
       let_it_be(:milestone_2) { create(:milestone, project: project) }
-      let_it_be(:release) { create(:release, :with_evidence, project: project, milestones: [milestone_1, milestone_2], released_at: released_at) }
+      let_it_be(:release, reload: true) { create(:release, :with_evidence, project: project, milestones: [milestone_1, milestone_2], released_at: released_at) }
       let_it_be(:release_link_1) { create(:release_link, release: release) }
       let_it_be(:release_link_2) { create(:release_link, release: release, filepath: link_filepath) }
 

spec/validators/variable_duplicates_validator_spec.rb → spec/validators/nested_attributes_duplicates_validator_spec.rb

@@ -2,13 +2,16 @@
 
 require 'spec_helper'
 
-RSpec.describe VariableDuplicatesValidator do
-  let(:validator) { described_class.new(attributes: [:variables], **options) }
+RSpec.describe NestedAttributesDuplicatesValidator do
+  let(:validator) { described_class.new(attributes: [attribute], **options) }
 
   describe '#validate_each' do
     let(:project) { build(:project) }
+    let(:record) { project }
+    let(:attribute) { :variables }
+    let(:value) { project.variables }
 
-    subject { validator.validate_each(project, :variables, project.variables) }
+    subject { validator.validate_each(record, attribute, value) }
 
     context 'with no scope' do
       let(:options) { {} }
@@ -65,5 +68,46 @@ RSpec.describe VariableDuplicatesValidator do
         end
       end
     end
+
+    context 'with a child attribute' do
+      let(:release) { build(:release) }
+      let(:first_link) { build(:release_link, name: 'test1', url: 'https://www.google1.com', release: release) }
+      let(:second_link) { build(:release_link, name: 'test2', url: 'https://www.google2.com', release: release) }
+      let(:record) { release }
+      let(:attribute) { :links }
+      let(:value) { release.links }
+      let(:options) { { scope: :release, child_attributes: %i[name url] } }
+
+      before do
+        release.links << first_link
+        release.links << second_link
+      end
+
+      it 'does not have any errors' do
+        subject
+
+        expect(release.errors.empty?).to be true
+      end
+
+      context 'when name is duplicated' do
+        let(:second_link) { build(:release_link, name: 'test1', release: release) }
+
+        it 'has a duplicate error' do
+          subject
+
+          expect(release.errors).to have_key(attribute)
+        end
+      end
+
+      context 'when url is duplicated' do
+        let(:second_link) { build(:release_link, url: 'https://www.google1.com', release: release) }
+
+        it 'has a duplicate error' do
+          subject
+
+          expect(release.errors).to have_key(attribute)
+        end
+      end
+    end
   end
 end