Redirects: apply all matching redirects instead of stopping at the first invalid one (infinite redirect)

[jeanas]

Details

• Read the Docs project URL: https://readthedocs.org/projects/rtd-redirects-test/
• Read the Docs username (if applicable): https://readthedocs.org/profiles/jeanas/

Expected Result

When prefix redirects are in use, they do not prevent other redirects from working.

Actual Result

There are two redirects:

• Prefix redirect: [empty] → /en/latest/
• Page redirect: /specifications/foobar → /specifications/pyproject-toml

The URL https://rtd-redirects-test.readthedocs.io/en/latest/specifications/foobar does not redirect to https://rtd-redirects-test.readthedocs.io/en/latest/specifications/pyproject-toml, as it should.

This bug is sensitive to the order between redirects. It only shows up if the prefix redirect is before the page redirect. If I click "modify" and "save" with the page redirect in order to bring it to the top (I suppose that this deletes a DB entry and adds a fresh one), then the link starts working.

If I then do the same for the prefix redirect... it keeps working.

But if I change the foobar to foobaz, then again “refresh” the prefix redirect to bring it to the top, then https://rtd-redirects-test.readthedocs.io/en/latest/specifications/foobaz does not work. So, I suppose the case where the bug unexpectedly doesn't show up is some caching effect going on.

[stsewd]

Hi, with #10881, redirects will have an explicit ordering that you can define.

[jeanas]

That's great, but I would not have expected the ordering to matter in this case.

I don't know if RTD applies all redirects in order or applies only the first that matches, but either way, the prefix redirect doesn't match my URL so it shouldn't have any influence.

[stsewd]

I don't know if RTD applies all redirects in order or applies only the first that matches

Current implementation applies the first redirect that matches ordererd by modification date.

The prefix redirect doesn't match my URL so it shouldn't have any influence.

Your prefix redirect is from /, so it will match any path that 404. But since the original URL already has the /en/latest/ part, it will redirect to the same path, when we find an infinite redirect, we bail out of the redirect and return a 404.

[jeanas]

Ok, that makes sense, but this sounds like either /en/latest/ links should not be matched by prefix redirects, proceeding to the next redirect, or, better, it should first apply the first matching prefix redirect and then apply the first matching page redirect.

[stsewd]

We are removing prefix redirects in favor of a more explicit aproach with exact redirects (/* -> /en/latest/:splat). We still need to match page and exact redirects when we have a path that includes /en/latest/.

[jeanas]

In that case, how about applying all redirects in order, instead of the first that matches?

[stsewd]

@jeanas performance would be my main worry here, since evaluating redirects and detecting if they produce an infinite redirect requires us to evaluate the final URL (for page, HTML and clean URL redirects).

But the number of redirects matching the same URL will probably be low in most cases, so performance may not be a big problem.

[humitos]

I think this issue is solved by the new redirects refactor since they are ran in order now and can be re-ordered as wanted.

On the other hand, applying all matching redirects and making the application to automatically decide which one to follow would lead to more confusions, since the behavior would be implicit.

If there are 2 matching redirects and the first one (in order) performs an infinite redirect, the user should be responsible to move that redirect down in the list or to remove it.

I'd close this issue as "Not planned".

[jeanas]

Well, my use case is that of packaging.python.org, where we really want to have our redirects managed by a file in the repository (see pypa/packaging.python.org#1408). I was planning to use readthedocs-cli, but it sounds like it will need to be modified to preserve the ordering. Is defining the order supported via the API? (I couldn't test, as it looks like the change hasn't been deployed yet.)

[stsewd]

Specifying the order is supported via the API. That tool should probably use the order of the redirects in the file to set the order in the API.

[stsewd]

The new improvements will go out later today.