From 98e69078d5e2fe9a13bd912bbb5da00be4d43497 Mon Sep 17 00:00:00 2001 From: Jan Habermann Date: Thu, 3 Apr 2014 01:52:42 +0200 Subject: Properly handle scoping with has_many :through. Fixes #14537. --- .../lib/active_record/associations/through_association.rb | 9 +++++++-- .../cases/associations/has_many_through_associations_test.rb | 12 ++++++++++++ activerecord/test/models/post.rb | 4 ++++ activerecord/test/models/reader.rb | 2 ++ 4 files changed, 25 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index ba7d2a3782..66b1616949 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -14,9 +14,14 @@ module ActiveRecord def target_scope scope = super chain.drop(1).each do |reflection| + relation = if reflection.scope + reflection.klass.all.instance_eval(&reflection.scope) + else + reflection.klass.all + end + scope.merge!( - reflection.klass.all. - except(:select, :create_with, :includes, :preload, :joins, :eager_load) + relation.except(:select, :create_with, :includes, :preload, :joins, :eager_load) ) end scope 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 026a7fe635..fee0d2c627 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -1105,4 +1105,16 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase def test_has_many_through_with_includes_in_through_association_scope assert_not_empty posts(:welcome).author_address_extra_with_address end + + def test_has_many_through_unscope_default_scope + post = Post.create!(:title => 'Beaches', :body => "I like beaches!") + Reader.create! :person => people(:david), :post => post + LazyReader.create! :person => people(:susan), :post => post + + assert_equal 2, post.people.to_a.size + assert_equal 1, post.lazy_people.to_a.size + + assert_equal 2, post.lazy_readers_unscope_skimmers.to_a.size + assert_equal 2, post.lazy_people_unscope_skimmers.to_a.size + end end diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index faf539a562..f52130278c 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -144,6 +144,10 @@ class Post < ActiveRecord::Base has_many :lazy_readers has_many :lazy_readers_skimmers_or_not, -> { where(skimmer: [ true, false ]) }, :class_name => 'LazyReader' + + has_many :lazy_people, :through => :lazy_readers, :source => :person + has_many :lazy_readers_unscope_skimmers, -> { skimmers_or_not }, :class_name => 'LazyReader' + has_many :lazy_people_unscope_skimmers, :through => :lazy_readers_unscope_skimmers, :source => :person def self.top(limit) ranked_by_comments.limit_by(limit) diff --git a/activerecord/test/models/reader.rb b/activerecord/test/models/reader.rb index 3a6b7fad34..14b5f60a72 100644 --- a/activerecord/test/models/reader.rb +++ b/activerecord/test/models/reader.rb @@ -15,6 +15,8 @@ end class LazyReader < ActiveRecord::Base self.table_name = "readers" default_scope -> { where(skimmer: true) } + + scope :skimmers_or_not, -> { unscope(:where => :skimmer) } belongs_to :post belongs_to :person -- cgit v1.2.3 From a3d41cb071ae0fc567c3e3ab7afea9356789cbcf Mon Sep 17 00:00:00 2001 From: Jan Habermann Date: Thu, 3 Apr 2014 02:25:16 +0200 Subject: Add a CHANGELOG entry for the has_many :through scoping fix (See #14537 for details). --- activerecord/CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 18ec203fa3..3fc6e9e158 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,12 @@ +* Fixed unexpected behavior for `has_many :through` associations going through a scoped `has_many`. + + If a `has_many` association is adjusted using a scope, and another `has_many :through` + uses this association, then the scope adjustment is unexpectedly neglected. + + Fixes #14537. + + *Jan Habermann* + * Fixed error when specifying a non-empty default value on a PostgreSQL array column. Fixes #10613. -- cgit v1.2.3 From 384984d7ca188d1dae59b204650b8319de9f9f05 Mon Sep 17 00:00:00 2001 From: Jan Habermann Date: Thu, 3 Apr 2014 12:40:38 +0200 Subject: Simplify the code in target_scope --- activerecord/lib/active_record/associations/through_association.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 66b1616949..942abf7df1 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -14,11 +14,8 @@ module ActiveRecord def target_scope scope = super chain.drop(1).each do |reflection| - relation = if reflection.scope - reflection.klass.all.instance_eval(&reflection.scope) - else - reflection.klass.all - end + relation = reflection.klass.all + relation.instance_eval(&reflection.scope) if reflection.scope scope.merge!( relation.except(:select, :create_with, :includes, :preload, :joins, :eager_load) -- cgit v1.2.3 From 47a04b8bbf35238639b00bfab500a84607d8d871 Mon Sep 17 00:00:00 2001 From: Jan Habermann Date: Thu, 3 Apr 2014 19:03:31 +0200 Subject: Minor improvement: Use the merge method on the relation instead of instance_eval directly. --- activerecord/lib/active_record/associations/through_association.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 942abf7df1..ce65948133 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -15,7 +15,7 @@ module ActiveRecord scope = super chain.drop(1).each do |reflection| relation = reflection.klass.all - relation.instance_eval(&reflection.scope) if reflection.scope + relation.merge!(reflection.scope) if reflection.scope scope.merge!( relation.except(:select, :create_with, :includes, :preload, :joins, :eager_load) -- cgit v1.2.3 From 6e6c76d2ffb6eb2e72bcd7da37f6c648315bfc1c Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Mon, 21 Apr 2014 15:54:28 -0500 Subject: Rearrange deck chairs on the titanic. Organize connection handling test cases. --- .../connection_adapters/abstract_adapter_test.rb | 56 ------ .../connection_adapters/adapter_leasing_test.rb | 54 ++++++ .../connection_adapters/connection_handler_test.rb | 193 --------------------- .../merge_and_resolve_default_url_config_test.rb | 192 ++++++++++++++++++++ 4 files changed, 246 insertions(+), 249 deletions(-) delete mode 100644 activerecord/test/cases/connection_adapters/abstract_adapter_test.rb create mode 100644 activerecord/test/cases/connection_adapters/adapter_leasing_test.rb create mode 100644 activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb (limited to 'activerecord') diff --git a/activerecord/test/cases/connection_adapters/abstract_adapter_test.rb b/activerecord/test/cases/connection_adapters/abstract_adapter_test.rb deleted file mode 100644 index deed226eab..0000000000 --- a/activerecord/test/cases/connection_adapters/abstract_adapter_test.rb +++ /dev/null @@ -1,56 +0,0 @@ -require "cases/helper" - -module ActiveRecord - module ConnectionAdapters - class ConnectionPool - def insert_connection_for_test!(c) - synchronize do - @connections << c - @available.add c - end - end - end - - class AbstractAdapterTest < ActiveRecord::TestCase - attr_reader :adapter - - def setup - @adapter = AbstractAdapter.new nil, nil - end - - def test_in_use? - assert_not adapter.in_use?, 'adapter is not in use' - assert adapter.lease, 'lease adapter' - assert adapter.in_use?, 'adapter is in use' - end - - def test_lease_twice - assert adapter.lease, 'should lease adapter' - assert_not adapter.lease, 'should not lease adapter' - end - - def test_expire_mutates_in_use - assert adapter.lease, 'lease adapter' - assert adapter.in_use?, 'adapter is in use' - adapter.expire - assert_not adapter.in_use?, 'adapter is in use' - end - - def test_close - pool = ConnectionPool.new(ConnectionSpecification.new({}, nil)) - pool.insert_connection_for_test! adapter - adapter.pool = pool - - # Make sure the pool marks the connection in use - assert_equal adapter, pool.connection - assert adapter.in_use? - - # Close should put the adapter back in the pool - adapter.close - assert_not adapter.in_use? - - assert_equal adapter, pool.connection - end - end - end -end diff --git a/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb b/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb new file mode 100644 index 0000000000..662e19f35e --- /dev/null +++ b/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb @@ -0,0 +1,54 @@ +require "cases/helper" + +module ActiveRecord + module ConnectionAdapters + class AdapterLeasingTest < ActiveRecord::TestCase + class Pool < ConnectionPool + def insert_connection_for_test!(c) + synchronize do + @connections << c + @available.add c + end + end + end + + def setup + @adapter = AbstractAdapter.new nil, nil + end + + def test_in_use? + assert_not @adapter.in_use?, 'adapter is not in use' + assert @adapter.lease, 'lease adapter' + assert @adapter.in_use?, 'adapter is in use' + end + + def test_lease_twice + assert @adapter.lease, 'should lease adapter' + assert_not @adapter.lease, 'should not lease adapter' + end + + def test_expire_mutates_in_use + assert @adapter.lease, 'lease adapter' + assert @adapter.in_use?, 'adapter is in use' + @adapter.expire + assert_not @adapter.in_use?, 'adapter is in use' + end + + def test_close + pool = Pool.new(ConnectionSpecification.new({}, nil)) + pool.insert_connection_for_test! @adapter + @adapter.pool = pool + + # Make sure the pool marks the connection in use + assert_equal @adapter, pool.connection + assert @adapter.in_use? + + # Close should put the adapter back in the pool + @adapter.close + assert_not @adapter.in_use? + + assert_equal @adapter, pool.connection + end + end + end +end diff --git a/activerecord/test/cases/connection_adapters/connection_handler_test.rb b/activerecord/test/cases/connection_adapters/connection_handler_test.rb index f2d18e812d..3e33b30144 100644 --- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb @@ -2,199 +2,6 @@ require "cases/helper" module ActiveRecord module ConnectionAdapters - - class MergeAndResolveDefaultUrlConfigTest < ActiveRecord::TestCase - - def klass - ActiveRecord::ConnectionHandling::MergeAndResolveDefaultUrlConfig - end - - def setup - @previous_database_url = ENV.delete("DATABASE_URL") - end - - teardown do - ENV["DATABASE_URL"] = @previous_database_url - end - - def resolve(spec, config) - ConnectionSpecification::Resolver.new(klass.new(config).resolve).resolve(spec) - end - - def spec(spec, config) - ConnectionSpecification::Resolver.new(klass.new(config).resolve).spec(spec) - end - - def test_resolver_with_database_uri_and_current_env_symbol_key - ENV['DATABASE_URL'] = "postgres://localhost/foo" - config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } - actual = resolve(:default_env, config) - expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } - assert_equal expected, actual - end - - def test_resolver_with_database_uri_and_and_current_env_string_key - ENV['DATABASE_URL'] = "postgres://localhost/foo" - config = { "default_env" => { "adapter" => "not_postgres", "database" => "not_foo" } } - actual = assert_deprecated { resolve("default_env", config) } - expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } - assert_equal expected, actual - end - - def test_resolver_with_database_uri_and_known_key - ENV['DATABASE_URL'] = "postgres://localhost/foo" - config = { "production" => { "adapter" => "not_postgres", "database" => "not_foo", "host" => "localhost" } } - actual = resolve(:production, config) - expected = { "adapter"=>"not_postgres", "database"=>"not_foo", "host"=>"localhost" } - assert_equal expected, actual - end - - def test_resolver_with_database_uri_and_unknown_symbol_key - ENV['DATABASE_URL'] = "postgres://localhost/foo" - config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } - assert_raises AdapterNotSpecified do - resolve(:production, config) - end - end - - def test_resolver_with_database_uri_and_unknown_string_key - ENV['DATABASE_URL'] = "postgres://localhost/foo" - config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } - assert_deprecated do - assert_raises AdapterNotSpecified do - spec("production", config) - end - end - end - - def test_resolver_with_database_uri_and_supplied_url - ENV['DATABASE_URL'] = "not-postgres://not-localhost/not_foo" - config = { "production" => { "adapter" => "also_not_postgres", "database" => "also_not_foo" } } - actual = resolve("postgres://localhost/foo", config) - expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } - assert_equal expected, actual - end - - def test_jdbc_url - config = { "production" => { "url" => "jdbc:postgres://localhost/foo" } } - actual = klass.new(config).resolve - assert_equal config, actual - end - - def test_environment_does_not_exist_in_config_url_does_exist - ENV['DATABASE_URL'] = "postgres://localhost/foo" - config = { "not_default_env" => { "adapter" => "not_postgres", "database" => "not_foo" } } - actual = klass.new(config).resolve - expect_prod = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } - assert_equal expect_prod, actual["default_env"] - end - - def test_url_with_hyphenated_scheme - ENV['DATABASE_URL'] = "ibm-db://localhost/foo" - config = { "default_env" => { "adapter" => "not_postgres", "database" => "not_foo", "host" => "localhost" } } - actual = resolve(:default_env, config) - expected = { "adapter"=>"ibm_db", "database"=>"foo", "host"=>"localhost" } - assert_equal expected, actual - end - - def test_string_connection - config = { "default_env" => "postgres://localhost/foo" } - actual = klass.new(config).resolve - expected = { "default_env" => - { "adapter" => "postgresql", - "database" => "foo", - "host" => "localhost" - } - } - assert_equal expected, actual - end - - def test_url_sub_key - config = { "default_env" => { "url" => "postgres://localhost/foo" } } - actual = klass.new(config).resolve - expected = { "default_env" => - { "adapter" => "postgresql", - "database" => "foo", - "host" => "localhost" - } - } - assert_equal expected, actual - end - - def test_hash - config = { "production" => { "adapter" => "postgres", "database" => "foo" } } - actual = klass.new(config).resolve - assert_equal config, actual - end - - def test_blank - config = {} - actual = klass.new(config).resolve - assert_equal config, actual - end - - def test_blank_with_database_url - ENV['DATABASE_URL'] = "postgres://localhost/foo" - - config = {} - actual = klass.new(config).resolve - expected = { "adapter" => "postgresql", - "database" => "foo", - "host" => "localhost" } - assert_equal expected, actual["default_env"] - assert_equal nil, actual["production"] - assert_equal nil, actual["development"] - assert_equal nil, actual["test"] - assert_equal nil, actual[:production] - assert_equal nil, actual[:development] - assert_equal nil, actual[:test] - end - - def test_url_sub_key_with_database_url - ENV['DATABASE_URL'] = "NOT-POSTGRES://localhost/NOT_FOO" - - config = { "default_env" => { "url" => "postgres://localhost/foo" } } - actual = klass.new(config).resolve - expected = { "default_env" => - { "adapter" => "postgresql", - "database" => "foo", - "host" => "localhost" - } - } - assert_equal expected, actual - end - - def test_merge_no_conflicts_with_database_url - ENV['DATABASE_URL'] = "postgres://localhost/foo" - - config = {"default_env" => { "pool" => "5" } } - actual = klass.new(config).resolve - expected = { "default_env" => - { "adapter" => "postgresql", - "database" => "foo", - "host" => "localhost", - "pool" => "5" - } - } - assert_equal expected, actual - end - - def test_merge_conflicts_with_database_url - ENV['DATABASE_URL'] = "postgres://localhost/foo" - - config = {"default_env" => { "adapter" => "NOT-POSTGRES", "database" => "NOT-FOO", "pool" => "5" } } - actual = klass.new(config).resolve - expected = { "default_env" => - { "adapter" => "postgresql", - "database" => "foo", - "host" => "localhost", - "pool" => "5" - } - } - assert_equal expected, actual - end - end - class ConnectionHandlerTest < ActiveRecord::TestCase def setup @klass = Class.new(Base) { def self.name; 'klass'; end } diff --git a/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb b/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb new file mode 100644 index 0000000000..da852aaa02 --- /dev/null +++ b/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb @@ -0,0 +1,192 @@ +require "cases/helper" + +module ActiveRecord + module ConnectionAdapters + class MergeAndResolveDefaultUrlConfigTest < ActiveRecord::TestCase + def setup + @previous_database_url = ENV.delete("DATABASE_URL") + end + + teardown do + ENV["DATABASE_URL"] = @previous_database_url + end + + def resolve_config(config) + ActiveRecord::ConnectionHandling::MergeAndResolveDefaultUrlConfig.new(config).resolve + end + + def resolve_spec(spec, config) + ConnectionSpecification::Resolver.new(resolve_config(config)).resolve(spec) + end + + def test_resolver_with_database_uri_and_current_env_symbol_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } + actual = resolve_spec(:default_env, config) + expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } + assert_equal expected, actual + end + + def test_resolver_with_database_uri_and_and_current_env_string_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "default_env" => { "adapter" => "not_postgres", "database" => "not_foo" } } + actual = assert_deprecated { resolve_spec("default_env", config) } + expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } + assert_equal expected, actual + end + + def test_resolver_with_database_uri_and_known_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "production" => { "adapter" => "not_postgres", "database" => "not_foo", "host" => "localhost" } } + actual = resolve_spec(:production, config) + expected = { "adapter"=>"not_postgres", "database"=>"not_foo", "host"=>"localhost" } + assert_equal expected, actual + end + + def test_resolver_with_database_uri_and_unknown_symbol_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } + assert_raises AdapterNotSpecified do + resolve_spec(:production, config) + end + end + + def test_resolver_with_database_uri_and_unknown_string_key + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } + assert_deprecated do + assert_raises AdapterNotSpecified do + resolve_spec("production", config) + end + end + end + + def test_resolver_with_database_uri_and_supplied_url + ENV['DATABASE_URL'] = "not-postgres://not-localhost/not_foo" + config = { "production" => { "adapter" => "also_not_postgres", "database" => "also_not_foo" } } + actual = resolve_spec("postgres://localhost/foo", config) + expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } + assert_equal expected, actual + end + + def test_jdbc_url + config = { "production" => { "url" => "jdbc:postgres://localhost/foo" } } + actual = resolve_config(config) + assert_equal config, actual + end + + def test_environment_does_not_exist_in_config_url_does_exist + ENV['DATABASE_URL'] = "postgres://localhost/foo" + config = { "not_default_env" => { "adapter" => "not_postgres", "database" => "not_foo" } } + actual = resolve_config(config) + expect_prod = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } + assert_equal expect_prod, actual["default_env"] + end + + def test_url_with_hyphenated_scheme + ENV['DATABASE_URL'] = "ibm-db://localhost/foo" + config = { "default_env" => { "adapter" => "not_postgres", "database" => "not_foo", "host" => "localhost" } } + actual = resolve_spec(:default_env, config) + expected = { "adapter"=>"ibm_db", "database"=>"foo", "host"=>"localhost" } + assert_equal expected, actual + end + + def test_string_connection + config = { "default_env" => "postgres://localhost/foo" } + actual = resolve_config(config) + expected = { "default_env" => + { "adapter" => "postgresql", + "database" => "foo", + "host" => "localhost" + } + } + assert_equal expected, actual + end + + def test_url_sub_key + config = { "default_env" => { "url" => "postgres://localhost/foo" } } + actual = resolve_config(config) + expected = { "default_env" => + { "adapter" => "postgresql", + "database" => "foo", + "host" => "localhost" + } + } + assert_equal expected, actual + end + + def test_hash + config = { "production" => { "adapter" => "postgres", "database" => "foo" } } + actual = resolve_config(config) + assert_equal config, actual + end + + def test_blank + config = {} + actual = resolve_config(config) + assert_equal config, actual + end + + def test_blank_with_database_url + ENV['DATABASE_URL'] = "postgres://localhost/foo" + + config = {} + actual = resolve_config(config) + expected = { "adapter" => "postgresql", + "database" => "foo", + "host" => "localhost" } + assert_equal expected, actual["default_env"] + assert_equal nil, actual["production"] + assert_equal nil, actual["development"] + assert_equal nil, actual["test"] + assert_equal nil, actual[:production] + assert_equal nil, actual[:development] + assert_equal nil, actual[:test] + end + + def test_url_sub_key_with_database_url + ENV['DATABASE_URL'] = "NOT-POSTGRES://localhost/NOT_FOO" + + config = { "default_env" => { "url" => "postgres://localhost/foo" } } + actual = resolve_config(config) + expected = { "default_env" => + { "adapter" => "postgresql", + "database" => "foo", + "host" => "localhost" + } + } + assert_equal expected, actual + end + + def test_merge_no_conflicts_with_database_url + ENV['DATABASE_URL'] = "postgres://localhost/foo" + + config = {"default_env" => { "pool" => "5" } } + actual = resolve_config(config) + expected = { "default_env" => + { "adapter" => "postgresql", + "database" => "foo", + "host" => "localhost", + "pool" => "5" + } + } + assert_equal expected, actual + end + + def test_merge_conflicts_with_database_url + ENV['DATABASE_URL'] = "postgres://localhost/foo" + + config = {"default_env" => { "adapter" => "NOT-POSTGRES", "database" => "NOT-FOO", "pool" => "5" } } + actual = resolve_config(config) + expected = { "default_env" => + { "adapter" => "postgresql", + "database" => "foo", + "host" => "localhost", + "pool" => "5" + } + } + assert_equal expected, actual + end + end + end +end -- cgit v1.2.3 From 70b377f4648403b6facbe29b10e179eb649327a9 Mon Sep 17 00:00:00 2001 From: Earl J St Sauver Date: Tue, 15 Apr 2014 21:43:24 -0700 Subject: select! renamed to avoid name collision Array#select! Fixes #14752 Select mimics the block interface of arrays, but does not mock the block interface for select!. This change moves the api to be a private method, _select!. --- .../lib/active_record/associations/preloader/association.rb | 2 +- activerecord/lib/active_record/relation/delegation.rb | 2 +- activerecord/lib/active_record/relation/merger.rb | 10 +++++++++- activerecord/lib/active_record/relation/query_methods.rb | 4 ++-- activerecord/test/cases/relation/mutation_test.rb | 7 ++++++- 5 files changed, 19 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index 69b65982b3..e2b1cef463 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -116,7 +116,7 @@ module ActiveRecord scope.where_values = Array(values[:where]) + Array(preload_values[:where]) scope.references_values = Array(values[:references]) + Array(preload_values[:references]) - scope.select! preload_values[:select] || values[:select] || table[Arel.star] + scope._select! preload_values[:select] || values[:select] || table[Arel.star] scope.includes! preload_values[:includes] || values[:includes] if preload_values.key? :order diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb index 21beed332f..9c666dcd3b 100644 --- a/activerecord/lib/active_record/relation/delegation.rb +++ b/activerecord/lib/active_record/relation/delegation.rb @@ -40,7 +40,7 @@ module ActiveRecord BLACKLISTED_ARRAY_METHODS = [ :compact!, :flatten!, :reject!, :reverse!, :rotate!, :map!, :shuffle!, :slice!, :sort!, :sort_by!, :delete_if, - :keep_if, :pop, :shift, :delete_at, :compact + :keep_if, :pop, :shift, :delete_at, :compact, :select! ].to_set # :nodoc: delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, to: :to_a diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index be44fccad5..fcb28a18f6 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -30,6 +30,8 @@ module ActiveRecord else other.joins!(*v) end + elsif k == :select + other._select!(v) else other.send("#{k}!", v) end @@ -62,7 +64,13 @@ module ActiveRecord # expensive), most of the time the value is going to be `nil` or `.blank?`, the only catch is that # `false.blank?` returns `true`, so there needs to be an extra check so that explicit `false` values # don't fall through the cracks. - relation.send("#{name}!", *value) unless value.nil? || (value.blank? && false != value) + unless value.nil? || (value.blank? && false != value) + if name == :select + relation._select!(*value) + else + relation.send("#{name}!", *value) + end + end end merge_multi_values diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 4287304945..5340fcb0b6 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -235,11 +235,11 @@ module ActiveRecord to_a.select { |*block_args| yield(*block_args) } else raise ArgumentError, 'Call this with at least one field' if fields.empty? - spawn.select!(*fields) + spawn._select!(*fields) end end - def select!(*fields) # :nodoc: + def _select!(*fields) # :nodoc: fields.flatten! fields.map! do |field| klass.attribute_alias?(field) ? klass.attribute_alias(field) : field diff --git a/activerecord/test/cases/relation/mutation_test.rb b/activerecord/test/cases/relation/mutation_test.rb index c81a3002d6..1da5c36e1c 100644 --- a/activerecord/test/cases/relation/mutation_test.rb +++ b/activerecord/test/cases/relation/mutation_test.rb @@ -24,13 +24,18 @@ module ActiveRecord @relation ||= Relation.new FakeKlass.new('posts'), Post.arel_table end - (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order, :unscope]).each do |method| + (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order, :unscope, :select]).each do |method| test "##{method}!" do assert relation.public_send("#{method}!", :foo).equal?(relation) assert_equal [:foo], relation.public_send("#{method}_values") end end + test "#_select!" do + assert relation.public_send("_select!", :foo).equal?(relation) + assert_equal [:foo], relation.public_send("select_values") + end + test '#order!' do assert relation.order!('name ASC').equal?(relation) assert_equal ['name ASC'], relation.order_values -- cgit v1.2.3 From 40335e73ccdd6b84f69a21cfa101b9abfa9c3447 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 22 Apr 2014 18:17:01 -0500 Subject: Add CHANGELOG entry for #14757 [ci skip] --- activerecord/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 9138230b5e..cd47d0bbb4 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Fix name collision with `Array#select!` with `Relation#select!`. + + Fixes #14752. + + *Earl St Sauver* + * Fixed unexpected behavior for `has_many :through` associations going through a scoped `has_many`. If a `has_many` association is adjusted using a scope, and another `has_many :through` -- cgit v1.2.3 From 8116411abcb7dd887f06a1fe63885e105ea862e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 22 Apr 2014 18:27:19 -0500 Subject: Fix syntax error --- .../test/cases/associations/has_many_through_associations_test.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'activerecord') 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 e30577fb49..4755966259 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -1116,6 +1116,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase club.reload assert_equal [member], club.favourites + end def test_has_many_through_unscope_default_scope post = Post.create!(:title => 'Beaches', :body => "I like beaches!") -- cgit v1.2.3 From d10e2ca9f199105849444b39f55d4922339d9c8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 22 Apr 2014 18:29:16 -0500 Subject: Perfer to define methods instead of calling test This file is using this pattern already --- .../test/cases/associations/has_many_through_associations_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'activerecord') 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 4755966259..e68ad74f70 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -1082,7 +1082,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_equal ["parrot", "bulbul"], owner.toys.map { |r| r.pet.name } end - test "has many through associations on new records use null relations" do + def test_has_many_through_associations_on_new_records_use_null_relations person = Person.new assert_no_queries do @@ -1094,7 +1094,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end end - test "has many through with default scope on the target" do + def test_has_many_through_with_default_scope_on_the_target person = people(:michael) assert_equal [posts(:thinking)], person.first_posts @@ -1102,11 +1102,11 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_equal [posts(:thinking)], person.reload.first_posts end - test "has many through with includes in through association scope" do + def test_has_many_through_with_includes_in_through_association_scope assert_not_empty posts(:welcome).author_address_extra_with_address end - test "insert records via has_many_through association with scope" do + def test_insert_records_via_has_many_through_association_with_scope club = Club.create! member = Member.create! Membership.create!(club: club, member: member) -- cgit v1.2.3 From 4866399054c9facf11bc743c0e0969e6c81f6c31 Mon Sep 17 00:00:00 2001 From: Lucas Mazza Date: Tue, 22 Apr 2014 23:37:13 -0500 Subject: `ActiveRecord::Base.no_touching` no longer triggers callbacks or start empty transactions. Closes #14841. --- activerecord/CHANGELOG.md | 6 ++++++ activerecord/lib/active_record/base.rb | 2 +- activerecord/test/cases/timestamp_test.rb | 21 ++++++++++++++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index cd47d0bbb4..da07633b03 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* `ActiveRecord::Base.no_touching` no longer triggers callbacks or start empty transactions. + + Fixes #14841. + + *Lucas Mazza* + * Fix name collision with `Array#select!` with `Relation#select!`. Fixes #14752. diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 1d47cba234..db4d5f0129 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -296,7 +296,6 @@ module ActiveRecord #:nodoc: include Core include Persistence - include NoTouching include ReadonlyAttributes include ModelSchema include Inheritance @@ -318,6 +317,7 @@ module ActiveRecord #:nodoc: include NestedAttributes include Aggregations include Transactions + include NoTouching include Reflection include Serialization include Store diff --git a/activerecord/test/cases/timestamp_test.rb b/activerecord/test/cases/timestamp_test.rb index 594b4fb07b..77ab427be0 100644 --- a/activerecord/test/cases/timestamp_test.rb +++ b/activerecord/test/cases/timestamp_test.rb @@ -112,7 +112,7 @@ class TimestampTest < ActiveRecord::TestCase previous_starting = task.starting previous_ending = task.ending task.touch(:starting, :ending) - + assert_not_equal previous_starting, task.starting assert_not_equal previous_ending, task.ending assert_in_delta Time.now, task.starting, 1 @@ -170,6 +170,25 @@ class TimestampTest < ActiveRecord::TestCase assert !@developer.no_touching? end + def test_no_touching_with_callbacks + klass = Class.new(ActiveRecord::Base) do + self.table_name = "developers" + + attr_accessor :after_touch_called + + after_touch do |user| + user.after_touch_called = true + end + end + + developer = klass.first + + klass.no_touching do + developer.touch + assert_not developer.after_touch_called + end + end + def test_saving_a_record_with_a_belongs_to_that_specifies_touching_the_parent_should_update_the_parent_updated_at pet = Pet.first owner = pet.owner -- cgit v1.2.3 From cd440c9ea716ebb78eb936cda9cf9389bc1359e8 Mon Sep 17 00:00:00 2001 From: Eric Bouchut Date: Thu, 24 Apr 2014 10:55:52 +0200 Subject: Fix a typo in the doc of forty_two AR FinderMethod --- activerecord/lib/active_record/relation/finder_methods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 7af4b29ebc..d5ab761f89 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -242,7 +242,7 @@ module ActiveRecord # If no order is defined it will order by primary key. # # Person.forty_two # returns the forty-second object fetched by SELECT * FROM people - # Person.offset(3).forty_two # returns the fifth object from OFFSET 3 (which is OFFSET 44) + # Person.offset(3).forty_two # returns the forty-second object from OFFSET 3 (which is OFFSET 44) # Person.where(["user_name = :u", { u: user_name }]).forty_two def forty_two find_nth(41, offset_index) -- cgit v1.2.3 From d080e8c201b7200f7f851c7d837cd658968c5255 Mon Sep 17 00:00:00 2001 From: Jefferson Lai Date: Thu, 24 Apr 2014 04:20:30 -0700 Subject: PostgreSQL Timestamps always map to `:datetime`. The PG Adapter should use `:datetime` consistently instead of mapping mispellings to `:timestamp`. See #14513 --- activerecord/CHANGELOG.md | 14 ++++++++++++++ .../active_record/connection_adapters/postgresql/oid.rb | 7 +------ 2 files changed, 15 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index da07633b03..a4c6e5f499 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,17 @@ +* PostgreSQL should internally use `:datetime` consistently for TimeStamp. Assures + different spellings of timestamps are treated the same. + + Example: + + mytimestamp.simplified_type('timestamp without time zone') + # => :datetime + mytimestamp.simplified_type('timestamp(6) without time zone') + # => also :datetime (previously would be :timestamp) + + See #14513. + + *Jefferson Lai* + * `ActiveRecord::Base.no_touching` no longer triggers callbacks or start empty transactions. Fixes #14841. diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb index 9e898015a6..540b3694b5 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb @@ -209,12 +209,7 @@ This is not reliable and will be removed in the future. class Timestamp < Type def type; :timestamp; end def simplified_type(sql_type) - case sql_type - when /^timestamp with(?:out)? time zone$/ - :datetime - else - :timestamp - end + :datetime end def type_cast(value) -- cgit v1.2.3 From 1b7aa62b184c4410c99208f71b59bbac5c5f03be Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Thu, 24 Apr 2014 12:34:33 -0500 Subject: reset `@arel` when modifying a Relation in place. /cc @tenderlove --- activerecord/CHANGELOG.md | 5 +++++ .../associations/preloader/through_association.rb | 2 +- activerecord/lib/active_record/relation/query_methods.rb | 12 ++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index a4c6e5f499..a48d52a47e 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,8 @@ +* Reset the cache when modifying a Relation with cached Arel. + Additionally display a warning message to make the user aware. + + *Yves Senn* + * PostgreSQL should internally use `:datetime` consistently for TimeStamp. Assures different spellings of timestamps are treated the same. diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index 70e97432e4..1fed7f74e7 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -84,7 +84,7 @@ module ActiveRecord end scope.references! reflection_scope.values[:references] - scope.order! reflection_scope.values[:order] if scope.eager_loading? + scope = scope.order reflection_scope.values[:order] if scope.eager_loading? end scope diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 5340fcb0b6..7c1fe83ff3 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -64,6 +64,7 @@ module ActiveRecord # def #{name}_values=(values) # def select_values=(values) raise ImmutableRelation if @loaded # raise ImmutableRelation if @loaded + check_cached_relation @values[:#{name}] = values # @values[:select] = values end # end CODE @@ -81,11 +82,22 @@ module ActiveRecord class_eval <<-CODE, __FILE__, __LINE__ + 1 def #{name}_value=(value) # def readonly_value=(value) raise ImmutableRelation if @loaded # raise ImmutableRelation if @loaded + check_cached_relation @values[:#{name}] = value # @values[:readonly] = value end # end CODE end + def check_cached_relation # :nodoc: + if defined?(@arel) && @arel + @arel = nil + ActiveSupport::Deprecation.warn <<-WARNING +Modifying already cached Relation. The cache will be reset. +Use a cloned Relation to prevent this warning. +WARNING + end + end + def create_with_value # :nodoc: @values[:create_with] || {} end -- cgit v1.2.3 From 960707aeda1909c5a56d21e6fa5200acc47cf459 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Thu, 24 Apr 2014 16:14:08 -0500 Subject: No need for trailing slash on migration path. Causes a double // in Dir.glob that breaks Ruby 2.2-trunk. Not really a bug, but not relevant to this test either. Originally added in ed21f0c50270139ddb6993acfdaea4586ffd09a3 --- activerecord/test/cases/migrator_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/migrator_test.rb b/activerecord/test/cases/migrator_test.rb index c77a818b93..9568aa2217 100644 --- a/activerecord/test/cases/migrator_test.rb +++ b/activerecord/test/cases/migrator_test.rb @@ -92,7 +92,7 @@ module ActiveRecord def test_relative_migrations list = Dir.chdir(MIGRATIONS_ROOT) do - ActiveRecord::Migrator.migrations("valid/") + ActiveRecord::Migrator.migrations("valid") end migration_proxy = list.find { |item| -- cgit v1.2.3 From 7fe5ae8d237c8f821bc5e984f98d9d7eb7c35266 Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Mon, 25 Mar 2013 16:00:24 +0100 Subject: move AR length validation tests into separate test-case. Conflicts: activerecord/test/cases/validations/association_validation_test.rb --- .../validations/association_validation_test.rb | 43 +------------------- .../cases/validations/length_validation_test.rb | 47 ++++++++++++++++++++++ 2 files changed, 48 insertions(+), 42 deletions(-) create mode 100644 activerecord/test/cases/validations/length_validation_test.rb (limited to 'activerecord') diff --git a/activerecord/test/cases/validations/association_validation_test.rb b/activerecord/test/cases/validations/association_validation_test.rb index 602f633c45..e4edc437e6 100644 --- a/activerecord/test/cases/validations/association_validation_test.rb +++ b/activerecord/test/cases/validations/association_validation_test.rb @@ -1,44 +1,14 @@ -# encoding: utf-8 require "cases/helper" require 'models/topic' require 'models/reply' -require 'models/owner' -require 'models/pet' require 'models/man' require 'models/interest' class AssociationValidationTest < ActiveRecord::TestCase - fixtures :topics, :owners + fixtures :topics repair_validations(Topic, Reply) - def test_validates_size_of_association - repair_validations Owner do - assert_nothing_raised { Owner.validates_size_of :pets, :minimum => 1 } - o = Owner.new('name' => 'nopets') - assert !o.save - assert o.errors[:pets].any? - o.pets.build('name' => 'apet') - assert o.valid? - end - end - - def test_validates_size_of_association_using_within - repair_validations Owner do - assert_nothing_raised { Owner.validates_size_of :pets, :within => 1..2 } - o = Owner.new('name' => 'nopets') - assert !o.save - assert o.errors[:pets].any? - - o.pets.build('name' => 'apet') - assert o.valid? - - 2.times { o.pets.build('name' => 'apet') } - assert !o.save - assert o.errors[:pets].any? - end - end - def test_validates_associated_many Topic.validates_associated(:replies) Reply.validates_presence_of(:content) @@ -94,17 +64,6 @@ class AssociationValidationTest < ActiveRecord::TestCase assert r.valid? end - def test_validates_size_of_association_utf8 - repair_validations Owner do - assert_nothing_raised { Owner.validates_size_of :pets, :minimum => 1 } - o = Owner.new('name' => 'あいうえおかきくけこ') - assert !o.save - assert o.errors[:pets].any? - o.pets.build('name' => 'あいうえおかきくけこ') - assert o.valid? - end - end - def test_validates_presence_of_belongs_to_association__parent_is_new_record repair_validations(Interest) do # Note that Interest and Man have the :inverse_of option set diff --git a/activerecord/test/cases/validations/length_validation_test.rb b/activerecord/test/cases/validations/length_validation_test.rb new file mode 100644 index 0000000000..4a92da38ce --- /dev/null +++ b/activerecord/test/cases/validations/length_validation_test.rb @@ -0,0 +1,47 @@ +# -*- coding: utf-8 -*- +require "cases/helper" +require 'models/owner' +require 'models/pet' + +class LengthValidationTest < ActiveRecord::TestCase + fixtures :owners + repair_validations(Owner) + + def test_validates_size_of_association + repair_validations Owner do + assert_nothing_raised { Owner.validates_size_of :pets, :minimum => 1 } + o = Owner.new('name' => 'nopets') + assert !o.save + assert o.errors[:pets].any? + o.pets.build('name' => 'apet') + assert o.valid? + end + end + + def test_validates_size_of_association_using_within + repair_validations Owner do + assert_nothing_raised { Owner.validates_size_of :pets, :within => 1..2 } + o = Owner.new('name' => 'nopets') + assert !o.save + assert o.errors[:pets].any? + + o.pets.build('name' => 'apet') + assert o.valid? + + 2.times { o.pets.build('name' => 'apet') } + assert !o.save + assert o.errors[:pets].any? + end + end + + def test_validates_size_of_association_utf8 + repair_validations Owner do + assert_nothing_raised { Owner.validates_size_of :pets, :minimum => 1 } + o = Owner.new('name' => 'あいうえおかきくけこ') + assert !o.save + assert o.errors[:pets].any? + o.pets.build('name' => 'あいうえおかきくけこ') + assert o.valid? + end + end +end -- cgit v1.2.3