STI on resources: always use base class when saving roles on resources

[doits]

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?)

[coveralls]

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

[phlegx]

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

[doits]

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]

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.

[thijsnado]

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.

[doits]

Thank for you fedback @thijsnado! I finally came back to this PR and adapted it to use polymorphic_name, definately simpler like this!

[bopm]

What do you know, for the first time in a long time, I attacked this problem without paying attention to open PRs, and now in exactly the same timeframe as you commenting on that @doits, I offered my PR on the same thing.

[doits]

To have it in the discussion here, too: I just updated it to be more simpler by using #polymorphic_name (thanks @thijsnado).

Not sure abouth the migration path for existing users though, who have saved old values in the database already 🤔