Redirects: allow to redirect even if a page exists #9243

stsewd · 2022-05-19T01:54:45Z

Netify call this option "force", I think it is okay to call them that way in our platform.

This is currently under a feature flag (should I remove the docs till we make this feature available to everyone? or maybe just mention that's a beta feature)
Redirects are cached as normal as the other requests (there is another PR on rtd-ext to purge the cache)

stsewd · 2022-05-19T17:02:29Z

docs/user/user-defined-redirects.rst

+Features
+--------


Wasn't sure about where to describe this option,
also the mention of redirect to external sites doesn't look like a limitation :D

stsewd · 2022-05-19T17:03:44Z

readthedocs/projects/forms.py

+            # TODO: remove after migration.
+            self.fields["force"].widget = forms.CheckboxInput()
+            self.fields["force"].empty_value = False


We could also make the field non-nullable and just deal with a couple of minutes of users no being able to create redirects.

No strong preference here, but being safe seems fine.

stsewd · 2022-05-19T17:07:04Z

readthedocs/proxito/views/serve.py

-        # )
-        # if redirect_path and http_status:
-        #     return self.get_redirect_response(request, redirect_path, http_status)
+        redirect_path, http_status = self.get_redirect(


We could just call final_project.get_redirect_path_with_status() directly, we aren't adding anything extra on get_redirect.

stsewd · 2022-05-19T17:08:12Z

readthedocs/proxito/views/serve.py

-        # ``redirect_filename`` is the path without ``/<lang>/<version>`` and
-        # without query, starting with a ``/``. This matches our old logic:
-        # https://github.com/readthedocs/readthedocs.org/blob/4b09c7a0ab45cd894c3373f7f07bad7161e4b223/readthedocs/redirects/utils.py#L60   # noqa
-        #
-        # We parse ``filename`` to:
-        # - Remove the query params (probably it doesn't contain any query params at this point)
-        # - Remove any invalid URL chars (\r, \n, \t).
-        #
-        # We don't use ``.path`` to avoid parsing the filename as a full url.
-        # For example if the filename is ``http://example.com/my-path``,
-        # ``.path`` would return ``my-path``.
-        parsed = urlparse(filename)
-        redirect_filename = parsed._replace(query="").geturl()
-
-        # We can't check for lang and version here to decide if we need to add
-        # the ``/`` or not because ``/install.html`` is a valid path to use as
-        # redirect and does not include lang and version on it. It should be
-        # fine always adding the ``/`` to the beginning.
-        redirect_filename = '/' + redirect_filename.lstrip('/')


this was moved to the get_redirect_path_with_status method itself

stsewd · 2022-05-19T17:09:13Z

readthedocs/proxito/views/serve.py

                        full_path=proxito_path,
                    )
                    return resp

        self._register_broken_link(
            project=final_project,
            version=version,
-            path=redirect_filename,
+            path=filename,


We already normalize the filename before recording the broken link, so we are fine using the original filename

ericholscher

This looks straightforward. I'd like to get it deployed so we can test it out for performance. I think it's probably going to be painful, but it's useful to have if we need it for a user, for now.

ericholscher · 2022-05-26T21:18:29Z

docs/user/user-defined-redirects.rst

@@ -128,6 +130,17 @@ Similarly, if you maintain several branches of your documentation (e.g. ``3.0``
 ``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 to migrate your documentation to another domain,


This section needs some subsections. There's a lot of content here, I think a few subsections would make it easier to follow. Probably headers with what each example is doing?

I have put the examples in subsections.

There is a lot of nesting, maybe we could address a more structural change as part of #9258

ericholscher · 2022-05-26T21:19:39Z

readthedocs/projects/forms.py

+            # TODO: remove after migration.
+            self.fields["force"].widget = forms.CheckboxInput()
+            self.fields["force"].empty_value = False


No strong preference here, but being safe seems fine.

ericholscher · 2022-05-26T21:20:46Z

readthedocs/proxito/views/serve.py

+                    http_status=http_status,
+                )
+            except InfiniteRedirectException:
+                # Continue with our normal serve.


Should we log something here?

We already log that here

readthedocs.org/readthedocs/proxito/views/mixins.py

Lines 248 to 254 in 0acd5e4

if from_url == to:

# check that we do have a response and avoid infinite redirect

log.warning(

'Infinite Redirect: FROM URL is the same than TO URL.',

url=to,

)

raise InfiniteRedirectException()

but we can add another here to be more specific

We should add force= attribute to that log line (or to log.bind when it's called originally) to have more context when debugging.

ericholscher · 2022-05-26T21:22:16Z

readthedocs/redirects/querysets.py

+        if forced_only and not self.filter(force=True).exists():
+            return None, None
+
+        def normalize_path(path):


This inline function feels a bit weird.. Is there a reason it isn't in a utils file?

it was very specific to this method, I'll move it to be a private method

docs/user/user-defined-redirects.rst

Co-authored-by: Eric Holscher <[email protected]>

stsewd force-pushed the allow-to-force-redirects branch 2 times, most recently from a4ed43f to 266284c Compare May 19, 2022 16:53

stsewd marked this pull request as ready for review May 19, 2022 16:56

stsewd requested review from a team as code owners May 19, 2022 16:56

auto-assign bot assigned stsewd May 19, 2022

Redirects: allow to redirect even if a page exists

6b8f556

stsewd force-pushed the allow-to-force-redirects branch from 266284c to 6b8f556 Compare May 19, 2022 17:00

stsewd commented May 19, 2022

View reviewed changes

ericholscher mentioned this pull request May 25, 2022

Support 200 on Redirects, now that we're on a CDN. #9267

Closed

agjohnson linked an issue May 26, 2022 that may be closed by this pull request

Support 200 on Redirects, now that we're on a CDN. #9267

Closed

ericholscher approved these changes May 26, 2022

View reviewed changes

stsewd and others added 5 commits May 26, 2022 16:30

Update docs/user/user-defined-redirects.rst

91a58a7

Co-authored-by: Eric Holscher <[email protected]>

Updates from review

0ce44d3

format

0acd5e4

Merge branch 'main' into allow-to-force-redirects

d610181

Log forced_redirect

5fb46a0

stsewd enabled auto-merge (squash) May 31, 2022 20:10

stsewd merged commit 410baff into main May 31, 2022

stsewd deleted the allow-to-force-redirects branch May 31, 2022 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redirects: allow to redirect even if a page exists #9243

Redirects: allow to redirect even if a page exists #9243

stsewd commented May 19, 2022 •

edited

Loading

stsewd May 19, 2022

stsewd May 19, 2022

ericholscher May 26, 2022

stsewd May 19, 2022

stsewd May 19, 2022

stsewd May 19, 2022

ericholscher left a comment

ericholscher May 26, 2022

stsewd May 26, 2022

ericholscher May 26, 2022

ericholscher May 26, 2022

stsewd May 26, 2022

humitos May 30, 2022 •

edited

Loading

ericholscher May 26, 2022

stsewd May 26, 2022

	if from_url == to:
	# check that we do have a response and avoid infinite redirect
	log.warning(
	'Infinite Redirect: FROM URL is the same than TO URL.',
	url=to,
	)
	raise InfiniteRedirectException()

		Features
		--------

Redirects: allow to redirect even if a page exists #9243

Redirects: allow to redirect even if a page exists #9243

Conversation

stsewd commented May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented May 19, 2022 •

edited

Loading

humitos May 30, 2022 •

edited

Loading