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

  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)
    jobs = Job.includes(:company, :skills)
    jobs = jobs.where(id: jobs_id) if jobs_id.present?
    jobs = jobs.most_recent
    @jobs = jobs

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


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


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


1 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.

    "Did you really name your son Robert'); DROP TABLE Student;--?" "Oh. Yes. Little Bobby Tables, we call him." "Well we've lost this year's student records. I hope you're happy."

I also suspect that


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})})

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.

  • \$\begingroup\$ thanks a lot. Regarding INNER JOIN, I want to display only the jobs that have the required skills in the params. So I'm guessing that .joins(:skills) is correct. For the WHERE "skills"."id" IN (3, 8) this will on return the jobs with skills 3 and 8 but I want jobs with skills 3 or 8. \$\endgroup\$
    – bl0b
    Commented Oct 26, 2014 at 8:55
  • \$\begingroup\$ WHERE id IN (3, 8) is equivalent to WHERE id=3 OR id=8. \$\endgroup\$ Commented Oct 26, 2014 at 8:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.