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 !
PabloBM
July 7, 2008, July 07, 2008 08:21, permalink
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
Jeremy
July 7, 2008, July 07, 2008 13:44, permalink
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
PabloBM
July 7, 2008, July 07, 2008 13:47, permalink
Ooopsie. Indeed, Jeremy. Thanks for pointing out.
David Calavera
July 7, 2008, July 07, 2008 14:02, permalink
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
Maciej Piechotka
July 7, 2008, July 07, 2008 14:55, permalink
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?
foxdemon
July 8, 2008, July 08, 2008 07:15, permalink
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.
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