Implement uniqueness_of_in_memory_validator

3 files changed, 72 insertions, 37 deletions

diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb
index df9df45edb0..ad9f8b89924 100644
--- a/app/models/ci/pipeline_schedule.rb
+++ b/app/models/ci/pipeline_schedule.rb
@@ -15,6 +15,11 @@ module Ci
     validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? }
     validates :ref, presence: { unless: :importing? }
     validates :description, presence: true
+    validates :variables, uniqueness_of_in_memory: {
+                :collection => :variables,
+                :attrs => [:pipeline_schedule_id, :key],
+                :message => ['variables.key', 'keys are duplicated']
+              }
 
     before_save :set_next_run_at
 
diff --git a/app/validators/uniqueness_of_in_memory_validator.rb b/app/validators/uniqueness_of_in_memory_validator.rb
new file mode 100644
index 00000000000..84e88b2eb76
--- /dev/null
+++ b/app/validators/uniqueness_of_in_memory_validator.rb
@@ -0,0 +1,37 @@
+# UniquenessOfInMemoryValidator
+#
+# This validtor 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
+#
+# Inspired by https://stackoverflow.com/a/2883129/2522666
+module ActiveRecord
+  class Base
+    # Validate that the the objects in +collection+ are unique
+    # when compared against all their non-blank +attrs+. If not
+    # add +message+ to the base errors.
+    def validate_uniqueness_of_in_memory(collection, attrs, message)
+      hashes = collection.inject({}) do |hash, record|
+        key = attrs.map { |a| record.send(a).to_s }.join
+        if key.blank? || record.marked_for_destruction?
+          key = record.object_id
+        end
+        hash[key] = record unless hash[key]
+        hash
+      end
+
+      if collection.length > hashes.length
+        self.errors.add(*message)
+      end
+    end
+  end
+end
+
+class UniquenessOfInMemoryValidator < ActiveModel::Validator
+  def validate(record)
+    record.validate_uniqueness_of_in_memory(
+      record.public_send(options[:collection]),
+      options[:attrs],
+      options[:message])
+  end
+end
diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
index a3a5fe32307..12eeca5408b 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -150,22 +150,21 @@ describe Projects::PipelineSchedulesController do
         end
       end
 
-      # This test no longer passes, since we removed a custom validation
-      # context 'when variables_attributes has two variables and duplicted' do
-      #   let(:schedule) do
-      #     basic_param.merge({
-      #       variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
-      #     })
-      #   end
-
-      #   it 'returns an error that the keys of variable are duplicated' do
-      #     expect { go }
-      #       .to change { Ci::PipelineSchedule.count }.by(0)
-      #       .and change { Ci::PipelineScheduleVariable.count }.by(0)
-
-      #     expect(assigns(:schedule).errors['variables.key']).not_to be_empty
-      #   end
-      # end
+      context 'when variables_attributes has two variables and duplicted' do
+        let(:schedule) do
+          basic_param.merge({
+            variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
+          })
+        end
+
+        it 'returns an error that the keys of variable are duplicated' do
+          expect { go }
+            .to change { Ci::PipelineSchedule.count }.by(0)
+            .and change { Ci::PipelineScheduleVariable.count }.by(0)
+
+          expect(assigns(:schedule).errors['variables.key']).not_to be_empty
+        end
+      end
     end
 
     describe 'security' do
@@ -194,7 +193,6 @@ describe Projects::PipelineSchedulesController do
 
       before do
         project.add_developer(user)
-
         sign_in(user)
       end
 
@@ -210,7 +208,6 @@ describe Projects::PipelineSchedulesController do
             go
 
             pipeline_schedule.reload
-
             expect(response).to have_http_status(:found)
             expect(pipeline_schedule.description).to eq('updated_desc')
             expect(pipeline_schedule.cron).to eq('0 1 * * *')
@@ -232,7 +229,6 @@ describe Projects::PipelineSchedulesController do
             expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(1)
 
             pipeline_schedule.reload
-
             expect(response).to have_http_status(:found)
             expect(pipeline_schedule.variables.last.key).to eq('AAA')
             expect(pipeline_schedule.variables.last.value).to eq('AAA123')
@@ -250,7 +246,6 @@ describe Projects::PipelineSchedulesController do
             expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(2)
 
             pipeline_schedule.reload
-
             expect(response).to have_http_status(:found)
             expect(pipeline_schedule.variables.first.key).to eq('AAA')
             expect(pipeline_schedule.variables.first.value).to eq('AAA123')
@@ -259,21 +254,20 @@ describe Projects::PipelineSchedulesController do
           end
         end
 
-        # This test no longer passes, since we removed a custom validation
-        # context 'when params include two duplicated variables' do
-        #   let(:schedule) do
-        #     basic_param.merge({
-        #       variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
-        #     })
-        #   end
-
-        #   it 'returns an error that variables are duplciated' do
-        #     put :update, namespace_id: project.namespace.to_param,
-        #       project_id: project, id: pipeline_schedule, schedule: schedule
-
-        #     expect(assigns(:schedule).errors['variables.key']).not_to be_empty
-        #   end
-        # end
+        context 'when params include two duplicated variables' do
+          let(:schedule) do
+            basic_param.merge({
+              variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
+            })
+          end
+
+          it 'returns an error that variables are duplciated' do
+            put :update, namespace_id: project.namespace.to_param,
+              project_id: project, id: pipeline_schedule, schedule: schedule
+
+            expect(assigns(:schedule).errors['variables.key']).not_to be_empty
+          end
+        end
       end
 
       context 'when a pipeline schedule has one variable' do
@@ -293,7 +287,6 @@ describe Projects::PipelineSchedulesController do
             go
 
             pipeline_schedule.reload
-
             expect(response).to have_http_status(:found)
             expect(pipeline_schedule.description).to eq('updated_desc')
             expect(pipeline_schedule.cron).to eq('0 1 * * *')
@@ -316,6 +309,7 @@ describe Projects::PipelineSchedulesController do
             it 'adds the new variable' do
               expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(1)
 
+              pipeline_schedule.reload
               expect(pipeline_schedule.variables.last.key).to eq('AAA')
             end
           end
@@ -331,7 +325,6 @@ describe Projects::PipelineSchedulesController do
               expect { go }.not_to change { Ci::PipelineScheduleVariable.count }
 
               pipeline_schedule_variable.reload
-
               expect(pipeline_schedule_variable.value).to eq('new_value')
             end
           end