ransack icon indicating copy to clipboard operation
ransack copied to clipboard

Fix `not_in` search with deep nesting

Open giovannelli opened this issue 1 year ago • 2 comments

Description

This pull request addresses issue #1411, where the generated SQL query for excluding articles based on associated tags was incorrect (I've used tags the my example).

The current query joins and filters tags improperly, resulting in an unintended query structure. Specifically, the subquery logic used for filtering tags produces incorrect results and may impact performance.

SELECT "articles".* 
FROM "articles" 
LEFT OUTER JOIN "comments" 
  ON "comments"."disabled" = 0 
  AND "comments"."article_id" = "articles"."id" 
WHERE ('default_scope' = 'default_scope') 
  AND "articles"."id" NOT IN (
    SELECT "comments_tags"."comment_id" 
    FROM "comments_tags" 
    INNER JOIN "tags" 
      ON "tags"."id" = "comments_tags"."tag_id" 
    WHERE "comments_tags"."comment_id" = "articles"."id" 
      AND NOT ("tags"."name" NOT IN ('Fantasy', 'Scifi'))
  )

The solution modifies the query structure to use proper joins and filtering, simplifying the SQL and ensuring correct results.

SELECT "articles".* 
FROM "articles" 
LEFT OUTER JOIN "comments" 
  ON "comments"."disabled" = 0 
  AND "comments"."article_id" = "articles"."id" 
LEFT OUTER JOIN "comments_tags" 
  ON "comments_tags"."comment_id" = "comments"."id" 
LEFT OUTER JOIN "tags" 
  ON "tags"."id" = "comments_tags"."tag_id" 
WHERE ('default_scope' = 'default_scope') 
  AND "tags"."name" NOT IN ('Fantasy', 'Scifi')

giovannelli avatar Nov 18 '24 12:11 giovannelli

@clgiovannelli I've incorporated your changes into #1561 as I'm dealing with the complex scenario that requires multiple interconnected changes from your PR, #1279, and my own to be applied at the same time. I also added additional test coverage. Tell me if you're ok with that.

bopm avatar Jun 02 '25 18:06 bopm

@clgiovannelli I've incorporated your changes into #1561 as I'm dealing with the complex scenario that requires multiple interconnected changes from your PR, #1279, and my own to be applied at the same time. I also added additional test coverage. Tell me if you're ok with that.

Thank you for taking the time to look into this. I'll check your changes today.

clgiovannelli avatar Jun 03 '25 08:06 clgiovannelli

@giovannelli can this be closed now https://github.com/activerecord-hackery/ransack/pull/1561 is merged?

scarroll32 avatar Sep 25 '25 06:09 scarroll32

@giovannelli can this be closed now #1561 is merged?

Thank you!

giovannelli avatar Sep 25 '25 10:09 giovannelli

@giovannelli can this be closed now #1561 is merged?

Thank you!

Great! Thank you for getting back to me @giovannelli 🤝

scarroll32 avatar Sep 25 '25 13:09 scarroll32