Permalink
Browse files

ActiveRecord: Fix eager loading so that giving a blank order clause g…

…enerates valid SQL
  • Loading branch information...
1 parent b475f74 commit 0e1d617b8b869426960ec25b62620fe1599cb5e6 @mcmire mcmire committed Jul 18, 2011
@@ -243,7 +243,7 @@ def apply_join_dependency(relation, join_dependency)
end
def construct_limited_ids_condition(relation)
- orders = relation.order_values
+ orders = relation.order_values.map(&:presence).compact
@gmile

gmile Jul 19, 2011

Contributor

They say Symbol#to_proc is not recommended anymore...

@sikachu

sikachu Jul 19, 2011

Member

Yep, symbol to proc in core code is not recommend until we get rid of 1.8.7 support. Do you mind changing that?

@mcmire

mcmire Jul 19, 2011

Contributor

Yeah..... I can. I'll take a look at it later today.

@josevalim

josevalim via email Jul 19, 2011

Member
@sikachu

sikachu Jul 19, 2011

Member

It does exists, but I think @tenderlove was saying that we should avoid it because it's slow. I even had a commit which helped him to remove symbol to proc.

@josevalim

josevalim via email Jul 19, 2011

Member
@spastorino

spastorino Jul 19, 2011

Owner

Can someone cook a pull request for this please?

values = @klass.connection.distinct("#{@klass.connection.quote_table_name table_name}.#{primary_key}", orders)
relation = relation.dup
@@ -1048,4 +1048,16 @@ def test_joins_with_includes_should_preload_via_joins
assert_not_equal 0, post.comments.to_a.count
end
end
+
+ def test_join_eager_with_empty_order_should_generate_valid_sql
+ assert_nothing_raised(ActiveRecord::StatementInvalid) do
+ Post.includes(:comments).order("").where(:comments => {:body => "Thank you for the welcome"}).first
+ end
+ end
+
+ def test_join_eager_with_nil_order_should_generate_valid_sql
+ assert_nothing_raised(ActiveRecord::StatementInvalid) do
+ Post.includes(:comments).order(nil).where(:comments => {:body => "Thank you for the welcome"}).first
+ end
+ end
end

0 comments on commit 0e1d617

Please sign in to comment.