J Seabolt
J Seabolt

Reputation: 2998

Search not finding correct results

I am learning how to create rails search bars. I have a model named movies. I would like to search movies by title and display the results. I am doing this in the "new" view, not "index." I am also using will_paginate on my results. Currently, my search presents no results, even if the movie IS present. If I don't do the search, I still see all of my movies... but obviously, I need the search feature to work too. Can someone help?

new movie view:

<div class="row">
<div class="col-xs-12">
    <h2 class="white">Home</h2>
    <hr />
    <h4 class="white">Add Movie</h4>
    <%= form_for @movie, class: 'form-horizontal' do |m| %>
        <div class="form-group">
            <label for="title" class="white">Title: </label><br/>
            <%= m.text_field :title,  class: 'form-control' %>
        </div>
        <div class="form-group"> 
            <label for="title" class="white">Year: </label><br/>
            <%= m.text_field :year, class: 'form-control' %>
        </div>
        <div class="form-group">
            <%= m.submit %> 
        <div class="form-group">
    <% end %>
</div>  
</div>

<hr />

<div class="row">
<div class="col-xs-12">
<h4 class="white">Database</h4>
<%= form_tag new_movie_path, :method=> 'get' do %>
    <%= text_field_tag :search, params[:search] %>
    <%= submit_tag "Search" %>
<% end %>
<br />
    <% if @movies.exists? %>
    <div class="bg_white">
        <table class="table table-hover table-striped">
        <tr>
            <th>
                Title
            </th>
            <th>
                Status
            </th>
            <th>
                Year
            </th>
            <th>
                Delete
            </th>
            <th>
                Edit
            </th>
        </tr>
        <% @movies.each do |m| %>
        <tr>
            <td>
                <%= link_to m.title, new_movie_rental_path(m) %>
            </td> 
            <td> 
                <%= m.status %>
            </td>
            <td>
                <%= m.year %>
            </td>   
            <td> 
                <%= link_to "Delete", movie_path(m), method: :delete %>
            </td>  
            <td>
                <%= link_to "Edit", edit_movie_path(m) %>
            </td>  
        </tr>
        <% end %>
        </table>
    </div>
    <%= will_paginate @movies, class: 'white' %>
    <% else %>
        <p class="white">No movies have been entered</p>
    <% end %> 
</div>  
</div>
<div class="row">
<div class="col-xs-12">
    <hr />
    <%= link_to "Add Customer", new_customer_path, class: 'white' %>
</div>
</div>
<br /><br />

Movie model:

class Movie < ApplicationRecord
has_many :rentals, dependent: :destroy 


def status 
    if self.rentals.empty?
        return "In Stock"
    else
        self.rentals.order(borrowed_on: :desc).each do |x|
            if !x.returned_on.nil?
                return "In Stock"
            else 
                return "Rented"
            end
        end
    end
end

def self.search(search)
    if search
        @movies = Movie.where(["title","%#{[:search]}%"])
    else
        all
    end 
end 

end

Movies controller

 def new
    @movie = Movie.new
    @movies = Movie.search(params[:search]).order(title: :asc).paginate(:page => params[:page], :per_page => 15) 
 end

Upvotes: 0

Views: 67

Answers (1)

mlabarca
mlabarca

Reputation: 806

I'd say the problem is on this this class method you have as search scope:

def self.search(search)
  if search
    @movies = Movie.where(["title","%#{[:search]}%"])
  else
    all
  end 
end

I guess you were attempting to interpolate search params into the query, but in this case using #{} you are passing the symbol search instead of interpolating it. Thus your query ends up being SELECT 1 AS one FROM "movies" WHERE (title). But More importantly, you are leaving yourself VERY vulnerable to SQL injection by interpolating directly into the query. What if a user enters into the search field something like ')DROP TABLE MOVIES ('? I'd rewrite it like this:

def self.search(search = nil)
  if search
    Movie.where('title LIKE :search', search: "%#{search}%")
  else
    Movie.scoped
  end
end

Notice also how i'm returning Movie.scoped instead of all when search is empty; that returns a relation, .all returns an array of movie objects, which you don't want if you will call order and other chains on it.

Read on sql injection and param interpolation: http://rails-sqli.org/#where http://api.rubyonrails.org/v5.0.0.1/classes/ActiveRecord/QueryMethods.html#method-i-where

Upvotes: 1

Related Questions