Add Mount middleware #267

SimonTeixidor · 2015-08-23T13:41:44Z

Attempt at #259.

This middleware takes a mount point and a Middleware. When invoked, the mount point is compared with the incoming request, and if they match the underlying middleware is invoked but with the request path rewritten to have the mount point as root.

Ping @cburgdorf

Ryman · 2015-08-23T18:57:47Z

src/mount.rs

+
+use hyper::uri::RequestUri::AbsolutePath;
+
+pub struct Mount {


Should be able to get static dispatch with the following:

pub struct Mount<M: Middleware + Send + Sync + 'static> { mount_point: String, middleware: M }

(It may not be required to specify +Send+Sync+'static as they are implied by Middleware but I forget if that propagates)

Ryman · 2015-08-23T19:19:02Z

Looks like it could do the job 👍

I did take a look at iron's version mentioned in #259 and after looking at the code I'm left wondering a bit if we could abstract the core logic and both benefit from improvements, something to consider for the future I guess.

As for the api, it does contrast a bit with iron's version where they can mount multiple things with the same mount instance, which I guess could potentially have efficiency benefits if mounting multiple things (while being less efficient for single mounts). I'm happy enough with single mounts for now, but I'm wondering if it's a substantial usecase.

Finally, I'm wondering if this is worth isolating in a separate crate? Or do we just hold off until we have infrastructure in place to make that easy (whenever that is)? Again, I'm probably happy enough either way currently.

SimonTeixidor · 2015-08-25T20:42:27Z

@Ryman, I've been through all your comments and adressed everything except the unnecessary copy when the matching branch is not taken. I can't seem to find an elegant way to solve that. If you can, please tell :) My problem is that all solutions I can think of introduces an extra if, and I'd like to not introduce an extra branch where it is not really needed.

Ryman · 2015-08-25T21:57:56Z

My problem is that all solutions I can think of introduces an extra if, and I'd like to not introduce an extra branch where it is not really needed.

What's the concern with an extra branch?

Ryman · 2015-08-25T22:04:37Z

examples/mount.rs

+     * Fall-through behaviour, if StaticFilesHandler does not find a matching file,
+     * the request uri must be reset so that it can be matched against other middleware.
+     */
+    server.mount("/static/files/", StaticFilesHandler::new("examples/assets/"));


Looking at this again, the rewrite rules would make this "/static/files"? In that case, does this mean that the mount will activate for paths such as "/static/files_and_stuff/bar" and pass the uri as "_and_stuff/bar"?

I'm also curious about the behavior of "/", as the uri passed to the mounted middleware would be "" if the base route is caught (i.e. the client sends "/")? I would have expected "/" to be passed?

I'm not really sure what motivates the rewrite rules, can you add a comment to rewrite_mount_point with expected behavior?

SimonTeixidor · 2015-08-25T22:26:30Z

My problem is that all solutions I can think of introduces an extra if, and I'd like to not introduce an extra branch where it is not really needed.

What's the concern with an extra branch?

No big concerns, just that it looks a bit ugly. If there's a way around it, I'd like to avoid it. Otherwise, no big deal.

Looking at this again, the rewrite rules would make this "/static/files"? In that case, does this mean that the mount will activate for paths such as "/static/files_and_stuff/bar" and pass the uri as "_and_stuff/bar"?

Im also curious about the behavior of "/", as the uri passed to the mounted middleware would be "" if the base route is caught (i.e. the client sends "/")? I would have expected "/" to be passed?

Good catch, that's probably right! It's not really related to the rewrite rules though, even without those, this same issue would arise. If the mount path is "/static/files", then we also match "/static/files_and_stuff". If the mount path is "/static/files/", then a request to "/static/files/abc" ends up with the path "abc" where I'd expect it to be "/abc". I think this will work if we make sure that mount paths always end with a slash, and then prepend a slash to the path before calling the underlying middleware. That should solve both those failing cases.

I'm not really sure what motivates the rewrite rules, can you add a comment to rewrite_mount_point with expected behavior?

I'm not sure about them either. We can also just document which format mount points must be written in and hope that users get it right. Or, we could check for leading and trailing slash and return an error if the user got it wrong.

Ryman · 2015-08-25T22:34:12Z

Or, we could check for leading and trailing slash and return an error if the user got it wrong.

Assuming no-one is trying to mount things on an already running server I think this is a good option which allows people to fail quickly at runtime so they can get their stuff working with least surprise. Does make the usage kind of ugly though if it requires an unwrap, would this be a good place to just panic on the basis that it's most likely not what the user intended?

cc @cburgdorf

Ryman · 2015-08-25T22:47:45Z

src/mount.rs

+impl<M: Middleware> Middleware for Mount<M> {
+    fn invoke<'mw, 'conn>(&'mw self, req: &mut Request<'mw, 'conn>, res: Response<'mw>)
+        -> MiddlewareResult<'mw> {
+        if let AbsolutePath(path) = req.origin.uri.clone() {


untested but might work and isn't totally ugly?

let subpath = match req.origin.uri { AbsolutePath(ref path) if path.starts_with(self.mount_point) => { AbsolutePath(path[self.mount_point.len()..].to_owned()) }, _ => return Ok(Continue(res)) }; let original = mem::replace(&mut req.origin.uri, subpath); let result = self.middleware.invoke(req, res); req.origin.uri = original; result

Of course! I never thought to return early in the non mathing case! This will probably work and looks nicer than what I could think of :)

cburgdorf · 2015-08-25T22:55:00Z

I've no idea why github doesn't let me add my comment where it belongs so I'll add it here.

would this be a good place to just panic on the basis that it's most likely not what the user intended?

I'm absolutely in favor of the panic! here. It's probably the best way to notify the developer about a misconfiguration.

GitCop · 2015-08-25T23:23:36Z

Thanks for contributing! Unfortunately, I'm here to tell you there were style issues with your Pull Request:

Commit: a023aa1
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: dc8043c
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 9169c1d
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/nickel-org/nickel.rs/blob/master/contributing.md

This message was auto-generated by https://gitcop.com

SimonTeixidor · 2015-08-25T23:24:59Z

I've fixed the broken logic discovered by @Ryman, removed the rewrite rules and replaced them with a panic!, and fixed the unnecessary copy when the mount point does not match the request.

homu · 2015-08-25T23:27:03Z

☔ The latest upstream changes (presumably #270) made this pull request unmergeable. Please resolve the merge conflicts.

cburgdorf · 2015-08-25T23:28:54Z

Yay! We finally have a bot! Love it :)

GitCop · 2015-08-25T23:52:32Z

Thanks for contributing! Unfortunately, I'm here to tell you there were style issues with your Pull Request:

Commit: a023aa1
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: dc8043c
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 9169c1d
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 3e7bda5
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 807566e
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/nickel-org/nickel.rs/blob/master/contributing.md

This message was auto-generated by https://gitcop.com

GitCop · 2015-08-26T00:05:09Z

Thanks for contributing! Unfortunately, I'm here to tell you there were style issues with your Pull Request:

Commit: a023aa1
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: dc8043c
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 9169c1d
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 3e7bda5
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 807566e
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 4193447
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/nickel-org/nickel.rs/blob/master/contributing.md

This message was auto-generated by https://gitcop.com

Ryman · 2015-08-28T14:53:34Z

@simonpersson If you reword commits or squash I think we're good to go with this for now? Or perhaps we should test @cburgdorf's project with commits from this branch?

SimonTeixidor · 2015-08-28T15:17:06Z

I squashed the commits, let's see if the tests pass and you can feel free to merge it!

Ryman · 2015-08-29T14:46:13Z

@homu r+

homu · 2015-08-29T14:46:14Z

📌 Commit 0000000 has been approved by Ryman

Ryman · 2015-08-29T14:48:39Z

Err, it's definitely not commit 00000000

Ryman · 2015-08-29T14:49:14Z

@homu r-

Ryman · 2015-08-29T14:51:06Z

@homu r+

homu · 2015-08-29T14:51:07Z

📌 Commit 0000000 has been approved by Ryman

Ryman · 2015-08-29T14:51:19Z

Sigh :D, let's see what happens I guess

Ryman · 2015-08-29T17:06:31Z

@homu r+

homu · 2015-08-29T17:06:33Z

💊 [DEBUG] head_sha reset from 0000000000000000000000000000000000000000 to 0f25836

homu · 2015-08-29T17:06:33Z

📌 Commit 0f25836 has been approved by Ryman

@cburgdorf

Add Mount middleware Attempt at #259. This middleware takes a mount point and a `Middleware`. When invoked, the mount point is compared with the incoming request, and if they match the underlying middleware is invoked but with the request path rewritten to have the mount point as root. Ping @cburgdorf

homu · 2015-08-29T17:06:35Z

⌛ Testing commit 0f25836 with merge 0622106...

homu · 2015-08-29T17:13:32Z

☀️ Test successful - status

Ryman · 2015-08-29T17:15:25Z

Thanks @barosl! 🏄

cburgdorf · 2015-08-31T12:04:14Z

That's awesome! I'll test that with my classroom project later this week.

Ryman reviewed Aug 23, 2015
View reviewed changes

Ryman mentioned this pull request Aug 24, 2015

Router #263

Open

Ryman reviewed Aug 25, 2015
View reviewed changes

SimonTeixidor force-pushed the mount branch from 4193447 to 0f25836 Compare August 28, 2015 15:15

feat(nickel): Add Mount middleware.

0f25836

Ryman mentioned this pull request Aug 29, 2015

Homu chooses the wrong commit barosl/homu#99

Closed

homu merged commit 0f25836 into nickel-org:master Aug 29, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Mount middleware #267

Add Mount middleware #267

SimonTeixidor commented Aug 23, 2015

Ryman Aug 23, 2015

Ryman commented Aug 23, 2015

SimonTeixidor commented Aug 25, 2015

Ryman commented Aug 25, 2015

Ryman Aug 25, 2015

SimonTeixidor commented Aug 25, 2015

Ryman commented Aug 25, 2015

Ryman Aug 25, 2015

SimonTeixidor Aug 25, 2015

cburgdorf commented Aug 25, 2015

GitCop commented Aug 25, 2015

SimonTeixidor commented Aug 25, 2015

homu commented Aug 25, 2015

cburgdorf commented Aug 25, 2015

GitCop commented Aug 25, 2015

GitCop commented Aug 26, 2015

Ryman commented Aug 28, 2015

SimonTeixidor commented Aug 28, 2015

Ryman commented Aug 29, 2015

homu commented Aug 29, 2015

Ryman commented Aug 29, 2015

Ryman commented Aug 29, 2015

Ryman commented Aug 29, 2015

homu commented Aug 29, 2015

Ryman commented Aug 29, 2015

Ryman commented Aug 29, 2015

homu commented Aug 29, 2015

homu commented Aug 29, 2015

homu commented Aug 29, 2015

homu commented Aug 29, 2015

Ryman commented Aug 29, 2015

cburgdorf commented Aug 31, 2015

Add Mount middleware #267

Add Mount middleware #267

Conversation

SimonTeixidor commented Aug 23, 2015

Ryman Aug 23, 2015

Choose a reason for hiding this comment

Ryman commented Aug 23, 2015

SimonTeixidor commented Aug 25, 2015

Ryman commented Aug 25, 2015

Ryman Aug 25, 2015

Choose a reason for hiding this comment

SimonTeixidor commented Aug 25, 2015

Ryman commented Aug 25, 2015

Ryman Aug 25, 2015

Choose a reason for hiding this comment

SimonTeixidor Aug 25, 2015

Choose a reason for hiding this comment

cburgdorf commented Aug 25, 2015

GitCop commented Aug 25, 2015

SimonTeixidor commented Aug 25, 2015

homu commented Aug 25, 2015

cburgdorf commented Aug 25, 2015

GitCop commented Aug 25, 2015

GitCop commented Aug 26, 2015

Ryman commented Aug 28, 2015

SimonTeixidor commented Aug 28, 2015

Ryman commented Aug 29, 2015

homu commented Aug 29, 2015

Ryman commented Aug 29, 2015

Ryman commented Aug 29, 2015

Ryman commented Aug 29, 2015

homu commented Aug 29, 2015

Ryman commented Aug 29, 2015

Ryman commented Aug 29, 2015

homu commented Aug 29, 2015

homu commented Aug 29, 2015

homu commented Aug 29, 2015

homu commented Aug 29, 2015

Ryman commented Aug 29, 2015

cburgdorf commented Aug 31, 2015