1.6.8 causes problems with multiple can clauses with join conditions #724

Open
ronalchn opened this Issue Aug 19, 2012 · 3 comments

Comments

Projects
None yet
2 participants
@ronalchn

Recently upgraded to 1.6.8 and it made the index accessible_by stop working.

There was the problem that a more specific can clause overwrites one without condtitions, but a simple re-ordering is enough.

However, the following two clauses:

can :manage, Problem, :owner_id => user.id
can :read, Problem, :problem_sets.outer => {:groups.outer => {:users.outer => {:id => user.id}}}

is causing me significant problems.

Notice I am using the .outer extension to force using the outer join instead of the inner join, which would make above clauses incompatible.

With 1.6.8, no matter how they are ordered, they do not work.

Please make OUTER JOIN the default when checking accessible_by when there are multiple can clauses, and fix whatever causes above to stop working.

elabs-dev pushed a commit to varvet/cancan that referenced this issue Aug 22, 2012

Jonas Nicklas and Nicklas Ramhöj
Allow multiple abilities with associations
There are two issues with the current way cancan handles associations:

1) Records are returned multiple times in some circumstances
2) Several defined abilities prevent some records to show up under certain circumstances

This commit includes tests for both cases. It fixes both problems by changing `joins` to `includes` for the AR adapters. This could have performance implications, since `includes` will also select all columns in the associated records. We tried various ways of achieving the same thing using Arel directly, but were unable to make this work due to lack of support for outer joins in Rails 3.1.

This closes issues #724, #566 and #613
@ronalchn

This comment has been minimized.

Show comment Hide comment
@ronalchn

ronalchn Aug 23, 2012

I have just realised a possible way to deal with this.

The idea is that the following works normally:

Problem.where(:id => Problem.joins(:problem_sets).where(:problem_sets => {:owner_id => 35}))

This translates to:

SELECT "problems".* FROM "problems" WHERE "problems"."id" IN (SELECT "problems"."id" FROM "problems" INNER JOIN "problem_sets_problems" ON "problem_sets_problems"."problem_id" = "problems"."id" INNER JOIN "problem_sets" ON "problem_sets"."id" = "problem_sets_problems"."problem_set_id" WHERE "problem_sets"."owner_id" = 35)

So basically, the idea is that we can do something like:

can :read, Problem, :id => Problem.joins(:problem_sets).where(:problem_sets => {:owner_id => 35})

The idea is that instead of outer joins (or any joins), we can convert them to WHERE id IN (subquery).

I tested this in CanCan, and some of the functionality "kind of" works (but with bugs). The problem is that it also performs the query when running Ability.new().initialize(). That means that when you run the above can :read ... command, immediately, it will query the database:

SELECT "problems".* FROM "problems"
INNER JOIN "problem_sets_problems" ON "problem_sets_problems"."problem_id" = "problems"."id"
INNER JOIN "problem_sets" ON "problem_sets"."id" = "problem_sets_problems"."problem_set_id"
WHERE "problem_sets"."owner_id" = 35

It will do the correct query when using accessible_by:

Problem.accessible_by(Ability.new().initialize(User.find(35)))

If we modify CanCan so that it does not try to resolve ActiveRelation objects when they are found in the conditions hash, this will be a good way to solve the situation. This means that CanCan will resolve the subquery at query time, so that it does not resolve unused subqueries. By unused subqueries I mean that when an unrelated controller/action is accessed, CanCan will not perform the unnecessary query. It will only perform it if the index action (which calls accessible_by) is required.

Doing this also breaks the functionality of the can? command, which doesn't seem to do the right thing.

current_ability.can? :read, Problem.find(8)

In addition, I notice that the following two statements are similar:

can :read, Problem, :id => Problem.joins(:problem_sets).where(:problem_sets => {:owner_id => 35})
can :read, Problem, Problem.joins(:problem_sets).where(:problem_sets => {:owner_id => 35})

The difference is that the 2nd is a scope, which cannot be merged automatically.

I suggest that when a scope is found, assuming we know that the model is unique on one of the attributes, we can add that attribute automatically (ie. adds :id => scope).

This change would allow scopes to be merged automatically.

Compared to the outer join method, we would not have joining problems, and it would not select a record multiple times. We would basically have something like:

SELECT * FROM problems WHERE id IN (subquery1) OR id IN (subquery2) OR id IN (subquery3)

I have just realised a possible way to deal with this.

The idea is that the following works normally:

Problem.where(:id => Problem.joins(:problem_sets).where(:problem_sets => {:owner_id => 35}))

This translates to:

SELECT "problems".* FROM "problems" WHERE "problems"."id" IN (SELECT "problems"."id" FROM "problems" INNER JOIN "problem_sets_problems" ON "problem_sets_problems"."problem_id" = "problems"."id" INNER JOIN "problem_sets" ON "problem_sets"."id" = "problem_sets_problems"."problem_set_id" WHERE "problem_sets"."owner_id" = 35)

So basically, the idea is that we can do something like:

can :read, Problem, :id => Problem.joins(:problem_sets).where(:problem_sets => {:owner_id => 35})

The idea is that instead of outer joins (or any joins), we can convert them to WHERE id IN (subquery).

I tested this in CanCan, and some of the functionality "kind of" works (but with bugs). The problem is that it also performs the query when running Ability.new().initialize(). That means that when you run the above can :read ... command, immediately, it will query the database:

SELECT "problems".* FROM "problems"
INNER JOIN "problem_sets_problems" ON "problem_sets_problems"."problem_id" = "problems"."id"
INNER JOIN "problem_sets" ON "problem_sets"."id" = "problem_sets_problems"."problem_set_id"
WHERE "problem_sets"."owner_id" = 35

It will do the correct query when using accessible_by:

Problem.accessible_by(Ability.new().initialize(User.find(35)))

If we modify CanCan so that it does not try to resolve ActiveRelation objects when they are found in the conditions hash, this will be a good way to solve the situation. This means that CanCan will resolve the subquery at query time, so that it does not resolve unused subqueries. By unused subqueries I mean that when an unrelated controller/action is accessed, CanCan will not perform the unnecessary query. It will only perform it if the index action (which calls accessible_by) is required.

Doing this also breaks the functionality of the can? command, which doesn't seem to do the right thing.

current_ability.can? :read, Problem.find(8)

In addition, I notice that the following two statements are similar:

can :read, Problem, :id => Problem.joins(:problem_sets).where(:problem_sets => {:owner_id => 35})
can :read, Problem, Problem.joins(:problem_sets).where(:problem_sets => {:owner_id => 35})

The difference is that the 2nd is a scope, which cannot be merged automatically.

I suggest that when a scope is found, assuming we know that the model is unique on one of the attributes, we can add that attribute automatically (ie. adds :id => scope).

This change would allow scopes to be merged automatically.

Compared to the outer join method, we would not have joining problems, and it would not select a record multiple times. We would basically have something like:

SELECT * FROM problems WHERE id IN (subquery1) OR id IN (subquery2) OR id IN (subquery3)
@ronalchn

This comment has been minimized.

Show comment Hide comment
@ronalchn

ronalchn Aug 23, 2012

Ah, I see the problem with passing scopes - you then need to use a block to get the correct current_ability.can? behaviour (in case of !persisted?).

Nevertheless, this is a way that:

  • can be used internally for merging multiple nested hash conditions for accessible_by (instead of inner or outer joins)
  • can be used to merge scopes (even though blocks are then also needed to define their behaviour)

Ah, I see the problem with passing scopes - you then need to use a block to get the correct current_ability.can? behaviour (in case of !persisted?).

Nevertheless, this is a way that:

  • can be used internally for merging multiple nested hash conditions for accessible_by (instead of inner or outer joins)
  • can be used to merge scopes (even though blocks are then also needed to define their behaviour)

bryanrite added a commit to bryanrite/cancan that referenced this issue Jan 27, 2014

Allow multiple abilities with associations
There are two issues with the current way cancan handles associations:

1) Records are returned multiple times in some circumstances
2) Several defined abilities prevent some records to show up under certain circumstances

This commit includes tests for both cases. It fixes both problems by changing `joins` to `includes` for the AR adapters. This could have performance implications, since `includes` will also select all columns in the associated records. We tried various ways of achieving the same thing using Arel directly, but were unable to make this work due to lack of support for outer joins in Rails 3.1.

This closes issues #724, #566 and #613
@xhoy

This comment has been minimized.

Show comment Hide comment
@xhoy

xhoy Apr 10, 2014

Dear submitter, Since cancan/raynB hasn't been active for more than 6 months and no body else then ryam himself has commit permissions the cancan project is on a stand still.
Since cancan has several issues including missing support for rails 4 cancan is moving forward to cancancan. More details on: #994

If your feel that your pull request or bug is still applicable (and hasn't been merged in to cancan) it would be really appreciated if you would resubmit it to cancancan (https://github.com/cancancommunity/cancancan)

We hope to see you on the other side!

xhoy commented Apr 10, 2014

Dear submitter, Since cancan/raynB hasn't been active for more than 6 months and no body else then ryam himself has commit permissions the cancan project is on a stand still.
Since cancan has several issues including missing support for rails 4 cancan is moving forward to cancancan. More details on: #994

If your feel that your pull request or bug is still applicable (and hasn't been merged in to cancan) it would be really appreciated if you would resubmit it to cancancan (https://github.com/cancancommunity/cancancan)

We hope to see you on the other side!

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