diff --git a/docs/user/api/v3.rst b/docs/user/api/v3.rst index 09d178c3f7d..3783b3c5cd5 100644 --- a/docs/user/api/v3.rst +++ b/docs/user/api/v3.rst @@ -1145,6 +1145,11 @@ Redirect details "from_url": "/docs/", "to_url": "/documentation/", "type": "page", + "http_status": 302, + "description": "", + "enabled": true, + "force": false, + "position": 0, "_links": { "_self": "/api/v3/projects/pip/redirects/1/", "project": "/api/v3/projects/pip/" @@ -1227,18 +1232,21 @@ Redirect create { "from_url": "/docs/", "to_url": "/documentation/", - "type": "page" + "type": "page", + "position": 0, } .. note:: - ``type`` can be one of ``prefix``, ``page``, ``exact``, ``sphinx_html`` and ``sphinx_htmldir``. + - ``type`` can be one of ``page``, ``exact``, ``clean_url_to_html`` and ``html_to_clean_url``. + - Depending on the ``type`` of the redirect, some fields may not be needed: - Depending on the ``type`` of the redirect, some fields may not be needed: + * ``page`` and ``exact`` types require ``from_url`` and ``to_url``. + * ``clean_url_to_html`` and ``html_to_clean_url`` types do not require ``from_url`` and ``to_url``. - * ``prefix`` type does not require ``to_url``. - * ``page`` and ``exact`` types require ``from_url`` and ``to_url``. - * ``sphinx_html`` and ``sphinx_htmldir`` types do not require ``from_url`` and ``to_url``. + - Forced redirects are not enabled by default, + you can ask for them to be enabled via support. + - Position starts at 0 and is used to order redirects. **Example response**: @@ -1291,6 +1299,11 @@ Redirect update "type": "page" } + .. note:: + + If the position of the redirect is changed, + it will be inserted in the new position and the other redirects will be reordered. + **Example response**: `See Redirect details <#redirect-details>`_ diff --git a/docs/user/guides/redirects.rst b/docs/user/guides/redirects.rst index 5f46b67ff4d..0c77464c538 100644 --- a/docs/user/guides/redirects.rst +++ b/docs/user/guides/redirects.rst @@ -42,12 +42,6 @@ you can mark the choice in order to experiment and **preview** the final rule ge Here is a quick overview of the options available in :guilabel:`Redirect Type`: -Prefix redirect - This option is often relevant when moving a project from a former host to Read the Docs. - In this case, often URL paths hierarchies are redirected. - - Read more about this option in :ref:`user-defined-redirects:Prefix redirects` - Page redirect With this option, you can specify a page in your documentation to redirect elsewhere. @@ -64,18 +58,18 @@ Exact redirect Read more about this option in :ref:`user-defined-redirects:Exact redirects` -Sphinx HTMLDir => HTML - If you choose to change your Sphinx builder from *DirHTML writer* to the default *html5writer*, +Clean URL to HTML + If you choose to change the style of your URLs from *clean URLs* (``/en/latest/tutorial/``) to *HTML URLs* (``/en/latest/tutorial.html``), you can redirect all mismatches automatically. - Read more about this option in :ref:`user-defined-redirects:Sphinx redirects` + Read more about this option in :ref:`user-defined-redirects:Clean/HTML URLs redirects` -Sphinx HTML => HTMLDir +HTML to clean URL Similarly to the former option, - if you choose to change your Sphinx builder from the default *html5writer* to *DirHTML writer*, + if you choose to change the style of your URLs from *HTML URLs* (``/en/latest/tutorial.html``) to *clean URLs* (``/en/latest/tutorial/``), you can redirect all mismatches automatically. - Read more about this option in :ref:`user-defined-redirects:Sphinx redirects` + Read more about this option in :ref:`user-defined-redirects:Clean/HTML URLs redirects` .. note:: @@ -114,3 +108,15 @@ in order to delete a rule or edit it. When editing a rule, you can change its :guilabel:`Redirect Type` and its :guilabel:`From URL` or :guilabel:`To URL`. + +Changing the order of redirects +------------------------------- + +The order of redirects is important, +if you have multiple rules that match the same URL, +the first redirect in the list will be used. + +You can change the order of the redirect from the :menuselection:`Admin > Redirects` page, +by using the up and down arrow buttons. + +New redirects are added at the start of the list (i.e. they have the highest priority). diff --git a/docs/user/user-defined-redirects.rst b/docs/user/user-defined-redirects.rst index 4a1cfe40938..620c6a45bee 100644 --- a/docs/user/user-defined-redirects.rst +++ b/docs/user/user-defined-redirects.rst @@ -24,18 +24,6 @@ the bad user experience of a 404 page is usually best to avoid. :doc:`/guides/deprecating-content` A guide to deprecating features and other topics in a documentation. - -Limitations ------------ - -- By default, redirects only apply on pages that don't exist. - **Forced redirects** allow you to apply redirects on existing pages, - but incur a small performance penalty, so aren't enabled by default. - You can ask for them to be enabled via support. -- Only :ref:`user-defined-redirects:page redirects` and :ref:`user-defined-redirects:exact redirects` - can redirect to URLs outside Read the Docs, - just include the protocol in ``To URL``, e.g ``https://example.com``. - Built-in redirects ------------------ @@ -106,135 +94,249 @@ You can reach these docs at https://docs.rtfd.io. User-defined redirects ---------------------- -Prefix redirects -~~~~~~~~~~~~~~~~ - -The most useful and requested feature of redirects was when migrating to Read the Docs from an old host. -You would have your docs served at a previous URL, -but that URL would break once you moved them. -Read the Docs includes a language and version slug in your documentation, -but not all documentation is hosted this way. - -Say that you previously had your docs hosted at ``https://docs.example.com/dev/``, -you move ``docs.example.com`` to point at Read the Docs. -So users will have a bookmark saved to a page at ``https://docs.example.com/dev/install.html``. +Page redirects +~~~~~~~~~~~~~~ -You can now set a *Prefix Redirect* that will redirect all 404's with a prefix to a new place. -The example configuration would be:: +*Page Redirects* let you redirect a page across all versions of your documentation. - Type: Prefix Redirect - From URL: /dev/ +.. note:: -Your users query would now redirect in the following manner:: + Since pages redirects apply to all versions, + ``From URL`` doesn't need to include the ``//`` prefix (e.g. ``/en/latest``), + but just the version-specific part of the URL. + If you want to set redirects only for some languages or some versions, you should use + :ref:`user-defined-redirects:exact redirects` with the fully-specified path. - docs.example.com/dev/install.html -> - docs.example.com/en/latest/install.html +Exact redirects +~~~~~~~~~~~~~~~ -Where ``en`` and ``latest`` are the default language and version values for your project. +*Exact Redirects* take into account the full URL (including language and version), +allowing you to create a redirect for a specific version or language of your documentation. -.. note:: +Clean/HTML URLs redirects +~~~~~~~~~~~~~~~~~~~~~~~~~ - If you were hosting your docs without a prefix, you can create a ``/`` Prefix Redirect, - which will prepend ``/$lang/$version/`` to all incoming URLs. +If you decide to change the style of the URLs of your documentation, +you can use *Clean URL to HTML* or *HTML to clean URL* redirects to redirect users to the new URL style. +For example, if a previous page was at ``/en/latest/install.html``, +and now is served at ``/en/latest/install/``, or vice versa, +users will be redirected to the new URL. -Page redirects -~~~~~~~~~~~~~~ +Limitations and observations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -A more specific case is when you move a page around in your docs. -The old page will start 404'ing, -and your users will be confused. -*Page Redirects* let you redirect a specific page. +- By default, redirects only apply on pages that don't exist. + **Forced redirects** allow you to apply redirects on existing pages, + but incur a small performance penalty, so aren't enabled by default. + You can ask for them to be enabled via support. +- Redirects aren't applied on :doc:`previews of pull requests `. + You should treat these domains as ephemeral and not rely on them for user-facing content. +- You can redirect to URLs outside Read the Docs, + just include the protocol in ``To URL``, e.g ``https://example.com``. +- A wildcard can be used at the end of ``From URL`` (suffix wildcard) to redirect all pages matching a prefix. + Prefix and infix wildcards are not supported. +- If a wildcard is used in ``From URL``, + the part of the URL that matches the wildcard can be used in ``To URL`` with the ``:splat`` placeholder. +- Redirects without a wildcard match paths with or without a trailing slash, + e.g. ``/install`` matches ``/install`` and ``/install/``. +- The order of redirects matters. + If multiple redirects match the same URL, + the first one will be applied. + The order of redirects :ref:`can be changed from your project's dashboard `. +- If an infinite redirect is detected, a 404 error will be returned, + and no other redirects will be applied. + +Examples +~~~~~~~~ + +Redirecting a page +`````````````````` Say you move the ``example.html`` page into a subdirectory of examples: ``examples/intro.html``. -You would set the following configuration:: +You can create a redirect with the following configuration:: Type: Page Redirect From URL: /example.html To URL: /examples/intro.html -**Page Redirects apply to all versions of your documentation.** -Because of this, -the ``/`` at the start of the ``From URL`` doesn't include the ``/$lang/$version`` prefix (e.g. -``/en/latest``), but just the version-specific part of the URL. -If you want to set redirects only for some languages or some versions, you should use -:ref:`user-defined-redirects:exact redirects` with the fully-specified path. +Users will now be redirected: -Exact redirects -~~~~~~~~~~~~~~~ +- From ``https://docs.example.com/en/latest/example.html`` + to ``https://docs.example.com/en/latest/examples/intro.html``. +- From ``https://docs.example.com/en/stable/example.html`` + to ``https://docs.example.com/en/stable/examples/intro.html``. + +If you want this redirect to apply to a specific version of your documentation, +you can create a redirect with the following configuration:: + + Type: Exact Redirect + From URL: /en/latest/example.html + To URL: /en/latest/examples/intro.html + +.. note:: + + Use the desired version and language instead of ``latest`` and ``en``. + +Redirecting a directory +``````````````````````` + +Say you rename the ``/api/`` directory to ``/api/v1/``. +Instead of creating a redirect for each page in the directory, +you can use a wildcard to redirect all pages in that directory:: + + Type: Page Redirect + From URL: /api/* + To URL: /api/v1/:splat + +Users will now be redirected: + +- From ``https://docs.example.com/en/latest/api/`` + to ``https://docs.example.com/en/latest/api/v1/``. +- From ``https://docs.example.com/en/latest/api/projects.html`` + to ``https://docs.example.com/en/latest/api/v1/projects.html``. + +If you want this redirect to apply to a specific version of your documentation, +you can create a redirect with the following configuration:: + + Type: Exact Redirect + From URL: /en/latest/api/* + To URL: /en/latest/api/v1/:splat + +.. note:: + + Use the desired version and language instead of ``latest`` and ``en``. -*Exact Redirects* are for redirecting a single URL, -taking into account the full URL (including language and version). +Redirecting a directory to a single page +```````````````````````````````````````` -You can also redirect a subset of URLs by including the ``$rest`` keyword -at the end of the ``From URL``. +Say you put the contents of the ``/examples/`` directory into a single page at ``/examples.html``. +You can use a wildcard to redirect all pages in that directory to the new page:: -Exact redirects examples -^^^^^^^^^^^^^^^^^^^^^^^^ + Type: Page Redirect + From URL: /examples/* + To URL: /examples.html -Redirecting a single URL -```````````````````````` +Users will now be redirected: -Say you're moving ``docs.example.com`` to Read the Docs and want to redirect traffic -from an old page at ``https://docs.example.com/dev/install.html`` to a new URL -of ``https://docs.example.com/en/latest/installing-your-site.html``. +- From ``https://docs.example.com/en/latest/examples/`` + to ``https://docs.example.com/en/latest/examples.html``. +- From ``https://docs.example.com/en/latest/examples/intro.html`` + to ``https://docs.example.com/en/latest/examples.html``. -The example configuration would be:: +If you want this redirect to apply to a specific version of your documentation, +you can create a redirect with the following configuration:: Type: Exact Redirect - From URL: /dev/install.html - To URL: /en/latest/installing-your-site.html + From URL: /en/latest/examples/* + To URL: /en/latest/examples.html + +.. note:: + + Use the desired version and language instead of ``latest`` and ``en``. + +Redirecting a page to the latest version +```````````````````````````````````````` + +Say you want your users to always be redirected to the latest version of a page, +your security policy (``/security.html``) for example. +You can use a wildcard with a forced redirect to redirect all versions of that page to the latest version:: + + Type: Page Redirect + From URL: /security.html + To URL: https://docs.example.com/en/latest/security.html + Force Redirect: True + +Users will now be redirected: -Your users query would now redirect in the following manner:: +- From ``https://docs.example.com/en/v1.0/security.html`` + to ``https://docs.example.com/en/latest/security.html``. +- From ``https://docs.example.com/en/v2.5/security.html`` + to ``https://docs.example.com/en/latest/security.html``. - docs.example.com/dev/install.html -> - docs.example.com/en/latest/installing-your-site.html +.. note:: -Note that you should insert the desired language for "en" and version for "latest" to -achieve the desired redirect. + ``To URL`` includes the domain, this is required, + otherwise the redirect will be relative to the current version, + resulting in a redirect to ``https://docs.example.com/en/v1.0/en/latest/security.html``. -Redirecting a whole sub-path to a different one -``````````````````````````````````````````````` +Redirecting an old version to a new one +``````````````````````````````````````` -*Exact Redirects* could be also useful to redirect a whole sub-path to a different one by using a special ``$rest`` keyword in the "From URL". Let's say that you want to redirect your readers of your version ``2.0`` of your documentation under ``/en/2.0/`` because it's deprecated, to the newest ``3.0`` version of it at ``/en/3.0/``. - -This example would be:: +You can use an exact redirect to do so:: Type: Exact Redirect - From URL: /en/2.0/$rest - To URL: /en/3.0/ + From URL: /en/2.0/* + To URL: /en/3.0/:splat + +Users will now be redirected: + +- From ``https://docs.example.com/en/2.0/dev/install.html`` + to ``https://docs.example.com/en/3.0/dev/install.html``. + +.. note:: + + For this redirect to work, your old version must be disabled, + if the version is still active, you can use the ``Force Redirect`` option. + +Creating a shortlink +```````````````````` + +Say you want to redirect ``https://docs.example.com/security`` to ``https://docs.example.com/en/latest/security.html``, +so it's easier to share the link to the page. +You can create a redirect with the following configuration:: -The readers of your documentation will now be redirected as:: + Type: Exact Redirect + From URL: /security + To URL: /en/latest/security.html + +Users will now be redirected: + +- From ``https://docs.example.com/security`` (no trailing slash) + to ``https://docs.example.com/en/latest/security.html``. +- From ``https://docs.example.com/security/`` (trailing slash) + to ``https://docs.example.com/en/latest/security.html``. - docs.example.com/en/2.0/dev/install.html -> - docs.example.com/en/3.0/dev/install.html +Migrating your docs to Read the Docs +```````````````````````````````````` + +Say that you previously had your docs hosted at ``https://docs.example.com/dev/``, +and choose to migrate to Read the Docs with support for multiple versions and translations. +Your documentation will now be served at ``https://docs.example.com/en/latest/``, +but your users may have bookmarks saved with the old URL structure, for example ``https://docs.example.com/dev/install.html``. -Similarly, if you maintain several branches of your documentation (e.g. ``3.0`` and -``latest``) and decide to move pages in ``latest`` but not the older branches, you can use -*Exact Redirects* to do so. +You can use an exact redirect with a wildcard to redirect all pages from the old URL structure to the new one:: + + Type: Exact Redirect + From URL: /dev/* + To URL: /en/latest/:splat + +Users will now be redirected: + +- From ``https://docs.example.com/dev/install.html`` + to ``https://docs.example.com/en/latest/install.html``. Migrating your documentation to another domain `````````````````````````````````````````````` -You can use an exact redirect to migrate your documentation to another domain, +You can use an exact redirect with the force option to migrate your documentation to another domain, for example:: Type: Exact Redirect - From URL: /$rest + From URL: /* To URL: https://newdocs.example.com/ Force Redirect: True -Then all pages will redirect to the new domain, for example -``https://docs.example.com/en/latest/install.html`` will redirect to -``https://newdocs.example.com/en/latest/install.html``. +Users will now be redirected: + +- From ``https://docs.example.com/en/latest/install.html`` + to ``https://newdocs.example.com/en/latest/install.html``. -Sphinx redirects -~~~~~~~~~~~~~~~~ +Changing your Sphinx builder from ``html`` to ``dirhtml`` +````````````````````````````````````````````````````````` -We also support redirects for changing the type of documentation Sphinx is building. -If you switch between *HTMLDir* and *HTML*, your URLs will change. -A page at ``/en/latest/install.html`` will be served at ``/en/latest/install/``, -or vice versa. -The built in redirects for this will handle redirecting users appropriately. +When you change your Sphinx builder from ``html`` to ``dirhtml``, +all your URLs will change from ``/page.html`` to ``/page/``. +You can create a redirect of type ``HTML to clean URL`` to redirect all your old URLs to the new style. diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index cab929f874a..e0c9237388d 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -27,8 +27,9 @@ Project, ProjectRelationship, ) -from readthedocs.redirects.models import TYPE_CHOICES as REDIRECT_TYPE_CHOICES +from readthedocs.redirects.constants import TYPE_CHOICES as REDIRECT_TYPE_CHOICES from readthedocs.redirects.models import Redirect +from readthedocs.redirects.validators import validate_redirect class UserSerializer(FlexFieldsModelSerializer): @@ -851,8 +852,71 @@ class Meta: "type", "from_url", "to_url", + "force", + "enabled", + "description", + "http_status", + "position", "_links", ] + # TODO: allow editing this field for projects that have this feature enabled. + read_only_fields = ["force"] + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + # Allow using the old redirect types, so we can raise the proper error in ``validate_type`. + self._removed_redirects = [ + ("prefix", "Removed, use an `exact` redirect instead."), + ("sphinx_html", "Renamed, use `clean_url_to_html` instead."), + ("sphinx_htmldir", "Renamed, use `html_to_clean_url` instead."), + ] + self.fields["type"].choices = ( + list(REDIRECT_TYPE_CHOICES) + self._removed_redirects + ) + + def validate_type(self, value): + blog_link = "https://blog.readthedocs.com/new-improvements-to-redirects/" + if value == "prefix": + raise serializers.ValidationError( + _( + f"Prefix redirects have been removed. Please use an exact redirect `/prefix/*` instead. See {blog_link}." + ) + ) + if value == "sphinx_html": + raise serializers.ValidationError( + _( + f"sphinx_html redirect has been renamed to clean_url_to_html. See {blog_link}." + ) + ) + if value == "sphinx_htmldir": + raise serializers.ValidationError( + _( + f"sphinx_htmldir redirect has been renamed to html_to_clean_url. See {blog_link}." + ) + ) + return value + + def create(self, validated_data): + validate_redirect( + project=validated_data["project"], + pk=None, + redirect_type=validated_data["redirect_type"], + from_url=validated_data.get("from_url", ""), + to_url=validated_data.get("to_url", ""), + error_class=serializers.ValidationError, + ) + return super().create(validated_data) + + def update(self, instance, validated_data): + validate_redirect( + project=instance.project, + pk=instance.pk, + redirect_type=validated_data["redirect_type"], + from_url=validated_data.get("from_url", ""), + to_url=validated_data.get("to_url", ""), + error_class=serializers.ValidationError, + ) + return super().update(instance, validated_data) class RedirectCreateSerializer(RedirectSerializerBase): diff --git a/readthedocs/api/v3/tests/responses/projects-redirects-detail.json b/readthedocs/api/v3/tests/responses/projects-redirects-detail.json index 87992a9840d..5693fb98d07 100644 --- a/readthedocs/api/v3/tests/responses/projects-redirects-detail.json +++ b/readthedocs/api/v3/tests/responses/projects-redirects-detail.json @@ -5,9 +5,14 @@ }, "created": "2019-04-29T10:00:00Z", "modified": "2019-04-29T12:00:00Z", - "from_url": "/docs/", + "from_url": "/docs", "pk": 1, "project": "project", "type": "page", - "to_url": "/documentation/" + "to_url": "/documentation/", + "http_status": 302, + "description": "", + "enabled": true, + "force": false, + "position": 0 } diff --git a/readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json b/readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json index eab7e6a9e6a..f2cfdbddcc5 100644 --- a/readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json +++ b/readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json @@ -5,9 +5,14 @@ }, "created": "2019-04-29T10:00:00Z", "modified": "2019-04-29T12:00:00Z", - "from_url": "/changed/", + "from_url": "/changed", "pk": 1, "project": "project", "type": "page", - "to_url": "/toanother/" + "to_url": "/toanother/", + "http_status": 302, + "description": "", + "enabled": true, + "force": false, + "position": 0 } diff --git a/readthedocs/api/v3/tests/responses/projects-redirects-list.json b/readthedocs/api/v3/tests/responses/projects-redirects-list.json index ce8ca8ea42a..d4df35ea2b8 100644 --- a/readthedocs/api/v3/tests/responses/projects-redirects-list.json +++ b/readthedocs/api/v3/tests/responses/projects-redirects-list.json @@ -10,11 +10,16 @@ }, "created": "2019-04-29T10:00:00Z", "modified": "2019-04-29T12:00:00Z", - "from_url": "/docs/", + "from_url": "/docs", "pk": 1, "project": "project", "type": "page", - "to_url": "/documentation/" + "to_url": "/documentation/", + "http_status": 302, + "description": "", + "enabled": true, + "force": false, + "position": 0 } ] } diff --git a/readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json b/readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json index 90285fb9406..d1e8633bffe 100644 --- a/readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json +++ b/readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json @@ -5,9 +5,14 @@ }, "created": "2019-04-29T10:00:00Z", "modified": "2019-04-29T12:00:00Z", - "from_url": "/page/", + "from_url": "/page", "pk": 2, "project": "project", "type": "page", - "to_url": "/another/" + "to_url": "/another/", + "http_status": 302, + "description": "", + "enabled": true, + "force": false, + "position": 0 } diff --git a/readthedocs/api/v3/tests/test_redirects.py b/readthedocs/api/v3/tests/test_redirects.py index 05483c12205..3a14cb63c29 100644 --- a/readthedocs/api/v3/tests/test_redirects.py +++ b/readthedocs/api/v3/tests/test_redirects.py @@ -1,8 +1,15 @@ -from .mixins import APIEndpointMixin from django.urls import reverse +from django_dynamic_fixture import get +from readthedocs.redirects.constants import ( + CLEAN_URL_TO_HTML_REDIRECT, + EXACT_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, +) from readthedocs.redirects.models import Redirect +from .mixins import APIEndpointMixin + class RedirectsEndpointTests(APIEndpointMixin): def test_unauthed_projects_redirects_list(self): @@ -118,35 +125,33 @@ def test_projects_redirects_list_post(self): self._get_response_dict("projects-redirects-list_POST"), ) - def test_projects_redirects_type_prefix_list_post(self): - self.assertEqual(Redirect.objects.count(), 1) - data = { - "from_url": "/redirect-this/", - "type": "prefix", - } + def test_projects_redirects_old_type_post(self): + for redirect_type in ["prefix", "sphinx_html", "sphinx_htmldir"]: + self.assertEqual(Redirect.objects.count(), 1) + data = { + "from_url": "/redirect-this/", + "type": redirect_type, + } - self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") - response = self.client.post( - reverse( - "projects-redirects-list", - kwargs={ - "parent_lookup_project__slug": self.project.slug, - }, - ), - data, - ) - self.assertEqual(response.status_code, 201) - self.assertEqual(Redirect.objects.all().count(), 2) - - redirect = Redirect.objects.first() - self.assertEqual(redirect.redirect_type, "prefix") - self.assertEqual(redirect.from_url, "/redirect-this/") - self.assertEqual(redirect.to_url, "") + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + response = self.client.post( + reverse( + "projects-redirects-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + }, + ), + data, + ) + self.assertEqual(response.status_code, 400) + self.assertEqual(Redirect.objects.all().count(), 1) + json_response = response.json() + self.assertIn("type", json_response) - def test_projects_redirects_type_sphinx_html_list_post(self): + def test_projects_redirects_type_clean_url_to_html_list_post(self): self.assertEqual(Redirect.objects.count(), 1) data = { - "type": "sphinx_html", + "type": CLEAN_URL_TO_HTML_REDIRECT, } self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") @@ -163,7 +168,7 @@ def test_projects_redirects_type_sphinx_html_list_post(self): self.assertEqual(Redirect.objects.all().count(), 2) redirect = Redirect.objects.first() - self.assertEqual(redirect.redirect_type, "sphinx_html") + self.assertEqual(redirect.redirect_type, CLEAN_URL_TO_HTML_REDIRECT) self.assertEqual(redirect.from_url, "") self.assertEqual(redirect.to_url, "") @@ -196,6 +201,165 @@ def test_projects_redirects_detail_put(self): self._get_response_dict("projects-redirects-detail_PUT"), ) + def test_projects_redirects_position(self): + url = reverse( + "projects-redirects-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + }, + ) + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + + # A new redirect will be created at the start by default. + response = self.client.post( + url, + data={ + "from_url": "/one/", + "to_url": "/two/", + "type": EXACT_REDIRECT, + }, + ) + self.assertEqual(response.status_code, 201) + second_redirect = Redirect.objects.get(pk=response.json()["pk"]) + + self.redirect.refresh_from_db() + + self.assertEqual(second_redirect.position, 0) + self.assertEqual(self.redirect.position, 1) + + # Explicitly create a redirect at the end. + response = self.client.post( + url, + data={ + "from_url": "/two/", + "to_url": "/three/", + "type": EXACT_REDIRECT, + "position": 2, + }, + ) + self.assertEqual(response.status_code, 201) + third_redirect = Redirect.objects.get(pk=response.json()["pk"]) + + self.redirect.refresh_from_db() + second_redirect.refresh_from_db() + + self.assertEqual(second_redirect.position, 0) + self.assertEqual(self.redirect.position, 1) + self.assertEqual(third_redirect.position, 2) + + url = reverse( + "projects-redirects-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "redirect_pk": self.redirect.pk, + }, + ) + + # Move redirect to the end. + response = self.client.put(url, {"position": 2, "type": EXACT_REDIRECT}) + self.assertEqual(response.status_code, 200) + + self.redirect.refresh_from_db() + second_redirect.refresh_from_db() + third_redirect.refresh_from_db() + + self.assertEqual(second_redirect.position, 0) + self.assertEqual(third_redirect.position, 1) + self.assertEqual(self.redirect.position, 2) + + url = reverse( + "projects-redirects-detail", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + "redirect_pk": second_redirect.pk, + }, + ) + response = self.client.delete(url) + self.assertEqual(response.status_code, 204) + + self.redirect.refresh_from_db() + third_redirect.refresh_from_db() + + self.assertEqual(third_redirect.position, 0) + self.assertEqual(self.redirect.position, 1) + + def test_projects_redirects_validations(self): + url = reverse( + "projects-redirects-list", + kwargs={ + "parent_lookup_project__slug": self.project.slug, + }, + ) + self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}") + + response = self.client.post( + url, + data={ + "from_url": "/one/$rest", + "to_url": "/two/", + "type": EXACT_REDIRECT, + }, + ) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.json(), ["The $rest wildcard has been removed in favor of *."] + ) + + response = self.client.post( + url, + data={ + "from_url": "/one/*.html", + "to_url": "/two/", + "type": EXACT_REDIRECT, + }, + ) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.json(), ["The * wildcard must be at the end of the path."] + ) + + response = self.client.post( + url, + data={ + "from_url": "/one/", + "to_url": "/two/:splat", + "type": EXACT_REDIRECT, + }, + ) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.json(), + [ + "The * wildcard must be at the end of from_url to use the :splat placeholder in to_url." + ], + ) + + get(Redirect, redirect_type=CLEAN_URL_TO_HTML_REDIRECT, project=self.project) + response = self.client.post( + url, + data={ + "type": CLEAN_URL_TO_HTML_REDIRECT, + }, + ) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.json(), + ["Only one redirect of type `clean_url_to_html` is allowed per project."], + ) + + get(Redirect, redirect_type=HTML_TO_CLEAN_URL_REDIRECT, project=self.project) + response = self.client.post( + url, + data={ + "type": HTML_TO_CLEAN_URL_REDIRECT, + }, + ) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.json(), + ["Only one redirect of type `html_to_clean_url` is allowed per project."], + ) + def test_projects_redirects_detail_delete(self): url = reverse( "projects-redirects-detail", diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 1a236ee7710..82646cf5532 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -8,7 +8,6 @@ import structlog from django.conf import settings from django.db import models -from django.db.models import F from django.urls import reverse from django.utils import timezone from django.utils.translation import gettext @@ -81,6 +80,7 @@ SPHINX_SINGLEHTML, ) from readthedocs.projects.models import APIProject, Project +from readthedocs.projects.ordering import ProjectItemPositionManager from readthedocs.projects.validators import validate_build_config_file from readthedocs.projects.version_handling import determine_stable_version @@ -1283,6 +1283,8 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel): choices=VERSION_TYPES, ) + _position_manager = ProjectItemPositionManager(position_field_name="priority") + class Meta: unique_together = (('project', 'priority'),) ordering = ('priority', '-modified', '-created') @@ -1359,118 +1361,17 @@ def move(self, steps): self.priority = new_priority self.save() - def _change_priority(self): - """ - Re-order the priorities of the other rules when the priority of this rule changes. - - If the rule is new, we just need to move all other rules down, - so there is space for the new rule. - - If the rule already exists, we need to move the other rules up or down, - depending on the new priority, so we can insert the rule at the new priority. - - The save() method needs to be called after this method. - """ - total = self.project.automation_rules.count() - - # If the rule was just created, we just need to insert it at the given priority. - # We do this by moving the other rules down before saving. - if not self.pk: - # A new rule can be created at the end as max. - self.priority = min(self.priority, total) - - # A new rule can't be created with a negative priority. All rules start at 0. - self.priority = max(self.priority, 0) - - rules = ( - self.project.automation_rules.filter(priority__gte=self.priority) - # We sort the queryset in desc order - # to be updated in that order - # to avoid hitting the unique constraint (project, priority). - .order_by('-priority') - ) - expression = F('priority') + 1 - else: - current_priority = self.project.automation_rules.values_list( - "priority", - flat=True, - ).get(pk=self.pk) - - # An existing rule can't be moved past the end. - self.priority = min(self.priority, total - 1) - - # A new rule can't be created with a negative priority. all rules start at 0. - self.priority = max(self.priority, 0) - - # The rule wasn't moved, so we don't need to do anything. - if self.priority == current_priority: - return - - if self.priority > current_priority: - # It was moved down, so we need to move the other rules up. - rules = ( - self.project.automation_rules.filter( - priority__gt=current_priority, priority__lte=self.priority - ) - # We sort the queryset in asc order - # to be updated in that order - # to avoid hitting the unique constraint (project, priority). - .order_by("priority") - ) - expression = F("priority") - 1 - else: - # It was moved up, so we need to move the other rules down. - rules = ( - self.project.automation_rules.filter( - priority__lt=current_priority, priority__gte=self.priority - ) - # We sort the queryset in desc order - # to be updated in that order - # to avoid hitting the unique constraint (project, priority). - .order_by("-priority") - ) - expression = F("priority") + 1 - - # Put an impossible priority to avoid - # the unique constraint (project, priority) while updating. - # We use update() instead of save() to avoid calling the save() method again. - if self.pk: - self._meta.model.objects.filter(pk=self.pk).update(priority=total + 99) - - # NOTE: we can't use rules.update(priority=expression), because SQLite is used - # in tests and hits a UNIQUE constraint error. PostgreSQL doesn't have this issue. - # We use update() instead of save() to avoid calling the save() method. - for rule in rules: - self._meta.model.objects.filter(pk=rule.pk).update(priority=expression) - def save(self, *args, **kwargs): """Override method to update the other priorities before save.""" - self._change_priority() + self._position_manager.change_position_before_save(self) if not self.description: self.description = self.get_description() super().save(*args, **kwargs) def delete(self, *args, **kwargs): """Override method to update the other priorities after delete.""" - current_priority = self.priority - project = self.project super().delete(*args, **kwargs) - - rules = ( - project.automation_rules - .filter(priority__gte=current_priority) - # We sort the queryset in asc order - # to be updated in that order - # to avoid hitting the unique constraint (project, priority). - .order_by('priority') - ) - # We update each object one by one to - # avoid hitting the unique constraint (project, priority). - # We use update() instead of save() to avoid calling the save() method. - for rule in rules: - self._meta.model.objects.filter(pk=rule.pk).update( - priority=F("priority") - 1, - ) + self._position_manager.change_position_after_delete(self) def get_description(self): if self.description: diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 7369b0b51d7..e5051dc7a71 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -766,12 +766,25 @@ class RedirectForm(forms.ModelForm): class Meta: model = Redirect - fields = ["project", "redirect_type", "from_url", "to_url", "force"] + fields = [ + "project", + "redirect_type", + "from_url", + "to_url", + "http_status", + "force", + "enabled", + "description", + ] def __init__(self, *args, **kwargs): self.project = kwargs.pop('project', None) super().__init__(*args, **kwargs) + # Remove the nullable option from the form. + self.fields["enabled"].widget = forms.CheckboxInput() + self.fields["enabled"].empty_value = False + if self.project.has_feature(Feature.ALLOW_FORCED_REDIRECTS): # Remove the nullable option from the form. # TODO: remove after migration. diff --git a/readthedocs/projects/ordering.py b/readthedocs/projects/ordering.py new file mode 100644 index 00000000000..f8e7f37fa04 --- /dev/null +++ b/readthedocs/projects/ordering.py @@ -0,0 +1,136 @@ +"""Utilities to manage the position of items in a project.""" + +from django.db.models import F + + +class ProjectItemPositionManager: + def __init__(self, position_field_name): + self.position_field_name = position_field_name + + def change_position_before_save(self, item): + """ + Re-order the positions of the other items when the position of this item changes. + + If the item is new, we just need to move all other items down, + so there is space for the one. + + If the item already exists, we need to move the other items up or down, + depending on the new position, so we can insert the item at the new position. + + The save() method needs to be called after this. + """ + model = item._meta.model + total = model.objects.filter(project=item.project).count() + + # If the item was just created, we just need to insert it at the given position. + # We do this by moving the other items down before saving. + if not item.pk: + # A new item can be created at the end as max. + position = min(getattr(item, self.position_field_name), total) + setattr(item, self.position_field_name, position) + + # A new item can't be created with a negative position. All items start at 0. + position = max(getattr(item, self.position_field_name), 0) + setattr(item, self.position_field_name, position) + + items = ( + model.objects.filter(project=item.project).filter( + **{self.position_field_name + "__gte": position} + ) + # We sort the queryset in desc order + # to be updated in that order + # to avoid hitting the unique constraint (project, position). + .order_by(f"-{self.position_field_name}") + ) + expression = F(self.position_field_name) + 1 + else: + current_position = model.objects.values_list( + self.position_field_name, + flat=True, + ).get(pk=item.pk) + + # An existing item can't be moved past the end. + position = min(getattr(item, self.position_field_name), total - 1) + setattr(item, self.position_field_name, position) + + # A new item can't be created with a negative position, all items start at 0. + position = max(getattr(item, self.position_field_name), 0) + setattr(item, self.position_field_name, position) + + # The item wasn't moved, so we don't need to do anything. + if position == current_position: + return + + if position > current_position: + # It was moved down, so we need to move the other items up. + items = ( + model.objects.filter(project=item.project).filter( + **{ + self.position_field_name + "__gt": current_position, + self.position_field_name + "__lte": position, + } + ) + # We sort the queryset in asc order + # to be updated in that order + # to avoid hitting the unique constraint (project, position). + .order_by(self.position_field_name) + ) + expression = F(self.position_field_name) - 1 + else: + # It was moved up, so we need to move the other items down. + items = ( + model.objects.filter(project=item.project).filter( + **{ + self.position_field_name + "__lt": current_position, + self.position_field_name + "__gte": position, + } + ) + # We sort the queryset in desc order + # to be updated in that order + # to avoid hitting the unique constraint (project, position). + .order_by(f"-{self.position_field_name}") + ) + expression = F(self.position_field_name) + 1 + + # Put an impossible position to avoid + # the unique constraint (project, position) while updating. + # We use update() instead of save() to avoid calling the save() method again. + if item.pk: + model.objects.filter(pk=item.pk).update( + **{self.position_field_name: total + 99} + ) + + # NOTE: we can't use items.update(position=expression), because SQLite is used + # in tests and hits a UNIQUE constraint error. PostgreSQL doesn't have this issue. + # We use update() instead of save() to avoid calling the save() method. + for item_to_update in items: + model.objects.filter(pk=item_to_update.pk).update( + **{self.position_field_name: expression} + ) + + def change_position_after_delete(self, item): + """ + Update the order of the other items after deleting an item. + + After deleting an item, we move the items below it up by one position. + + The delete() method needs to be called before this. + """ + model = item._meta.model + previous_position = getattr(item, self.position_field_name) + items = ( + model.objects.filter(project=item.project).filter( + **{self.position_field_name + "__gte": previous_position} + ) + # We sort the queryset in asc order + # to be updated in that order + # to avoid hitting the unique constraint (project, position). + .order_by(self.position_field_name) + ) + # We update each object one by one to + # avoid hitting the unique constraint (project, position). + # We use update() instead of save() to avoid calling the save() method. + for item_to_update in items: + model.objects.filter(pk=item_to_update.pk).update( + **{self.position_field_name: F(self.position_field_name) - 1} + ) diff --git a/readthedocs/projects/urls/private.py b/readthedocs/projects/urls/private.py index 8a926f8c5d6..afa607ae510 100644 --- a/readthedocs/projects/urls/private.py +++ b/readthedocs/projects/urls/private.py @@ -35,6 +35,7 @@ ProjectNotificationsDelete, ProjectRedirectsCreate, ProjectRedirectsDelete, + ProjectRedirectsInsert, ProjectRedirectsList, ProjectRedirectsUpdate, ProjectTranslationsDelete, @@ -141,6 +142,11 @@ ProjectRedirectsCreate.as_view(), name="projects_redirects_create", ), + re_path( + r"^(?P[-\w]+)/redirects/(?P\d+)/insert/(?P\d+)/$", + ProjectRedirectsInsert.as_view(), + name="projects_redirects_insert", + ), re_path( r"^(?P[-\w]+)/redirects/(?P[-\w]+)/edit/$", ProjectRedirectsUpdate.as_view(), diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index e5e9e63acef..9c895d8f9e0 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -725,6 +725,31 @@ class ProjectRedirectsUpdate(ProjectRedirectsMixin, UpdateView): pass +class ProjectRedirectsInsert(ProjectRedirectsMixin, GenericModelView): + + """ + Insert a redirect in a specific position. + + This is done by changing the position of the redirect, + after saving the redirect, all other positions are updated + automatically. + """ + + http_method_names = ["post"] + + def post(self, request, *args, **kwargs): + redirect = self.get_object() + position = int(self.kwargs["position"]) + redirect.position = position + redirect.save() + return HttpResponseRedirect( + reverse( + "projects_redirects", + args=[self.get_project().slug], + ) + ) + + class ProjectRedirectsDelete(ProjectRedirectsMixin, DeleteView): http_method_names = ['post'] diff --git a/readthedocs/proxito/tests/test_old_redirects.py b/readthedocs/proxito/tests/test_old_redirects.py index 807c4b039a5..7bc744feaaf 100644 --- a/readthedocs/proxito/tests/test_old_redirects.py +++ b/readthedocs/proxito/tests/test_old_redirects.py @@ -13,8 +13,15 @@ from django.http import Http404 from django.test.utils import override_settings +from readthedocs.builds.constants import EXTERNAL from readthedocs.builds.models import Version from readthedocs.projects.models import Feature +from readthedocs.redirects.constants import ( + CLEAN_URL_TO_HTML_REDIRECT, + EXACT_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, + PAGE_REDIRECT, +) from readthedocs.redirects.models import Redirect from .base import BaseDocServing @@ -147,13 +154,14 @@ def test_root_redirect_with_query_params(self): PYTHON_MEDIA=True, PUBLIC_DOMAIN="dev.readthedocs.io", ROOT_URLCONF="readthedocs.proxito.tests.handler_404_urls", + RTD_EXTERNAL_VERSION_DOMAIN="readthedocs.build", ) class UserRedirectTests(MockStorageMixin, BaseDocServing): def test_forced_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", force=True, @@ -167,12 +175,106 @@ def test_forced_redirect(self): "http://project.dev.readthedocs.io/en/latest/tutorial/install.html", ) + def test_disabled_redirect(self): + redirect = fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/install.html", + to_url="/en/latest/tutorial/install.html", + enabled=True, + ) + url = "/en/latest/install.html" + r = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/tutorial/install.html", + ) + + redirect.enabled = False + redirect.save() + + with self.assertRaises(Http404): + self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + + def test_redirect_order(self): + redirect_a = fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="/en/latest/tutorial/:splat", + enabled=True, + ) + redirect_b = fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/install.html", + to_url="/en/latest/installation.html", + enabled=True, + ) + + redirect_a.refresh_from_db() + self.assertEqual(redirect_b.position, 0) + self.assertEqual(redirect_a.position, 1) + + url = "/en/latest/install.html" + r = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/installation.html", + ) + + redirect_a.position = 0 + redirect_a.save() + redirect_b.refresh_from_db() + + self.assertEqual(redirect_a.position, 0) + self.assertEqual(redirect_b.position, 1) + + url = "/en/latest/install.html" + r = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/tutorial/install.html", + ) + + def test_redirect_ignored_on_external_domain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", + ) + url = "/install.html" + r = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], "http://project.dev.readthedocs.io/en/latest/install.html" + ) + + fixture.get( + Version, + project=self.project, + active=True, + built=True, + slug="22", + type=EXTERNAL, + ) + with self.assertRaises(Http404): + self.client.get(url, headers={"host": "project--22.readthedocs.build"}) + def test_infinite_redirect(self): host = "project.dev.readthedocs.io" fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/install.html", ) @@ -187,7 +289,7 @@ def test_infinite_redirect_changing_protocol(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url=f"https://{host}/en/latest/install.html", ) @@ -197,7 +299,7 @@ def test_infinite_redirect_changing_protocol(self): with pytest.raises(Http404): self.client.get("/en/latest/install.html?foo=bar", headers={"host": host}) - def test_redirect_prefix_infinite(self): + def test_exact_redirect_avoid_infinite_redirect(self): """ Avoid infinite redirects. @@ -205,13 +307,14 @@ def test_redirect_prefix_infinite(self): return a 404. These examples comes from this issue: - * http://github.com/rtfd/readthedocs.org/issues/4673 + * http://github.com/readthedocs/readthedocs.org/issues/4673 """ fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", ) r = self.client.get( "/redirect.html", headers={"host": "project.dev.readthedocs.io"} @@ -232,15 +335,84 @@ def test_redirect_prefix_infinite(self): ) with self.assertRaises(Http404): - r = self.client.get( + self.client.get( "/en/latest/redirect/", headers={"host": "project.dev.readthedocs.io"} ) + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="/en/latest/subdir/:splat", + ) + r = self.client.get( + "/en/latest/redirect.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/subdir/redirect.html", + ) + + with self.assertRaises(Http404): + self.client.get( + "/en/latest/subdir/redirect.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + + def test_page_redirect_avoid_infinite_redirect(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/*", + to_url="/subdir/:splat", + ) + r = self.client.get( + "/en/latest/redirect.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/subdir/redirect.html", + ) + + with self.assertRaises(Http404): + self.client.get( + "/en/latest/subdir/redirect.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/dir/*", + to_url="/dir/subdir/:splat", + ) + r = self.client.get( + "/en/latest/dir/redirect.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/dir/subdir/redirect.html", + ) + + with self.assertRaises(Http404): + self.client.get( + "/en/latest/dir/subdir/redirect.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + def test_redirect_root(self): Redirect.objects.create( project=self.project, - redirect_type="prefix", - from_url="/woot/", + redirect_type=EXACT_REDIRECT, + from_url="/woot/*", + to_url="/en/latest/:splat", ) r = self.client.get( "/woot/faq.html", headers={"host": "project.dev.readthedocs.io"} @@ -261,7 +433,7 @@ def test_redirect_root(self): def test_redirect_page(self): Redirect.objects.create( project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", ) @@ -282,7 +454,7 @@ def test_redirect_with_query_params_from_url(self): ) Redirect.objects.create( project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", ) @@ -303,7 +475,7 @@ def test_redirect_with_query_params_to_url(self): ) Redirect.objects.create( project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html?query=one", ) @@ -335,7 +507,7 @@ def test_redirect_exact(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", ) @@ -352,7 +524,7 @@ def test_redirect_exact_looks_like_version(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/versions.json", to_url="/en/latest/versions.json", ) @@ -365,20 +537,20 @@ def test_redirect_exact_looks_like_version(self): "http://project.dev.readthedocs.io/en/latest/versions.json", ) - def test_redirect_exact_with_rest(self): + def test_redirect_exact_with_wildcard(self): """ - Exact redirects can have a ``$rest`` in the ``from_url``. + Exact redirects can have a ``*`` at the end of ``from_url``. Use case: we want to deprecate version ``2.0`` and replace it by - ``3.0``. We write an exact redirect from ``/en/2.0/$rest`` to - ``/en/3.0/``. + ``3.0``. We write an exact redirect from ``/en/2.0/*`` to + ``/en/3.0/:splat``. """ fixture.get( Redirect, project=self.project, - redirect_type="exact", - from_url="/en/latest/$rest", - to_url="/en/version/", # change version + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="/en/version/:splat", # change version ) self.assertEqual(self.project.redirects.count(), 1) r = self.client.get( @@ -398,9 +570,9 @@ def test_redirect_exact_with_rest(self): fixture.get( Redirect, project=self.translation, - redirect_type="exact", - from_url="/es/version/$rest", - to_url="/en/master/", # change language and version + redirect_type=EXACT_REDIRECT, + from_url="/es/version/*", + to_url="/en/master/:splat", # change language and version ) r = self.client.get( "/es/version/guides/install.html", @@ -412,6 +584,73 @@ def test_redirect_exact_with_rest(self): "http://project.dev.readthedocs.io/en/master/guides/install.html", ) + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/tutorials/*", + to_url="/en/latest/tutorial.html", + ) + r = self.client.get( + "/en/latest/tutorials/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], "http://project.dev.readthedocs.io/en/latest/tutorial.html" + ) + + def test_page_redirect_with_wildcard(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/*", + to_url="/guides/:splat", + ) + r = self.client.get( + "/en/latest/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/guides/install.html", + ) + + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/guides/*", + to_url="/guides/redirects/:splat", + ) + r = self.client.get( + "/en/latest/guides/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/en/latest/guides/redirects/install.html", + ) + + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/tutorials/*", + to_url="/tutorial.html", + ) + r = self.client.get( + "/en/latest/tutorials/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], "http://project.dev.readthedocs.io/en/latest/tutorial.html" + ) + def test_redirect_inactive_version(self): """ Inactive Version (``active=False``) should redirect properly. @@ -428,7 +667,7 @@ def test_redirect_inactive_version(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/oldversion/", to_url="/en/newversion/", ) @@ -445,7 +684,7 @@ def test_redirect_keeps_version_number(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/how_to_install.html", to_url="/install.html", ) @@ -465,7 +704,7 @@ def test_redirect_keeps_language(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/how_to_install.html", to_url="/install.html", ) @@ -483,7 +722,7 @@ def test_redirect_recognizes_custom_cname(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", ) @@ -511,7 +750,7 @@ def test_redirect_html(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) r = self.client.get( "/en/latest/faq/", headers={"host": "project.dev.readthedocs.io"} @@ -533,7 +772,7 @@ def test_redirect_html_root_index(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) with override_settings(PYTHON_MEDIA=False): @@ -558,7 +797,7 @@ def test_redirect_html_index(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) r = self.client.get( "/en/latest/faq/index.html", headers={"host": "project.dev.readthedocs.io"} @@ -573,7 +812,7 @@ def test_redirect_htmldir(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_htmldir", + redirect_type=HTML_TO_CLEAN_URL_REDIRECT, ) r = self.client.get( "/en/latest/faq.html", headers={"host": "project.dev.readthedocs.io"} @@ -588,8 +827,9 @@ def test_redirect_root_with_301_status(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/woot/", + redirect_type=EXACT_REDIRECT, + from_url="/woot/*", + to_url="/en/latest/:splat", http_status=301, ) r = self.client.get( @@ -606,8 +846,9 @@ def test_not_found_page_without_trailing_slash(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", ) with self.assertRaises(Http404): @@ -617,6 +858,40 @@ def test_not_found_page_without_trailing_slash(self): headers={"host": "project.dev.readthedocs.io"}, ) + def test_page_redirect_with_and_without_trailing_slash(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/install", + to_url="/tutorial/install/", + ) + + for url in ["/en/latest/install", "/en/latest/install/"]: + resp = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(resp.status_code, 302) + self.assertEqual( + resp["Location"], + "http://project.dev.readthedocs.io/en/latest/tutorial/install/", + ) + + def test_exact_redirect_with_and_without_trailing_slash(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/install", + to_url="/en/latest/tutorial/install/", + ) + + for url in ["/en/latest/install", "/en/latest/install/"]: + resp = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(resp.status_code, 302) + self.assertEqual( + resp["Location"], + "http://project.dev.readthedocs.io/en/latest/tutorial/install/", + ) + @override_settings(PUBLIC_DOMAIN="dev.readthedocs.io") class UserForcedRedirectTests(BaseDocServing): @@ -624,7 +899,7 @@ def test_no_forced_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", force=False, @@ -634,7 +909,7 @@ def test_no_forced_redirect(self): ) self.assertEqual(r.status_code, 200) - def test_prefix_redirect(self): + def test_exact_redirect_with_wildcard(self): """ Test prefix redirect. @@ -645,8 +920,9 @@ def test_prefix_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/woot/", + redirect_type=EXACT_REDIRECT, + from_url="/woot/*", + to_url="/en/latest/:splat", force=True, ) r = self.client.get( @@ -659,7 +935,7 @@ def test_infinite_redirect(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/install.html", force=True, @@ -675,7 +951,7 @@ def test_infinite_redirect_changing_protocol(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/install.html", to_url=f"https://{host}/install.html", force=True, @@ -693,7 +969,7 @@ def test_redirect_page(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", force=True, @@ -711,7 +987,7 @@ def test_redirect_with_query_params(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", force=True, @@ -730,7 +1006,7 @@ def test_redirect_exact(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", force=True, @@ -744,13 +1020,13 @@ def test_redirect_exact(self): "http://project.dev.readthedocs.io/en/latest/tutorial/install.html", ) - def test_redirect_exact_with_rest(self): + def test_redirect_exact_with_wildcard(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", - from_url="/en/latest/$rest", - to_url="/en/version/", + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="/en/version/:splat", force=True, ) self.assertEqual(self.project.redirects.count(), 1) @@ -767,9 +1043,9 @@ def test_redirect_exact_with_rest(self): fixture.get( Redirect, project=self.translation, - redirect_type="exact", - from_url="/es/latest/$rest", - to_url="/en/master/", + redirect_type=EXACT_REDIRECT, + from_url="/es/latest/*", + to_url="/en/master/:splat", force=True, ) r = self.client.get( @@ -788,7 +1064,7 @@ def test_redirect_keeps_language(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/how_to_install.html", to_url="/install.html", force=True, @@ -807,7 +1083,7 @@ def test_redirect_recognizes_custom_cname(self): fixture.get( Redirect, project=self.project, - redirect_type="page", + redirect_type=PAGE_REDIRECT, from_url="/install.html", to_url="/tutorial/install.html", force=True, @@ -831,7 +1107,7 @@ def test_redirect_html(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, force=True, ) r = self.client.get( @@ -847,7 +1123,7 @@ def test_redirect_html_index(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, force=True, ) r = self.client.get( @@ -863,7 +1139,7 @@ def test_redirect_htmldir(self): fixture.get( Redirect, project=self.project, - redirect_type="sphinx_htmldir", + redirect_type=HTML_TO_CLEAN_URL_REDIRECT, force=True, ) r = self.client.get( @@ -879,7 +1155,7 @@ def test_redirect_with_301_status(self): fixture.get( Redirect, project=self.project, - redirect_type="exact", + redirect_type=EXACT_REDIRECT, from_url="/en/latest/install.html", to_url="/en/latest/tutorial/install.html", http_status=301, @@ -901,7 +1177,7 @@ def test_redirect_with_301_status(self): ROOT_URLCONF="readthedocs.proxito.tests.handler_404_urls", ) class UserRedirectCrossdomainTest(BaseDocServing): - def test_redirect_prefix_crossdomain(self): + def test_redirect_exact_redirect_with_wildcard_crossdomain(self): """ Avoid redirecting to an external site unless the external site is in to_url. @@ -911,8 +1187,9 @@ def test_redirect_prefix_crossdomain(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", ) urls = [ @@ -949,12 +1226,13 @@ def test_redirect_prefix_crossdomain(self): self.assertEqual(r.status_code, 302, url) self.assertEqual(r["Location"], expected_location, url) - def test_redirect_prefix_crossdomain_with_newline_chars(self): + def test_redirect_exact_with_wildcard_crossdomain_with_newline_chars(self): fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type=EXACT_REDIRECT, + from_url="/*", + to_url="/en/latest/:splat", ) urls = [ ( @@ -971,14 +1249,14 @@ def test_redirect_prefix_crossdomain_with_newline_chars(self): self.assertEqual(r.status_code, 302, url) self.assertEqual(r["Location"], expected_location, url) - def test_redirect_sphinx_htmldir_crossdomain(self): + def test_redirect_html_to_clean_url_crossdomain(self): """ Avoid redirecting to an external site unless the external site is in to_url """ fixture.get( Redirect, project=self.project, - redirect_type="sphinx_htmldir", + redirect_type=HTML_TO_CLEAN_URL_REDIRECT, ) urls = [ @@ -1007,12 +1285,12 @@ def test_redirect_sphinx_htmldir_crossdomain(self): self.assertEqual(r.status_code, 302, url) self.assertEqual(r["Location"], expected_location, url) - def test_redirect_sphinx_html_crossdomain(self): + def test_redirect_clean_url_to_html_crossdomain(self): """Avoid redirecting to an external site unless the external site is in to_url.""" fixture.get( Redirect, project=self.project, - redirect_type="sphinx_html", + redirect_type=CLEAN_URL_TO_HTML_REDIRECT, ) urls = [ @@ -1047,9 +1325,9 @@ def test_redirect_using_projects_prefix(self): redirect = fixture.get( Redirect, project=self.project, - redirect_type="exact", - from_url="/projects/$rest", - to_url="https://example.com/projects/", + redirect_type=EXACT_REDIRECT, + from_url="/projects/*", + to_url="https://example.com/projects/:splat", ) self.assertEqual(self.project.redirects.count(), 1) r = self.client.get( @@ -1062,8 +1340,8 @@ def test_redirect_using_projects_prefix(self): "https://example.com/projects/deleted-subproject/en/latest/guides/install.html", ) - redirect.from_url = "/projects/not-found/$rest" - redirect.to_url = "/projects/subproject/" + redirect.from_url = "/projects/not-found/*" + redirect.to_url = "/projects/subproject/:splat" redirect.save() r = self.client.get( "/projects/not-found/en/latest/guides/install.html", @@ -1074,3 +1352,60 @@ def test_redirect_using_projects_prefix(self): r["Location"], "http://project.dev.readthedocs.io/projects/subproject/en/latest/guides/install.html", ) + + def test_page_redirect_crossdomain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/install.html", + to_url="https://example.com/", + ) + r = self.client.get( + "/en/latest/install.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual(r["Location"], "https://example.com/") + + def test_page_redirect_with_wildcard_crossdomain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=PAGE_REDIRECT, + from_url="/tutorial/*", + to_url="https://example.com/:splat", + ) + r = self.client.get( + "/en/latest/tutorial/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual(r["Location"], "https://example.com/install.html") + + def test_exact_redirect_crossdomain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/install.html", + to_url="https://example.com/", + ) + r = self.client.get( + "/en/latest/install.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual(r["Location"], "https://example.com/") + + def test_exact_redirect_with_wildcard_crossdomain(self): + fixture.get( + Redirect, + project=self.project, + redirect_type=EXACT_REDIRECT, + from_url="/en/latest/*", + to_url="https://example.com/:splat", + ) + r = self.client.get( + "/en/latest/install.html", headers={"host": "project.dev.readthedocs.io"} + ) + self.assertEqual(r.status_code, 302) + self.assertEqual(r["Location"], "https://example.com/install.html") diff --git a/readthedocs/proxito/views/mixins.py b/readthedocs/proxito/views/mixins.py index 98b1f4560f3..00ee6964c6c 100644 --- a/readthedocs/proxito/views/mixins.py +++ b/readthedocs/proxito/views/mixins.py @@ -340,7 +340,7 @@ def system_redirect( return resp def get_redirect( - self, project, lang_slug, version_slug, filename, full_path, forced_only=False + self, project, lang_slug, version_slug, filename, path, forced_only=False ): """ Check for a redirect for this project that matches ``full_path``. @@ -351,8 +351,8 @@ def get_redirect( redirect_path, http_status = project.redirects.get_redirect_path_with_status( language=lang_slug, version_slug=version_slug, - path=filename, - full_path=full_path, + filename=filename, + path=path, forced_only=forced_only, ) return redirect_path, http_status diff --git a/readthedocs/proxito/views/serve.py b/readthedocs/proxito/views/serve.py index 147871b1331..be0a60e40d7 100644 --- a/readthedocs/proxito/views/serve.py +++ b/readthedocs/proxito/views/serve.py @@ -331,27 +331,28 @@ def serve_path(self, request, path): is_external_version=unresolved_domain.is_from_external_domain, ) - # Check for forced redirects. - redirect_path, http_status = self.get_redirect( - project=project, - lang_slug=project.language, - version_slug=version.slug, - filename=filename, - full_path=request.path, - forced_only=True, - ) - if redirect_path and http_status: - log.bind(forced_redirect=True) - try: - return self.get_redirect_response( - request=request, - redirect_path=redirect_path, - proxito_path=request.path, - http_status=http_status, - ) - except InfiniteRedirectException: - # Continue with our normal serve. - pass + # Check for forced redirects on non-external domains only. + if not unresolved_domain.is_from_external_domain: + redirect_path, http_status = self.get_redirect( + project=project, + lang_slug=project.language, + version_slug=version.slug, + filename=filename, + path=request.path, + forced_only=True, + ) + if redirect_path and http_status: + log.bind(forced_redirect=True) + try: + return self.get_redirect_response( + request=request, + redirect_path=redirect_path, + proxito_path=request.path, + http_status=http_status, + ) + except InfiniteRedirectException: + # Continue with our normal serve. + pass # Check user permissions and return an unauthed response if needed. if not self.allowed_user(request, version): @@ -479,27 +480,28 @@ def get(self, request, proxito_path): if response: return response - # Check and perform redirects on 404 handler - # NOTE: this redirect check must be done after trying files like + # Check and perform redirects on 404 handler for non-external domains only. + # NOTE: This redirect check must be done after trying files like # ``index.html`` and ``README.html`` to emulate the behavior we had when # serving directly from NGINX without passing through Python. - redirect_path, http_status = self.get_redirect( - project=project, - lang_slug=lang_slug, - version_slug=version_slug, - filename=filename, - full_path=proxito_path, - ) - if redirect_path and http_status: - try: - return self.get_redirect_response( - request, redirect_path, proxito_path, http_status - ) - except InfiniteRedirectException: - # ``get_redirect_response`` raises this when it's redirecting back to itself. - # We can safely ignore it here because it's logged in ``canonical_redirect``, - # and we don't want to issue infinite redirects. - pass + if not unresolved_domain.is_from_external_domain: + redirect_path, http_status = self.get_redirect( + project=project, + lang_slug=lang_slug, + version_slug=version_slug, + filename=filename, + path=proxito_path, + ) + if redirect_path and http_status: + try: + return self.get_redirect_response( + request, redirect_path, proxito_path, http_status + ) + except InfiniteRedirectException: + # ``get_redirect_response`` raises this when it's redirecting back to itself. + # We can safely ignore it here because it's logged in ``canonical_redirect``, + # and we don't want to issue infinite redirects. + pass # Register 404 pages into our database for user's analytics self._register_broken_link( diff --git a/readthedocs/redirects/constants.py b/readthedocs/redirects/constants.py new file mode 100644 index 00000000000..f4594fd29a9 --- /dev/null +++ b/readthedocs/redirects/constants.py @@ -0,0 +1,18 @@ +from django.utils.translation import gettext_lazy as _ + +HTTP_STATUS_CHOICES = ( + (302, _("302 - Temporary Redirect")), + (301, _("301 - Permanent Redirect")), +) + +PAGE_REDIRECT = "page" +EXACT_REDIRECT = "exact" +CLEAN_URL_TO_HTML_REDIRECT = "clean_url_to_html" +HTML_TO_CLEAN_URL_REDIRECT = "html_to_clean_url" + +TYPE_CHOICES = ( + (PAGE_REDIRECT, _("Page Redirect")), + (EXACT_REDIRECT, _("Exact Redirect")), + (CLEAN_URL_TO_HTML_REDIRECT, _("Clean URL to HTML (file/ to file.html)")), + (HTML_TO_CLEAN_URL_REDIRECT, _("HTML to clean URL (file.html to file/)")), +) diff --git a/readthedocs/redirects/migrations/0006_add_new_fields.py b/readthedocs/redirects/migrations/0006_add_new_fields.py new file mode 100644 index 00000000000..96b424159da --- /dev/null +++ b/readthedocs/redirects/migrations/0006_add_new_fields.py @@ -0,0 +1,83 @@ +# Generated by Django 4.2.5 on 2023-10-31 17:57 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("redirects", "0005_allow_to_force_redirects"), + ] + + operations = [ + migrations.AddField( + model_name="redirect", + name="description", + field=models.CharField( + blank=True, + max_length=255, + verbose_name="Description", + null=True, + default="", + ), + ), + migrations.AddField( + model_name="redirect", + name="enabled", + field=models.BooleanField( + default=True, + help_text="Enable or disable the redirect.", + verbose_name="Enabled", + null=True, + ), + ), + migrations.AddField( + model_name="redirect", + name="position", + field=models.PositiveIntegerField( + default=0, + help_text="Order of execution of the redirect.", + null=True, + verbose_name="Position", + ), + ), + migrations.AlterField( + model_name="redirect", + name="http_status", + field=models.SmallIntegerField( + choices=[ + (302, "302 - Temporary Redirect"), + (301, "301 - Permanent Redirect"), + ], + default=302, + verbose_name="HTTP status code", + ), + ), + migrations.AlterField( + model_name="redirect", + name="status", + field=models.BooleanField(choices=[], default=True, null=True), + ), + migrations.AlterField( + model_name="redirect", + name="redirect_type", + field=models.CharField( + choices=[ + ("page", "Page Redirect"), + ("exact", "Exact Redirect"), + ("clean_url_to_html", "Clean URL to HTML (file/ to file.html)"), + ("html_to_clean_url", "HTML to clean URL (file.html to file/)"), + ], + help_text="The type of redirect you wish to use.", + max_length=255, + verbose_name="Redirect Type", + ), + ), + migrations.AlterModelOptions( + name="redirect", + options={ + "ordering": ("position", "-update_dt"), + "verbose_name": "redirect", + "verbose_name_plural": "redirects", + }, + ), + ] diff --git a/readthedocs/redirects/migrations/0007_migrate_to_new_syntax.py b/readthedocs/redirects/migrations/0007_migrate_to_new_syntax.py new file mode 100644 index 00000000000..9601e70d52d --- /dev/null +++ b/readthedocs/redirects/migrations/0007_migrate_to_new_syntax.py @@ -0,0 +1,43 @@ +# Generated by Django 4.2.5 on 2023-12-07 15:32 + +from django.db import migrations + + +def forwards_func(apps, schema_editor): + """ + Migrate redirects to the new modeling. + + Migrating the syntax of redirects is done outside the migration, + since models from migrations don't have access to some methods + required for the migration. + """ + Redirect = apps.get_model("redirects", "Redirect") + Project = apps.get_model("projects", "Project") + + # Enable all redirects. + Redirect.objects.filter(enabled=None).update(enabled=True) + + # Rename Sphinx redirects. + Redirect.objects.filter(redirect_type="sphinx_html").update( + redirect_type="clean_url_to_html" + ) + Redirect.objects.filter(redirect_type="sphinx_htmldir").update( + redirect_type="html_to_clean_url" + ) + + # Set positions with the same order as updated_dt. + for project in Project.objects.filter(redirects__isnull=False).distinct(): + for i, redirect_pk in enumerate( + project.redirects.order_by("-update_dt").values_list("pk", flat=True).all() + ): + Redirect.objects.filter(pk=redirect_pk).update(position=i) + + +class Migration(migrations.Migration): + dependencies = [ + ("redirects", "0006_add_new_fields"), + ] + + operations = [ + migrations.RunPython(forwards_func), + ] diff --git a/readthedocs/redirects/models.py b/readthedocs/redirects/models.py index b9d7ba14c92..2bd8d347781 100644 --- a/readthedocs/redirects/models.py +++ b/readthedocs/redirects/models.py @@ -9,34 +9,26 @@ from readthedocs.core.resolver import Resolver from readthedocs.projects.models import Project +from readthedocs.projects.ordering import ProjectItemPositionManager +from readthedocs.redirects.constants import ( + CLEAN_URL_TO_HTML_REDIRECT, + EXACT_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, + HTTP_STATUS_CHOICES, + PAGE_REDIRECT, + TYPE_CHOICES, +) +from readthedocs.redirects.validators import validate_redirect from .querysets import RedirectQuerySet log = structlog.get_logger(__name__) -HTTP_STATUS_CHOICES = ( - (301, _("301 - Permanent Redirect")), - (302, _("302 - Temporary Redirect")), -) - -STATUS_CHOICES = ( - (True, _("Active")), - (False, _("Inactive")), -) - -TYPE_CHOICES = ( - ("prefix", _("Prefix Redirect")), - ("page", _("Page Redirect")), - ("exact", _("Exact Redirect")), - ("sphinx_html", _("Sphinx HTMLDir -> HTML")), - ("sphinx_htmldir", _("Sphinx HTML -> HTMLDir")), - # ('advanced', _('Advanced')), -) # FIXME: this help_text message should be dynamic since "Absolute path" doesn't # make sense for "Prefix Redirects" since the from URL is considered after the -# ``/$lang/$version/`` part. Also, there is a feature for the "Exact -# Redirects" that should be mentioned here: the usage of ``$rest`` +# ``/$lang/$version/`` part. Also, there is a feature for the "Exact Redirects" +# that should be mentioned here: the usage of ``*``. from_url_helptext = _( "Absolute path, excluding the domain. " "Example: /docs/ or /install.html", @@ -73,8 +65,7 @@ class Redirect(models.Model): blank=True, ) - # We are denormalizing the database here to easily query for Exact Redirects - # with ``$rest`` on them from El Proxito + # Store the from_url without the ``*`` wildcard for easier and faster querying. from_url_without_rest = models.CharField( max_length=255, db_index=True, @@ -90,6 +81,7 @@ class Redirect(models.Model): help_text=to_url_helptext, blank=True, ) + force = models.BooleanField( _("Force redirect"), null=True, @@ -98,30 +90,112 @@ class Redirect(models.Model): ) http_status = models.SmallIntegerField( - _("HTTP Status"), + _("HTTP status code"), choices=HTTP_STATUS_CHOICES, default=302, ) - status = models.BooleanField(choices=STATUS_CHOICES, default=True) + + enabled = models.BooleanField( + _("Enabled"), + default=True, + null=True, + help_text=_("Enable or disable the redirect."), + ) + + description = models.CharField( + _("Description"), + blank=True, + null=True, + max_length=255, + default="", + ) + + position = models.PositiveIntegerField( + _("Position"), + default=0, + null=True, + help_text=_("Order of execution of the redirect."), + ) + + # TODO: remove this field and use `enabled` instead. + status = models.BooleanField(choices=[], default=True, null=True) create_dt = models.DateTimeField(auto_now_add=True) update_dt = models.DateTimeField(auto_now=True) + _position_manager = ProjectItemPositionManager(position_field_name="position") + objects = RedirectQuerySet.as_manager() class Meta: verbose_name = _("redirect") verbose_name_plural = _("redirects") - ordering = ("-update_dt",) + ordering = ( + "position", + "-update_dt", + ) + # TODO: add the project, position unique_together constraint once + # all redirects have a position set. def save(self, *args, **kwargs): - if self.redirect_type == "exact" and "$rest" in self.from_url: - self.from_url_without_rest = self.from_url.replace("$rest", "") + self.from_url_without_rest = None + if self.redirect_type in [ + CLEAN_URL_TO_HTML_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, + ]: + # These redirects don't make use of the ``from_url``/``to_url`` fields. + self.to_url = "" + self.from_url = "" + else: + self.to_url = self.normalize_to_url(self.to_url) + self.from_url = self.normalize_from_url(self.from_url) + if self.from_url.endswith("*"): + self.from_url_without_rest = self.from_url.removesuffix("*") + + self._position_manager.change_position_before_save(self) super().save(*args, **kwargs) + def delete(self, *args, **kwargs): + super().delete(*args, **kwargs) + self._position_manager.change_position_after_delete(self) + + def normalize_from_url(self, path): + """ + Normalize from_url to be used for matching. + + Normalize the path to always start with one slash, + and end without a slash, so we can match both, + with and without a trailing slash. + """ + path = path.rstrip("/") + path = "/" + path.lstrip("/") + return path + + def normalize_to_url(self, path): + """ + Normalize to_url to be used for redirecting. + + Normalize the path to always start with one slash, + if the path is not an absolute URL. + Otherwise, return the path as is. + """ + if re.match("^https?://", path): + return path + path = "/" + path.lstrip("/") + return path + + def clean(self): + validate_redirect( + project=self.project, + pk=self.pk, + redirect_type=self.redirect_type, + from_url=self.from_url, + to_url=self.to_url, + ) + def __str__(self): redirect_text = "{type}: {from_to_url}" - if self.redirect_type in ["prefix", "page", "exact"]: + if self.redirect_type in [PAGE_REDIRECT, EXACT_REDIRECT]: return redirect_text.format( type=self.get_redirect_type_display(), from_to_url=self.get_from_to_url_display(), @@ -133,17 +207,10 @@ def __str__(self): ) def get_from_to_url_display(self): - if self.redirect_type in ["prefix", "page", "exact"]: - from_url = self.from_url - to_url = self.to_url - if self.redirect_type == "prefix": - to_url = "/{lang}/{version}/".format( - lang=self.project.language, - version=self.project.default_version, - ) + if self.redirect_type in [PAGE_REDIRECT, EXACT_REDIRECT]: return "{from_url} -> {to_url}".format( - from_url=from_url, - to_url=to_url, + from_url=self.from_url, + to_url=self.to_url, ) return "" @@ -167,7 +234,21 @@ def get_full_path( filename=filename, ) - def get_redirect_path(self, path, full_path=None, language=None, version_slug=None): + def get_redirect_path(self, filename, path=None, language=None, version_slug=None): + """ + Resolve the redirect for the given filename. + + .. note:: + + This method doesn't check if the current path matches ``from_url``, + that should be done before calling this method + using ``Redirect.objects.get_redirect_path_with_status``. + + :param filename: The filename being served. + :param path: The whole path from the request. + :param language: The language of the project. + :param version_slug: The slug of the current version. + """ method = getattr( self, "redirect_{type}".format( @@ -175,51 +256,59 @@ def get_redirect_path(self, path, full_path=None, language=None, version_slug=No ), ) return method( - path, full_path=full_path, language=language, version_slug=version_slug + filename=filename, path=path, language=language, version_slug=version_slug ) - 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 :. - cut_path = path[len(self.from_url) :] # noqa - - to = self.get_full_path( - filename=cut_path, - language=language, - version_slug=version_slug, - allow_crossdomain=False, - ) - return to - - 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( - filename=self.to_url.lstrip("/"), + def _redirect_with_wildcard(self, current_path): + if self.from_url.endswith("*"): + # Detect infinite redirects of the form: + # /dir/* -> /dir/subdir/:splat + # For example: + # /dir/test.html will redirect to /dir/subdir/test.html, + # and if file doesn't exist, it will redirect to + # /dir/subdir/subdir/test.html and then to /dir/subdir/subdir/test.html and so on. + if ":splat" in self.to_url: + to_url_without_splat = self.to_url.split(":splat", maxsplit=1)[0] + if current_path.startswith(to_url_without_splat): + log.debug( + "Infinite redirect loop detected", + redirect=self, + ) + return None + + splat = current_path[len(self.from_url_without_rest) :] + to_url = self.to_url.replace(":splat", splat) + return to_url + return self.to_url + + def redirect_page(self, filename, path, language=None, version_slug=None): + log.debug("Redirecting...", redirect=self) + to_url = self._redirect_with_wildcard(current_path=filename) + if to_url: + return self.get_full_path( + filename=to_url, language=language, version_slug=version_slug, allow_crossdomain=True, ) - return to - - 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 - # Handle full sub-level redirects - if "$rest" in self.from_url: - match = self.from_url.split("$rest", maxsplit=1)[0] - if full_path.startswith(match): - cut_path = full_path.replace(match, self.to_url, 1) - return cut_path - - 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) - path = path[1:] # Strip leading slash. - to = re.sub(ending + "$", ".html", path) + return None + + def redirect_exact(self, filename, path, language=None, version_slug=None): + log.debug("Redirecting...", redirect=self) + return self._redirect_with_wildcard(current_path=path) + + def redirect_clean_url_to_html( + self, filename, path, language=None, version_slug=None + ): + log.debug("Redirecting...", redirect=self) + suffixes = ["/", "/index.html"] + for suffix in suffixes: + if filename.endswith(suffix): + to = filename[: -len(suffix)] + if not to: + to = "index.html" + else: + to += ".html" return self.get_full_path( filename=to, language=language, @@ -227,16 +316,14 @@ def redirect_sphinx_html(self, path, full_path, language=None, version_slug=None allow_crossdomain=False, ) - def redirect_sphinx_htmldir( - self, path, full_path, language=None, version_slug=None + def redirect_html_to_clean_url( + self, filename, 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( - filename=to, - language=language, - version_slug=version_slug, - allow_crossdomain=False, - ) + log.debug("Redirecting...", redirect=self) + to = filename.removesuffix(".html") + "/" + return self.get_full_path( + filename=to, + language=language, + version_slug=version_slug, + allow_crossdomain=False, + ) diff --git a/readthedocs/redirects/querysets.py b/readthedocs/redirects/querysets.py index 63b258214c7..29f430475fe 100644 --- a/readthedocs/redirects/querysets.py +++ b/readthedocs/redirects/querysets.py @@ -6,6 +6,12 @@ from django.db.models import CharField, F, Q, Value from readthedocs.core.permissions import AdminPermission +from readthedocs.redirects.constants import ( + CLEAN_URL_TO_HTML_REDIRECT, + EXACT_REDIRECT, + HTML_TO_CLEAN_URL_REDIRECT, + PAGE_REDIRECT, +) log = structlog.get_logger(__name__) @@ -34,79 +40,100 @@ def api(self, user=None): return queryset def get_redirect_path_with_status( - self, path, full_path=None, language=None, version_slug=None, forced_only=False + self, filename, path=None, language=None, version_slug=None, forced_only=False ): """ Get the final redirect with its status code. - :param path: Is the path without the language and version parts. - :param full_path: Is the full path including the language and version parts. + :param filename: The filename being served. + :param path: The whole path from the request. :param forced_only: Include only forced redirects in the results. """ # Small optimization to skip executing the big query below. - if forced_only and not self.filter(force=True).exists(): + # TODO: use filter(enabled=True) once we have removed the null option from the field. + if forced_only and not self.filter(force=True).exclude(enabled=False).exists(): return None, None + normalized_filename = self._normalize_path(filename) normalized_path = self._normalize_path(path) - normalized_full_path = self._normalize_path(full_path) - # add extra fields with the ``path`` and ``full_path`` to perform a + # Useful to allow redirects to match paths with or without trailling slash. + # For example, ``/docs`` will match ``/docs/`` and ``/docs``. + filename_without_trailling_slash = normalized_filename.rstrip("/") + path_without_trailling_slash = normalized_path.rstrip("/") + + # Add extra fields with the ``filename`` and ``path`` to perform a # filter at db level instead with Python. queryset = self.annotate( + filename=Value( + filename, + output_field=CharField(), + ), path=Value( normalized_path, output_field=CharField(), ), - full_path=Value( - normalized_full_path, + filename_without_trailling_slash=Value( + filename_without_trailling_slash, + output_field=CharField(), + ), + path_without_trailling_slash=Value( + path_without_trailling_slash, output_field=CharField(), ), - ) - prefix = Q( - redirect_type="prefix", - full_path__startswith=F("from_url"), ) page = Q( - redirect_type="page", - path__exact=F("from_url"), - ) - exact = Q( - redirect_type="exact", - from_url__endswith="$rest", - full_path__startswith=F("from_url_without_rest"), + redirect_type=PAGE_REDIRECT, + from_url_without_rest__isnull=True, + filename_without_trailling_slash__exact=F("from_url"), ) | Q( - redirect_type="exact", - full_path__exact=F("from_url"), + redirect_type=PAGE_REDIRECT, + from_url_without_rest__isnull=False, + filename__startswith=F("from_url_without_rest"), ) - sphinx_html = Q( - redirect_type="sphinx_html", - path__endswith="/", + exact = Q( + redirect_type=EXACT_REDIRECT, + from_url_without_rest__isnull=True, + path_without_trailling_slash__exact=F("from_url"), ) | Q( - redirect_type="sphinx_html", - path__endswith="/index.html", + redirect_type=EXACT_REDIRECT, + from_url_without_rest__isnull=False, + path__startswith=F("from_url_without_rest"), ) - sphinx_htmldir = Q( - redirect_type="sphinx_htmldir", - path__endswith=".html", - ) - - queryset = queryset.filter(prefix | page | exact | sphinx_html | sphinx_htmldir) + clean_url_to_html = Q(redirect_type=CLEAN_URL_TO_HTML_REDIRECT) + html_to_clean_url = Q(redirect_type=HTML_TO_CLEAN_URL_REDIRECT) + + if filename in ["/index.html", "/"]: + # If the filename is a root index file (``/index.html`` or ``/``), we only need to match page and exact redirects, + # since we don't have a filename to redirect to for clean_url_to_html and html_to_clean_url redirects. + queryset = queryset.filter(page | exact) + elif filename: + if filename.endswith(("/index.html", "/")): + queryset = queryset.filter(page | exact | clean_url_to_html) + elif filename.endswith(".html"): + queryset = queryset.filter(page | exact | html_to_clean_url) + else: + queryset = queryset.filter(page | exact) + else: + # If the filename is empty, we only need to match exact redirects. + # Since the other types of redirects are not valid without a filename. + queryset = queryset.filter(exact) + + # TODO: use filter(enabled=True) once we have removed the null option from the field. + queryset = queryset.exclude(enabled=False) if forced_only: queryset = queryset.filter(force=True) - # There should be one and only one redirect returned by this query. I - # can't think in a case where there can be more at this point. I'm - # leaving the loop just in case for now - for redirect in queryset.select_related("project"): + redirect = queryset.select_related("project").first() + if redirect: new_path = redirect.get_redirect_path( + filename=normalized_filename, path=normalized_path, - full_path=normalized_full_path, language=language, version_slug=version_slug, ) - if new_path: - return new_path, redirect.http_status - return (None, None) + return new_path, redirect.http_status + return None, None def _normalize_path(self, path): r""" diff --git a/readthedocs/redirects/templates/redirects/redirect_list.html b/readthedocs/redirects/templates/redirects/redirect_list.html index 998125f8931..8ca2df442f9 100644 --- a/readthedocs/redirects/templates/redirects/redirect_list.html +++ b/readthedocs/redirects/templates/redirects/redirect_list.html @@ -1,9 +1,14 @@ {% extends "projects/project_edit_base.html" %} {% load i18n %} +{% load static %} {% block title %}{% trans "Redirects" %}{% endblock %} +{% block extra_links %} + +{% endblock %} + {% block project-redirects-active %}active{% endblock %} {% block project_edit_content_header %}{% trans "Redirects" %}{% endblock %} @@ -26,7 +31,7 @@
-
+
    {% for redirect in redirects %} @@ -40,6 +45,28 @@
{% endif %}