From 15e2da656f41af0124f7577858536f3b65462ad5 Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Thu, 6 Oct 2016 13:47:26 +0100 Subject: Restore RecordNotFound when *_ids= can't find records by ID 9c9fb19 changed the behaviour of the _ids= setters for associations to raise an AssociationTypeMismatch when unknown IDs are given: Class: Message: <"Developer(#43811860) expected, got NilClass(#16732720)"> This restores the original ActiveRecord::RecordNotFound exception with a much clearer error message: Class: Message: <"Couldn't find all Developers with 'id': (1, -9999) [WHERE \"contracts\".\"company_id\" = ?] (found 1 results, but was looking for 2)"> Fixes #25719 --- activerecord/CHANGELOG.md | 5 +++++ .../lib/active_record/associations/collection_association.rb | 8 ++++++-- activerecord/lib/active_record/relation/finder_methods.rb | 8 ++++---- .../test/cases/associations/has_many_through_associations_test.rb | 3 ++- 4 files changed, 17 insertions(+), 7 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 0b9a2f9943..f4218cc110 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,8 @@ +* Raise ActiveRecord::RecordNotFound from collection `*_ids` setters + for unknown IDs with a better error message. + + *Dominic Cleal* + * For PostgreSQL >= 9.4 use `pgcrypto`'s `gen_random_uuid()` instead of `uuid-ossp`'s UUID generation function. diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index b2cf4713bb..99e6696c66 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -73,8 +73,12 @@ module ActiveRecord ids.map! { |i| pk_type.cast(i) } records = klass.where(reflection.association_primary_key => ids).index_by do |r| r.send(reflection.association_primary_key) - end.values_at(*ids) - replace(records) + end.values_at(*ids).compact + if records.size != ids.size + scope.raise_record_not_found_exception!(ids, records.size, ids.size, reflection.association_primary_key) + else + replace(records) + end end def reset diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 55ded4c6d0..93c8722aa3 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -345,7 +345,7 @@ module ActiveRecord # of results obtained should be provided in the +result_size+ argument and # the expected number of results should be provided in the +expected_size+ # argument. - def raise_record_not_found_exception!(ids = nil, result_size = nil, expected_size = nil) # :nodoc: + def raise_record_not_found_exception!(ids = nil, result_size = nil, expected_size = nil, key = primary_key) # :nodoc: conditions = arel.where_sql(@klass.arel_engine) conditions = " [#{conditions}]" if conditions name = @klass.name @@ -355,10 +355,10 @@ module ActiveRecord error << " with#{conditions}" if conditions raise RecordNotFound.new(error, name) elsif Array(ids).size == 1 - error = "Couldn't find #{name} with '#{primary_key}'=#{ids}#{conditions}" - raise RecordNotFound.new(error, name, primary_key, ids) + error = "Couldn't find #{name} with '#{key}'=#{ids}#{conditions}" + raise RecordNotFound.new(error, name, key, ids) else - error = "Couldn't find all #{name.pluralize} with '#{primary_key}': " + error = "Couldn't find all #{name.pluralize} with '#{key}': " error << "(#{ids.join(", ")})#{conditions} (found #{result_size} results, but was looking for #{expected_size})" raise RecordNotFound.new(error, name, primary_key, ids) 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 c2239ac03a..08a22a58b1 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -886,7 +886,8 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase def test_collection_singular_ids_setter_raises_exception_when_invalid_ids_set company = companies(:rails_core) ids = [Developer.first.id, -9999] - assert_raises(ActiveRecord::AssociationTypeMismatch) { company.developer_ids = ids } + e = assert_raises(ActiveRecord::RecordNotFound) { company.developer_ids = ids } + assert_match(/Couldn't find all Developers with 'id'/, e.message) end def test_build_a_model_from_hm_through_association_with_where_clause -- cgit v1.2.3 From 935502062e647def60288944808240667f7893cc Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Fri, 7 Oct 2016 08:56:05 +0100 Subject: Add test for collection *_ids= setter when association primary key set Fixes casting of IDs to the data type of the association primary key, rather than then the data type of the model's primary key. (Tests use a string primary key on the association, rather than an int.) Tests issue #20995 --- activerecord/CHANGELOG.md | 4 ++++ .../active_record/associations/collection_association.rb | 2 +- activerecord/lib/active_record/reflection.rb | 4 ++++ .../associations/has_many_through_associations_test.rb | 14 ++++++++++++++ 4 files changed, 23 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index f4218cc110..35f7c158a6 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,6 +1,10 @@ * Raise ActiveRecord::RecordNotFound from collection `*_ids` setters for unknown IDs with a better error message. + Changes the collection `*_ids` setters to cast provided IDs the data + type of the primary key set in the association, not the model + primary key. + *Dominic Cleal* * For PostgreSQL >= 9.4 use `pgcrypto`'s `gen_random_uuid()` instead of diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 99e6696c66..3d23fa1e46 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -68,7 +68,7 @@ module ActiveRecord # Implements the ids writer method, e.g. foo.item_ids= for Foo.has_many :items def ids_writer(ids) - pk_type = reflection.primary_key_type + pk_type = reflection.association_primary_key_type ids = Array(ids).reject(&:blank?) ids.map! { |i| pk_type.cast(i) } records = klass.where(reflection.association_primary_key => ids).index_by do |r| diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index ef3c3bfae8..17751c9571 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -397,6 +397,10 @@ module ActiveRecord options[:primary_key] || primary_key(klass || self.klass) end + def association_primary_key_type + klass.type_for_attribute(association_primary_key) + end + def active_record_primary_key @active_record_primary_key ||= options[:primary_key] || primary_key(active_record) 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 08a22a58b1..8defb09db7 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -883,6 +883,13 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end + def test_collection_singular_ids_setter_with_changed_primary_key + company = companies(:first_firm) + client = companies(:first_client) + company.clients_using_primary_key_ids = [client.name] + assert_equal [client], company.clients_using_primary_key + end + def test_collection_singular_ids_setter_raises_exception_when_invalid_ids_set company = companies(:rails_core) ids = [Developer.first.id, -9999] @@ -890,6 +897,13 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_match(/Couldn't find all Developers with 'id'/, e.message) end + def test_collection_singular_ids_setter_raises_exception_when_invalid_ids_set_with_changed_primary_key + company = companies(:first_firm) + ids = [Client.first.name, "unknown client"] + e = assert_raises(ActiveRecord::RecordNotFound) { company.clients_using_primary_key_ids = ids } + assert_match(/Couldn't find all Clients with 'name'/, e.message) + end + def test_build_a_model_from_hm_through_association_with_where_clause assert_nothing_raised { books(:awdr).subscribers.where(nick: "marklazz").build } end -- cgit v1.2.3