Skip to content

Loading…

Support SQL sanitization in AR::QueryMethods#order #13008

Merged
merged 1 commit into from

8 participants

@ktheory

Add support for sanitizing arrays in SQL ORDER clauses.

This is useful when using MySQL ORDER BY FIELD() to return records in
a predetermined way.

  Tag.order(['field(id, ?)', [1,3,2]].to_sql
  # => SELECT "tags".* FROM "tags"   ORDER BY field(id, 1,3,2)

Prior to this, developers must be careful to sanitize #order arguments
themselves.

@ktheory ktheory Support SQL sanitization in AR::QueryMethods#order
Add support for sanitizing arrays in SQL ORDER clauses.

This is useful when using MySQL `ORDER BY FIELD()` to return records in
a predetermined way.

```ruby
  Tag.order(['field(id, ?', [1,3,2]].to_sql
  # => SELECT "tags".* FROM "tags"   ORDER BY field(id, 1,3,2)
```

Prior to this, developers must be careful to sanitize `#order` arguments
themselves.
b173733
@ktheory

This issue came up previously in #5949, where @dazuma and @mildmojo supported it. That patch no longer merges cleanly.

@rafaelfranca
Ruby on Rails member
@robin850 robin850 commented on the diff
activerecord/lib/active_record/relation/query_methods.rb
@@ -1029,6 +1029,13 @@ def validate_order_args(args)
end
def preprocess_order_args(order_args)
+ order_args.map! do |arg|
+ if arg.is_a?(Array) && arg.first.to_s.include?('?')
@robin850 Ruby on Rails member

It may be a stupid question but what's the point of checking with include? instead of arg.first.to_s == "?" please ?

@igor04
igor04 added a note

because the first element can contain not only "?" char, like in our case 'field(id, ?)'

@robin850 Ruby on Rails member

Ah right, sorry for the misreading. Thank you!

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

nice to have this :+1:

@mmozuras

Ran into this today, would be great to have this :+1:

@JacobEvelyn

Same!

@sgrif sgrif was assigned by rails-bot
@sgrif sgrif merged commit b173733 into rails:master

1 check passed

Details default The Travis CI build passed
@sgrif sgrif added a commit that referenced this pull request
@sgrif sgrif Add a changelog entry for #13008 7f41321
@ktheory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 22, 2013
  1. @ktheory

    Support SQL sanitization in AR::QueryMethods#order

    ktheory committed
    Add support for sanitizing arrays in SQL ORDER clauses.
    
    This is useful when using MySQL `ORDER BY FIELD()` to return records in
    a predetermined way.
    
    ```ruby
      Tag.order(['field(id, ?', [1,3,2]].to_sql
      # => SELECT "tags".* FROM "tags"   ORDER BY field(id, 1,3,2)
    ```
    
    Prior to this, developers must be careful to sanitize `#order` arguments
    themselves.
View
7 activerecord/lib/active_record/relation/query_methods.rb
@@ -1029,6 +1029,13 @@ def validate_order_args(args)
end
def preprocess_order_args(order_args)
+ order_args.map! do |arg|
+ if arg.is_a?(Array) && arg.first.to_s.include?('?')
@robin850 Ruby on Rails member

It may be a stupid question but what's the point of checking with include? instead of arg.first.to_s == "?" please ?

@igor04
igor04 added a note

because the first element can contain not only "?" char, like in our case 'field(id, ?)'

@robin850 Ruby on Rails member

Ah right, sorry for the misreading. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ klass.send(:sanitize_sql, arg)
+ else
+ arg
+ end
+ end
order_args.flatten!
validate_order_args(order_args)
View
5 activerecord/test/cases/relations_test.rb
@@ -231,6 +231,11 @@ def test_finding_with_complex_order
assert_equal 3, tags.length
end
+ def test_finding_with_sanitized_order
+ query = Tag.order(["field(id, ?)", [1,3,2]]).to_sql
+ assert_match(/field\(id, 1,3,2\)/, query)
+ end
+
def test_finding_with_order_limit_and_offset
entrants = Entrant.order("id ASC").limit(2).offset(1)
Something went wrong with that request. Please try again.