Implement Ci::NestedUniquenessValidator

7 files changed, 59 insertions, 47 deletions

diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index aa71f606657..10d478b1eea 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/app/controllers/projects/pipeline_schedules_controller.rb
@@ -33,7 +33,8 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   end
 
   def update
-    if schedule.update(schedule_params)
+    if Ci::UpdatePipelineScheduleService
+        .new(@project, current_user, schedule_params).execute(schedule)
       redirect_to namespace_project_pipeline_schedules_path(@project.namespace.becomes(Namespace), @project)
     else
       render :edit
diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb
index ad9f8b89924..df9df45edb0 100644
--- a/app/models/ci/pipeline_schedule.rb
+++ b/app/models/ci/pipeline_schedule.rb
@@ -15,11 +15,6 @@ 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/services/ci/create_pipeline_schedule_service.rb b/app/services/ci/create_pipeline_schedule_service.rb
index cd40deb6187..7016592cd10 100644
--- a/app/services/ci/create_pipeline_schedule_service.rb
+++ b/app/services/ci/create_pipeline_schedule_service.rb
@@ -1,13 +1,22 @@
 module Ci
   class CreatePipelineScheduleService < BaseService
     def execute
-      project.pipeline_schedules.create(pipeline_schedule_params)
+      pipeline_schedule = project.pipeline_schedules.build(pipeline_schedule_params)
+
+      if Ci::NestedUniquenessValidator.duplicated?(pipeline_schedule_params['variables_attributes'], 'key')
+        pipeline_schedule.errors.add('variables.key', "keys are duplicated")
+
+        return pipeline_schedule
+      end
+
+      pipeline_schedule.save
+      pipeline_schedule
     end
 
     private
 
     def pipeline_schedule_params
-      params.merge(owner: current_user)
+      @pipeline_schedule_params ||= params.merge(owner: current_user)
     end
   end
 end
diff --git a/app/services/ci/update_pipeline_schedule_service.rb b/app/services/ci/update_pipeline_schedule_service.rb
new file mode 100644
index 00000000000..0ab84e19583
--- /dev/null
+++ b/app/services/ci/update_pipeline_schedule_service.rb
@@ -0,0 +1,19 @@
+module Ci
+  class UpdatePipelineScheduleService < BaseService
+    def execute(pipeline_schedule)
+      if Ci::NestedUniquenessValidator.duplicated?(pipeline_schedule_params['variables_attributes'], 'key')
+        pipeline_schedule.errors.add('variables.key', "keys are duplicated")
+
+        return false
+      end
+
+      pipeline_schedule.update(pipeline_schedule_params)
+    end
+
+    private
+
+    def pipeline_schedule_params
+      @pipeline_schedule_params ||= params.merge(owner: current_user)
+    end
+  end
+end
diff --git a/app/validators/uniqueness_of_in_memory_validator.rb b/app/validators/uniqueness_of_in_memory_validator.rb
deleted file mode 100644
index 84e88b2eb76..00000000000
--- a/app/validators/uniqueness_of_in_memory_validator.rb
+++ /dev/null
@@ -1,37 +0,0 @@
-# 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/lib/ci/nested_uniqueness_validator.rb b/lib/ci/nested_uniqueness_validator.rb
new file mode 100644
index 00000000000..7c6a109b0d8
--- /dev/null
+++ b/lib/ci/nested_uniqueness_validator.rb
@@ -0,0 +1,11 @@
+module Ci
+  module NestedUniquenessValidator
+    class << self
+      def duplicated?(nested_attributes, unique_key)
+        return false unless nested_attributes.is_a?(Array)
+
+        nested_attributes.map { |v| v[unique_key] }.uniq.length != nested_attributes.length
+      end
+    end
+  end
+end
diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
index bfd31f9409b..aa3aa06f917 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -267,8 +267,7 @@ describe Projects::PipelineSchedulesController do
           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
+            go
 
             expect(assigns(:schedule).errors['variables.key']).not_to be_empty
           end
@@ -340,6 +339,21 @@ describe Projects::PipelineSchedulesController do
               expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(-1)
             end
           end
+
+          context 'when deletes and creates the same keys' do
+            let(:schedule) do
+              basic_param.merge({
+                variables_attributes: [{ id: pipeline_schedule_variable.id, key: pipeline_schedule_variable.key, _destroy: true },
+                                       { key: pipeline_schedule_variable.key, value: 'new_value' }]
+              })
+            end
+
+            it 'returns an error that variables are duplciated' do
+              go
+
+              expect(assigns(:schedule).errors['variables.key']).not_to be_empty
+            end
+          end
         end
       end
     end