Permalink
Browse files

fix activerecord query_method regression with offset into Fixnum

add test to show offset query_methods on mysql & mysql2

change test to cover public API
  • Loading branch information...
1 parent 0533ee4 commit a2c2f406612a1855fbc6fe816cf3e15b4ef531d3 @denisj denisj committed with arunagw Jan 11, 2012
@@ -329,7 +329,7 @@ def build_arel
arel.having(*@having_values.uniq.reject{|h| h.blank?}) unless @having_values.empty?
arel.take(connection.sanitize_limit(@limit_value)) if @limit_value
- arel.skip(@offset_value) if @offset_value
+ arel.skip(@offset_value.to_i) if @offset_value
arel.group(*@group_values.uniq.reject{|g| g.blank?}) unless @group_values.empty?
@@ -1185,6 +1185,10 @@ def test_find_one_message_with_custom_primary_key
end
end
+ def test_finder_with_offset_string
+ assert_nothing_raised(ActiveRecord::StatementInvalid) { Topic.find(:all, :offset => "3") }
+ end
+
protected
def bind(statement, *vars)
if vars.first.is_a?(Hash)

3 comments on commit a2c2f40

Contributor

metaskills replied Apr 13, 2012

@tenderlove Can you look at this. I feel like we keep going back and forth with calling #to_i on offset values. I have a test in the SQL Server adapter that is now failing where we were testing to make sure SQL literals could be use for an offset, like so.

Book.all :limit => 3, :offset => Arel.sql('SELECT 8 AS [count]')

I found this commit via #4409 and would have thought there were test in AR for it to allow order to not just be munged by calling #to_i on it.

Contributor

metaskills replied Apr 13, 2012

I can confirm that the above test worked in ActiveRecord 3.2.2 and the regression was introduced in 3.2.3.

Contributor

metaskills replied Apr 13, 2012

FYI, there is a test named test_limit_should_allow_sql_literal for limit to accept a literal. Not sure what happened to the offset.

Please sign in to comment.