Query parameters cause invalid matches

[fidian]

I'm aware that the ordering of query parameters is outside the scope of this project, as it clearly states in the README. However, this isn't about ordering. If the route ends with a parameter and it is followed by a query string, the parameter gets the ?. Using the Node.js REPL, I can replicate this problem.

> p2r = require('path-to-regexp');
> re = p2r('/whatever/:moo?query=str')
/^\/whatever(?:\/([^\/]+?))?query\=str(?:\/)?$/i
> '/whatever/moo?query=str'.match(re)
[
  '/whatever/moo?query=str',
  'moo?',
  index: 0,
  input: '/whatever/moo?query=str',
  groups: undefined
]

The result of capturing moo? is broken. It appears that the regular expression constructed should look like the following.

/^\/whatever(?:\/([^\/?]+?))?\?query\=str(?:\/)?$/i

I would suggest updating the regular expression so the question mark can not be included in the path portion, which changes (?:\/([^\/]+?))? into (?:\/([^\/?]+?))?. This should be done for safety, since question marks are technically not part of the path as per the RFC, section 3.

Also, it would be nice to clearly update the documentation to indicate one of two positions:

1. If the path to parse contains query parameters, one must escape the question mark, changing "/path?query=str" into "/path\\?query=str" (two backslashes = send a literal backslash). This isn't really explained in the optional section.

2. Query strings are not supported at all and they must be removed before trying to parse the path. This appears to be the opinion stated in issues URL parameters behind a variable #47 and query parameters and RegExp's involving path parameter #91, but the documentation currently indicates that the library doesn't manage the ordering of query strings as opposed to query strings being forbidden.

[blakeembrey]

1. /whatever/:moo? is a valid optional path, it's impossible to tell that you actually meant /whatever/:moo\\? without you writing it. Feel free to submit a PR documenting how to escape, this doesn't seem like an issue with the path since you wrote optional :moo.
2. Ordered data is not supported, that includes query strings. This is documented in the README, but feel free to submit a PR clarify it further if the wording is confusing. Query strings are not forbidden though, you could use it if you escaped the ?.

There's also an option that lets you change the delimiter to /? if you're not matching pathnames.

[fidian]

I guess the point of the discussion I had was not explained correctly. Let's assume the regexp is created from this:

re = p2r('/:arg?test');

I get that :arg is optional because of the question mark. I'll submit a PR so others can know in advance they can parse URLs with query strings.

Next, I want to test if a URL matches.

matches = '/word?test'

At this point, matches[1] is word? with a question mark. I'm sure you know URIs use a question mark to delimit between the path and the query string. The argument :arg is a path argument, right? If so, it should not match the question mark.

[blakeembrey]

It makes sense, but you can adjust that using delimiters: '/?#' today. Maybe it's unclear, but it sounded like you either wanted /:arg\\?test or /:arg?\\?test instead? Is the issue the inverse, that you expected /:arg?test to not match query parameters at all?

[blakeembrey]

Taking a quick look now into some projects to see if changing the default would be possible or whether it would break existing applications.

[fidian]

I didn't expect /:arg?test to match the URL /word?test.

I also don't expect /:arg to match the URL /thing?query=string because :arg is a path parameter and should not continue beyond the question mark.

[blakeembrey]

@fidian Did this come up in an application you're working on? The reason I'm clarifying is that, if you're using it for query params, it's possible others do rely on the matching behavior today. Can you share more about how you're using this package so I can understand how to better support it?

[blakeembrey]

Closing with 2ea948c.

[fidian]

Thanks for the update! I'll still answer your question because it could be helpful to know how my team uses your library.

I have a mock server built into a UI for testing purposes. It effectively has a list of patterns and the route handler.

routes = [
    { method: 'GET', url: pathToRegexp('/person/:id'), handler: getPerson },
    { method: 'POST', url: pathToRegexp('/person/:id'), handler: updatePerson },
    { method: 'POST', url: pathToRegexp('/person/:id\\?force=true'), handler: forceUpdatePerson }
];

When a mock request comes in, the routes are checked in order and the first one to handle the request will be used.

for (const route of routes) {
    if (route.method === request.method) {
        matches = route.uri.exec(request.uri);
        if (matches) {
            route.handler(matches, request, response);
        }
    }
}

The breakdown is that the updatePerson route will match before forceUpdatePerson does. With a URL of /person/fidian?force=true, it will try to load matches[1] and that will be fidian?force=true. In this scenario, my problem would be solved by switching the order of the lines. However, I believe that having a path positional argument capturing the ? is incorrect. Even with the order switched, this would still break with a URL of /person/fidian? (or any other query string).

[blakeembrey]

Do you find that it's good enough for query parameters that this package works for you? How are you handling (or not handling) out of order and extra parameters?

[fidian]

The app I am using typically does not use query parameters and relies primarily on the body of the request. When it does use query parameters, they are in a predictable order. Due to the limited number and the predictable order, it is fairly easy to make rules for each route that would be used, so the library is working great.