From 350ef7a727950d2bee93677f95769fa29fbf7b5e Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Mon, 18 Sep 2017 12:08:23 +0200 Subject: [PATCH] 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 | 1 + src/olympia/addons/indexers.py | 2 +- src/olympia/addons/tests/test_views.py | 37 +++++++- src/olympia/addons/views.py | 12 +-- src/olympia/bandwagon/indexers.py | 2 +- src/olympia/search/filters.py | 111 +++++++++++++++-------- src/olympia/search/tests/test_filters.py | 17 ++++ 7 files changed, 133 insertions(+), 49 deletions(-) diff --git a/docs/topics/api/addons.rst b/docs/topics/api/addons.rst index d21d4cd7c3..a8dd3183b6 100644 --- a/docs/topics/api/addons.rst +++ b/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 `. ``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 `) :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. diff --git a/src/olympia/addons/indexers.py b/src/olympia/addons/indexers.py index bfad394a32..bb53cf72b5 100644 --- a/src/olympia/addons/indexers.py +++ b/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'}, diff --git a/src/olympia/addons/tests/test_views.py b/src/olympia/addons/tests/test_views.py index 6e275629c6..290d63d72e 100644 --- a/src/olympia/addons/tests/test_views.py +++ b/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 diff --git a/src/olympia/addons/views.py b/src/olympia/addons/views.py index 5bdc6e61ed..edade63038 100644 --- a/src/olympia/addons/views.py +++ b/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.') diff --git a/src/olympia/bandwagon/indexers.py b/src/olympia/bandwagon/indexers.py index 13a9b25160..9c537226a8 100644 --- a/src/olympia/bandwagon/indexers.py +++ b/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'}, }, } diff --git a/src/olympia/search/filters.py b/src/olympia/search/filters.py index 88ab8b35a1..5db36c82f0 100644 --- a/src/olympia/search/filters.py +++ b/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 ] diff --git a/src/olympia/search/tests/test_filters.py b/src/olympia/search/tests/test_filters.py index 5ef1128f74..95bd0780a6 100644 --- a/src/olympia/search/tests/test_filters.py +++ b/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]