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

Add HTTPServerRequest.routerRootDir property #1301

Merged
merged 3 commits into from Nov 9, 2015

Conversation

Projects
None yet
2 participants
@Yoplitein
Contributor

Yoplitein commented Oct 21, 2015

This allows for emitting the correct path when using a URLRouter with a prefix.

Consider the index of a router with prefix "/a/deep/path" which renders:

a(href="#{req.rootDir}") rootDir
a(href="#{req.routerRootDir}") routerRootDir

The following is output:

<a href="../../../">rootDir</a>
<a href="../../../a/deep/path/">routerRootDir</a>
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 22, 2015

Member

This idea sounds useful, I worked around this by hand for some projects already. For new projects , something similar for registerWebInterface would be similarly useful.

I see a two issues with the concrete approach:

  • This obviously only works for a single router, but routers could be nested. Probably not the common case, but it exists. A solution would be to update the request when an actual route match was found instead of once at the beginning.
  • The router is a higher level concept that is layered on top. The low-level HTTP API shouldn't be affected by it. What I'd propose is to simply put the relative path into HTTPServerRequest.params["routerRootDir"] instead. For computing the relative path, it's probably a good idea to use Path.relativeTo, to get a shorter path.
Member

s-ludwig commented Oct 22, 2015

This idea sounds useful, I worked around this by hand for some projects already. For new projects , something similar for registerWebInterface would be similarly useful.

I see a two issues with the concrete approach:

  • This obviously only works for a single router, but routers could be nested. Probably not the common case, but it exists. A solution would be to update the request when an actual route match was found instead of once at the beginning.
  • The router is a higher level concept that is layered on top. The low-level HTTP API shouldn't be affected by it. What I'd propose is to simply put the relative path into HTTPServerRequest.params["routerRootDir"] instead. For computing the relative path, it's probably a good idea to use Path.relativeTo, to get a shorter path.
@Yoplitein

This comment has been minimized.

Show comment
Hide comment
@Yoplitein

Yoplitein Oct 23, 2015

Contributor

I'm trying to use Path, but I can't figure out how to wrangle it into submission.
The following was the only way I could get it to actually do anything:

Path path = Path([PathEntry("a"), PathEntry("deep"), PathEntry("path")], true);
Path root = Path([PathEntry(".."), PathEntry(".."), PathEntry("..")], true);
string routerRootDir = path.relativeTo(root).toString;

Everything else causes the assert at path.d:145 to fail.
And it still emits ../../../a/deep/path.

How do I properly go about this?

Contributor

Yoplitein commented Oct 23, 2015

I'm trying to use Path, but I can't figure out how to wrangle it into submission.
The following was the only way I could get it to actually do anything:

Path path = Path([PathEntry("a"), PathEntry("deep"), PathEntry("path")], true);
Path root = Path([PathEntry(".."), PathEntry(".."), PathEntry("..")], true);
string routerRootDir = path.relativeTo(root).toString;

Everything else causes the assert at path.d:145 to fail.
And it still emits ../../../a/deep/path.

How do I properly go about this?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 23, 2015

Member

It should be Path(router.prefix).relativeTo(Path(req.path)).toString() - assuming that both, router.prefix and req.path, start with a "/", which should always be true.

Member

s-ludwig commented Oct 23, 2015

It should be Path(router.prefix).relativeTo(Path(req.path)).toString() - assuming that both, router.prefix and req.path, start with a "/", which should always be true.

@Yoplitein

This comment has been minimized.

Show comment
Hide comment
@Yoplitein

Yoplitein Oct 23, 2015

Contributor

That doesn't seem to be quite right either.

For paths without a trailing slash, it leaves off the topmost path component. E.g. /a/deep/path/param => /a/deep/
And at the index of the router, it is equivalent to req.rootDir.

Edit: I should mention this is how the browser handles it.

Contributor

Yoplitein commented Oct 23, 2015

That doesn't seem to be quite right either.

For paths without a trailing slash, it leaves off the topmost path component. E.g. /a/deep/path/param => /a/deep/
And at the index of the router, it is equivalent to req.rootDir.

Edit: I should mention this is how the browser handles it.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 23, 2015

Member

Hm right, forgot about that, this issue just came up somewhere else recently. Trailing slash handling doesn't quite match the rules needed here. This should fix it:

auto rp = Path(req.path);
if (!rp.endsWithSlash) rp = rp[0 .. $-1];
string result = Path(prefix).relativeTo(req.path).toString();
Member

s-ludwig commented Oct 23, 2015

Hm right, forgot about that, this issue just came up somewhere else recently. Trailing slash handling doesn't quite match the rules needed here. This should fix it:

auto rp = Path(req.path);
if (!rp.endsWithSlash) rp = rp[0 .. $-1];
string result = Path(prefix).relativeTo(req.path).toString();
@Yoplitein

This comment has been minimized.

Show comment
Hide comment
@Yoplitein

Yoplitein Oct 23, 2015

Contributor

Ok, I think that's all the edge cases ironed out. Thanks for the help.

Contributor

Yoplitein commented Oct 23, 2015

Ok, I think that's all the edge cases ironed out. Thanks for the help.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 9, 2015

Member

Thanks, looks good.

Member

s-ludwig commented Nov 9, 2015

Thanks, looks good.

s-ludwig added a commit that referenced this pull request Nov 9, 2015

Merge pull request #1301 from Yoplitein/addBaseDir
Add HTTPServerRequest.routerRootDir property

@s-ludwig s-ludwig merged commit 55dc9f8 into vibe-d:master Nov 9, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment