From b9defe5fe1782a1547e9483254f72a4b75106e13 Mon Sep 17 00:00:00 2001 From: Eric Holscher <25510+ericholscher@users.noreply.github.com> Date: Mon, 29 Aug 2022 16:25:01 -0700 Subject: [PATCH] Proxito redirects: pass full_path instead of re-creating it. (#9557) * Pass full_path to exact redirect * Don't re-create full_path, we already have it. * Fix submodule * Remove debug logging * Be more defensive & add test * Fix lint * Remove storage exists that isn't used * Raise Http404 in fast_404 so that we still process redirects.. * Mark test xfail for now, and revert breaking test change --- .../proxito/tests/test_old_redirects.py | 22 +++++++++++++++- readthedocs/proxito/views/utils.py | 2 +- readthedocs/redirects/models.py | 26 +++++++++---------- readthedocs/redirects/querysets.py | 1 + 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/readthedocs/proxito/tests/test_old_redirects.py b/readthedocs/proxito/tests/test_old_redirects.py index 20b355556d3..27588c2a237 100644 --- a/readthedocs/proxito/tests/test_old_redirects.py +++ b/readthedocs/proxito/tests/test_old_redirects.py @@ -374,6 +374,27 @@ def test_redirect_exact(self): 'http://project.dev.readthedocs.io/en/latest/tutorial/install.html', ) + @pytest.mark.xfail( + reason="This is hitting fast_404 and not triggering the nginx handler in testing. It works in prod." + ) + def test_redirect_exact_looks_like_version(self): + fixture.get( + Redirect, + project=self.project, + redirect_type="exact", + from_url="/en/versions.json", + to_url="/en/latest/versions.json", + ) + r = self.client.get( + "/en/versions.json", + HTTP_HOST="project.dev.readthedocs.io", + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/versions.json", + ) + def test_redirect_exact_with_rest(self): """ Exact redirects can have a ``$rest`` in the ``from_url``. @@ -914,7 +935,6 @@ def test_redirect_with_301_status(self): "http://project.dev.readthedocs.io/en/latest/tutorial/install.html", ) - @override_settings( PYTHON_MEDIA=True, PUBLIC_DOMAIN="dev.readthedocs.io", diff --git a/readthedocs/proxito/views/utils.py b/readthedocs/proxito/views/utils.py index 81c20632f04..77cb58d7fdb 100644 --- a/readthedocs/proxito/views/utils.py +++ b/readthedocs/proxito/views/utils.py @@ -16,7 +16,7 @@ def fast_404(request, *args, **kwargs): This stops us from running RTD logic in our error handling. We already do this in RTD prod when we fallback to it. """ - return HttpResponse('Not Found.', status=404) + return HttpResponse("Not Found.", status=404) def proxito_404_page_handler(request, exception=None, template_name='404.html'): diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index 8bd9193d0e6..b0e97e96e94 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -166,16 +166,18 @@ def get_full_path(self, filename, language=None, version_slug=None, allow_crossd filename=filename, ) - def get_redirect_path(self, path, language=None, version_slug=None): + def get_redirect_path(self, path, full_path=None, language=None, version_slug=None): method = getattr( self, 'redirect_{type}'.format( type=self.redirect_type, ), ) - return method(path, language=language, version_slug=version_slug) + return method( + path, full_path=full_path, language=language, version_slug=version_slug + ) - def redirect_prefix(self, path, language=None, version_slug=None): + def redirect_prefix(self, path, full_path, language=None, version_slug=None): if path.startswith(self.from_url): log.debug("Redirecting...", redirect=self) # pep8 and blank don't agree on having a space before :. @@ -189,7 +191,7 @@ def redirect_prefix(self, path, language=None, version_slug=None): ) return to - def redirect_page(self, path, language=None, version_slug=None): + def redirect_page(self, path, full_path, language=None, version_slug=None): if path == self.from_url: log.debug('Redirecting...', redirect=self) to = self.get_full_path( @@ -200,11 +202,7 @@ def redirect_page(self, path, language=None, version_slug=None): ) return to - def redirect_exact(self, path, language=None, version_slug=None): - full_path = path - if language and version_slug: - # reconstruct the full path for an exact redirect - full_path = self.get_full_path(path, language, version_slug, allow_crossdomain=False) + def redirect_exact(self, path, full_path, language=None, version_slug=None): if full_path == self.from_url: log.debug('Redirecting...', redirect=self) return self.to_url @@ -215,7 +213,7 @@ def redirect_exact(self, path, language=None, version_slug=None): cut_path = full_path.replace(match, self.to_url, 1) return cut_path - def redirect_sphinx_html(self, path, language=None, version_slug=None): + def redirect_sphinx_html(self, path, full_path, language=None, version_slug=None): for ending in ['/', '/index.html']: if path.endswith(ending): log.debug('Redirecting...', redirect=self) @@ -228,9 +226,11 @@ def redirect_sphinx_html(self, path, language=None, version_slug=None): allow_crossdomain=False, ) - def redirect_sphinx_htmldir(self, path, language=None, version_slug=None): - if path.endswith('.html'): - log.debug('Redirecting...', redirect=self) + def redirect_sphinx_htmldir( + self, path, full_path, language=None, version_slug=None + ): + if path.endswith(".html"): + log.debug("Redirecting...", redirect=self) path = path[1:] # Strip leading slash. to = re.sub('.html$', '/', path) return self.get_full_path( diff --git a/readthedocs/redirects/querysets.py b/readthedocs/redirects/querysets.py index 931547434af..d166477693e 100644 --- a/readthedocs/redirects/querysets.py +++ b/readthedocs/redirects/querysets.py @@ -107,6 +107,7 @@ def get_redirect_path_with_status( for redirect in queryset.select_related('project'): new_path = redirect.get_redirect_path( path=normalized_path, + full_path=normalized_full_path, language=language, version_slug=version_slug, )