Skip to content
This repository
Browse code

Raise error on unknown primary key.

If we don't have a primary key when we ask for it, it's better to fail
fast. Fixes GH #2307.
  • Loading branch information...
commit ee2be435b1e5c0e94a4ee93a1a310e0471a77d07 1 parent 5711a35
Jon Leighton authored October 05, 2011
7  activerecord/lib/active_record/attribute_methods/primary_key.rb
@@ -14,6 +14,8 @@ module ClassMethods
14 14
         # primary_key_prefix_type setting, though.
15 15
         def primary_key
16 16
           @primary_key ||= reset_primary_key
  17
+          raise ActiveRecord::UnknownPrimaryKey.new(self) unless @primary_key
  18
+          @primary_key
17 19
         end
18 20
 
19 21
         # Returns a quoted version of the primary key name, used to construct SQL statements.
@@ -29,6 +31,11 @@ def reset_primary_key #:nodoc:
29 31
           key
30 32
         end
31 33
 
  34
+        def primary_key? #:nodoc:
  35
+          @primary_key ||= reset_primary_key
  36
+          !@primary_key.nil?
  37
+        end
  38
+
32 39
         def get_primary_key(base_name) #:nodoc:
33 40
           return 'id' unless base_name && !base_name.blank?
34 41
 
6  activerecord/lib/active_record/attribute_methods/read.rb
@@ -40,7 +40,7 @@ def define_method_attribute(attr_name)
40 40
               define_read_method(attr_name, attr_name, columns_hash[attr_name])
41 41
             end
42 42
 
43  
-            if attr_name == primary_key && attr_name != "id"
  43
+            if primary_key? && attr_name == primary_key && attr_name != "id"
44 44
               define_read_method('id', attr_name, columns_hash[attr_name])
45 45
             end
46 46
           end
@@ -63,7 +63,7 @@ def define_read_method(method_name, attr_name, column)
63 63
             cast_code = column.type_cast_code('v')
64 64
             access_code = "(v=@attributes['#{attr_name}']) && #{cast_code}"
65 65
 
66  
-            unless attr_name.to_s == self.primary_key.to_s
  66
+            unless primary_key? && attr_name.to_s == primary_key.to_s
67 67
               access_code.insert(0, "missing_attribute('#{attr_name}', caller) unless @attributes.has_key?('#{attr_name}'); ")
68 68
             end
69 69
 
@@ -107,7 +107,7 @@ def read_attribute(attr_name)
107 107
 
108 108
       def _read_attribute(attr_name)
109 109
         attr_name = attr_name.to_s
110  
-        attr_name = self.class.primary_key if attr_name == 'id'
  110
+        attr_name = self.class.primary_key? && self.class.primary_key if attr_name == 'id'
111 111
         value = @attributes[attr_name]
112 112
         unless value.nil?
113 113
           if column = column_for_attribute(attr_name)
2  activerecord/lib/active_record/attribute_methods/write.rb
@@ -18,7 +18,7 @@ def define_method_attribute=(attr_name)
18 18
               end
19 19
             end
20 20
 
21  
-            if attr_name == primary_key && attr_name != "id"
  21
+            if primary_key? && attr_name == primary_key && attr_name != "id"
22 22
               generated_attribute_methods.module_eval("alias :id= :'#{primary_key}='")
23 23
             end
24 24
           end
9  activerecord/lib/active_record/base.rb
@@ -708,7 +708,7 @@ def table_exists?
708 708
       # Returns an array of column objects for the table associated with this class.
709 709
       def columns
710 710
         if defined?(@primary_key)
711  
-          connection_pool.primary_keys[table_name] ||= primary_key
  711
+          connection_pool.primary_keys[table_name] ||= @primary_key
712 712
         end
713 713
 
714 714
         connection_pool.columns[table_name]
@@ -953,7 +953,7 @@ def before_remove_const #:nodoc:
953 953
       # objects of different types from the same table.
954 954
       def instantiate(record)
955 955
         sti_class = find_sti_class(record[inheritance_column])
956  
-        record_id = sti_class.primary_key && record[sti_class.primary_key]
  956
+        record_id = sti_class.primary_key? && record[sti_class.primary_key]
957 957
 
958 958
         if ActiveRecord::IdentityMap.enabled? && record_id
959 959
           if (column = sti_class.columns_hash[sti_class.primary_key]) && column.number?
@@ -1941,8 +1941,9 @@ def ensure_proper_type
1941 1941
 
1942 1942
       # The primary key and inheritance column can never be set by mass-assignment for security reasons.
1943 1943
       def self.attributes_protected_by_default
1944  
-        default = [ primary_key, inheritance_column ]
1945  
-        default << 'id' unless primary_key.eql? 'id'
  1944
+        default = [ inheritance_column ]
  1945
+        default << primary_key if primary_key?
  1946
+        default << 'id' unless primary_key? && primary_key == 'id'
1946 1947
         default
1947 1948
       end
1948 1949
 
14  activerecord/lib/active_record/errors.rb
@@ -169,4 +169,18 @@ def initialize(errors)
169 169
       @errors = errors
170 170
     end
171 171
   end
  172
+
  173
+  # Raised when a model attempts to fetch its primary key from the database, but the table
  174
+  # has no primary key declared.
  175
+  class UnknownPrimaryKey < ActiveRecordError
  176
+    attr_reader :model
  177
+
  178
+    def initialize(model)
  179
+      @model = model
  180
+    end
  181
+
  182
+    def message
  183
+      "Unknown primary key for table #{model.table_name} in model #{model}."
  184
+    end
  185
+  end
172 186
 end
2  activerecord/lib/active_record/fixtures.rb
@@ -622,7 +622,7 @@ def table_rows
622 622
 
623 623
     private
624 624
       def primary_key_name
625  
-        @primary_key_name ||= model_class && model_class.primary_key
  625
+        @primary_key_name ||= model_class && model_class.primary_key? && model_class.primary_key
626 626
       end
627 627
 
628 628
       def has_primary_key_column?
2  activerecord/lib/active_record/persistence.rb
@@ -314,7 +314,7 @@ def create
314 314
 
315 315
       new_id = self.class.unscoped.insert attributes_values
316 316
 
317  
-      self.id ||= new_id if self.class.primary_key
  317
+      self.id ||= new_id if self.class.primary_key?
318 318
 
319 319
       IdentityMap.add(self) if IdentityMap.enabled?
320 320
       @new_record = false
6  activerecord/lib/active_record/relation.rb
@@ -13,7 +13,7 @@ class Relation
13 13
 
14 14
     # These are explicitly delegated to improve performance (avoids method_missing)
15 15
     delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to => :to_a
16  
-    delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key, :connection, :column_hash,:to => :klass
  16
+    delegate :table_name, :quoted_table_name, :primary_key, :primary_key?, :quoted_primary_key, :connection, :column_hash,:to => :klass
17 17
 
18 18
     attr_reader :table, :klass, :loaded
19 19
     attr_accessor :extensions, :default_scoped
@@ -36,7 +36,7 @@ def initialize(klass, table)
36 36
     def insert(values)
37 37
       primary_key_value = nil
38 38
 
39  
-      if primary_key && Hash === values
  39
+      if primary_key? && Hash === values
40 40
         primary_key_value = values[values.keys.find { |k|
41 41
           k.name == primary_key
42 42
         }]
@@ -70,7 +70,7 @@ def insert(values)
70 70
       conn.insert(
71 71
         im,
72 72
         'SQL',
73  
-        primary_key,
  73
+        primary_key? && primary_key,
74 74
         primary_key_value,
75 75
         nil,
76 76
         binds)
2  activerecord/lib/active_record/transactions.rb
@@ -303,7 +303,7 @@ def with_transaction_returning_status
303 303
     # Save the new record state and id of a record so it can be restored later if a transaction fails.
304 304
     def remember_transaction_record_state #:nodoc
305 305
       @_start_transaction_state ||= {}
306  
-      @_start_transaction_state[:id] = id if has_attribute?(self.class.primary_key)
  306
+      @_start_transaction_state[:id] = id if self.class.primary_key?
307 307
       unless @_start_transaction_state.include?(:new_record)
308 308
         @_start_transaction_state[:new_record] = @new_record
309 309
       end
4  activerecord/test/cases/attribute_methods/read_test.rb
@@ -24,6 +24,10 @@ def self.column_names
24 24
           def self.primary_key
25 25
           end
26 26
 
  27
+          def self.primary_key?
  28
+            false
  29
+          end
  30
+
27 31
           def self.columns
28 32
             column_names.map { FakeColumn.new(name) }
29 33
           end
4  activerecord/test/cases/base_test.rb
@@ -97,10 +97,6 @@ def test_columns_should_obey_set_primary_key
97 97
     assert pk.primary, 'nick should be primary key'
98 98
   end
99 99
 
100  
-  def test_primary_key_with_no_id
101  
-    assert_nil Edge.primary_key
102  
-  end
103  
-
104 100
   unless current_adapter?(:PostgreSQLAdapter,:OracleAdapter,:SQLServerAdapter)
105 101
     def test_limit_with_comma
106 102
       assert_nothing_raised do
14  activerecord/test/cases/primary_keys_test.rb
@@ -5,6 +5,7 @@
5 5
 require 'models/movie'
6 6
 require 'models/keyboard'
7 7
 require 'models/mixed_case_monkey'
  8
+require 'models/edge'
8 9
 
9 10
 class PrimaryKeysTest < ActiveRecord::TestCase
10 11
   fixtures :topics, :subscribers, :movies, :mixed_case_monkeys
@@ -161,4 +162,17 @@ def test_set_primary_key_with_no_connection
161 162
 
162 163
     assert_equal 'foo', model.primary_key
163 164
   end
  165
+
  166
+  def test_no_primary_key_raises
  167
+    assert_raises(ActiveRecord::UnknownPrimaryKey) do
  168
+      Edge.primary_key
  169
+    end
  170
+
  171
+    begin
  172
+      Edge.primary_key
  173
+    rescue ActiveRecord::UnknownPrimaryKey => e
  174
+      assert e.message.include?('edges')
  175
+      assert e.message.include?('Edge')
  176
+    end
  177
+  end
164 178
 end

9 notes on commit ee2be43

Jon Leighton
Owner

@spastorino @tenderlove @josevalim I haven't cherry-picked this into 3-1-stable. Please could you review and check that you're okay with it? Or else we need to figure out another way of dealing with #2307 (perhaps ignoring for now).

We're now doing primary_key? && primary_key in a few places. That's not ideal but can be cleaned up later I think.

Jon Leighton
Owner

Oops, I meant to link #3207.

Santiago Pastorino
Owner

My vote is to not merge this to 3-1-stable, just use set_primary_key or add a primary key to the schema

Aaron Patterson
Owner

Is it really necessary to raise the exception here? From our discussion, it seemed better if we raise the exception from the code that is doing the join. Something like raise CannotJoin unless blah.primary_key?. Raising here seems very heavy handed, especially since we already return nil to indicate that there is no primary_key (which is the behavior I would expect).

Aaron Patterson
Owner

@spastorino my vote is to revert this, and raise an exception where the actual error is occurring (during join construction). ;-)

Jon Leighton
Owner

I agree it feels heavy handed.

The thing is, there are multiple places where we're calling primary_key and expect it to be non-nil. (E.g. search for primary_key in reflection.rb.) Raising every time at source would get painful.

Maybe we should revert this and implement a primary_key! method that raises on nil? Then, change all the calls to primary_key in Reflection to be primary_key!. That should solve the majority of cases, although I am sure there will still be edge cases that are missed.

What do you think?

Yasuo Honda

I prefer this commit revoked or implemented in different way.
because rake test_oracle got 141 errors with Oracle enhanced adapter with following reasons.

ActiveRecord::UnknownPrimaryKey: Unknown primary key for table edges in model Edge.
ActiveRecord::UnknownPrimaryKey: Unknown primary key for table mateys in model Matey.

if it's better to file a new issue, I'll do that.

Jon Leighton
Owner

Reverted here 6474765

New fix here 2e9e647

Yasuo Honda

Thanks! new fix works like a charm.

Please sign in to comment.
Something went wrong with that request. Please try again.