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

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.

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.

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?

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.

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 ?

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

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