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

Denormalize from_url_without_rest onto the redirects model #6780

Merged
merged 7 commits into from Apr 6, 2020

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 14, 2020

This should make querying faster and the logic to support it easier

Refs #6779

humitos and others added 3 commits Mar 14, 2020
This commit reverts 88c7640 from PR #6430

We found a problem with `from_url` that are smaller than 5 char
length. I'm reverting this here and adding a new commit that will
workaround this problem.
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 ericholscher requested a review from humitos Mar 17, 2020
@ericholscher
Copy link
Member Author

@ericholscher ericholscher commented Mar 17, 2020

@humitos I think this is mostly ready, and we should be able to ship it if we want 🤞

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 17, 2020

Shouldn't this have a data migration for old redirects?

@humitos humitos requested a review from stsewd Mar 19, 2020
exact = (
Q(
redirect_type='exact',
from_url__endswith='$rest',
Copy link
Member

@humitos humitos Mar 19, 2020

Choose a reason for hiding this comment

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

We can test using from_url_without_rest__isnull=False, which may be faster than this.

Copy link
Member

@humitos humitos Mar 19, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

@humitos humitos left a 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.

stsewd
stsewd approved these changes Mar 19, 2020
@@ -0,0 +1,38 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.26 on 2020-03-14 14:15
Copy link
Member

@stsewd stsewd Mar 19, 2020

Choose a reason for hiding this comment

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

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
Copy link
Member

@stsewd stsewd Mar 19, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@stsewd stsewd Mar 19, 2020

Choose a reason for hiding this comment

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

Probably we should sort the result by creation date if we don't do that already

Copy link
Member

@humitos humitos Mar 20, 2020

Choose a reason for hiding this comment

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

like /en/latest/foo/$rest and /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.

Copy link
Member Author

@ericholscher ericholscher left a comment

I'm happy to merge and ship this 👍

@ericholscher ericholscher merged commit 9a28437 into master Apr 6, 2020
2 of 3 checks passed
@ericholscher ericholscher deleted the redirects-denormalized branch Apr 6, 2020
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

3 participants