From 03ad60dcb8c44bb60c7960ed97816a9efa4dd4e3 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 8 Jan 2019 19:10:40 -0500 Subject: [PATCH 1/7] Tests --- readthedocs/rtd_tests/tests/test_redirects.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_redirects.py b/readthedocs/rtd_tests/tests/test_redirects.py index 335c8feba86..fcac05bd6c6 100644 --- a/readthedocs/rtd_tests/tests/test_redirects.py +++ b/readthedocs/rtd_tests/tests/test_redirects.py @@ -54,6 +54,14 @@ def test_proper_page_on_main_site(self): self.assertEqual(r['Location'], 'http://readthedocs.org/docs/pip/en/latest/test.html') + def test_page_redirect_with_query_params(self): + r = self.client.get('/docs/pip/page/test.html?foo=bar') + self.assertEqual(r.status_code, 302) + self.assertEqual( + r['Location'], + 'http://readthedocs.org/docs/pip/en/latest/test.html?foo=bar' + ) + # If slug is neither valid lang nor valid version, it should 404. # TODO: This should 404 directly, not redirect first def test_improper_url_with_nonexistent_slug(self): @@ -175,6 +183,21 @@ def test_redirect_page(self): self.assertEqual( r['Location'], 'http://pip.readthedocs.org/en/latest/tutorial/install.html') + @override_settings(USE_SUBDOMAIN=True) + def test_redirect_with_query_params(self): + Redirect.objects.create( + project=self.pip, redirect_type='page', + from_url='/install.html', to_url='/tutorial/install.html' + ) + r = self.client.get( + '/install.html?foo=bar', HTTP_HOST='pip.readthedocs.org' + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r['Location'], + 'http://pip.readthedocs.org/en/latest/tutorial/install.html?foo=bar' + ) + @override_settings(USE_SUBDOMAIN=True) def test_redirect_exact(self): Redirect.objects.create( From 828e231a3a6495e83ba8f7ce72190746259bafd0 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 8 Jan 2019 19:18:13 -0500 Subject: [PATCH 2/7] Allow query params in redirects --- readthedocs/core/views/__init__.py | 2 +- readthedocs/redirects/managers.py | 1 + readthedocs/redirects/utils.py | 23 ++++++++++++++++------- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/readthedocs/core/views/__init__.py b/readthedocs/core/views/__init__.py index 6f25e9bd92b..113317b7db2 100644 --- a/readthedocs/core/views/__init__.py +++ b/readthedocs/core/views/__init__.py @@ -113,7 +113,7 @@ def server_error_404(request, exception=None, template_name='404.html'): # pyli Marking exception as optional to make /404/ testing page to work. """ - response = get_redirect_response(request, path=request.get_full_path()) + response = get_redirect_response(request, full_path=request.get_full_path()) if response: if response.url == request.build_absolute_uri(): diff --git a/readthedocs/redirects/managers.py b/readthedocs/redirects/managers.py index 9c0f1bf47fa..e8fe799a3a7 100644 --- a/readthedocs/redirects/managers.py +++ b/readthedocs/redirects/managers.py @@ -12,6 +12,7 @@ def get_redirect_path(self, path, language=None, version_slug=None): path=path, language=language, version_slug=version_slug) if new_path: return new_path + return None RedirectManager = Manager.from_queryset(RedirectQuerySet) diff --git a/readthedocs/redirects/utils.py b/readthedocs/redirects/utils.py index 1edc628626a..83714c88538 100644 --- a/readthedocs/redirects/utils.py +++ b/readthedocs/redirects/utils.py @@ -8,9 +8,12 @@ handlers, so that redirects only take effect if no other view matches. """ from __future__ import absolute_import -from django.http import HttpResponseRedirect + import logging import re +from urllib.parse import urlparse, urlunparse + +from django.http import HttpResponseRedirect from readthedocs.constants import LANGUAGES_REGEX from readthedocs.projects.models import Project @@ -65,22 +68,28 @@ def language_and_version_from_path(path): return None, None, path -def get_redirect_response(request, path): - project, path = project_and_path_from_request(request, path) +def get_redirect_response(request, full_path): + project, full_path = project_and_path_from_request(request, full_path) if not project: return None language = None version_slug = None + schema, netloc, path, params, query, fragments = urlparse(full_path) if not project.single_version: - language, version_slug, path = language_and_version_from_path(path) + language, version_slug, path = language_and_version_from_path( + path + ) - new_path = project.redirects.get_redirect_path( - path=path, language=language, version_slug=version_slug) + path = project.redirects.get_redirect_path( + path=path, language=language, version_slug=version_slug + ) - if new_path is None: + if path is None: return None + new_path = urlunparse((schema, netloc, path, params, query, fragments)) + # Re-use the domain and protocol used in the current request. # Redirects shouldn't change the domain, version or language. new_path = request.build_absolute_uri(new_path) From bf3b888d55eb146a28dac8b11eeabe12a01f6cb7 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 8 Jan 2019 20:26:22 -0500 Subject: [PATCH 3/7] One more test --- readthedocs/rtd_tests/tests/test_redirects.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_redirects.py b/readthedocs/rtd_tests/tests/test_redirects.py index fcac05bd6c6..4fe4f358200 100644 --- a/readthedocs/rtd_tests/tests/test_redirects.py +++ b/readthedocs/rtd_tests/tests/test_redirects.py @@ -1,10 +1,11 @@ from __future__ import absolute_import + +import logging + from django.http import Http404 from django.test import TestCase from django.test.utils import override_settings - -from django_dynamic_fixture import get -from django_dynamic_fixture import fixture +from django_dynamic_fixture import fixture, get from mock import patch from readthedocs.builds.constants import LATEST @@ -12,8 +13,6 @@ from readthedocs.projects.models import Project from readthedocs.redirects.models import Redirect -import logging - @override_settings(PUBLIC_DOMAIN='readthedocs.org', USE_SUBDOMAIN=False, APPEND_SLASH=False) class RedirectTests(TestCase): @@ -100,6 +99,15 @@ def test_proper_subdomain(self): self.assertEqual( r['Location'], 'http://pip.readthedocs.org/en/latest/') + @override_settings(USE_SUBDOMAIN=True) + def test_root_redirect_with_query_params(self): + r = self.client.get('/?foo=bar', HTTP_HOST='pip.readthedocs.org') + self.assertEqual(r.status_code, 302) + self.assertEqual( + r['Location'], + 'http://pip.readthedocs.org/en/latest/?foo=bar' + ) + # Specific Page Redirects @override_settings(USE_SUBDOMAIN=True) def test_proper_page_on_subdomain(self): From 41c4a81457d5ecb0fcb7c98214b7575b110ccf38 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 8 Jan 2019 20:27:06 -0500 Subject: [PATCH 4/7] Pass query_params to resolver --- readthedocs/core/resolver.py | 18 +++++++++--------- readthedocs/core/views/serve.py | 27 ++++++++++++++++++++------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index 43fce68b3f6..9d2f7e1ce14 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -1,13 +1,15 @@ """URL resolver for documentation.""" from __future__ import absolute_import -from builtins import object + import re +from urllib.parse import urlunparse +from builtins import object from django.conf import settings -from readthedocs.projects.constants import PRIVATE, PUBLIC from readthedocs.core.utils.extend import SettingsOverrideObject +from readthedocs.projects.constants import PRIVATE, PUBLIC class ResolverBase(object): @@ -138,8 +140,8 @@ def resolve_domain(self, project, private=None): return getattr(settings, 'PRODUCTION_DOMAIN') - def resolve(self, project, require_https=False, filename='', private=None, - **kwargs): + def resolve(self, project, require_https=False, filename='', + query_params='', private=None, **kwargs): if private is None: version_slug = kwargs.get('version_slug') if version_slug is None: @@ -170,12 +172,10 @@ def resolve(self, project, require_https=False, filename='', private=None, ]) protocol = 'https' if use_https_protocol else 'http' - return '{protocol}://{domain}{path}'.format( - protocol=protocol, - domain=domain, - path=self.resolve_path(project, filename=filename, private=private, - **kwargs), + path = self.resolve_path( + project, filename=filename, private=private, **kwargs ) + return urlunparse((protocol, domain, path, '', query_params, '')) def _get_canonical_project(self, project, projects=None): """ diff --git a/readthedocs/core/views/serve.py b/readthedocs/core/views/serve.py index 36510440ae3..929038fee0e 100644 --- a/readthedocs/core/views/serve.py +++ b/readthedocs/core/views/serve.py @@ -26,17 +26,21 @@ """ from __future__ import ( - absolute_import, division, print_function, unicode_literals) + absolute_import, + division, + print_function, + unicode_literals, +) import logging import mimetypes import os from functools import wraps +from urllib.parse import urlparse from django.conf import settings -from django.http import HttpResponse, HttpResponseRedirect, Http404 -from django.shortcuts import get_object_or_404 -from django.shortcuts import render +from django.http import Http404, HttpResponse, HttpResponseRedirect +from django.shortcuts import get_object_or_404, render from django.views.static import serve from readthedocs.builds.models import Version @@ -46,6 +50,7 @@ from readthedocs.projects import constants from readthedocs.projects.models import Project, ProjectRelationship + log = logging.getLogger(__name__) @@ -102,15 +107,23 @@ def inner_view(request, project=None, project_slug=None, *args, **kwargs): # no @map_subproject_slug def redirect_project_slug(request, project, subproject): # pylint: disable=unused-argument """Handle / -> /en/latest/ directs on subdomains.""" - return HttpResponseRedirect(resolve(subproject or project)) + urlparse_result = urlparse(request.get_full_path()) + return HttpResponseRedirect(resolve( + subproject or project, + query_params=urlparse_result.query, + )) @map_project_slug @map_subproject_slug def redirect_page_with_filename(request, project, subproject, filename): # pylint: disable=unused-argument # noqa """Redirect /page/file.html to /en/latest/file.html.""" - return HttpResponseRedirect( - resolve(subproject or project, filename=filename)) + urlparse_result = urlparse(request.get_full_path()) + return HttpResponseRedirect(resolve( + subproject or project, + filename=filename, + query_params=urlparse_result.query, + )) def _serve_401(request, project): From 6d05ea52c5c1f6b5b2ae4db3fa98298f77006fd3 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Jan 2019 18:35:53 -0500 Subject: [PATCH 5/7] Precommit --- readthedocs/core/resolver.py | 52 ++++++++++++++++++------------- readthedocs/core/views/serve.py | 52 ++++++++++++++++++------------- readthedocs/redirects/managers.py | 6 ++-- readthedocs/redirects/utils.py | 14 +++------ 4 files changed, 69 insertions(+), 55 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index 9d2f7e1ce14..c76557a55c9 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -1,11 +1,9 @@ +# -*- coding: utf-8 -*- """URL resolver for documentation.""" -from __future__ import absolute_import - import re from urllib.parse import urlunparse -from builtins import object from django.conf import settings from readthedocs.core.utils.extend import SettingsOverrideObject @@ -53,9 +51,11 @@ class ResolverBase(object): /docs//projects// """ - def base_resolve_path(self, project_slug, filename, version_slug=None, - language=None, private=False, single_version=None, - subproject_slug=None, subdomain=None, cname=None): + def base_resolve_path( + self, project_slug, filename, version_slug=None, language=None, + private=False, single_version=None, subproject_slug=None, + subdomain=None, cname=None + ): """Resolve a with nothing smart, just filling in the blanks.""" # Only support `/docs/project' URLs outside our normal environment. Normally # the path should always have a subdomain or CNAME domain @@ -74,14 +74,18 @@ def base_resolve_path(self, project_slug, filename, version_slug=None, url += u'{language}/{version_slug}/{filename}' return url.format( - project_slug=project_slug, filename=filename, - version_slug=version_slug, language=language, - single_version=single_version, subproject_slug=subproject_slug, + project_slug=project_slug, + filename=filename, + version_slug=version_slug, + language=language, + single_version=single_version, + subproject_slug=subproject_slug, ) - def resolve_path(self, project, filename='', version_slug=None, - language=None, single_version=None, subdomain=None, - cname=None, private=None): + def resolve_path( + self, project, filename='', version_slug=None, language=None, + single_version=None, subdomain=None, cname=None, private=None + ): """Resolve a URL with a subset of fields defined.""" cname = cname or project.domains.filter(canonical=True).first() version_slug = version_slug or project.get_default_version() @@ -140,8 +144,10 @@ def resolve_domain(self, project, private=None): return getattr(settings, 'PRODUCTION_DOMAIN') - def resolve(self, project, require_https=False, filename='', - query_params='', private=None, **kwargs): + def resolve( + self, project, require_https=False, filename='', query_params='', + private=None, **kwargs + ): if private is None: version_slug = kwargs.get('version_slug') if version_slug is None: @@ -212,7 +218,7 @@ def _get_project_subdomain(self, project): if self._use_subdomain(): project = self._get_canonical_project(project) subdomain_slug = project.slug.replace('_', '-') - return "%s.%s" % (subdomain_slug, public_domain) + return '%s.%s' % (subdomain_slug, public_domain) def _get_project_custom_domain(self, project): return project.domains.filter(canonical=True).first() @@ -223,7 +229,9 @@ def _get_private(self, project, version_slug): version = project.versions.get(slug=version_slug) private = version.privacy_level == PRIVATE except Version.DoesNotExist: - private = getattr(settings, 'DEFAULT_PRIVACY_LEVEL', PUBLIC) == PRIVATE + private = getattr( + settings, 'DEFAULT_PRIVACY_LEVEL', PUBLIC + ) == PRIVATE return private def _fix_filename(self, project, filename): @@ -241,17 +249,17 @@ def _fix_filename(self, project, filename): if filename: if filename.endswith('/') or filename.endswith('.html'): path = filename - elif project.documentation_type == "sphinx_singlehtml": - path = "index.html#document-" + filename - elif project.documentation_type in ["sphinx_htmldir", "mkdocs"]: - path = filename + "/" + elif project.documentation_type == 'sphinx_singlehtml': + path = 'index.html#document-' + filename + elif project.documentation_type in ['sphinx_htmldir', 'mkdocs']: + path = filename + '/' elif '#' in filename: # do nothing if the filename contains URL fragments path = filename else: - path = filename + ".html" + path = filename + '.html' else: - path = "" + path = '' return path def _use_custom_domain(self, custom_domain): diff --git a/readthedocs/core/views/serve.py b/readthedocs/core/views/serve.py index 1151acac1b2..01e6cb0b64d 100644 --- a/readthedocs/core/views/serve.py +++ b/readthedocs/core/views/serve.py @@ -1,4 +1,5 @@ # -*- coding: utf-8 -*- + """ Doc serving from Python. @@ -25,13 +26,6 @@ SERVE_DOCS (['private']) - The list of ['private', 'public'] docs to serve. """ -from __future__ import ( - absolute_import, - division, - print_function, - unicode_literals, -) - import logging import mimetypes import os @@ -63,8 +57,11 @@ def map_subproject_slug(view_func): .. warning:: Does not take into account any kind of privacy settings. """ + @wraps(view_func) - def inner_view(request, subproject=None, subproject_slug=None, *args, **kwargs): # noqa + def inner_view( + request, subproject=None, subproject_slug=None, *args, **kwargs + ): # noqa if subproject is None and subproject_slug: # Try to fetch by subproject alias first, otherwise we might end up # redirected to an unrelated project. @@ -90,8 +87,11 @@ def map_project_slug(view_func): .. warning:: Does not take into account any kind of privacy settings. """ + @wraps(view_func) - def inner_view(request, project=None, project_slug=None, *args, **kwargs): # noqa + def inner_view( + request, project=None, project_slug=None, *args, **kwargs + ): # noqa if project is None: if not project_slug: project_slug = request.slug @@ -109,10 +109,12 @@ def inner_view(request, project=None, project_slug=None, *args, **kwargs): # no def redirect_project_slug(request, project, subproject): # pylint: disable=unused-argument """Handle / -> /en/latest/ directs on subdomains.""" urlparse_result = urlparse(request.get_full_path()) - return HttpResponseRedirect(resolve( - subproject or project, - query_params=urlparse_result.query, - )) + return HttpResponseRedirect( + resolve( + subproject or project, + query_params=urlparse_result.query, + ) + ) @map_project_slug @@ -120,11 +122,13 @@ def redirect_project_slug(request, project, subproject): # pylint: disable=unus def redirect_page_with_filename(request, project, subproject, filename): # pylint: disable=unused-argument # noqa """Redirect /page/file.html to /en/latest/file.html.""" urlparse_result = urlparse(request.get_full_path()) - return HttpResponseRedirect(resolve( - subproject or project, - filename=filename, - query_params=urlparse_result.query, - )) + return HttpResponseRedirect( + resolve( + subproject or project, + filename=filename, + query_params=urlparse_result.query, + ) + ) def _serve_401(request, project): @@ -142,7 +146,8 @@ def _serve_file(request, filename, basepath): # Serve from Nginx content_type, encoding = mimetypes.guess_type( - os.path.join(basepath, filename)) + os.path.join(basepath, filename) + ) content_type = content_type or 'application/octet-stream' response = HttpResponse(content_type=content_type) if encoding: @@ -169,8 +174,10 @@ def _serve_file(request, filename, basepath): @map_subproject_slug def serve_docs( request, project, subproject, lang_slug=None, version_slug=None, - filename=''): - """Exists to map existing proj, lang, version, filename views to the file format.""" + filename='' +): + """Exists to map existing proj, lang, version, filename views to the file + format.""" if not version_slug: version_slug = project.get_default_version() try: @@ -235,4 +242,5 @@ def _serve_symlink_docs(request, project, privacy_level, filename=''): files_tried.append(os.path.join(basepath, filename)) raise Http404( - 'File not found. Tried these files: %s' % ','.join(files_tried)) + 'File not found. Tried these files: %s' % ','.join(files_tried) + ) diff --git a/readthedocs/redirects/managers.py b/readthedocs/redirects/managers.py index e8fe799a3a7..b7e5a50e3c1 100644 --- a/readthedocs/redirects/managers.py +++ b/readthedocs/redirects/managers.py @@ -1,15 +1,17 @@ +# -*- coding: utf-8 -*- """Manager and queryset for the redirects app.""" -from __future__ import absolute_import from django.db.models import Manager from django.db.models.query import QuerySet class RedirectQuerySet(QuerySet): + def get_redirect_path(self, path, language=None, version_slug=None): for redirect in self.select_related('project'): new_path = redirect.get_redirect_path( - path=path, language=language, version_slug=version_slug) + path=path, language=language, version_slug=version_slug + ) if new_path: return new_path return None diff --git a/readthedocs/redirects/utils.py b/readthedocs/redirects/utils.py index 83714c88538..1d23f3f0ca3 100644 --- a/readthedocs/redirects/utils.py +++ b/readthedocs/redirects/utils.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """ Redirection view support. @@ -7,8 +8,6 @@ These are not used directly as views; they are instead included into 404 handlers, so that redirects only take effect if no other view matches. """ -from __future__ import absolute_import - import logging import re from urllib.parse import urlparse, urlunparse @@ -38,9 +37,7 @@ def project_and_path_from_request(request, path): elif path.startswith('/docs/'): # In this case we use the docs without subdomains. So let's strip the # docs prefix. - match = re.match( - r'^/docs/(?P[^/]+)(?P/.*)$', - path) + match = re.match(r'^/docs/(?P[^/]+)(?P/.*)$', path) if match: project_slug = match.groupdict()['project_slug'] path = match.groupdict()['path'] @@ -59,7 +56,8 @@ def project_and_path_from_request(request, path): def language_and_version_from_path(path): match = re.match( r'^/(?P%s)/(?P[^/]+)(?P/.*)$' % LANGUAGES_REGEX, - path) + path + ) if match: language = match.groupdict()['language'] version_slug = match.groupdict()['version_slug'] @@ -77,9 +75,7 @@ def get_redirect_response(request, full_path): version_slug = None schema, netloc, path, params, query, fragments = urlparse(full_path) if not project.single_version: - language, version_slug, path = language_and_version_from_path( - path - ) + language, version_slug, path = language_and_version_from_path(path) path = project.redirects.get_redirect_path( path=path, language=language, version_slug=version_slug From a6c8d3727216fde43be24679a846f34e17f31d4e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 21 Jan 2019 19:03:40 -0500 Subject: [PATCH 6/7] Pre-commit --- readthedocs/core/resolver.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index 774d0f9e383..0d6e8f901ab 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -160,8 +160,8 @@ def resolve_domain(self, project, private=None): return getattr(settings, 'PRODUCTION_DOMAIN') def resolve( - self, project, require_https=False, filename='', private=None, - **kwargs + self, project, require_https=False, filename='', query_params='', + private=None, **kwargs ): if private is None: version_slug = kwargs.get('version_slug') From daff0d8def018d8723166a8dfd16011a9d270039 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 22 Jan 2019 18:22:48 -0500 Subject: [PATCH 7/7] Linter --- readthedocs/redirects/utils.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/readthedocs/redirects/utils.py b/readthedocs/redirects/utils.py index 63af660bd70..64ca294edd0 100644 --- a/readthedocs/redirects/utils.py +++ b/readthedocs/redirects/utils.py @@ -1,5 +1,3 @@ -# -*- coding: utf-8 -*- - """ Redirection view support. @@ -15,8 +13,6 @@ from django.http import HttpResponseRedirect -from django.http import HttpResponseRedirect - from readthedocs.constants import LANGUAGES_REGEX from readthedocs.projects.models import Project