Merge pull request #225 from edx/diana/elasticsearch-update

Diana/elasticsearch update

Gemfile

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

Gemfile.lock

@@ -154,6 +154,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)
@@ -215,6 +217,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

@@ -125,14 +125,20 @@ 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_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

@@ -27,14 +27,34 @@ if ENV["ENABLE_GC_PROFILER"]
   GC::Profiler.enable
 end
 
+def get_logger(progname, threshold=nil)
+  logger = Logger.new(STDERR)
+  logger.progname = progname
+  logger.level = threshold || Logger::INFO
+  logger
+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
+
+# Setup Mongo
 Mongoid.load!("config/mongoid.yml", environment)
 Mongoid.logger.level = Logger::INFO
 Mongo::Logger.logger.level = ENV["ENABLE_MONGO_DEBUGGING"] ? Logger::DEBUG : Logger::INFO
 
-# set up i18n
+# Setup Elasticsearch
+# NOTE (CCB): If you want to see all data sent to Elasticsearch (e.g. for debugging purposes), set the tracer argument
+# to the value of a logger.
+# Example: Elascisearch.Client.new(tracer: get_logger('elasticsearch.tracer'))
+Elasticsearch::Model.client = Elasticsearch::Client.new(
+    host: CommentService.config[:elasticsearch_server],
+    logger: get_logger('elasticsearch', Logger::WARN)
+)
+
+# Setup i18n
 I18n.load_path += Dir[File.join(File.dirname(__FILE__), 'locale', '*.yml').to_s]
 I18n.default_locale = CommentService.config[:default_locale]
 I18n.enforce_available_locales = false
@@ -51,7 +71,6 @@ Dir[File.dirname(__FILE__) + '/lib/**/*.rb'].each { |file| require file }
 Dir[File.dirname(__FILE__) + '/models/*.rb'].each { |file| require file }
 Dir[File.dirname(__FILE__) + '/presenters/*.rb'].each { |file| require file }
 
-Elasticsearch::Model.client = Elasticsearch::Client.new(host: CommentService.config[:elasticsearch_server], log: false)
 
 $check_index_mapping_exists = defined?(RAKE_SEARCH_INITIALIZE) === nil || RAKE_SEARCH_INITIALIZE === false
 if $check_index_mapping_exists

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"
-        assert_empty_response
+      context 'with an invalid sort key' do
+        let(:parameters) { {text: 'foobar', sort_key: 'invalid'} }
+        it_behaves_like 'response for invalid request'
       end
 
       describe "search for updated/deleted comment/thread works" do
@@ -91,6 +93,7 @@ describe "app" do
         end
       end
 
+
       describe "filtering works" do
         let!(:threads) do
           threads = (0..34).map do |i|
@@ -375,21 +378,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/app_spec.rb

@@ -174,4 +174,17 @@ describe 'app' do
       expect(last_response.body).to include File.expand_path(__FILE__)
     end
   end
+
+  describe 'config' do
+    describe 'Elasticsearch client' do
+      subject { Elasticsearch::Model.client }
+
+      it 'has a host value set to that from application.yaml' do
+        expected = URI::parse(CommentService.config[:elasticsearch_server])
+        host = subject.transport.hosts[0]
+        host[:port] = host[:port].to_i
+        expect(URI::HTTP.build(host)).to eq expected
+      end
+    end
+  end
 end

spec/spec_helper.rb

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

spec/support/matchers.rb

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