Merge pull request #5081 from stsewd/allow-query-params-in-redirects

Allow query params in redirects

Author: ericholscher

readthedocs/core/resolver.py

@@ -3,6 +3,7 @@
 """URL resolver for documentation."""
 
 import re
+from urllib.parse import urlunparse
 
 from django.conf import settings
 
@@ -159,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')
@@ -192,12 +193,10 @@ def resolve(
         ])
         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):
         """

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():

readthedocs/core/views/serve.py

@@ -30,6 +30,7 @@
 import mimetypes
 import os
 from functools import wraps
+from urllib.parse import urlparse
 
 from django.conf import settings
 from django.http import Http404, HttpResponse, HttpResponseRedirect
@@ -107,15 +108,26 @@ def inner_view(  # noqa
 @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."""
+    urlparse_result = urlparse(request.get_full_path())
     return HttpResponseRedirect(
-        resolve(subproject or project, filename=filename),
+        resolve(
+            subproject or project,
+            filename=filename,
+            query_params=urlparse_result.query,
+        )
     )
 
 

readthedocs/redirects/managers.py

@@ -17,6 +17,7 @@ def get_redirect_path(self, path, language=None, version_slug=None):
             )
             if new_path:
                 return new_path
+        return None
 
 
 RedirectManager = Manager.from_queryset(RedirectQuerySet)

readthedocs/redirects/utils.py

@@ -1,5 +1,3 @@
-# -*- coding: utf-8 -*-
-
 """
 Redirection view support.
 
@@ -11,6 +9,7 @@
 """
 import logging
 import re
+from urllib.parse import urlparse, urlunparse
 
 from django.http import HttpResponseRedirect
 
@@ -69,25 +68,26 @@ 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)
 
-    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)

readthedocs/rtd_tests/tests/test_redirects.py

@@ -58,6 +58,14 @@ def test_proper_page_on_main_site(self):
             '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):
@@ -97,6 +105,15 @@ def test_proper_subdomain(self):
             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):
@@ -190,6 +207,21 @@ def test_redirect_page(self):
             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(