Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix activerecord query_method regression with offset into Fixnum #5390

Merged
merged 1 commit into from

4 participants

@arunagw
Collaborator

add test to show offset query_methods on mysql & mysql2

change test to cover public API

See PR #4410

@denisj denisj 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
a2c2f40
@josevalim josevalim merged commit 1e2de2c into from
@metaskills

@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.

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 12, 2012
  1. @denisj @arunagw

    fix activerecord query_method regression with offset into Fixnum

    denisj authored arunagw committed
    add test to show offset query_methods on mysql & mysql2
    
    change test to cover public API
This page is out of date. Refresh to see the latest.
View
2  activerecord/lib/active_record/relation/query_methods.rb
@@ -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?
View
4 activerecord/test/cases/finder_test.rb
@@ -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)
Something went wrong with that request. Please try again.