Allow query params in redirects

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

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

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

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)

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)

readthedocs/rtd_tests/tests/test_redirects.py

@@ -1,19 +1,18 @@
 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
 from readthedocs.builds.models import Version
 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):
@@ -54,6 +53,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):
@@ -92,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):
@@ -175,6 +191,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(