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

Support path normalization before matching routes #737

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support path normalization before matching routes #737

wants to merge 2 commits into from

Conversation

sobels
Copy link

@sobels sobels commented Jul 22, 2014

Adds support for path normalization (fixes #667) by adding a new class, NormalizedURLRouter. Rationale for using a new class:

  • Using a new class allows URLRouter to encapsulate only the logic for matching routes, and lets another class deal with what paths are considered canonical.
  • It seems to be more convenient than adding an intermediate request handler: it is more brief, and the odds are probably pretty low that someone would only want to normalize paths for some subset of routes.

Several sites claim that redirects are better for SEO, but several documents show that there are multiple solutions to this (e.g. rel="canonical" links, sitemaps). Perhaps redirects should be an optional mode for NormalizedURLRouter, but for now I think a simpler and slightly faster default would be to avoid the redirect.

For now, the only normalization performed is to resolve .., ., and /.

Steve Sobel added 2 commits July 21, 2014 10:36
s-ludwig added a commit that referenced this pull request Jul 22, 2014
@s-ludwig
Copy link
Member

Some thoughts:

  • Since URLRouter often is the glue used to combine multiple web components, that may be an argument against a separate router class. After all, a sub component may depend on a particular behavior for canonical matches.
  • Performance may be another argument for integrating it into URLRouter. This would make canonical matching possible without any memory allocations or increase in computational complexity.
  • I'm unsure yet if this is the right abstraction level for this kind of functionality. registerWebInterface and registerRestInterface are obvious candidates on a higher level (I've just committed simple support for the web interface generator). They have some limited semantic knowledge about all URLs and can in turn decide if a certain URL matches without the explicit normalization step.

@Geod24
Copy link
Contributor

Geod24 commented Sep 24, 2014

Regarding this situation, the RESTful moto is:

  • Trailing slash to refers to a collection of resources (e.g. example.com/users/)
  • No trailing slash to refers to a resource (e.g example.com/users/42)

However, for ease of use, some (most?) of the frameworks out there allow the lack of (or sometimes an optional) trailing slash.
I was also considering generating 301 response for request to example.com/users.

Considering vibe.d, IMHO, the only case where path shouldn't be normalized is when serving files. I can't see any healthy behavior where resource res should be treated differently than resource res/.

So I would be in favor of integrating this to URLRouter too.

@s-ludwig
Copy link
Member

s-ludwig commented Oct 7, 2014

Regarding the trailing slash, registerWebInterface now takes a WebInterfaceSettings with a ignoreTrailingSlash field (registerRestInterface should probably be extended to do this by default). When this is set to true, it will optionally accept a trailing slash for all routes and perform a redirect for GET requests, or directly serve the request for all other request methods.

But I'd keep this separate from the issue of resolving ., .. and multiple consecutive slashes. I'm currently leaning towards integrating this into URLRouter, so that it's possible to later implement a fast and allocation-less match algorithm that normalizes while matching.

Considering vibe.d, IMHO, the only case where path shouldn't be normalized is when serving files. I can't see any healthy behavior where resource res should be treated differently than resource res/.

Actually, paths are now always normalized in the file server, so that potentially malicious paths outside of the served directory can be recognized, so it would fit in well there.

@Geod24
Copy link
Contributor

Geod24 commented Jun 13, 2015

But I'd keep this separate from the issue of resolving ., .. and multiple consecutive slashes. I'm currently leaning towards integrating this into URLRouter, so that it's possible to later implement a fast and allocation-less match algorithm that normalizes while matching.

Is it still something you'd be keen to accept ? This path issue is becoming more and more present on my end and I'm also quite interested in #1062 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support path normalization before matching routes in URLRouter
3 participants