Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

#becomes works with configured inheritance_column #7506

Merged
merged 1 commit into from

6 participants

@senny
Owner

This is a patch for #7503

I had to create a new table and a model since the current STI models all relied on type and it was important for this issue not to have a field named type

@morgoth

Somehow related to #3023

@rafaelfranca
Owner

I would not create a new model. I think you can use the Company model that already has the ruby_type column.

activerecord/lib/active_record/persistence.rb
@@ -161,7 +161,7 @@ def becomes(klass)
became.instance_variable_set("@new_record", new_record?)
became.instance_variable_set("@destroyed", destroyed?)
became.instance_variable_set("@errors", errors)
- became.type = klass.name unless self.class.descends_from_active_record?
+ became.send("#{klass.inheritance_column}=", klass.name) unless self.class.descends_from_active_record?

Please use public_send instead, we shouldn't bypass private methods here.

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

+1 to not create a new model if possible, thanks!

@senny
Owner

@carlosantoniodasilva changed send to public_send.
@rafaelfranca Of course I understand that adding new models should be prevented whenever possible. The problem is, the current Company model has both a type and a ruby_type field. The tests then switch the inheritance_column but for both configurations the same model is used. In my opinion this leads to flawed tests and false positives. My test-case passed with the Company model because it had an accessor called type. To make sure that inerhitance_column works as expected I think we need different models. What do you think?

@pixeltrix
Owner

@senny can't you create a new class using the same table and either remove the method or mark it private?

@senny
Owner

I guess hacks are possible but In my opinion the tests should reflect a production situation as close as possible. Patching away certain features seems odd to me. I see that the model currently is a lot of code for this small bug but if it gets merged I'll rewrite the alternate tests to use the new model instead so we can make sure it actually works.

Of course If you like to have the workarounds in the testing code, I'll drop the models and get it working somehow. Someone with more internal rails knowledge should give me a hint what direction to take.

activerecord/test/models/vegetables.rb
@@ -0,0 +1,16 @@
+class Vegetable
+
+ include ActiveRecord::Model
@rafaelfranca Owner

Could you inherit from ActiveRecord::Base?

@senny Owner
senny added a note

sure, I'll do it in a second. Is there a specific reason to prefere inheritance? I've seen that there are models of both kinds.

@rafaelfranca Owner

ActiveRecord::Base is still the official way to create Active Record models. I prefer to use it in our tests.

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

I'm fine with the new model and I think it is better to use it in the tests that needs to change the inherited column in a future pull request.

@senny
Owner

@rafaelfranca thanks for the feedback. I make the changes you pointed out and rework the existing tests to use the new model afterwards.

@senny
Owner

@rafaelfranca ok, pushed a new version.

@rafaelfranca
Owner

Could you squash the commits. It is easier to see the reason for the change when the commit has the tests.

@senny senny set the configured #inheritance_column on #become (#7503)
I had to create a new table because I needed an STI table,
which does not have both a "type" and a "custom_type"

the test fails with:
  1) Error:
test_alt_becomes_works_with_sti(InheritanceTest):
NoMethodError: undefined method `type=' for #<Cabbage id: 1, name: "my cucumber", custom_type: "Cucumber">
    /Users/username/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:432:in `method_missing'
    /Users/username/Projects/rails/activerecord/lib/active_record/attribute_methods.rb:100:in `method_missing'
    /Users/username/Projects/rails/activerecord/lib/active_record/persistence.rb:165:in `becomes'
    test/cases/inheritance_test.rb:134:in `test_becomes_works_with_sti'
    test/cases/inheritance_test.rb:140:in `test_alt_becomes_works_with_sti'
2057495
@senny
Owner
@rafaelfranca rafaelfranca merged commit 30a8f0d into from
@rafaelfranca
Owner

Thanks

@rafaelfranca
Owner

@senny we will need a pull request pointing 3-2-stable branch. But we cant use public_send since it doesn't work with ruby 1.8

@rafaelfranca
Owner

Also please add a CHANGELOG entry

@kayakyakr

This should probably reference the method "ensure_proper_type" from ActiveRecord::Inheritance instead of changing the value itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 3, 2012
  1. @senny

    set the configured #inheritance_column on #become (#7503)

    senny authored senny committed
    I had to create a new table because I needed an STI table,
    which does not have both a "type" and a "custom_type"
    
    the test fails with:
      1) Error:
    test_alt_becomes_works_with_sti(InheritanceTest):
    NoMethodError: undefined method `type=' for #<Cabbage id: 1, name: "my cucumber", custom_type: "Cucumber">
        /Users/username/Projects/rails/activemodel/lib/active_model/attribute_methods.rb:432:in `method_missing'
        /Users/username/Projects/rails/activerecord/lib/active_record/attribute_methods.rb:100:in `method_missing'
        /Users/username/Projects/rails/activerecord/lib/active_record/persistence.rb:165:in `becomes'
        test/cases/inheritance_test.rb:134:in `test_becomes_works_with_sti'
        test/cases/inheritance_test.rb:140:in `test_alt_becomes_works_with_sti'
This page is out of date. Refresh to see the latest.
View
2  activerecord/lib/active_record/persistence.rb
@@ -161,7 +161,7 @@ def becomes(klass)
became.instance_variable_set("@new_record", new_record?)
became.instance_variable_set("@destroyed", destroyed?)
became.instance_variable_set("@errors", errors)
- became.type = klass.name unless self.class.descends_from_active_record?
+ became.public_send("#{klass.inheritance_column}=", klass.name) unless self.class.descends_from_active_record?
became
end
View
10 activerecord/test/cases/inheritance_test.rb
@@ -5,9 +5,10 @@
require 'models/project'
require 'models/subscriber'
require 'models/teapot'
+require 'models/vegetables'
class InheritanceTest < ActiveRecord::TestCase
- fixtures :companies, :projects, :subscribers, :accounts
+ fixtures :companies, :projects, :subscribers, :accounts, :vegetables
def test_class_with_store_full_sti_class_returns_full_name
old = ActiveRecord::Base.store_full_sti_class
@@ -127,6 +128,13 @@ def test_alt_inheritance_find
switch_to_default_inheritance_column
end
+ def test_alt_becomes_works_with_sti
+ vegetable = Vegetable.find(1)
+ assert_kind_of Vegetable, vegetable
+ cabbage = vegetable.becomes(Cabbage)
+ assert_kind_of Cabbage, cabbage
+ end
+
def test_inheritance_find_all
companies = Company.all.merge!(:order => 'id').to_a
assert_kind_of Firm, companies[0], "37signals should be a firm"
View
9 activerecord/test/fixtures/vegetables.yml
@@ -0,0 +1,9 @@
+first_cucumber:
+ id: 1
+ custom_type: Cucumber
+ name: 'my cucumber'
+
+first_cabbage:
+ id: 2
+ custom_type: Cabbage
+ name: 'my cabbage'
View
14 activerecord/test/models/vegetables.rb
@@ -0,0 +1,14 @@
+class Vegetable < ActiveRecord::Base
+
+ validates_presence_of :name
+
+ def self.inheritance_column
+ 'custom_type'
+ end
+end
+
+class Cucumber < Vegetable
+end
+
+class Cabbage < Vegetable
+end
View
5 activerecord/test/schema/schema.rb
@@ -184,6 +184,11 @@ def create_table(*args, &block)
add_index :companies, [:firm_id, :type, :rating, :ruby_type], :name => "company_index"
add_index :companies, [:firm_id, :type], :name => "company_partial_index", :where => "rating > 10"
+ create_table :vegetables, :force => true do |t|
+ t.string :name
+ t.string :custom_type
+ end
+
create_table :computers, :force => true do |t|
t.integer :developer, :null => false
t.integer :extendedWarranty, :null => false
Something went wrong with that request. Please try again.