From 8233f8314ba00838e8ef6b2d95cdfb7d58c8dece Mon Sep 17 00:00:00 2001 From: Will Bryant Date: Wed, 24 Sep 2008 16:44:56 +1200 Subject: wrote a test showing eager loading's misbehavior (sanitizing against the wrong table) when the association has a :conditions hash Signed-off-by: Michael Koziarski --- activerecord/test/cases/associations/eager_test.rb | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'activerecord') diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index e78624a98d..3dd2cb028a 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -116,6 +116,13 @@ class EagerAssociationTest < ActiveRecord::TestCase assert_equal 2, posts.first.comments.size end + def test_loading_from_an_association_that_has_a_hash_of_conditions + assert_nothing_raised do + Author.find(:all, :include => :hello_posts_with_hash_conditions) + end + assert !Author.find(authors(:david).id, :include => :hello_posts_with_hash_conditions).hello_posts.empty? + end + def test_loading_with_no_associations assert_nil Post.find(posts(:authorless).id, :include => :author).author end -- cgit v1.2.3 From 35d731ef0aaf1b6103121bb2c2d21f5b17c26b3c Mon Sep 17 00:00:00 2001 From: Will Bryant Date: Wed, 24 Sep 2008 17:00:35 +1200 Subject: fix eager loading's :condition sanitizing expanding against the wrong table Signed-off-by: Michael Koziarski --- activerecord/lib/active_record/association_preload.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index c60850fc77..284dc7dca0 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -96,7 +96,7 @@ module ActiveRecord options = reflection.options conditions = "t0.#{reflection.primary_key_name} #{in_or_equals_for_ids(ids)}" - conditions << append_conditions(options, preload_options) + conditions << append_conditions(reflection, preload_options) associated_records = reflection.klass.find(:all, :conditions => [conditions, ids], :include => options[:include], @@ -233,7 +233,7 @@ module ActiveRecord end end conditions = "#{table_name}.#{connection.quote_column_name(primary_key)} #{in_or_equals_for_ids(ids)}" - conditions << append_conditions(options, preload_options) + conditions << append_conditions(reflection, preload_options) associated_records = klass.find(:all, :conditions => [conditions, ids], :include => options[:include], :select => options[:select], @@ -254,7 +254,7 @@ module ActiveRecord conditions = "#{reflection.klass.quoted_table_name}.#{foreign_key} #{in_or_equals_for_ids(ids)}" end - conditions << append_conditions(options, preload_options) + conditions << append_conditions(reflection, preload_options) reflection.klass.find(:all, :select => (preload_options[:select] || options[:select] || "#{table_name}.*"), @@ -270,9 +270,9 @@ module ActiveRecord instance_eval("%@#{sql.gsub('@', '\@')}@") end - def append_conditions(options, preload_options) + def append_conditions(reflection, preload_options) sql = "" - sql << " AND (#{interpolate_sql_for_preload(sanitize_sql(options[:conditions]))})" if options[:conditions] + sql << " AND (#{interpolate_sql_for_preload(reflection.sanitized_conditions)})" if reflection.sanitized_conditions sql << " AND (#{sanitize_sql preload_options[:conditions]})" if preload_options[:conditions] sql end -- cgit v1.2.3 From 8d337e9ec2e25007d557150dbe7557ab3c3bd05f Mon Sep 17 00:00:00 2001 From: Eloy Duran Date: Fri, 3 Oct 2008 13:55:35 +0200 Subject: Dynamic finders should use the ActiveRecord::Base::find method instead of ::find_initial, :find_last, and ::find_all. This is so when people override ActiveRecord::Base::find, the new ::find method will also be invoked by the dynamic finders. Associations for instance do go through ::find, so this makes it more consistent. Also removed the unnecessary deprecation silence blocks. Signed-off-by: Michael Koziarski [#1162 state:committed] --- activerecord/lib/active_record/base.rb | 8 +++---- .../lib/active_record/dynamic_finder_match.rb | 8 +++---- activerecord/test/cases/finder_test.rb | 27 ++++++++++++++++++---- 3 files changed, 30 insertions(+), 13 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 69ea155ace..3e8c573d09 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1724,10 +1724,10 @@ module ActiveRecord #:nodoc: #{'result = ' if bang}if options[:conditions] with_scope(:find => finder_options) do - ActiveSupport::Deprecation.silence { send(:#{finder}, options) } + find(:#{finder}, options) end else - ActiveSupport::Deprecation.silence { send(:#{finder}, options.merge(finder_options)) } + find(:#{finder}, options.merge(finder_options)) end #{'result || raise(RecordNotFound)' if bang} end @@ -1750,9 +1750,9 @@ module ActiveRecord #:nodoc: options = { :conditions => find_attributes } set_readonly_option!(options) - record = find_initial(options) + record = find(:first, options) - if record.nil? + if record.nil? record = self.new { |r| r.send(:attributes=, attributes, guard_protected_attributes) } #{'yield(record) if block_given?'} #{'record.save' if instantiator == :create} diff --git a/activerecord/lib/active_record/dynamic_finder_match.rb b/activerecord/lib/active_record/dynamic_finder_match.rb index f4a5712981..8f9f05ce36 100644 --- a/activerecord/lib/active_record/dynamic_finder_match.rb +++ b/activerecord/lib/active_record/dynamic_finder_match.rb @@ -6,11 +6,11 @@ module ActiveRecord end def initialize(method) - @finder = :find_initial + @finder = :first case method.to_s when /^find_(all_by|last_by|by)_([_a-zA-Z]\w*)$/ - @finder = :find_last if $1 == 'last_by' - @finder = :find_every if $1 == 'all_by' + @finder = :last if $1 == 'last_by' + @finder = :all if $1 == 'all_by' names = $2 when /^find_by_([_a-zA-Z]\w*)\!$/ @bang = true @@ -31,7 +31,7 @@ module ActiveRecord end def instantiator? - @finder == :find_initial && !@instantiator.nil? + @finder == :first && !@instantiator.nil? end def bang? diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 292b88edbc..853474916c 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -21,7 +21,7 @@ class DynamicFinderMatchTest < ActiveRecord::TestCase match = ActiveRecord::DynamicFinderMatch.match("find_by_age_and_sex_and_location") assert_not_nil match assert match.finder? - assert_equal :find_initial, match.finder + assert_equal :first, match.finder assert_equal %w(age sex location), match.attribute_names end @@ -30,7 +30,7 @@ class DynamicFinderMatchTest < ActiveRecord::TestCase assert_not_nil match assert match.finder? assert match.bang? - assert_equal :find_initial, match.finder + assert_equal :first, match.finder assert_equal %w(age sex location), match.attribute_names end @@ -38,7 +38,7 @@ class DynamicFinderMatchTest < ActiveRecord::TestCase match = ActiveRecord::DynamicFinderMatch.match("find_all_by_age_and_sex_and_location") assert_not_nil match assert match.finder? - assert_equal :find_every, match.finder + assert_equal :all, match.finder assert_equal %w(age sex location), match.attribute_names end @@ -47,7 +47,7 @@ class DynamicFinderMatchTest < ActiveRecord::TestCase assert_not_nil match assert !match.finder? assert match.instantiator? - assert_equal :find_initial, match.finder + assert_equal :first, match.finder assert_equal :new, match.instantiator assert_equal %w(age sex location), match.attribute_names end @@ -57,7 +57,7 @@ class DynamicFinderMatchTest < ActiveRecord::TestCase assert_not_nil match assert !match.finder? assert match.instantiator? - assert_equal :find_initial, match.finder + assert_equal :first, match.finder assert_equal :create, match.instantiator assert_equal %w(age sex location), match.attribute_names end @@ -500,6 +500,23 @@ class FinderTest < ActiveRecord::TestCase assert_equal(2, Entrant.count_by_sql(["SELECT COUNT(*) FROM entrants WHERE id > ?", 1])) end + uses_mocha('test_dynamic_finder_should_go_through_the_find_class_method') do + def test_dynamic_finders_should_go_through_the_find_class_method + Topic.expects(:find).with(:first, :conditions => { :title => 'The First Topic!' }) + Topic.find_by_title("The First Topic!") + + Topic.expects(:find).with(:last, :conditions => { :title => 'The Last Topic!' }) + Topic.find_last_by_title("The Last Topic!") + + Topic.expects(:find).with(:all, :conditions => { :title => 'A Topic.' }) + Topic.find_all_by_title("A Topic.") + + Topic.expects(:find).with(:first, :conditions => { :title => 'Does not exist yet for sure!' }).times(2) + Topic.find_or_initialize_by_title('Does not exist yet for sure!') + Topic.find_or_create_by_title('Does not exist yet for sure!') + end + end + def test_find_by_one_attribute assert_equal topics(:first), Topic.find_by_title("The First Topic") assert_nil Topic.find_by_title("The First Topic!") -- cgit v1.2.3 From 7553a23c0a84424bdbc09cc81791f41bfebe1b25 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Fri, 3 Oct 2008 21:35:01 +0200 Subject: Remove AS for oracle compatibility --- activerecord/lib/active_record/calculations.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb index 80992dd34b..634236e959 100644 --- a/activerecord/lib/active_record/calculations.rb +++ b/activerecord/lib/active_record/calculations.rb @@ -217,7 +217,7 @@ module ActiveRecord sql << " ORDER BY #{options[:order]} " if options[:order] add_limit!(sql, options, scope) - sql << ") AS #{aggregate_alias}_subquery" if use_workaround + sql << ") #{aggregate_alias}_subquery" if use_workaround sql end -- cgit v1.2.3 From 1bc267d21679408b3624d1c697656ec250c01972 Mon Sep 17 00:00:00 2001 From: Luca Guidi Date: Fri, 3 Oct 2008 16:08:17 +0200 Subject: Make sure recreate MySQL test database with the proper encoding and collation [#1165 state:resolved] Signed-off-by: Michael Koziarski [#1165 state:committed] --- activerecord/lib/active_record/connection_adapters/mysql_adapter.rb | 4 ++-- activerecord/test/cases/active_schema_test_mysql.rb | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index a26fd02b90..3aa27bfc99 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -371,9 +371,9 @@ module ActiveRecord end end - def recreate_database(name) #:nodoc: + def recreate_database(name, options = {}) #:nodoc: drop_database(name) - create_database(name) + create_database(name, options) end # Create a new MySQL database with optional :charset and :collation. diff --git a/activerecord/test/cases/active_schema_test_mysql.rb b/activerecord/test/cases/active_schema_test_mysql.rb index 2a42dc3517..9aff538ce9 100644 --- a/activerecord/test/cases/active_schema_test_mysql.rb +++ b/activerecord/test/cases/active_schema_test_mysql.rb @@ -25,6 +25,11 @@ class ActiveSchemaTest < ActiveRecord::TestCase assert_equal "CREATE DATABASE `aimonetti` DEFAULT CHARACTER SET `latin1`", create_database(:aimonetti, {:charset => 'latin1'}) assert_equal "CREATE DATABASE `matt_aimonetti` DEFAULT CHARACTER SET `big5` COLLATE `big5_chinese_ci`", create_database(:matt_aimonetti, {:charset => :big5, :collation => :big5_chinese_ci}) end + + def test_recreate_mysql_database_with_encoding + create_database(:luca, {:charset => 'latin1'}) + assert_equal "CREATE DATABASE `luca` DEFAULT CHARACTER SET `latin1`", recreate_database(:luca, {:charset => 'latin1'}) + end end def test_add_column -- cgit v1.2.3 From f2dee4915306e6bd1554e47640d201600ac39c41 Mon Sep 17 00:00:00 2001 From: Chris Kampmeier Date: Sat, 4 Oct 2008 00:16:29 -0700 Subject: Fix typography and punctuation in ActiveRecord#find_by_sql --- activerecord/lib/active_record/base.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index ac15eed408..6fb05b5551 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -610,8 +610,8 @@ module ActiveRecord #:nodoc: # Executes a custom SQL query against your database and returns all the results. The results will # be returned as an array with columns requested encapsulated as attributes of the model you call - # this method from. If you call +Product.find_by_sql+ then the results will be returned in a Product - # object with the attributes you specified in the SQL query. + # this method from. If you call Product.find_by_sql then the results will be returned in + # a Product object with the attributes you specified in the SQL query. # # If you call a complicated SQL query which spans multiple tables the columns specified by the # SELECT will be attributes of the model, whether or not they are columns of the corresponding @@ -620,7 +620,7 @@ module ActiveRecord #:nodoc: # The +sql+ parameter is a full SQL query as a string. It will be called as is, there will be # no database agnostic conversions performed. This should be a last resort because using, for example, # MySQL specific terms will lock you to using that particular database engine or require you to - # change your call if you switch engines + # change your call if you switch engines. # # ==== Examples # # A simple SQL query spanning multiple tables -- cgit v1.2.3 From 2fc835d646ec4c420639a2b5e8edd5fb204c0414 Mon Sep 17 00:00:00 2001 From: Mike Gunderloy Date: Sat, 4 Oct 2008 08:30:20 -0500 Subject: Remove erroneous warning about counter_cache column defaults. --- activerecord/lib/active_record/associations.rb | 2 -- 1 file changed, 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 4d36216c34..d1a0b2f96a 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -955,8 +955,6 @@ module ActiveRecord # destroyed. This requires that a column named #{table_name}_count (such as +comments_count+ for a belonging Comment class) # is used on the associate class (such as a Post class). You can also specify a custom counter cache column by providing # a column name instead of a +true+/+false+ value to this option (e.g., :counter_cache => :my_custom_counter.) - # When creating a counter cache column, the database statement or migration must specify a default value of 0, failing to do - # this results in a counter with +NULL+ value, which will never increment. # Note: Specifying a counter cache will add it to that model's list of readonly attributes using +attr_readonly+. # [:include] # Specify second-order associations that should be eager loaded when this object is loaded. -- cgit v1.2.3 From 4cb3d27443fa30fde528f07658e76537f38a0661 Mon Sep 17 00:00:00 2001 From: Aliaksey Kandratsenka Date: Sat, 6 Sep 2008 11:22:01 +0300 Subject: don't quote decimal values for mysql. It doesn't make sense and breaks in newer versions of mysql Signed-off-by: Michael Koziarski [#1168 state:committed] --- activerecord/lib/active_record/connection_adapters/mysql_adapter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 3aa27bfc99..1e452ae88a 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -218,7 +218,7 @@ module ActiveRecord s = column.class.string_to_binary(value).unpack("H*")[0] "x'#{s}'" elsif value.kind_of?(BigDecimal) - "'#{value.to_s("F")}'" + value.to_s("F") else super end -- cgit v1.2.3 From 21eb18a70c7a1f08e7e2dc1c5bc17d67e1d14c46 Mon Sep 17 00:00:00 2001 From: Aliaksey Kandratsenka Date: Thu, 2 Oct 2008 21:44:49 +0300 Subject: Fix race in ConnectionPool#checkout After releasing monitor some connection(s) may appear in pool before monitor is re-aquired. When this happens we'll wait for connection which is already available. Signed-off-by: Michael Koziarski --- .../abstract/connection_pool.rb | 29 +++++++++++----------- 1 file changed, 14 insertions(+), 15 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 54a39db1eb..74381437ca 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -131,21 +131,20 @@ module ActiveRecord # Check-out a database connection from the pool. def checkout # Checkout an available connection - conn = @connection_mutex.synchronize do - if @checked_out.size < @connections.size - checkout_existing_connection - elsif @connections.size < @size - checkout_new_connection - end - end - return conn if conn - - # No connections available; wait for one @connection_mutex.synchronize do - if @queue.wait(@timeout) - checkout_existing_connection - else - raise ConnectionTimeoutError, "could not obtain a database connection within #{@timeout} seconds. The pool size is currently #{@size}, perhaps you need to increase it?" + loop do + conn = if @checked_out.size < @connections.size + checkout_existing_connection + elsif @connections.size < @size + checkout_new_connection + end + return conn if conn + # No connections available; wait for one + if @queue.wait(@timeout) + next + else + raise ConnectionTimeoutError, "could not obtain a database connection within #{@timeout} seconds. The pool size is currently #{@size}, perhaps you need to increase it?" + end end end end @@ -275,4 +274,4 @@ module ActiveRecord end end end -end \ No newline at end of file +end -- cgit v1.2.3 From 834361145a805b5077f962dc2e67a0a9ea882535 Mon Sep 17 00:00:00 2001 From: Aliaksey Kandratsenka Date: Thu, 2 Oct 2008 22:59:49 +0300 Subject: made ConnectionPool#checkout more robust by trying to loot dead threads when pool is empty Signed-off-by: Michael Koziarski [#1169 state:committed] --- .../active_record/connection_adapters/abstract/connection_pool.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 74381437ca..b7d7384548 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -143,7 +143,11 @@ module ActiveRecord if @queue.wait(@timeout) next else - raise ConnectionTimeoutError, "could not obtain a database connection within #{@timeout} seconds. The pool size is currently #{@size}, perhaps you need to increase it?" + # try looting dead threads + clear_stale_cached_connections! + if @size == @checked_out.size + raise ConnectionTimeoutError, "could not obtain a database connection within #{@timeout} seconds. The pool size is currently #{@size}, perhaps you need to increase it?" + end end end end -- cgit v1.2.3 From f550c8625739106e957e4260d3d592a9665fc943 Mon Sep 17 00:00:00 2001 From: Aliaksey Kandratsenka Date: Fri, 3 Oct 2008 13:40:51 +0300 Subject: Fix performance bug in AttibuteMethods#respond_to? in handling of private methods We have hit dramatic increase in tests time after upgrading rails. Profiling revealed this particular place. After this fix our test times returned back to norm. Signed-off-by: Michael Koziarski [#1173 state:committed] --- activerecord/lib/active_record/attribute_methods.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index e5486738f0..1c753524de 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -342,7 +342,9 @@ module ActiveRecord method_name = method.to_s if super return true - elsif self.private_methods.include?(method_name) && !include_private_methods + elsif !include_private_methods && super(method, true) + # If we're here than we haven't found among non-private methods + # but found among all methods. Which means that given method is private. return false elsif !self.class.generated_methods? self.class.define_attribute_methods -- cgit v1.2.3 From 7659fb6a2b638703a99a63033d947d19089a6b85 Mon Sep 17 00:00:00 2001 From: Lawrence Pit Date: Sat, 4 Oct 2008 15:31:04 +0100 Subject: Try reloading model on class mismatch [#229 state:resolved] Signed-off-by: Pratik Naik --- .../active_record/associations/association_proxy.rb | 2 +- activerecord/test/cases/reload_models_test.rb | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 activerecord/test/cases/reload_models_test.rb (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index acdcd14ec8..b617147f67 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -240,7 +240,7 @@ module ActiveRecord # the kind of the class of the associated objects. Meant to be used as # a sanity check when you are about to assign an associated record. def raise_on_type_mismatch(record) - unless record.is_a?(@reflection.klass) + unless record.is_a?(@reflection.klass) || record.is_a?(@reflection.class_name.constantize) message = "#{@reflection.class_name}(##{@reflection.klass.object_id}) expected, got #{record.class}(##{record.class.object_id})" raise ActiveRecord::AssociationTypeMismatch, message end diff --git a/activerecord/test/cases/reload_models_test.rb b/activerecord/test/cases/reload_models_test.rb new file mode 100644 index 0000000000..411b5f6afa --- /dev/null +++ b/activerecord/test/cases/reload_models_test.rb @@ -0,0 +1,20 @@ +require "cases/helper" +require 'models/owner' +require 'models/pet' + +class ReloadModelsTest < ActiveRecord::TestCase + def test_has_one_with_reload + pet = Pet.find_by_name('parrot') + pet.owner = Owner.find_by_name('ashley') + + # Reload the class Owner, simulating auto-reloading of model classes in a + # development environment. Note that meanwhile the class Pet is not + # reloaded, simulating a class that is present in a plugin. + Object.class_eval { remove_const :Owner } + Kernel.load(File.expand_path(File.join(File.dirname(__FILE__), "../models/owner.rb"))) + + pet = Pet.find_by_name('parrot') + pet.owner = Owner.find_by_name('ashley') + assert_equal pet.owner, Owner.find_by_name('ashley') + end +end \ No newline at end of file -- cgit v1.2.3 From 95e1cf4812d4b964d7ab0fdf4bfa31177d27909c Mon Sep 17 00:00:00 2001 From: Zach Dennis Date: Sat, 4 Oct 2008 15:42:36 +0100 Subject: Fix has_many :through when the source is a belongs_to association. [#323 state:resolved] Signed-off-by: Pratik Naik --- .../lib/active_record/association_preload.rb | 44 +++++++++++++++++----- .../associations/has_many_through_association.rb | 11 ++++++ activerecord/test/cases/associations/eager_test.rb | 9 +++++ .../associations/has_many_associations_test.rb | 1 + .../has_many_through_associations_test.rb | 17 ++++++++- activerecord/test/models/post.rb | 2 + 6 files changed, 73 insertions(+), 11 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 284dc7dca0..0f96ee95d5 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -4,6 +4,28 @@ module ActiveRecord base.extend(ClassMethods) end + class HasManyAssociationStrategy + def initialize(through_reflection) + @through_reflection = through_reflection + end + + def primary_key + if @through_reflection && @through_reflection.macro == :belongs_to + @through_reflection.klass.primary_key + else + @through_reflection.primary_key_name + end + end + + def primary_key_name + if @through_reflection && @through_reflection.macro == :belongs_to + @through_reflection.primary_key_name + else + nil + end + end + end + module ClassMethods # Loads the named associations for the activerecord record (or records) given @@ -77,12 +99,13 @@ module ActiveRecord end end - def construct_id_map(records) + def construct_id_map(records, primary_key=nil) id_to_record_map = {} ids = [] records.each do |record| - ids << record.id - mapped_records = (id_to_record_map[record.id.to_s] ||= []) + primary_key ||= record.class.primary_key + ids << record[primary_key] + mapped_records = (id_to_record_map[ids.last.to_s] ||= []) mapped_records << record end ids.uniq! @@ -129,23 +152,24 @@ module ActiveRecord end def preload_has_many_association(records, reflection, preload_options={}) - id_to_record_map, ids = construct_id_map(records) - records.each {|record| record.send(reflection.name).loaded} options = reflection.options + through_reflection = reflections[options[:through]] + strat = HasManyAssociationStrategy.new(through_reflection) + id_to_record_map, ids = construct_id_map(records, strat.primary_key_name) + records.each {|record| record.send(reflection.name).loaded} if options[:through] through_records = preload_through_records(records, reflection, options[:through]) through_reflection = reflections[options[:through]] - through_primary_key = through_reflection.primary_key_name unless through_records.empty? source = reflection.source_reflection.name - #add conditions from reflection! - through_records.first.class.preload_associations(through_records, source, reflection.options) + through_records.first.class.preload_associations(through_records, source, options) through_records.each do |through_record| - add_preloaded_records_to_collection(id_to_record_map[through_record[through_primary_key].to_s], - reflection.name, through_record.send(source)) + through_record_id = through_record[strat.primary_key].to_s + add_preloaded_records_to_collection(id_to_record_map[through_record_id], reflection.name, through_record.send(source)) end end + else set_association_collection_records(id_to_record_map, reflection.name, find_associated_records(ids, reflection, preload_options), reflection.primary_key_name) 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 3171665e19..a0bb3a45b0 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -32,6 +32,14 @@ module ActiveRecord end protected + def target_reflection_has_associated_record? + if @reflection.through_reflection.macro == :belongs_to && @owner[@reflection.through_reflection.primary_key_name].blank? + false + else + true + end + end + def construct_find_options!(options) options[:select] = construct_select(options[:select]) options[:from] ||= construct_from @@ -61,6 +69,7 @@ module ActiveRecord end def find_target + return [] unless target_reflection_has_associated_record? @reflection.klass.find(:all, :select => construct_select, :conditions => construct_conditions, @@ -102,6 +111,8 @@ module ActiveRecord "#{as}_type" => reflection.klass.quote_value( @owner.class.base_class.name.to_s, reflection.klass.columns_hash["#{as}_type"]) } + elsif reflection.macro == :belongs_to + { reflection.klass.primary_key => @owner[reflection.primary_key_name] } else { reflection.primary_key_name => owner_quoted_id } end diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 3dd2cb028a..7f42577ab0 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -275,6 +275,15 @@ class EagerAssociationTest < ActiveRecord::TestCase assert_equal authors(:david), assert_no_queries { posts_with_comments_and_author.first.author } end + def test_eager_with_has_many_through_a_belongs_to_association + author = authors(:mary) + post = Post.create!(:author => author, :title => "TITLE", :body => "BODY") + author.author_favorites.create(:favorite_author_id => 1) + author.author_favorites.create(:favorite_author_id => 2) + posts_with_author_favorites = author.posts.find(:all, :include => :author_favorites) + assert_no_queries { posts_with_author_favorites.first.author_favorites.first.author_id } + end + def test_eager_with_has_many_through_an_sti_join_model author = Author.find(:first, :include => :special_post_comments, :order => 'authors.id') assert_equal [comments(:does_it_hurt)], assert_no_queries { author.special_post_comments } diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 315d77de07..1bc9c39c19 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1081,3 +1081,4 @@ class HasManyAssociationsTest < ActiveRecord::TestCase 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 12cce98c26..a07f4bcbdd 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -5,7 +5,7 @@ require 'models/reader' require 'models/comment' class HasManyThroughAssociationsTest < ActiveRecord::TestCase - fixtures :posts, :readers, :people, :comments + fixtures :posts, :readers, :people, :comments, :authors def test_associate_existing assert_queries(2) { posts(:thinking);people(:david) } @@ -229,4 +229,19 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end end end + + def test_has_many_association_through_a_belongs_to_association_where_the_association_doesnt_exist + author = authors(:mary) + post = Post.create!(:title => "TITLE", :body => "BODY") + assert_equal [], post.author_favorites + end + + def test_has_many_association_through_a_belongs_to_association + author = authors(:mary) + post = Post.create!(:author => author, :title => "TITLE", :body => "BODY") + author.author_favorites.create(:favorite_author_id => 1) + author.author_favorites.create(:favorite_author_id => 2) + author.author_favorites.create(:favorite_author_id => 3) + assert_equal post.author.author_favorites, post.author_favorites + end end diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 3adbc0ce1f..6da37c31ff 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -22,6 +22,8 @@ class Post < ActiveRecord::Base end end + has_many :author_favorites, :through => :author + has_many :comments_with_interpolated_conditions, :class_name => 'Comment', :conditions => ['#{"#{aliased_table_name}." rescue ""}body = ?', 'Thank you for the welcome'] -- cgit v1.2.3 From 25ca21ae21d49f06708357a5ce0670103ced2d58 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Sat, 4 Oct 2008 16:53:13 +0100 Subject: Introduce ActiveRecord::Reflection::ThroughReflection to simplify hm:t reflection logic --- activerecord/lib/active_record/reflection.rb | 119 +++++++++++++++------------ activerecord/test/cases/reflection_test.rb | 4 + 2 files changed, 70 insertions(+), 53 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index a1b498eceb..77d03493dc 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -13,14 +13,15 @@ module ActiveRecord def create_reflection(macro, name, options, active_record) case macro when :has_many, :belongs_to, :has_one, :has_and_belongs_to_many - reflection = AssociationReflection.new(macro, name, options, active_record) + klass = options[:through] ? ThroughReflection : AssociationReflection + reflection = klass.new(macro, name, options, active_record) when :composed_of reflection = AggregateReflection.new(macro, name, options, active_record) end write_inheritable_hash :reflections, name => reflection reflection end - + # Returns a hash containing all AssociationReflection objects for the current class # Example: # @@ -30,7 +31,7 @@ module ActiveRecord def reflections read_inheritable_attribute(:reflections) || write_inheritable_attribute(:reflections, {}) end - + # Returns an array of AggregateReflection objects for all the aggregations in the class. def reflect_on_all_aggregations reflections.values.select { |reflection| reflection.is_a?(AggregateReflection) } @@ -192,6 +193,49 @@ module ActiveRecord end end + def check_validity! + end + + def through_reflection + false + end + + def source_reflection + nil + end + + private + def derive_class_name + class_name = name.to_s.camelize + class_name = class_name.singularize if [ :has_many, :has_and_belongs_to_many ].include?(macro) + class_name + end + + def derive_primary_key_name + if macro == :belongs_to + "#{name}_id" + elsif options[:as] + "#{options[:as]}_id" + else + active_record.name.foreign_key + end + end + end + + # Holds all the meta-data about a :through association as it was specified in the Active Record class. + class ThroughReflection < AssociationReflection #:nodoc: + # Gets the source of the through reflection. It checks both a singularized and pluralized form for :belongs_to or :has_many. + # (The :tags association on Tagging below.) + # + # class Post < ActiveRecord::Base + # has_many :taggings + # has_many :tags, :through => :taggings + # end + # + def source_reflection + @source_reflection ||= source_reflection_names.collect { |name| through_reflection.klass.reflect_on_association(name) }.compact.first + end + # Returns the AssociationReflection object specified in the :through option # of a HasManyThrough or HasOneThrough association. Example: # @@ -204,7 +248,7 @@ module ActiveRecord # taggings_reflection = tags_reflection.through_reflection # def through_reflection - @through_reflection ||= options[:through] ? active_record.reflect_on_association(options[:through]) : false + @through_reflection ||= active_record.reflect_on_association(options[:through]) end # Gets an array of possible :through source reflection names: @@ -215,63 +259,32 @@ module ActiveRecord @source_reflection_names ||= (options[:source] ? [options[:source]] : [name.to_s.singularize, name]).collect { |n| n.to_sym } end - # Gets the source of the through reflection. It checks both a singularized and pluralized form for :belongs_to or :has_many. - # (The :tags association on Tagging below.) - # - # class Post < ActiveRecord::Base - # has_many :taggings - # has_many :tags, :through => :taggings - # end - # - def source_reflection - return nil unless through_reflection - @source_reflection ||= source_reflection_names.collect { |name| through_reflection.klass.reflect_on_association(name) }.compact.first - end - def check_validity! - if options[:through] - if through_reflection.nil? - raise HasManyThroughAssociationNotFoundError.new(active_record.name, self) - end - - if source_reflection.nil? - raise HasManyThroughSourceAssociationNotFoundError.new(self) - end + if through_reflection.nil? + raise HasManyThroughAssociationNotFoundError.new(active_record.name, self) + end - if options[:source_type] && source_reflection.options[:polymorphic].nil? - raise HasManyThroughAssociationPointlessSourceTypeError.new(active_record.name, self, source_reflection) - end - - if source_reflection.options[:polymorphic] && options[:source_type].nil? - raise HasManyThroughAssociationPolymorphicError.new(active_record.name, self, source_reflection) - end - - unless [:belongs_to, :has_many].include?(source_reflection.macro) && source_reflection.options[:through].nil? - raise HasManyThroughSourceAssociationMacroError.new(self) - end + if source_reflection.nil? + raise HasManyThroughSourceAssociationNotFoundError.new(self) + end + + if options[:source_type] && source_reflection.options[:polymorphic].nil? + raise HasManyThroughAssociationPointlessSourceTypeError.new(active_record.name, self, source_reflection) + end + + if source_reflection.options[:polymorphic] && options[:source_type].nil? + raise HasManyThroughAssociationPolymorphicError.new(active_record.name, self, source_reflection) + end + + unless [:belongs_to, :has_many].include?(source_reflection.macro) && source_reflection.options[:through].nil? + raise HasManyThroughSourceAssociationMacroError.new(self) end end private def derive_class_name # get the class_name of the belongs_to association of the through reflection - if through_reflection - options[:source_type] || source_reflection.class_name - else - class_name = name.to_s.camelize - class_name = class_name.singularize if [ :has_many, :has_and_belongs_to_many ].include?(macro) - class_name - end - end - - def derive_primary_key_name - if macro == :belongs_to - "#{name}_id" - elsif options[:as] - "#{options[:as]}_id" - else - active_record.name.foreign_key - end + options[:source_type] || source_reflection.class_name end end end diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb index e339ef41ab..e0ed3e5886 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/activerecord/test/cases/reflection_test.rb @@ -170,6 +170,10 @@ class ReflectionTest < ActiveRecord::TestCase assert_nothing_raised { Firm.reflections[:clients] == Object.new } end + def test_has_many_through_reflection + assert_kind_of ActiveRecord::Reflection::ThroughReflection, Subscriber.reflect_on_association(:books) + end + private def assert_reflection(klass, association, options) assert reflection = klass.reflect_on_association(association) -- cgit v1.2.3 From 4918e6de989a80bb2ae92183f1b4eb98c15b487f Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Sat, 4 Oct 2008 17:41:27 +0100 Subject: Remove HasManyAssociationStrategy and move the logic to ActiveRecord::Reflection::ThroughReflection. --- .../lib/active_record/association_preload.rb | 30 +++------------------- activerecord/lib/active_record/reflection.rb | 18 ++++++++++++- 2 files changed, 21 insertions(+), 27 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index 0f96ee95d5..c38b98258a 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -4,28 +4,6 @@ module ActiveRecord base.extend(ClassMethods) end - class HasManyAssociationStrategy - def initialize(through_reflection) - @through_reflection = through_reflection - end - - def primary_key - if @through_reflection && @through_reflection.macro == :belongs_to - @through_reflection.klass.primary_key - else - @through_reflection.primary_key_name - end - end - - def primary_key_name - if @through_reflection && @through_reflection.macro == :belongs_to - @through_reflection.primary_key_name - else - nil - end - end - end - module ClassMethods # Loads the named associations for the activerecord record (or records) given @@ -153,9 +131,9 @@ module ActiveRecord def preload_has_many_association(records, reflection, preload_options={}) options = reflection.options - through_reflection = reflections[options[:through]] - strat = HasManyAssociationStrategy.new(through_reflection) - id_to_record_map, ids = construct_id_map(records, strat.primary_key_name) + + primary_key_name = reflection.through_reflection_primary_key_name + id_to_record_map, ids = construct_id_map(records, primary_key_name) records.each {|record| record.send(reflection.name).loaded} if options[:through] @@ -165,7 +143,7 @@ module ActiveRecord source = reflection.source_reflection.name through_records.first.class.preload_associations(through_records, source, options) through_records.each do |through_record| - through_record_id = through_record[strat.primary_key].to_s + through_record_id = through_record[reflection.through_reflection_primary_key].to_s add_preloaded_records_to_collection(id_to_record_map[through_record_id], reflection.name, through_record.send(source)) end end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 77d03493dc..dbff4f24d6 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -117,6 +117,11 @@ module ActiveRecord @sanitized_conditions ||= klass.send(:sanitize_sql, options[:conditions]) if options[:conditions] end + # Returns +true+ if +self+ is a +belongs_to+ reflection. + def belongs_to? + macro == :belongs_to + end + private def derive_class_name name.to_s.camelize @@ -200,6 +205,9 @@ module ActiveRecord false end + def through_reflection_primary_key_name + end + def source_reflection nil end @@ -212,7 +220,7 @@ module ActiveRecord end def derive_primary_key_name - if macro == :belongs_to + if belongs_to? "#{name}_id" elsif options[:as] "#{options[:as]}_id" @@ -281,6 +289,14 @@ module ActiveRecord end end + def through_reflection_primary_key + through_reflection.belongs_to? ? through_reflection.klass.primary_key : through_reflection.primary_key_name + end + + def through_reflection_primary_key_name + through_reflection.primary_key_name if through_reflection.belongs_to? + end + private def derive_class_name # get the class_name of the belongs_to association of the through reflection -- cgit v1.2.3 From 9599948fbcd67c1c2c5fecc2dca148e998479e33 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Sat, 4 Oct 2008 20:03:42 +0100 Subject: Ensure Model.sum and Model.avg typecast appropriately. [#1066 state:resolved] Model.sum delegates typecasting to the column being summed. If that's not feasible, returns a string. Model.avg always returns big decimal. --- activerecord/lib/active_record/calculations.rb | 10 +++++++--- activerecord/test/cases/calculations_test.rb | 6 +++--- 2 files changed, 10 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb index 634236e959..5e33cf1bd4 100644 --- a/activerecord/lib/active_record/calculations.rb +++ b/activerecord/lib/active_record/calculations.rb @@ -285,11 +285,15 @@ module ActiveRecord operation = operation.to_s.downcase case operation when 'count' then value.to_i - when 'sum' then value =~ /\./ ? value.to_f : value.to_i - when 'avg' then value && value.to_f - else column ? column.type_cast(value) : value + when 'sum' then type_cast_using_column(value || '0', column) + when 'avg' then value && value.to_d + else type_cast_using_column(value, column) end end + + def type_cast_using_column(value, column) + column ? column.type_cast(value) : value + end end end end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 754fd58f35..0fa61500c0 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -18,8 +18,8 @@ class CalculationsTest < ActiveRecord::TestCase def test_should_average_field value = Account.average(:credit_limit) - assert_kind_of Float, value - assert_in_delta 53.0, value, 0.001 + assert_kind_of BigDecimal, value + assert_equal BigDecimal.new('53.0'), value end def test_should_return_nil_as_average @@ -273,7 +273,7 @@ class CalculationsTest < ActiveRecord::TestCase end def test_should_sum_expression - assert_equal 636, Account.sum("2 * credit_limit") + assert_equal '636', Account.sum("2 * credit_limit") end def test_count_with_from_option -- cgit v1.2.3