From 2f5c81156fcbd4048e98ac693d0dd49d27a55a22 Mon Sep 17 00:00:00 2001
From: John Hensley <john@opencraft.com>
Date: Sat, 27 Jan 2018 17:43:51 -0500
Subject: [PATCH] Remove hyphen, quote from reserved characters

Remove the hyphen and double quote from the list of reserved characters
in Elastic searches, allowing more effective searches for hyphenated
terms and quoted phrases.

This introduces the possibility of Elastic rejecting malformed quoted
queries, so additional exception handling is provided for that
case. Actually presenting the more specific errors to users requires
changes in edx-platform; installing this revision of edx-search without
those changes will result in malformed queries producing the same "There
was an error, try searching again." message any search error returns.
---
 .gitignore                                  |  1 +
 search/api.py                               | 11 +++
 search/elastic.py                           | 28 ++++---
 search/tests/mock_search_engine.py          | 28 +++----
 search/tests/test_course_discovery.py       | 60 ++++++++++++++-
 search/tests/test_course_discovery_views.py |  5 +-
 search/tests/test_views.py                  | 82 ++++++++++++++++++++-
 search/tests/utils.py                       | 10 ++-
 search/views.py                             | 12 ++-
 setup.py                                    |  2 +-
 test_requirements.txt                       |  4 +-
 11 files changed, 203 insertions(+), 40 deletions(-)

diff --git a/.gitignore b/.gitignore
index 06bad32c..d387259f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -4,3 +4,4 @@
 
 .coverage
 .idea/
+.tox
\ No newline at end of file
diff --git a/search/api.py b/search/api.py
index 3d085591..1065bce8 100644
--- a/search/api.py
+++ b/search/api.py
@@ -27,6 +27,17 @@ class NoSearchEngineError(Exception):
     pass
 
 
+class QueryParseError(Exception):
+    """QueryParseError will be thrown if the query is malformed.
+
+    If a query has mismatched quotes (e.g. '"some phrase', return a
+    more specific exception so the view can provide a more helpful
+    error message to the user.
+
+    """
+    pass
+
+
 def perform_search(
         search_term,
         user=None,
diff --git a/search/elastic.py b/search/elastic.py
index 8aabcd0c..63d2921a 100644
--- a/search/elastic.py
+++ b/search/elastic.py
@@ -1,4 +1,4 @@
-""" Elatic Search implementation for courseware search index """
+""" Elastic Search implementation for courseware search index """
 import copy
 import logging
 
@@ -7,6 +7,7 @@
 from elasticsearch import Elasticsearch, exceptions
 from elasticsearch.helpers import bulk, BulkIndexError
 
+from search.api import QueryParseError
 from search.search_engine_base import SearchEngine
 from search.utils import ValueRange, _is_iterable
 
@@ -17,7 +18,7 @@
 # We _may_ want to use these for their special uses for certain queries,
 # but for analysed fields these kinds of characters are removed anyway, so
 # we can safely remove them from analysed matches
-RESERVED_CHARACTERS = "+-=><!(){}[]^\"~*:\\/&|?"
+RESERVED_CHARACTERS = "+=><!(){}[]^~*:\\/&|?"
 
 
 def _translate_hits(es_response):
@@ -129,12 +130,12 @@ def filter_item(field):
                     }
                 ]
             }
-        else:
-            return {
-                "missing": {
-                    "field": field
-                }
+
+        return {
+            "missing": {
+                "field": field
             }
+        }
 
     return [filter_item(field) for field in filter_dictionary]
 
@@ -433,7 +434,7 @@ def search(self,
                facet_terms=None,
                exclude_ids=None,
                use_field_match=False,
-               **kwargs):  # pylint: disable=too-many-arguments, too-many-locals, too-many-branches
+               **kwargs):  # pylint: disable=too-many-arguments, too-many-locals, too-many-branches, arguments-differ
         """
         Implements call to search the index for the desired content.
 
@@ -599,8 +600,13 @@ def search(self,
                 **kwargs
             )
         except exceptions.ElasticsearchException as ex:
-            # log information and re-raise
-            log.exception("error while searching index - %s", ex.message)
-            raise
+            message = unicode(ex)
+            if 'QueryParsingException' in message:
+                log.exception("Malformed search query: %s", message)
+                raise QueryParseError('Malformed search query.')
+            else:
+                # log information and re-raise
+                log.exception("error while searching index - %s", ex.message)
+                raise
 
         return _translate_hits(es_response)
diff --git a/search/tests/mock_search_engine.py b/search/tests/mock_search_engine.py
index 39b9ae8c..0836cb89 100644
--- a/search/tests/mock_search_engine.py
+++ b/search/tests/mock_search_engine.py
@@ -45,8 +45,8 @@ def _find_field(doc, field_name):
 
     if isinstance(field_value, dict):
         return _find_field(field_value, remaining_path)
-    else:
-        return field_value
+
+    return field_value
 
 
 def _filter_intersection(documents_to_search, dictionary_object, include_blanks=False):
@@ -66,7 +66,7 @@ def value_matches(doc, field_name, field_value):
             return include_blanks
 
         # if we have a string that we are trying to process as a date object
-        if isinstance(field_value, DateRange) or isinstance(field_value, datetime):
+        if isinstance(field_value, (DateRange, datetime)):
             if isinstance(compare_value, basestring):
                 compare_value = json_date_to_datetime(compare_value)
 
@@ -98,8 +98,7 @@ def value_matches(doc, field_name, field_value):
         elif _is_iterable(compare_value) and _is_iterable(field_value):
             return any((unicode(item) in field_value for item in compare_value))
 
-        else:
-            return compare_value == field_value
+        return compare_value == field_value
 
     filtered_documents = documents_to_search
     for field_name, field_value in dictionary_object.items():
@@ -321,20 +320,17 @@ def __init__(self, index=None):
         super(MockSearchEngine, self).__init__(index)
         MockSearchEngine.load_index(self.index_name)
 
-    def index(self, doc_type, sources):
+    def index(self, doc_type, sources):  # pylint: disable=arguments-differ
         """ Add/update documents of given type to the index """
-        if MockSearchEngine._disabled:
-            return None
-
-        doc_ids = [s["id"] for s in sources if "id" in s]
-        MockSearchEngine.remove_documents(self.index_name, doc_type, doc_ids)
-        MockSearchEngine.add_documents(self.index_name, doc_type, sources)
+        if not MockSearchEngine._disabled:
+            doc_ids = [s["id"] for s in sources if "id" in s]
+            MockSearchEngine.remove_documents(self.index_name, doc_type, doc_ids)
+            MockSearchEngine.add_documents(self.index_name, doc_type, sources)
 
-    def remove(self, doc_type, doc_ids):
+    def remove(self, doc_type, doc_ids):  # pylint: disable=arguments-differ
         """ Remove documents of type with given ids from the index """
-        if MockSearchEngine._disabled:
-            return None
-        MockSearchEngine.remove_documents(self.index_name, doc_type, doc_ids)
+        if not MockSearchEngine._disabled:
+            MockSearchEngine.remove_documents(self.index_name, doc_type, doc_ids)
 
     def search(self,
                query_string=None,
diff --git a/search/tests/test_course_discovery.py b/search/tests/test_course_discovery.py
index 6a91e3f9..cf7eb049 100644
--- a/search/tests/test_course_discovery.py
+++ b/search/tests/test_course_discovery.py
@@ -7,13 +7,15 @@
 """ Tests for search functionalty """
 import copy
 from datetime import datetime
+import ddt
 
+from django.core.cache import cache
 from django.test import TestCase
 from django.test.utils import override_settings
 from elasticsearch import Elasticsearch
 
 from search.api import course_discovery_search, NoSearchEngineError
-from search.elastic import ElasticSearchEngine
+from search.elastic import ElasticSearchEngine, QueryParseError
 from search.tests.utils import SearcherMixin, TEST_INDEX_NAME
 from .mock_search_engine import MockSearchEngine
 
@@ -100,6 +102,9 @@ def setUp(self):
             # Make sure that we are fresh
             _elasticsearch.indices.delete(index=TEST_INDEX_NAME, ignore=[400, 404])
 
+            # remove cached property mappings (along with everything else)
+            cache.clear()
+
             config_body = {}
             # ignore unexpected-keyword-arg; ES python client documents that it can be used
             _elasticsearch.indices.create(index=TEST_INDEX_NAME, ignore=400, body=config_body)
@@ -298,6 +303,7 @@ def test_faceting_override(self):
 
 
 @override_settings(SEARCH_ENGINE="search.tests.utils.ForceRefreshElasticSearchEngine")
+@ddt.ddt
 class TestElasticCourseDiscoverySearch(TestMockCourseDiscoverySearch):
     """ version of tests that use Elastic Backed index instead of mock """
 
@@ -305,6 +311,58 @@ def setUp(self):
         super(TestElasticCourseDiscoverySearch, self).setUp()
         self.searcher.index("doc_type_that_is_meaninless_to_bootstrap_index", [{"test_doc_type": "bootstrap"}])
 
+    def test_course_matching_empty_index(self):
+        """ Check for empty result count before indexing """
+        results = course_discovery_search("defensive")
+        self.assertEqual(results["total"], 0)
+
+    @ddt.data(
+        ('defensive', 1),
+        ('offensive', 2),
+        ('move', 3),
+        ('"offensive move"', 1),
+        ('"offensive move"', 1),
+        ('"highly-offensive"', 1),
+        ('"highly-offensive teams"', 1),
+    )
+    @ddt.unpack
+    # pylint: disable=arguments-differ
+    def test_course_matching(self, term, result_count):
+        """ Make sure that matches within content can be located and processed """
+        DemoCourse.get_and_index(self.searcher, {
+            "content": {
+                "short_description": "This is a defensive move",
+                "overview": "Defensive teams often win"
+            }
+        })
+
+        DemoCourse.get_and_index(self.searcher, {
+            "content": {
+                "short_description": "This is an offensive move",
+                "overview": "Offensive teams often win"
+            }
+        })
+
+        DemoCourse.get_and_index(self.searcher, {
+            "content": {
+                "short_description": "This is a hyphenated move",
+                "overview": "Highly-offensive teams often win"
+            }
+        })
+
+        results = course_discovery_search()
+
+        self.assertEqual(results["total"], 3)
+
+        results = course_discovery_search(term)
+        self.assertEqual(results["total"], result_count)
+
+    def test_malformed_query_handling(self):
+        """Make sure that mismatched quotes produce a specific exception. """
+
+        with self.assertRaises(QueryParseError):
+            course_discovery_search('"missing quote')
+
 
 @override_settings(SEARCH_ENGINE=None)
 class TestNone(TestCase):
diff --git a/search/tests/test_course_discovery_views.py b/search/tests/test_course_discovery_views.py
index 36d3a982..63537600 100644
--- a/search/tests/test_course_discovery_views.py
+++ b/search/tests/test_course_discovery_views.py
@@ -2,9 +2,9 @@
 from django.test.utils import override_settings
 
 from search.tests.tests import TEST_INDEX_NAME
+from search.tests.utils import post_discovery_request
 from .test_views import MockSearchUrlTest
 from .test_course_discovery import DemoCourse
-from search.tests.utils import post_discovery_request
 
 # Any class that inherits from TestCase will cause too-many-public-methods pylint error
 # pylint: disable=too-many-public-methods
@@ -35,9 +35,6 @@ def setUp(self):
             self.searcher, {"content": {"short_description": "Find this one somehow"}}
         )
 
-    def tearDown(self):
-        super(DiscoveryUrlTest, self).tearDown()
-
     def test_search_from_url(self):
         """ test searching using the url """
         code, results = post_discovery_request({})
diff --git a/search/tests/test_views.py b/search/tests/test_views.py
index b060bef2..0916325e 100644
--- a/search/tests/test_views.py
+++ b/search/tests/test_views.py
@@ -1,5 +1,6 @@
 """ High-level view tests"""
 from datetime import datetime
+import ddt
 
 from django.core.urlresolvers import Resolver404, resolve
 from django.test import TestCase
@@ -9,7 +10,7 @@
 from search.search_engine_base import SearchEngine
 from search.tests.mock_search_engine import MockSearchEngine
 from search.tests.tests import TEST_INDEX_NAME
-from search.tests.utils import post_request, SearcherMixin
+from search.tests.utils import post_discovery_request, post_request, SearcherMixin
 
 
 # Any class that inherits from TestCase will cause too-many-public-methods pylint error
@@ -447,3 +448,82 @@ def test_search_from_url(self):
         searcher = SearchEngine.get_search_engine(TEST_INDEX_NAME)
         with self.assertRaises(StandardError):
             searcher.index("courseware_content", [{"id": "FAKE_ID_3", "content": {"text": "Here comes the sun"}}])
+
+
+@override_settings(SEARCH_ENGINE="search.tests.utils.ForceRefreshElasticSearchEngine")
+@override_settings(ELASTIC_FIELD_MAPPINGS={"start_date": {"type": "date"}})
+@override_settings(COURSEWARE_INDEX_NAME=TEST_INDEX_NAME)
+@ddt.ddt
+class ElasticSearchUrlTest(TestCase, SearcherMixin):
+    """Elastic-specific tests"""
+    def setUp(self):
+        self.searcher.index(
+            "courseware_content",
+            [
+                {
+                    "course": "ABC/DEF/GHI",
+                    "id": "FAKE_ID_1",
+                    "content": {
+                        "text": "It seems like k-means clustering would work in this context."
+                    },
+                    "test_date": datetime(2015, 1, 1),
+                    "test_string": "ABC, It's easy as 123"
+                }
+            ]
+        )
+        self.searcher.index(
+            "courseware_content",
+            [
+                {
+                    "course": "ABC/DEF/GHI",
+                    "id": "FAKE_ID_2",
+                    "content": {
+                        "text": "It looks like k-means clustering could work in this context."
+                    }
+                }
+            ]
+        )
+        self.searcher.index(
+            "courseware_content",
+            [
+                {
+                    "course": "ABC/DEF/GHI",
+                    "id": "FAKE_ID_3",
+                    "content": {
+                        "text": "It looks like k means something different in this context."
+                    }
+                }
+            ]
+        )
+
+    @ddt.data(
+        # Quoted phrases
+        ('"in this context"', None, 3),
+        ('"in this context"', "ABC/DEF/GHI", 3),
+        ('"looks like"', None, 2),
+        ('"looks like"', "ABC/DEF/GHI", 2),
+        # Hyphenated phrases
+        ('k-means', None, 3),
+        ('k-means', "ABC/DEF/GHI", 3),
+    )
+    @ddt.unpack
+    def test_valid_search(self, query, course_id, result_count):
+        code, results = post_request({"search_string": query}, course_id)
+        self.assertTrue(199 < code < 300)
+        self.assertEqual(results["total"], result_count)
+
+    def test_malformed_query_handling(self):
+        # root
+        code, results = post_request({"search_string": "\"missing quote"})
+        self.assertGreater(code, 499)
+        self.assertEqual(results["error"], 'Your query seems malformed. Check for unmatched quotes.')
+
+        # course ID
+        code, results = post_request({"search_string": "\"missing quote"}, "ABC/DEF/GHI")
+        self.assertGreater(code, 499)
+        self.assertEqual(results["error"], 'Your query seems malformed. Check for unmatched quotes.')
+
+        # course discovery
+        code, results = post_discovery_request({"search_string": "\"missing quote"})
+        self.assertGreater(code, 499)
+        self.assertEqual(results["error"], 'Your query seems malformed. Check for unmatched quotes.')
diff --git a/search/tests/utils.py b/search/tests/utils.py
index 5535866c..78dce445 100644
--- a/search/tests/utils.py
+++ b/search/tests/utils.py
@@ -63,14 +63,18 @@ def remove(self, doc_type, doc_ids, **kwargs):
 class ErroringSearchEngine(MockSearchEngine):
     """ Override to generate search engine error to test """
 
-    def search(self, query_string=None, field_dictionary=None, filter_dictionary=None, **kwargs):
+    def search(self,
+               query_string=None,
+               field_dictionary=None,
+               filter_dictionary=None,
+               **kwargs):  # pylint: disable=arguments-differ
         raise StandardError("There is a problem here")
 
 
 class ErroringIndexEngine(MockSearchEngine):
     """ Override to generate search engine error to test """
 
-    def index(self, doc_type, sources, **kwargs):  # pylint: disable=unused-argument
+    def index(self, doc_type, sources, **kwargs):  # pylint: disable=unused-argument, arguments-differ
         raise StandardError("There is a problem here")
 
 
@@ -78,6 +82,6 @@ class ErroringElasticImpl(Elasticsearch):
     """ Elasticsearch implementation that throws exceptions"""
 
     # pylint: disable=unused-argument
-    def search(self, **kwargs):
+    def search(self, **kwargs):  # pylint: disable=arguments-differ
         """ this will definitely fail """
         raise exceptions.ElasticsearchException("This search operation failed")
diff --git a/search/views.py b/search/views.py
index 9c509ec6..0d496148 100644
--- a/search/views.py
+++ b/search/views.py
@@ -11,7 +11,7 @@
 from django.views.decorators.http import require_POST
 
 from eventtracking import tracker as track
-from .api import perform_search, course_discovery_search, course_discovery_filter_fields
+from .api import QueryParseError, perform_search, course_discovery_search, course_discovery_filter_fields
 from .initializer import SearchInitializer
 
 # log appears to be standard name used for logger
@@ -124,6 +124,11 @@ def do_search(request, course_id=None):
         }
         log.debug(unicode(invalid_err))
 
+    except QueryParseError:
+        results = {
+            "error": _('Your query seems malformed. Check for unmatched quotes.')
+        }
+
     # Allow for broad exceptions here - this is an entry point from external reference
     except Exception as err:  # pylint: disable=broad-except
         results = {
@@ -214,6 +219,11 @@ def course_discovery(request):
         }
         log.debug(unicode(invalid_err))
 
+    except QueryParseError:
+        results = {
+            "error": _('Your query seems malformed. Check for unmatched quotes.')
+        }
+
     # Allow for broad exceptions here - this is an entry point from external reference
     except Exception as err:  # pylint: disable=broad-except
         results = {
diff --git a/setup.py b/setup.py
index dbed2bd5..33f9e11a 100755
--- a/setup.py
+++ b/setup.py
@@ -5,7 +5,7 @@
 
 setup(
     name='edx-search',
-    version='1.1.0',
+    version='1.2.0',
     description='Search and index routines for index access',
     author='edX',
     author_email='oscm@edx.org',
diff --git a/test_requirements.txt b/test_requirements.txt
index baf449b8..2f19ebd6 100644
--- a/test_requirements.txt
+++ b/test_requirements.txt
@@ -1,12 +1,12 @@
 -e .
 coverage==4.0.3
-edx-lint==0.4.0
+ddt
+edx-lint==0.5.5
 mock==1.3.0
 pep8==1.6.2
 pytz
 codecov
 tox>=2.3.1,<3.0.0
 
-
 # edX libraries
 -e git+https://github.com/edx/event-tracking.git@0.1.0#egg=event-tracking