Permalink
Browse files

Raise an exception on unknown primary key inside AssociationReflection.

An association between two models cannot be made if a relevant key is
unknown, so fail fast rather than generating invalid SQL. Fixes #3207.
  • Loading branch information...
1 parent 4f2c238 commit f8beca00dc54e625dd98926ef0c0f05d67078a0c @jonleighton jonleighton committed Oct 5, 2011
@@ -1,5 +1,10 @@
*Rails 3.1.1 (unreleased)*
+* Raise an exception if the primary key of a model in an association is needed
+ but unknown. Fixes #3207.
+
+ [Jon Leighton]
+
* Add deprecation for the preload_associations method. Fixes #3022.
[Jon Leighton]
@@ -169,4 +169,17 @@ def initialize(errors)
@errors = errors
end
end
+
+ # Raised when a primary key is needed, but there is not one specified in the schema or model.
+ class UnknownPrimaryKey < ActiveRecordError
+ attr_reader :model
+
+ def initialize(model)
+ @model = model
+ end
+
+ def message
+ "Unknown primary key for table #{model.table_name} in model #{model}."
+ end
+ end
end
@@ -225,11 +225,11 @@ def association_foreign_key
# klass option is necessary to support loading polymorphic associations
def association_primary_key(klass = nil)
- options[:primary_key] || (klass || self.klass).primary_key
+ options[:primary_key] || primary_key(klass || self.klass)
end
def active_record_primary_key
- @active_record_primary_key ||= options[:primary_key] || active_record.primary_key
+ @active_record_primary_key ||= options[:primary_key] || primary_key(active_record)
end
def counter_cache_column
@@ -369,6 +369,10 @@ def derive_foreign_key
active_record.name.foreign_key
end
end
+
+ def primary_key(klass)
+ klass.primary_key || raise(UnknownPrimaryKey.new(klass))
+ end
end
# Holds all the meta-data about a :through association as it was specified
@@ -473,15 +477,15 @@ def nested?
# We want to use the klass from this reflection, rather than just delegate straight to
# the source_reflection, because the source_reflection may be polymorphic. We still
# need to respect the source_reflection's :primary_key option, though.
- def association_primary_key(klass = self.klass)
+ def association_primary_key(klass = nil)
# Get the "actual" source reflection if the immediate source reflection has a
# source reflection itself
source_reflection = self.source_reflection
while source_reflection.source_reflection
source_reflection = source_reflection.source_reflection
end
- source_reflection.options[:primary_key] || klass.primary_key
+ source_reflection.options[:primary_key] || primary_key(klass || self.klass)
end
# Gets an array of possible <tt>:through</tt> source reflection names:
@@ -18,6 +18,7 @@
require 'models/subscription'
require 'models/tag'
require 'models/sponsor'
+require 'models/edge'
class ReflectionTest < ActiveRecord::TestCase
include ActiveRecord::Reflection
@@ -252,11 +253,25 @@ def test_association_primary_key
assert_equal "custom_primary_key", Author.reflect_on_association(:tags_with_primary_key).association_primary_key.to_s # nested
end
+ def test_association_primary_key_raises_when_missing_primary_key
+ reflection = ActiveRecord::Reflection::AssociationReflection.new(:fuu, :edge, {}, Author)
+ assert_raises(ActiveRecord::UnknownPrimaryKey) { reflection.association_primary_key }
+
+ through = ActiveRecord::Reflection::ThroughReflection.new(:fuu, :edge, {}, Author)
+ through.stubs(:source_reflection).returns(stub_everything(:options => {}, :class_name => 'Edge'))
+ assert_raises(ActiveRecord::UnknownPrimaryKey) { through.association_primary_key }
+ end
+
def test_active_record_primary_key
assert_equal "nick", Subscriber.reflect_on_association(:subscriptions).active_record_primary_key.to_s
assert_equal "name", Author.reflect_on_association(:essay).active_record_primary_key.to_s
end
+ def test_active_record_primary_key_raises_when_missing_primary_key
+ reflection = ActiveRecord::Reflection::AssociationReflection.new(:fuu, :author, {}, Edge)
+ assert_raises(ActiveRecord::UnknownPrimaryKey) { reflection.active_record_primary_key }
+ end
+
def test_foreign_type
assert_equal "sponsorable_type", Sponsor.reflect_on_association(:sponsorable).foreign_type.to_s
assert_equal "sponsorable_type", Sponsor.reflect_on_association(:thing).foreign_type.to_s

8 comments on commit f8beca0

@jeremy
Member
jeremy commented on f8beca0 Oct 9, 2011

@jonleighton, this was applied to 3-1-stable only. Master?

@jonleighton
Member

@jeremy It is in master also. Commit 2e9e647

@jonleighton
Member

@jeremy just read through the campfire chat about changelogs etc. my process has generally been to fix stuff on master, then backport it to 3-1-stable and add a changelog there as part of the backport commit, assuming that the 3-1-stable changelog would be merged back to master when a release is made. the reason for this is that 1) sometimes you don't know if a change in master will end up getting backported and 2) a bugfix is changelog-worthy for a point release, but not for a major release. however, I agree our process is a bit convoluted so if you have better ideas about how we should do it please let them be known :)

(but fwiw, I always bugfix first in master and then backport, so there shouldn't be any bugfixes from me that are missing from master)

@jeremy
Member
jeremy commented on f8beca0 Oct 10, 2011

I only caught one fix on 3-1-stable and not on master. I wouldn't have thought to look if the changelogs weren't so obviously divergent. Seeing things in 3-1-stable changelog and not master implies that those changes weren't made to master.

Merging 3-1-stable changelog to master on release propagates that same (dangerous) assumption, that those changes were actually applied.

Let's just keep the changelog in sync and accurate at all times. Much clearer. Or, git rm CHANGELOG and use a NEWS instead!

@tenderlove
Member

@jeremy We should use GNU style changelog and NEWS. :-) ❤️

@jeremy
Member
jeremy commented on f8beca0 Oct 10, 2011

@tenderlove the GNU changelog duplicates git history too much, I think. I like Ruby's NEWS, though: https://github.com/ruby/ruby/blob/ruby_1_9_3/NEWS

@tenderlove
Member

Well, we don't need to update CHANGELOG on every commit. Only update CHANGELOG for "changelog worthy" entries. To me, the important thing is removal of the version numbers from CHANGELOG. That said, I can totally get behind just rm'ing the file.

@jeremy
Member
jeremy commented on f8beca0 Oct 10, 2011

I mean the GNU format has too much junk in it: full timestamp, author name/email, files affected. All stuff better learned from git history.

👍 to rm

Please sign in to comment.