Route add order mechanism about tsed HOT 43 CLOSED

Error with response.setHeader("X-Managed-By", "Express-router-decorator"); is patched with v1.3.1 @zoubarevm ;)

from tsed.

Sorry. I'll just re tested this case with another project and you've right... never answer to a question when I'm tired XD. I'll inspect the problem and fix that. If isn't a big problem...

from tsed.

Fixed with v1.4.15

from tsed.

@m0uneer And thanks for your patience ^^.

from tsed.

Hi @ArtemDerevnjuk,

I’ll check that tomorrow, but It should works. It’s covered by one of my e2e test :)

from tsed.

@vembry yes you have to switch methode order (like with a pure express app).

order is determined by code implementation which is totally logic ^^.

See you

from tsed.

Hello Alex,

You have the same problem when you use the express method.

app.get('/account/:id', cb);
app.get('/account/manager', cb);

The request /account/manager satisfy always the first condition according to the routing list.

It is the responsibility of the developer to put his route in the right order :)

For me isn't a bug.

from tsed.

Yes is the same problem (order matters) but I was thinking if we can see routes not starting with : and add them first and add : starting routes at the end when processing decorators. In this way we can take some burden off the developers shoulder.

Maybe is not a bug but an improvement.

from tsed.

Ok I see. I think, it's a big works and behavior is unpredictable compared to the implementation that the developer will do.

from tsed.

But right now is the order of apparition of methods in the class guaranteed to be kept by typescript when constructing method metadata array ?

from tsed.

Typescript keeps the declaration order of the methods and therefore associated metadata.
So, the routing will be predicted by the order of the methods/decorators implementations :)

from tsed.

Hey guys,

It seems like in my case, both routes will get called no matter which order I put them in. The second one would even throw an exception indicating the the headers can no longer be modified (see #37).

Is there a way to handle a route and prevent it from another controller method from being hit? Maybe it has something to do with the fact I'm returning promises?

Thanks in advance for any help!

from tsed.

Hey @zoubarevm .
Have you a complete example of your controller ?

• For the header, I see what you mean. Because you have two rules that respond to the request, the response.setHeader("X-Managed-By", "Express-router-decorator"); is called once too. I'll fix that.

• Is there a way to handle a route and prevent it from another controller method from being hit? Maybe it has something to do with the fact I'm returning promises?

Promises isn't the cause. TsExpressDecorators use the nextFunction to call the next middleware. The nextFunction is binded with the promise returned by your method.

Prevent isn't possible because with Express you can add a lot of middleware for one request/route. But I can also be mistaken.

from tsed.

@Romakita awesome, it seems like it worked.

I also targeted the routes more specifically (by adding a regex to the parameter (\\d+)). Seemed like it solved the duplicate route resolution problem.

Thanks for your help!

from tsed.

Hello everybody,
I face a similar problem when I return a promise even if route order is fine.

  @Get('/new')
  public new (
    @Request() req: Express.Request,
    @Response() res: Express.Response,
  ) {
    console.log('new');
    return new Promise((resolve, reject) => {
      resolve('new');
    }).then(d => {
      res.send(d);
    });
  }

  @Get('/:id')
  public param (
    @PathParams('id') id: string,
    @Request() req: Express.Request,
    @Response() res: Express.Response,
  ) {
    console.log('param');
    res.send(id);
  }

It will log both new and param in that order and fire a console error with Error: Can't set headers after they are sent.

Console:

new
param
Error: Can't set headers after they are sent.

from tsed.

Hi @m0uneer
Your problem is that you to use two way to send you response and two methods to intercept an incoming request.

First case:

@Get('/new')
  public new (
    @Request() req: Express.Request
  ) {
    console.log('new');
    return new Promise((resolve, reject) => {
      resolve('new');
    });
  }

You don't need to use response if you want just return the "new" data. ts-express-decorators will sent the response automatically.

But in some case you need to send manually your response. In this case, don't return a promise.

Just do that:

@Get('/new')
  public new (
    @Request() req: Express.Request,
    @Response() res: Express.Response,
  ) {
    console.log('new');
    // remove return instruction.
    new Promise((resolve, reject) => {
      resolve('new');
    }).then(d => {
      res.send(d);
    });
  }

In your case you have problem your routes implementation. When you give theses routes:

• /new
• /:id

express will resolve the two routes when the called route is /new. But you send response in the first and second routes. You have a conflict => Error: Can't set headers after they are sent.. It's pure problem with Express and not with TsExpressDecorators ;).

I will not change the behavior of Express !

So I recommend you to change your implementation to do not have two routes that can be resolved at the same time for one request, if that's not what you want.

Here an example that will work fine:

  @Get('/')  // remove new or change the second routes
  public new (
    @Request() req: Express.Request,
    @Response() res: Express.Response
  ) {
    console.log('new');
    // 1st usecase
   response.send('new');

    // or by promise
    return new Promise((resolve, reject) => {
       resolve('new');
    });
  }

  @Get('/:id')  // or @Get('find/:id') or other stuff that not match with the first route.
  public param (
    @PathParams('id') id: string,
    @Request() req: Express.Request,
    @Response() res: Express.Response,
  ) {
    console.log('param');
    res.send(id);
  }

I hope my explanation will help you.

See you,
Romain

from tsed.

@Romakita, Thanks a lot Romain your explanations helps a lot. I rechecked Express Doc and found that I must call next explicitly to go to the next matching route. So, the problem I'm facing now - as far as I know - isn't an Express matter So I had a close look into the code to see what's going on.
I found this method invokeMethod() inside MiddlewareService class at /src/services/middleware.ts file,

    public invokeMethod(settings: IInjectableMiddlewareMethod, localScope: IInvokableScope): any {
        ...

        return new Promise((resolve, reject) => {
            ...
            Promise
                .resolve()
                .then(() => {
                  ...
                })
                .then((data) => {
                    if (data !== undefined) {
                        localScope.request.storeData(data);
                    }

                    if (!hasNextFn || isPromise) {
                        resolve();
                    }
                })
                .catch(reject);

        })
            .then(
                () => {
                    next();  // <--- Here is my question
                },
                (err) => {
                    next(err);
                }
            );
    }

Isn't this next() call the responsible for the conflict? What if we just return the response here and let next to be called explicitly if required? I appreciate your explanation very much. Thank you.

from tsed.

Waiting your prompt response @Romakita :)

from tsed.

Hi @m0uneer,

To use promise with Express, I'm obliged to ask Express to pass me the next function. If I don't get the next function, the middleware will be finished before the Promise has finished his execution.

When you get the next express function, you must to call this. If you don't that, Express wouldn't call the next middleware. Even if you send a response in your middleware, you must call the next express function. But if you use Express, you certainly know this use case to send a response:

app.get("/", function (request, response) {
     response.send("data");
});

This code don't call the next express function. Express, call implicitly the next function for you ! It's an equivalent to:

app.get("/", function (request, response, next) {
     response.send("data");
     next();
});

Finally, I take your code in pure express implementation

app
 .get("/new", function (request, response) {
        response.send("data1");
  })
  .get("/:id", function (request, response) {
       response.send("data2");
  });

You will have a conflict and you will get an error when you call /new => headers are already sent.

To answer to your question, here the complete code:

 return new Promise((resolve, reject) => {

            let parameters, isPromise: boolean;

            localScope.next = reject;  // ! IMPORTANT

            Promise
                .resolve()
                .then(() => {
                     const result = handler()(...parameters);

                     isPromise = result && result.then;

                     return result;
                })
                .then((data) => {

                    if (data !== undefined) {
                        localScope.request.storeData(data);
                    }

                    if (!hasNextFn || isPromise) {
                        resolve(); // (1)
                    }
                })
                .catch(reject);

        })
            .then(
                () => {
                    next(); //  and (1)
                },
                (err) => {
                    next(err); // (2)
                }
            );

The function call that you pointed will be called in this cases:

• Your method return Promise, so the script waiting the resolution of the promise. Final call is (1).

myMethod() {
   return Promise.resolve({});
}

=> isPromise = true, hasNextFn = false

• Your method invoke @Next() decorator, so in this case, I inject the reject() function in @Next() decorator. Final call is (2).

myMethod(@Next() next) {
    next() // or next(new Error());
}

=> isPromise = false, hasNextFn = true

• Your method return an Object directly. Final call is (1).

myMethod() {
   return {};
}

=> isPromise = false, hasNextFn = false

• Your method use classic call:

myMethod(request, response, next)

=> isPromise = false, hasNextFn = true

• Your method use next and Promise at the same time:

myMethod(@Next()){
    next("next data");
    return Promise.resolve("promise data");
}

=> isPromise = true, hasNextFn = true
In this case, next will be called before the promise. I'm lucky because, the promise can be resolved once a time. So the final call is (2). If I resolve myMethod's promise before next(), it's works too, but the final call is (1). My recommendation is : not to use at the same time Promise and @Next().

But I know, I have one case where this script doesn't works fine:

• Your method doesn't invoke next and doesn't return a Promise:

myMethod(request, response){}

=> isPromise = false, hasNextFn = false

See you :)
Romain

from tsed.

When you get the next express function, you must to call this. If you don't that, Express wouldn't call the next middleware.

I tried a pure javascript installation of express to make sure this behavior is normal but unfortunately, the next middleware is never called without injecting next and calling it.

router.get('/test', function (req, res) {
  console.log(1); // logged and server hangs
}, function (req, res, next) {
  console.log(2); // never been called
  res.json();
});

And I returned to documentation but couldn't find any useful info about this bahaviour. Could you please refer to the documentation of Express for this?

from tsed.

Hi @m0uneer ,

In this case, I'm not sure if express follow the rules that I had explained. I'll tried ASAP ;)

This example works:

router.use(function (req, res) {
  console.log(1); // logged and server hangs
})
.use(function (req, res, next) {
  console.log(2); // logged and server hangs
  next();
})

But It didn't work in this case:

router.use(function (req, res, next) { // next is injected but never called
  console.log(1); // logged and server hangs
})
.use(function (req, res, next) {
  console.log(2); // never log
  next();
})

Here a link on the middleware, maybe you can find the right explication:
http://expressjs.com/en/guide/writing-middleware.html

See you
Romain

from tsed.

I'm sorry, Romain, but the first and second example are not working... both only log 1 then server hangs and never log 2 (Tested with a fresh Express installation). The only way to call the next middleware is to inject next(); and call it (The Express way). But the scenarios you're talking about are not mentioned in the docs and unfortunately will not work.

Consider the following:

  @Get('/posts')
  public async getPost (@PathParams('postId') postId: number,
                                     @Response() res: Express.Response) {
    const hasAccess = await this.hasUserAccess(postId);
    if (!hasAccess) {
      res.status(401).json({
        level: 'core',
        redirectUrl: 'A URL'
      });
      
      return;
    }

    return this.postService.getPost(postId);
  }

  @Get('/:id')
  public async anyDummyMethod (){
  }

1. Those routes are properly sorted and I never expect a routing conflict with normal express (and it doesn't conflict) but with current ts-express-decorators implementations, it conflicts causing headers being sent error. Even injecting @Next() without calling, doesn't help with the conflict. The only way to prevent the conflict is to never use async/await and injecting @Next() without calling (Which, actually, cannot be considered a solution)
2. Back to my question, you recommended:

But in some case you need to send manually your response. In this case, don't return a promise.

I want to use async/await or return a promise and at the same time assert and return a custom response json inside the controller method? This will require injecting the @response() and (implicitly) will be compiled to return a promise.

As I understand your point of view, this will not work properly. So what do you think?

from tsed.

Hi @m0uneer,

I'm sorry for the bad example... I'm surprised too.

I have two proposal for your problem :

With Middleware

import {Unauthozised} from "ts-httpexceptions";

@Middleware()
export class AccessPostMiddleware {
   use(@PathParams('postId') postId: number, @Response() response, @Next() next) {
        if(!this.hasUserAccess(postId)) {
          next(new Unauthozised(json.stringify({
             level: 'core',
             redirectUrl: 'A URL'
          })));
        } else {
           next();
        }
   }
   
   private hasUserAccess(postId) {
      return true;
   }
}

@Controller("/")
class SomeClass {

   @Get("/:id")
   @UseBefore(AccessPostMiddleware)
   public async getPost (@PathParams("postId") postId: number) {
       return this.postService.getPost(postId);
   }
}

In addition, you can create your custom decorator like this:

export function AccessPost (target, propertyKey, descriptor) {
    return UseBefore(AccessMiddleware)(target, propertyKey, descriptor);
}

then:

@Controller("/")
class SomeClass {

   @Get("/:id")
   @AccessPost
   public async getPost (@PathParams("postId") postId: number) {
       return this.postService.getPost(postId);
   }
}

With throwing exception

import {Unauthozised} from "ts-httpexceptions";
class PostCtrl {
  @Get('/posts')
  public async getPost (@PathParams('postId') postId: number) {

    const hasAccess = await this.hasUserAccess(postId);

    if (!hasAccess) {
      throw new Unauthozised(json.stringify({
        level: 'core',
        redirectUrl: 'A URL'
      }));
      
      return;
    }

    return this.postService.getPost(postId);
  }

  @Get('/:id')
  public async anyDummyMethod (){
  }
}

Middleware approach is more reusable (with decorator). But it's maybe overkill in your case ^^.

Actually, I work on your problem to solve it in the future version. But, promisify express middleware it's very hard with all possible use case...

See you,
Romain

from tsed.

Thanks Romain for your proposals, but they both don't solve the problem of the conflict. Also the way you throw the Exception will return a stringified error message and not a json.
Waiting for your prompt response :)

from tsed.

Hi Romain,

Any news about this issue? and I think it should be open not to be forgotten.

Thanks for your help

from tsed.

Hi @m0uneer ,

My apologize :(

Have you test the 1.4.11 ? I've added some fix about this feature. If this version didn't solve it, you can set the debug option to true on the serverSettings.

@ServerSettings({debug: true})
class Server extends ServerLoader {}

Send me the log trace when you have your error :) (since the first trace with incoming request).

See you,
Romain

from tsed.

Thanks, Romain,

I tried your last version but routes still conflict.

This is an example (of many) to reproduce the conflict

  @Get('/test')
  public test () {
    return 'test';
  }

  @Get('/:id')
  public get (@PathParams('id') id: number) {
    return 'id';
  }

Log Trace:

[DEBUG] [#1] -- Incoming request --------------------------------------------------------
[DEBUG] [#1] Route => GET /api/test
[DEBUG] [#1] Request.id => 1
[DEBUG] [#1] Headers => {"host":"192.168.1.101:3020","connection":"keep-alive","postman-token":"1f5979d6-aec3-0bc4-11fb-aa1c555424c4","cache-control":"no-cache","x-postman-interceptor-id":"79cdef0f-5d41-1789-6dc7-d240ff1ec67f","xsrf-token":"P4OFDp6A-a12J4GvdL-t3TCO7YcFecqBa07w","user-agent":"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36","content-type":"application/json","accept":"*/*","accept-encoding":"gzip, deflate, sdch, br","accept-language":"en-US,en;q=0.8,ar;q=0.6,ru;q=0.4,nl;q=0.2","cookie":"connect.sid=s%3AQtiXZlJ6-nFj6Rg5Hqw0YTOH62HnzG5J.tQj9yaWp6vRLetRjnpT7X6UtDJU2htbsEc1FXAMroWs"}
[DEBUG] [#1] ----------------------------------------------------------------------------
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":false}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"cookieParser","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"cookieParser","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"compression","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"compression","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"jsonParser","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"jsonParser","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"urlencodedParser","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"urlencodedParser","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"session","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"session","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"bound initialize","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"bound initialize","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"bound authenticate","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"bound authenticate","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[WARN ] [#1] [INVOKE][DUPLICATE] The handler have been resolved twice. See your code and find if you use @Next() and Promise at the same time.
[WARN ] [#1] [INVOKE][DUPLICATE] Trace: {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true,"returnPromise":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true,"returnPromise":true}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true,"returnPromise":true}
[DEBUG] [#1] Endpoint => {"target":"Ctrl","methodClass":"test","httpMethod":"get"}
[DEBUG] [#1] [INVOKE][START] {"type":"ENDPOINT","target":"Ctrl","methodName":"test","injectable":false,"hasNextFn":false}
[DEBUG] [#1] [INVOKE][END  ] {"type":"ENDPOINT","target":"Ctrl","methodName":"test","injectable":false,"hasNextFn":false,"data":"test"}
[DEBUG] [#1] [INVOKE][START] {"type":"MIDDLEWARE","target":"SendResponseMiddleware","methodName":"use","injectable":true,"hasNextFn":false}
[WARN ] [#1] [INVOKE][DUPLICATE] The handler have been resolved twice. See your code and find if you use @Next() and Promise at the same time.
[WARN ] [#1] [INVOKE][DUPLICATE] Trace: {"type":"MIDDLEWARE","target":"anonymous","injectable":false,"hasNextFn":true,"returnPromise":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"MIDDLEWARE","target":"SendResponseMiddleware","methodName":"use","injectable":true,"hasNextFn":false}
[DEBUG] [#1] Endpoint => {"target":"Ctrl","methodClass":"get","httpMethod":"get"}
[DEBUG] [#1] [INVOKE][START] {"type":"ENDPOINT","target":"Ctrl","methodName":"get","injectable":true,"hasNextFn":false}
[WARN ] [#1] [INVOKE] {"name":"BAD_REQUEST","type":"HTTP_EXCEPTION","status":400,"message":"Bad request, parameter request.params.id. Cast error. Expression value is not a number."}

[DEBUG] [#1] [INVOKE][START] {"type":"ERROR","target":"bound $onError","injectable":false,"hasNextFn":true}
BadRequest {
  name: 'BAD_REQUEST',
  type: 'HTTP_EXCEPTION',
  status: 400,
  message: 'Bad request, parameter request.params.id. Cast error. Expression value is not a number.' }
[DEBUG] [#1] [INVOKE][START] {"type":"ERROR","target":"GlobalErrorHandlerMiddleware","methodName":"use","injectable":true,"hasNextFn":true}
[DEBUG] [#1] [INVOKE][END  ] {"type":"ERROR","target":"GlobalErrorHandlerMiddleware","methodName":"use","injectable":true,"hasNextFn":true,"error":{}}
[WARN ] [#1] [INVOKE] Error: Can't set headers after they are sent. 
    at ServerResponse.OutgoingMessage.setHeader (_http_outgoing.js:346:11)
    at ServerResponse.header (server/node_modules/express/lib/response.js:730:10)
    at ServerResponse.res.json (server/dist/app.js:131:28)
    at Server.$onError (server/dist/app.js:216:13)
    at server/node_modules/ts-express-decorators/src/services/middleware.ts:283:45
    at propagateAslWrapper (server/node_modules/async-listener/index.js:468:23)
    at server/node_modules/async-listener/glue.js:188:31
    at server/node_modules/async-listener/index.js:505:70
    at server/node_modules/async-listener/glue.js:188:31
[WARN ] [#1] [INVOKE] Error: Can't set headers after they are sent. 
    at ServerResponse.OutgoingMessage.setHeader (_http_outgoing.js:346:11)
    at ServerResponse.header (server/node_modules/express/lib/response.js:730:10)
    at ServerResponse.res.json (server/dist/app.js:131:28)
    at Server.$onError (server/dist/app.js:216:13)
    at server/node_modules/ts-express-decorators/src/services/middleware.ts:283:45
    at propagateAslWrapper (server/node_modules/async-listener/index.js:468:23)
    at server/node_modules/async-listener/glue.js:188:31
    at server/node_modules/async-listener/index.js:505:70
    at server/node_modules/async-listener/glue.js:188:31
info: GET /api/test 400 959ms statusCode=400, url=/api/test, host=192.168.1.101:3020, connection=keep-alive, postman-token=1f5979d6-aec3-0bc4-11fb-aa1c555424c4, cache-control=no-cache, x-postman-interceptor-id=79cdef0f-5d41-1789-6dc7-d240ff1ec67f, xsrf-token=P4OFDp6A-a12J4GvdL-t3TCO7YcFecqBa07w, user-agent=Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36, content-type=application/json, accept=*/*, accept-encoding=gzip, deflate, sdch, br, accept-language=en-US,en;q=0.8,ar;q=0.6,ru;q=0.4,nl;q=0.2, cookie=connect.sid=s%3AQtiXZlJ6-nFj6Rg5Hqw0YTOH62HnzG5J.tQj9yaWp6vRLetRjnpT7X6UtDJU2htbsEc1FXAMroWs, method=GET, httpVersion=1.1, originalUrl=/api/test, , responseTime=959
Error: Can't set headers after they are sent.
    at ServerResponse.OutgoingMessage.setHeader (_http_outgoing.js:346:11)
    at ServerResponse.header (server/node_modules/express/lib/response.js:730:10)
    at ServerResponse.res.json (server/dist/app.js:131:28)
    at Server.$onError (server/dist/app.js:216:13)
    at server/node_modules/ts-express-decorators/src/services/middleware.ts:283:45
    at propagateAslWrapper (server/node_modules/async-listener/index.js:468:23)
    at server/node_modules/async-listener/glue.js:188:31
    at server/node_modules/async-listener/index.js:505:70
    at server/node_modules/async-listener/glue.js:188:31

The Express way, a request to /test should response with 'test' and should never - by any mean - reach /:id. We, thankfully, agreed on this on the previous replies. And also agreed that the only way to let the request reach /:id is to call next() explicitly, this is Express.

You're implementations consider some useful use cases but they are strange for someone familiar with Express known use cases. Also, those implementations are causing problems and there are no working proposals from your replies above.

Thanks for your sincere patience and support, Romain

Mouneer

from tsed.

Hi @m0uneer ,

I'll fix that ASAP. Thanks for the log trace :)
Romain

from tsed.

Thanks, Romain :)

from tsed.

HI @m0uneer ,

It's fixed :D v1.4.12

See you

from tsed.

Thanks, Romain,

The conflict itself has been fixed but what if I want to navigate through two route handlers. The fix has blocked this use case.

  @Get('/:id')
  public assert (@PathParams('id') id: number,
                         @Next() next: Function) {
    return assert().then(() => { // Please notice that `return` here is not optional
      // if i want use async/await. e,i `public async assert(...`.
      next(); // I expect `next()` to call the next handler but it doesn't.
    });
  }

  @Get('/:id')
  public get (@PathParams('id') id: number) {
    // This handler isn't called.
    return 'id';
  }

What do you think?

Thanks a lot Romain

from tsed.

I thinks if you try the same example in express version, you'll have the same problem :)
(I'm sure because I'd just tested this case).
Here the pure express implementation:

app.get(':id', (request, response, next) => {
   console.log('ID1');
   Promise.resolve().then(() => {
      next();
    });
})

app.get(':id', (request, response) => { // will be ignored by express
   console.log('ID1');
   response.send("id")
})

I thinks your approach isn't the good way. Two handler on the same route add twice with app.get() (or other method) will give a problem.

If you need to have to handler on same route, you need to use the first handler as middleware and the second as an Endpoint.

So you can solve your problem like this:

  public assert (@PathParams('id') id: number,
                         @Next() next: Function) {
    return Promise.resolve().then(() => {
      next(); // not necessary normally but why not, I accept this challenge :p
    });
  }

  @Get('/:id', this.assert)
  public get (@PathParams('id') id: number) {
    // This handler isn't called.
    return 'id';
  }

This solution is described on https://github.com/Romakita/ts-express-decorators/wiki/Controllers

But with the experience, I'll preferred to use Middleware abstraction with the decorators.

Maybe you case try this:

@Middleware()
export class CustomMiddleware {
   use(@PathParams('id') id: number,
                         @Next() next: Function) {
      return Promise.resolve().then(() => {
         next(); // not necessary normally but why not, I accept this challenge :p
      });
   }
}

@Controller("/")
class SomeClass {

  @Get("/:id")
  @UseBefore(CustomMiddleware)
  public get (@PathParams('id') id: number) {
    // This handler isn't called.
    return 'id';
  }
}

You can see the middleware documentation https://github.com/Romakita/ts-express-decorators/wiki/Middlewares

You have the choice :)
See you,
Romain

from tsed.

I will try it myself today and let you know ... thanks again Romain

from tsed.

I thinks if you try the same example in express version, you'll have the same problem :)
(I'm sure because I'd just tested this case).

Unfortunately, I tried it and I found it valid express implementation. (I'm also sure :D, I've just tested it)

Also, I see that my approach is good because:

1. I'm filtering the requests. I don't need the request reach some endpoints if it has anything wrong.
2. I agree that it is applicable through a decorator or a middleware, but I see (depending on my use cases) they are just duplications. Why should I add the decorator/middleware for each endpoint? This also increases the potential of a human mistake.
3. Dependencies, more code, testing, maintenance, ...

So I see it's a bug :)

Thanks, Romain

from tsed.

Well, if your example with Express work can you forward me the code. Maybe I forgot something.

from tsed.

Great Romain ... It now works! ... Thanks for your support :)

from tsed.

@Romakita seems this bug raised up in v5.9.0 again

from tsed.

Hello @ArtemDerevnjuk
For me all works as expected.
Here e2e test https://github.com/TypedProject/ts-express-decorators/blob/production/test/integration/response.spec.ts
And his controller with the different response scenario:
https://github.com/TypedProject/ts-express-decorators/blob/production/test/integration/app/controllers/responses/ResponseScenarioCtrl.ts

Can you check different examples and tell me if one of this match with your usecase/issue or not. Probably I missed something, but today all usecases described in ResponseScenarioCtrl are officially supported.

See you,
Romain

from tsed.

@Romakita Hi) Thanks for quick reply!
I'll test again, if I find something I'll write you here.

from tsed.

Problem with the different endpoints which have the same or near route pattern. In progress

from tsed.

Hi @Romakita !
sorry for commenting on an old post, but i just want to make sure, that there are no code fix/improvement for this un-orderly path declaration?

e.g if i have something like below

  @Get("/:documentId")
  @(Returns(200, Document).Description("get single document"))
  getByDocumentId(
    @HeaderParams("x-token") token: string, 
    @PathParams("documentId") documentId: number
  ): Document {
    return this.documentService.getDocumentById(documentId);
  }

  @Get("/q")
  @(Returns(200, Array).Of(Document).Description("search documents"))
  getBySearch(
    @HeaderParams("x-token") token: string, 
    @QueryParams("content") searchText: string
  ): Document[] {
    return this.documentService.getDocuments();
  }

i'd have to switch and declare /q before /:documentId like below?

  @Get("/q")
  @(Returns(200, Array).Of(Document).Description("search documents"))
  getBySearch(
    @HeaderParams("x-token") token: string, 
    @QueryParams("content") searchText: string
  ): Document[] {
    return this.documentService.getDocuments();
  }
  
  @Get("/:documentId")
  @(Returns(200, Document).Description("get single document"))
  getByDocumentId(
    @HeaderParams("x-token") token: string, 
    @PathParams("documentId") documentId: number
  ): Document {
    return this.documentService.getDocumentById(documentId);
  }

to prevent request went to /:documentId instead of /q, is it like that?

from tsed.

i apologize, apparently it's already covered in the documentation here

from tsed.

No worries ;)

from tsed.