Best way to move this SQL out of the Controller?

rubyonrails-talk
1

I’ve got a fairly long and complicated query method in my Controller that I think should mostly be moved into the model. Is that best practice or does this kind of query logic make sense in the Controller? Would it move into the Orders model even though it joins a bunch of other tables?

A lot of the logic is in default params. Is best practice to just forward the params to a model function, or should they be teased apart here in the controller before passing them down?

Any other changes that make sense to clean this up?

  def query
    safe_params = orders_query_params
    per_page = safe_params[:per_page] || 10
    status_query = safe_params[:status].present? ? 'orders.status in (?)' : 'true'
    tags_query = safe_params[:tags].present? ? 'tags.name in (?)' : 'true'
    neg_status_query = safe_params[:negative_status].present? ? 'orders.status not in (?)' : 'true'
    neg_tags_query_text = 'orders.id not in (select order_tags.order_id from order_tags left outer join tags on order_tags.tag_id = tags.id where tags.name in (?))'
    neg_tags_query = safe_params[:negative_tags].present? ? neg_tags_query_text : 'true'
    order_events_query = 'left outer join (select order_events.order_id, MAX(order_events.created_at) created_at FROM order_events group by order_events.order_id) order_events on order_events.order_id = orders.id'
    country_query = safe_params[:countries].present? ? 'countries.iso in (?)' : 'true'
    neg_country_query = safe_params[:negative_countries].present? ? 'countries.iso not in (?)' : 'true'
    created_after_query = safe_params[:created_after].blank? ? 'true' : 'orders.created_at >= ?'
    created_before_query = safe_params[:created_before].blank? ? 'true' : 'orders.created_at < ?'
    search_query = 'true'

    if safe_params[:search].present? && safe_params[:search].length > 0
      search_query = SearchQueryBuilder.new(safe_params[:search])
                                       .or_clauses([
                                        'merchants.login ilike ?',
                                        'merchants.name ilike ?',
                                        'users.email ilike ?',
                                        'orders.send_to_email_index = ?',
                                        'orders.send_to_email_domain_index = ?',
                                        'orders.external_receipt_id ilike ?',
                                        'orders.short_code ilike ?',
                                        'countries.iso ilike ?',
                                        'countries.iso3 ilike ?',
                                        'countries.iso_name ilike ?'
                                       ])
                                       .id_clauses([
                                        'orders.id = ?',
                                        'users.id = ?'
                                       ])
                                       .create
    end

    query = Order.includes(:user, :tags, merchant: [:currency], property: [address: [:country]])
                 .joins(order_events_query)
                 .where(status_query, safe_params[:status])
                 .where(country_query, safe_params[:countries])
                 .where(tags_query, safe_params[:tags])
                 .where(neg_status_query, safe_params[:negative_status])
                 .where(neg_tags_query, safe_params[:negative_tags])
                 .where(neg_country_query, safe_params[:negative_countries])
                 .where(created_after_query, safe_params[:created_after])
                 .where(created_before_query, safe_params[:created_before])
                 .where(search_query)
                 .references(:merchants, :users, :tags, :order_tags, :order_events, :countries)
    count = query.count
    pages = (count.to_f / per_page).ceil

    if safe_params[:order_by_attribute].present?
      order_by = Arel.sql("#{safe_params[:order_by_attribute]} #{safe_params[:order_by_direction]} NULLS LAST")
    end

    orders = query.paginate(page: safe_params[:current_page], per_page: per_page)
                  .order(order_by)
                  .map do |order|

      structure_order(order)
    end

    render json: { orders: orders, pages: pages, total: count }.to_json
  end
2

You might consider something like ransack. It is basically an off-the-shelf gem that does what you propose. It transforms request params into an AR search. It has defined conventions for various the param names to indicate the type of search operations. Since it is an object you can even use the search when building your search form but you don’t have to use the form stuff. I use ransack on my API endpoints to give flexible querying abilities to my APIs.

3

I’d expand the scope of the SearchQueryBuilder into a SearchQuery which returns the final Order relation. Pass in params, get an Order relation which you can display / paginate as needed. All the SQL stuff then gets wrapped into a query object which can be easily unit tested and refactored.

4

Agree with ferngus on using a Query Object. Here are a few articles I found useful on query objects:

I also like using a Form Object to handle complicated search forms, giving an ActiveRecord-like place to define the forms’ attributes and validations. Since your query is complicated, you could still call the Query Object from the Form Object to build the actual relation after validation passes:

# app/forms/order_search_form.rb
# Behaves almost like an ActiveRecord model
class OrderSearchForm
  include ActiveModel::Model
  include ActiveModel::Attributes

  # Attributes
  attribute :merchant_name, :string
  attribute :email, :string
  attribute :tags, :string # Caveat: no array attribute support in ActiveModel::Model

  # Validations
  validates :email, presence: true

  # Returns a relation, applying the search to the provided relation.
  # If no relation is given, defaults to searching all orders.
  def apply(orders: Order.all)
    return orders unless valid?

    OrderSearchQuery.new(attributes).relation(orders: orders)
  end

If you’re accepting user-provided params, this gives you a chance to, for example, give the user a nice validation error when they provide a :created_after that falls after :created_before.

2 Likes
5

Those are really great resources and exactly what I was looking for. Thank you!