Multiple endpoints executed

[darko1979]

When we have multiple endpoints that can be matched with the request path all endpoints are executed, but I expected only first one to be executed.
I made an example to demonstrate my problem:

import * as express from "express";
import { Server, Path, GET, PathParam } from "typescript-rest";

@Path("/api")
class ApiController {
    @Path("/test")
    @GET
    f1(): string {
        console.log('path: /test');
        return 'path: /test';
    }
    @Path("/:name")
    @GET
    f2(@PathParam('name') name: string): string {
        console.log('path: /:name (' + name + ')');
        return 'path: /:name (' + name + ')';
    }
}

let app: express.Application = express();
Server.buildServices(app);

app.listen(8080, function () {
    console.log('Rest Server listening on port 8080!');
});

If I make a GET request 'http://localhost:8080/api/test' it will execute both f1 and f2 functions but will fail with following error:
Error: Can't set headers after they are sent.
because response headers are set in the first one.

Using pure javascript I don't have this problem:

const express = require('express');
const app = express();

const router = express.Router();
router.get('/test', function (req, res, next) {
    console.log('path: /test');
    res.json({ message: 'path: /test' });
});
router.get('/:name', function (req, res, next) {
    console.log('path: /:name');
    res.json({ message: 'path: /:name' });
});
app.use('/api', router);
app.listen(8080);
console.log('app started...');

because after matched endpoint next is not called.

Using typescript-rest I don't have this control and next is always called.
I traced a possible solution in server-container.ts line 328 (part of buildServiceMiddleware function) where I could replace
next();
with
if (!res.headersSent) next();

If this is intended by design could this be made optional by adding another decorator or adding options to path decorator that will indicate to only execute first matched endpoint?

I'm not satisfied with current solution to change paths so this does not happen.

[thiagobustamante]

Hi @darko1979 ,

Yes. it is a problem... But I am not sure what is the best solution to solve it. We need to call next to ensure that any middleware placed after the typescript-rest middlewares be called.

Take a look at #68

[thiagobustamante]

I think we need to add something to give more control to developer to specify if he wants that typescript-rest send a response to the client... I will think about how could be this contract...

And we are accepting suggestions

[darko1979]

I have a suggestion: add option in Path decorator to control if next is called. Here are proposed changes:

feat: path decorator option to call next if headers are sent

In my solution this option to call next if headers are sent is enabled by default, and if somebody does not wan't this it can set this option to false, for example:
@Path("/api", false)
This can be applied to whole class or only a single method.
Or this option can be disabled by default so that default behavior is not to call next if headers are send and if somebody want's this functionality then it has to turn it on with this Path option.

[greghart]

"We need to call next to ensure that any middleware placed after the typescript-rest middlewares be called."
I would argue that this is where the crux of the issue lies -- if typescript-rest sends a response to the client, then we should actually not be calling the next middleware, since that is the convention of how the middleware stack works.

For example, a common pattern (straight from Express docs) is a 404 handler at the bottom of your middleware stack.

app.use(function (req, res, next) {
  res.status(404).send("Sorry can't find that!")
})

Obviously this would not work with how things are now. I think typescript-rest should by default behave according to this functionality -- I'm not even sure an option to change that is necessary, since I'm not sure what the use case for #68 is. Any insight on your use case @ruslan-sh-r ?

[ruslan-sh-r]

@greghart
I had an experience where calling next is a required option. E.g. in order to log all incoming requests with some additional information of it's processing, such as request processing duration.
Also I can imagine a lot of cases where it is very useful to have middlewares after route processing: clean some common resources, close db connections, logging and monitoring purpose.

[greghart]

So I think first we should decide what that would look like in vanilla Express, and then decide how to approach w.r.t. typescript-rest.

Always call next method

necessitates all middleware to have to check if response has gone out yet

// route handler -- this is not even possible through `typescript-rest` in current implementation
express.get( "/test", (req, res, next) => {
  if (!res.headersSent) { 
    res.send( "hello world" );
  }
  next(); // Calling next always
});

express.get('/:token', (req, res, next) => {
  if (!res.headersSent) {
    res.send('some token');
  }
  next();
});

// Performance handling 
express.use((req, res, next ) {
  // Do some logging or something
} )

// 404 handler
express.use((req, res, next ) {
  if (!res.headersSent) {
    res.status(404).send("Sorry can't find that!");
  }
} );

As we can see, one case is completely not supported, and it adds complexity across the board.

Don't next after responding

As an alternative, you could just add event handler onto response if you need to do something after response has gone out.

// Performance handling -- setup listener before routes
express.use((req, res, next ) {
  res.once('finish', () => {
    // Do some logging or something
  })
})

// route handler 
express.get( "/test", (req, res, next) => {
  res.send( "hello world" );
});
express.get('/:token', (req, res, next) => {
  res.send('some token');
});

// 404 handler
express.use((req, res, next ) {
  res.status(404).send("Sorry can't find that!");
} );

And even better, this is supported through typescript-rest -- just setup middleware before your routes, or even use preprocessors to setup cleanup as needed per route.

[krazar]

Hello,

I landed here as we are experiencing this issue.
We have a few resources that has the pattern "ressource/:id and "resource/someCommand" that are now broken.

The change linked to this issue looks more like a breaking change to me.

Besides, I don't see a good enough reason why you would add a middleware after builder services. I believe this makes the lib less predictable.

Adding an option to choose the strategy (nextAfterResponse) globally would fix the issue for me.

Cheers

[Qwerios]

Just want to add that if you follow the recommended pattern of doing an app.use(...) as the last route to capture and format 404 routes you will get bitten by this as well. Every supposedly handled path will also trigger the 404 catch all.

EDIT: And I totally glossed over @greghart his comment saying pretty much the same thing.

[thiagobustamante]

Hi,

I've put together the different suggestions made here and I think we have a solution:

Next function

By default, we call the next function after endpoint execution (even if headers are already sent). As discussed previously, there are use cases that need this behaviour (logs, for example). If we need to disable this, we must be able to inform it explicitly.

So, the proposal is to have an annotation @IgnoreNextMiddlewares. Check here the documentation for the proposal.

@Path('test')
class TestService {
   @GET
   @IgnoreNextMiddlewares
   test() {
       //...
      return 'OK';
   }
}

Remember that we already have a way to explicitly call next if we need to do it according with a specific condition inside our service handler. We can use Context.next

Service Return

By default, we serialize the service result to the user as a response. If the method does not return nothing (void), we send an empty response with a 204 status. It is useful to simplify all the situations where we don't have nothing to send as response.

But, if you need to handle the user response by yourself, you should be able to explicitly inform this. The proposal here is to have a specific return value to inform that you don't want to return nothing.
The Return.NoResponse (check the proposal here) should be used to this:

import {Return} from "typescript-rest";

@Path("noresponse")
class TestNoResponse {
  @GET
  public test() {
    return Return.NoResponse;
  }
}
app.use("/noresponse", (req, res, next) => {
  res.send("I am handling the response here, in other middleware");
});

or

import {Return} from "typescript-rest";

@Path("noresponse")
class TestNoResponse {
  @GET
  public test(@ContextResponse res: express.Response) {
    res.send("I am handling the response here, don't do it automatically");
    return Return.NoResponse;
  }
}

I think that these proposal:

• keeps compatibility with previous versions
• Keeps the usage simple for the most used cases
• And allow customisations to handle all those cases reported here

[greghart]

Re: Next Function, I still believe the logging use case has better ways of handling than altering the expected workflow for Express users (such as response event handling which is an already existing solution). Or at least switching the default so most users don't have to annotate IgnoreNextMiddlewares on every service.

W.r.t. backwards compatibility, since #68 was itself a breaking change, I see this more as a bugfix than a backwards compatibility issue.

[thiagobustamante]

Hi @greghart ,

I still believe the logging use case has better ways of handling than altering the expected workflow for Express users

I did not disagree about the logging use case. But the question is what is the expected workflow for Express users?

I believe that if somebody add a middleware, after the Typescript-rest handler, directly in the express router, that person expects that it will be called after a service call.

let app: express.Application = express();
Server.buildServices(app);

app.use("/mypath", (req, res, next) => {
  // If I add something here, with a middleware in express directly, I expect that it must be called.
});

That is why we handled #68 as a bug and not a change.

But we can put a configuration property in the Serverto allow switch the default, avoiding IgnoreNextMiddlewares in more than one service, if you need to disable it more than once.

something like:

Server.ignoreNextMiddlewares(true);

[greghart]

I think your example highlights the mis-communication, as I think conventional Express users (and the other people in this thread) would expect that next middleware not to happen if Server.buildServices(app) is going to end the response by calling res.send.
That's just what seems to be the case based on documentation and examples of Express, and common usage. It's not prohibited to call next after send, but AFAIK it's not the expected norm.

FWIW +1 to a top level switch to reconcile this 👍

[thiagobustamante]

Hi,

I think your example highlights the mis-communication, as I think conventional Express users (and the other people in this thread) would expect that next middleware not to happen if Server.buildServices(app) is going to end the response by calling res.send.

Maybe... but the point is that expressjs users have the chance to choose when and how to call the methods next and response.send, so I don't think we can reduce the problem to "Don't call next after a service method".

We need to add support to give more control to users of typescript-rest. The changes proposed here allow all possible uses cases.

And with the top level switch (Server.ignoreNextMiddlewares(true);) it is possible to keep the previous behaviour.

And thanks a lot for all contributions on this topic