Skip to content
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 on resources: always use base class when saving roles on resources #517

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

doits
Copy link

@doits doits commented Jun 5, 2019

I had the problem fetching and retrieving roles by passing a resource which was actually a STI sub class:

class User
  rolify
end

class House
end

class BigHouse < House
  resourcify
end

user = User.create
big_house = BigHouse.create

user.add_role :owner, big_house
# Created a role with { resource_type: 'BigHouse' }

Role.where(resource: big_house)
# Actually searched for { resource_type: 'House' }
# => []

With this PR it should always call base_class on the classes, so it always uses the base_class to fetch and save records, which is in accordance to Rails' docs:

Using polymorphic associations in combination with single table inheritance (STI) is a little tricky. In order for the associations to work as expected, ensure that you store the base model for the STI models in the type column of the polymorphic association.


ToDo:

  • Write a spec for this
  • Think about migration path of existing databases (migrate resource_type?)

@doits doits changed the title always use base class when saving roles on resources STI on resources: always use base class when saving roles on resources Jun 5, 2019
@doits doits force-pushed the sti branch 3 times, most recently from b5b9126 to 4a43f60 Compare June 5, 2019 18:12
@doits doits marked this pull request as ready for review June 5, 2019 19:58
@coveralls
Copy link

coveralls commented Mar 12, 2020

Coverage Status

Coverage increased (+0.007%) to 93.267% when pulling f831526 on doits:sti into 7c144ad on RolifyCommunity:master.

@phlegx
Copy link

phlegx commented Aug 4, 2020

In my opinion, this should be an optional. A configuration where one can set if using base class or not for STI.

@doits
Copy link
Author

doits commented Nov 29, 2020

In my opinion, this should be an optional. A configuration where one can set if using base class or not for STI.

Do you have a special use case in mind where this is needed or not solvable by other means?

Otherwise I'd say following what Rails suggests and expects for polymorphic associations prevents other surprises (emphasis by me):

Using polymorphic associations in combination with single table inheritance (STI) is a little tricky. In order for the associations to work as expected, ensure that you store the base model for the STI models in the type column of the polymorphic association.

@thijsnado
Copy link

I agree with @doits on this one. The default for rails is to save the polymorhpic name when doing something like:

Role.create(resource: a_resource_with_sti)

Since the type of a resource can change in rails, saving the base class is safer. I think the only argument for making this optional instead of default is for backward compatibility for people already using rolify.

@@ -57,7 +57,7 @@ def add(relation, role)

def remove(relation, role_name, resource = nil)
cond = { :name => role_name }
cond[:resource_type] = (resource.is_a?(Class) ? resource.to_s : resource.class.name) if resource
cond[:resource_type] = (resource.is_a?(Class) ? Rolify.base_class_for(resource).to_s : Rolify.base_class_for(resource.class).name) if resource

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't write original code, but it is confusing that we do SomeClass.to_s and some_instance.class.name. I figure we should be consistent and do SomeClass.name and some_instance.class.name. 99% of the time to_s and name act the same but I could imagine someone overriding to_s on an AR class and having this break. I also think if you get rid of Rolify.base_class_for this could be simplified with:

(resource.is_a?(Class) ? resource.polymorphic_name : resource.class.polymorphic_name) if resource

https://api.rubyonrails.org/classes/ActiveRecord/Inheritance/ClassMethods.html#method-i-polymorphic_name documents this method. It is what should be saved in resource_type without having to find the base class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants