Skip to content

Commit

Permalink
Allow symbol (i.e. quoted identifier) as safe SQL string
Browse files Browse the repository at this point in the history
`pluck(:id)` / `order(:id)` are very common use case, and passed symbol
(i.e. quoted identifier) is obviously safe argument, but
`:id.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) }` is
useless and a bit expensive operation for each such safe symbols (will
make extra 2 mutable strings, 1 array, 1 proc).

This avoids the expensive operation to such safe symbols, it makes
`pluck(:id)` / `order(:id)` itself about 9% faster.

https://gist.github.com/kamipo/11d428b57f3629a72ae89c6f21721326

Before (93e6407):

```
Warming up --------------------------------------
    users.pluck(:id)     1.217k i/100ms
users.order(:id).to_sql  1.848k i/100ms
Calculating -------------------------------------
    users.pluck(:id)     12.239k (± 8.2%) i/s -     60.850k in   5.013839s
users.order(:id).to_sql  19.111k (± 7.5%) i/s -     96.096k in   5.064450s
```

After (this change):

```
Warming up --------------------------------------
    users.pluck(:id)     1.293k i/100ms
users.order(:id).to_sql  2.036k i/100ms
Calculating -------------------------------------
    users.pluck(:id)     13.257k (± 6.9%) i/s -     65.943k in   5.002568s
users.order(:id).to_sql  20.957k (± 7.6%) i/s -    105.872k in   5.086102s
```
  • Loading branch information
kamipo committed May 28, 2019
1 parent b55f5a3 commit bc83789
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions activerecord/lib/active_record/attribute_methods.rb
Expand Up @@ -185,12 +185,14 @@ def attribute_names
/ix

def disallow_raw_sql!(args, permit: COLUMN_NAME) # :nodoc:
unexpected = args.reject do |arg|
Arel.arel_node?(arg) ||
unexpected = nil
args.each do |arg|
next if arg.is_a?(Symbol) || Arel.arel_node?(arg) ||
arg.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) }
(unexpected ||= []) << arg
end

return if unexpected.none?
return unless unexpected

if allow_unsafe_raw_sql == :deprecated
ActiveSupport::Deprecation.warn(
Expand Down

0 comments on commit bc83789

Please sign in to comment.