Revert "Implement Ci::NestedUniquenessValidator"

This reverts commit 8f0a2b6d780347a5ce258ac1a6a6902ce9695ca1.

7 files changed, 47 insertions, 59 deletions

diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index 10d478b1eea..aa71f606657 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/app/controllers/projects/pipeline_schedules_controller.rb
@@ -33,8 +33,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   end
 
   def update
-    if Ci::UpdatePipelineScheduleService
-        .new(@project, current_user, schedule_params).execute(schedule)
+    if schedule.update(schedule_params)
       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 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/services/ci/create_pipeline_schedule_service.rb b/app/services/ci/create_pipeline_schedule_service.rb
index 7016592cd10..cd40deb6187 100644
--- a/app/services/ci/create_pipeline_schedule_service.rb
+++ b/app/services/ci/create_pipeline_schedule_service.rb
@@ -1,22 +1,13 @@
 module Ci
   class CreatePipelineScheduleService < BaseService
     def execute
-      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
+      project.pipeline_schedules.create(pipeline_schedule_params)
     end
 
     private
 
     def pipeline_schedule_params
-      @pipeline_schedule_params ||= params.merge(owner: current_user)
+      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
deleted file mode 100644
index 0ab84e19583..00000000000
--- a/app/services/ci/update_pipeline_schedule_service.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-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
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/lib/ci/nested_uniqueness_validator.rb b/lib/ci/nested_uniqueness_validator.rb
deleted file mode 100644
index 7c6a109b0d8..00000000000
--- a/lib/ci/nested_uniqueness_validator.rb
+++ /dev/null
@@ -1,11 +0,0 @@
-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 aa3aa06f917..bfd31f9409b 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -267,7 +267,8 @@ describe Projects::PipelineSchedulesController do
           end
 
           it 'returns an error that variables are duplciated' do
-            go
+            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
@@ -339,21 +340,6 @@ 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