Skip to content
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

Fix proxito redirects breaking without a / #6558

Merged
merged 4 commits into from Jan 21, 2020
Merged

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 21, 2020

This is an artifact of our old redirects code:

(Pdb) l
 59         match = re.match(
 60             r'^/(?P<language>%s)/(?P<version_slug>[^/]+)(?P<path>/.*)$' % LANGUAGES_REGEX,
 61             path,
 62         )
 63         import pdb; pdb.set_trace()
 64  ->     if match:
 65             language = match.groupdict()['language']
 66             version_slug = match.groupdict()['version_slug']
 67             path = match.groupdict()['path']
 68             return language, version_slug, path
 69         return None, None, path
(Pdb) match.groups()
('en', 'latest', '/foo.html')
(Pdb)

In particular, the regex was capturing the language as everything up to /, which then left the / always in the filename.

This is an artifact of our old redirects code:

```
(Pdb) l
 59         match = re.match(
 60             r'^/(?P<language>%s)/(?P<version_slug>[^/]+)(?P<path>/.*)$' % LANGUAGES_REGEX,
 61             path,
 62         )
 63         import pdb; pdb.set_trace()
 64  ->     if match:
 65             language = match.groupdict()['language']
 66             version_slug = match.groupdict()['version_slug']
 67             path = match.groupdict()['path']
 68             return language, version_slug, path
 69         return None, None, path
(Pdb) match.groups()
('en', 'latest', '/foo.html')
(Pdb)
```
@ericholscher ericholscher requested a review from humitos Jan 21, 2020
Copy link
Member

@humitos humitos left a comment

This works properly in my local tests.

  • [OK] /deep/section/tests.html -> conf.html
  • [EXPECTED FAIL] noslash/section/tests.html -> /conf.html
  • [OK] /tests.html -> /conf.html

We will need to figure it out how to run the same redirects tests via El Proxito later. There are probably other edge cases that need to be fixed, but I think we are OK merging this PR for now.

@ericholscher ericholscher added the PR: hotfix label Jan 21, 2020
@ericholscher ericholscher merged commit a2475a7 into master Jan 21, 2020
3 checks passed
@ericholscher ericholscher deleted the fix-proxito-redirect branch Jan 21, 2020
# Always add a `/` to the filename to match our old logic:
# https://github.com/readthedocs/readthedocs.org/blob/4b09c7a0ab45cd894c3373f7f07bad7161e4b223/readthedocs/redirects/utils.py#L60
redirect_filename = filename
if lang_slug and version_slug:
redirect_filename = '/' + filename
Copy link
Member

@humitos humitos Jan 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericholscher If I understand correctly, our previous logic is wrong and our current one is correct. Although, since we have been supporting this, we want to keep supporting it --which means that we are making the "bug" to continue working.

If that's the case, wouldn't be better to update our data in the database to prepend a / to those "From URL" that are missing it and completely fix this issue instead of still supporting the bug?

Copy link
Member Author

@ericholscher ericholscher Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the redirects code works this way, so I think we need to be consistent in what we're passing in. Changing the DB feels really tricky and a larger refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants