-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Redirects (design doc): improving existing functionality #10825
Conversation
Marked this PR as draft, but it's ready to have some feedback. Preview at https://dev--10825.org.readthedocs.build/en/10825/design/redirects.html |
@stsewd I think this is definitely more focused on user-defined redirects. I don't think we have too much to change about our automatic ones, except maybe some changes to domain canonicalization at some point, so we can ignore that for this PR 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great document, and I'm 💯 on moving forward with all the various proposed features. I do wonder if we can just remove the entire concept of redirect types, and use wildcards and presets to solve this problem? In particular, the HtmlDir redirects could just be a documented option for an Exact redirect that does *.html
-> /
. This is also a good use case for prefix wildcards 🤔
- Placeholders. | ||
I haven't seen users requesting this feature. | ||
We can consider adding it in the future. | ||
Maybe we can expose the current language and version as placeholders. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea, and perhaps removes the need for Page redirects, since it's the same as /$lang/$version/$path
as an exact redirect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be also useful for custom URLconf in the future when we support changing the position of them, like:
From: /:lang/:version/:path
To: /:lang/prefix/:version/:path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we will be allowing to change the positions of components, we already discussed why this introduces a lot of complexity and doesn't solve user requirements (they only need to add a prefix, which we already suport).
@ericholscher I thought about having just one type of redirect, but the only two types of redirects that we can easily replace are: prefix redirects and HTML URLs to clean URLs redirects. Why we can't replace page redirects?We could, but it complicates things, since we will be building on top of exact redirects, we need a way to specify in the from URL that we only want to match the filename.
Having a page redirect type is easier to explain to users, and they will work just like exact redirects, allowing prefix (if we still have the need for them) and suffix redirects. Why we can't replace clean URLs to .html URLs redirects?With the suggestion about normalizing paths that end with slash, it becomes impossible to capture paths that end with |
@stsewd Gotcha.. I do think Page redirects are a pretty useful feature of versioning, so it makes sense to have them. Perhaps we could call them Versioned redirects to make it a bit more explicit? The HTMLDir redirect with the normalization is a bit unfortunate. I could see users wanting to match URLs that end with |
There is only one relevant pending decision #10825 (comment) |
I'd love to remove the "types of redirects" if possible. Mainly because they are hard to explain to users and, once you understand how they work, they are hard to remember. The document starts "Our current implement has 5 types of redirects" which seems scary to me as a user. Why redirects should be so complex? 😅 If redirects scares myself, I'd assume that our users are being even more scared with them 🙃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I review this document and made some comments. I have no idea how complex the implementation of the things I've said could be but I didn't want to avoid mention them. I'm fine getting a reply like "this is good but it will be too complex to implement/maintain" 👍🏼
In general, apart from the limitations our redirects have, I think it's a feature that currently is hard to explain. I think that's because the "multiple types of redirects" and it seems we could potentially remove the types and only have one that depending on if it contains or not a *
/ $rest
behaves in one or another way.
I will try to take another look at this PR and think deeper on this features and also in the conversation that you are having with Eric here as well and try to provide more feedback.
- Placeholders. | ||
I haven't seen users requesting this feature. | ||
We can consider adding it in the future. | ||
Maybe we can expose the current language and version as placeholders. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be also useful for custom URLconf in the future when we support changing the position of them, like:
From: /:lang/:version/:path
To: /:lang/prefix/:version/:path
For example, if we have a redirect with the value ``/foo?blue=1&yellow=2&red=3``, | ||
if would be normalized in the DB as ``/foo`` and ``blue=1&red=3&yellow=2``. | ||
This implies that the URL to be matched must have the exact same query arguments, | ||
it can't have more or less. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be solved with a JSONField on the db to save the query arguments. In that case, the order and number of the arguments won't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But matching can't be done with one query even using a JSONField either, matching will need to happen on the Python side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand your reply.
I'm saying that with a JSONField that stores the query arguments you can perform the match of the query arguments at DB level. Once you get those, you can perform the path/URL matching with Python.
This will reduce the complexity of handling all the matching on Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the current URl is ?one&two&three
, we don't know which of those arguments we need to match in the rules. A rule can be one
, or be two
, or one&three
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is solvable by "matching the query arguments from the rule into the DB field" -- no the other way around as you are saying. Example:
.filter(queryargs__haskeys=rule.queryargs.keys())
https://docs.djangoproject.com/en/4.2/ref/contrib/postgres/fields/#has-keys
or we can use contained_by
if we want to match specific key, value
pairs:
.filter(queryargs__contained_by=rule.queryargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, didn't know about those queries, I have updated the doc to mention this approach.
We can reduce them to 3, prefix redirects and .html to clean URLs redirects can be removed (if we allow prefix wildcards). We can't replace page redirects and clean URLs to .html as described in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for answering all my questions. I think we are ready to move forward with the implementation since the big picture is already discussed. We can define more specific things during the implementation, probably.
Is there anything that's not defined yet that I'm missing?
Maybe if we should merge prefix redirects with exact redirects readthedocs.org/docs/dev/design/redirects.rst Lines 297 to 298 in 9ebd216
|
I'm 👍 on merging them. The fewer types we have the better. I think the HTMLDir-style redirects are not super common, so I'm fine making that functionality a bit less obvious in the UI if it simplifies how we explain it to users. |
👍🏼 on merging them. |
Didn't include anything about built-in redirects, should I touch that too in this document? Maybe in another one?
Ref #4221
📚 Documentation previews 📚
docs
): https://docs--10825.org.readthedocs.build/en/10825/dev
): https://dev--10825.org.readthedocs.build/en/10825/