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

Use relation.and instead of relation.merge whenever possible. #695

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

albb0920
Copy link

@albb0920 albb0920 commented Feb 24, 2021

Currently cancancan uses relation.merge to apply scope in ability rule to the scope of load_resource through.
However, since relation.merge intends to mimic Hash#merge,
(i.e. later condition on the same column replaces the previous one),
this causes the load_resource through become ineffective when the scope in ability also specifics conditions on the same column.

This PR adds model adapter for ActiveRecord >= 6.1 and overrides database_records to use relation.and instead. It also tries to keep the query simple by only wrapping scopes to sub queries when necessary, which relies on private ActiveRecord API structurally_incompatible_values_for

Example:

ability file:

can :manage, Group

can :manage, Post, Post.where(group_id: user.groups) do |post|
  post.gorup.users.exists?(user.id)
end

controller:

class PostsController < ApplicationController
  load_and_authorize_resource :group
  load_and_authorize_resource through: :group
  
  def index
    # @posts will be from all groups,
    # as the condition for Post#group_id setted on `load_and_authorize_resource through`
    # is replaced by the condition specified in ability rule during relation.merge 
  end
end

References

For more information on the behavior of relation.merge and relation.and that was introduced in rails 6.1,
please refer to rails/rails#39250 & rails/rails#40944

`relation.merge` intends to mimics Hash#merge,
(i.e. later condition on the same column replaces the previous one)

see rails/rails#39250 & rails/rails#40944

Using relation#merge would cause suprising result when you try to limit
the resource collection through nesting while the foreign_key also has
condition in ability rule scope.
@coorasse
Copy link
Member

I'd like to tackle this by the next release that focuses on nested associations. Would it be possible for you to add the test for this case and rebase on the latest develop? If you don't have time, I'll take over. Thanks for your feedback and work

@albb0920
Copy link
Author

@coorasse I won't have a lot of free time until mid April, plus I'm not very familiar with the test suite of this project. Please take this over, thanks!

@kwent
Copy link

kwent commented Dec 17, 2023

Would love to see this one go through :). Rails 7 forced us to monkeypatch with @albb0920 solution.

@s-mage
Copy link
Contributor

s-mage commented Apr 24, 2024

Hey @coorasse thanks for a wonderful lib you're working on! Could I bring your attention to this PR please? It's already explained in the description but yeah, I have a very similar problem and the test that is missing would be

can :manage, Post, Post.where(group_id: user.groups)
load_resource :posts, through: :group 
# should return effectively Post.where(group_id: user.groups).where(group_id: id_from_the_path)
# now returns Post.where(group_id: user.groups)

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

Successfully merging this pull request may close these issues.

4 participants