404: skip not built versions (#10681)

- Don't check for index files
- Don't check for custom 404 pages

Since if the version is marked as not built,
there aren't any files to check.

---------

Co-authored-by: Manuel Kaufmann <[email protected]>

Author: stsewd

readthedocs/proxito/tests/test_full.py

@@ -1007,6 +1007,88 @@ def test_redirects_to_correct_index_ending_without_slash(self):
         self.assertEqual(response['location'], '/en/fancy-version/not-found/README.html')
 
     @mock.patch.object(BuildMediaFileSystemStorageTest, 'open')
+    def test_404_index_redirect_skips_not_built_versions(self, storage_open):
+        self.version.built = False
+        self.version.save()
+        get(
+            HTMLFile,
+            project=self.project,
+            version=self.version,
+            path="foo/index.html",
+            name="index.html",
+        )
+
+        response = self.client.get(
+            reverse(
+                "proxito_404_handler",
+                kwargs={"proxito_path": "/en/latest/foo"},
+            ),
+            headers={"host": "project.readthedocs.io"},
+        )
+        self.assertEqual(response.status_code, 404)
+        storage_open.assert_not_called()
+
+    @mock.patch.object(BuildMediaFileSystemStorageTest, "open")
+    def test_custom_404_skips_not_built_versions(self, storage_open):
+        self.version.built = False
+        self.version.save()
+
+        fancy_version = fixture.get(
+            Version,
+            slug="fancy-version",
+            privacy_level=constants.PUBLIC,
+            active=True,
+            built=False,
+            project=self.project,
+        )
+
+        get(
+            HTMLFile,
+            project=self.project,
+            version=self.version,
+            path="404.html",
+            name="404.html",
+        )
+
+        get(
+            HTMLFile,
+            project=self.project,
+            version=fancy_version,
+            path="404.html",
+            name="404.html",
+        )
+
+        response = self.client.get(
+            reverse(
+                "proxito_404_handler",
+                kwargs={"proxito_path": "/en/fancy-version/not-found"},
+            ),
+            headers={"host": "project.readthedocs.io"},
+        )
+        self.assertEqual(response.status_code, 404)
+        storage_open.assert_not_called()
+
+    @mock.patch.object(BuildMediaFileSystemStorageTest, "open")
+    def test_custom_404_doesnt_exist_in_storage(self, storage_open):
+        storage_open.side_effect = FileNotFoundError
+        get(
+            HTMLFile,
+            project=self.project,
+            version=self.version,
+            path="404.html",
+            name="404.html",
+        )
+        response = self.client.get(
+            reverse(
+                "proxito_404_handler",
+                kwargs={"proxito_path": "/en/latest/not-found"},
+            ),
+            headers={"host": "project.readthedocs.io"},
+        )
+        self.assertEqual(response.status_code, 404)
+        storage_open.assert_called_once_with("html/project/latest/404.html")
+
+    @mock.patch.object(BuildMediaFileSystemStorageTest, "open")
     def test_404_storage_serves_custom_404_sphinx_single_html(self, storage_open):
         self.project.versions.update(active=True, built=True)
         fancy_version = fixture.get(

readthedocs/proxito/views/serve.py

@@ -444,8 +444,10 @@ def get(self, request, proxito_path):
         # If we were able to resolve to a valid version, it means that the
         # current file doesn't exist. So we check if we can redirect to its
         # index file if it exists before doing anything else.
+        # If the version isn't marked as built, we don't check for index files,
+        # since the version doesn't have any files.
         # This is /en/latest/foo -> /en/latest/foo/index.html.
-        if version:
+        if version and version.built:
             response = self._get_index_file_redirect(
                 request=request,
                 project=project,
@@ -549,15 +551,18 @@ def _get_custom_404_page(self, request, project, version=None):
 
         We check for a 404.html or 404/index.html file.
 
+        We don't check for a custom 404 page in versions that aren't marked as built,
+        since they don't have any files.
+
         If a 404 page is found, we return a response with the content of that file,
         `None` otherwise.
         """
-        versions_404 = [version] if version else []
+        versions_404 = [version] if version and version.built else []
         if not version or version.slug != project.default_version:
             default_version = project.versions.filter(
                 slug=project.default_version
             ).first()
-            if default_version:
+            if default_version and default_version.built:
                 versions_404.append(default_version)
 
         if not versions_404:
@@ -594,11 +599,15 @@ def _get_custom_404_page(self, request, project, version=None):
                     version_slug_404=version_404.slug,
                     storage_filename_path=storage_filename_path,
                 )
-                resp = HttpResponse(
-                    build_media_storage.open(storage_filename_path).read()
-                )
-                resp.status_code = 404
-                return resp
+                try:
+                    content = build_media_storage.open(storage_filename_path).read()
+                    return HttpResponse(content, status=404)
+                except FileNotFoundError:
+                    log.warning(
+                        "File not found in storage. File out of sync with DB.",
+                        file=storage_filename_path,
+                    )
+                    return None
         return None
 
     def _get_index_file_redirect(self, request, project, version, filename, full_path):