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

Eliminate `//` in paths #1286

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@edk0
Copy link
Member

commented Apr 22, 2018

Incoming paths are redirected to remove repeated slashes upon matching. Rules containing repeated slashes collapse them to just one and send a warning.

I haven't included a way to turn this off; do people have reasons to?

closes #1132

edk0 added some commits Apr 22, 2018

@edk0 edk0 force-pushed the edk0:doubleslash branch from b738b1a to 94865ef Apr 23, 2018

@davidism

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

The only problematic but somewhat valid case I can think of is /scrape/https://example.com, so it might be good to be a little conservative and only merge slashes that aren't preceded by :.

There should be a merge_slashes=True attribute on Rule and Map, similar to strict_slashes, to control this behavior.

Like strict_slashes, Rule.match should raise an exception that MapAdapater handles.

I was considering only merging slashes in static parts, not variable parts, of matched URLs, but that would get really complicated. I think it's OK to just merge everything.

@davidism

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

Also remember to add a changelog entry and a versionchanged note to the docstring.

@edk0

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2018

Like strict_slashes, Rule.match should raise an exception that MapAdapater handles.

I was considering only merging slashes in static parts, not variable parts, of matched URLs, but that would get really complicated. I think it's OK to just merge everything.

Doing the work in Rule.match would mean each rule had to repeat the transformation, which I didn't like (I thought it would be slow, but since measured, and it isn't really), or altering the generated regexp, which might be a pain but would make it fairly easy to do the "only merge in static parts" bit.

Oh, and probably if we want to go /a//b -> /a/b/ it should take only one redirect. Hmm.

edk0 added some commits Apr 24, 2018

routing: add some tests
- test that merged slashes and strict slashes use only one redirect
  between them
- test that http:// in matched urls is unaffected

@edk0 edk0 force-pushed the edk0:doubleslash branch from 689766a to 820cc5c Apr 24, 2018

@edk0

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2018

So I've done all that (I think). The top commit makes only static parts affected—as you can see it's not a whole lot of extra work, but I'm happy to remove it if you prefer the other thing. (Personally, I like that it eliminates the special case for ://)

One thing testing this threw up is that a <path> can't start with /. With merge_slashes on, any inital slashes are eaten by the merging. Without it I think it will always 404. Anyway, it's trivial to let paths start with a slash, if you want.

routing: don't merge slashes on construction
We don't know whether the Map wants us to yet, so there's no ideal place
to do it at all. The associated warning and its corresponding test are
removed. merge_slashes is now enforced at `Rule.compile` time for
building as well as parsing.

@edk0 edk0 force-pushed the edk0:doubleslash branch from 820cc5c to 82447eb Apr 24, 2018

@edk0 edk0 force-pushed the edk0:doubleslash branch from 82447eb to 838ca91 Apr 24, 2018

@davidism davidism added this to the 1.0 milestone Feb 12, 2019

@davidism davidism added routing server and removed server labels Mar 21, 2019

@pallets pallets deleted a comment from lolobosse Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.