Skip to content

Fix not_in search with deep nesting #1537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Fix not_in search with deep nesting #1537

wants to merge 3 commits into from

Conversation

giovannelli
Copy link

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 giovannelli force-pushed the main branch 2 times, most recently from eafb922 to 4566158 Compare November 18, 2024 14:12
@bopm bopm mentioned this pull request Jun 2, 2025
Merged
@bopm
Copy link
Contributor

bopm commented Jun 2, 2025

@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.

@clgiovannelli
Copy link
Contributor

@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.

@scarroll32 scarroll32 requested a review from Copilot September 24, 2025 16:34
Copilot
Copilot AI reviewed Sep 24, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the not_in search functionality with deep nesting by addressing an issue where generated SQL queries for excluding articles based on associated tags were incorrect. The fix modifies the query structure to use proper joins instead of subqueries, improving both correctness and performance.

  • Adds support for HABTM associations between comments and tags
  • Implements logic to handle table aliases in through reflections correctly
  • Adds comprehensive test coverage for negative conditions on HABTM associations

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
spec/support/schema.rb Adds HABTM relationship between comments and tags with join table
spec/ransack/search_spec.rb Uncomments previously pending test for published articles
spec/ransack/adapters/active_record/base_spec.rb Adds comprehensive test cases for negative conditions with HABTM associations
lib/ransack/nodes/condition.rb Implements core fix for table alias handling and nested condition detection

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@scarroll32
Copy link
Member

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

@giovannelli
Copy link
Author

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

Thank you!

@scarroll32
Copy link
Member

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

Thank you!

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

@scarroll32 scarroll32 closed this Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@giovannelli @bopm @clgiovannelli @scarroll32