Active Record's subclass_from_attributes shouldn't assume :type is for STI unless there is a type column. #13710

Merged
merged 1 commit into from Jan 14, 2014

Conversation

Projects
None yet
3 participants
Contributor

ujjwalt commented Jan 14, 2014

Active Record's subclass_from_attrs assumes type is for STI whether or not type exists as a column or not. This fixes this bug allowing you to use type as a reference in non-STI classes.

# Activate the gem you are reporting the issue against.
gem 'activerecord', '4.0.2'
require 'active_record'
require 'minitest/autorun'
require 'logger'

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :products do |t|
    t.references :type
    t.string :name
  end

  add_index :products, :type_id

  create_table :product_types do |t|
    t.string :name
  end

  add_index :product_types, :name, unique: true
end

class Product < ActiveRecord::Base
  class Type < ActiveRecord::Base
    has_many :products
  end

  belongs_to :type
end

class BugTest < Minitest::Test
  def test_sti_type_from_attributes_disabled_in_non_sti_class
    phone = Product::Type.new(name: 'Phone')
    product = Product.new(:type => phone)
    assert product.save
  end
end

It seems you have way too many unrelated commits, mind taking a look and properly rebasing from master? Thanks.

Contributor

ujjwalt commented Jan 14, 2014

Ya I just realized that I messed up. Fixing that right now.

On 14 January 2014 15:46, Carlos Antonio da Silva
notifications@github.comwrote:

It seems you have way too many unrelated commits, mind taking a look and
properly rebasing from master? Thanks.


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/13710#issuecomment-32252640
.

Thanks
Ujjwal

Contributor

ujjwalt commented Jan 14, 2014

Can't rebase from master unfortuantely. Got that messed up for my Google Summer of Code. It's taught me to never reopen a PR from master again.

activerecord/test/schema/schema.rb
@@ -806,6 +806,19 @@ def create_table(*args, &block)
t.integer :department_id
end
+ create_table :products do |t|
@pixeltrix

pixeltrix Jan 14, 2014

Owner

There's already a products table - can you make the changes there, thanks.

@ujjwalt

ujjwalt Jan 14, 2014

Contributor

My apologies.

activerecord/test/models/product.rb
@@ -0,0 +1,7 @@
+class Product < ActiveRecord::Base
@pixeltrix

pixeltrix Jan 14, 2014

Owner

There's already a Product class inside the Shop module - can you add the belongs_to to that model and put Product::Type under Shop::Product, thanks.

@ujjwalt

ujjwalt Jan 14, 2014

Contributor

Done.

Owner

pixeltrix commented Jan 14, 2014

Can you squash your commits, thanks!

Contributor

ujjwalt commented Jan 14, 2014

Ok.

activerecord/test/schema/schema.rb
@@ -806,6 +807,14 @@ def create_table(*args, &block)
t.integer :department_id
end
+ add_index :products, :type_id
+
+ create_table :product_types do |t|
@pixeltrix

pixeltrix Jan 14, 2014

Owner

Can you move this table definition so that it's next to the :products one and you can drop the index definitions - they aren't needed for the test to fail.

@ujjwalt

ujjwalt Jan 14, 2014

Contributor

I saw that and moved it while squashing. Do you still need me to remove it? If so I'll do that, squash again and force update.

activerecord/lib/active_record/inheritance.rb
@@ -172,6 +176,10 @@ def type_condition(table = arel_table)
# is not self or a valid subclass, raises ActiveRecord::SubclassNotFound
# If this is a StrongParameters hash, and access to inheritance_column is not permitted,
# this will ignore the inheritance column and return nil
+ def subclass_from_attributes?(attrs)
+ columns_hash.includes?(inheritance_column) && attrs.is_a?(Hash)
@pixeltrix

pixeltrix Jan 14, 2014

Owner

Typo here - it should be include?

Owner

pixeltrix commented Jan 14, 2014

Can you also amend the commit message with a bit more of an explanation:

Don't try to get the subclass if the inheritance column doesn't exist

The `subclass_from_attrs` method is called even if the column specified by
the `inheritance_column` setting doesn't exist. This prevents setting associations
via the attributes hash if the association name clashes with the value of the setting,
typically `:type`. This worked previously in Rails 3.2.
Owner

pixeltrix commented Jan 14, 2014

Also update the CHANGELOG with the expanded explanation as well, thanks.

Contributor

ujjwalt commented Jan 14, 2014

Updated the explanation and CHANGELOG

Don't try to get the subclass if the inheritance column doesn't exist
The `subclass_from_attrs` method is called even if the column specified by
the `inheritance_column` setting doesn't exist. This prevents setting associations
via the attributes hash if the association name clashes with the value of the setting,
typically `:type`. This worked previously in Rails 3.2.

pixeltrix added a commit that referenced this pull request Jan 14, 2014

Merge pull request #13710 from ujjwalt/hotfix/sti
Active Record's subclass_from_attributes shouldn't assume :type is for STI unless there is a type column.

@pixeltrix pixeltrix merged commit 8a60f47 into rails:master Jan 14, 2014

1 check passed

default The Travis CI build passed
Details

@ujjwalt ujjwalt deleted the ujjwalt:hotfix/sti branch Jan 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment