Implement Ci::NestedUniquenessValidator

5 files changed, 32 insertions, 45 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