Skip to content

Proxito: don't check for index.html if the path already ends with /. #10153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions readthedocs/proxito/tests/test_full.py
Original file line number Diff line number Diff line change
Expand Up @@ -796,10 +796,42 @@ def test_404_storage_serves_custom_404_sphinx(self, storage_exists, storage_open
self.assertEqual(response.status_code, 404)

@mock.patch.object(BuildMediaFileSystemStorageTest, 'exists')
def test_redirects_to_correct_index(self, storage_exists):
"""This case is when the project uses a README.html as index."""
def test_redirects_to_correct_index_ending_with_slash(self, storage_exists):
"""When the path ends with a slash, we try README.html as index."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually, we should remove this and stop trying README.html. That's a pretty legacy decision that I don't think it applies anymore.

We should probably start with new projects and keeping this behavior only for old projects and remove this logic finally after a couple of months.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of removing it or documenting it explicitly. This is not expected default functionality from a webserver/host although it isn't a super uncommon reconfiguration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we should start by logging when this happens, so we know how many people it will effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea 💡

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericholscher we are already logging this 😉 . Take a look at this https://onenr.io/0LREdxdX5wa

There are more project than I've expected doing this 😞

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 340 projects in .org and 1 in .com https://onenr.io/0BQ1Gv2BMRx

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should continue this at #9993

self.project.versions.update(active=True, built=True)
fancy_version = fixture.get(
fixture.get(
Version,
slug="fancy-version",
privacy_level=constants.PUBLIC,
active=True,
built=True,
project=self.project,
documentation_type=SPHINX,
)

storage_exists.side_effect = [True]
response = self.client.get(
reverse(
"proxito_404_handler",
kwargs={"proxito_path": "/en/fancy-version/not-found/"},
),
HTTP_HOST="project.readthedocs.io",
)
storage_exists.assert_has_calls(
[
mock.call("html/project/fancy-version/not-found/README.html"),
]
)
self.assertEqual(response.status_code, 302)
self.assertEqual(
response["location"], "/en/fancy-version/not-found/README.html"
)

@mock.patch.object(BuildMediaFileSystemStorageTest, "exists")
def test_redirects_to_correct_index_ending_without_slash(self, storage_exists):
"""When the path doesn't end with a slash, we try both, index.html and README.html."""
self.project.versions.update(active=True, built=True)
fixture.get(
Version,
slug='fancy-version',
privacy_level=constants.PUBLIC,
Expand All @@ -811,8 +843,11 @@ def test_redirects_to_correct_index(self, storage_exists):

storage_exists.side_effect = [False, True]
response = self.client.get(
reverse('proxito_404_handler', kwargs={'proxito_path': '/en/fancy-version/not-found/'}),
HTTP_HOST='project.readthedocs.io',
reverse(
"proxito_404_handler",
kwargs={"proxito_path": "/en/fancy-version/not-found"},
),
HTTP_HOST="project.readthedocs.io",
)
storage_exists.assert_has_calls(
[
Expand Down Expand Up @@ -1143,7 +1178,7 @@ def test_track_broken_link_custom_404(self, storage_exists, storage_open):
HTTP_HOST="project.readthedocs.io",
)
self.assertEqual(resp.status_code, 404)
storage_open.assert_called_once_with("html/project/latest/404.html")
storage_open.assert_called_once()

self.assertEqual(PageView.objects.all().count(), 2)
version = self.project.versions.get(slug="latest")
Expand Down
9 changes: 8 additions & 1 deletion readthedocs/proxito/views/serve.py
Original file line number Diff line number Diff line change
Expand Up @@ -679,8 +679,15 @@ def _get_index_file_redirect(self, request, project, version, filename, full_pat
version_type=self.version_type,
)

tryfiles = ["index.html", "README.html"]
# If the path ends with `/`, we already tried to serve
# the `/index.html` file, so we only need to test for
# the `/README.html` file.
if full_path.endswith("/"):
tryfiles = ["README.html"]
Comment on lines +682 to +687
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good! We are reducing 1 call to S3 for all the 404 pages 💪🏼


# First, check for dirhtml with slash
for tryfile in ("index.html", "README.html"):
for tryfile in tryfiles:
storage_filename_path = build_media_storage.join(
storage_root_path,
f"{filename}/{tryfile}".lstrip("/"),
Expand Down