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

Polymorphic associations do not support computing the class #793

Closed
chubchenko opened this issue Jun 29, 2022 · 12 comments
Closed

Polymorphic associations do not support computing the class #793

chubchenko opened this issue Jun 29, 2022 · 12 comments

Comments

@chubchenko
Copy link

Steps to reproduce

https://gist.github.com/chubchenko/883eb5a7c49b194a223be3843774670f

Expected behavior

Does not raise an exception ArgumentError: Polymorphic associations do not support computing the class

Actual behavior

Raises an exception:

Error:
CommentsControllerTest#test_action:
ArgumentError: Polymorphic associations do not support computing the class.
    activerecord-7.0.3/lib/active_record/reflection.rb:423:in `compute_class'
    activerecord-7.0.3/lib/active_record/reflection.rb:382:in `klass'
    cancancan-3.4.0/lib/cancan/model_adapters/active_record_adapter.rb:42:in `block in parent_child_conditions'
    cancancan-3.4.0/lib/cancan/model_adapters/active_record_adapter.rb:41:in `each'
    cancancan-3.4.0/lib/cancan/model_adapters/active_record_adapter.rb:41:in `find'
    cancancan-3.4.0/lib/cancan/model_adapters/active_record_adapter.rb:41:in `parent_child_conditions'
    cancancan-3.4.0/lib/cancan/model_adapters/active_record_adapter.rb:27:in `override_nested_subject_conditions_matching?'
    cancancan-3.4.0/lib/cancan/conditions_matcher.rb:44:in `nested_subject_matches_conditions?'
    cancancan-3.4.0/lib/cancan/conditions_matcher.rb:28:in `matches_non_block_conditions'
...

System configuration

Rails version: 7.0.3

Ruby version: 3.1.2

CanCanCan version 3.4.0

@chubchenko
Copy link
Author

The error occurs in this line in the #parent_child_conditions method during iteration though reflect_on_all_associations(:belongs_to).

If change the order of the association definition from:

class Comment < ActiveRecord::Base
  belongs_to :commentable, polymorphic: true
  belongs_to :user
end

to:

class Comment < ActiveRecord::Base
  belongs_to :user
  belongs_to :commentable, polymorphic: true
end

the issue will be resolved but it's just a temporary life hack 😄

@gryphon
Copy link

gryphon commented Jul 18, 2022

This will only fix the issue if you check user attribute in your case.
But if you check Comment model field (can :read, Comment, private: true), it will fail in any case

@javinto
Copy link

javinto commented Jul 28, 2022

I got this error with the devise_invitable gem which adds the belongs_to :invited_by, polymorphic: true association in Rails 6.1. Downgrading to Cancancan 3.3.0 solved it for me.

@mobilutz
Copy link

mobilutz commented Aug 8, 2022

The issue seems to be caused by this change:
d0ea89a#diff-468c7fa6110fedf2a4ae5259ca4d209daea99810746321092dac9cf373b87fdd

@martijn10kb
Copy link

I have the same issue, downgraded for now to 3.3.0

@bowtiesolutions
Copy link

bowtiesolutions commented Nov 3, 2022

The issue seems to be caused by this change: d0ea89a#diff-468c7fa6110fedf2a4ae5259ca4d209daea99810746321092dac9cf373b87fdd

Can confirm, the new offending behaviour is:

def parent_child_conditions(parent, child, all_conditions)
  child_class = child.is_a?(Class) ? child : child.class
  foreign_key = child_class.reflect_on_all_associations(:belongs_to).find do |association|
                  association.klass == parent.class
                end&.foreign_key&.to_sym
  foreign_key.nil? ? nil : all_conditions[foreign_key]
end

For ActiveRecord::Reflection::BelongsToReflection, 'klass' cannot be calculated for polymorphic associations so if one is utilised on the child class, the finder will error. As 'find' halts on the first instance, the temporary fix above to reoder polymorphic associations solves the issue for some.

Suggestion would be to add 'unless association.polymorphic?' to the finder to ensure polymorphic associations are ignored.

@WriterZephos
Copy link
Contributor

Any luck with this? I am currently facing this issue and not finding a good work around so far.

@javinto
Copy link

javinto commented Jan 24, 2023

Unfortunately I did not implement any Work around except for downgrading to version 3.3.0.

However, with a little experimenting I found some workaround for my situation, although not ideal.

My original Ability code was:

can [:read, :update, :destroy], User, account_id: current_user.account_id

So, this one triggers the above exception with version 3.4.0.

I can rewrite it to

can [:read, :update, :destroy], User do |user|
  user.account_id==current_user.account_id
end

Now the exception is gone away and the right authorization is applied to the individual records BUT there is one drawback:

now the :read does not apply to the listing anymore. In other words: @users gives me all users and not just that of the account they belong to. So, I should apply an extra filter in my controller to make it work. But that could be acceptable.

Hopes this helps for your situation.

@caseyli
Copy link

caseyli commented Jan 24, 2023

I ran into this same issue and I think I can explain why changing the order of the association works, but I don't know what the long term fix is.

The issue seems to stem from lib/cancan/model_adapters/active_record_adapter.rb#parent_child_conditions. Look at this snippet of code:

def parent_child_conditions(parent, child, all_conditions)
  ...
  foreign_key = child_class.reflect_on_all_associations(:belongs_to).find do |association|
    association.klass == parent.class
  end&.foreign_key&.to_sym
  ...
end

When you do something like load_and_authorize_resource through: :current_user, it eventually ends up calling this method. This method loops through all your belongs_to associations on the resource you're trying to use.

On each of the belongs_to associations, it calls association.klass and sees if it matches the parent class you're trying to load through.

The problem is this rails/rails@fb86ecd doesn't allow you to call association.klass on polymorphic associations. So when it hits that association, the app throws an exception.

The reason the code will work if you re-arrange your associations, is that if the association you're dealing with happens to be near the top and matches BEFORE it hits a polymorphic association, the loop stops. I'm assuming .find just stops after it's found a match. So you never hit the error of calling .klass on a polymorphic association.

So re-arranging your associations may work - but it's quite brittle and won't work in a lot of cases.

Long term fix would be to update this loop code to handle polymorphic associations.

@WriterZephos
Copy link
Contributor

WriterZephos commented Jan 31, 2023

@caseyli please see my PR linked above. It does just as you suggest and adds support for polymorphic associations.

@WriterZephos
Copy link
Contributor

Here it is for convenience: #814

@coorasse
Copy link
Member

coorasse commented Mar 5, 2023

Closed on 3.5.0. Thank you!

@coorasse coorasse closed this as completed Mar 5, 2023
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

No branches or pull requests

9 participants