Building query for search engine

Question

I'm writing a basic search engine for my website.

It works but I'm really unhappy with the code. I know it can't be improved but I'm lacking some experiences here. In my params from my search I get:

{"utf8"=>"✓", "authenticity_token"=>"aNY2ZVpeyZsLnIVegQPpPz/22VYx2ug+tbfu0Nzngac=", "skills"=>"3,8", "country"=>"USA", "state"=>"TX", "city"=>"Paris", "commit"=>"Search", "controller"=>"jobs", "action"=>"index"}

Here is my controller:

class JobsController < ApplicationController
  before_action :authenticate_user!, :except => [:index]

  def index
    search_jobs
  end

  def search_jobs
    search_filters = search_params
    if search_filters.present?
      skills = search_filters[:skills]
      city, state, country = search_filters[:city], search_filters[:state], search_filters[:country]
      search_jobs = Job.select(:id).joins(:skills)
      search_jobs = search_jobs.where(build_where(skills)) if skills.present?
      search_jobs = search_jobs.group("jobs.id")
      search_jobs = search_jobs.group("jobs.city, jobs.state") if city.present? || state.present?
      search_jobs = search_jobs.group("jobs.created_at")
      search_jobs = search_jobs.order("city <> '#{city}'") if city.present?
      search_jobs = search_jobs.order("state <> '#{state}'") if state.present?
      search_jobs = search_jobs.order("country <> '#{country}'") if country.present?
      search_jobs = search_jobs.order("count(*) desc")
      search_jobs = search_jobs.order('jobs.created_at DESC')
      jobs_id = search_jobs.map(&:id)
    end
    jobs = Job.includes(:company, :skills)
    jobs = jobs.where(id: jobs_id) if jobs_id.present?
    jobs = jobs.most_recent
    @jobs = jobs
  end

  def build_where(skills_filters)
    skills = skills_filters.split(", ")
    query = "'skills'.'id' = #{skills.first}"
    skills.shift
    skills.each do |skill| 
      query << " OR 'skills'.'id' = #{skill}"
    end
    return query
  end

  private

  def search_params
    params.permit(:skills, :country, :city, :state)
  end

end

Few, things:

• I'm not happy with the build_where method, but I can't think of a way to build a where dynamically based on the skills the users are looking for.
• The search_jobs seems the be a bad idea. When I run this in the developer console it looks like each line is executing a SQL query which is an issue performance wise. 10 queries to get a list of IDs seems to be terrible
• Then I'm doing 3 more queries afterward: to get all the jobs with all the information needed.

So, I think my code is not really good, but I'm not sure how I can improve it. My goal overall is

Answer 1

There are some bugs related to parameter handling:

• Splitting by ", ": In #build_where, you split the skill parameter by ", ". However, you example has just "skills"=>"3,8" — with no space after the comma. Splitting will therefore fail.

• SQL injection vulnerability: Parts of the value of the skills parameter end up in the SQL query's WHERE clause, but you never validated, escaped, or parameterized the query. You use the city, state, and country parameters with similar carelessness, interpolating values directly into an ORDER BY clause. If a user tries to specify St. John's as the city, the query will break due to the apostrophe being treated as a quotation mark. A malicious user can exploit that to do nasty things to your database.

  

---

I also suspect that

Job.select(:id).joins(:skills)

is the wrong thing to do. That produces an INNER JOIN. Any job that has no special required skills will therefore not appear in the join result. I'm guessing that a LEFT OUTER JOIN would be more appropriate.

---

As for the skills filter itself, the WHERE clause you want in SQL is

WHERE "skills"."id" IN (3, 8)

(Writing 'skills'.'id' results in much brokenness. To quote identifiers in PostgreSQL, use double-quotes. Single quotes are used to quote literal strings, not identifiers.)

I would therefore write it as

def restrict_skills(search_jobs, skills_filters)
  # Check for non-empty string consisting only of digits, commas, and spaces
  if skills_filters.match(/\A[0-9, ]+\Z/)
     search_jobs.where(%Q{"skills"."id" IN (#{skills_filters})})
  end
end

Usually, I recommend parameterizing queries. Escaping is my second choice. Validating is normally my third choice. However, here I've used validation instead of parameterizing and escaping because validation is easy: just check that the skills parameter consists of just digits, commas, and spaces. It also happens that the SQL IN clause expects comma-delimited integers here.

I've also changed the helper method to manipulate the query-building object rather than to return a fragment of SQL.