Rails 3.2.6 breaks conditions in nested hashes

[edruder]

Given a stock Rails 3.2.x app with the following AR models (and migrations):

class A < ActiveRecord::Base
  belongs_to :b
end

class B < ActiveRecord::Base
  belongs_to :c
end

class C < ActiveRecord::Base
  attr_accessible :foo
end

And the following ability in CanCan:

can :read, A, :b => {:c => {:foo => true}}

In Rails 3.2.6, the call to load_resource will result in bad SQL being executed (and failing). In Rails 3.2.5, it works as expected.

As I read it, a comment on the Rails bug that causes this, https://github.com/rails/rails/issues/6718, states that the ActiveRecord behavior that CanCan depends on for this to work is unintended.

[andhapp]

@eruder: Thanks for reporting this. Would you like to take a stab at it and create a pull request?

[edruder]

@andhapp If I get some time, I'd love to give it a shot.

BTW, the Rails bug has been closed, as the behavior is unintended and the query can be written without the nesting. I.e., the ActiveRecord query that the CanCan ability (above) ultimately resolves to is:

A.joins(:b => :c).where(:b => {:cs => {:foo => true}})

And the following AR query is equivalent, and works in Rails 3.2.5 & 3.2.6:

A.joins(:b => :c).where(:cs => {:foo => true})

You can rephrase the CanCan ability using a scope on A and a block. I didn't come up with (but didn't try very hard) an equivalent ability using hash notation that works in Rails 3.2.6--is there one?

[the8472]

You can rephrase the CanCan ability using a scope on A and a block.

That doesn't work with multiple cancan rules as you can only have one scoped rule per class. Therefore it's currently not possible to combine multiple complex rules.

Simplified Example:

can :edit, Comment, {:thread => {:owner_id => @current_user.id}
can :edit, Comment, {:thread => :board => :users => {:id => :@current_user.id, :is_mod => true}

This requires joins and nesting with the current cancan syntax. Using a scope + block means i'd have to automatically generate the scope and the block separately from merged hashes. Something that cancan did for me so far.

[the8472]

Since I need this working right now: monkeypatch/fix, white-hot from the foundry, untested, use at your own risk etc. etc. https://gist.github.com/2948257 (requires squeel)

[rafakuch]

I built a method to dig into the conditions hash and get the portions that matters...

Not fully tested but It seems to work... Any thoughts would be appreciated!

https://gist.github.com/3842277

[the8472]

Any thoughts would be appreciated!

You might want to use association reflections on the classes to determine the actual table names.

E.g. can :manage, A, {:foo => {:bar => :baz}} should result in something like this:

table_name = A.reflect_on_association(:foo).klass.table_name
col_name = :"#{table_name}.bar"

conditions = {col_name => "baz"}

[vollnhals]

does anybody have a working production-quality fix for this yet?

[vollnhals]

I tried the referenced gist above (https://gist.github.com/3842277). That fix does not work with multiple conditions in the last hash.

For example:

can :manage, Note, :board => {:project => {:participations => {:user_id => user.id, :permission => "owner"}}}

I changed the gist slightly to work with multiple conditions. see https://gist.github.com/4031511

(I did not try https://gist.github.com/2948257, because i did not want to introduce another dependency)

[rafakuch]

@vollnhals you're right, I haven't noticed that

[heliostatic]

Well, at least I know I'm hitting a known bug. Is there any plan to roll https://gist.github.com/4031511 into 2.0?

[heliostatic]

Additionally, trying @vollnhals's fix, I've still got an issue where a join isn't being performed on the column specified as :source in the has_many :through relationship. Example:

ability.rb can :read, :submissions, competition: { judges: {id: user.id } }

competition.rb has_many :judges, :through => :submissions, :conditions => {submissions: {role: 2}}, :source => :user

So I get an error no such column: users.id because the inner join on Users never happens.

[heliostatic]

Last comment, I promise. Also hitting this issue with @vollnhals's fix: undefined methodto_sym' for Thu, 08 Nov 2012:Date` which is coming from this ability:

cannot :read, :competitions, ['deadline < ?', Date.today] do |c|
  c.expired
end

[the8472]

Yet another issue with the AR-Adapter...

Here are my collected fixes for the AR adapter (again, relying on squeel), maybe it will help: https://gist.github.com/4039777

• converts inner joins to outer joins as necessary
• allows for nested conditions
• since it's using squeel it will give nested joins a better treatment and match conditions and joined table names appropriately
• can merge scope based conditions (no more warnings about unmergeable conditions), this part is far from perfect though

[heliostatic]

@the8472, looks great. Dumb question: what needs to go in the Squeel initializer for everything to work as designed?

[the8472]

This is what I'm using right now:

Squeel.configure do |config|
  config.load_core_extensions :hash, :symbol
end

At least the symbol extensions are necessary (for the outer joins), the hash ones probably not.

You have to be aware of this caveat though: https://github.com/ernie/squeel#symbols-as-the-value-side-of-a-condition-in-normal-where-clauses

If you use symbols instead of strings as values in any active record conditions hash in your application you'll get some breakage.

[heliostatic]

The symbol conditions seems to be my issue. Here we go a-replacing.

[heliostatic]

@the8472 , this doesn't seem to resolve the issue of creating the join when there's a has_many :through, :source, and seems to introduce a new issue with a scope based condition (drops the name of the column for some reason). Probably user error on my end, in any case.

[the8472]

I think I am using has_many through in my cancan abilities and they're working. So your problem might be that the conditions don't work properly with nested hashes.

Try this:

has_many :judge_submissions, :class_name => Submission.name, :conditions => {role: 2} # just guessing at your structure here
has_many :judges, :through => :judge_submissions, :source => :user

I usually auto-generated through conditions in pairs for that reason.

[the8472]

Probably user error on my end, in any case.

Not necessarily. This monkey patch is only tailored to the use-cases that i've needed fixed. There might be edge-cases that are not covered.

It's by no means a clean solution. It's more of a stop-gap measure to work around an otherwise - from my perspective - totally broken cancan.

[heliostatic]

Per your comment, https://github.com/ryanb/cancan/issues/646#issuecomment-10199312, I updated my model, which makes a ton of sense. Looks like things are working in that area. Thanks so much.

Finally, any thoughts on why this rule would break things?

cannot :read, :competitions, ['deadline < ?', Date.today] do |c|
  c.expired
end

Given this scope:

scope :expired, -> { where("deadline < ?", Date.today) }

[the8472]

Here's my cancan rule, does it look right?

Pass in the class directly, I don't think cancan does singularize magic: can :read, Submission, ...

Finally, any thoughts on why this rule would break things?

The where clause/scope goes to the 3rd parameter, the ruby evaluation for already-loaded instances into the block. And with my patch + scopes in a cannot rule... might very well break.

[heliostatic]

Thanks for the response. I'll debug now.

I'm using CanCan 2.0, which is why I'm passing the plural symbol (per the documentation...?)

Also could be why I'm running into extra special breaking cases.

Finally, the conditions are all working now, but the accessible_by call is breaking. Much more tractable.

[the8472]

Oh, this was written for 1.6.8

[heliostatic]

Figured.

[heliostatic]

For what it's worth, clobbered the last line of the case statement and everything seems to be working. Have to write some tests to make sure I'm not missing anything, but manual testing looks ok.

[the8472]

clobbered the last line of the case statement

Careful with that one, it's responsible for combining multiple conditions, especially negated ones.

[heliostatic]

I saw. I found the fact that anything worked afterwards surprising. Still digging.

[the8472]

I completely rewrote the patch since i realized that i was doing more work to accomodate existing infrastructure.

It's basically a pure squeel-adapter now replacing the AR adapter.

https://gist.github.com/4039777/9309e24065d4f3eaadbd5e7ff447acac53bd9deb

Again, mostly untested.

[heliostatic]

Excellent, I'll give it a test run shortly.

[heliostatic]

One issue I'm hitting with this:

model:

has_many :judge_submissions, :class_name => Submission.name, :conditions => {role: 2}
has_many :judges, :through => :judge_submissions, :source => :user

generated query: "judges"."id" = 9

It seems like it's not picking up the source and conditions across the split association.

[heliostatic]

Actually, is the :source at all. Collapsed my relations and ran into the same issue.

[the8472]

Updated the gist, try that.

[heliostatic]

That seems to have fixed the issue. I'm still have a problem with the combined sql/block rule definitions. eg,

cannot :read, :competitions, ['deadline < ?', Date.today] do |c|
  c.deadline < Date.today
end

but everything else seems to be working. (I'm still in cancan 2.0, so that could well be the reason for the issue)

[the8472]

I would recommend using scopes for the SQL there (i'm not using raw sql statements, hence i didn't think about supporting them). Should be easy enough to fix though.

And wrt. to blocks. I think that changed in cancan 2. Have a look at the wiki and the can-rule API in the sourcecode.

[the8472]

Ok, raw SQL should work too now with 1.6.8. Everything else should just be a matter of the rule definition API.

[heliostatic]

Switched to scope + block and everything seems to be working on sqlite and postgres -- great patch! Thanks.

[dieb]

Hello!

I just flipped through this thread because I had the same problem. Thanks everyone for all the comments.

Could someone clear this for me: Is this a bug on AR or cancan?

Anyone working on fixing this?

I was wondering if it would be possible to fix this within cancan (that is, without third-parties).

Thanks!

[the8472]

@dieb, it's mostly a bug in cancan since it relied on AR behavior which was unintended.

But the real problem is that cancan's rules cannot be mapped completely to the active record api. They can only be mapped to arel or squeel nodes. The main problem is the lack of outer joins and aliasing. Even if you want to approximate the behavior with active record you still have to perform reflection on the model relations to extract the proper table names for qualified column names. @rafakuch posted a very crude approach like that above, but as i said, it's not a correct representation of cancan rule semantics.

[codezomb]

This is definitely going to be more of an issue now that people are being urged to use rails 3.2.11 for the latest security patches. Is cancan even being actively developed anymore?

[xhoy]

Thanks for your submission! The ryanb/cancan repository has been inactive since Sep 06, 2013. Since only Ryan himself has commit permissions, the CanCan project is on a standstill.

CanCan has many open issues, including missing support for Rails 4. To keep CanCan alive, an active fork exists at cancancommunity/cancancan. The new gem is cancancan. More info is available at #994.

If your pull request or issue is still applicable, it would be really appreciated if you resubmit it to CanCanCan.

We hope to see you on the other side!