Fix non-exact match of pattern with trailing slash. #3923

aaugustin · 2016-09-20T19:27:10Z

Currently, in the v4 branch, <Match pattern="/foo/" component={Foo} /> fails to match at /foo/bar/, because the URL is truncated to /foo/bar instead of /foo/ prior to being matched.

aaugustin · 2016-09-20T19:27:59Z

For better or worse, some communities (notably Django) have chosen to include trailing slashes in URLs.

aaugustin · 2016-09-20T19:29:26Z

I didn't include any documentation changes because the v4 branch doesn't appear to contain a changelog yet (until it's deemed stable, I guess).

timdorr

LGTM.

ryanflorence · 2016-09-21T14:59:59Z

in a browser foo and foo/ are different, I want to be like the browser, not django, I need to look deeper at this commit, on a phone right now.

ryanflorence · 2016-09-21T15:01:53Z

With this code will a location of "foo" match pattern of "foo/"?

timdorr · 2016-09-21T15:26:41Z

In a browser foo and foo/ are different.

Just ran into this yesterday. More specifically, in any RFC 3986 compliant URL parser will distinguish with the trailing slash.

But in terms of what this API is attempting to do, which is match a URL pattern, I can confirm if you go into something like the Basic example and change the /about Match pattern to /about/ and the Link to /about/foo/ (or /about/foo) , it no longer matches for either URL. Here's something more complete:

<Match pattern="/about/" component={About} />

// These match
<Link to="/about">About</Link> // Wrong!
<Link to="/about/">About</Link>
// These don't match :(
<Link to="/about/foo">About</Link>
<Link to="/about/foo/">About</Link>

This looks to be fixing a specific pattern condition.

The spec violation is another issue entirely, but should be fixed. Could that be coming from path-to-regexp?

ryanflorence · 2016-09-21T15:34:15Z

<Match pattern="/foo/bar/"/>
// SHOULD match
"/foo/bar/"
// SHOULD NOT match
"/foo/bar"

<Match pattern="/foo/bar"/>
// SHOULD match
"/foo/bar/"
"/foo/bar"

ryanflorence · 2016-09-21T15:56:27Z

@timdorr I fixed my message and deleted our other ones to keep this thread less crazy than it was heading :)

timdorr · 2016-09-21T16:00:38Z

No worries. Again, that's sort of a separate issue from this PR. This is fixing matching for any non-exact pattern that has a trailing slash (E.g., /foo/ pattern doesn't match /foo/bar URL).

I say we merge this in as a start and then look at the issue of exact-ish non-trailing URLs matching a trailing slash pattern. (E.g., /foo/ pattern matches /foo URL)

aaugustin · 2016-09-21T16:02:10Z

Indeed browsers don' t give slashes any special treatment. Web servers serving static files off a filesystem might have filesystem-like semantics (e.g. collapse duplicate slashes, handle ../ for navigating to a parent directory, etc.) but that's unrelated to HTTP.

path-to-regex has an option related to trailing slashes:

strict When false the trailing slash is optional. (default: false)

I'm not sure if or how it comes into play here, but it's something to keep in mind.

I agree with the behavior @ryanflorence specified two comments above. Basically react-router should do a prefix match when exactly isn't specified.

I'm happy to investigate further and add more tests, either in this PR or in a follow-up PR. Whatever works best for you :-)

aaugustin · 2016-09-21T16:06:19Z

@timdorr There's a good chance that setting the strict option to true in path-to-regex will prevent /foo/ pattern from matching /foo URL. Probably best to do that in another PR since it looks related to a different piece of code.

timdorr · 2016-09-21T16:15:19Z

@aaugustin Yep, that's definitely what we need. Good find!

aaugustin · 2016-09-21T16:26:22Z

Should I add this fix to this PR or start a new PR?

timdorr · 2016-09-21T16:31:28Z

If it's that simple, I say add it here. Might as well tackle all the trailing slash stuff in one go.

timdorr · 2016-09-21T21:11:02Z

modules/matchPattern.js

@@ -1,21 +1,30 @@
 import pathToRegexp from 'path-to-regexp'

 const cache = {}
+const pathToRegexpOptions = {strict: true}


I would just inline this. No need to extract it out. We're not using it anywhere else and there are only a few potential options anyways.

Also, a small nitpick: Can you put spaces inside of the brackets for an object? It's not officially enforced, but it's kind of the standard around the code and looks good inline with everything else.

Will do. Pulling the constant out was a nano-optimization. Sorry about not checking the coding style.

timdorr

Changes noted

aaugustin · 2016-09-21T21:20:28Z

I just added another commit to prevent URL /foo from incorrectly matching pattern /foo/.

FYI while we're looking at path-to-regex options:

If you set {end: false}, it makes a prefix match instead of an exact match. That would allow react-router to get rid of the whole truncatePathnameToPattern method. Now that we have a good test coverage, I'd like to try using that option. That should be slightly more efficient as it's implemented purely as a regexp.

> pathToRegexp('/foo', keys, {strict: true})
{ /^\/foo$/i keys: [] }
> pathToRegexp('/foo', keys, {end: false, strict: true})
{ /^\/foo(?=\/|$)/i keys: [] }

Unless you set {sensitive: true}, it creates case-insensitive regexps. I don't have a strong opinion about that. Most projects use lower-case URLs. Then case sensitivity doesn't matter. I suppose case-insensitive matching could prevent some projects from implementing fully backwards compatible routing with pre-existing URL schemes that require telling apart upper case and lower case paths, but this sounds rather esoteric.

aaugustin · 2016-09-21T21:23:10Z

Let's keep this PR on hold for today, I'll try {end: false} tomorrow and see if that allows simplifying the code.

aaugustin · 2016-09-25T12:59:31Z

I just added a commit that removes the truncatePathnameToPattern function entirely and relies on features of pathToRegexp instead. I think that's much better than complicating truncatePathnameToPattern like my previous commit did. In fact, the PR simplifies matchPattern.js (-15, +11).

As far as I can tell, I have addressed all comments in the thread. This should be ready to merge. Thanks!

aaugustin · 2016-09-27T20:59:44Z

While this PR still carries @timdorr's "Changes requested" mark, I made the requested changes, and then some.

It's ready for a new round of review and hopefully a merge. Please let me know if there's any way I can help.

timdorr

It reads a little oddly, but I'm fine with it as-is.

timdorr · 2016-10-03T18:18:52Z

modules/matchPattern.js

@@ -1,22 +1,20 @@
 import pathToRegexp from 'path-to-regexp'

-const cache = {}
+// cache[exactly][pattern] contains getMatcher(pattern, exactly)
+const cache = {true: {}, false: {}}


So, this is the only weird part that bothers me. But implicit type conversion always does that for me...

Good catch! As a Pythonista, I failed to notice that these keys are strings / symbols, not booleans. I agree that it feels sloppy...

That said, adding explicit type conversions doesn't make the code more readable. Perhaps it's best to live with the implicit conversion.

You can add a small ternary to use proper string keys. To help with that, you can store that as a separate variable, cache_type for instance.

OK. I added a commit with that change.

Currently the test for a non-exact, partial match fails.

timdorr approved these changes Sep 20, 2016

View reviewed changes

timdorr reviewed Sep 21, 2016

View reviewed changes

timdorr requested changes Sep 21, 2016

View reviewed changes

timdorr added this to the v4.0.0 milestone Sep 28, 2016

timdorr approved these changes Oct 3, 2016

View reviewed changes

aaugustin added 5 commits October 4, 2016 09:22

Add tests for matching pattern with trailing slash.

fa29777

Currently the test for a non-exact, partial match fails.

Fix non-exact match of pattern with trailing slash.

78116f7

Fix incorrect match of pattern with trailing slash.

7029943

Simplify matchPattern with pathToRegexp 'end' option.

3b905f3

Avoid implicit typecasts.

fad9c49

timdorr mentioned this pull request Oct 6, 2016

pathIsActive to support trailing slash #4003

Closed

ryanflorence merged commit e5be03a into remix-run:v4 Oct 6, 2016

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Uh oh!

Fix non-exact match of pattern with trailing slash. #3923

Fix non-exact match of pattern with trailing slash. #3923

Uh oh!

Conversation

aaugustin commented Sep 20, 2016

Uh oh!

aaugustin commented Sep 20, 2016

Uh oh!

aaugustin commented Sep 20, 2016

Uh oh!

timdorr left a comment

Choose a reason for hiding this comment

Uh oh!

ryanflorence commented Sep 21, 2016

Uh oh!

ryanflorence commented Sep 21, 2016

Uh oh!

timdorr commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanflorence commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanflorence commented Sep 21, 2016

Uh oh!

timdorr commented Sep 21, 2016

Uh oh!

aaugustin commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaugustin commented Sep 21, 2016

Uh oh!

timdorr commented Sep 21, 2016

Uh oh!

aaugustin commented Sep 21, 2016

Uh oh!

timdorr commented Sep 21, 2016

Uh oh!

timdorr Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

timdorr Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

aaugustin Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

timdorr left a comment

Choose a reason for hiding this comment

Uh oh!

aaugustin commented Sep 21, 2016

Uh oh!

aaugustin commented Sep 21, 2016

Uh oh!

aaugustin commented Sep 25, 2016

Uh oh!

aaugustin commented Sep 27, 2016

Uh oh!

timdorr left a comment

Choose a reason for hiding this comment

Uh oh!

timdorr Oct 3, 2016

Choose a reason for hiding this comment

Uh oh!

aaugustin Oct 3, 2016

Choose a reason for hiding this comment

Uh oh!

timdorr Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

aaugustin Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timdorr commented Sep 21, 2016 •

edited

Loading

ryanflorence commented Sep 21, 2016 •

edited

Loading

aaugustin commented Sep 21, 2016 •

edited

Loading