Collapse where constraints to the Arel::Nodes::And node #12011
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @tenderlove, @senny, @rafaelfranca |
|
Build should be restarted because of minitest problems. |
|
@pftg done. |
|
Thanks @senny |
|
@senny, also may you fast review this. Travis failed because of |
|
Rebased and tests are passed. |
| @@ -41,6 +41,15 @@ def test_join_conditions_added_to_join_clause | |||
| assert_no_match(/WHERE/i, sql) | |||
| end | |||
|
|
|||
| def test_join_association_conditions_added_to_join_clause | |||
| sql = Author.joins(:welcome_posts_with_comment).to_sql | |||
| assert_match(/AND \(comments_count = 1\)/i, sql) | |||
There was a problem hiding this comment.
Can we assert the result instead of the SQL? The SQL generation can change in the future and this test will broke.
There was a problem hiding this comment.
This gist has very good test cases. Mind to reuse they? https://gist.github.com/iwiznia/6295952
In order to remove duplication with joining arel where constraints with `AND`, all constraints on `build_arel` are collapsed into one head node: `Arel::Nodes::And` Closes: rails#11963
|
@rafaelfranca thanks for feedback, I updated Changelog, and replaced sql assertions similar to gists examples by asserting results. |
rafaelfranca
added a commit
that referenced
this pull request
Sep 16, 2013
…ation_scope Collapse where constraints to the Arel::Nodes::And node Conflicts: activerecord/CHANGELOG.md
rafaelfranca
added a commit
that referenced
this pull request
Sep 16, 2013
…ation_scope Collapse where constraints to the Arel::Nodes::And node Conflicts: activerecord/CHANGELOG.md Conflicts: activerecord/CHANGELOG.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to remove duplication with joining where constraints by
ANDon traversearel.constraints,refactored
collapse_wheresto add toarel.wheresonbuild_arelonly one nodeArel::Nodes::And.This refactoring remove duplication to which fixed #11963:
because
Arel::Nodes::Node#andexpected that argument (in our caserel.arel.constraints) is notArray, in that wayArel::Visitors::ToSqlon visitingArrayreturns,instead ofAND.