From fc9fe439132038b7640c81dcddc73f61a6d8f4ae Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Tue, 12 May 2015 19:34:23 -0400 Subject: [PATCH 1/2] [bug 1164205] Refactor search suggest api This refactors the search suggest API to use more DRF-y things including built-in DRF-y validation. In the process of doing this, I changed it so that you can provide the filter parameters in the querystring OR in the HTTP request body as JSON. --- kitsune/search/api.py | 99 ++++++++++++++++++++------------ kitsune/search/tests/test_api.py | 51 +++++++++++++--- 2 files changed, 105 insertions(+), 45 deletions(-) diff --git a/kitsune/search/api.py b/kitsune/search/api.py index da4204baa88..5c6627507d8 100644 --- a/kitsune/search/api.py +++ b/kitsune/search/api.py @@ -1,5 +1,6 @@ from django.conf import settings +from rest_framework import serializers from rest_framework.decorators import api_view from rest_framework.response import Response @@ -11,49 +12,75 @@ from kitsune.wiki.models import DocumentMappingType +def positive_integer(value): + if value < 0: + raise serializers.ValidationError('This field must be positive.') + + +def valid_product(value): + if not value: + return + + if not Product.objects.filter(slug=value).exists(): + raise serializers.ValidationError( + 'Could not find product with slug "{0}".'.format(value) + ) + + +def valid_locale(value): + if not value: + return + + if value not in settings.SUMO_LANGUAGES: + if value in settings.NON_SUPPORTED_LOCALES: + fallback = settings.NON_SUPPORTED_LOCALES[value] or settings.WIKI_DEFAULT_LANGUAGE + raise serializers.ValidationError( + '"{0}" is not supported, but has fallback locale "{1}".'.format( + value, fallback)) + else: + raise serializers.ValidationError( + 'Could not find locale "{0}".'.format(value) + ) + + +class SuggestSerializer(serializers.Serializer): + q = serializers.CharField(required=True) + locale = serializers.CharField( + required=False, default=settings.WIKI_DEFAULT_LANGUAGE, + validators=[valid_locale]) + product = serializers.CharField( + required=False, default='', + validators=[valid_product]) + max_questions = serializers.IntegerField( + required=False, default=10, + validators=[positive_integer]) + max_documents = serializers.IntegerField( + required=False, default=10, + validators=[positive_integer]) + + @api_view(['GET', 'POST']) def suggest(request): - text = request.body or request.GET.get('q') - locale = request.GET.get('locale', settings.WIKI_DEFAULT_LANGUAGE) - product = request.GET.get('product') - max_questions = request.GET.get('max_questions', 10) - max_documents = request.GET.get('max_documents', 10) - - errors = {} - try: - max_questions = int(max_questions) - except ValueError: - errors['max_questions'] = 'This field must be an integer.' - try: - max_documents = int(max_documents) - except ValueError: - errors['max_documents'] = 'This field must be an integer.' - if text is None: - errors['q'] = 'This field is required.' - if product is not None and not Product.objects.filter(slug=product).exists(): - errors['product'] = 'Could not find product with slug "{0}".'.format(product) - if locale not in settings.SUMO_LANGUAGES: - if locale in settings.NON_SUPPORTED_LOCALES: - fallback = settings.NON_SUPPORTED_LOCALES[locale] or settings.WIKI_DEFAULT_LANGUAGE - errors['locale'] = ( - 'Locale "{0}" is not supported, but has fallback locale "{1}".'.format( - locale, fallback - ) - ) - else: - errors['locale'] = 'Could not find locale "{0}".'.format(locale) + if request.DATA and request.GET: + raise GenericAPIException( + 400, 'Put all parameters either in the querystring or the HTTP request body.') - if errors: - raise GenericAPIException(400, errors) + serializer = SuggestSerializer(data=(request.DATA or request.GET)) + if not serializer.is_valid(): + raise GenericAPIException(400, serializer.errors) searcher = ( es_utils.AnalyzerS() .es(urls=settings.ES_URLS) .indexes(es_utils.read_index('default'))) + data = serializer.object + return Response({ - 'questions': _question_suggestions(searcher, text, locale, product, max_questions), - 'documents': _document_suggestions(searcher, text, locale, product, max_documents), + 'questions': _question_suggestions( + searcher, data['q'], data['locale'], data['product'], data['max_questions']), + 'documents': _document_suggestions( + searcher, data['q'], data['locale'], data['product'], data['max_documents']), }) @@ -66,9 +93,9 @@ def _question_suggestions(searcher, text, locale, product, max_results): question_is_archived=False, question_is_locked=False, question_is_solved=True) - if product is not None: + if product: search_filter &= es_utils.F(product=product) - if locale is not None: + if locale: search_filter &= es_utils.F(question_locale=locale) questions = [] @@ -91,7 +118,7 @@ def _document_suggestions(searcher, text, locale, product, max_results): document_locale=locale, document_is_archived=False) - if product is not None: + if product: search_filter &= es_utils.F(product=product) documents = [] diff --git a/kitsune/search/tests/test_api.py b/kitsune/search/tests/test_api.py index 0b080e77059..1b5fff468f6 100644 --- a/kitsune/search/tests/test_api.py +++ b/kitsune/search/tests/test_api.py @@ -1,3 +1,4 @@ +import json from nose.tools import eq_ from rest_framework.test import APIClient @@ -65,7 +66,7 @@ def test_invalid_product(self): 'q': 'search', }) eq_(res.status_code, 400) - eq_(res.data['detail'], {'product': 'Could not find product with slug "nonexistant".'}) + eq_(res.data['detail'], {'product': [u'Could not find product with slug "nonexistant".']}) def test_invalid_locale(self): res = self.client.get(reverse('search.suggest'), { @@ -73,7 +74,7 @@ def test_invalid_locale(self): 'q': 'search', }) eq_(res.status_code, 400) - eq_(res.data['detail'], {'locale': 'Could not find locale "bad-medicine".'}) + eq_(res.data['detail'], {'locale': [u'Could not find locale "bad-medicine".']}) def test_invalid_fallback_locale_none_case(self): # Test the locale -> locale case. @@ -90,8 +91,8 @@ def test_invalid_fallback_locale_none_case(self): eq_(res.status_code, 400) eq_( res.data['detail'], - {'locale': 'Locale "{0}" is not supported, but has fallback locale "{1}".'.format( - locale, fallback)} + {'locale': [u'"{0}" is not supported, but has fallback locale "{1}".'.format( + locale, fallback)]} ) def test_invalid_fallback_locale_non_none_case(self): @@ -109,8 +110,8 @@ def test_invalid_fallback_locale_non_none_case(self): eq_(res.status_code, 400) eq_( res.data['detail'], - {'locale': 'Locale "{0}" is not supported, but has fallback locale "{1}".'.format( - locale, settings.WIKI_DEFAULT_LANGUAGE)} + {'locale': [u'"{0}" is not supported, but has fallback locale "{1}".'.format( + locale, settings.WIKI_DEFAULT_LANGUAGE)]} ) def test_invalid_numbers(self): @@ -121,14 +122,14 @@ def test_invalid_numbers(self): }) eq_(res.status_code, 400) eq_(res.data['detail'], { - 'max_questions': 'This field must be an integer.', - 'max_documents': 'This field must be an integer.', + 'max_questions': [u'Enter a whole number.'], + 'max_documents': [u'Enter a whole number.'], }) def test_q_required(self): res = self.client.get(reverse('search.suggest')) eq_(res.status_code, 400) - eq_(res.data['detail'], {'q': 'This field is required.'}) + eq_(res.data['detail'], {'q': [u'This field is required.']}) def test_it_works(self): q1 = self._make_question() @@ -139,6 +140,38 @@ def test_it_works(self): eq_([q['id'] for q in req.data['questions']], [q1.id]) eq_([d['title'] for d in req.data['documents']], [d1.title]) + def test_filters_in_postdata(self): + q1 = self._make_question() + d1 = self._make_document() + self.refresh() + + data = json.dumps({'q': 'emails'}) + # Note: Have to use .generic() because .get() will convert the + # data into querystring params and then it's clownshoes all + # the way down. + req = self.client.generic( + 'GET', reverse('search.suggest'), data=data, + content_type='application/json') + eq_(req.status_code, 200) + eq_([q['id'] for q in req.data['questions']], [q1.id]) + eq_([d['title'] for d in req.data['documents']], [d1.title]) + + def test_both_querystring_and_body_raises_error(self): + self._make_question() + self._make_document() + self.refresh() + + data = json.dumps({'q': 'emails'}) + # Note: Have to use .generic() because .get() will convert the + # data into querystring params and then it's clownshoes all + # the way down. + req = self.client.generic( + 'GET', reverse('search.suggest') + '?max_documents=3', data=data, + content_type='application/json') + eq_(req.status_code, 400) + eq_(req.data, + {u'detail': 'Put all parameters either in the querystring or the HTTP request body.'}) + def test_questions_max_results_0(self): self._make_question() self.refresh() From d1e3f6ff337f77d77dfc3c6698434ae7be19fb07 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Wed, 13 May 2015 10:29:28 -0400 Subject: [PATCH 2/2] [bug 1164205] Added api docs for search suggest api I have no idea if this is the format we want to go with, but it is some documentation. Maintaining it will be painful. It's probably better for us to figure out a way to autogenerate the documentation or somehow move the documentation to the code. That work is covered in bug #1164247. --- docs/api.rst | 141 +++++++++++++++++++++++++++++++++++++++++++++++++ docs/index.rst | 1 + 2 files changed, 142 insertions(+) create mode 100644 docs/api.rst diff --git a/docs/api.rst b/docs/api.rst new file mode 100644 index 00000000000..69ec4c917c6 --- /dev/null +++ b/docs/api.rst @@ -0,0 +1,141 @@ +=== +API +=== + +SUMO has a series of API endpoints to access data. + +.. contents:: + + +Search suggest API +================== + +:Endpoint: ``/api/2/search/suggest/`` +:Method: ``GET`` +:Content type: ``application/json`` +:Response: ``application/json`` + +The search suggest API allows you to get back kb documents and aaq +questions that match specified arguments. + +Arguments can be specified in the url querystring or in the HTTP +request body. + + +Required arguments +------------------ + ++-------------------+--------+--------------------------------------------------------+ +|argument |type |notes | ++===================+========+========================================================+ +|q |string |This is the text you're querying for. | ++-------------------+--------+--------------------------------------------------------+ + + +Optional arguments +------------------ + ++-------------------+--------+--------------------------------------------------------+ +|argument |type |notes | ++===================+========+========================================================+ +|locale |string |default: ``settings.WIKI_DEFAULT_LANGUAGE`` | +| | | | +| | |The locale code to restrict results to. | +| | | | +| | |Examples: | +| | | | +| | |* ``en-US`` | +| | |* ``fr`` | ++-------------------+--------+--------------------------------------------------------+ +|product |string |default: None | +| | | | +| | |The product to restrict results to. | +| | | | +| | |Example: | +| | | | +| | |* ``firefox`` | ++-------------------+--------+--------------------------------------------------------+ +|max_documents |integer |default: 10 | +| | | | +| | |The maximum number of documents you want back. | ++-------------------+--------+--------------------------------------------------------+ +|max_questions |integer |default: 10 | +| | | | +| | |The maximum number of questions you want back. | ++-------------------+--------+--------------------------------------------------------+ + + +Responses +--------- + +All response bodies are in JSON. + +HTTP 200: Success +~~~~~~~~~~~~~~~~~ + +With an HTTP 200, you'll get back a set of results in JSON. + +:: + + { + "documents": [ + { + "title": ... # title of kb article + "url": ... # url of kb article + "slug": ... # slug of kb article + "summary": ... # paragraph summary of kb article (plaintext) + } + ... + ], + "questions": [ + { + "id": ... # integer id of the question + "answers": ... # list of answer ids + "content": ... # content of question (in html) + "created": ... # datetime stamp in iso-8601 format + "creator": ... # JSON object describing the creator + "involved": ... # list of JSON objects describing everyone who + participated in the question + "is_archived": ... # boolean for whether this question is archived + "is_locked": ... # boolean for whether this question is locked + "is_solved": ... # boolean for whether this question is solved + "is_spam": ... # boolean for whether this question is spam + "is_taken": ... # FIXME: + "last_answer": ... # id for the last answer + "num_answers": ... # total number of answers + "locale": ... # the locale for the question + "metadata": ... # metadata collected for the question + "tags": ... # tags for the qeustion + "num_votes_past_week": ... # the number of votes in the last week + "num_votes": ... # the total number of votes + "product": ... # the product + "solution": ... # id of answer marked as a solution if any + "taken_until": ... # FIXME: + "taken_by": ... # FIXME: + "title": ... # title of the question + "topic": ... # FIXME: + "updated_by": ... # FIXME: + "updated": ... # FIXME: + }, + ... + ] + } + + +Examples +-------- + +Using curl:: + + curl -X GET "http://localhost:8000/api/2/search/suggest/?q=videos" + + curl -X GET "http://localhost:8000/api/2/search/suggest/?q=videos&max_documents=3&max_questions=3" + + curl -X GET "http://localhost:8000/api/2/search/suggest/" \ + -H "Content-Type: application/json" \ + -d ' + { + "q": "videos", + "max_documents": 3, + "max_questions": 0 + }' diff --git a/docs/index.rst b/docs/index.rst index 6b0991d94b8..a4301ac1f7d 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -43,6 +43,7 @@ Part 3: SUMO .. toctree:: :maxdepth: 2 + api bots deployments sop