Fix pluck and select with from if from has original table name
#35360
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
This is caused by 0ee96d1. Since rails#18744, `select` columns doesn't be qualified by table name if using `from`. 0ee96d1 follows that for `pluck` as well. But people depends that `pluck` columns are qualified even if using `from`. So I've fixed that to be qualified if `from` has the original table name to keep the behavior as much as before. Fixes rails#35359.
kamipo
commented
Feb 21, 2019
| def test_select_with_subquery_in_from_uses_original_table_name | ||
| relation = Comment.joins(:post).select(:id).order(:id) | ||
| subquery = Comment.from(Comment.all, Comment.quoted_table_name).joins(:post).select(:id).order(:id) | ||
| assert_equal relation.map(&:id), subquery.map(&:id) |
There was a problem hiding this comment.
Weird... This test isn't failed locally... I suspect a bug on SQLite3...
https://travis-ci.org/rails/rails/jobs/496726410#L1198-L1208
There was a problem hiding this comment.
It did fail on buildkite as well https://buildkite.com/rails/rails/builds/58981
There was a problem hiding this comment.
ah... skipped on buildkite as well for now a333ba3.
…` on Travis I'm not sure why the test is failed on Travis, it passed on locally. I suspect that failure is a bug on SQLite3, so just skip the test for now, since it was not covered by before. https://travis-ci.org/rails/rails/jobs/496726410#L1198-L1208
kamipo
added a commit
that referenced
this pull request
Feb 21, 2019
Fix `pluck` and `select` with `from` if `from` has original table name
eileencodes
added a commit
to eileencodes/rails
that referenced
this pull request
Feb 28, 2019
After rails#35360 was merged we bumped Rails again and while the test script we sent passed we still had the same failures. I debugged a bit and found that the new code assumes that the `from` is passing only a table name and that we can get that from `name` or from `value` but there are cases (like this test case) where you may not pass a name along with the `FromClause` and the value may not be just a table name. This test demonstrates what happens when you add an index to the from clause.
kamipo
added a commit
to kamipo/rails
that referenced
this pull request
Mar 1, 2019
…X hint rails#35360 allows table name qualified if `from` has original table name. But that is still too strict. We have a valid use case that `from` with INDEX hint (e.g. `from("comments USE INDEX (PRIMARY)")`). So I've relaxed the table name detection in `from` to allow any extension like INDEX hint. Fixes rails#35359.
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.
This is caused by 0ee96d1.
Since #18744,
selectcolumns doesn't be qualified by table name ifusing
from. 0ee96d1 follows that forpluckas well.But people depends that
pluckcolumns are qualified even if usingfrom.So I've fixed that to be qualified if
fromhas the original table nameto keep the behavior as much as before.
Fixes #35359.
cc @eileencodes