From cccb0e6b9327fb562b72007a012933c9c61a33fa Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Sun, 9 Aug 2009 21:47:32 -0500 Subject: [PATCH 01/10] Add validates_format_of :without => /regexp/ option [Elliot Winkler, Peer Allan] [#430 state:resolved] Example : validates_format_of :subdomain, :without => /www|admin|mail/ Signed-off-by: Pratik Naik --- activemodel/CHANGELOG | 6 +++ .../lib/active_model/validations/format.rb | 37 +++++++++++++++---- .../validations/format_validation_test.rb | 29 +++++++++++++++ 3 files changed, 64 insertions(+), 8 deletions(-) diff --git a/activemodel/CHANGELOG b/activemodel/CHANGELOG index 142038cc87..26500568ee 100644 --- a/activemodel/CHANGELOG +++ b/activemodel/CHANGELOG @@ -1,5 +1,11 @@ *Edge* +* Add validates_format_of :without => /regexp/ option. #430 [Elliot Winkler, Peer Allan] + + Example : + + validates_format_of :subdomain, :without => /www|admin|mail/ + * Introduce validates_with to encapsulate attribute validations in a class. #2630 [Jeff Dean] * Extracted from Active Record and Active Resource. diff --git a/activemodel/lib/active_model/validations/format.rb b/activemodel/lib/active_model/validations/format.rb index 6f3b668bf0..3b3dd4b827 100644 --- a/activemodel/lib/active_model/validations/format.rb +++ b/activemodel/lib/active_model/validations/format.rb @@ -1,22 +1,30 @@ module ActiveModel module Validations module ClassMethods - # Validates whether the value of the specified attribute is of the correct form by matching it against the regular expression - # provided. + # Validates whether the value of the specified attribute is of the correct form, going by the regular expression provided. + # You can require that the attribute matches the regular expression: # # class Person < ActiveRecord::Base # validates_format_of :email, :with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i, :on => :create # end # + # Alternatively, you can require that the specified attribute does _not_ match the regular expression: + # + # class Person < ActiveRecord::Base + # validates_format_of :email, :without => /NOSPAM/ + # end + # # Note: use \A and \Z to match the start and end of the string, ^ and $ match the start/end of a line. # - # A regular expression must be provided or else an exception will be raised. + # You must pass either :with or :without as an option. In addition, both must be a regular expression, + # or else an exception will be raised. # # Configuration options: # * :message - A custom error message (default is: "is invalid"). # * :allow_nil - If set to true, skips this validation if the attribute is +nil+ (default is +false+). # * :allow_blank - If set to true, skips this validation if the attribute is blank (default is +false+). - # * :with - The regular expression used to validate the format with (note: must be supplied!). + # * :with - Regular expression that if the attribute matches will result in a successful validation. + # * :without - Regular expression that if the attribute does not match will result in a successful validation. # * :on - Specifies when this validation is active (default is :save, other options :create, :update). # * :if - Specifies a method, proc or string to call to determine if the validation should # occur (e.g. :if => :allow_validation, or :if => Proc.new { |user| user.signup_step > 2 }). The @@ -25,13 +33,26 @@ module ActiveModel # not occur (e.g. :unless => :skip_validation, or :unless => Proc.new { |user| user.signup_step <= 2 }). The # method, proc or string should return or evaluate to a true or false value. def validates_format_of(*attr_names) - configuration = { :with => nil } - configuration.update(attr_names.extract_options!) + configuration = attr_names.extract_options! - raise(ArgumentError, "A regular expression must be supplied as the :with option of the configuration hash") unless configuration[:with].is_a?(Regexp) + unless configuration.include?(:with) ^ configuration.include?(:without) # ^ == xor, or "exclusive or" + raise ArgumentError, "Either :with or :without must be supplied (but not both)" + end + + if configuration[:with] && !configuration[:with].is_a?(Regexp) + raise ArgumentError, "A regular expression must be supplied as the :with option of the configuration hash" + end + + if configuration[:without] && !configuration[:without].is_a?(Regexp) + raise ArgumentError, "A regular expression must be supplied as the :without option of the configuration hash" + end validates_each(attr_names, configuration) do |record, attr_name, value| - unless value.to_s =~ configuration[:with] + if configuration[:with] && value.to_s !~ configuration[:with] + record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) + end + + if configuration[:without] && value.to_s =~ configuration[:without] record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) end end diff --git a/activemodel/test/cases/validations/format_validation_test.rb b/activemodel/test/cases/validations/format_validation_test.rb index 2c06a9dd02..e19e4bf7b3 100644 --- a/activemodel/test/cases/validations/format_validation_test.rb +++ b/activemodel/test/cases/validations/format_validation_test.rb @@ -71,6 +71,35 @@ class PresenceValidationTest < ActiveModel::TestCase assert_equal ["can't be Invalid title"], t.errors[:title] end + def test_validate_format_with_not_option + Topic.validates_format_of(:title, :without => /foo/, :message => "should not contain foo") + t = Topic.new + + t.title = "foobar" + t.valid? + assert_equal ["should not contain foo"], t.errors[:title] + + t.title = "something else" + t.valid? + assert_equal [], t.errors[:title] + end + + def test_validate_format_of_without_any_regexp_should_raise_error + assert_raise(ArgumentError) { Topic.validates_format_of(:title) } + end + + def test_validates_format_of_with_both_regexps_should_raise_error + assert_raise(ArgumentError) { Topic.validates_format_of(:title, :with => /this/, :without => /that/) } + end + + def test_validates_format_of_when_with_isnt_a_regexp_should_raise_error + assert_raise(ArgumentError) { Topic.validates_format_of(:title, :with => "clearly not a regexp") } + end + + def test_validates_format_of_when_not_isnt_a_regexp_should_raise_error + assert_raise(ArgumentError) { Topic.validates_format_of(:title, :without => "clearly not a regexp") } + end + def test_validates_format_of_with_custom_error_using_quotes repair_validations(Developer) do Developer.validates_format_of :name, :with => /^(A-Z*)$/, :message=> "format 'single' and \"double\" quotes" From e202c6c814886251d3c7a9b6a33ba6a8f1a2d448 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 10 Aug 2009 15:24:48 +0100 Subject: [PATCH 02/10] Move :with/:without check outside the method generated by validates_format_of --- activemodel/lib/active_model/validations/format.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/activemodel/lib/active_model/validations/format.rb b/activemodel/lib/active_model/validations/format.rb index 3b3dd4b827..c670dafc7c 100644 --- a/activemodel/lib/active_model/validations/format.rb +++ b/activemodel/lib/active_model/validations/format.rb @@ -47,13 +47,13 @@ module ActiveModel raise ArgumentError, "A regular expression must be supplied as the :without option of the configuration hash" end - validates_each(attr_names, configuration) do |record, attr_name, value| - if configuration[:with] && value.to_s !~ configuration[:with] - record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) + if configuration[:with] + validates_each(attr_names, configuration) do |record, attr_name, value| + record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) if value.to_s !~ configuration[:with] end - - if configuration[:without] && value.to_s =~ configuration[:without] - record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) + elsif configuration[:without] + validates_each(attr_names, configuration) do |record, attr_name, value| + record.errors.add(attr_name, :invalid, :default => configuration[:message], :value => value) if value.to_s =~ configuration[:without] end end end From 55b5cf586ac448aedd4e6f38e368607e729feb48 Mon Sep 17 00:00:00 2001 From: Arthur Zapparoli Date: Mon, 10 Aug 2009 12:07:05 -0300 Subject: [PATCH 03/10] Fixed typo in test name and CHANGELOG [#3017 state:resolved] Signed-off-by: Pratik Naik --- activerecord/CHANGELOG | 4 ++-- .../test/cases/validations/uniqueness_validation_test.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index d40251b9ca..acfd470c95 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -2199,7 +2199,7 @@ during calendar reform. #7649, #7724 [fedot, Geoff Buesing] * Escape database name in MySQL adapter when creating and dropping databases. #3409 [anna@wota.jp] -* Disambiguate table names for columns in validates_uniquness_of's WHERE clause. #3423 [alex.borovsky@gmail.com] +* Disambiguate table names for columns in validates_uniqueness_of's WHERE clause. #3423 [alex.borovsky@gmail.com] * .with_scope imposed create parameters now bypass attr_protected [Tobias Lütke] @@ -3714,7 +3714,7 @@ in effect. Added :readonly finder constraint. Calling an association collectio * Escape database name in MySQL adapter when creating and dropping databases. #3409 [anna@wota.jp] -* Disambiguate table names for columns in validates_uniquness_of's WHERE clause. #3423 [alex.borovsky@gmail.com] +* Disambiguate table names for columns in validates_uniqueness_of's WHERE clause. #3423 [alex.borovsky@gmail.com] * .with_scope imposed create parameters now bypass attr_protected [Tobias Lütke] diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb index cb123d3498..17ba4e2f8a 100644 --- a/activerecord/test/cases/validations/uniqueness_validation_test.rb +++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb @@ -59,7 +59,7 @@ class UniquenessValidationTest < ActiveRecord::TestCase assert t2.save, "Should now save t2 as unique" end - def test_validates_uniquness_with_newline_chars + def test_validates_uniqueness_with_newline_chars Topic.validates_uniqueness_of(:title, :case_sensitive => false) t = Topic.new("title" => "new\nline") From d574cb31f0406e267edb0e9ed1ffc7998d0da1ee Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 10 Aug 2009 11:53:10 -0500 Subject: [PATCH 04/10] Centralize attr method name concatenation in AttributeMethodMatch --- .../lib/active_model/attribute_methods.rb | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index de80559036..1ae042e00f 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -135,16 +135,19 @@ module ActiveModel def define_attribute_methods(attr_names) return if attribute_methods_generated? - attr_names.each do |name| - attribute_method_matchers.each do |method| - method_name = "#{method.prefix}#{name}#{method.suffix}" - unless instance_method_already_implemented?(method_name) - generate_method = "define_method_#{method.prefix}attribute#{method.suffix}" + attr_names.each do |attr_name| + attribute_method_matchers.each do |matcher| + unless instance_method_already_implemented?(matcher.method_name(attr_name)) + generate_method = "define_method_#{matcher.prefix}attribute#{matcher.suffix}" if respond_to?(generate_method) - send(generate_method, name) + send(generate_method, attr_name) else - generated_attribute_methods.module_eval("def #{method_name}(*args); send(:#{method.prefix}attribute#{method.suffix}, '#{name}', *args); end", __FILE__, __LINE__) + generated_attribute_methods.module_eval <<-STR, __FILE__, __LINE__+1 + def #{matcher.method_name(attr_name)}(*args) + send(:#{matcher.method_missing_target}, '#{attr_name}', *args) + end + STR end end end @@ -180,7 +183,7 @@ module ActiveModel class AttributeMethodMatcher attr_reader :prefix, :suffix - AttributeMethodMatch = Struct.new(:prefix, :base, :suffix) + AttributeMethodMatch = Struct.new(:target, :attr_name) def initialize(options = {}) options.symbolize_keys! @@ -190,11 +193,19 @@ module ActiveModel def match(method_name) if matchdata = @regex.match(method_name) - AttributeMethodMatch.new(matchdata[1], matchdata[2], matchdata[3]) + AttributeMethodMatch.new(method_missing_target, matchdata[2]) else nil end end + + def method_name(attr_name) + "#{prefix}#{attr_name}#{suffix}" + end + + def method_missing_target + :"#{prefix}attribute#{suffix}" + end end def attribute_method_matchers #:nodoc: @@ -214,7 +225,7 @@ module ActiveModel method_name = method_id.to_s if match = match_attribute_method?(method_name) guard_private_attribute_method!(method_name, args) - return __send__("#{match.prefix}attribute#{match.suffix}", match.base, *args, &block) + return __send__(match.target, match.attr_name, *args, &block) end super end @@ -246,7 +257,7 @@ module ActiveModel # The struct's attributes are prefix, base and suffix. def match_attribute_method?(method_name) self.class.send(:attribute_method_matchers).each do |method| - if (match = method.match(method_name)) && attribute_method?(match.base) + if (match = method.match(method_name)) && attribute_method?(match.attr_name) return match end end From 391f978acdf9b4789f9ac301a72b99e05ace64f1 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 10 Aug 2009 11:58:44 -0500 Subject: [PATCH 05/10] AMo overrides alias_attribute and manages aliasing all known attribute method matchers --- .../lib/active_model/attribute_methods.rb | 10 ++++++++++ .../active_record/attribute_methods/dirty.rb | 17 ----------------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index 1ae042e00f..1091ad3095 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -133,6 +133,16 @@ module ActiveModel undefine_attribute_methods end + def alias_attribute(new_name, old_name) + attribute_method_matchers.each do |matcher| + module_eval <<-STR, __FILE__, __LINE__+1 + def #{matcher.method_name(new_name)}(*args) + send(:#{matcher.method_name(old_name)}, *args) + end + STR + end + end + def define_attribute_methods(attr_names) return if attribute_methods_generated? attr_names.each do |attr_name| diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index 911c908c8b..37a46f7d88 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -182,23 +182,6 @@ module ActiveRecord old != value end - - module ClassMethods - def self.extended(base) - class << base - alias_method_chain :alias_attribute, :dirty - end - end - - def alias_attribute_with_dirty(new_name, old_name) - alias_attribute_without_dirty(new_name, old_name) - DIRTY_AFFIXES.each do |affixes| - module_eval <<-STR, __FILE__, __LINE__+1 - def #{affixes[:prefix]}#{new_name}#{affixes[:suffix]}; self.#{affixes[:prefix]}#{old_name}#{affixes[:suffix]}; end # def reset_subject!; self.reset_title!; end - STR - end - end - end end end end From f97dae5ebe2f19273d3f92e5ea9baba788c8e89f Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Mon, 10 Aug 2009 13:51:48 -0500 Subject: [PATCH 06/10] Extract common dirty tracking methods in AMo --- activemodel/lib/active_model.rb | 1 + activemodel/lib/active_model/dirty.rb | 112 ++++++++++++++++ .../active_record/attribute_methods/dirty.rb | 123 ++---------------- 3 files changed, 122 insertions(+), 114 deletions(-) create mode 100644 activemodel/lib/active_model/dirty.rb diff --git a/activemodel/lib/active_model.rb b/activemodel/lib/active_model.rb index 9bb4cf8b54..b24a929ff5 100644 --- a/activemodel/lib/active_model.rb +++ b/activemodel/lib/active_model.rb @@ -29,6 +29,7 @@ module ActiveModel autoload :AttributeMethods, 'active_model/attribute_methods' autoload :Conversion, 'active_model/conversion' autoload :DeprecatedErrorMethods, 'active_model/deprecated_error_methods' + autoload :Dirty, 'active_model/dirty' autoload :Errors, 'active_model/errors' autoload :Name, 'active_model/naming' autoload :Naming, 'active_model/naming' diff --git a/activemodel/lib/active_model/dirty.rb b/activemodel/lib/active_model/dirty.rb new file mode 100644 index 0000000000..624c3647ca --- /dev/null +++ b/activemodel/lib/active_model/dirty.rb @@ -0,0 +1,112 @@ +module ActiveModel + # Track unsaved attribute changes. + # + # A newly instantiated object is unchanged: + # person = Person.find_by_name('Uncle Bob') + # person.changed? # => false + # + # Change the name: + # person.name = 'Bob' + # person.changed? # => true + # person.name_changed? # => true + # person.name_was # => 'Uncle Bob' + # person.name_change # => ['Uncle Bob', 'Bob'] + # person.name = 'Bill' + # person.name_change # => ['Uncle Bob', 'Bill'] + # + # Save the changes: + # person.save + # person.changed? # => false + # person.name_changed? # => false + # + # Assigning the same value leaves the attribute unchanged: + # person.name = 'Bill' + # person.name_changed? # => false + # person.name_change # => nil + # + # Which attributes have changed? + # person.name = 'Bob' + # person.changed # => ['name'] + # person.changes # => { 'name' => ['Bill', 'Bob'] } + # + # Resetting an attribute returns it to its original state: + # person.reset_name! # => 'Bill' + # person.changed? # => false + # person.name_changed? # => false + # person.name # => 'Bill' + # + # Before modifying an attribute in-place: + # person.name_will_change! + # person.name << 'y' + # person.name_change # => ['Bill', 'Billy'] + module Dirty + extend ActiveSupport::Concern + include ActiveModel::AttributeMethods + + included do + attribute_method_suffix '_changed?', '_change', '_will_change!', '_was' + attribute_method_affix :prefix => 'reset_', :suffix => '!' + end + + # Do any attributes have unsaved changes? + # person.changed? # => false + # person.name = 'bob' + # person.changed? # => true + def changed? + !changed_attributes.empty? + end + + # List of attributes with unsaved changes. + # person.changed # => [] + # person.name = 'bob' + # person.changed # => ['name'] + def changed + changed_attributes.keys + end + + # Map of changed attrs => [original value, new value]. + # person.changes # => {} + # person.name = 'bob' + # person.changes # => { 'name' => ['bill', 'bob'] } + def changes + changed.inject({}) { |h, attr| h[attr] = attribute_change(attr); h } + end + + private + # Map of change attr => original value. + def changed_attributes + @changed_attributes ||= {} + end + + # Handle *_changed? for +method_missing+. + def attribute_changed?(attr) + changed_attributes.include?(attr) + end + + # Handle *_change for +method_missing+. + def attribute_change(attr) + [changed_attributes[attr], __send__(attr)] if attribute_changed?(attr) + end + + # Handle *_was for +method_missing+. + def attribute_was(attr) + attribute_changed?(attr) ? changed_attributes[attr] : __send__(attr) + end + + # Handle *_will_change! for +method_missing+. + def attribute_will_change!(attr) + begin + value = __send__(attr) + value = value.duplicable? ? value.clone : value + rescue TypeError, NoMethodError + end + + changed_attributes[attr] = value + end + + # Handle reset_*! for +method_missing+. + def reset_attribute!(attr) + __send__("#{attr}=", changed_attributes[attr]) if attribute_changed?(attr) + end + end +end diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index 37a46f7d88..b6c4df2a49 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -1,92 +1,21 @@ +require 'active_support/core_ext/object/tap' + module ActiveRecord module AttributeMethods - # Track unsaved attribute changes. - # - # A newly instantiated object is unchanged: - # person = Person.find_by_name('Uncle Bob') - # person.changed? # => false - # - # Change the name: - # person.name = 'Bob' - # person.changed? # => true - # person.name_changed? # => true - # person.name_was # => 'Uncle Bob' - # person.name_change # => ['Uncle Bob', 'Bob'] - # person.name = 'Bill' - # person.name_change # => ['Uncle Bob', 'Bill'] - # - # Save the changes: - # person.save - # person.changed? # => false - # person.name_changed? # => false - # - # Assigning the same value leaves the attribute unchanged: - # person.name = 'Bill' - # person.name_changed? # => false - # person.name_change # => nil - # - # Which attributes have changed? - # person.name = 'Bob' - # person.changed # => ['name'] - # person.changes # => { 'name' => ['Bill', 'Bob'] } - # - # Resetting an attribute returns it to its original state: - # person.reset_name! # => 'Bill' - # person.changed? # => false - # person.name_changed? # => false - # person.name # => 'Bill' - # - # Before modifying an attribute in-place: - # person.name_will_change! - # person.name << 'y' - # person.name_change # => ['Bill', 'Billy'] module Dirty extend ActiveSupport::Concern - - DIRTY_AFFIXES = [ - { :suffix => '_changed?' }, - { :suffix => '_change' }, - { :suffix => '_will_change!' }, - { :suffix => '_was' }, - { :prefix => 'reset_', :suffix => '!' } - ] + include ActiveModel::Dirty included do - attribute_method_affix *DIRTY_AFFIXES - - alias_method_chain :save, :dirty - alias_method_chain :save!, :dirty - alias_method_chain :update, :dirty - alias_method_chain :reload, :dirty + alias_method_chain :save, :dirty + alias_method_chain :save!, :dirty + alias_method_chain :update, :dirty + alias_method_chain :reload, :dirty superclass_delegating_accessor :partial_updates self.partial_updates = true end - # Do any attributes have unsaved changes? - # person.changed? # => false - # person.name = 'bob' - # person.changed? # => true - def changed? - !changed_attributes.empty? - end - - # List of attributes with unsaved changes. - # person.changed # => [] - # person.name = 'bob' - # person.changed # => ['name'] - def changed - changed_attributes.keys - end - - # Map of changed attrs => [original value, new value]. - # person.changes # => {} - # person.name = 'bob' - # person.changes # => { 'name' => ['bill', 'bob'] } - def changes - changed.inject({}) { |h, attr| h[attr] = attribute_change(attr); h } - end - # Attempts to +save+ the record and clears changed attributes if successful. def save_with_dirty(*args) #:nodoc: if status = save_without_dirty(*args) @@ -97,49 +26,15 @@ module ActiveRecord # Attempts to save! the record and clears changed attributes if successful. def save_with_dirty!(*args) #:nodoc: - status = save_without_dirty!(*args) - changed_attributes.clear - status + save_without_dirty!(*args).tap { changed_attributes.clear } end # reload the record and clears changed attributes. def reload_with_dirty(*args) #:nodoc: - record = reload_without_dirty(*args) - changed_attributes.clear - record + reload_without_dirty(*args).tap { changed_attributes.clear } end private - # Map of change attr => original value. - def changed_attributes - @changed_attributes ||= {} - end - - # Handle *_changed? for +method_missing+. - def attribute_changed?(attr) - changed_attributes.include?(attr) - end - - # Handle *_change for +method_missing+. - def attribute_change(attr) - [changed_attributes[attr], __send__(attr)] if attribute_changed?(attr) - end - - # Handle *_was for +method_missing+. - def attribute_was(attr) - attribute_changed?(attr) ? changed_attributes[attr] : __send__(attr) - end - - # Handle reset_*! for +method_missing+. - def reset_attribute!(attr) - self[attr] = changed_attributes[attr] if attribute_changed?(attr) - end - - # Handle *_will_change! for +method_missing+. - def attribute_will_change!(attr) - changed_attributes[attr] = clone_attribute_value(:read_attribute, attr) - end - # Wrap write_attribute to remember original attribute value. def write_attribute(attr, value) attr = attr.to_s From d9c4087a9e73ea31dbb83baa26c02904039fb480 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 10 Aug 2009 21:02:06 +0100 Subject: [PATCH 07/10] Unify hm:t#create and create! implementation --- .../has_many_through_association.rb | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 292da58831..c662bb8ec4 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -8,25 +8,11 @@ module ActiveRecord alias_method :new, :build def create!(attrs = nil) - ensure_owner_is_not_new - - transaction do - self << (object = attrs ? @reflection.klass.send(:with_scope, :create => attrs) { @reflection.create_association! } : @reflection.create_association!) - object - end + create_record(attrs, true) end def create(attrs = nil) - ensure_owner_is_not_new - - transaction do - object = attrs ? @reflection.klass.send(:with_scope, :create => attrs) { @reflection.create_association } : @reflection.create_association - raise_on_type_mismatch(object) - add_record_to_target_with_callbacks(object) do |r| - insert_record(object, false) - end - object - end + create_record(attrs, false) end def destroy(*records) @@ -44,8 +30,18 @@ module ActiveRecord return @target.size if loaded? return count end - + protected + def create_record(attrs, force = true) + ensure_owner_is_not_new + + transaction do + object = attrs ? @reflection.klass.send(:with_scope, :create => attrs) { @reflection.create_association } : @reflection.create_association + add_record_to_target_with_callbacks(object) {|r| insert_record(object, force) } + object + end + end + def target_reflection_has_associated_record? if @reflection.through_reflection.macro == :belongs_to && @owner[@reflection.through_reflection.primary_key_name].blank? false From 50b83984f111552b91a25b3b1d139f65efcc8e8f Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 10 Aug 2009 21:06:49 +0100 Subject: [PATCH 08/10] Remove unnecessary scoping and validation checks from hm:t#create --- .../active_record/associations/has_many_through_association.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index c662bb8ec4..f09289ada3 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -36,7 +36,7 @@ module ActiveRecord ensure_owner_is_not_new transaction do - object = attrs ? @reflection.klass.send(:with_scope, :create => attrs) { @reflection.create_association } : @reflection.create_association + object = @reflection.klass.new(attrs) add_record_to_target_with_callbacks(object) {|r| insert_record(object, force) } object end From ad28e0037b1d27494b846b1e65db874c37445e91 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 10 Aug 2009 21:20:01 +0100 Subject: [PATCH 09/10] Remove unnecessary scoping for creating hm:t join record --- .../associations/has_many_through_association.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index f09289ada3..829f0ac0c5 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -65,9 +65,10 @@ module ActiveRecord return false unless record.save(validate) end end - through_reflection = @reflection.through_reflection - klass = through_reflection.klass - @owner.send(@reflection.through_reflection.name).proxy_target << klass.send(:with_scope, :create => construct_join_attributes(record)) { through_reflection.create_association! } + + through_association = @owner.send(@reflection.through_reflection.name) + through_record = through_association.create!(construct_join_attributes(record)) + through_association.proxy_target << through_record end # TODO - add dependent option support From d0f891ae0215d7963b3918f3847ee4c015a6b90c Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 10 Aug 2009 21:29:48 +0100 Subject: [PATCH 10/10] Rewrite hm:t#create tests using assert_no_difference and assert_difference --- .../has_many_through_associations_test.rb | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 59985374d3..a9d4d88148 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -180,27 +180,23 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end def test_associate_with_create_and_invalid_options - peeps = companies(:first_firm).developers.count - assert_nothing_raised { companies(:first_firm).developers.create(:name => '0') } - assert_equal peeps, companies(:first_firm).developers.count + firm = companies(:first_firm) + assert_no_difference('firm.developers.count') { assert_nothing_raised { firm.developers.create(:name => '0') } } end def test_associate_with_create_and_valid_options - peeps = companies(:first_firm).developers.count - assert_nothing_raised { companies(:first_firm).developers.create(:name => 'developer') } - assert_equal peeps + 1, companies(:first_firm).developers.count + firm = companies(:first_firm) + assert_difference('firm.developers.count', 1) { firm.developers.create(:name => 'developer') } end def test_associate_with_create_bang_and_invalid_options - peeps = companies(:first_firm).developers.count - assert_raises(ActiveRecord::RecordInvalid) { companies(:first_firm).developers.create!(:name => '0') } - assert_equal peeps, companies(:first_firm).developers.count + firm = companies(:first_firm) + assert_no_difference('firm.developers.count') { assert_raises(ActiveRecord::RecordInvalid) { firm.developers.create!(:name => '0') } } end def test_associate_with_create_bang_and_valid_options - peeps = companies(:first_firm).developers.count - assert_nothing_raised { companies(:first_firm).developers.create!(:name => 'developer') } - assert_equal peeps + 1, companies(:first_firm).developers.count + firm = companies(:first_firm) + assert_difference('firm.developers.count', 1) { firm.developers.create!(:name => 'developer') } end def test_clear_associations