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 !
macournoyer
October 16, 2007, October 16, 2007 01:04, permalink
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
erockenjew.myopenid.com
October 16, 2007, October 16, 2007 02:44, permalink
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
Barry Hess
October 16, 2007, October 16, 2007 03:47, permalink
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
erockenjew.myopenid.com
October 16, 2007, October 16, 2007 04:00, permalink
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)
erockenjew.myopenid.com
October 16, 2007, October 16, 2007 04:02, permalink
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.
Teflon Ted
October 18, 2007, October 18, 2007 12:51, permalink
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.
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