From 8f0a2b6d780347a5ce258ac1a6a6902ce9695ca1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 4 Jul 2017 19:04:21 +0900 Subject: [PATCH] Implement Ci::NestedUniquenessValidator --- .../projects/pipeline_schedules_controller.rb | 3 +- app/models/ci/pipeline_schedule.rb | 5 --- .../ci/create_pipeline_schedule_service.rb | 13 ++++++- .../ci/update_pipeline_schedule_service.rb | 19 ++++++++++ .../uniqueness_of_in_memory_validator.rb | 37 ------------------- lib/ci/nested_uniqueness_validator.rb | 11 ++++++ .../pipeline_schedules_controller_spec.rb | 18 ++++++++- 7 files changed, 59 insertions(+), 47 deletions(-) create mode 100644 app/services/ci/update_pipeline_schedule_service.rb delete mode 100644 app/validators/uniqueness_of_in_memory_validator.rb create mode 100644 lib/ci/nested_uniqueness_validator.rb 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