mixing MetaWhere and non-MetaWhere hash conditions breaks the SQL #327

Closed
kirel opened this Issue Mar 31, 2011 · 22 comments

Comments

Projects
None yet
6 participants
@kirel

kirel commented Mar 31, 2011

When using MetaWhere and non MetaWhere hash conditions in rules the SQL breaks ( when passed to sanitize_sql in CanCan::ModelAdapters::ActiveRecordAdapter ).

This is a spec that fails:

it "should restrict articles given mixed MetaWhere and hash conditions" do
  @ability.can :read, Article, :priority.lt => 2
  @ability.can :read, Article, :priority => 1
  article1 = Article.create!(:priority => 1)
  article2 = Article.create!(:priority => 3)
  Article.accessible_by(@ability).should == [article1]
  @ability.should be_able_to(:read, article1)
  @ability.should_not be_able_to(:read, article2)
end

Update: Changed WhetaWhere -> MetaWhere (20 May 2012)

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb Mar 31, 2011

Owner

Thanks for providing a failing spec. I'll work on getting this fixed soon.

Owner

ryanb commented Mar 31, 2011

Thanks for providing a failing spec. I'll work on getting this fixed soon.

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb May 2, 2011

Owner

A solution was presented in issue #333 but as discussed there I believe there is a way to do this through the MetaWhere gem itself. I'll need to research this though.

Owner

ryanb commented May 2, 2011

A solution was presented in issue #333 but as discussed there I believe there is a way to do this through the MetaWhere gem itself. I'll need to research this though.

@treybean

This comment has been minimized.

Show comment Hide comment
@treybean

treybean May 6, 2011

Hi Ryan,

I'm hitting this problem as well and am starting to dig into cancan and metawhere to see if there's a better way to fix this. Do you have any suggestions about where in cancan I should be looking? It's like it's putting the class representation of the MetaWhere::Column directly in the sql instead of returning the actual column name. Here's my error:

PGError: ERROR:  column books.#<MetaWhere::Column:0x00000105af2ea8> does not exist
LINE 1: ..."."user_id" = 1 AND "books"."published" = 't' AND "books"."#...
                                                         ^
: SELECT "contributions".* FROM "contributions" INNER JOIN "books" ON "books"."id" = "contributions"."book_id" WHERE ("contributions".book_id = 1) AND (not ("contributions"."user_id" = 1 AND "books"."published" = 't' AND "books"."#<MetaWhere::Column:0x00000105af2ea8>" = '2011-05-06') AND ("books"."user_id" = 1))

If you can give me a little direction about how the metawhere conditions are managed I'd be happy to work on a patch. It looks like they're only dealt with in CanCan::ModelAdapters::ActiveRecordAdapter, is that right?

treybean commented May 6, 2011

Hi Ryan,

I'm hitting this problem as well and am starting to dig into cancan and metawhere to see if there's a better way to fix this. Do you have any suggestions about where in cancan I should be looking? It's like it's putting the class representation of the MetaWhere::Column directly in the sql instead of returning the actual column name. Here's my error:

PGError: ERROR:  column books.#<MetaWhere::Column:0x00000105af2ea8> does not exist
LINE 1: ..."."user_id" = 1 AND "books"."published" = 't' AND "books"."#...
                                                         ^
: SELECT "contributions".* FROM "contributions" INNER JOIN "books" ON "books"."id" = "contributions"."book_id" WHERE ("contributions".book_id = 1) AND (not ("contributions"."user_id" = 1 AND "books"."published" = 't' AND "books"."#<MetaWhere::Column:0x00000105af2ea8>" = '2011-05-06') AND ("books"."user_id" = 1))

If you can give me a little direction about how the metawhere conditions are managed I'd be happy to work on a patch. It looks like they're only dealt with in CanCan::ModelAdapters::ActiveRecordAdapter, is that right?

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb May 11, 2011

Owner

@treybean sorry for not getting back to you sooner. The CanCan::ModelAdapters::ActiveRecordAdapter is the right place to add this to CanCan. The problem is that CanCan does not know how to convert the MetaWhere conditions into SQL.

I wouldn't worry too much about the CanCan side of things though. If you or anyone can show a way to convert a MetaWhere condition into an SQL string then I can easily merge that into CanCan. I looked into MetaWhere briefly but could not figure it out enough to find a solution.

Owner

ryanb commented May 11, 2011

@treybean sorry for not getting back to you sooner. The CanCan::ModelAdapters::ActiveRecordAdapter is the right place to add this to CanCan. The problem is that CanCan does not know how to convert the MetaWhere conditions into SQL.

I wouldn't worry too much about the CanCan side of things though. If you or anyone can show a way to convert a MetaWhere condition into an SQL string then I can easily merge that into CanCan. I looked into MetaWhere briefly but could not figure it out enough to find a solution.

@treybean

This comment has been minimized.

Show comment Hide comment
@treybean

treybean May 11, 2011

@ryanb, I just did a pretty decent code dive on MetaWhere and didn't find anything. As near as I can tell, it hooks straight into arel for a lot of the to_sql functionality. Still, it seems like it has to translate its functions to something arel knows about at some point. I'll revisit it tomorrow.

Cheers.

@ryanb, I just did a pretty decent code dive on MetaWhere and didn't find anything. As near as I can tell, it hooks straight into arel for a lot of the to_sql functionality. Still, it seems like it has to translate its functions to something arel knows about at some point. I'll revisit it tomorrow.

Cheers.

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb May 12, 2011

Owner

@treybean, thanks for looking into this. We know the logic exists for MetaWhere conditions to be converted into SQL queries, whether it be through Arel or not. Perhaps we need to get the Arel query from MetaWhere and work with that directly.

Owner

ryanb commented May 12, 2011

@treybean, thanks for looking into this. We know the logic exists for MetaWhere conditions to be converted into SQL queries, whether it be through Arel or not. Perhaps we need to get the Arel query from MetaWhere and work with that directly.

@mtalcott

This comment has been minimized.

Show comment Hide comment
@mtalcott

mtalcott May 13, 2011

I'm able to get the same error (Unknown column '<table>.#<MetaWhere::Column:0x1053fff38>') whenever:

  • multiple can statements are declared on the same model
  • at least one of them has a MetaWhere call on a symbol

I don't see the error mixing MetaWhere and standard where syntax in the same can statement.

Hope that helps!

I'm able to get the same error (Unknown column '<table>.#<MetaWhere::Column:0x1053fff38>') whenever:

  • multiple can statements are declared on the same model
  • at least one of them has a MetaWhere call on a symbol

I don't see the error mixing MetaWhere and standard where syntax in the same can statement.

Hope that helps!

@mtalcott

This comment has been minimized.

Show comment Hide comment
@mtalcott

mtalcott May 13, 2011

Taking a look at what sanitize_sql does in ActiveRecordAdapter#merge_conditions, it looks like that's causing the problem.

User.send :sanitize_sql, :id => 3
 => "`users`.`id` = 3"
User.send :sanitize_sql, :id.eq => 3
 => "`users`.`#<MetaWhere::Column:0x10556c308>` = 3"

Like you said, maybe there's some way have MetaWhere do a similar thing.

Taking a look at what sanitize_sql does in ActiveRecordAdapter#merge_conditions, it looks like that's causing the problem.

User.send :sanitize_sql, :id => 3
 => "`users`.`id` = 3"
User.send :sanitize_sql, :id.eq => 3
 => "`users`.`#<MetaWhere::Column:0x10556c308>` = 3"

Like you said, maybe there's some way have MetaWhere do a similar thing.

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb May 14, 2011

Owner

@mtalcott yeah, we basically need some way to convert the MetaWhere condition into an SQL query in order to merge it with the others.

Owner

ryanb commented May 14, 2011

@mtalcott yeah, we basically need some way to convert the MetaWhere condition into an SQL query in order to merge it with the others.

@treybean

This comment has been minimized.

Show comment Hide comment
@treybean

treybean May 15, 2011

Okay, I'm not saying I have the solution, but I've dug around in MetaWhere a bit over the past few days and Ryan, the answer to your question about where MetaWhere is constructing the SQL query seems to be MetaWhere::Relation#build_arel (https://github.com/ernie/meta_where/blob/master/lib/meta_where/relation.rb#L150).

It first gets an arel table, then uses the visitor pattern to determine how to determine how to transform the different types of predicates into arel nodes, which are then collapsed onto the main arel object, which could be rendered to_sql at anytime.

I'm not exactly sure how best to use this knowledge to help us solve the problem in cancan, though. I rearranged my ability declarations a while ago to get around my immediate problem. Still, it seems like something we should be able to solve.

@ryanb, why exactly does cancan need to intercept the MetaWhere condition in the first place? Can't we just let it get passed on through to arel/ActiveRecord?

Okay, I'm not saying I have the solution, but I've dug around in MetaWhere a bit over the past few days and Ryan, the answer to your question about where MetaWhere is constructing the SQL query seems to be MetaWhere::Relation#build_arel (https://github.com/ernie/meta_where/blob/master/lib/meta_where/relation.rb#L150).

It first gets an arel table, then uses the visitor pattern to determine how to determine how to transform the different types of predicates into arel nodes, which are then collapsed onto the main arel object, which could be rendered to_sql at anytime.

I'm not exactly sure how best to use this knowledge to help us solve the problem in cancan, though. I rearranged my ability declarations a while ago to get around my immediate problem. Still, it seems like something we should be able to solve.

@ryanb, why exactly does cancan need to intercept the MetaWhere condition in the first place? Can't we just let it get passed on through to arel/ActiveRecord?

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb May 15, 2011

Owner

@treybean thanks for looking into this. The problem is CanCan currently constructs the SQL directly when there are multiple can conditions. We should probably switch this to use Arel, but that would make it require Rails 3. Hmm.

Owner

ryanb commented May 15, 2011

@treybean thanks for looking into this. The problem is CanCan currently constructs the SQL directly when there are multiple can conditions. We should probably switch this to use Arel, but that would make it require Rails 3. Hmm.

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb May 16, 2011

Owner

So the next step for this is to rewrite the ActiveRecordAdapter to use Arel for combining the queries, I would like to fall back to this old behavior if to_arel isn't available so it continues to work in Rails 2. I am not certain when I will get around to doing this, but if someone wants to provide a pull request that would be awesome.

Owner

ryanb commented May 16, 2011

So the next step for this is to rewrite the ActiveRecordAdapter to use Arel for combining the queries, I would like to fall back to this old behavior if to_arel isn't available so it continues to work in Rails 2. I am not certain when I will get around to doing this, but if someone wants to provide a pull request that would be awesome.

@treybean

This comment has been minimized.

Show comment Hide comment
@treybean

treybean May 16, 2011

@ryanb, MetaWhere requires arel, right? Does it even work with Rails 2? I see that it tries to include MetaWhere::Relation in ActiveRecord::Relation, which I would assume would fail in Rails 2, since I don't think there is an ActiveRecord::Relation in Rails 2.

I'll try to look into it a bit more, but I'm going on vacation on Wednesday for a week, so it might be a while before I get to it.

@ryanb, MetaWhere requires arel, right? Does it even work with Rails 2? I see that it tries to include MetaWhere::Relation in ActiveRecord::Relation, which I would assume would fail in Rails 2, since I don't think there is an ActiveRecord::Relation in Rails 2.

I'll try to look into it a bit more, but I'm going on vacation on Wednesday for a week, so it might be a while before I get to it.

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb May 16, 2011

Owner

I think MetaWhere requires Rails 3, but I'm talking about using Arel for the non MetaWhere query joins too. This way they can all be joined together easily. I would like to keep this working in Rails 2 projects so it should fall back to the old behavior.

Owner

ryanb commented May 16, 2011

I think MetaWhere requires Rails 3, but I'm talking about using Arel for the non MetaWhere query joins too. This way they can all be joined together easily. I would like to keep this working in Rails 2 projects so it should fall back to the old behavior.

@clyfe

This comment has been minimized.

Show comment Hide comment
@clyfe

clyfe Jun 7, 2011

Hi all,

I solved this using this monkey patch: https://gist.github.com/1012332
Maybe Ryan want's to integrate it in core? It should ilustrate how to make a sql fragment from a meta-where column.

That initializer only works with top-level keys, ie :name.matches => 'a%' but nested fails ie. :parent => {:name.matches => 'a%'} but that should be easy to fix I think, given that now we have the mechanism to make the sql fragment (see sanitize_meta_where method).

clyfe commented Jun 7, 2011

Hi all,

I solved this using this monkey patch: https://gist.github.com/1012332
Maybe Ryan want's to integrate it in core? It should ilustrate how to make a sql fragment from a meta-where column.

That initializer only works with top-level keys, ie :name.matches => 'a%' but nested fails ie. :parent => {:name.matches => 'a%'} but that should be easy to fix I think, given that now we have the mechanism to make the sql fragment (see sanitize_meta_where method).

@clyfe

This comment has been minimized.

Show comment Hide comment
@clyfe

clyfe Jun 7, 2011

I updated the gist with the monkey patch, now it should always work no matter the nesting used.

https://gist.github.com/1012332

clyfe commented Jun 7, 2011

I updated the gist with the monkey patch, now it should always work no matter the nesting used.

https://gist.github.com/1012332

@ryanb

This comment has been minimized.

Show comment Hide comment
@ryanb

ryanb Jun 8, 2011

Owner

Thanks for the Gist, I'll definitely check this out and look into incorporating it.

Owner

ryanb commented Jun 8, 2011

Thanks for the Gist, I'll definitely check this out and look into incorporating it.

@ghost ghost assigned andhapp May 16, 2012

@andhapp

This comment has been minimized.

Show comment Hide comment
@andhapp

andhapp May 16, 2012

Collaborator

I will attempt at incorporating this gist.

Collaborator

andhapp commented May 16, 2012

I will attempt at incorporating this gist.

@clyfe

This comment has been minimized.

Show comment Hide comment
@clyfe

clyfe May 17, 2012

@andhapp You should rather go this route https://gist.github.com/1523940

clyfe commented May 17, 2012

@andhapp You should rather go this route https://gist.github.com/1523940

@andhapp

This comment has been minimized.

Show comment Hide comment
@andhapp

andhapp May 17, 2012

Collaborator

@clyfe Thanks. Ofcouse, I will look at the gist and that's what I said in my message earlier.

Collaborator

andhapp commented May 17, 2012

@clyfe Thanks. Ofcouse, I will look at the gist and that's what I said in my message earlier.

@andhapp

This comment has been minimized.

Show comment Hide comment
@andhapp

andhapp May 30, 2012

Collaborator

Added a fix for this here. Please test and give feedback. Thanks.

Collaborator

andhapp commented May 30, 2012

Added a fix for this here. Please test and give feedback. Thanks.

@andhapp

This comment has been minimized.

Show comment Hide comment
@andhapp

andhapp Jun 8, 2012

Collaborator

I'm going to close this issue as a fix was merged into the master. Please comment if you'd like to open this for discussion.

Collaborator

andhapp commented Jun 8, 2012

I'm going to close this issue as a fix was merged into the master. Please comment if you'd like to open this for discussion.

@andhapp andhapp closed this Jun 8, 2012

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