From 217a0b04ea2185893aa0b15e63e911ef5d4b5c6e Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 12 Jun 2019 23:01:46 -0600 Subject: [PATCH] Presence: check before assigning empty string A previous commit updated the presence matcher so that when it is testing invalid values for the attribute in question, it now considers an empty string as one of those values. However, a string is not always a valid value for an attribute. For instance, an attribute that is decorated with `serialize` where the serialization class is Array cannot be set to a string at all, and doing so will raise an error immediately. With that in mind, this commit adds checks to ensure that it is safe to try an empty string. --- .../validate_presence_of_matcher.rb | 39 +++++- .../unit/helpers/active_model_versions.rb | 4 + spec/support/unit/helpers/model_builder.rb | 6 +- .../model_creation_strategies/active_model.rb | 87 +++++++----- .../validate_presence_of_matcher_spec.rb | 126 ++++++++++++++++-- 5 files changed, 212 insertions(+), 50 deletions(-) diff --git a/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb b/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb index 49ef78d8..65c47baf 100644 --- a/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb +++ b/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb @@ -239,7 +239,7 @@ validation for you? Instead of using `validate_presence_of`, try else values = [] - if !association_being_validated? + if attribute_accepts_string_values? values << '' end @@ -316,12 +316,16 @@ validation for you? Instead of using `validate_presence_of`, try end def belongs_to_association_being_validated? - association_being_validated? && + !!association_reflection && association_reflection.macro == :belongs_to end - def association_being_validated? - !!association_reflection + def attribute_accepts_string_values? + !association_reflection && ( + !attribute_type.respond_to?(:coder) || + !attribute_type.coder || + attribute_type.coder.object_class == String + ) end def association_name @@ -337,6 +341,14 @@ validation for you? Instead of using `validate_presence_of`, try model.reflect_on_association(@attribute) end + def attribute_type + if model.respond_to?(:attribute_types) + model.attribute_types[@attribute.to_s] + else + LegacyAttributeType.new(model, @attribute) + end + end + def presence_validation_exists_on_attribute? model._validators.include?(@attribute) end @@ -344,6 +356,25 @@ validation for you? Instead of using `validate_presence_of`, try def model @subject.class end + + class LegacyAttributeType + def initialize(model, attribute_name) + @model = model + @attribute_name = attribute_name + end + + def coder + if model.respond_to?(:serialized_attributes) + ActiveSupport::Deprecation.silence do + model.serialized_attributes[attribute_name.to_s] + end + end + end + + private + + attr_reader :model, :attribute_name + end end end end diff --git a/spec/support/unit/helpers/active_model_versions.rb b/spec/support/unit/helpers/active_model_versions.rb index db7037de..15468cf2 100644 --- a/spec/support/unit/helpers/active_model_versions.rb +++ b/spec/support/unit/helpers/active_model_versions.rb @@ -28,5 +28,9 @@ module UnitTests def active_model_supports_strict? active_model_version >= 3.2 end + + def active_model_supports_full_attributes_api? + active_model_version >= '5.2' + end end end diff --git a/spec/support/unit/helpers/model_builder.rb b/spec/support/unit/helpers/model_builder.rb index fe6058df..e2e44ebb 100644 --- a/spec/support/unit/helpers/model_builder.rb +++ b/spec/support/unit/helpers/model_builder.rb @@ -56,13 +56,9 @@ module UnitTests def define_active_model_class(class_name, options = {}, &block) attribute_names = options.delete(:accessors) { [] } - columns = attribute_names.reduce({}) do |hash, attribute_name| - hash.merge(attribute_name => nil) - end - UnitTests::ModelCreationStrategies::ActiveModel.call( class_name, - columns, + attribute_names, options, &block ) diff --git a/spec/support/unit/model_creation_strategies/active_model.rb b/spec/support/unit/model_creation_strategies/active_model.rb index 5f51bec3..45b5f755 100644 --- a/spec/support/unit/model_creation_strategies/active_model.rb +++ b/spec/support/unit/model_creation_strategies/active_model.rb @@ -1,13 +1,19 @@ module UnitTests module ModelCreationStrategies class ActiveModel - def self.call(name, columns = {}, options = {}, &block) - new(name, columns, options, &block).call + def self.call(name, attribute_names = [], options = {}, &block) + new(name, attribute_names, options, &block).call end - def initialize(name, columns = {}, options = {}, &block) + def initialize(name, attribute_names = [], options = {}, &block) @name = name - @columns = columns + @attribute_names = + if attribute_names.is_a?(Hash) + # mimicking columns + attribute_names.keys + else + attribute_names + end @options = options @model_customizers = [] @@ -22,7 +28,9 @@ module UnitTests def call ClassBuilder.define_class(name, Model).tap do |model| - model.columns = columns.keys + attribute_names.each do |attribute_name| + model.attribute(attribute_name) + end model_customizers.each do |block| run_block(model, block) @@ -30,12 +38,10 @@ module UnitTests end end - protected - - attr_reader :name, :columns, :model_customizers, :options - private + attr_reader :name, :attribute_names, :model_customizers, :options + def run_block(model, block) if block if block.arity == 0 @@ -46,29 +52,40 @@ module UnitTests end end - class Model - class << self - def columns - @_columns ||= [] - end + module PoorMansAttributes + extend ActiveSupport::Concern - def columns=(columns) - existing_columns = self.columns - new_columns = columns - existing_columns + included do + class_attribute :attribute_names - @_columns += new_columns + self.attribute_names = Set.new + end + module ClassMethods + def attribute(name) include attributes_module - attributes_module.module_eval do - new_columns.each do |column| - define_method(column) do - attributes[column] - end + name = name.to_sym - define_method("#{column}=") do |value| - attributes[column] = value - end + if ( + attribute_names.include?(name) && + attributes_module.instance_methods.include?(name) + ) + attributes_module.module_eval do + remove_method(name) + remove_method("#{name}=") + end + end + + self.attribute_names += [name] + + attributes_module.module_eval do + define_method(name) do + attributes[name] + end + + define_method("#{name}=") do |value| + attributes[name] = value end end end @@ -80,8 +97,6 @@ module UnitTests end end - include ::ActiveModel::Model - attr_reader :attributes def initialize(attributes = {}) @@ -92,7 +107,7 @@ module UnitTests middle = '%s:0x%014x%s' % [ self.class, object_id * 2, - ' ' + inspected_attributes.join(' ') + ' ' + inspected_attributes.join(' '), ] "#<#{middle.strip}>" @@ -101,11 +116,21 @@ module UnitTests private def inspected_attributes - self.class.columns.map do |key, value| - "#{key}: #{attributes[key].inspect}" + self.class.attribute_names.map do |name| + "#{name}: #{attributes[name].inspect}" end end end + + class Model + include ::ActiveModel::Model + + if defined?(::ActiveModel::Attributes) + include ::ActiveModel::Attributes + else + include PoorMansAttributes + end + end end end end diff --git a/spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb b/spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb index 57f80be3..76301d27 100644 --- a/spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb @@ -55,6 +55,50 @@ this could not be proved. expect(&assertion).to fail_with_message(message) end + + context 'when the attribute is decorated with serialize' do + context 'and the type is a string' do + it 'still works' do + record = record_validating_presence_of(:traits) do + serialize :traits, String + end + + expect(record).to validate_presence_of(:traits) + end + end + + context 'and the type is not a string' do + it 'still works' do + record = record_validating_presence_of(:traits) do + serialize :traits, Array + end + + expect(record).to validate_presence_of(:traits) + end + end + end + + context 'when the column backing the attribute is a scalar, but not a string' do + it 'still works' do + record = record_validating_presence_of( + :pinned_on, + column_options: { type: :date }, + ) + + expect(record).to validate_presence_of(:pinned_on) + end + end + + context 'when the column backing the attribute is an array' do + it 'still works' do + record = record_validating_presence_of( + :possible_meeting_dates, + column_options: { type: :date, array: true }, + ) + + expect(record).to validate_presence_of(:possible_meeting_dates) + end + end end context 'a model without a presence validation' do @@ -112,6 +156,30 @@ could not be proved. } ) + if active_model_supports_full_attributes_api? + context 'when the attribute has been configured with a type' do + context 'and it is a string' do + it 'works' do + record = active_model_object_validating_presence_of(:age) do + attribute :age, :string + end + + expect(record).to validate_presence_of(:age) + end + end + + context 'and it is not a string' do + it 'still works' do + record = active_model_object_validating_presence_of(:age) do + attribute :age, :time + end + + expect(record).to validate_presence_of(:age) + end + end + end + end + def model_creator :active_model end @@ -120,7 +188,9 @@ could not be proved. context 'an ActiveModel class without a presence validation' do it 'rejects with the correct failure message' do assertion = lambda do - expect(active_model).to matcher + record = plain_active_model_object_with(:attr, model_name: 'Example') + + expect(record).to matcher end message = <<-MESSAGE @@ -826,22 +896,58 @@ could not be proved. validate_presence_of(:attr) end - def validating_presence(options = {}) - define_model :example, attr: :string do - validates_presence_of :attr, options - end.new + def record_validating_presence_of( + attribute_name = :attr, + column_options: { type: :string }, + **options, + &block + ) + model = define_model 'Example', attribute_name => column_options do + validates_presence_of(attribute_name, options) + + if block + class_eval(&block) + end + end + + model.new end + alias_method :validating_presence, :record_validating_presence_of def without_validating_presence define_model(:example, attr: :string).new end - def active_model(&block) - define_active_model_class('Example', accessors: [:attr], &block).new - end + def active_model_object_validating_presence_of( + attribute_name = :attr, + **options, + &block + ) + plain_active_model_object_with(attribute_name, **options) do + validates_presence_of(attribute_name) - def active_model_validating_presence - active_model { validates_presence_of :attr } + if block + class_eval(&block) + end + end + end + alias_method :active_model_validating_presence, + :active_model_object_validating_presence_of + + def plain_active_model_object_with( + attribute_name = :attr, + model_name: 'Example', + **options, + &block + ) + model = define_active_model_class( + model_name, + accessors: [attribute_name], + **options, + &block + ) + + model.new end def has_many_children(options = {})