From edc176d33be9499f4c096779c5b4711b5daf0c06 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 15 Oct 2010 17:46:09 +0100 Subject: [PATCH] Make sure nested through associations are read only --- .../lib/active_record/associations.rb | 6 +++ .../has_many_through_association.rb | 10 +++++ .../has_one_through_association.rb | 2 + .../associations/through_association_scope.rb | 27 ++++++++---- activerecord/lib/active_record/reflection.rb | 4 ++ ...sted_has_many_through_associations_test.rb | 42 +++++++++++++++++++ 6 files changed, 82 insertions(+), 9 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 22a693540e..1111033435 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -64,6 +64,12 @@ module ActiveRecord super("Cannot dissociate new records through '#{owner.class.name}##{reflection.name}' on '#{reflection.source_reflection.class_name rescue nil}##{reflection.source_reflection.name rescue nil}'. Both records must have an id in order to delete the has_many :through record associating them.") end end + + class HasManyThroughNestedAssociationsAreReadonly < ActiveRecordError #:nodoc + def initialize(owner, reflection) + super("Cannot modify association '#{owner.class.name}##{reflection.name}' because it goes through more than one other association.") + end + end class HasAndBelongsToManyAssociationWithPrimaryKeyError < ActiveRecordError #:nodoc: def initialize(reflection) 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 313d9da621..f0ad166802 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -8,6 +8,11 @@ module ActiveRecord class HasManyThroughAssociation < HasManyAssociation #:nodoc: include ThroughAssociationScope + def build(attributes = {}, &block) + ensure_not_nested + super + end + alias_method :new, :build def create!(attrs = nil) @@ -37,6 +42,7 @@ module ActiveRecord protected def create_record(attrs, force = true) + ensure_not_nested ensure_owner_is_not_new transaction do @@ -60,6 +66,8 @@ module ActiveRecord end def insert_record(record, force = true, validate = true) + ensure_not_nested + if record.new_record? if force record.save! @@ -75,6 +83,8 @@ module ActiveRecord # TODO - add dependent option support def delete_records(records) + ensure_not_nested + klass = @reflection.through_reflection.klass records.each do |associate| klass.delete_all(construct_join_attributes(associate)) diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index fba0a2bfcc..8153eb7c57 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -14,6 +14,8 @@ module ActiveRecord private def create_through_record(new_value) #nodoc: + ensure_not_nested + klass = @reflection.through_reflection.klass current_object = @owner.send(@reflection.through_reflection.name) diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index a52672eecd..51ab8869ed 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -8,15 +8,18 @@ module ActiveRecord protected def construct_scope - { :create => construct_owner_attributes(@reflection), - :find => { :conditions => construct_conditions, - :joins => construct_joins, - :include => @reflection.options[:include] || @reflection.source_reflection.options[:include], - :select => construct_select, - :order => @reflection.options[:order], - :limit => @reflection.options[:limit], - :readonly => @reflection.options[:readonly], - } } + scope = {} + scope[:find] = { + :conditions => construct_conditions, + :joins => construct_joins, + :include => @reflection.options[:include] || @reflection.source_reflection.options[:include], + :select => construct_select, + :order => @reflection.options[:order], + :limit => @reflection.options[:limit], + :readonly => @reflection.options[:readonly] + } + scope[:create] = construct_owner_attributes(@reflection) unless @reflection.nested? + scope end # Build SQL conditions from attributes, qualified by table name. @@ -299,6 +302,12 @@ module ActiveRecord end alias_method :sql_conditions, :conditions + + def ensure_not_nested + if @reflection.nested? + raise HasManyThroughNestedAssociationsAreReadonly.new(@owner, @reflection) + end + end end end end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index b7cd466e13..ee63fcfce2 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -395,6 +395,10 @@ module ActiveRecord chain end end + + def nested? + through_reflection_chain.length > 2 + end # Gets an array of possible :through source reflection names: # diff --git a/activerecord/test/cases/associations/nested_has_many_through_associations_test.rb b/activerecord/test/cases/associations/nested_has_many_through_associations_test.rb index 4d5152ed5d..03ec4281d8 100644 --- a/activerecord/test/cases/associations/nested_has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/nested_has_many_through_associations_test.rb @@ -363,6 +363,48 @@ class NestedHasManyThroughAssociationsTest < ActiveRecord::TestCase assert !scope.where("comments.type" => "SpecialComment").empty? assert !scope.where("comments.type" => "SubSpecialComment").empty? end + + def test_nested_has_many_through_writers_should_raise_error + david = authors(:david) + subscriber = subscribers(:first) + + assert_raises(ActiveRecord::HasManyThroughNestedAssociationsAreReadonly) do + david.subscribers = [subscriber] + end + + assert_raises(ActiveRecord::HasManyThroughNestedAssociationsAreReadonly) do + david.subscriber_ids = [subscriber.id] + end + + assert_raises(ActiveRecord::HasManyThroughNestedAssociationsAreReadonly) do + david.subscribers << subscriber + end + + assert_raises(ActiveRecord::HasManyThroughNestedAssociationsAreReadonly) do + david.subscribers.delete(subscriber) + end + + assert_raises(ActiveRecord::HasManyThroughNestedAssociationsAreReadonly) do + david.subscribers.clear + end + + assert_raises(ActiveRecord::HasManyThroughNestedAssociationsAreReadonly) do + david.subscribers.build + end + + assert_raises(ActiveRecord::HasManyThroughNestedAssociationsAreReadonly) do + david.subscribers.create + end + end + + def test_nested_has_one_through_writers_should_raise_error + groucho = members(:groucho) + founding = member_types(:founding) + + assert_raises(ActiveRecord::HasManyThroughNestedAssociationsAreReadonly) do + groucho.nested_member_type = founding + end + end private