Add a helper to use pipelines with (New)Handlers

[TimNN]

(The code is still missing tests & documentation, but I would like to get some initial feedback before I add those).

This was the solution I ended up with for #292. While cleaning the code up, I remembered that when I was first looking into solutions for my issue, I was very surprised that there didn't seem to be a standard way to use pipelines with plain handlers, only Routers appeared to be supported. So I decided to phrase my solution in those terms: A helper to use a pipeline with a plain handler. (I'd be happy to accept suggestions regarding naming, code organization and better bounds).

(Also note that I find the behavior of the Router, to only apply pipelines when it successfully matches a route, unexpected and surprising. But even if that were to change, I think the utility proposed here would still be valuable since it allows using pipelines without the router).

cc @whitfin, @colinbankier

Fixes #292

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@9e60d87). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #293   +/-   ##
=========================================
  Coverage          ?   79.03%           
=========================================
  Files             ?       91           
  Lines             ?     4503           
  Branches          ?        0           
=========================================
  Hits              ?     3559           
  Misses            ?      944           
  Partials          ?        0

Impacted Files	Coverage Δ
gotham/src/pipeline/chain.rs	36.84% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e60d87...e7ea38d. Read the comment docs.

[whitfin]

Also note that I find the behavior of the Router, to only apply pipelines when it successfully matches a route, unexpected and surprising.

I used to think this when I initially got involved with this project, but have since come to realise that it's much better this way. You don't want to run something like authentication middleware on 404s; it'd be a complete waste of time and energy. If you /do/ want that, it's much better to explicitly do it some other way (which is currently missing, afaik).

I believe all that would happen is that people would start getting confused why things are firing when the routes are invalid. There's no real answer; different frameworks do it differently. We should likely just add some sort of configuration to enable middlewares to fire on 404 (and default to off, for compatibility), and allow the developer to choose.

I'm unsure if there's a way to adapt the existing API space to cover this.

[nyarly]

I'm late to review this, and for that I apologize.

If you're looking to have a pipeline added to every route, that's what the first argument of gotham::router::build::build_router() is for. Am I missing some requirement that isn't served by that function?

[TimNN]

It's been a while since I looked at this, but IIRC the issue I had back then was that build_router only applies the middleware if it successfully matches a route.

[nyarly]

Got it. So the idea was to add a middle-applying handler that you could wrap around the router-as-Handler. That makes sense.

What confused me is that this approach allows for wrapping any Handler in a pipeline, which means that there'd be two ways for a pipeline to be interposed above the resource handler: either in its route, or with a wrapper. That strikes me as a recipe for confusing behaviors and gnarly bugs.

I don't want to shut out your use case, and I apologize again for jumping in late here.

As I see it, we could use pipeline wrappers on handlers everywhere, or specialize this wrapper to surrounding Routers.

What do you think, @colinbankier @whitfin @TimNN ?

[nyarly]

@TimNN Apologies for the delays here. Are you inclined to resolve the conflicts here and land it?

[msrd0]

I'm closing this PR as it has not seen any activity in the last 2 years.