8fa97b9513b66cccc2db021782863b3b

I want to construct a search query for the Rails acts_as_ferret plugin, but as you can see I'm... a noob. Surely there is a more elegant way to solve this problem.
http://www.railsenvy.com/2007/2/19/acts-as-ferret-tutorial

1
2
3
4
5
6
7
8
9
10
11
12
13
def construct_search_query
    query = '*' 
    query << 'AND (' << params[:search_all] << ')' if params[:search_all] && params[:search_all].length > 3
    query << ' AND (' << params[:search_any].split(' ').join('OR') <<')' if params[:search_any] && params[:search_any].length > 3
    query<< ' AND (\"' << params[:search_exact] <<'\")' if params[:search_exact] && params[:search_exact].length > 3
    query << ' AND NOT (' << params[:search_exclude] <<')' if params[:search_exclude] && params[:search_exclude].length > 3
    query << ' AND (title:' << params[:search_title] <<')' if params[:search_title] && params[:search_title].length > 3
    query << ' AND (author_names:' << params[:search_author] <<')' if params[:search_author] && params[:search_author].length > 3
    query << ' AND (publisher_name:' << params[:search_publisher] <<')' if params[:search_publisher] && params[:search_publisher].length >3
    query << ' AND (description:' << params[:search_description] <<')' if params[:search_description] && params[:search_description].length > 3
    query << ' AND (tags:' << params[:search_tags] <<')' if params[:search_tags] && params[:search_tags].length > 3    
    return query
  end  

Refactorings

No refactoring yet !

4ebfc8d183472ac3cffff96be3e0ec68

PabloBM

July 7, 2008, July 07, 2008 08:21, permalink

No rating. Login to rate!

Not sure of what you are trying to do there, as that doesn't look like valid SQL to me, but you surely can do a bit better at least in the second half of the method. See example

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
def construct_search_query
	query = '*' 
	query << 'AND (' << params[:search_all] << ')' if params[:search_all] && params[:search_all].length > 3
	query << ' AND (' << params[:search_any].split(' ').join('OR') <<')' if params[:search_any] && params[:search_any].length > 3
	query<< ' AND (\"' << params[:search_exact] <<'\")' if params[:search_exact] && params[:search_exact].length > 3
	query << ' AND NOT (' << params[:search_exclude] <<')' if params[:search_exclude] && params[:search_exclude].length > 3

	valid_params = {
		:search_title => 'title',
		:search_author => 'author_names',
		:search_publisher => 'publisher_name',
		:search_description => 'description',
		:search_tags => 'tags'
	}
	valid_params.each do |key, field|
		query << " AND (title:#{params[key]})" if params[key] && params[key].length > 3
	end
	
    query
end  
5170ca260dbd2cdfd5a887a4dba7636f

Jeremy

July 7, 2008, July 07, 2008 13:44, permalink

No rating. Login to rate!

Slight correction to Pablo's refactoring: need to parameterize the "title" portion of the query. I also extracted the logic that determines if a parameter should be considered to the method #valid_param? to DRY things up a bit. Also using string interpolation to build all of the parts of the query, which I think is easier to read than concatenation.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
def valid_param?(key)
  params[key] && params[key].length > 3
end

def construct_search_query
  query = '*' 
  
  query << "AND (#{params[:search_all]})" if valid_param?(:search_all)
  query << " AND (#{params[:search_any].split(' ').join('OR')})" if valid_param?(:search_any)
  query << " AND (\"#{params[:search_exact]}\")" if valid_param?(:search_exact)
  query << " AND NOT (#{params[:search_exclude]})" if valid_param?(:search_exclude)
  
  { :search_title => 'title',
    :search_author => 'author_names',
    :search_publisher => 'publisher_name',
    :search_description => 'description',
    :search_tags => 'tags' }.each do |param, field|
    query << " AND (#{field}:#{params[param]})" if valid_param?(param)
  end
  
  query
end  
4ebfc8d183472ac3cffff96be3e0ec68

PabloBM

July 7, 2008, July 07, 2008 13:47, permalink

No rating. Login to rate!

Ooopsie. Indeed, Jeremy. Thanks for pointing out.

0c39b828636367fc6e22b7be8c803c74

David Calavera

July 7, 2008, July 07, 2008 14:02, permalink

3 ratings. Login to rate!
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
def construct_search_query
    search = {
      :search_all => ' AND (%s)',
      :search_any => ' AND (%s)',
      :search_exact => ' AND (\"%s\")',
      :search_exclude => ' AND NOT (%s)'
    }
    valid_params = {
      :search_title => 'title',
      :search_author => 'author_names',
      :search_publisher => 'publisher_name',
      :search_description => 'description',
      :search_tags => 'tags'
    }

    query = '*'
    params = params.reject {|k, v| v.nil? || v.length <= 3}
    params.keys.each do |k|
      if (search[k])
        query << search[k] % params[k]
      else
        query << " AND (%s:%s)"  % [valid_params[k], params[k]] 
      end  
    end 

    query
  end
1e8f141e7857d397d8020ed3b759e88a

Maciej Piechotka

July 7, 2008, July 07, 2008 14:55, permalink

No rating. Login to rate!

DON'T DO IT!
It is broken by design - you can easily inject SQL Code into it.

PS. What it has in common with ferret?

8fa97b9513b66cccc2db021782863b3b

foxdemon

July 8, 2008, July 08, 2008 07:15, permalink

No rating. Login to rate!

Want to add rapid full-text searching to your Rails app? Acts_As_Ferret is the way to go.
http://www.railsenvy.com/2007/2/19/acts-as-ferret-tutorial

This method constructs a search querry for ferret (not SQL), so no SQL injection.

Your refactoring





Format Copy from initial code

or Cancel