Denormalize from_url_without_rest onto the redirects model #6780
Conversation
ericholscher
force-pushed
the
redirects-denormalized
branch
from
2262a15 to
a87c537
Compare
We first perform a `.count()` query to check if the DB SQL query will fail. If it fails, we ignore the `exact` Q object and we continue with the redirects check ignoring all the Exact Redirects for this project. This query will *only fail* if there is at least one Exact Redirect with less than 5 chars long on this project. This is because we are substracting the `$rest` part from the `from_url` field and we can't end up with a negative number.
This should make querying faster and the logic to support it easier
ericholscher
force-pushed
the
redirects-denormalized
branch
from
a87c537 to
3e41eff
Compare
|
@humitos I think this is mostly ready, and we should be able to ship it if we want 🤞 |
|
Shouldn't this have a data migration for old redirects? |
It's better to keep `from_url_without_rest` as None instead of `''` for non-Exact Redirect type.
| exact = ( | ||
| Q( | ||
| redirect_type='exact', | ||
| from_url__endswith='$rest', |
There was a problem hiding this comment.
We can test using from_url_without_rest__isnull=False, which may be faster than this.
There was a problem hiding this comment.
The downside of this, is that it depends on how the from_url_without_rest ends up after the migration and the handling. We strictly depend on setting it correctly.
There was a problem hiding this comment.
I finished the data migration and change small things in the code (added a comment, tune a little the query and the save method). Please, @ericholscher take a look at them and feel free to merge when ready.
| @@ -0,0 +1,38 @@ | |||
| # -*- coding: utf-8 -*- | |||
| # Generated by Django 1.11.26 on 2020-03-14 14:15 | |||
There was a problem hiding this comment.
This file should be generated with out current version of django
| # There should be one and only one redirect returned by this query. I | ||
| # can't think in a case where there can be more at this point. I'm |
There was a problem hiding this comment.
A user could create several redirects with the same value, or with $rest in different positions, like /en/latest/foo/$rest and /en/latest/foo/bar/$rest
There was a problem hiding this comment.
Probably we should sort the result by creation date if we don't do that already
There was a problem hiding this comment.
like
/en/latest/foo/$restand/en/latest/foo/bar/$rest
Good point. Although, the first one contains the second one in this case, so it does not make too much sense at this point.
We could implement a new feature that the one that matches more gets priority, though. But that's a different story.
This should make querying faster and the logic to support it easier
Refs #6779