Add 'exclude_addons' param to search API (#6414)

* Change slugs to keywords type in ES to prevent them from being analyzed

They'll often match the name anyway, and analyzing it prevents us from
doing exact matches on it.

* Add 'exclude_addons' param to search API

* Rename *FilterParam to *QueryParam to follow base class name change

* Avoid naming variable "rval"

docs/topics/api/addons.rst

@@ -44,6 +44,7 @@ This endpoint allows you to search through public add-ons.
     :query string appversion: Filter by application version compatibility. Pass the full version as a string, e.g. ``46.0``. Only valid when the ``app`` parameter is also present.
     :query string author: Filter by exact author username. Multiple author names can be specified, separated by comma(s), in which case add-ons with at least one matching author are returned.
     :query string category: Filter by :ref:`category slug <category-list>`. ``app`` and ``type`` parameters need to be set, otherwise this parameter is ignored.
+    :query string exclude_addons: Exclude add-ons by ``slug`` or ``id``. Multiple add-ons can be specified, separated by comma(s).
     :query string lang: Activate translations in the specific language for that query. (See :ref:`translated fields <api-overview-translations>`)
     :query int page: 1-based page number. Defaults to 1.
     :query int page_size: Maximum number of results to return for the requested page. Defaults to 25.

src/olympia/addons/indexers.py

@@ -160,7 +160,7 @@ class AddonIndexer(BaseSearchIndexer):
                             'average': {'type': 'float', 'index': False}
                         }
                     },
-                    'slug': {'type': 'text'},
+                    'slug': {'type': 'keyword'},
                     'requires_payment': {'type': 'boolean', 'index': False},
                     'status': {'type': 'byte'},
                     'summary': {'type': 'text', 'analyzer': 'snowball'},

src/olympia/addons/tests/test_views.py

@@ -2487,7 +2487,7 @@ class TestAddonSearchView(ESTestCase):
         assert result['slug'] == 'my-addon'
 
     def test_with_query(self):
-        addon = addon_factory(slug='my-addon', name=u'My Addôn',
+        addon = addon_factory(slug='my-addon', name=u'My Addon',
                               tags=['some_tag'])
         addon_factory(slug='unrelated', name=u'Unrelated')
         self.refresh()
@@ -2498,7 +2498,7 @@ class TestAddonSearchView(ESTestCase):
 
         result = data['results'][0]
         assert result['id'] == addon.pk
-        assert result['name'] == {'en-US': u'My Addôn'}
+        assert result['name'] == {'en-US': u'My Addon'}
         assert result['slug'] == 'my-addon'
 
     def test_with_session_cookie(self):
@@ -2796,6 +2796,39 @@ class TestAddonSearchView(ESTestCase):
                 assert result['id'] == addon.pk
                 assert result['slug'] == addon.slug
 
+    def test_exclude_addons(self):
+        addon1 = addon_factory()
+        addon2 = addon_factory()
+        addon3 = addon_factory()
+        self.refresh()
+
+        # Exclude addon2 and addon3 by slug.
+        data = self.perform_search(
+            self.url, {'exclude_addons': u','.join(
+                (addon2.slug, addon3.slug))})
+
+        assert len(data['results']) == 1
+        assert data['count'] == 1
+        assert data['results'][0]['id'] == addon1.pk
+
+        # Exclude addon1 and addon2 by pk.
+        data = self.perform_search(
+            self.url, {'exclude_addons': u','.join(
+                map(unicode, (addon2.pk, addon1.pk)))})
+
+        assert len(data['results']) == 1
+        assert data['count'] == 1
+        assert data['results'][0]['id'] == addon3.pk
+
+        # Exclude addon1 by pk and addon3 by slug.
+        data = self.perform_search(
+            self.url, {'exclude_addons': u','.join(
+                (unicode(addon1.pk), addon3.slug))})
+
+        assert len(data['results']) == 1
+        assert data['count'] == 1
+        assert data['results'][0]['id'] == addon2.pk
+
 
 class TestAddonAutoCompleteSearchView(ESTestCase):
     client_class = APITestClient

src/olympia/addons/views.py

@@ -49,7 +49,7 @@ from olympia.api.permissions import (
 from olympia.reviews.forms import ReviewForm
 from olympia.reviews.models import Review, GroupedRating
 from olympia.search.filters import (
-    AddonAppFilterParam, AddonCategoryFilterParam, AddonTypeFilterParam,
+    AddonAppQueryParam, AddonCategoryQueryParam, AddonTypeQueryParam,
     ReviewedContentFilter, SearchParameterFilter, SearchQueryFilter,
     SortingFilter)
 from olympia.stats.models import Contribution
@@ -841,10 +841,10 @@ class AddonFeaturedView(GenericAPIView):
             # If a category is passed then the app and type parameters are
             # mandatory because we need to find a category in the constants to
             # pass to get_creatured_ids(), and category slugs are not unique.
-            # AddonCategoryFilterParam parses the request parameters for us to
+            # AddonCategoryQueryParam parses the request parameters for us to
             # determine the category.
             try:
-                category = AddonCategoryFilterParam(self.request).get_value()
+                category = AddonCategoryQueryParam(self.request).get_value()
             except ValueError:
                 raise ParseError(
                     'Invalid app, category and/or type parameter(s).')
@@ -855,11 +855,11 @@ class AddonFeaturedView(GenericAPIView):
             # to pick addons from. It can optionally filter by type, so we
             # parse request for that as well.
             try:
-                app = AddonAppFilterParam(
+                app = AddonAppQueryParam(
                     self.request).get_object_from_reverse_dict()
                 type_ = None
                 if 'type' in self.request.GET:
-                    type_ = AddonTypeFilterParam(self.request).get_value()
+                    type_ = AddonTypeQueryParam(self.request).get_value()
             except ValueError:
                 raise ParseError(
                     'Invalid app, category and/or type parameter(s).')
@@ -905,7 +905,7 @@ class LanguageToolsView(ListAPIView):
 
     def get_queryset(self):
         try:
-            application_id = AddonAppFilterParam(self.request).get_value()
+            application_id = AddonAppQueryParam(self.request).get_value()
         except ValueError:
             raise ParseError('Invalid app parameter.')
 

src/olympia/bandwagon/indexers.py

@@ -30,7 +30,7 @@ class CollectionIndexer(BaseSearchIndexer):
                     'name': {'type': 'text',
                              'analyzer': 'standardPlusWordDelimiter'},
                     'type': {'type': 'byte'},
-                    'slug': {'type': 'text'},
+                    'slug': {'type': 'keyword'},
 
                 },
             }

src/olympia/search/filters.py

@@ -16,8 +16,8 @@ def get_locale_analyzer(lang):
     return analyzer
 
 
-class AddonFilterParam(object):
-    """Helper to build a simple ES lookup query from a request.GET param."""
+class AddonQueryParam(object):
+    """Helper to build a simple ES query from a request.GET param."""
     operator = 'term'  # ES filter to use when filtering.
     query_param = None
     reverse_dict = None
@@ -53,11 +53,11 @@ class AddonFilterParam(object):
     def get_value_from_object_from_reverse_dict(self):
         return self.get_object_from_reverse_dict().id
 
-    def get_es_filter(self):
+    def get_es_query(self):
         return [Q(self.operator, **{self.es_field: self.get_value()})]
 
 
-class AddonAppFilterParam(AddonFilterParam):
+class AddonAppQueryParam(AddonQueryParam):
     query_param = 'app'
     reverse_dict = amo.APPS
     valid_values = amo.APP_IDS
@@ -67,7 +67,7 @@ class AddonAppFilterParam(AddonFilterParam):
         return self.get_value_from_object_from_reverse_dict()
 
 
-class AddonAppVersionFilterParam(AddonFilterParam):
+class AddonAppVersionQueryParam(AddonQueryParam):
     query_param = 'appversion'
     # appversion need special treatment. We need to convert the query parameter
     # into a set of min and max integer values, and filter on those 2 values
@@ -76,7 +76,7 @@ class AddonAppVersionFilterParam(AddonFilterParam):
 
     def get_values(self):
         appversion = self.request.GET.get(self.query_param)
-        app = AddonAppFilterParam(self.request).get_value()
+        app = AddonAppQueryParam(self.request).get_value()
 
         if appversion and app:
             # Get a min version less than X.0, and a max greater than X.0a
@@ -87,10 +87,10 @@ class AddonAppVersionFilterParam(AddonFilterParam):
             return app, low, high
         raise ValueError(
             'Invalid combination of "%s" and "%s" parameters.' % (
-                AddonAppFilterParam.query_param,
+                AddonAppQueryParam.query_param,
                 self.query_param))
 
-    def get_es_filter(self):
+    def get_es_query(self):
         app_id, low, high = self.get_values()
         return [
             Q('range', **{'current_version.compatible_apps.%d.min' % app_id:
@@ -100,8 +100,8 @@ class AddonAppVersionFilterParam(AddonFilterParam):
         ]
 
 
-class AddonAuthorFilterParam(AddonFilterParam):
-    # Note: this filter returns add-ons that have at least one matching author
+class AddonAuthorQueryParam(AddonQueryParam):
+    # Note: this returns add-ons that have at least one matching author
     # when several are provided (separated by a comma).
     # It works differently from the tag filter below that needs all tags
     # provided to match.
@@ -113,7 +113,7 @@ class AddonAuthorFilterParam(AddonFilterParam):
         return self.request.GET.get(self.query_param, '').split(',')
 
 
-class AddonPlatformFilterParam(AddonFilterParam):
+class AddonPlatformQueryParam(AddonQueryParam):
     query_param = 'platform'
     reverse_dict = amo.PLATFORM_DICT
     valid_values = amo.PLATFORMS
@@ -121,7 +121,7 @@ class AddonPlatformFilterParam(AddonFilterParam):
     operator = 'terms'  # Because we'll be sending a list every time.
 
     def get_value(self):
-        value = super(AddonPlatformFilterParam, self).get_value()
+        value = super(AddonPlatformQueryParam, self).get_value()
         # No matter what platform the client wants to see, we always need to
         # include PLATFORM_ALL to match add-ons compatible with all platforms.
         if value != amo.PLATFORM_ALL.id:
@@ -134,48 +134,48 @@ class AddonPlatformFilterParam(AddonFilterParam):
         return self.get_value_from_object_from_reverse_dict()
 
 
-class AddonTypeFilterParam(AddonFilterParam):
+class AddonTypeQueryParam(AddonQueryParam):
     query_param = 'type'
     reverse_dict = amo.ADDON_SEARCH_SLUGS
     valid_values = amo.ADDON_SEARCH_TYPES
     es_field = 'type'
 
 
-class AddonStatusFilterParam(AddonFilterParam):
+class AddonStatusQueryParam(AddonQueryParam):
     query_param = 'status'
     reverse_dict = amo.STATUS_CHOICES_API_LOOKUP
     valid_values = amo.STATUS_CHOICES_API
     es_field = 'status'
 
 
-class AddonCategoryFilterParam(AddonFilterParam):
+class AddonCategoryQueryParam(AddonQueryParam):
     query_param = 'category'
     es_field = 'category'
     valid_values = CATEGORIES_BY_ID.keys()
 
     def __init__(self, request):
-        super(AddonCategoryFilterParam, self).__init__(request)
+        super(AddonCategoryQueryParam, self).__init__(request)
         # Category slugs are only unique for a given type+app combination.
         # Once we have that, it's just a matter of selecting the corresponding
         # dict in the categories constants and use that as the reverse dict,
         # and make sure to use get_value_from_object_from_reverse_dict().
         try:
-            app = AddonAppFilterParam(self.request).get_value()
-            type_ = AddonTypeFilterParam(self.request).get_value()
+            app = AddonAppQueryParam(self.request).get_value()
+            type_ = AddonTypeQueryParam(self.request).get_value()
 
             self.reverse_dict = CATEGORIES[app][type_]
         except KeyError:
             raise ValueError(
                 'Invalid combination of "%s", "%s" and "%s" parameters.' % (
-                    AddonAppFilterParam.query_param,
-                    AddonTypeFilterParam.query_param,
+                    AddonAppQueryParam.query_param,
+                    AddonTypeQueryParam.query_param,
                     self.query_param))
 
     def get_value_from_reverse_dict(self):
         return self.get_value_from_object_from_reverse_dict()
 
 
-class AddonTagFilterParam(AddonFilterParam):
+class AddonTagQueryParam(AddonQueryParam):
     # query_param is needed for SearchParameterFilter below, so we need it
     # even with the custom get_value() implementation.
     query_param = 'tag'
@@ -183,12 +183,30 @@ class AddonTagFilterParam(AddonFilterParam):
     def get_value(self):
         return self.request.GET.get(self.query_param, '').split(',')
 
-    def get_es_filter(self):
+    def get_es_query(self):
         # Just using 'terms' would not work, as it would return any tag match
         # in the list, but we want to exactly match all of them.
         return [Q('term', tags=tag) for tag in self.get_value()]
 
 
+class AddonExcludeAddonsQueryParam(AddonQueryParam):
+    query_param = 'exclude_addons'
+
+    def get_value(self):
+        return self.request.GET.get(self.query_param, '').split(',')
+
+    def get_es_query(self):
+        filters = []
+        values = self.get_value()
+        ids = [value for value in values if value.isdigit()]
+        slugs = [value for value in values if not value.isdigit()]
+        if ids:
+            filters.append(Q('ids', values=ids))
+        if slugs:
+            filters.append(Q('terms', slug=slugs))
+        return filters
+
+
 class SearchQueryFilter(BaseFilterBackend):
     """
     A django-rest-framework filter backend that performs an ES query according
@@ -327,29 +345,44 @@ class SearchQueryFilter(BaseFilterBackend):
 class SearchParameterFilter(BaseFilterBackend):
     """
     A django-rest-framework filter backend for ES queries that look for items
-    matching a specific set of fields in request.GET: app, appversion,
-    platform, tag and type.
+    matching a specific set of params in request.GET: app, appversion,
+    author, category, exclude_addons, platform, tag and type.
     """
-    available_filters = [AddonAppFilterParam, AddonAppVersionFilterParam,
-                         AddonPlatformFilterParam, AddonTypeFilterParam,
-                         AddonCategoryFilterParam, AddonTagFilterParam,
-                         AddonAuthorFilterParam]
+    available_filters = [AddonAppQueryParam, AddonAppVersionQueryParam,
+                         AddonAuthorQueryParam, AddonCategoryQueryParam,
+                         AddonPlatformQueryParam, AddonTagQueryParam,
+                         AddonTypeQueryParam]
 
-    def filter_queryset(self, request, qs, view):
-        must = []
+    available_excludes = [AddonExcludeAddonsQueryParam]
 
-        for filter_class in self.available_filters:
+    def get_applicable_clauses(self, request, params_to_try):
+        clauses = []
+        for param_class in params_to_try:
             try:
-                # Initialize the filter class if its query parameter is present
-                # in the request, otherwise don't, to avoid  raising exceptions
-                # because of missing params in complex filters.
-                if filter_class.query_param in request.GET:
-                    filter_ = filter_class(request)
-                    must.extend(filter_.get_es_filter())
+                # Initialize the param class if its query parameter is
+                # present in the request, otherwise don't, to avoid raising
+                # exceptions because of missing params in complex filters.
+                if param_class.query_param in request.GET:
+                    clauses.extend(param_class(request).get_es_query())
             except ValueError as exc:
                 raise serializers.ValidationError(*exc.args)
+        return clauses
 
-        return qs.query(query.Bool(must=must)) if must else qs
+    def filter_queryset(self, request, qs, view):
+        bool_kwargs = {}
+
+        must = self.get_applicable_clauses(
+            request, self.available_filters)
+        must_not = self.get_applicable_clauses(
+            request, self.available_excludes)
+
+        if must:
+            bool_kwargs['must'] = must
+
+        if must_not:
+            bool_kwargs['must_not'] = must_not
+
+        return qs.query(query.Bool(**bool_kwargs)) if bool_kwargs else qs
 
 
 class InternalSearchParameterFilter(SearchParameterFilter):
@@ -359,7 +392,7 @@ class InternalSearchParameterFilter(SearchParameterFilter):
     # FIXME: also allow searching by listed/unlisted, deleted or not,
     # disabled or not.
     available_filters = SearchParameterFilter.available_filters + [
-        AddonStatusFilterParam
+        AddonStatusQueryParam
     ]
 
 

src/olympia/search/tests/test_filters.py

@@ -373,6 +373,23 @@ class TestSearchParameterFilter(FilterTestsBase):
         must = qs['query']['bool']['must']
         assert {'terms': {'listed_authors.username': ['foo', 'bar']}} in must
 
+    def test_exclude_addons(self):
+        qs = self._filter(data={'exclude_addons': 'fooBar'})
+        assert 'must' not in qs['query']['bool']
+        must_not = qs['query']['bool']['must_not']
+        assert must_not == [{'terms': {'slug': [u'fooBar']}}]
+
+        qs = self._filter(data={'exclude_addons': 1})
+        assert 'must' not in qs['query']['bool']
+        must_not = qs['query']['bool']['must_not']
+        assert must_not == [{'ids': {'values': [u'1']}}]
+
+        qs = self._filter(data={'exclude_addons': 'fooBar,1'})
+        assert 'must' not in qs['query']['bool']
+        must_not = qs['query']['bool']['must_not']
+        assert {'ids': {'values': [u'1']}} in must_not
+        assert {'terms': {'slug': [u'fooBar']}} in must_not
+
 
 class TestInternalSearchParameterFilter(TestSearchParameterFilter):
     filter_classes = [InternalSearchParameterFilter]