From 5853583f9b810d7186ad36801b3a97ed49f77799 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Wed, 22 Dec 2010 21:36:21 -0200 Subject: Allow registering javascript/stylesheet_expansions to existing symbols --- .../asset_tag_helpers/javascript_tag_helpers.rb | 7 +++++-- .../asset_tag_helpers/stylesheet_tag_helpers.rb | 7 +++++-- actionpack/test/template/asset_tag_helper_test.rb | 19 +++++++++++++++++++ 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_view/helpers/asset_tag_helpers/javascript_tag_helpers.rb b/actionpack/lib/action_view/helpers/asset_tag_helpers/javascript_tag_helpers.rb index 6581e1d6f2..c95808a219 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helpers/javascript_tag_helpers.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helpers/javascript_tag_helpers.rb @@ -59,7 +59,10 @@ module ActionView # # def register_javascript_expansion(expansions) - JavascriptIncludeTag.expansions.merge!(expansions) + js_expansions = JavascriptIncludeTag.expansions + expansions.each do |key, values| + js_expansions[key] = (js_expansions[key] || []) | Array(values) if values + end end end @@ -170,4 +173,4 @@ module ActionView end end -end \ No newline at end of file +end diff --git a/actionpack/lib/action_view/helpers/asset_tag_helpers/stylesheet_tag_helpers.rb b/actionpack/lib/action_view/helpers/asset_tag_helpers/stylesheet_tag_helpers.rb index d02b28d7f6..f3e041de95 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helpers/stylesheet_tag_helpers.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helpers/stylesheet_tag_helpers.rb @@ -44,7 +44,10 @@ module ActionView # # def register_stylesheet_expansion(expansions) - StylesheetIncludeTag.expansions.merge!(expansions) + style_expansions = StylesheetIncludeTag.expansions + expansions.each do |key, values| + style_expansions[key] = (style_expansions[key] || []) | Array(values) if values + end end end @@ -141,4 +144,4 @@ module ActionView end end -end \ No newline at end of file +end diff --git a/actionpack/test/template/asset_tag_helper_test.rb b/actionpack/test/template/asset_tag_helper_test.rb index 39ecafd072..4b4e13e1a7 100644 --- a/actionpack/test/template/asset_tag_helper_test.rb +++ b/actionpack/test/template/asset_tag_helper_test.rb @@ -54,6 +54,9 @@ class AssetTagHelperTest < ActionView::TestCase def teardown config.perform_caching = false ENV.delete('RAILS_ASSET_ID') + + JavascriptIncludeTag.expansions.clear + StylesheetIncludeTag.expansions.clear end AutoDiscoveryToTag = { @@ -268,6 +271,14 @@ class AssetTagHelperTest < ActionView::TestCase assert_dom_equal %(\n\n\n\n\n\n\n\n\n), javascript_include_tag('controls',:defaults, :robbery, 'effects') end + def test_registering_javascript_expansions_merges_with_existing_expansions + ENV["RAILS_ASSET_ID"] = "" + ActionView::Helpers::AssetTagHelper::register_javascript_expansion :can_merge => ['bank'] + ActionView::Helpers::AssetTagHelper::register_javascript_expansion :can_merge => ['robber'] + ActionView::Helpers::AssetTagHelper::register_javascript_expansion :can_merge => ['bank'] + assert_dom_equal %(\n), javascript_include_tag(:can_merge) + end + def test_custom_javascript_expansions_with_undefined_symbol ActionView::Helpers::AssetTagHelper::register_javascript_expansion :monkey => nil assert_raise(ArgumentError) { javascript_include_tag('first', :monkey, 'last') } @@ -327,6 +338,14 @@ class AssetTagHelperTest < ActionView::TestCase assert_raise(ArgumentError) { stylesheet_link_tag('first', :monkey, 'last') } end + def test_registering_stylesheet_expansions_merges_with_existing_expansions + ENV["RAILS_ASSET_ID"] = "" + ActionView::Helpers::AssetTagHelper::register_stylesheet_expansion :can_merge => ['bank'] + ActionView::Helpers::AssetTagHelper::register_stylesheet_expansion :can_merge => ['robber'] + ActionView::Helpers::AssetTagHelper::register_stylesheet_expansion :can_merge => ['bank'] + assert_dom_equal %(\n), stylesheet_link_tag(:can_merge) + end + def test_image_path ImagePathToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) } end -- cgit v1.2.3 From 186a1b11449a2b94030791b2c778a76b07bb099f Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 22 Dec 2010 17:30:57 -0800 Subject: build an AST rather than build SQL strings --- activerecord/lib/active_record/association_preload.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index f2094283a2..9504b00515 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -185,6 +185,9 @@ module ActiveRecord end def preload_has_and_belongs_to_many_association(records, reflection, preload_options={}) + + left = reflection.klass.arel_table + table_name = reflection.klass.quoted_table_name id_to_record_map, ids = construct_id_map(records) records.each {|record| record.send(reflection.name).loaded} @@ -193,9 +196,15 @@ module ActiveRecord conditions = "t0.#{reflection.primary_key_name} #{in_or_equals_for_ids(ids)}" conditions << append_conditions(reflection, preload_options) + right = Arel::Table.new(options[:join_table]).alias('t0') + condition = left[reflection.klass.primary_key].eq( + right[reflection.association_foreign_key]) + + join = left.create_join(right, left.create_on(condition)) + associated_records_proxy = reflection.klass.unscoped. includes(options[:include]). - joins("INNER JOIN #{connection.quote_table_name options[:join_table]} t0 ON #{reflection.klass.quoted_table_name}.#{reflection.klass.primary_key} = t0.#{reflection.association_foreign_key}"). + joins(join). select("#{options[:select] || table_name+'.*'}, t0.#{reflection.primary_key_name} as the_parent_record_id"). order(options[:order]) -- cgit v1.2.3 From 2de9b858a09e735fa4e7705e929f945947e68dae Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 22 Dec 2010 18:02:49 -0800 Subject: to_sym stuff before passing it to arel --- activerecord/lib/active_record/base.rb | 2 +- activerecord/lib/active_record/relation/predicate_builder.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 858ccebbfa..6aac25ba9b 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -913,7 +913,7 @@ module ActiveRecord #:nodoc: end def type_condition - sti_column = arel_table[inheritance_column] + sti_column = arel_table[inheritance_column.to_sym] condition = sti_column.eq(sti_name) descendants.each { |subclass| condition = condition.or(sti_column.eq(subclass.sti_name)) } diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index 70d84619a1..d748eebc41 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -15,7 +15,7 @@ module ActiveRecord table = Arel::Table.new(table_name, :engine => engine) end - attribute = table[column] || Arel::Attribute.new(table, column) + attribute = table[column.to_sym] || Arel::Attribute.new(table, column) case value when Array, ActiveRecord::Associations::AssociationCollection, ActiveRecord::Relation -- cgit v1.2.3 From 6ca921a98cefa2bce67486f1ba2a8f52f4170d78 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 22 Dec 2010 18:05:30 -0800 Subject: use arel to compile SQL rather than build strings --- activerecord/lib/active_record/association_preload.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 9504b00515..9ac792fe3f 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -201,11 +201,18 @@ module ActiveRecord right[reflection.association_foreign_key]) join = left.create_join(right, left.create_on(condition)) + select = [ + # FIXME: options[:select] is always nil in the tests. Do we really + # need it? + options[:select] || left[Arel.star], + right[reflection.primary_key_name].as( + Arel.sql('the_parent_record_id')) + ] associated_records_proxy = reflection.klass.unscoped. includes(options[:include]). joins(join). - select("#{options[:select] || table_name+'.*'}, t0.#{reflection.primary_key_name} as the_parent_record_id"). + select(select). order(options[:order]) all_associated_records = associated_records(ids) do |some_ids| -- cgit v1.2.3 From 3e64336647fefe598ff3b0775401e2c2c5d378d3 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 22 Dec 2010 18:22:54 -0800 Subject: removing SQL interpolation, please use scoping and attribute conditionals as a replacement --- activerecord/CHANGELOG | 3 +++ activerecord/lib/active_record/association_preload.rb | 7 +------ activerecord/test/cases/associations/eager_test.rb | 4 ---- activerecord/test/models/post.rb | 3 --- 4 files changed, 4 insertions(+), 13 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index fd571c4ca4..8ebc87145c 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,8 @@ *Rails 3.1.0 (unreleased)* +* Removed support for interpolated SQL conditions. Please use scoping +along with attribute conditionals as a replacement. + * Added ActiveRecord::Base#has_secure_password (via ActiveModel::SecurePassword) to encapsulate dead-simple password usage with BCrypt encryption and salting [DHH]. Example: # Schema: User(name:string, password_digest:string, password_salt:string) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 9ac792fe3f..fc7c544a55 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -389,14 +389,9 @@ module ActiveRecord end end - - def interpolate_sql_for_preload(sql) - instance_eval("%@#{sql.gsub('@', '\@')}@", __FILE__, __LINE__) - end - def append_conditions(reflection, preload_options) sql = "" - sql << " AND (#{interpolate_sql_for_preload(reflection.sanitized_conditions)})" if reflection.sanitized_conditions + sql << " AND (#{reflection.sanitized_conditions})" if reflection.sanitized_conditions sql << " AND (#{sanitize_sql preload_options[:conditions]})" if preload_options[:conditions] sql end diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index d5262b1ee4..6efb0d57e4 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -658,10 +658,6 @@ class EagerAssociationTest < ActiveRecord::TestCase assert_equal people(:david, :susan), Person.find(:all, :include => [:readers, :primary_contact, :number1_fan], :conditions => "number1_fans_people.first_name like 'M%'", :order => 'people.id', :limit => 2, :offset => 0) end - def test_preload_with_interpolation - assert_equal [comments(:greetings)], Post.find(posts(:welcome).id, :include => :comments_with_interpolated_conditions).comments_with_interpolated_conditions - end - def test_polymorphic_type_condition post = Post.find(posts(:thinking).id, :include => :taggings) assert post.taggings.include?(taggings(:thinking_general)) diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 0083560ebe..b51f7ff48f 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -40,9 +40,6 @@ class Post < ActiveRecord::Base has_many :author_favorites, :through => :author has_many :author_categorizations, :through => :author, :source => :categorizations - has_many :comments_with_interpolated_conditions, :class_name => 'Comment', - :conditions => ['#{"#{aliased_table_name}." rescue ""}body = ?', 'Thank you for the welcome'] - has_one :very_special_comment has_one :very_special_comment_with_post, :class_name => "VerySpecialComment", :include => :post has_many :special_comments -- cgit v1.2.3 From 35f5938c9128718a2c5515524c815def75a53f00 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 22 Dec 2010 18:30:18 -0800 Subject: probably should use the some_ids variable here. o_O --- activerecord/lib/active_record/association_preload.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index fc7c544a55..7f5c5dd2ad 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -216,7 +216,7 @@ module ActiveRecord order(options[:order]) all_associated_records = associated_records(ids) do |some_ids| - associated_records_proxy.where([conditions, ids]).to_a + associated_records_proxy.where([conditions, some_ids]).to_a end set_association_collection_records(id_to_record_map, reflection.name, all_associated_records, 'the_parent_record_id') -- cgit v1.2.3 From 83ffb82fb9f53e1c90d3b598067494a12258b605 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 22 Dec 2010 19:17:44 -0800 Subject: Arel::Table#[] always returns an attribute, so no need for || --- activerecord/lib/active_record/relation/predicate_builder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index d748eebc41..6760d65186 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -15,7 +15,7 @@ module ActiveRecord table = Arel::Table.new(table_name, :engine => engine) end - attribute = table[column.to_sym] || Arel::Attribute.new(table, column) + attribute = table[column.to_sym] case value when Array, ActiveRecord::Associations::AssociationCollection, ActiveRecord::Relation -- cgit v1.2.3 From c7f81f14dfe02a2b3839b86d9fd47ce784aeeca6 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 22 Dec 2010 19:20:08 -0800 Subject: arel can escape the id, so avoid using the database connection --- activerecord/lib/active_record/relation/predicate_builder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index 6760d65186..b3f1b9e36a 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -26,7 +26,7 @@ module ActiveRecord when Range, Arel::Relation attribute.in(value) when ActiveRecord::Base - attribute.eq(Arel.sql(value.quoted_id)) + attribute.eq(value.id) when Class # FIXME: I think we need to deprecate this behavior attribute.eq(value.name) -- cgit v1.2.3 From d9c8c47e3db89ca75de6ae9a8497659378ef0c1d Mon Sep 17 00:00:00 2001 From: Raimonds Simanovskis Date: Thu, 23 Dec 2010 23:11:32 +0800 Subject: Fix for default_scope tests to ensure comparing of equally sorted lists This is additional fix for commit ebc47465a5865ab91dc7d058d2d8a0cc961510d7 Respect the default_scope on a join model when reading a through association which otherwise was failing on Oracle (as it returned fixture comments in different order). --- .../test/cases/associations/has_many_through_associations_test.rb | 2 +- .../test/cases/associations/has_one_through_associations_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 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 cf0eedbd13..816b43ab9e 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -466,7 +466,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end def test_has_many_through_with_default_scope_on_join_model - assert_equal posts(:welcome).comments, authors(:david).comments_on_first_posts + assert_equal posts(:welcome).comments.order('id').all, authors(:david).comments_on_first_posts end def test_create_has_many_through_with_default_scope_on_join_model diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb index 93a4f498d0..856214997c 100644 --- a/activerecord/test/cases/associations/has_one_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb @@ -235,6 +235,6 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase end def test_has_one_through_with_default_scope_on_join_model - assert_equal posts(:welcome).comments.first, authors(:david).comment_on_first_posts + assert_equal posts(:welcome).comments.order('id').first, authors(:david).comment_on_first_posts end end -- cgit v1.2.3 From 2b795050de72c6d68aba7513510f74ddc8959ee7 Mon Sep 17 00:00:00 2001 From: Raimonds Simanovskis Date: Thu, 23 Dec 2010 21:55:42 +0800 Subject: fixed retrieval of primary key value in Ralation#insert method previously primary key value was always assigned nil which caused Oracle enhanced adapter failing tests --- activerecord/lib/active_record/relation.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 7ecba1c43a..d4c5c85594 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -31,7 +31,7 @@ module ActiveRecord im = arel.compile_insert values im.into @table primary_key_name = @klass.primary_key - primary_key_value = Hash === values ? values[primary_key_name] : nil + primary_key_value = primary_key_name && Hash === values ? values[primary_key] : nil @klass.connection.insert( im.to_sql, -- cgit v1.2.3 From df3cfa6aaea326bb60b639524abbcc1f73854d1f Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 23 Dec 2010 07:33:22 -0800 Subject: avoid duping and new objects --- activerecord/lib/active_record/association_preload.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 7f5c5dd2ad..6d905fe6fe 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -211,10 +211,11 @@ module ActiveRecord associated_records_proxy = reflection.klass.unscoped. includes(options[:include]). - joins(join). - select(select). order(options[:order]) + associated_records_proxy.joins_values = [join] + associated_records_proxy.select_values = select + all_associated_records = associated_records(ids) do |some_ids| associated_records_proxy.where([conditions, some_ids]).to_a end -- cgit v1.2.3 From 819b8cae40623d26ce3c050d482b490539a25b98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 23 Dec 2010 19:17:02 +0100 Subject: Clean up callbacks should also be called on exceptions. --- actionpack/lib/action_dispatch/middleware/reloader.rb | 3 +++ actionpack/test/dispatch/reloader_test.rb | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/actionpack/lib/action_dispatch/middleware/reloader.rb b/actionpack/lib/action_dispatch/middleware/reloader.rb index 7624a1871a..f6ab368ad8 100644 --- a/actionpack/lib/action_dispatch/middleware/reloader.rb +++ b/actionpack/lib/action_dispatch/middleware/reloader.rb @@ -68,6 +68,9 @@ module ActionDispatch response = @app.call(env) response[2].extend(CleanupOnClose) response + rescue Exception + _run_cleanup_callbacks + raise end end end diff --git a/actionpack/test/dispatch/reloader_test.rb b/actionpack/test/dispatch/reloader_test.rb index 995b19030c..eaabc1feb3 100644 --- a/actionpack/test/dispatch/reloader_test.rb +++ b/actionpack/test/dispatch/reloader_test.rb @@ -116,6 +116,20 @@ class ReloaderTest < Test::Unit::TestCase assert cleaned end + def test_cleanup_callbacks_are_called_on_exceptions + cleaned = false + Reloader.to_cleanup { cleaned = true } + + begin + call_and_return_body do + raise "error" + end + rescue + end + + assert cleaned + end + private def call_and_return_body(&block) @reloader ||= Reloader.new(block || proc {[200, {}, 'response']}) -- cgit v1.2.3 From d6efd3cfc2c6d6822aeac550852c49135fbe46c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 23 Dec 2010 19:20:57 +0100 Subject: Don't deprecate to_prepare. --- actionpack/lib/action_dispatch/middleware/callbacks.rb | 8 ++++---- actionpack/test/dispatch/callbacks_test.rb | 12 +++++++----- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/actionpack/lib/action_dispatch/middleware/callbacks.rb b/actionpack/lib/action_dispatch/middleware/callbacks.rb index 5776a7bb27..4d038c29f2 100644 --- a/actionpack/lib/action_dispatch/middleware/callbacks.rb +++ b/actionpack/lib/action_dispatch/middleware/callbacks.rb @@ -1,3 +1,5 @@ +require 'active_support/core_ext/module/delegation' + module ActionDispatch # Provide callbacks to be executed before and after the request dispatch. class Callbacks @@ -5,10 +7,8 @@ module ActionDispatch define_callbacks :call, :rescuable => true - def self.to_prepare(*args, &block) - ActiveSupport::Deprecation.warn "ActionDispatch::Callbacks.to_prepare is deprecated. " << - "Please use ActionDispatch::Reloader.to_prepare instead." - ActionDispatch::Reloader.to_prepare(*args, &block) + class << self + delegate :to_prepare, :to_cleanup, :to => "ActionDispatch::Reloader" end def self.before(*args, &block) diff --git a/actionpack/test/dispatch/callbacks_test.rb b/actionpack/test/dispatch/callbacks_test.rb index 5becb621de..eed2eca2ab 100644 --- a/actionpack/test/dispatch/callbacks_test.rb +++ b/actionpack/test/dispatch/callbacks_test.rb @@ -29,14 +29,16 @@ class DispatcherTest < ActiveSupport::TestCase assert_equal 4, Foo.b end - def test_to_prepare_deprecation - prepared = false - assert_deprecated do - ActionDispatch::Callbacks.to_prepare { prepared = true } - end + def test_to_prepare_and_cleanup_delegation + prepared = cleaned = false + ActionDispatch::Callbacks.to_prepare { prepared = true } + ActionDispatch::Callbacks.to_prepare { cleaned = true } ActionDispatch::Reloader.prepare! assert prepared + + ActionDispatch::Reloader.cleanup! + assert cleaned end private -- cgit v1.2.3 From c6db37e69b1ff07f7ad535d4752d0e6eb2d15bff Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 19 Dec 2010 14:17:29 +0000 Subject: Don't allow a has_one association to go :through a collection association [#2976 state:resolved] --- activerecord/lib/active_record/associations.rb | 6 ++++++ activerecord/lib/active_record/reflection.rb | 4 ++++ .../cases/associations/has_one_through_associations_test.rb | 12 +++++++----- activerecord/test/models/author.rb | 4 +++- activerecord/test/models/member.rb | 9 ++++++--- 5 files changed, 26 insertions(+), 9 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index cdc8f25119..c0cd222244 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -32,6 +32,12 @@ module ActiveRecord end end + class HasOneThroughCantAssociateThroughCollection < ActiveRecordError #:nodoc: + def initialize(owner_class_name, reflection, through_reflection) + super("Cannot have a has_one :through association '#{owner_class_name}##{reflection.name}' where the :through association '#{owner_class_name}##{through_reflection.name}' is a collection. Specify a has_one or belongs_to association in the :through option instead.") + end + end + class HasManyThroughSourceAssociationNotFoundError < ActiveRecordError #:nodoc: def initialize(reflection) through_reflection = reflection.through_reflection diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 70165c600e..7a7f7812df 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -388,6 +388,10 @@ module ActiveRecord raise HasManyThroughSourceAssociationMacroError.new(self) end + if macro == :has_one && through_reflection.collection? + raise HasOneThroughCantAssociateThroughCollection.new(active_record.name, self, through_reflection) + end + check_validity_of_inverse! end diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb index 856214997c..f6307e6cf0 100644 --- a/activerecord/test/cases/associations/has_one_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb @@ -25,10 +25,6 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase assert_equal clubs(:boring_club), @member.club end - def test_has_one_through_with_has_many - assert_equal clubs(:moustache_club), @member.favourite_club - end - def test_creating_association_creates_through_record new_member = Member.create(:name => "Chris") new_member.club = Club.create(:name => "LRUG") @@ -235,6 +231,12 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase end def test_has_one_through_with_default_scope_on_join_model - assert_equal posts(:welcome).comments.order('id').first, authors(:david).comment_on_first_posts + assert_equal posts(:welcome).comments.order('id').first, authors(:david).comment_on_first_post + end + + def test_has_one_through_many_raises_exception + assert_raise(ActiveRecord::HasOneThroughCantAssociateThroughCollection) do + members(:groucho).club_through_many + end end end diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index fd6d2b384a..244c5ac4f5 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -28,7 +28,9 @@ class Author < ActiveRecord::Base has_many :first_posts has_many :comments_on_first_posts, :through => :first_posts, :source => :comments, :order => 'posts.id desc, comments.id asc' - has_one :comment_on_first_posts, :through => :first_posts, :source => :comments, :order => 'posts.id desc, comments.id asc' + + has_one :first_post + has_one :comment_on_first_post, :through => :first_post, :source => :comments, :order => 'posts.id desc, comments.id asc' has_many :thinking_posts, :class_name => 'Post', :conditions => { :title => 'So I was thinking' }, :dependent => :delete_all has_many :welcome_posts, :class_name => 'Post', :conditions => { :title => 'Welcome to the weblog' } diff --git a/activerecord/test/models/member.rb b/activerecord/test/models/member.rb index 255fb569d7..5b0a5ebf7f 100644 --- a/activerecord/test/models/member.rb +++ b/activerecord/test/models/member.rb @@ -1,12 +1,15 @@ class Member < ActiveRecord::Base has_one :current_membership - has_many :memberships + has_one :membership has_many :fellow_members, :through => :club, :source => :members has_one :club, :through => :current_membership - has_one :favourite_club, :through => :memberships, :conditions => ["memberships.favourite = ?", true], :source => :club + has_one :favourite_club, :through => :membership, :conditions => ["memberships.favourite = ?", true], :source => :club has_one :sponsor, :as => :sponsorable has_one :sponsor_club, :through => :sponsor has_one :member_detail has_one :organization, :through => :member_detail belongs_to :member_type -end \ No newline at end of file + + has_many :current_memberships + has_one :club_through_many, :through => :current_memberships, :source => :club +end -- cgit v1.2.3 From b79823832e6cd30a9f14f97ffdf1642d4d63d4ea Mon Sep 17 00:00:00 2001 From: Will Bryant Date: Thu, 13 Aug 2009 12:38:20 +1200 Subject: Verify that has_one :through preload respects the :conditions [#2976 state:resolved] --- .../cases/associations/has_one_through_associations_test.rb | 12 ++++++++++++ activerecord/test/models/member.rb | 1 + 2 files changed, 13 insertions(+) diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb index f6307e6cf0..f6365e0216 100644 --- a/activerecord/test/cases/associations/has_one_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb @@ -84,6 +84,18 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase assert_not_nil assert_no_queries {members[0].sponsor_club} end + def test_has_one_through_with_conditions_eager_loading + # conditions on the through table + assert_equal clubs(:moustache_club), Member.find(@member.id, :include => :favourite_club).favourite_club + memberships(:membership_of_favourite_club).update_attribute(:favourite, false) + assert_equal nil, Member.find(@member.id, :include => :favourite_club).favourite_club + + # conditions on the source table + assert_equal clubs(:moustache_club), Member.find(@member.id, :include => :hairy_club).hairy_club + clubs(:moustache_club).update_attribute(:name, "Association of Clean-Shaven Persons") + assert_equal nil, Member.find(@member.id, :include => :hairy_club).hairy_club + end + def test_has_one_through_polymorphic_with_source_type assert_equal members(:groucho), clubs(:moustache_club).sponsored_member end diff --git a/activerecord/test/models/member.rb b/activerecord/test/models/member.rb index 5b0a5ebf7f..15ad6aedd3 100644 --- a/activerecord/test/models/member.rb +++ b/activerecord/test/models/member.rb @@ -4,6 +4,7 @@ class Member < ActiveRecord::Base has_many :fellow_members, :through => :club, :source => :members has_one :club, :through => :current_membership has_one :favourite_club, :through => :membership, :conditions => ["memberships.favourite = ?", true], :source => :club + has_one :hairy_club, :through => :membership, :conditions => {:clubs => {:name => "Moustache and Eyebrow Fancier Club"}}, :source => :club has_one :sponsor, :as => :sponsorable has_one :sponsor_club, :through => :sponsor has_one :member_detail -- cgit v1.2.3 From 85683f2a79dbf81130361cb6426786cf6b0d1925 Mon Sep 17 00:00:00 2001 From: Szymon Nowak Date: Thu, 13 Aug 2009 21:18:43 +0200 Subject: Fix creation of has_many through records with custom primary_key option on belongs_to [#2990 state:resolved] --- .../associations/through_association_scope.rb | 7 ++++- .../has_many_through_associations_test.rb | 30 +++++++++++++++++++++- activerecord/test/models/author.rb | 1 + activerecord/test/models/categorization.rb | 1 + activerecord/test/schema/schema.rb | 1 + 5 files changed, 38 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 5dc5b0c048..99920d4c63 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -106,7 +106,12 @@ module ActiveRecord # TODO: revisit this to allow it for deletion, supposing dependent option is supported raise ActiveRecord::HasManyThroughCantAssociateThroughHasOneOrManyReflection.new(@owner, @reflection) if [:has_one, :has_many].include?(@reflection.source_reflection.macro) - join_attributes = construct_owner_attributes(@reflection.through_reflection).merge(@reflection.source_reflection.primary_key_name => associate.id) + join_attributes = construct_owner_attributes(@reflection.through_reflection) + + join_attributes.merge!( + @reflection.source_reflection.primary_key_name => + associate.send(@reflection.source_reflection.association_primary_key) + ) if @reflection.options[:source_type] join_attributes.merge!(@reflection.source_reflection.options[:foreign_type] => associate.class.base_class.name) 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 816b43ab9e..d237273464 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -21,7 +21,7 @@ require 'models/categorization' require 'models/category' class HasManyThroughAssociationsTest < ActiveRecord::TestCase - fixtures :posts, :readers, :people, :comments, :authors, + fixtures :posts, :readers, :people, :comments, :authors, :categories, :owners, :pets, :toys, :jobs, :references, :companies, :subscribers, :books, :subscriptions, :developers, :categorizations @@ -397,6 +397,34 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_equal people(:susan).agents.map(&:agents).flatten, people(:susan).agents_of_agents end + def test_associate_existing_with_nonstandard_primary_key_on_belongs_to + Categorization.create(:author => authors(:mary), :named_category_name => categories(:general).name) + assert_equal categories(:general), authors(:mary).named_categories.first + end + + def test_collection_build_with_nonstandard_primary_key_on_belongs_to + author = authors(:mary) + category = author.named_categories.build(:name => "Primary") + author.save + assert Categorization.exists?(:author_id => author.id, :named_category_name => category.name) + assert author.named_categories(true).include?(category) + end + + def test_collection_create_with_nonstandard_primary_key_on_belongs_to + author = authors(:mary) + category = author.named_categories.create(:name => "Primary") + assert Categorization.exists?(:author_id => author.id, :named_category_name => category.name) + assert author.named_categories(true).include?(category) + end + + def test_collection_delete_with_nonstandard_primary_key_on_belongs_to + author = authors(:mary) + category = author.named_categories.create(:name => "Primary") + author.named_categories.delete(category) + assert !Categorization.exists?(:author_id => author.id, :named_category_name => category.name) + assert author.named_categories(true).empty? + end + def test_collection_singular_ids_getter_with_string_primary_keys book = books(:awdr) assert_equal 2, book.subscriber_ids.size diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index 244c5ac4f5..83a6f5d926 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -78,6 +78,7 @@ class Author < ActiveRecord::Base has_many :categorizations has_many :categories, :through => :categorizations + has_many :named_categories, :through => :categorizations has_many :special_categorizations has_many :special_categories, :through => :special_categorizations, :source => :category diff --git a/activerecord/test/models/categorization.rb b/activerecord/test/models/categorization.rb index fdb0a11540..45f50e4af3 100644 --- a/activerecord/test/models/categorization.rb +++ b/activerecord/test/models/categorization.rb @@ -1,6 +1,7 @@ class Categorization < ActiveRecord::Base belongs_to :post belongs_to :category + belongs_to :named_category, :class_name => 'Category', :foreign_key => :named_category_name, :primary_key => :name belongs_to :author belongs_to :author_using_custom_pk, :class_name => 'Author', :foreign_key => :author_id, :primary_key => :author_address_extra_id diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 99761a4160..3dea7e1492 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -110,6 +110,7 @@ ActiveRecord::Schema.define do create_table :categorizations, :force => true do |t| t.column :category_id, :integer + t.string :named_category_name t.column :post_id, :integer t.column :author_id, :integer t.column :special, :boolean -- cgit v1.2.3 From 030480ac1f4fbf8bf74a0d9298544426caf26894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=81omnicki?= Date: Sun, 12 Dec 2010 01:37:56 +0100 Subject: Fix behaviour of foo.has_many_through_association.select('custom select') [#6089 state:resolved] --- .../lib/active_record/associations/through_association_scope.rb | 4 ++-- .../test/cases/associations/has_many_through_associations_test.rb | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 99920d4c63..c11fce5db0 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -63,8 +63,8 @@ module ActiveRecord end def construct_select(custom_select = nil) - distinct = "DISTINCT " if @reflection.options[:uniq] - custom_select || @reflection.options[:select] || "#{distinct}#{@reflection.quoted_table_name}.*" + distinct = "DISTINCT #{@reflection.quoted_table_name}.*" if @reflection.options[:uniq] + custom_select || @reflection.options[:select] || distinct end def construct_joins 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 d237273464..6b71e73718 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -514,4 +514,9 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_equal [posts(:eager_other)], posts end + + def test_select_chosen_fields_only + author = authors(:david) + assert_equal ['body'], author.comments.select('comments.body').first.attributes.keys + end end -- cgit v1.2.3 From ff7bde62c857ec94f45a5be3bc76468deb8b0b3a Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 22 Dec 2010 00:19:59 +0000 Subject: When a has_many association is not :uniq, appending the same record multiple times should append it to the @target multiple times [#5964 state:resolved] --- .../active_record/associations/association_collection.rb | 4 ++-- activerecord/lib/active_record/nested_attributes.rb | 13 ++++++++++++- .../associations/has_many_through_associations_test.rb | 10 ++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index c513e8ab08..7964f4fa2b 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -462,10 +462,10 @@ module ActiveRecord callback(:before_add, record) yield(record) if block_given? @target ||= [] unless loaded? - if index = @target.index(record) + if @reflection.options[:uniq] && index = @target.index(record) @target[index] = record else - @target << record + @target << record end callback(:after_add, record) set_inverse_instance(record, @owner) diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 050b521b6a..16023defe3 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -405,7 +405,18 @@ module ActiveRecord end elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s } - association.send(:add_record_to_target_with_callbacks, existing_record) if !association.loaded? && !call_reject_if(association_name, attributes) + unless association.loaded? || call_reject_if(association_name, attributes) + # Make sure we are operating on the actual object which is in the association's + # proxy_target array (either by finding it, or adding it if not found) + target_record = association.proxy_target.detect { |record| record == existing_record } + + if target_record + existing_record = target_record + else + association.send(:add_record_to_target_with_callbacks, existing_record) + end + end + assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) else 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 6b71e73718..dfb3292502 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -45,6 +45,16 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert posts(:thinking).reload.people(true).include?(people(:david)) end + def test_associate_existing_record_twice_should_add_to_target_twice + post = posts(:thinking) + person = people(:david) + + assert_difference 'post.people.to_a.count', 2 do + post.people << person + post.people << person + end + end + def test_associating_new assert_queries(1) { posts(:thinking) } new_person = nil # so block binding catches it -- cgit v1.2.3 From 4e13625818579551c01640cb405a8c22a3bd0e68 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 22 Dec 2010 10:11:27 +0000 Subject: Test demonstrating problem with foo.association_ids where it's a has_many :through with :conditions, with a belongs_to as the source reflection --- .../test/cases/associations/has_many_through_associations_test.rb | 4 ++++ 1 file changed, 4 insertions(+) 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 dfb3292502..ad67886202 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -529,4 +529,8 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase author = authors(:david) assert_equal ['body'], author.comments.select('comments.body').first.attributes.keys end + + def test_get_has_many_through_belongs_to_ids_with_conditions + assert_equal [categories(:general).id], authors(:mary).categories_like_general_ids + end end -- cgit v1.2.3 From 1619c2435b2b9c821b2b0dcab9624dbb6b23eaaa Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 22 Dec 2010 10:13:21 +0000 Subject: Revert "Optimize _ids for hm:t with belongs_to source". The optimisation has too many edge cases, such as when the reflection, source reflection, or through reflection has conditions, orders, etc. [#6153 state:resolved] This reverts commit 373b053dc8b99dac1abc3879a17a2bf8c30302b5. Conflicts: activerecord/lib/active_record/associations.rb --- activerecord/lib/active_record/associations.rb | 9 +-------- .../cases/associations/has_many_through_associations_test.rb | 8 ++------ 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index c0cd222244..e4dff69efe 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1512,16 +1512,9 @@ module ActiveRecord if send(reflection.name).loaded? || reflection.options[:finder_sql] send(reflection.name).map { |r| r.id } else - if reflection.through_reflection && reflection.source_reflection.belongs_to? - through = reflection.through_reflection - primary_key = reflection.source_reflection.primary_key_name - send(through.name).select("DISTINCT #{through.quoted_table_name}.#{primary_key}").map! { |r| r.send(primary_key) } - else - send(reflection.name).select("#{reflection.quoted_table_name}.#{reflection.klass.primary_key}").except(:includes).map! { |r| r.id } - end + send(reflection.name).select("#{reflection.quoted_table_name}.#{reflection.klass.primary_key}").except(:includes).map! { |r| r.id } end end - end def collection_accessor_methods(reflection, association_proxy_class, writer = true) 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 ad67886202..a244d310c8 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -334,12 +334,8 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_equal 2, people(:michael).jobs.size end - def test_get_ids_for_belongs_to_source - assert_sql(/DISTINCT/) { assert_equal [posts(:welcome).id, posts(:authorless).id].sort, people(:michael).post_ids.sort } - end - - def test_get_ids_for_has_many_source - assert_equal [comments(:eager_other_comment1).id], authors(:mary).comment_ids + def test_get_ids + assert_equal [posts(:welcome).id, posts(:authorless).id].sort, people(:michael).post_ids.sort end def test_get_ids_for_loaded_associations -- cgit v1.2.3 From 3f17ed407c5d61bc01fd59776205486c2350f36e Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 22 Dec 2010 11:45:37 +0000 Subject: Test to verify that #2189 (count with has_many :through and a named_scope) is fixed --- .../test/cases/associations/has_many_through_associations_test.rb | 5 +++++ activerecord/test/models/category.rb | 2 ++ 2 files changed, 7 insertions(+) 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 a244d310c8..a417345780 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -529,4 +529,9 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase def test_get_has_many_through_belongs_to_ids_with_conditions assert_equal [categories(:general).id], authors(:mary).categories_like_general_ids end + + def test_count_has_many_through_with_named_scope + assert_equal 2, authors(:mary).categories.count + assert_equal 1, authors(:mary).categories.general.count + end end diff --git a/activerecord/test/models/category.rb b/activerecord/test/models/category.rb index 48415846dd..06908ea85e 100644 --- a/activerecord/test/models/category.rb +++ b/activerecord/test/models/category.rb @@ -23,6 +23,8 @@ class Category < ActiveRecord::Base has_many :categorizations has_many :authors, :through => :categorizations, :select => 'authors.*, categorizations.post_id' + + scope :general, :conditions => { :name => 'General' } end class SpecialCategory < Category -- cgit v1.2.3 From 2d9626fc74c2d57f90c856c37e5967bbe6651bd8 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 22 Dec 2010 20:57:41 +0000 Subject: Improved strategy for updating a belongs_to association when the foreign key changes. Rather than resetting each affected association when the foreign key changes, we should lazily check for 'staleness' (where fk does not match target id) when the association is accessed. --- activerecord/lib/active_record/associations.rb | 43 +--------------------- .../associations/association_proxy.rb | 10 +++++ .../associations/belongs_to_association.rb | 8 ++++ .../belongs_to_polymorphic_association.rb | 9 +++++ activerecord/lib/active_record/reflection.rb | 5 ++- activerecord/test/cases/nested_attributes_test.rb | 5 ++- activerecord/test/cases/reflection_test.rb | 2 + 7 files changed, 37 insertions(+), 45 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index e4dff69efe..c6c41a7a12 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1229,14 +1229,12 @@ module ActiveRecord if reflection.options[:polymorphic] association_accessor_methods(reflection, BelongsToPolymorphicAssociation) - association_foreign_type_setter_method(reflection) else association_accessor_methods(reflection, BelongsToAssociation) association_constructor_method(:build, reflection, BelongsToAssociation) association_constructor_method(:create, reflection, BelongsToAssociation) end - association_foreign_key_setter_method(reflection) add_counter_cache_callbacks(reflection) if options[:counter_cache] add_touch_callbacks(reflection, options[:touch]) if options[:touch] @@ -1456,7 +1454,7 @@ module ActiveRecord force_reload = params.first unless params.empty? association = association_instance_get(reflection.name) - if association.nil? || force_reload + if association.nil? || force_reload || association.stale_target? association = association_proxy_class.new(self, reflection) retval = force_reload ? reflection.klass.uncached { association.reload } : association.reload if retval.nil? and association_proxy_class == BelongsToAssociation @@ -1556,45 +1554,6 @@ module ActiveRecord end end - def association_foreign_key_setter_method(reflection) - setters = reflect_on_all_associations(:belongs_to).map do |belongs_to_reflection| - if belongs_to_reflection.primary_key_name == reflection.primary_key_name - "association_instance_set(:#{belongs_to_reflection.name}, nil);" - end - end.compact.join - - if method_defined?(:"#{reflection.primary_key_name}=") - undef_method :"#{reflection.primary_key_name}=" - end - - class_eval <<-FILE, __FILE__, __LINE__ + 1 - def #{reflection.primary_key_name}=(new_id) - write_attribute :#{reflection.primary_key_name}, new_id - if #{reflection.primary_key_name}_changed? - #{ setters } - end - end - FILE - end - - def association_foreign_type_setter_method(reflection) - setters = reflect_on_all_associations(:belongs_to).map do |belongs_to_reflection| - if belongs_to_reflection.options[:foreign_type] == reflection.options[:foreign_type] - "association_instance_set(:#{belongs_to_reflection.name}, nil);" - end - end.compact.join - - field = reflection.options[:foreign_type] - class_eval <<-FILE, __FILE__, __LINE__ + 1 - def #{field}=(new_id) - write_attribute :#{field}, new_id - if #{field}_changed? - #{ setters } - end - end - FILE - end - def add_counter_cache_callbacks(reflection) cache_column = reflection.counter_cache_column diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index 252ff7e7ea..f4eceeed8c 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -130,6 +130,16 @@ module ActiveRecord @loaded = true end + # The target is stale if the target no longer points to the record(s) that the + # relevant foreign_key(s) refers to. If stale, the association accessor method + # on the owner will reload the target. It's up to subclasses to implement this + # method if relevant. + # + # Note that if the target has not been loaded, it is not considered stale. + def stale_target? + false + end + # Returns the target of this proxy, same as +proxy_target+. def target @target diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb index b438620c8f..c9abfe36e8 100644 --- a/activerecord/lib/active_record/associations/belongs_to_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_association.rb @@ -42,6 +42,14 @@ module ActiveRecord @updated end + def stale_target? + if @target && @target.persisted? + @target.send(@reflection.association_primary_key).to_i != @owner.send(@reflection.primary_key_name).to_i + else + false + end + end + private def find_target find_method = if @reflection.options[:primary_key] diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index a0df860623..844ae94c3d 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -23,6 +23,15 @@ module ActiveRecord @updated end + def stale_target? + if @target && @target.persisted? + @target.send(@reflection.association_primary_key).to_i != @owner.send(@reflection.primary_key_name).to_i || + @target.class.base_class.name.to_s != @owner.send(@reflection.options[:foreign_type]).to_s + else + false + end + end + private # NOTE - for now, we're only supporting inverse setting from belongs_to back onto diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 7a7f7812df..87a842bc6b 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -209,7 +209,10 @@ module ActiveRecord end def association_primary_key - @association_primary_key ||= options[:primary_key] || klass.primary_key + @association_primary_key ||= + options[:primary_key] || + !options[:polymorphic] && klass.primary_key || + 'id' end def active_record_primary_key diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index ffcc7a081a..7d9b1104cd 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -298,7 +298,7 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true @ship.delete - + @pirate.reload.update_attributes(:update_only_ship_attributes => { :name => 'Mayflower' }) assert_not_nil @pirate.ship @@ -411,6 +411,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase def test_should_modify_an_existing_record_if_there_is_a_matching_composite_id @pirate.stubs(:id).returns('ABC1X') + @ship.stubs(:pirate_id).returns(@pirate.id) # prevents pirate from being reloaded due to non-matching foreign key @ship.pirate_attributes = { :id => @pirate.id, :catchphrase => 'Arr' } assert_equal 'Arr', @ship.pirate.catchphrase @@ -451,7 +452,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase def test_should_not_destroy_the_associated_model_until_the_parent_is_saved pirate = @ship.pirate - + @ship.attributes = { :pirate_attributes => { :id => pirate.id, '_destroy' => true } } assert_nothing_raised(ActiveRecord::RecordNotFound) { Pirate.find(pirate.id) } @ship.save diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb index 1e205714a7..901c12b26c 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/activerecord/test/cases/reflection_test.rb @@ -7,6 +7,7 @@ require 'models/subscriber' require 'models/ship' require 'models/pirate' require 'models/price_estimate' +require 'models/tagging' class ReflectionTest < ActiveRecord::TestCase include ActiveRecord::Reflection @@ -194,6 +195,7 @@ class ReflectionTest < ActiveRecord::TestCase def test_association_primary_key assert_equal "id", Author.reflect_on_association(:posts).association_primary_key.to_s assert_equal "name", Author.reflect_on_association(:essay).association_primary_key.to_s + assert_equal "id", Tagging.reflect_on_association(:taggable).association_primary_key.to_s end def test_active_record_primary_key -- cgit v1.2.3 From 1c07b84df95e932d50376c1d0a13585b2e2ef868 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 23 Dec 2010 13:36:25 +0000 Subject: If a has_many goes :through a belongs_to, and the foreign key of the belongs_to changes, then the has_many should be considered stale. --- activerecord/lib/active_record/associations.rb | 6 ++- .../associations/belongs_to_association.rb | 5 +- .../belongs_to_polymorphic_association.rb | 8 ++- .../associations/has_many_through_association.rb | 1 + .../associations/has_one_through_association.rb | 1 + .../associations/through_association_scope.rb | 19 +++++++ .../associations/belongs_to_associations_test.rb | 60 +++++++++++++--------- .../has_many_through_associations_test.rb | 16 ++++++ .../has_one_through_associations_test.rb | 15 ++++++ activerecord/test/fixtures/dashboards.yml | 5 +- activerecord/test/fixtures/members.yml | 2 + activerecord/test/fixtures/memberships.yml | 6 +-- activerecord/test/fixtures/speedometers.yml | 6 ++- activerecord/test/fixtures/sponsors.yml | 9 ++-- 14 files changed, 124 insertions(+), 35 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index c6c41a7a12..0aea05b348 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1501,7 +1501,11 @@ module ActiveRecord association_instance_set(reflection.name, association) end - reflection.klass.uncached { association.reload } if force_reload + if force_reload + reflection.klass.uncached { association.reload } + elsif association.stale_target? + association.reload + end association end diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb index c9abfe36e8..bbfe18f9fb 100644 --- a/activerecord/lib/active_record/associations/belongs_to_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_association.rb @@ -44,7 +44,10 @@ module ActiveRecord def stale_target? if @target && @target.persisted? - @target.send(@reflection.association_primary_key).to_i != @owner.send(@reflection.primary_key_name).to_i + target_id = @target.send(@reflection.association_primary_key).to_s + foreign_key = @owner.send(@reflection.primary_key_name).to_s + + target_id != foreign_key else false end diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index 844ae94c3d..c580de7fbe 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -25,8 +25,12 @@ module ActiveRecord def stale_target? if @target && @target.persisted? - @target.send(@reflection.association_primary_key).to_i != @owner.send(@reflection.primary_key_name).to_i || - @target.class.base_class.name.to_s != @owner.send(@reflection.options[:foreign_type]).to_s + target_id = @target.send(@reflection.association_primary_key).to_s + foreign_key = @owner.send(@reflection.primary_key_name).to_s + target_type = @target.class.base_class.name + foreign_type = @owner.send(@reflection.options[:foreign_type]).to_s + + target_id != foreign_key || target_type != foreign_type else false end 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 f0bc6aedf2..5f4667b4d8 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -59,6 +59,7 @@ module ActiveRecord def find_target return [] unless target_reflection_has_associated_record? + update_stale_state scoped.all end 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 e8cf73976b..eb17935d6a 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -33,6 +33,7 @@ module ActiveRecord private def find_target + update_stale_state scoped.first end end diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index c11fce5db0..e57de84f66 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -10,6 +10,17 @@ module ActiveRecord end end + def stale_target? + if @target && @reflection.through_reflection.macro == :belongs_to && defined?(@through_foreign_key) + previous_key = @through_foreign_key.to_s + current_key = @owner.send(@reflection.through_reflection.primary_key_name).to_s + + previous_key != current_key + else + false + end + end + protected def construct_find_scope @@ -165,6 +176,14 @@ module ActiveRecord end alias_method :sql_conditions, :conditions + + def update_stale_state + construct_scope if stale_target? + + if @reflection.through_reflection.macro == :belongs_to + @through_foreign_key = @owner.send(@reflection.through_reflection.primary_key_name) + end + end end end end diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb index 1820f95261..cdde9a80d3 100644 --- a/activerecord/test/cases/associations/belongs_to_associations_test.rb +++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb @@ -17,7 +17,7 @@ require 'models/essay' class BelongsToAssociationsTest < ActiveRecord::TestCase fixtures :accounts, :companies, :developers, :projects, :topics, :developers_projects, :computers, :authors, :author_addresses, - :posts, :tags, :taggings, :comments + :posts, :tags, :taggings, :comments, :sponsors, :members def test_belongs_to Client.find(3).firm.name @@ -488,39 +488,53 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase end def test_reassigning_the_parent_id_updates_the_object - original_parent = Firm.create! :name => "original" - updated_parent = Firm.create! :name => "updated" + client = companies(:second_client) - client = Client.new("client_of" => original_parent.id) - assert_equal original_parent, client.firm - assert_equal original_parent, client.firm_with_condition - assert_equal original_parent, client.firm_with_other_name + client.firm + client.firm_with_condition + firm_proxy = client.send(:association_instance_get, :firm) + firm_with_condition_proxy = client.send(:association_instance_get, :firm_with_condition) - client.client_of = updated_parent.id - assert_equal updated_parent, client.firm - assert_equal updated_parent, client.firm_with_condition - assert_equal updated_parent, client.firm_with_other_name + assert !firm_proxy.stale_target? + assert !firm_with_condition_proxy.stale_target? + assert_equal companies(:first_firm), client.firm + assert_equal companies(:first_firm), client.firm_with_condition + + client.client_of = companies(:another_firm).id + + assert firm_proxy.stale_target? + assert firm_with_condition_proxy.stale_target? + assert_equal companies(:another_firm), client.firm + assert_equal companies(:another_firm), client.firm_with_condition end def test_polymorphic_reassignment_of_associated_id_updates_the_object - member1 = Member.create! - member2 = Member.create! + sponsor = sponsors(:moustache_club_sponsor_for_groucho) + + sponsor.sponsorable + proxy = sponsor.send(:association_instance_get, :sponsorable) + + assert !proxy.stale_target? + assert_equal members(:groucho), sponsor.sponsorable - sponsor = Sponsor.new("sponsorable_type" => "Member", "sponsorable_id" => member1.id) - assert_equal member1, sponsor.sponsorable + sponsor.sponsorable_id = members(:some_other_guy).id - sponsor.sponsorable_id = member2.id - assert_equal member2, sponsor.sponsorable + assert proxy.stale_target? + assert_equal members(:some_other_guy), sponsor.sponsorable end def test_polymorphic_reassignment_of_associated_type_updates_the_object - member1 = Member.create! + sponsor = sponsors(:moustache_club_sponsor_for_groucho) - sponsor = Sponsor.new("sponsorable_type" => "Member", "sponsorable_id" => member1.id) - assert_equal member1, sponsor.sponsorable + sponsor.sponsorable + proxy = sponsor.send(:association_instance_get, :sponsorable) - sponsor.sponsorable_type = "Firm" - assert_not_equal member1, sponsor.sponsorable - end + assert !proxy.stale_target? + assert_equal members(:groucho), sponsor.sponsorable + + sponsor.sponsorable_type = 'Firm' + assert proxy.stale_target? + assert_equal companies(:first_firm), sponsor.sponsorable + end end 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 a417345780..e98d178ff5 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -19,6 +19,7 @@ require 'models/book' require 'models/subscription' require 'models/categorization' require 'models/category' +require 'models/essay' class HasManyThroughAssociationsTest < ActiveRecord::TestCase fixtures :posts, :readers, :people, :comments, :authors, :categories, @@ -534,4 +535,19 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_equal 2, authors(:mary).categories.count assert_equal 1, authors(:mary).categories.general.count end + + def test_has_many_through_belongs_to_should_update_when_the_through_foreign_key_changes + post = posts(:eager_other) + + post.author_categorizations + proxy = post.send(:association_instance_get, :author_categorizations) + + assert !proxy.stale_target? + assert_equal authors(:mary).categorizations.sort_by(&:id), post.author_categorizations.sort_by(&:id) + + post.author_id = authors(:david).id + + assert proxy.stale_target? + assert_equal authors(:david).categorizations.sort_by(&:id), post.author_categorizations.sort_by(&:id) + end end diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb index f6365e0216..7d55d909a7 100644 --- a/activerecord/test/cases/associations/has_one_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb @@ -251,4 +251,19 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase members(:groucho).club_through_many end end + + def test_has_one_through_belongs_to_should_update_when_the_through_foreign_key_changes + minivan = minivans(:cool_first) + + minivan.dashboard + proxy = minivan.send(:association_instance_get, :dashboard) + + assert !proxy.stale_target? + assert_equal dashboards(:cool_first), minivan.dashboard + + minivan.speedometer_id = speedometers(:second).id + + assert proxy.stale_target? + assert_equal dashboards(:second), minivan.dashboard + end end diff --git a/activerecord/test/fixtures/dashboards.yml b/activerecord/test/fixtures/dashboards.yml index e75bf46e6c..a4c7e0d309 100644 --- a/activerecord/test/fixtures/dashboards.yml +++ b/activerecord/test/fixtures/dashboards.yml @@ -1,3 +1,6 @@ cool_first: dashboard_id: d1 - name: my_dashboard \ No newline at end of file + name: my_dashboard +second: + dashboard_id: d2 + name: second diff --git a/activerecord/test/fixtures/members.yml b/activerecord/test/fixtures/members.yml index 6db945e61d..824840b7e5 100644 --- a/activerecord/test/fixtures/members.yml +++ b/activerecord/test/fixtures/members.yml @@ -1,6 +1,8 @@ groucho: + id: 1 name: Groucho Marx member_type_id: 1 some_other_guy: + id: 2 name: Englebert Humperdink member_type_id: 2 diff --git a/activerecord/test/fixtures/memberships.yml b/activerecord/test/fixtures/memberships.yml index b9722dbc8a..eed8b22af8 100644 --- a/activerecord/test/fixtures/memberships.yml +++ b/activerecord/test/fixtures/memberships.yml @@ -1,20 +1,20 @@ membership_of_boring_club: joined_on: <%= 3.weeks.ago.to_s(:db) %> club: boring_club - member: groucho + member_id: 1 favourite: false type: CurrentMembership membership_of_favourite_club: joined_on: <%= 3.weeks.ago.to_s(:db) %> club: moustache_club - member: groucho + member_id: 1 favourite: true type: Membership other_guys_membership: joined_on: <%= 4.weeks.ago.to_s(:db) %> club: boring_club - member: some_other_guy + member_id: 2 favourite: false type: CurrentMembership diff --git a/activerecord/test/fixtures/speedometers.yml b/activerecord/test/fixtures/speedometers.yml index 6a471aba0a..e12398f0c4 100644 --- a/activerecord/test/fixtures/speedometers.yml +++ b/activerecord/test/fixtures/speedometers.yml @@ -1,4 +1,8 @@ cool_first: speedometer_id: s1 name: my_speedometer - dashboard_id: d1 \ No newline at end of file + dashboard_id: d1 +second: + speedometer_id: s2 + name: second + dashboard_id: d2 diff --git a/activerecord/test/fixtures/sponsors.yml b/activerecord/test/fixtures/sponsors.yml index 42df8957d1..bfc6b238b1 100644 --- a/activerecord/test/fixtures/sponsors.yml +++ b/activerecord/test/fixtures/sponsors.yml @@ -1,9 +1,12 @@ moustache_club_sponsor_for_groucho: sponsor_club: moustache_club - sponsorable: groucho (Member) + sponsorable_id: 1 + sponsorable_type: Member boring_club_sponsor_for_groucho: sponsor_club: boring_club - sponsorable: some_other_guy (Member) + sponsorable_id: 2 + sponsorable_type: Member crazy_club_sponsor_for_groucho: sponsor_club: crazy_club - sponsorable: some_other_guy (Member) \ No newline at end of file + sponsorable_id: 2 + sponsorable_type: Member -- cgit v1.2.3 From fb3a8c51b4028e8d122fdbb783d73d0ed37ca168 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 23 Dec 2010 14:19:08 +0000 Subject: Raise an error for associations which try to go :through a polymorphic association [#6212 state:resolved] --- activerecord/lib/active_record/associations.rb | 8 +++++++- activerecord/lib/active_record/reflection.rb | 6 +++++- activerecord/test/cases/associations/join_model_test.rb | 11 ++++++++--- activerecord/test/models/tagging.rb | 3 ++- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 0aea05b348..1056c51a3d 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -20,12 +20,18 @@ module ActiveRecord end end - class HasManyThroughAssociationPolymorphicError < ActiveRecordError #:nodoc: + class HasManyThroughAssociationPolymorphicSourceError < ActiveRecordError #:nodoc: def initialize(owner_class_name, reflection, source_reflection) super("Cannot have a has_many :through association '#{owner_class_name}##{reflection.name}' on the polymorphic object '#{source_reflection.class_name}##{source_reflection.name}'.") end end + class HasManyThroughAssociationPolymorphicThroughError < ActiveRecordError #:nodoc: + def initialize(owner_class_name, reflection) + super("Cannot have a has_many :through association '#{owner_class_name}##{reflection.name}' which goes through the polymorphic association '#{owner_class_name}##{reflection.through_reflection.name}'.") + end + end + class HasManyThroughAssociationPointlessSourceTypeError < ActiveRecordError #:nodoc: def initialize(owner_class_name, reflection, source_reflection) super("Cannot have a has_many :through association '#{owner_class_name}##{reflection.name}' with a :source_type option if the '#{reflection.through_reflection.class_name}##{source_reflection.name}' is not polymorphic. Try removing :source_type on your association.") diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 87a842bc6b..0310e7050d 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -375,6 +375,10 @@ module ActiveRecord raise HasManyThroughAssociationNotFoundError.new(active_record.name, self) end + if through_reflection.options[:polymorphic] + raise HasManyThroughAssociationPolymorphicThroughError.new(active_record.name, self) + end + if source_reflection.nil? raise HasManyThroughSourceAssociationNotFoundError.new(self) end @@ -384,7 +388,7 @@ module ActiveRecord end if source_reflection.options[:polymorphic] && options[:source_type].nil? - raise HasManyThroughAssociationPolymorphicError.new(active_record.name, self, source_reflection) + raise HasManyThroughAssociationPolymorphicSourceError.new(active_record.name, self, source_reflection) end unless [:belongs_to, :has_many, :has_one].include?(source_reflection.macro) && source_reflection.options[:through].nil? diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb index 58542bc939..263c90097f 100644 --- a/activerecord/test/cases/associations/join_model_test.rb +++ b/activerecord/test/cases/associations/join_model_test.rb @@ -340,11 +340,16 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase end def test_has_many_polymorphic - assert_raise ActiveRecord::HasManyThroughAssociationPolymorphicError do - assert_equal posts(:welcome, :thinking), tags(:general).taggables + assert_raise ActiveRecord::HasManyThroughAssociationPolymorphicSourceError do + tags(:general).taggables end + + assert_raise ActiveRecord::HasManyThroughAssociationPolymorphicThroughError do + taggings(:welcome_general).things + end + assert_raise ActiveRecord::EagerLoadPolymorphicError do - assert_equal posts(:welcome, :thinking), tags(:general).taggings.find(:all, :include => :taggable, :conditions => 'bogus_table.column = 1') + tags(:general).taggings.find(:all, :include => :taggable, :conditions => 'bogus_table.column = 1') end end diff --git a/activerecord/test/models/tagging.rb b/activerecord/test/models/tagging.rb index a1fa1a9750..33ffc623d7 100644 --- a/activerecord/test/models/tagging.rb +++ b/activerecord/test/models/tagging.rb @@ -7,4 +7,5 @@ class Tagging < ActiveRecord::Base belongs_to :super_tag, :class_name => 'Tag', :foreign_key => 'super_tag_id' belongs_to :invalid_tag, :class_name => 'Tag', :foreign_key => 'tag_id' belongs_to :taggable, :polymorphic => true, :counter_cache => true -end \ No newline at end of file + has_many :things, :through => :taggable +end -- cgit v1.2.3 From 6974c595fd480dc0ae3311ef60920fa87c5ff9d0 Mon Sep 17 00:00:00 2001 From: oleg dashevskii Date: Thu, 26 Aug 2010 11:22:27 +0700 Subject: Verify that there is no unwanted implicit readonly set on Model.has_many_through.find(id) [#5442 state:resolved] --- activerecord/test/cases/readonly_test.rb | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/activerecord/test/cases/readonly_test.rb b/activerecord/test/cases/readonly_test.rb index 98011f40a4..a1eb96ef09 100644 --- a/activerecord/test/cases/readonly_test.rb +++ b/activerecord/test/cases/readonly_test.rb @@ -12,7 +12,7 @@ def Project.foo() find :first end class ReadOnlyTest < ActiveRecord::TestCase - fixtures :posts, :comments, :developers, :projects, :developers_projects + fixtures :posts, :comments, :developers, :projects, :developers_projects, :people, :readers def test_cant_save_readonly_record dev = Developer.find(1) @@ -71,6 +71,18 @@ class ReadOnlyTest < ActiveRecord::TestCase assert !people.any?(&:readonly?) end + def test_has_many_with_through_is_not_implicitly_marked_readonly_while_finding_by_id + assert !posts(:welcome).people.find(1).readonly? + end + + def test_has_many_with_through_is_not_implicitly_marked_readonly_while_finding_first + assert !posts(:welcome).people.first.readonly? + end + + def test_has_many_with_through_is_not_implicitly_marked_readonly_while_finding_last + assert !posts(:welcome).people.last.readonly? + end + def test_readonly_scoping Post.send(:with_scope, :find => { :conditions => '1=1' }) do assert !Post.find(1).readonly? -- cgit v1.2.3