0218fde3a78fadbadb566bdb40d7b0dd

I found myself in a situation where I had a nice tidy model method encapsulating a complex finder. The finder included conditions and other parameters. I'm using the method within proxy method definitions found in other models, so I'd like to be able to pass options down to the method for use on the finder. This includes conditions.

So I wrote up a method that merges two sets of conditions. Conditions, as you know, can be defined either as a hash, an array or a string. I account for most of those scenarios, and error on the rest

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
# Merges two sets of condition options for ActiveRecord::Base find calls
def merge_conditions(base_conditions, new_conditions, boolean_operator="AND")
  return base_conditions if new_conditions.blank?
  return new_conditions  if base_conditions.blank?
  
  case base_conditions
  when Hash
    case new_conditions
    when Hash
      base_conditions.merge(new_conditions)
    else
      raise "Hash conditions can only be merged with other hash conditions"
    end
  when Array
    case new_conditions
    when Array
      ["#{base_conditions[0]} #{boolean_operator} #{new_conditions[0]}", base_conditions[1..-1], new_conditions[1..-1]].flatten
    when String
      conditions = base_conditions.dup
      conditions[0] = "#{base_conditions[0]} #{boolean_operator} #{new_conditions}"
      conditions
    else
      raise "Array conditions can only be merged with array or string conditions"
    end
  when String
    case new_conditions
    when String
      "#{base_conditions} #{boolean_operator} #{new_conditions}"
    when Array
      conditions = new_conditions.dup
      conditions[0] = "#{base_conditions} #{boolean_operator} #{new_conditions[0]}"
      conditions
    else
      raise "String conditions can only be merged with string or array conditions"
    end
  end
end

Refactorings

No refactoring yet !

Bfec5f7d1a4aaafc5a2451be8c42d26a

macournoyer

October 16, 2007, October 16, 2007 01:04, permalink

3 ratings. Login to rate!

ActiveRecord#sanitize_sql already does all this magic

1
2
3
4
5
class ActiveRecord::Base
  def merge_conditions(base_conditions, new_conditions, boolean_operator="AND")
    "(#{sanitize_sql(base_conditions)}) #{boolean_operator} (#{sanitize_sql(new_conditions)})"
  end
end
0a3ce535ca2e42e3b6f7857b3f0b4bef

erockenjew.myopenid.com

October 16, 2007, October 16, 2007 02:44, permalink

2 ratings. Login to rate!

I wrote an extremely small class to deal with this same problem, which makes use of ActiveRecord::Base.sanitize_sql, and makes it a little more apparent in the code what is happening. Its also really easy to test.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
# used for building a conditions string for an ActiveRecord find
# 
# usage:
#   
#   conditions = Conditioner.new
#   conditions << ['open = ? AND user_id = ?', 1, @user.id]
#   conditions << ['search_term LIKE ? ', "%#{params[:search_term]}"]
#   
#   Opportunity.find(:all, :conditions => conditions.to_conditions)
#   # select * from opportunity WHERE open = 1 AND user_id = 1 AND search_term LIKE '%hello%'
class Conditioner
  
  attr_reader :conditions 

  def initialize
    @conditions = []
  end
  
  # adds an array to the conditioner
  #
  # usage:
  #
  #   conditions = Conditioner.new
  #   conditions << ['open = ? AND user_id = ?', 1, @user.id]
  #
  def <<(con)
    @conditions << ActiveRecord::Base.send(:sanitize_sql_array, con)
  end
  
  alias_method :add, :<<
  
  
  # outputs the conditions in a string
  #
  # params:
  #
  #  * seperator (choose either AND or OR, defaults to AND)
  def to_conditions(seperator="AND")
    @conditions.join(" #{seperator} ")
  end
  
  
end
0218fde3a78fadbadb566bdb40d7b0dd

Barry Hess

October 16, 2007, October 16, 2007 03:47, permalink

No rating. Login to rate!

Thanks, all.

I really like that class-based version, but it's a little heavy for my purposes. Basically, I'm not real fond of instantiating an object to do this. My refactor takes macournoyer's and adds a couple blank condition checks. Without those, a blank condition may result in SQL like "(id=1) AND ()", which I'm pretty sure fails in MySQL.

For a bonus, I also added a function that takes a set of conditions plus a typical options hash, pulls the conditions out of the hash and merges the conditions for return. You'd use this if you want to merge conditions and then also merge the current method's options with options passed into it without overwriting the newly built conditions.

1
2
3
4
5
6
7
8
9
10
11
12
13
# Merges two sets of condition options for ActiveRecord::Base find calls
def merge_conditions(base_conditions, new_conditions, boolean_operator="AND")
  return base_conditions if new_conditions.blank?
  return new_conditions  if base_conditions.blank?

  "(#{sanitize_sql(base_conditions)}) #{boolean_operator} (#{sanitize_sql(new_conditions)})"
end

# Permanently pulls any conditions found in options and merges them with the base set of conditions
def pull_conditions_and_merge!(base_conditions, options, boolean_operator="AND")
  option_conditions = options.delete(:conditions)
  merge_conditions(base_conditions, option_conditions, boolean_operator)
end
0a3ce535ca2e42e3b6f7857b3f0b4bef

erockenjew.myopenid.com

October 16, 2007, October 16, 2007 04:00, permalink

No rating. Login to rate!

Or you could just switch to using ruby-sequel instead of ActiveRecord. ruby-sequel defines a flued interface for building queries, and queries are 'lazy-loaded' which means they don't get executed until they need to be (such as when you call .each)

1
2
DB[:users].filter(:country => "US").or("confirmed = ?", 1).filter("ids NOT IN (?)", [1, 10, 100])
#=> SELECT * FROM users WHERE country = 'US' OR confirmed = 1 AND ids NOT IN (1,10,100)
0a3ce535ca2e42e3b6f7857b3f0b4bef

erockenjew.myopenid.com

October 16, 2007, October 16, 2007 04:02, permalink

No rating. Login to rate!

BTW, i know switching to ruby-sequel isn't actually an acceptable solution, just wanted to point out how the :conditions => in ActiveRecord::Base.find is not the best implementation of building SQL statements.

517a3f8ee4a9da153ee1b4959446e0ac

Teflon Ted

October 18, 2007, October 18, 2007 12:51, permalink

No rating. Login to rate!

Mac and Barry, I just wanted to warn that in your respective refactorings you aren't applying sanitize_sql to the value of boolean_operator which could leave you open to an SQL injection vulnerability if that value were being passed in from a form or URL.

Your refactoring





Format Copy from initial code

or Cancel