Ruby on rails complex select statement...there has to be a better way!

Question

I am building a query that could end up look like this:

SELECT "patients".* FROM "patients" 
INNER JOIN "users" ON "users"."id" = "patients"."user_id" 
WHERE (users.username LIKE '%Bob%' AND users.last_name LIKE '%Smith%' 
AND users.active = '1' AND users.disabled = '1') 
ORDER BY users.first_name DESC LIMIT 10 OFFSET 0

Below is code to create a complex query such as:

conditions = []
where = ''
where_parameters = []
order = 'users.last_name ASC'
per_page = 20
if is_admin?
 if defined?(params[:username]) and !params[:username].blank?
 where += 'users.username LIKE ? AND '
 where_parameters.push("%#{params[:username]}%")
 end
 if defined?(params[:last_name]) and !params[:last_name].blank?
 where += 'users.last_name LIKE ? AND '
 where_parameters.push("%#{params[:last_name]}%")
 end
 if defined?(params[:active]) and !params[:active].blank?
 where += 'users.active = ? AND '
 where_parameters.push(params[:active])
 end
 if defined?(params[:disabled]) and !params[:disabled].blank?
 where += 'users.disabled = ? AND '
 where_parameters.push(params[:disabled])
 end
 if !where.empty?
 where = where[0, where.length - 5]
 conditions = [where, *where_parameters]
 end
 if defined?(params[:order_by]) and !params[:order_by].blank?
 order = params[:order_by] + ' '
 end
 if defined?(params[:order]) and !params[:order].blank?
 order += params[:order]
 end
 if defined?(params[:per_page]) and !params[:per_page].blank?
 per_page = params[:per_page].to_i
 end
 @patients = Patient.joins(:user).paginate(:all, :include =>:user, :conditions => conditions, :order => order, :page => params[:page], :per_page => per_page) 

Is there a better way to do this, this is just ugly?

Answer 1

First of all be very careful when building queries from request data. When building your where string, you're using parametrized queries, so that's fine. But in your order string, you're building actual SQL code from data you directly take out of params. Do NOT do that. This is a big risk of SQL injection.

What you should do instead is take the order string apart (splitting at the comma), then check that each element is a column name optionally followed by ASC or DESC, then put the string back together accordingly.

A method for this could look something like this:

def safe_order_from_param(collection, order_string)
 tokens = order_string.split(/ *, */)
 tokens.each do |token|
 column, direction = token.match(/^(\w+)(? +(\w+))?$/).captures
 if Patient.column_names.include? column
 # if direction is nil, it will just be turned into the empty
 # string, so no problem there
 collection = collection.order("#{column} #{direction}")
 else
 raise ArgumentError, "No such column: #{column}"
 end
 end
end

---

Second of all defined?(params[:foo]) does is check that params is defined. It does not check that params is a hash and has the key :foo. Since params will always exist, there really is no need for the check. Checking params[:foo].blank? is entirely sufficient.

---

Now to the main part of your question: the ugly building of the query string:

I assume from the fact that you're using the joins method that this is rails 3. In rails 3 options like :include, :conditions and :order are deprecated in favor of the corresponding query methods. The beauty of those methods is that they're chainable. So instead of building a query string using concatenation, it is much cleaner to just chain together multiple wheres.

if is_admin?
 @patients = Patient.joins(:user).includes(:user).order('users.last_name ASC')
 unless params[:username].blank?
 @patients = @patients.where("users.username LIKE ?", params[:username])
 end
 # same for :last_name
 unless params[:active].blank?
 @patients = @patients.where("users.active" => params[:active])
 end
 # same for disabled
 unless params[:order_by].blank?
 @patients = safe_order_from_param(@patients, params[:order_by])
 end
 # same for order
 per_page = params[:per_page].blank? ? 20 : params[:per_page].to_i
 @patients = @patients.paginate(:page => params[:page], :per_page => per_page)
end

• Chris Muench: Thank you! that was very helpful! What would I do in the case that I wanted to OR instead of AND? I guess I am probably in trouble...
• sepp2k: @chris: Yes, if you want OR, you can't just chain wheres. However you can still improve your string concatenation method by building an array instead of a string and then using join(' OR ') on it. That way you don't have to remove the trailing OR at the end like you're doing now with AND.