Partially updated Search API spec

- Using sinatra-param to validate parameters instead of waiting for search failure
- Consolidated some code with shared_examples

.ruby-version

+1.9.3-p551

Gemfile

@@ -10,6 +10,7 @@ gem 'bundler'
 gem 'rake'
 
 gem 'sinatra'
+gem 'sinatra-param', '~> 1.4'
 
 gem 'yajl-ruby'
 

Gemfile.lock

@@ -153,6 +153,8 @@ GEM
       rack (~> 1.3, >= 1.3.6)
       rack-protection (~> 1.2)
       tilt (~> 1.3, >= 1.3.3)
+    sinatra-param (1.4.0)
+      sinatra (~> 1.3)
     slop (3.3.2)
     thor (0.16.0)
     thread_safe (0.3.5)
@@ -214,6 +216,7 @@ DEPENDENCIES
   rs_voteable_mongo!
   rspec (~> 2.11.0)
   sinatra
+  sinatra-param (~> 1.4)
   unicorn
   webmock (~> 1.22)
   will_paginate_mongoid (~> 2.0)

api/search.rb

@@ -126,16 +126,22 @@ def get_threads(context, group_ids, local_params, search_text)
   result_obj.to_json
 end
 
+error Sinatra::Param::InvalidParameterError do
+  # NOTE (CCB): The current behavior of the service is to return a seemingly positive response
+  # for an invalid request. In the future the API's contract should be modified so that HTTP 400
+  # is returned. This informs the client that the request was invalid, rather than having to guess
+  # about an empty response body.
+  [200, '{}']
+end
+
 get "#{APIPREFIX}/search/threads" do
+  param :text, String, required: true
+  param :context, String, default: 'course'
+  param :sort_order, String, in: %w(asc desc), transform: :downcase
+  param :sort_key, String, in: %w(activity comments date votes), transform: :downcase
+
   local_params = params # Necessary for params to be available inside blocks
   group_ids = get_group_ids_from_params(local_params)
-  context = local_params["context"] ? local_params["context"] : "course"
-  search_text = local_params["text"]
-  if !search_text
-    '{}'
-  else
 
-
-    get_threads(context, group_ids, local_params, search_text)
-  end
+  get_threads(params[:context], group_ids, local_params, params[:text])
 end

app.rb

@@ -30,6 +30,9 @@ end
 application_yaml = ERB.new(File.read("config/application.yml")).result()
 CommentService.config = YAML.load(application_yaml).with_indifferent_access
 
+# Raise sinatra-param exceptions so that we can process, and respond to, them appropriately
+set :raise_sinatra_param_exceptions, true
+
 Mongoid.load!("config/mongoid.yml", environment)
 Mongoid.logger.level = Logger::INFO
 Mongo::Logger.logger.level = ENV["ENABLE_MONGO_DEBUGGING"] ? Logger::DEBUG : Logger::INFO

spec/api/search_spec.rb

 require 'spec_helper'
 require 'unicode_shared_examples'
 
-describe "app" do
-  describe "search" do
+describe 'app' do
+  describe 'search' do
     include_context 'search_enabled'
 
     before (:each) { set_api_key_header }
-    let(:author) { create_test_user(42) }
-    let(:course_id) { "test/course/id" }
+    let(:author) { create(:user) }
+    let(:course_id) { 'test/course/id' }
 
     def get_result_ids(result)
-      result["collection"].map { |t| t["id"] }
+      result['collection'].map { |t| t['id'] }
     end
 
     describe "GET /api/v1/search/threads" do
-      def assert_empty_response
-        last_response.should be_ok
-        result = parse(last_response.body)
-        result.should == {}
+      shared_examples_for 'response for invalid request' do
+        before (:each) { refresh_es_index }
+        subject { get '/api/v1/search/threads', {course_id: course_id}.merge!(parameters) }
+
+        it { should be_ok }
+        it { should be_an_empty_response }
       end
 
-      it "returns an empty result if text parameter is missing" do
-        get "/api/v1/search/threads", course_id: course_id
-        assert_empty_response
+      context 'without text parameter' do
+        let(:parameters) { {} }
+        it_behaves_like 'response for invalid request'
       end
 
-      it "returns an empty result if sort key is invalid" do
-        get "/api/v1/search/threads", course_id: course_id, text: "foobar", sort_key: "invalid", sort_order: "desc"
-        assert_empty_response
+      context 'with an invalid sort key' do
+        let(:parameters) { {text: 'foobar', sort_key: 'invalid', sort_order: 'desc'} }
+        it_behaves_like 'response for invalid request'
       end
 
-      it "returns an empty result if sort order is invalid" do
-        get "/api/v1/search/threads", course_id: course_id, text: "foobar", sort_key: "date", sort_order: "invalid"
-        assert_empty_response
+      context 'with an invalid sort order' do
+        let(:parameters) { {text: 'foobar', sort_key: 'date', sort_order: 'invalid'} }
+        it_behaves_like 'response for invalid request'
       end
 
       describe "filtering works" do
@@ -325,21 +327,21 @@ describe "app" do
       def test_unicode_data(text)
         # Elasticsearch may not be able to handle searching for non-ASCII text,
         # so prepend the text with an ASCII term we can search for.
-        search_term = "artichoke"
-        course_id = "unicode/course"
-        thread = make_thread(author, "#{search_term} #{text}", course_id, "unicode_commentable")
-        make_comment(author, thread, text)
-        # Elasticsearch does not necessarily make newly indexed content
-        # available immediately, so we must explicitly refresh the index
+        search_term = 'artichoke'
+
+        thread = create(:comment_thread, body: "#{search_term} #{text}")
+        create(:comment, comment_thread: thread, body: text)
         refresh_es_index
-        get "/api/v1/search/threads", course_id: course_id, text: search_term
-        last_response.should be_ok
-        result = parse(last_response.body)["collection"]
-        result.length.should == 1
-        check_thread_result_json(nil, thread, result.first)
+
+        get '/api/v1/search/threads', course_id: thread.course_id, text: search_term
+
+        expect(last_response).to be_ok
+        json = parse(last_response.body)['collection']
+        expect(json.length).to eq 1
+        check_thread_result_json(nil, thread, json.first)
       end
 
-      include_examples "unicode data"
+      include_examples 'unicode data'
     end
   end
 end

spec/spec_helper.rb

@@ -16,6 +16,7 @@ require 'yajl'
 require 'support/database_cleaner'
 require 'support/elasticsearch'
 require 'support/factory_girl'
+require 'support/matchers'
 require 'webmock/rspec'
 
 WebMock.allow_net_connect!

spec/support/elasticsearch.rb

@@ -11,6 +11,7 @@ RSpec.shared_context 'search_enabled' do
   end
 
   before(:each) do
+    TaskHelpers::ElasticsearchHelper.delete_index(Content::ES_INDEX_NAME)
     index = TaskHelpers::ElasticsearchHelper.create_index
     TaskHelpers::ElasticsearchHelper.move_alias(Content::ES_INDEX_NAME, index)
   end

spec/support/matchers.rb

+require 'rspec/expectations'
+
+RSpec::Matchers.define :be_an_empty_response do
+  match do |actual|
+    actual.body == '{}'
+  end
+end