Skip to content
This repository has been archived by the owner on Mar 30, 2022. It is now read-only.

Fixed missing bind values when using #in with scope that joins polymorphic association #380

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

Conversation

andriusch
Copy link

This might also fix some other reported errors with bind values (#344 for example)

@rhomeister
Copy link

Tested this in my code. It solved a large number of PG::ProtocolViolation errors. Thanks! Would be great if this could be merged and released.

@rogierslag
Copy link

This might also fix #369

@rogierslag
Copy link

This breaks some queries!

eg

SELECT "users".* 
FROM   "users" 
       INNER JOIN "orders" 
               ON "orders"."user_id" = "users"."id" 
WHERE  "users"."id" = 'Shop' 
       AND ( "users"."id" = 201 
              OR "orders"."id" IN (SELECT "orders"."id" 
                                   FROM   "orders" 
                                   WHERE  ( "orders"."user_id" = 201 
                                             OR ( "orders"."shop_id" IN 
                                                  (SELECT "shops"."id" 
                                                   FROM   "shops" 
                                                  LEFT OUTER JOIN 
                                                  "access_rights" 
                                                               ON 
"access_rights"."accessible_id" 
= 
"shops"."id" 
AND 
"access_rights"."accessible_type" 
= 
'Event' 
                       WHERE 
( "shops"."event_id" IN 
  (SELECT "events"."id" 
   FROM   "events" 
  LEFT OUTER JOIN "access_rights" 
               ON 
  "access_rights"."accessible_id" 
  = 
  "events"."id" 
  AND 
"access_rights"."accessible_type" = 
'Organization' 
       WHERE  ( 
"access_rights"."user_id" = 201 
OR "events"."organization_id" IN 
(SELECT 
"organizations"."id" 
FROM 
"organizations" 
LEFT OUTER JOIN "access_rights" 
 ON 
"access_rights"."accessible_id" = 
"organizations"."id" 
AND 
"access_rights"."accessible_type" = 
201 
 WHERE  ( 
"access_rights"."user_id" 
= 201 
AND 
"access_rights"."accessible_type" = 
'Organization' )) ) 
ORDER  BY events.created_at) 
OR "access_rights"."user_id" = 201 ) 
ORDER  BY shops.created_at) 
AND "orders"."status" = 4 ) ) 
ORDER  BY orders.created_at ASC, 
orders.id ASC) ) 
ORDER  BY created_at 
LIMIT  1 

Note the match between a user_id and the string 'Shop'

@andriusch
Copy link
Author

I'd be interested to debug why it breaks that query if you can isolate the problem a bit and give me a sample code that breaks.

@rogierslag
Copy link

In this case it a policy_scope(User).find(params[:id]) which in turn requests some other stuff.

It looks like the find or where will always be added at the front (so the parameter binding should be done at the front as well) compared to the joins.

I had a similar problem as you to start with, and made a complete failing ruby test. See https://github.com/inventid/squeel-bug-369-testcase for that

@andriusch
Copy link
Author

I checked your project and I can't reproduce your bug. policy_scope(User).find(params[:id]) generates SELECT "users".* FROM "users" WHERE "users"."id" = 1 AND "users"."id" = ? LIMIT 1 [["id", 1]] so I don't know what you're referring to.
Also get rid of any third party gems when creating a demo. People can't be expected to learn all third party libraries (like pundit for example) when looking into your bug. Isolate what causes your problem, then I can look into it.

@shepmaster
Copy link

(#344 for example)

Unfortunately, this PR does not seem to help with #344

@jonatack
Copy link

jonatack commented Nov 4, 2015

For what it's worth, I updated Polyamorous, the polymorphic library that Squeel depends on, for Rails 5.0 today and while doing so, found a potential small regression in polymorphic joins that I mistakenly made last April.

Don't hesitate to try Polyamorous master with Squeel (add gem 'polyamorous', github: 'activerecord-hackery/polyamorous' to your Gemfile next to gem 'squeel') and let me know.

Thanks!

@andriusch
Copy link
Author

Tried, but can't test it, polyamorous is version 1.2.0 not, but squeel depends on ~1.1.0

@allenwq
Copy link

allenwq commented May 19, 2016

Experiencing the same issue, hope this can be merged soon.

jrreed added a commit to curious/squeel that referenced this pull request Jun 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants