Skip to content

Commit

Permalink
Move writing unknown column exception to null attribute
Browse files Browse the repository at this point in the history
Making this change revealed several subtle bugs related to models with
no primary key, and anonymous classes. These have been fixed as well,
with regression tests added.
  • Loading branch information
sgrif committed Jun 26, 2014
1 parent 031588e commit 3bc314e
Show file tree
Hide file tree
Showing 9 changed files with 34 additions and 13 deletions.
5 changes: 5 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
* Fix subtle bugs regarding attribute assignment on models with no primary
key. `'id'` will no longer be part of the attributes hash.

*Sean Griffin*

* Deprecate automatic counter caches on `has_many :through`. The behavior was
broken and inconsistent.

Expand Down
5 changes: 5 additions & 0 deletions activerecord/lib/active_record/attribute.rb
Expand Up @@ -90,6 +90,11 @@ def initialize(name)
def value
nil
end

This comment has been minimized.

Copy link
@nishu

nishu Jan 13, 2015

@sgrif Should an exception be thrown even if attribute is defined as attr_accessor in model. Compared to previous behaviour?

unless has_attribute?(attr_name) || self.class.columns_hash.key?(attr_name)

def with_value_from_database(value)
raise ActiveModel::MissingAttributeError, "can't write unknown attribute `#{name}`"
end
alias_method :with_value_from_user, :with_value_from_database
end

class Uninitialized < Attribute # :nodoc:
Expand Down
Expand Up @@ -83,12 +83,9 @@ def reset_primary_key #:nodoc:
end

def get_primary_key(base_name) #:nodoc:
return 'id' if base_name.blank?

case primary_key_prefix_type
when :table_name
if base_name && primary_key_prefix_type == :table_name
base_name.foreign_key(false)
when :table_name_with_underscore
elsif base_name && primary_key_prefix_type == :table_name_with_underscore
base_name.foreign_key
else
if ActiveRecord::Base != self && table_exists?
Expand Down
4 changes: 0 additions & 4 deletions activerecord/lib/active_record/attribute_methods/write.rb
Expand Up @@ -70,10 +70,6 @@ def write_attribute_with_type_cast(attr_name, value, should_type_cast)
attr_name = attr_name.to_s
attr_name = self.class.primary_key if attr_name == 'id' && self.class.primary_key

unless has_attribute?(attr_name) || self.class.columns_hash.key?(attr_name)
raise ActiveModel::MissingAttributeError, "can't write unknown attribute `#{attr_name}'"
end

if should_type_cast
@attributes.write_from_user(attr_name, value)
else
Expand Down
Expand Up @@ -381,7 +381,7 @@ def tables(name = nil, database = nil, like = nil) #:nodoc:
end

def table_exists?(name)
return false unless name
return false unless name.present?
return true if tables(nil, nil, name).any?

name = name.to_s
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/core.rb
Expand Up @@ -521,7 +521,7 @@ def to_ary # :nodoc:

def init_internals
pk = self.class.primary_key
unless @attributes.include?(pk)
if pk && !@attributes.include?(pk)
@attributes.write_from_database(pk, nil)
end

Expand Down
9 changes: 9 additions & 0 deletions activerecord/test/cases/attribute_methods_test.rb
Expand Up @@ -253,6 +253,15 @@ def test_case_sensitive_attributes_hash
assert_equal @loaded_fixtures['computers']['workstation'].to_hash, Computer.first.attributes
end

def test_attributes_without_primary_key
klass = Class.new(ActiveRecord::Base) do
self.table_name = 'developers_projects'
end

assert_equal klass.column_names, klass.new.attributes.keys
assert_not klass.new.attributes.key?('id')
end

def test_hashes_not_mangled
new_topic = { :title => "New Topic" }
new_topic_values = { :title => "AnotherTopic" }
Expand Down
12 changes: 10 additions & 2 deletions activerecord/test/cases/primary_keys_test.rb
Expand Up @@ -134,14 +134,22 @@ def test_supports_primary_key
end

def test_primary_key_returns_value_if_it_exists
klass = Class.new(ActiveRecord::Base) do
self.table_name = 'developers'
end

if ActiveRecord::Base.connection.supports_primary_key?
assert_equal 'id', ActiveRecord::Base.connection.primary_key('developers')
assert_equal 'id', klass.primary_key
end
end

def test_primary_key_returns_nil_if_it_does_not_exist
klass = Class.new(ActiveRecord::Base) do
self.table_name = 'developers_projects'
end

if ActiveRecord::Base.connection.supports_primary_key?
assert_nil ActiveRecord::Base.connection.primary_key('developers_projects')
assert_nil klass.primary_key
end
end

Expand Down
1 change: 1 addition & 0 deletions activerecord/test/models/contact.rb
Expand Up @@ -8,6 +8,7 @@ def self.extended(base)
table_name => 'id'
}

column :id, :integer
column :name, :string
column :age, :integer
column :avatar, :binary
Expand Down

0 comments on commit 3bc314e

Please sign in to comment.