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

Perform redirects at DB level #6398

Merged
merged 14 commits into from Nov 25, 2019
Merged

Perform redirects at DB level #6398

merged 14 commits into from Nov 25, 2019

Conversation

humitos
Copy link
Member

@humitos humitos commented Nov 21, 2019

These queries a little more complex but they allow us to rely on the database to perform the filter and check for all the redirect in a single query than fetching all the instances of the Redirect objects and iterating over them.

This will perform only one query to the DB and the result will be the redirect we need to apply or no redirects at all.

These queries a little more complex but they allow us to rely on the
database to perform the filter and check for all the redirect in a
single query than fetching all the instances of the Redirect objects
and iterating over them.

This will perform only one query to the DB and the result will be the
redirect we need to apply or no redirects at all.
@humitos humitos requested a review from ericholscher Nov 21, 2019
@@ -84,7 +84,7 @@ def get_redirect_response(request, full_path):
language, version_slug, path = language_and_version_from_path(path)

path, http_status = project.redirects.get_redirect_path_with_status(
path=path, language=language, version_slug=version_slug
path=path, full_path=full_path, language=language, version_slug=version_slug
Copy link
Member Author

@humitos humitos Nov 21, 2019

Choose a reason for hiding this comment

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

I don't know why we weren't passing this full_path already. We are re-calculating it for one of the cases at redirect_exact method.

Passing it at this point, allows us to use it as a value to compare at db level instead from Python itself.

Copy link
Member

@ericholscher ericholscher left a comment

This looks good, but I do wonder about the performance of the queries. It should almost certainly be faster and cleaner than the current implementation tho :)

readthedocs/redirects/querysets.py Outdated Show resolved Hide resolved
humitos added 2 commits Nov 21, 2019
Adapt El Proxito `serve_docs` to detect if there is any redirect
configured on the project that matches the URL and return the proper
redirect in that case.
`Func(function='replace')` allows us to modify a field in the row. We
use it to remove the `$rest` part of the URL and be able to compare
the beginning of the from URL (without the `$rest`) against the
`full_path`.
@humitos humitos force-pushed the humitos/redirects-at-db-level branch from 9de554b to 03ce54a Compare Nov 21, 2019
@humitos humitos requested a review from ericholscher Nov 24, 2019
@humitos
Copy link
Member Author

@humitos humitos commented Nov 24, 2019

@ericholscher I update the PR and all tests are passing now. The main change was output_field to be IntegerField for the length. Also, I bugfix the typo for htmldir query.

@humitos
Copy link
Member Author

@humitos humitos commented Nov 24, 2019

If we go in this direction, we will need to update the docs at https://docs.readthedocs.io/en/latest/user-defined-redirects.html where it mentions that redirects are only triggered on 404.

@humitos
Copy link
Member Author

@humitos humitos commented Nov 24, 2019

This looks good, but I do wonder about the performance of the queries. It should almost certainly be faster and cleaner than the current implementation tho :)

Even if they are faster and cleaner, one difference is that we are hitting this code for all the files (css, js, html, etc) which could decrease the performance.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 25, 2019

Even if they are faster and cleaner, one difference is that we are hitting this code for all the files (css, js, html, etc) which could decrease the performance.

Yea, that's a good point. We should probably filter those out and not do any of the redirect logic on them.

We are not prepared yet to check and perform the redirects on El
Proxito serve docs. So, we are moving it to the 404 handler for now.

Once we want to activate it, we can just uncomment the lines on serve docs.
"""
redirect_path, http_status = project.redirects.get_redirect_path_with_status(
language=lang_slug,
version_slug=version_slug,
Copy link
Member

@ericholscher ericholscher Nov 25, 2019

Choose a reason for hiding this comment

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

I'm not quite sure why we need the lang_slug and version_slug here. It feels a bit weird. I looked through the code and they're used a couple places, but they don't seem necessary. I wonder if we can just remove it and simplify the redirects code accordingly.

Copy link
Member Author

@humitos humitos Nov 25, 2019

Choose a reason for hiding this comment

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

I didn't take a deep look at this on purpose. I didn't want to refactor all this without being sure that this is the direction we want to move forward but besides, because we are changing the whole logic of redirects and I felt it was too much. I'm opening an issue to track this refactor.

Copy link
Member

@ericholscher ericholscher left a comment

This feels ready to ship to me 👍

Of note, the DB query changes will also effect normal 404 serving, since it's integrated into the existing logic, so hopefully it should speed up redirects with proxito and without.

@humitos humitos merged commit 81768fc into master Nov 25, 2019
3 checks passed
@humitos humitos deleted the humitos/redirects-at-db-level branch Nov 25, 2019
agjohnson added a commit that referenced this issue Dec 3, 2019
This queryset introduced a bug where a from_url of 5 characters would
produce and exception.
agjohnson added a commit that referenced this issue Dec 3, 2019
This queryset introduced a bug where a from_url of 5 characters would
produce and exception.
agjohnson added a commit that referenced this issue Dec 3, 2019
This queryset introduced a bug where a from_url of 5 characters would
produce and exception.
agjohnson added a commit that referenced this issue Dec 3, 2019
This queryset introduced a bug where a from_url of 5 characters would
produce and exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants