New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STI cast new instances to `default type` on initialize. #17169

Merged
merged 1 commit into from Dec 2, 2015

Conversation

Projects
None yet
6 participants
@kuldeepaggarwal
Contributor

kuldeepaggarwal commented Oct 3, 2014

fixes #17121

@kuldeepaggarwal

This comment has been minimized.

Contributor

kuldeepaggarwal commented Jan 3, 2015

any update?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jan 3, 2015

Please don't ping in the issue tracker. It will be reviewed when someone have time and want to review.

@kuldeepaggarwal

This comment has been minimized.

Contributor

kuldeepaggarwal commented Jan 3, 2015

Apology for creating noise in the issue tracker but its been 3 months. So, it was just a gentle reminder. I will take care of it.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jan 3, 2015

No problem.

Yeah, we are a lot behind the PRs schedule 😢. I'm trying to catch up, so expect this PR to being reviewed 🔜 😄

@kuldeepaggarwal

This comment has been minimized.

Contributor

kuldeepaggarwal commented Jan 3, 2015

Thanks. 👍

@ryanrperez

This comment has been minimized.

Contributor

ryanrperez commented Feb 17, 2015

I can verify that the added tests are correct and this is a fix for the issue.

@kuldeepaggarwal kuldeepaggarwal force-pushed the kuldeepaggarwal:fix-STI-default-type branch Mar 25, 2015

@jormon

This comment has been minimized.

jormon commented May 14, 2015

Workaround: in the class at the top of the inheritance hierarchy:

after_initialize do
    self.type = "Company" if self.class == Company
end

(for those too lazy to run off a patch) :/

@senny

This comment has been minimized.

Member

senny commented Sep 10, 2015

@kuldeepaggarwal can we try to make a copy of attrs with inheritance_column => inheritance_column_default if inheritance_column is not already in attrs. We could possible do that here: https://github.com/kuldeepaggarwal/rails/blob/fix-STI-default-type/activerecord/lib/active_record/inheritance.rb#L53 and then subclass_from_attributes? and subclass_from_attributes can be left as is.

@kuldeepaggarwal

This comment has been minimized.

Contributor

kuldeepaggarwal commented Sep 10, 2015

@senny Sure. will do it asap.

Thanks for the hint.

@kuldeepaggarwal kuldeepaggarwal force-pushed the kuldeepaggarwal:fix-STI-default-type branch to e4e9dc6 Sep 10, 2015

@kuldeepaggarwal

This comment has been minimized.

Contributor

kuldeepaggarwal commented Sep 10, 2015

Updated the PR.

if subclass_from_attributes?(attrs)
attrs = attrs.with_indifferent_access
attrs[inheritance_column] ||= columns_hash[inheritance_column].default

This comment has been minimized.

@senny

senny Sep 11, 2015

Member

@kuldeepaggarwal shouldn't this happen before if subclass_from_attributes?(attrs)? Otherwise subclass_from_attributes?(attrs) will not see the default.

This comment has been minimized.

@kuldeepaggarwal

kuldeepaggarwal Sep 11, 2015

Contributor

We are not actually seeing the defaults in subclass_from_attributes?(attrs),

def subclass_from_attributes?(attrs)
  attribute_names.include?(inheritance_column) && attrs.is_a?(Hash)
end

This comment has been minimized.

@senny

senny Sep 11, 2015

Member

👍 sorry, my bad. Misread attribute_names as attrs

@senny senny self-assigned this Sep 30, 2015

@senny

This comment has been minimized.

Member

senny commented Nov 30, 2015

@kuldeepaggarwal sorry for the long delay 😓. Finally got around to cleaning up my inbox. This doesn't apply cleanly on master. Can you submit a rebased version?

@kuldeepaggarwal

This comment has been minimized.

Contributor

kuldeepaggarwal commented Nov 30, 2015

@senny: Sure, give me sometime.

@kuldeepaggarwal kuldeepaggarwal force-pushed the kuldeepaggarwal:fix-STI-default-type branch from e4e9dc6 Nov 30, 2015

@kuldeepaggarwal

This comment has been minimized.

Contributor

kuldeepaggarwal commented Nov 30, 2015

@senny Updated the PR.. 😄

@senny

View changes

activerecord/lib/active_record/inheritance.rb Outdated
end
def subclass_from_attributes(attrs)
attrs = attrs.to_h if attrs.respond_to?(:permitted?)
subclass_name = attrs.with_indifferent_access[inheritance_column]
subclass_name = (attrs || {}).with_indifferent_access[inheritance_column] || columns_hash[inheritance_column].default

This comment has been minimized.

@senny

senny Nov 30, 2015

Member

@kuldeepaggarwal do we still need that ||. Previous code was calling with_indifferent_access directly, so that should never be nil, no?

This comment has been minimized.

@kuldeepaggarwal

kuldeepaggarwal Nov 30, 2015

Contributor

No, actually in my previous commit I was initializing attrs ||= {} so that's why I called with_indifferent_access directly, which is equivalent to current implementation.

This comment has been minimized.

@senny

senny Nov 30, 2015

Member

ah, I see. It's because we modify subclass_from_attributes? in a weird way. Why not keep subclass_from_attributes? as is but modify this part:

def new

        if subclass_from_attributes?(attrs)
          subclass = subclass_from_attributes(attrs)
        else
          subclass = columns_hash[inheritance_column].default
        end

This way the methods still do what their name suggests and it should be obvious at a glance how the subclass is determined.

This comment has been minimized.

@senny

senny Nov 30, 2015

Member

We'd still have to do the find_sti_class(subclass_name) though.

This comment has been minimized.

@kuldeepaggarwal

kuldeepaggarwal Dec 2, 2015

Contributor

Modified the implementation.

@akshay-vishnoi

View changes

activerecord/test/cases/inheritance_test.rb Outdated
@@ -478,4 +478,24 @@ def test_sti_type_from_attributes_disabled_in_non_sti_class
product = Shop::Product.new(:type => phone)
assert product.save
end
def test_inheritance_new_with_subcalss_as_default

This comment has been minimized.

@akshay-vishnoi

akshay-vishnoi Dec 2, 2015

Contributor

subcalss --> subclass.

This comment has been minimized.

@kuldeepaggarwal

kuldeepaggarwal Dec 2, 2015

Contributor

Nice catch!! will rectify in next push.

@kuldeepaggarwal kuldeepaggarwal force-pushed the kuldeepaggarwal:fix-STI-default-type branch 2 times, most recently Dec 2, 2015

@kuldeepaggarwal kuldeepaggarwal force-pushed the kuldeepaggarwal:fix-STI-default-type branch to 6b18bdd Dec 2, 2015

@senny senny merged commit 6b18bdd into rails:master Dec 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

senny added a commit that referenced this pull request Dec 2, 2015

Merge pull request #17169 from kuldeepaggarwal/fix-STI-default-type
STI cast new instances to `default type` on initialize.
@senny

This comment has been minimized.

Member

senny commented Dec 2, 2015

@kuldeepaggarwal thank you 💛

I made some modifications in the merge commit so that we can get rid of some duplicate conditionals.

@kuldeepaggarwal

This comment has been minimized.

Contributor

kuldeepaggarwal commented Dec 2, 2015

Awesome..

@kuldeepaggarwal kuldeepaggarwal deleted the kuldeepaggarwal:fix-STI-default-type branch Dec 2, 2015

senny added a commit that referenced this pull request Dec 2, 2015

don't rely on the columns hash to get defaults. follow-up to #17169.
This will also get the defaults from attribute definitions like:

     attribute :type, :string, default: "SomethingElse"

leonelgalan added a commit to leonelgalan/rails that referenced this pull request Jan 22, 2018

Ignores a default subclass when `becomes(Parent)`
Fixes issue described in rails#30399: A default value on the
inheritance column prevented `child.becomes(Parent)` to return
an instance of `Parent` as expected, instead it returns an instance
of the default subclass.

The change was introduced by rails#17169 and it was meant to affect
initialization, alone. Where `Parent.new` is expected to return
an instance of the default subclass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment