From 5aae20805bfb6a09bb5c910da71bcc31134a0418 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 29 Aug 2023 11:27:02 -0500 Subject: [PATCH 1/3] 404: skip not built versions - 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. --- readthedocs/proxito/tests/test_full.py | 62 ++++++++++++++++++++++++++ readthedocs/proxito/views/serve.py | 11 +++-- 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/readthedocs/proxito/tests/test_full.py b/readthedocs/proxito/tests/test_full.py index 715bc79c92a..48982743635 100644 --- a/readthedocs/proxito/tests/test_full.py +++ b/readthedocs/proxito/tests/test_full.py @@ -1007,6 +1007,68 @@ 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_404_storage_serves_custom_404_sphinx_single_html(self, storage_open): self.project.versions.update(active=True, built=True) fancy_version = fixture.get( diff --git a/readthedocs/proxito/views/serve.py b/readthedocs/proxito/views/serve.py index 35040c025fa..9c5c7ea67da 100644 --- a/readthedocs/proxito/views/serve.py +++ b/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: From e534fb097ba8016f714a1ce23058b92a25501565 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 29 Aug 2023 13:00:54 -0500 Subject: [PATCH 2/3] Catch FileNotFoundError exception --- readthedocs/proxito/tests/test_full.py | 20 ++++++++++++++++++++ readthedocs/proxito/views/serve.py | 14 +++++++++----- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/readthedocs/proxito/tests/test_full.py b/readthedocs/proxito/tests/test_full.py index 48982743635..9425dbcd16a 100644 --- a/readthedocs/proxito/tests/test_full.py +++ b/readthedocs/proxito/tests/test_full.py @@ -1068,6 +1068,26 @@ def test_custom_404_skips_not_built_versions(self, storage_open): 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) diff --git a/readthedocs/proxito/views/serve.py b/readthedocs/proxito/views/serve.py index 9c5c7ea67da..86e5b1bfbe4 100644 --- a/readthedocs/proxito/views/serve.py +++ b/readthedocs/proxito/views/serve.py @@ -599,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() + except FileNotFoundError: + log.warning( + "File not found in storage. File out of sync with DB.", + file=storage_filename_path, + ) + return None + return HttpResponse(content, status=404) return None def _get_index_file_redirect(self, request, project, version, filename, full_path): From 2dc9c64b371d0f4d9b9870f587dcb2d2a4d2717d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 31 Aug 2023 11:31:02 -0500 Subject: [PATCH 3/3] Update readthedocs/proxito/views/serve.py Co-authored-by: Manuel Kaufmann --- readthedocs/proxito/views/serve.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/proxito/views/serve.py b/readthedocs/proxito/views/serve.py index 86e5b1bfbe4..313740a5aa0 100644 --- a/readthedocs/proxito/views/serve.py +++ b/readthedocs/proxito/views/serve.py @@ -601,13 +601,13 @@ def _get_custom_404_page(self, request, project, version=None): ) 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 HttpResponse(content, status=404) return None def _get_index_file_redirect(self, request, project, version, filename, full_path):