Skip to content
This repository
Browse code

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.
commit e0356990856abc9a84e6e038e7b06e2931502728 1 parent 45321a6
Yves Senn senny authored
9 activerecord/CHANGELOG.md
Source Rendered
... ... @@ -1,5 +1,14 @@
1 1 ## Rails 4.0.0 (unreleased) ##
2 2
  3 +* Fix when performing an ordered join query. The bug only
  4 + affected queries where the order was given with a symbol.
  5 + Fixes #9275.
  6 +
  7 + Example:
  8 +
  9 + # This will expand the order :name to "authors".name.
  10 + Author.joins(:books).where('books.published = 1').order(:name)
  11 +
3 12 * Fixing issue #8345. Now throwing an error when one attempts to touch a
4 13 new object that has not yet been persisted. For instance:
5 14
5 activerecord/lib/active_record/relation/query_methods.rb
@@ -285,6 +285,11 @@ def order!(*args) # :nodoc:
285 285 references.map! { |arg| arg =~ /^([a-zA-Z]\w*)\.(\w+)/ && $1 }.compact!
286 286 references!(references) if references.any?
287 287
  288 + # if a symbol is given we prepend the quoted table name
  289 + args = args.map { |arg|
  290 + arg.is_a?(Symbol) ? "#{quoted_table_name}.#{arg} ASC" : arg
  291 + }
  292 +
288 293 self.order_values = args + self.order_values
289 294 self
290 295 end
7 activerecord/test/cases/associations/eager_test.rb
@@ -218,7 +218,7 @@ def test_finding_with_includes_on_belongs_to_association_with_same_include_inclu
218 218 def test_finding_with_includes_on_null_belongs_to_association_with_same_include_includes_only_once
219 219 post = posts(:welcome)
220 220 post.update!(author: nil)
221   - post = assert_queries(1) { Post.all.merge!(includes: {author_with_address: :author_address}).find(post.id) }
  221 + post = assert_queries(1) { Post.all.merge!(includes: {author_with_address: :author_address}).find(post.id) }
222 222 # find the post, then find the author which is null so no query for the author or address
223 223 assert_no_queries do
224 224 assert_equal nil, post.author_with_address
@@ -1173,4 +1173,9 @@ def test_deep_including_through_habtm
1173 1173 assert_no_queries { assert_equal 2, author.comments_with_order_and_conditions.size }
1174 1174 assert_no_queries { assert_equal 5, author.posts.size, "should not cache a subset of the association" }
1175 1175 end
  1176 +
  1177 + test "works in combination with order(:symbol)" do
  1178 + author = Author.includes(:posts).references(:posts).order(:name).where('posts.title IS NOT NULL').first
  1179 + assert_equal authors(:bob), author
  1180 + end
1176 1181 end
17 activerecord/test/cases/relation_test.rb
@@ -180,19 +180,32 @@ def test_references_values_dont_duplicate
180 180
181 181 class RelationMutationTest < ActiveSupport::TestCase
182 182 class FakeKlass < Struct.new(:table_name, :name)
  183 + def quoted_table_name
  184 + %{"#{table_name}"}
  185 + end
183 186 end
184 187
185 188 def relation
186   - @relation ||= Relation.new FakeKlass, :b
  189 + @relation ||= Relation.new FakeKlass.new('posts'), :b
187 190 end
188 191
189   - (Relation::MULTI_VALUE_METHODS - [:references, :extending]).each do |method|
  192 + (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order]).each do |method|
190 193 test "##{method}!" do
191 194 assert relation.public_send("#{method}!", :foo).equal?(relation)
192 195 assert_equal [:foo], relation.public_send("#{method}_values")
193 196 end
194 197 end
195 198
  199 + test "#order!" do
  200 + assert relation.order!('name ASC').equal?(relation)
  201 + assert_equal ['name ASC'], relation.order_values
  202 + end
  203 +
  204 + test "#order! with symbol prepends the table name" do
  205 + assert relation.order!(:name).equal?(relation)
  206 + assert_equal ['"posts".name ASC'], relation.order_values
  207 + end
  208 +
196 209 test '#references!' do
197 210 assert relation.references!(:foo).equal?(relation)
198 211 assert relation.references_values.include?('foo')

0 comments on commit e035699

Aaron Patterson

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

Yves Senn

@tenderlove Thanks for the fix and letting me know. :heart:

Please sign in to comment.
Something went wrong with that request. Please try again.