-
-
Notifications
You must be signed in to change notification settings - Fork 828
Check all children of Arel::Nodes::And
to extract correlated key
#1572
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.
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
Check all children of Arel::Nodes::And
to extract correlated key
#1572
Conversation
365c324
to
e471827
Compare
fix tests with mysql, which won't be failed e471827 |
There was a problem hiding this 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 an issue where correlated subqueries fail to extract the proper key when Arel::Nodes::And
nodes have more than two children. The fix changes from accessing only left
and right
children to iterating through all children in the children
array.
- Updates the
extract_correlated_key
method to handleArel::Nodes::And
nodes with multiple children - Adds test coverage for polymorphic associations with default scopes
- Adds a
RecentNote
model to support the new test case
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
lib/ransack/adapters/active_record/context.rb | Updates extract_correlated_key to iterate through all children of Arel::Nodes::And nodes |
spec/support/schema.rb | Adds RecentNote model and recent_notes association for testing |
spec/ransack/adapters/active_record/context_spec.rb | Adds test case for polymorphic associations with default scopes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
when Arel::Nodes::And | ||
extract_correlated_key(join_root.left) || extract_correlated_key(join_root.right) | ||
# And may have multiple children, so we need to check all, not via left/right | ||
join_root.children.each do |child| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@riseshia what is the potential for performance issues here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no performance issue here, it just has possibility that join_root
have more than 2 children.
…-multiple-children
…-multiple-children
🔗 #1571
This is proposed patch for #1571. Check issue for the detail of this.