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