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

Similiar paths ending with a different static part are not handled properly #2

Closed
lexor90 opened this issue Sep 14, 2016 · 5 comments
Closed
Labels

Comments

@lexor90
Copy link

lexor90 commented Sep 14, 2016

Let's have

´/:user_id/:id/photosand/:user_id/:id/pagesboth responding to the same method, let's sayPOST`.

The router will follow the structure only of the first matching path, so when coming to the last staticPath it will find either photos or pages (according to what has been declared first) and return 404, making them not usable togheter.

Example:

app.post('/:user_id/:id/pages', function (req, res) {...});
app.post('/:user_id/:id/photos', function (req, res) { ... });

POST /abc/def/pages -> 200 OK
POST /abc/def/photos -> 404 Not Found

I tried to remake it by changing the initial structure to merge toghether static paths under the same dynamic route, but it would not work if :user_id becomes something else.
It should be implemented a runtime logic to try all matching paths, so if the first one fails, but the structure has some other dynamicPath with '3' parts it should also try them before to fail with a not found.

Any idea of how this could be implemented in your logic?

BTW, thanks for this useful library!

@lexor90
Copy link
Author

lexor90 commented Sep 14, 2016

As of my understanding it should be implemented inside router.js where

table = table.dynamicRoutes[size]

if size is greater than 1 instead of simply return notFound(req, res) we should be able to reduce the original table by the already parsed element (e.g. delete table[justLoopedItem]) and remake the logic with the new table that is reduced by one item (remake the whole continue_to_next_value loop).

This will introduce worse performances if multiple paths are using the same method/structure, but sometimes, especially for RESTful webservices, route design is not a matter of choice and the /:parent/:child URL structuring is just better than /somenamespacetomakeitwork/:parent/:child.

@herenow herenow added the bug label Sep 30, 2016
@lexor90
Copy link
Author

lexor90 commented Oct 7, 2016

Hello @herenow any news about this? I could work on a patch if you would give to me some feedback on what should be the best way to achieve this.

Thanks.

@herenow
Copy link
Owner

herenow commented Oct 7, 2016

I'm going to send in a patch today :)

herenow added a commit that referenced this issue Oct 9, 2016
Fixes issue #2

[Bug]

Whenever we had a value from the path that would match a parameter, we
would immediately recurse into the next routing table. This is a
problem, because a routing table may contain multiple params, and we
don't really know if the table we just recursed into, will continue to
match later on, it mail not match when it encounters a regexp or base
path.

For example:

If we have this two routes:

`/:id/:second_id/users`
`/:id/:second_id/places`

The second route would never match, on a path such as:

`/123/123/places`

Since we would always match on the first route, and fail on the last
part of the path.

[Solution]

Save the last real table match in a temporary variable.

A "real table match" is any match that was performed on a "base" path,
since params may match any path, but may will later fail if any base
path does not match, thus we need to go back in the routing tree and try
the next param on the last table we had a "real" match.

[Notes]

I'm not satisfied with our code, we are maintaining way too much state,
we should probably write a more "functional" code, and write a recursive
function instead of retaining state in variables, but this will probably
affect our performance promises (although I don't really care anymore)....
herenow added a commit that referenced this issue Oct 9, 2016
Fixes issue #2

[Bug]

Whenever we had a value from the path that would match a parameter, we
would immediately recurse into the next routing table. This is a
problem, because a routing table may contain multiple params, and we
don't really know if the table we just recursed into, will continue to
match later on, it mail not match when it encounters a regexp or base
path.

For example:

If we have this two routes:

`/:id/:second_id/users`
`/:id/:second_id/places`

The second route would never match, on a path such as:

`/123/123/places`

Since we would always match on the first route, and fail on the last
part of the path.

[Solution]

Save the last real table match in a temporary variable.

A "real table match" is any match that was performed on a "base" path,
since params may match any path, but may will later fail if any base
path does not match, thus we need to go back in the routing tree and try
the next param on the last table we had a "real" match.

[Notes]

I'm not satisfied with our code, we are maintaining way too much state,
we should probably write a more "functional" code, and write a recursive
function instead of retaining state in variables, but this will probably
affect our performance promises (although I don't really care anymore)....
@herenow
Copy link
Owner

herenow commented Oct 9, 2016

I tried to fix this on #3

But I can't seem to figure an easy way to solve it, without refactoring the whole thing. Unfortunately I won't have time for this.

I don't plan on maintaining this project any further, it was just an experiment at the time.

I urge you to use another router if you need this kind of routes.

Feel free to send PR's though, if you wan't to fix this issue, I'm not sure how to fix it though :)

herenow added a commit that referenced this issue Oct 9, 2016
Fixes issue #2

[Bug]

Whenever we had a value from the path that would match a parameter, we
would immediately recurse into the next routing table. This is a
problem, because a routing table may contain multiple params, and we
don't really know if the table we just recursed into, will continue to
match later on, it mail not match when it encounters a regexp or base
path.

For example:

If we have this two routes:

`/:id/:second_id/users`
`/:id/:second_id/places`

The second route would never match, on a path such as:

`/123/123/places`

Since we would always match on the first route, and fail on the last
part of the path.

[Solution]

Save the last real table match in a temporary variable.

A "real table match" is any match that was performed on a "base" path,
since params may match any path, but may will later fail if any base
path does not match, thus we need to go back in the routing tree and try
the next param on the last table we had a "real" match.

[Notes]

I'm not satisfied with our code, we are maintaining way too much state,
we should probably write a more "functional" code, and write a recursive
function instead of retaining state in variables, but this will probably
affect our performance promises (although I don't really care anymore)....
@herenow
Copy link
Owner

herenow commented Oct 9, 2016

@lexor90 Actually, I gave the problem a rest, and my fix actually works. Although it may not be elegant.

herenow added a commit that referenced this issue Oct 9, 2016
Fixes issue #2

[Bug]

Whenever we had a value from the path that would match a parameter, we
would immediately recurse into the next routing table. This is a
problem, because a routing table may contain multiple params, and we
don't really know if the table we just recursed into, will continue to
match later on, it mail not match when it encounters a regexp or base
path.

For example:

If we have this two routes:

`/:id/:second_id/users`
`/:id/:second_id/places`

The second route would never match, on a path such as:

`/123/123/places`

Since we would always match on the first route, and fail on the last
part of the path.

[Solution]

Save the last real table match in a temporary variable.

A "real table match" is any match that was performed on a "base" path,
since params may match any path, but may will later fail if any base
path does not match, thus we need to go back in the routing tree and try
the next param on the last table we had a "real" match.

[Notes]

I'm not satisfied with our code, we are maintaining way too much state,
we should probably write a more "functional" code, and write a recursive
function instead of retaining state in variables, but this will probably
affect our performance promises (although I don't really care anymore)....
@herenow herenow closed this as completed Oct 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants