Add @Fallthrough() decorator and global option to enable route fallthrough, which is now off by default

[ArcanoxDragon]

Route fallthrough (route handlers calling next() without parameters) is now off by default, as it is not the implicit behavior in Express (I don't use Koa, but it appears to have similar paradigms to Express). If a route needs to fall through to other routes or "after" middleware, @Fallthrough() can be placed on the route handler. If the user wants all routes to fallthrough by default, the new automaticFallthrough global option can be enabled when initializing routing-controllers.

[NoNameProvided]

Hi @briman0094!

Thanks for the PR! The test (an so the implementation) will need some modification. In routing-controllers every after middleware should be executed when a route had been hit. The @Fallthrough() decorator should only prevent calling the following matching routes but not the after middlewares.

@19majkel94 @pleerock thoughts? I like this, but when implemented this will be a breaking change, however I doubt anyone took advantage of the route fallthrough.

[marshall007]

I'm not even really convinced that the automaticFallthrough config option and @Fallthrough() decorator should be exposed. I think the framework should take a more opinionated approach and disallow this behavior altogether. Given that we already have before/after middleware with plenty of configurability, I don't see a valid use case for allowing controller methods to fall through.

If we expose this stuff as part of the documented public API I think we're just encouraging people to do bad things.

[pleerock]

fallthrough is better then automaticFallthrough . Also TBH fallthrough does not indicate for me what this option does, but I can't find a good name for it. Maybe this terminology is highly used, simply Im not familiar with it, adding other thoughts @19majkel94 @NoNameProvided . Is this word better then something like callNext: boolean ?

[NoNameProvided]

Also TBH fallthrough does not indicate for me what this option does, but I can't find a good name for it. Maybe this terminology is highly used, simply Im not familiar with it

It should be FallThrough because there is no such word as fallthrough, it is fall through. It is used in programming so I am cool with it, however we can make it more explicit via naming it as routeFallThrough.

[pleerock]

So here we faced again with problem that framework calls next after.

I don't like decorator this PR provides, it seems to me that one global option is enough.

I'm okay to disable next call after each controller action by default.
What are you thoughts on it?

@19majkel94 can you please help me to remember what issues it will cause? I think "after" middlewares won't work, am I right?

[NoNameProvided]

@19majkel94 can you please help me to remember what issues it will cause? I think "after" middlewares won't work, am I right?

I think this is the related discussion: #114 (comment) and here #50. You are right the problem was that no after middlewares are called.

The solution would be to handle middlewares at routing-controller level instead of express, right?

[ArcanoxDragon]

I thinking handling after middleware at the routing level is the only solution. Express doesn't really have a concept of "after middleware" because it sends headers after a matched route handler is called. If we want anything to be called after a routing-controllers method, and before headers are sent, I think it will have to be in either callAction or handleSuccess of the driver.

[MichalLytek]

This PR is a move in right way but need more changes and discussion:

• I would prefer @Next() decorator (injecting next function as action param) for those who want explicit fallthrough per route rather than action fallthrough decorator
• this PR breaks after middlewares functionality - it should call manually the first middleware after action handling when the fallthrought if off
• routeFallThrough option name is much better than automaticFallthrough although I would remove this option if we want to have it disabled by default - there is no use case other than backward compatibility due to this breaking change
• there is no test for cases like Get and Head requests  #114 so we don't know if it really helped

@briman0094 please consider my comments and add more commits to this PR - it can't be merged in current state

[ArcanoxDragon]

Thanks for the feedback...I'll work on this a bit later today and into the next week.

[soulski]

Any progress on this PR?

[MichalLytek]

ping @briman0094

[ArcanoxDragon]

My apologies...I've been swamped at work. I'll make a note to make some progress on this over the weekend...hopefully I can finish it by the end of the weekend.

[soulski]

Hi @briman0094, I understand that you're really busy and I really need this PR so would you mine if I copy all of your code and send another PR or I can send PR to your repository to make this PR go through.

[ArcanoxDragon]

Yeah, that's fine. I feel bad for having to keep delaying the PR; I just haven't had the time to look at it yet with all my other projects. You should just be able to fork my fork of this repo, check out the branch for this PR, make your changes, and then make a new PR from your branch without having to copy/paste any code.

[MichalLytek]

@soulski Contributions and PRs are welcome but please take into account my comments 😉

[ArcanoxDragon]

Thanks for taking charge of this; it's really a load off my shoulders 😄

[soulski]

@19majkel94 After take a look on code, I found that If we don't call next, It's not only break after middleware but it also break before middleware in case Koa. Look like this will not be easy fix as I thought because we need to handle different flow on both Express and Koa. Do you have any suggestion?

[MichalLytek]

it also break before middleware in case Koa

But why? All koa's before midlewares do await next() so when we won't call next in our handler, it just return control down in the call stack to the first middleware.

After middlewares are really weird thing in routing-controllers, not used in koa (there is await next() for calling the after handler complete) and in express (everybody patch the .end() method).
You just need to get the after middlewares array in metadata storage and manually call it like in koa it would be return nextMiddleware(ctx, next).

[soulski]

I am mis-understanding koa-router. I just found that if there is middleware that didn't call next append as last middleware, router also match pattern won't be call. So what do you think if I found that routeFallThrough is off, I will put Terminate middleware which is middleware that doesn't call next in the last queue of middleware. This will help me doesn't need to simulate call after middleware.

[MichalLytek]

Bare koa handler doesn't have to call next to make middlewares work. Here is a sample from koa's website:

const Koa = require('koa');
const app = new Koa();

// x-response-time

app.use(async (ctx, next) => {
  const start = Date.now();
  await next();
  const ms = Date.now() - start;
  ctx.set('X-Response-Time', `${ms}ms`);
});

// logger

app.use(async (ctx, next) => {
  const start = Date.now();
  await next();
  const ms = Date.now() - start;
  console.log(`${ctx.method} ${ctx.url} - ${ms}`);
});

// response

app.use(async ctx => {
  ctx.body = 'Hello World';
});

app.listen(3000);

[jschutijzer]

When is this going to be released?

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.