Skip to content
This repository
Browse code

Raise error when using write_attribute with a non-existent attribute.

Previously we would just silently write the attribute. This can lead to
subtle bugs (for example, see the change in AutosaveAssociation where a
through association would wrongly gain an attribute.

Also, ensuring that we never gain any new attributes after
initialization will allow me to reduce our dependence on method_missing.
  • Loading branch information...
commit 50d395f96ea05da1e02459688e94bff5872c307b 1 parent 8667d3a
Jon Leighton authored September 10, 2011
10  activerecord/lib/active_record/attribute_methods/write.rb
@@ -28,12 +28,16 @@ def define_method_attribute=(attr_name)
28 28
       # for fixnum and float columns are turned into +nil+.
29 29
       def write_attribute(attr_name, value)
30 30
         attr_name = attr_name.to_s
31  
-        attr_name = self.class.primary_key if attr_name == 'id'
  31
+        attr_name = self.class.primary_key if attr_name == 'id' && self.class.primary_key
32 32
         @attributes_cache.delete(attr_name)
33  
-        if (column = column_for_attribute(attr_name)) && column.number?
  33
+        column = column_for_attribute(attr_name)
  34
+
  35
+        if column && column.number?
34 36
           @attributes[attr_name] = convert_number_column_value(value)
35  
-        else
  37
+        elsif column || @attributes.has_key?(attr_name)
36 38
           @attributes[attr_name] = value
  39
+        else
  40
+          raise ActiveModel::MissingAttributeError, "can't write unknown attribute `#{attr_name}'"
37 41
         end
38 42
       end
39 43
       alias_method :raw_write_attribute, :write_attribute
5  activerecord/lib/active_record/autosave_association.rb
@@ -370,7 +370,10 @@ def save_has_one_association(reflection)
370 370
         else
371 371
           key = reflection.options[:primary_key] ? send(reflection.options[:primary_key]) : id
372 372
           if autosave != false && (new_record? || record.new_record? || record[reflection.foreign_key] != key || autosave)
373  
-            record[reflection.foreign_key] = key
  373
+            unless reflection.through_reflection
  374
+              record[reflection.foreign_key] = key
  375
+            end
  376
+
374 377
             saved = record.save(:validate => !autosave)
375 378
             raise ActiveRecord::Rollback if !saved && autosave
376 379
             saved
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
  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
4  activerecord/lib/active_record/timestamp.rb
@@ -48,7 +48,9 @@ def create #:nodoc:
48 48
         current_time = current_time_from_proper_timezone
49 49
 
50 50
         all_timestamp_attributes.each do |column|
51  
-          write_attribute(column.to_s, current_time) if respond_to?(column) && self.send(column).nil?
  51
+          if respond_to?(column) && respond_to?("#{column}=") && self.send(column).nil?
  52
+            write_attribute(column.to_s, current_time)
  53
+          end
52 54
         end
53 55
       end
54 56
 
17  activerecord/test/cases/persistence_test.rb
@@ -202,9 +202,12 @@ def test_create_many
202 202
   end
203 203
 
204 204
   def test_create_columns_not_equal_attributes
205  
-    topic = Topic.new
206  
-    topic.title = 'Another New Topic'
207  
-    topic.send :write_attribute, 'does_not_exist', 'test'
  205
+    topic = Topic.allocate.init_with(
  206
+      'attributes' => {
  207
+        'title'          => 'Another New Topic',
  208
+        'does_not_exist' => 'test'
  209
+      }
  210
+    )
208 211
     assert_nothing_raised { topic.save }
209 212
   end
210 213
 
@@ -249,9 +252,11 @@ def test_update_columns_not_equal_attributes
249 252
     topic.title = "Still another topic"
250 253
     topic.save
251 254
 
252  
-    topicReloaded = Topic.find(topic.id)
253  
-    topicReloaded.title = "A New Topic"
254  
-    topicReloaded.send :write_attribute, 'does_not_exist', 'test'
  255
+    topicReloaded = Topic.allocate
  256
+    topicReloaded.init_with(
  257
+      'attributes' => topic.attributes.merge('does_not_exist' => 'test')
  258
+    )
  259
+    topicReloaded.title = 'A New Topic'
255 260
     assert_nothing_raised { topicReloaded.save }
256 261
   end
257 262
 
13  activerecord/test/cases/serialization_test.rb
@@ -7,12 +7,13 @@ class SerializationTest < ActiveRecord::TestCase
7 7
 
8 8
   def setup
9 9
     @contact_attributes = {
10  
-      :name        => 'aaron stack',
11  
-      :age         => 25,
12  
-      :avatar      => 'binarydata',
13  
-      :created_at  => Time.utc(2006, 8, 1),
14  
-      :awesome     => false,
15  
-      :preferences => { :gem => '<strong>ruby</strong>' }
  10
+      :name           => 'aaron stack',
  11
+      :age            => 25,
  12
+      :avatar         => 'binarydata',
  13
+      :created_at     => Time.utc(2006, 8, 1),
  14
+      :awesome        => false,
  15
+      :preferences    => { :gem => '<strong>ruby</strong>' },
  16
+      :alternative_id => nil
16 17
     }
17 18
   end
18 19
 
13  activerecord/test/models/contact.rb
@@ -11,12 +11,13 @@ def self.column(name, sql_type = nil, options = {})
11 11
     connection.merge_column('contacts', name, sql_type, options)
12 12
   end
13 13
 
14  
-  column :name,        :string
15  
-  column :age,         :integer
16  
-  column :avatar,      :binary
17  
-  column :created_at,  :datetime
18  
-  column :awesome,     :boolean
19  
-  column :preferences, :string
  14
+  column :name,           :string
  15
+  column :age,            :integer
  16
+  column :avatar,         :binary
  17
+  column :created_at,     :datetime
  18
+  column :awesome,        :boolean
  19
+  column :preferences,    :string
  20
+  column :alternative_id, :integer
20 21
 
21 22
   serialize :preferences
22 23
 
8  activerecord/test/schema/schema.rb
@@ -47,6 +47,7 @@ def create_table(*args, &block)
47 47
   create_table :audit_logs, :force => true do |t|
48 48
     t.column :message, :string, :null=>false
49 49
     t.column :developer_id, :integer, :null=>false
  50
+    t.integer :unvalidated_developer_id
50 51
   end
51 52
 
52 53
   create_table :authors, :force => true do |t|
@@ -156,6 +157,7 @@ def create_table(*args, &block)
156 157
     t.string  :type
157 158
     t.integer :taggings_count, :default => 0
158 159
     t.integer :children_count, :default => 0
  160
+    t.integer :parent_id
159 161
   end
160 162
 
161 163
   create_table :companies, :force => true do |t|
@@ -461,6 +463,7 @@ def create_table(*args, &block)
461 463
   create_table :pirates, :force => true do |t|
462 464
     t.column :catchphrase, :string
463 465
     t.column :parrot_id, :integer
  466
+    t.integer :non_validated_parrot_id
464 467
     t.column :created_on, :datetime
465 468
     t.column :updated_on, :datetime
466 469
   end
@@ -529,6 +532,7 @@ def create_table(*args, &block)
529 532
   create_table :ships, :force => true do |t|
530 533
     t.string :name
531 534
     t.integer :pirate_id
  535
+    t.integer :update_only_pirate_id
532 536
     t.datetime :created_at
533 537
     t.datetime :created_on
534 538
     t.datetime :updated_at
@@ -663,7 +667,9 @@ def create_table(*args, &block)
663 667
     t.string  :description
664 668
     t.integer :man_id
665 669
     t.integer :polymorphic_man_id
666  
-    t.string :polymorphic_man_type
  670
+    t.string  :polymorphic_man_type
  671
+    t.integer :horrible_polymorphic_man_id
  672
+    t.string  :horrible_polymorphic_man_type
667 673
   end
668 674
 
669 675
   create_table :interests, :force => true do |t|

2 notes on commit 50d395f

Jeffrey Lee

This doesn't appear to allow for virtual attributes. I've got an app that is using a sortOrder column that is based on the current user. I could write some very complex queries to try an manipulate things with joins to the sortOrder table, but the virtual attribute is the perfect solution. In particular, I'm trying to overcome the bug that causes activerecord association objects to be returned as arrays. In that circumstance, I definitely need to use a virtual attribute.

Carlos Antonio da Silva

You should be able to use a virtual attribute normally without using write_attribute on it, but saving as an instance variable instead.

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