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

Already on GitHub? Sign in to your account

Expand order(:symbol) to "table".symbol to prevent broken queries on PG. #9414

Merged
merged 1 commit into from Feb 26, 2013

Conversation

Projects
None yet
5 participants
Member

senny commented Feb 25, 2013

Fixes #9275.

When #order is called with a Symbol this patch will prepend the quoted_table_name.
Before the postgresql adapter failed to build queries containg a join and an order
with a symbol.

This expansion happens for all adapters.

I moved the expansion directly into relation becuase the Adapter#distinct (where the problem occurs) method does not know the table name. Also I don't think every method using order_values should know how to expand symbols when we can just deal with strings.

Member

senny commented Feb 25, 2013

@rafaelfranca @jonleighton could you take a look?

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Feb 25, 2013

activerecord/test/cases/associations/eager_test.rb
@@ -1173,4 +1173,10 @@ def test_deep_including_through_habtm
assert_no_queries { assert_equal 2, author.comments_with_order_and_conditions.size }
assert_no_queries { assert_equal 5, author.posts.size, "should not cache a subset of the association" }
end
+
+ test "works in combination with order(:symbol)" do
+ ActiveSupport::Deprecation.silence do
@rafaelfranca

rafaelfranca Feb 25, 2013

Owner

Why the deprecation here?

@senny

senny Feb 25, 2013

Member

good catch, this is a relict from an older version of the test-case which featured: where(:name => 'dotted.string')

Member

senny commented Feb 25, 2013

@rafaelfranca removed the silencing.

@rafaelfranca rafaelfranca commented on an outdated diff Feb 25, 2013

activerecord/test/cases/associations/eager_test.rb
@@ -1173,4 +1173,8 @@ def test_deep_including_through_habtm
assert_no_queries { assert_equal 2, author.comments_with_order_and_conditions.size }
assert_no_queries { assert_equal 5, author.posts.size, "should not cache a subset of the association" }
end
+
+ test "works in combination with order(:symbol)" do
+ Author.includes(:posts).references(:posts).order(:name).where('posts.title IS NOT NULL').first
@rafaelfranca

rafaelfranca Feb 25, 2013

Owner

Should not we asserting something?

Expand order(:symbol) to "table".symbol to prevent broken queries on PG.
Fixes #9275.

When `#order` is called with a Symbol this patch will prepend the quoted_table_name.
Before the postgresql adapter failed to build queries containg a join and an order
with a symbol.

This expansion happens for all adapters.
Member

senny commented Feb 25, 2013

@rafaelfranca added an assertion. It's not the most useful but I guess it's good enough to lock the behavior.

Owner

rafaelfranca commented Feb 25, 2013

Seems good. I'll wait the release to merge

Contributor

frodsan commented Feb 26, 2013

@rafaelfranca already released.

Contributor

khustochka commented Feb 26, 2013

Is this patch applied to rails 4.0.0.beta1? Because I am getting errors on queries with column aliases, like this:

Post.select('DISTINCT EXTRACT(year from face_date) AS year').order(:year)

ActiveRecord::StatementInvalid: PG::Error: ERROR:  column posts.year does not exist
Member

senny commented Feb 26, 2013

@khustochka no it's not yet applied.

Member

senny commented Feb 26, 2013

@khustochka but looking at your example this will certainly result in problems when this patch is applied. I think you should not use a Symbol in this case.

Contributor

khustochka commented Feb 26, 2013

@senny, thank you. I am just wondering as for me order(:symbol) is already expanded to "table".symbol, or rather "table"."symbol".

My example works on 3.2.12 because it never adds table name to this symbol. Looks like a new feature in rails 4.

rafaelfranca added a commit that referenced this pull request Feb 26, 2013

Merge pull request #9414 from senny/9275_order_with_symbol_and_join
Expand order(:symbol) to "table".symbol to prevent broken queries on PG.

@rafaelfranca rafaelfranca merged commit 8bc5e71 into rails:master Feb 26, 2013

Hi, I'd like to avoid slapping together SQL string literals if possible please. If you're writing SQL literal strings, please think twice.

I've removed the SQL literal here: d345ed4

Member

senny replied Jul 16, 2013

@tenderlove Thanks for the fix and letting me know. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment