Perform redirects at DB level #6398
Conversation
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.
| @@ -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 | |||
There was a problem hiding this comment.
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.
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
force-pushed
the
humitos/redirects-at-db-level
branch
from
9de554b to
03ce54a
Compare
|
@ericholscher I update the PR and all tests are passing now. The main change was |
…s/readthedocs.org into humitos/redirects-at-db-level
|
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. |
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.