Merge pull request #6761 from readthedocs/refactor-search-view

Refactor search view to make use of permission_classes

Author: stsewd

readthedocs/api/v2/permissions.py

@@ -91,7 +91,7 @@ class IsAuthorizedToViewVersion(permissions.BasePermission):
     """
     Checks if the user from the request has permissions to see the version.
 
-    This permission class used in the FooterHTML view.
+    This permission class used in the FooterHTML and PageSearchAPIView views.
 
     .. note::
 

readthedocs/search/api.py

@@ -1,16 +1,18 @@
 import itertools
 import logging
 
+from django.shortcuts import get_object_or_404
 from django.utils import timezone
 from rest_framework import generics, serializers
 from rest_framework.exceptions import ValidationError
 from rest_framework.pagination import PageNumberPagination
 
-from readthedocs.projects.models import HTMLFile
+from readthedocs.api.v2.permissions import IsAuthorizedToViewVersion
+from readthedocs.builds.models import Version
+from readthedocs.projects.models import HTMLFile, Project
 from readthedocs.search import tasks, utils
 from readthedocs.search.faceted_search import PageSearch
 
-
 log = logging.getLogger(__name__)
 
 
@@ -60,11 +62,50 @@ def get_inner_hits(self, obj):
 
 class PageSearchAPIView(generics.ListAPIView):
 
-    """Main entry point to perform a search using Elasticsearch."""
+    """
+    Main entry point to perform a search using Elasticsearch.
+
+    Required query params:
+    - q (search term)
+    - project
+    - version
+
+    .. note::
+
+       The methods `_get_project` and `_get_version`
+       are called many times, so a basic cache is implemented.
+    """
 
+    permission_classes = [IsAuthorizedToViewVersion]
     pagination_class = SearchPagination
     serializer_class = PageSearchSerializer
 
+    def _get_project(self):
+        cache_key = '_cached_project'
+        project = getattr(self, cache_key, None)
+
+        if not project:
+            project_slug = self.request.GET.get('project', None)
+            project = get_object_or_404(Project, slug=project_slug)
+            setattr(self, cache_key, project)
+
+        return project
+
+    def _get_version(self):
+        cache_key = '_cached_version'
+        version = getattr(self, cache_key, None)
+
+        if not version:
+            version_slug = self.request.GET.get('version', None)
+            project = self._get_project()
+            version = get_object_or_404(
+                project.versions.all(),
+                slug=version_slug,
+            )
+            setattr(self, cache_key, version)
+
+        return version
+
     def get_queryset(self):
         """
         Return Elasticsearch DSL Search object instead of Django Queryset.
@@ -78,7 +119,8 @@ def get_queryset(self):
         query = self.request.query_params.get('q', '')
         kwargs = {'filter_by_user': False, 'filters': {}}
         kwargs['filters']['project'] = [p.slug for p in self.get_all_projects()]
-        kwargs['filters']['version'] = self.request.query_params.get('version')
+        kwargs['filters']['version'] = self._get_version().slug
+        # Check to avoid searching all projects in case project is empty.
         if not kwargs['filters']['project']:
             log.info("Unable to find a project to search")
             return HTMLFile.objects.none()
@@ -118,19 +160,26 @@ def get_serializer_context(self):
 
     def get_all_projects(self):
         """
-        Return a list containing the project itself and all its subprojects.
-
-        The project slug is retrieved from ``project`` query param.
+        Return a list of the project itself and all its subprojects the user has permissions over.
 
         :rtype: list
-
-        :raises: Http404 if project is not found
         """
-        project_slug = self.request.query_params.get('project')
-        version_slug = self.request.query_params.get('version')
-        all_projects = utils.get_project_list_or_404(
-            project_slug=project_slug, user=self.request.user, version_slug=version_slug,
+        main_version = self._get_version()
+        main_project = self._get_project()
+
+        subprojects = Project.objects.filter(
+            superprojects__parent_id=main_project.id,
         )
+        all_projects = []
+        for project in list(subprojects) + [main_project]:
+            version = (
+                Version.internal
+                .public(user=self.request.user, project=project)
+                .filter(slug=main_version.slug)
+                .first()
+            )
+            if version:
+                all_projects.append(version.project)
         return all_projects
 
     def get_all_projects_url(self):
@@ -151,7 +200,7 @@ def get_all_projects_url(self):
         :rtype: dict
         """
         all_projects = self.get_all_projects()
-        version_slug = self.request.query_params.get('version')
+        version_slug = self._get_version().slug
         projects_url = {}
         for project in all_projects:
             projects_url[project.slug] = project.get_docs_url(version_slug=version_slug)
@@ -162,8 +211,8 @@ def list(self, request, *args, **kwargs):
 
         response = super().list(request, *args, **kwargs)
 
-        project_slug = self.request.query_params.get('project', None)
-        version_slug = self.request.query_params.get('version', None)
+        project_slug = self._get_project().slug
+        version_slug = self._get_version().slug
         total_results = response.data.get('count', 0)
         time = timezone.now()
 

readthedocs/search/tests/test_api.py

@@ -1,4 +1,5 @@
 import re
+from unittest import mock
 
 import pytest
 from django.urls import reverse
@@ -7,6 +8,7 @@
 from readthedocs.builds.models import Version
 from readthedocs.projects.constants import PUBLIC
 from readthedocs.projects.models import HTMLFile, Project
+from readthedocs.search.api import PageSearchAPIView
 from readthedocs.search.documents import PageDocument
 from readthedocs.search.tests.utils import (
     DOMAIN_FIELDS,
@@ -199,11 +201,17 @@ def test_doc_search_pagination(self, api_client, project):
         assert len(resp.data['results']) == 5
 
     def test_doc_search_without_parameters(self, api_client, project):
-        """Hitting Document Search endpoint without query parameters should return error"""
+        """Hitting Document Search endpoint without project and version should return 404."""
         resp = self.get_search(api_client, {})
+        assert resp.status_code == 404
+
+    def test_doc_search_without_query(self, api_client, project):
+        """Hitting Document Search endpoint without a query should return error."""
+        resp = self.get_search(
+            api_client, {'project': project.slug, 'version': project.versions.first().slug})
         assert resp.status_code == 400
         # Check error message is there
-        assert sorted(['q', 'project', 'version']) == sorted(resp.data.keys())
+        assert 'q' in resp.data.keys()
 
     def test_doc_search_subprojects(self, api_client, all_projects):
         """Test Document search return results from subprojects also"""
@@ -255,6 +263,20 @@ def test_doc_search_unexisting_version(self, api_client, project):
             'version': version,
         }
         resp = self.get_search(api_client, search_params)
+        assert resp.status_code == 404
+
+    @mock.patch.object(PageSearchAPIView, 'get_all_projects', list)
+    def test_get_all_projects_returns_empty_results(self, api_client, project):
+        """If there is a case where `get_all_projects` returns empty, we could be querying all projects."""
+
+        # `documentation` word is present both in `kuma` and `docs` files
+        # and not in `pipeline`, so search with this phrase but filter through project
+        search_params = {
+            'q': 'documentation',
+            'project': 'docs',
+            'version': 'latest'
+        }
+        resp = self.get_search(api_client, search_params)
         assert resp.status_code == 200
 
         data = resp.data['results']

readthedocs/search/utils.py

@@ -82,27 +82,6 @@ def remove_indexed_files(model, project_slug, version_slug=None, build_id=None):
         log.exception('Unable to delete a subset of files. Continuing.')
 
 
-# TODO: Rewrite all the views using this in Class Based View,
-# and move this function to a mixin
-def get_project_list_or_404(project_slug, user, version_slug=None):
-    """
-    Return list of project and its subprojects.
-
-    It filters by Version privacy instead of Project privacy,
-    so we can support public versions on private projects.
-    """
-    project_list = []
-    main_project = get_object_or_404(Project, slug=project_slug)
-    subprojects = Project.objects.filter(superprojects__parent_id=main_project.id)
-    for project in list(subprojects) + [main_project]:
-        version = Version.internal.public(user).filter(
-            project__slug=project.slug, slug=version_slug
-        )
-        if version.exists():
-            project_list.append(version.first().project)
-    return project_list
-
-
 def _get_index(indices, index_name):
     """
     Get Index from all the indices.