-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Close #6525: linkcheck: Add linkcheck_ignore_redirects and linkcheck_warn_redirects #9234
Close #6525: linkcheck: Add linkcheck_ignore_redirects and linkcheck_warn_redirects #9234
Conversation
Add a new confval; `linkcheck_warn_redirects` to emit a warning when the hyperlink is redirected. It's useful to detect unexpected redirects under the warn-is-error mode.
a975934
to
0515e8d
Compare
Add a new confval; linkcheck_ignore_redirects to ignore hyperlinks that are redirected as expected.
0515e8d
to
707319a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the change looks good to me and is definitely an improvement. 👍
Perhaps add two other tests, with a non-matching key and a non-matching value. In both tests, assert the redirection triggers a warning?
code = response.history[-1].status_code | ||
return 'redirected', new_url, code | ||
else: | ||
return 'redirected', new_url, 0 | ||
|
||
def ignored_redirect(url: str, new_url: str) -> bool: | ||
for from_url, to_url in self.config.linkcheck_ignore_redirects.items(): | ||
if from_url.match(url) and to_url.match(new_url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification from the issue also asks for a mean to reuse match groups from the from_url
in the to_url
.
IIUC, the issue asks something similar to:
from_match = from_url.match(url)
if from_match:
to_url = from_match.expand(to_url)
if to_url.match(new_url):
return True
I am unsure if that is feasible at all.
From the sub()
documentation (used by expand()
):
Unknown escapes of ASCII letters are reserved for future use and treated as errors.
For example:
>>> import re
>>> to_url = r"^https://example.org/\w{2}/\1"
>>> match = re.match(r'https://example\.org/(.*)', 'https://example.org/the-real-page/')
>>> match.expand(to_url)
Traceback (most recent call last):
File "/usr/lib/python3.9/sre_parse.py", line 1039, in parse_template
this = chr(ESCAPES[this][1])
KeyError: '\\w'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.9/re.py", line 322, in _expand
template = sre_parse.parse_template(template, pattern)
File "/usr/lib/python3.9/sre_parse.py", line 1042, in parse_template
raise s.error('bad escape %s' % this, len(this))
re.error: bad escape \w at position 21
There’s always the option of documenting that the canonical URI cannot contain special sequences (e.g. \d
, \w
, etc.), but that’s sure to bite most people.
Another idea to tighten the validation would be to use named groups in both the source URI and canonical URI, and compare the named groups. Something like:
# conf.py
linkcheck_ignore_redirects = {
'https://sphinx-doc.org/en/(?P<page>.*)': 'https://sphinx-doc.org/en/master/(?P<page>.*)',
}
# linkcheck
from_match = from_url.match(url)
to_match = to_url.match(to)
if from_match.groupdict() == to_match.group_dict():
return True
That may not be as robust as one might expect. For example, I can imagine somebody documenting their regexp with capturing groups. IDK if the extra validation is worth the effort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If https://www.sphinx-doc.org/en/stable/usage/quickstart.html
redirects to something other than https://www.sphinx-doc.org/en/master/usage/quickstart.html
then it implies that the page no longer exists in the stable version and it should be possible to consider that an error. The validation should be worth the effort if someone is taking the effort to link to /stable/
versions (although I'm not linking to readthedocs websites like this myself).
I would be inclined to use re.sub()
and document it. Alternatively, do the group dict checking but only if the group name exists in both matches?
There may also need to be a regex and non-regex version because the need to escape every .
for an accurate match may confuse people. Any examples should definitely use r'...'
strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If https://www.sphinx-doc.org/en/stable/usage/quickstart.html redirects to something other than https://www.sphinx-doc.org/en/master/usage/quickstart.html then it implies that the page no longer exists in the stable version and it should be possible to consider that an error.
For a page that does not exist, the server should return a 404 in the first place.
I understand not all web servers behave that way, but let’s keep in mind we’re dealing with poorly configured web servers here. I wouldn’t advise implementing a brittle solution to work around bad behaving servers. I consider the re.sub()
solution to be brittle:
- disallowing
\
in theto_url
is sure to trip a good portion of users, re.sub()
was design to perform replacements in a template string, not in a regular expression. There’s no guarantee the regular expression will be valid after the templating. The\
is a symptom of it, but there may be others.
The comparison of groups seems more robust to me. One can adjust the groups to capture the same portion on both sides. It needs some work to experiment (e.g. try to implement the validation in the #6525 with that system). I’ll try it in the days to come.
There may also need to be a regex and non-regex version because the need to escape every
.
for an accurate match may confuse people.
I think it would mostly be confusing. There would be two ways of doing the same thing. Besides, there’s a strong probability the unexpected redirect URL will be different enough that the regexp won’t match, even if r"."
is used instead r"\."
. Not discussing that r"\."
is tighter and correct, simply trying to explain why a second setting without regexp seems overkill to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @nomis says, the idea here is that it is possible to accept redirect targets matching a certain pattern to be considered "working" and, if they don't match, then they should still show up as "redirected".
I've had a play with the example that @francoisfreitag posted and think that we can make this work by escaping non-backreference escapes:
>>> import re
>>> to_url = r"^https://example.org/\w{2}/\1"
>>> match = re.match(r'https://example\.org/(.*)', 'https://example.org/the-real-page/')
>>> match.expand(to_url)
Traceback (most recent call last):
File "/usr/lib/python3.9/sre_parse.py", line 1039, in parse_template
this = chr(ESCAPES[this][1])
KeyError: '\\w'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.9/re.py", line 322, in _expand
template = sre_parse.parse_template(template, pattern)
File "/usr/lib/python3.9/sre_parse.py", line 1042, in parse_template
raise s.error('bad escape %s' % this, len(this))
re.error: bad escape \w at position 21
>>> to_url2 = re.sub(r'(\\[^0-9g])', r'\\\1', to_url)
>>> to_url
'^https://example.org/\\w{2}/\\1'
>>> to_url2
'^https://example.org/\\\\w{2}/\\1'
>>> match.expand(to_url2)
'^https://example.org/\\w{2}/the-real-page/'
So re.sub(r'(\\[^0-9g])', r'\\\1', to_url)
seems to do the trick, escaping non-backreference escapes. It seems that .expand()
also reverses this extra escaping for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this idea forward.
code = response.history[-1].status_code | ||
return 'redirected', new_url, code | ||
else: | ||
return 'redirected', new_url, 0 | ||
|
||
def ignored_redirect(url: str, new_url: str) -> bool: | ||
for from_url, to_url in self.config.linkcheck_ignore_redirects.items(): | ||
if from_url.match(url) and to_url.match(new_url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @nomis says, the idea here is that it is possible to accept redirect targets matching a certain pattern to be considered "working" and, if they don't match, then they should still show up as "redirected".
I've had a play with the example that @francoisfreitag posted and think that we can make this work by escaping non-backreference escapes:
>>> import re
>>> to_url = r"^https://example.org/\w{2}/\1"
>>> match = re.match(r'https://example\.org/(.*)', 'https://example.org/the-real-page/')
>>> match.expand(to_url)
Traceback (most recent call last):
File "/usr/lib/python3.9/sre_parse.py", line 1039, in parse_template
this = chr(ESCAPES[this][1])
KeyError: '\\w'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.9/re.py", line 322, in _expand
template = sre_parse.parse_template(template, pattern)
File "/usr/lib/python3.9/sre_parse.py", line 1042, in parse_template
raise s.error('bad escape %s' % this, len(this))
re.error: bad escape \w at position 21
>>> to_url2 = re.sub(r'(\\[^0-9g])', r'\\\1', to_url)
>>> to_url
'^https://example.org/\\w{2}/\\1'
>>> to_url2
'^https://example.org/\\\\w{2}/\\1'
>>> match.expand(to_url2)
'^https://example.org/\\w{2}/the-real-page/'
So re.sub(r'(\\[^0-9g])', r'\\\1', to_url)
seems to do the trick, escaping non-backreference escapes. It seems that .expand()
also reverses this extra escaping for us.
That seems to work in my testing 👍, I had not thought of that option. It’s quite clean, with an identified set of patterns to look for. I’m not yet convinced the approach of mangling the user regexp is valid and worth the complexity. Unknown parts come from the URL, the content would also need escaping. Consider the following example: >>> match = re.match(r'https://example\.org/(.*)', 'https://example.org/how-to-make-real-$$$/')
>>> match.expand(r"^https://example.org/\1")
'^https://example.org/how-to-make-real-$$$/'
>>> to_url = match.expand(r"^https://example.org/\1")
>>> re.match(to_url, 'https://example.org/how-to-make-real-$$$/') Doesn’t match because It looks like applying A minor note, if we keep going down the route of crafting regular expressions, I think we should remove the regexp compilation step for the Edit: Accidental send caused an intermediary version of the answer to be sent. |
The escaping approach prevents placing backrefs in the Example regexp: >>> import re
>>> re.match(r'https://example.org/([\w-]+)/\1/', 'https://example.org/foo/foo/')
<re.Match object; span=(0, 28), match='https://example.org/foo/foo/'>
>>> re.match(r'https://example.org/([\w-]+)/\1/', 'https://example.org/foo/bar/')
>>> |
I updated this PR except for the capturing feature. I think it's another topic. So I'd like to separate it to another PR (and it's helpful if somebody will post it :-) |
|
||
linkcheck_working_redirects = { | ||
# All HTTP redirections from the source URI to the canonical URI will be treated as "working". | ||
r'https://sphinx-doc\.org/.*': r'https://sphinx-doc\.org/en/master/.*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no regex capture then this isn't checking what it implies it's checking. To allow for a more complete example without comparing the source and destination paths, I've added an allowance for redirecting to www.
but this could be removed too.
r'https://sphinx-doc\.org/.*': r'https://sphinx-doc\.org/en/master/.*' | |
r'https://sphinx-doc\.org/': r'https://(www\.)?sphinx-doc\.org/.*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the example should use patterns for both source and canonical URIs. So I think the current one is enough to describe the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is not just allowing redirects to the canonical version of a URL, it's allowing any redirects.
You may as well use r'https://sphinx-doc\.org/.*': r'https://sphinx-doc\.org/.*'
because there isn't really any validation of the destination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? The current rule verifies the pages under https://sphinx-doc.org/
to the URL under https://sphinx-doc.org/en/master/
. It does not mean "any redirects".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's allowing practically any redirect, so not what I would consider a good example.
It's not far off allowing https://www\.example\.com/.*
to https://www\.example\.com/.*
which then causes linkcheck to ignore removed pages that just redirect to the home page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If https://sphinx-doc.org/foo/bar
redirects to https://sphinx-doc.org/other/path
, it's not "allowed". Is this allowing any redirect? I'm confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this patch is an improvement to the linkcheck builder.
Before, it was not possible to treat redirects as errors. This patch offers the tools to do it. It also offers basic tools to verify the redirection occurs within the expected domain and path.
The redirection validation tools can be improved (e.g. by comparing match groups, or injecting backreferences captured from the source URI to the canonical URI). IMO, that should not prevent this first step from landing.
Thanks @tk0miya!
Co-authored-by: François Freitag <mail@franek.fr>
Now I'm debating to withdraw I believe it's reasonable that the linkcheck builder emits warnings if "disallowed" redirection detected. In other words, it emits warnings if 1) |
Now linkcheck builder integrates `linkcheck_warn_redirects` into `linkcheck_allowed_redirects`. As a result, linkcheck builder will emit a warning when "disallowed" redirection detected via `linkcheck_allowed_redirects`.
It's time to release. I'm merging this. Please try the new option and let me know if something not working well. Hopefully, it would be nice if somebody will send us a pull request that uses regexp as discussed above. |
Feature or Bugfix
Purpose
linkcheck_warn_redirects
to emit a warning whenthe hyperlink is redirected. It's useful to detect unexpected redirects
under the warn-is-error mode.
that are redirected as expected.