Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setting status codes is not working within Promise #94

Closed
ryo-nbx opened this issue May 5, 2017 · 11 comments
Closed

setting status codes is not working within Promise #94

ryo-nbx opened this issue May 5, 2017 · 11 comments

Comments

@ryo-nbx
Copy link

ryo-nbx commented May 5, 2017

When defining error and success codes for API functions I realized a problem when using Promises. When setting a status code within a promise this code is not returned by the function.

For example this controller function:
@Tags('Car')
@SuccessResponse('200', 'successful')
@Response('404', 'not found')
@Response('407', 'internal error, could not process request')
@Get('{id}')
public async getCarById (@Path('id') id: string ) : Promise<Car> {
return service.getCar(id).then( (car): any => {
if (!car) {
// not found
this.setStatus(404);
}
return car;
}).catch(function (error) {
console.log(error);
// internal error
this.setStatus(407);
return null;
});
}

This function calls a method at a database service and gets a Promise. After evaluating the result of this promise a response code might be set. Calling this function with a missing ID should cause a 404 error. But in reality it returns 204. The console prompts a 404 object but this is not handled by the generated routes.

I tracked down the problem to the generated routes.ts file and its promiseHandler function.
function promiseHandler(promise: any, statusCode: any, response: any, next: any) {

The method takes a promise and status codes from the controller. This tells me that custom status codes are only used when they are set before the promiseHandler function call. If you set a code within a controller function (and not within a promised function) it will be used. If you use a Promise and set a code within it, this setStatus() call is executed after the promiseHandler() call and so the status code will be ignored.

I looked for a solution and there seems to be a simple one but it requires to change TSOA template code.

I think this should work:

tsoa/src/routeGeneration/templates/express.ts

...
export function RegisterRoutes(app: any) {
{{#each controllers}}
{{#each actions}}
...
const promise = controller.{{name}}.apply(controller, validatedArgs);
let tController = undefined;

if (controller instanceof Controller) {
tController = (controller as Controller);
}
promiseHandler(promise, tController, response, next);
...
function promiseHandler(promise: any, controller: Controller, response: any, next: any) {
return promise
.then((data: any) => {
let statusCode = undefined;
if (controller) {
statusCode = controller.getStatus();
}
if (data) {
response.json(data);
response.status(statusCode || 200);
} else {
response.status(statusCode || 204);
response.end();
}
})
.catch((error: any) => next(error));
}

My solution would be to let the generated promiseHandler() function ask the controller for status codes after retrieving a result from the promise.

@ryo-nbx
Copy link
Author

ryo-nbx commented May 8, 2017

I found a workaround that seem not to be documented. You can throw an Error in a Promise function. If the error contains a status member it will be used by the web API. With this discovery I could change my project to kinda this approach.

export class MyError extends Error {
status: number;
name: string;

constructor(code: number, name: string, msg: string) {
super(msg);
this.status = code;
this.name = name;
}
}

In a controller:
@Tags('Car')
@SuccessResponse('200', 'successful')
@Response('400', 'invalid input supplied')
@Response('404', 'not found')
@Put('{id}/type')
public async setCarType (@Path() id: string, @Body() type: CarType) : Promise<void> {
if (!type.id) {
this.setStatus(400);
return;
}
this.setStatus(200);
return data.setCarType(id, type.id);
}

In a data service class:

public async setCarType(id: string, type: string): Promise<void> {
let tmp: Car = null;
return this.getCar(id).then((car: Car): any => {
if (!car) {
throw new MyError(404, 'not_found', 'missing');
}
tmp = car;
// get Type
return this.getCarType(type);
}).then((tp: CarType): any => {
if (!tp) {
throw new MyError(404, 'not_found', 'missing');
}
tmp.type = tp;
return this.storeCar(tmp);
});
}

What is happening here? First the controller function checks the input parameter and if there is an invalid input value it sets the error code 400. Next, since all input values are valid, we set the error code 200 for success. If there is an error down the road the success state will be replaced with an error code.
In an asynchronous Promise function we throw a custom MyError with a status member. The API will catch the error and evaluate this status field and set the number value as response code. This works within my web service.

@kaspercools
Copy link

We are facing the same issue. It seems as if status codes are only applied upon the next reponse.

Throwing an error could be a solution to this problem when dealing with 4** codes but still doesn't provide a solution for object creation (201)

@isman-usoh
Copy link
Collaborator

express.js create error handle middleware

const statuses = require("statuses");

export function errorHandler() {
    return function (err: any, req: express.Request, res: express.Response, next: express.NextFunction) {
        let status = err.status || 500;
        if (status < 400) {
            status = 500;
        }

        const body: any = {
            statusCode: status,
        };


        // Internal server errors
        if (status >= 500) {
            console.error(err.stack);
            body.name = "ServerError";
            body.message = err.message || statuses[status];
            res.status(status).json(body);
            return;
        }

        // Client errors
        body.message = err.message || statuses[status];
        if (err.name) body.name = err.name;
        res.status(status).json(body);
    };
}

@jpramondon
Copy link

jpramondon commented May 19, 2017

We may have the same problem here. We're trying to make a controller to send 201 as a status when creation through a POST operation is successful.
Basically, what we're doing is :

@SuccessResponse('201') @Post() public create( @Body() vehicle: Vehicle): Promise<Vehicle> { ... this.setStatus(201); ... }

When logged through Morgan, Express shows a return status of 201, but whatever the client (cURL, Chrome, Postman), the status code stays 200 ...
Any help would be appreciated.

@ryo-nbx
Copy link
Author

ryo-nbx commented May 22, 2017

@jpramondon
Have you tried my workaround? Look at the example setCarType(). Just set your wanted success code (201) at the start of the controller method with setStatus(). This way the response for success will be used. After setting the success code you should check for validation or missing data and set a failure code. If you do some code within asynchronous code (Promise) then use the approach to throw an error with a status field as described above.

This is not a very elegant solution but a workaround until the issue is fixed.

@jpramondon
Copy link

Thank you for your reply @ryo-nbx. I think I'm going to test the latest version (1.2.0) first. There seem to be an interesting commit that we should try.
I agree on throwing an error to return 2xx statuses can only be a workaround and is bad by design.

@ryo-nbx
Copy link
Author

ryo-nbx commented May 22, 2017

@jpramondon
I would like to hear if the new version fixes this issue.

You don't need to throw a 2xx status with my solution. You just set it as default status before doing any further evaluation and processing. You only throw error status (4xx).

@jpramondon
Copy link

No problem @ryo-nbx.
I cloned the whole stuff, then packed it and installed locally.
It seems to do the job rather well now with the fix. I have been successfully sending 201 and 204 for my create and delete operations. I even issued a 999 status code.
Anyone knows when this 1.2.0 will be GA ?

@amozh-op
Copy link
Contributor

@isman-usoh This commit resolves part of the issue - it works if controller's method returns Promise which gets resolved immediately (like in your tests), but as soon as you add a delay for Promise resolving - it fails.
Example from your tests returns 201 as expected:
testController.ts

   @Get('customNomalStatusCode')
    public async customNomalStatusCode(): Promise<TestModel> {
        const that = this;
        const service = new ModelService();
        const promise = service.getModelPromise();

        return new Promise<TestModel>(resolve => {
            that.statusCode = 201;
            resolve(promise);
        });
    }

But next code will return status code 200:

    @Get('customNomalStatusCode')
    public async customNomalStatusCode(): Promise<TestModel> {
        const that = this;
        const service = new ModelService();
        const promise = service.getModelPromise();

        return new Promise<TestModel>(resolve => {
            setTimeout(() => {
                that.statusCode = 201;
                resolve(promise);
            }, 1000);
        });
    }

Or example from real life - doesn't work either, returns 200:

    @Post()
    public async createItem( @Body() a: CreateItemDto ): Promise<Item> {
        let that = this;
        return library.create(a).then(createdId => {
            that.statusCode = 201;
            return library.getItemById(createdId);
        });
    }

The only possible solution is to retrieve status code from Controller after Promise has been resolved, here is the possible way how promiseHandler method may be implemented:

    function promiseHandler(promise: any, getStatusCode: () => any, response: any, next: any) {
        return promise
            .then((data: any) => {
                let statusCode = getStatusCode();
                if (data) {
                    response.status(statusCode || 200).json(data);
                } else {
                    response.status(statusCode || 204).end();
                }
            })
            .catch((error: any) => next(error));
    }

And here is how express route template should be changed to make it work:

    app.{{method}}('{{../../basePath}}/{{../path}}{{path}}', 
            
            .........more code here.......

            const promise = controller.{{name}}.apply(controller, validatedArgs);

            function getStatusCode() {
              let statusCode: any;
              if (controller instanceof Controller) {
                statusCode = (controller as Controller).getStatus();
              }
              return statusCode
            }

            promiseHandler(promise, getStatusCode, response, next);
        });

I can create a PullRequest if you approve this.

@jpramondon
Copy link

I really didn't notice the behaviour you mention as I only ever used the fix either in a unit test or in a very minimal API, which apparently does not generate enough delay in the promise resolution.

@amozh, this really some good idea. The only effective way is indeed to resolve the status code after the Promise is completely terminated. I like the idea of accessing the status from inside the Promise resolution, based on an status accessor in the Controller.

This was referenced Jul 7, 2017
isman-usoh added a commit to isman-usoh/tsoa that referenced this issue Jul 20, 2017
- update route schema and update type
- change swagger enum inline type to enum definitions
- add new rule for tslint
- fix set status not work lukeautry#94
- support custom response header lukeautry#89
isman-usoh added a commit to isman-usoh/tsoa that referenced this issue Jul 21, 2017
- update route schema and update type
- change swagger enum inline type to enum definitions
- add new rule for tslint
- fix set status not work lukeautry#94
- support custom response header lukeautry#89
isman-usoh added a commit that referenced this issue Jul 27, 2017
- Support Query array type 
- Support custom response header (#89)
- Support tsoa config yaml format (#133)
- Support generate swagger yaml format
- Support string, number and boolan UnionType (#5)
- Support AnyKeyword (#114)
- update route schema and update type
- Change swagger enum inline type to enum definitions
- Add new rule for tslint
- Fix set status not work #94
- Fix validate 
- Set query array collectionFormat, default to multi
- Create file for Circle CI
isman-usoh added a commit that referenced this issue Jul 27, 2017
- Support Query array type
- Support custom response header (#89)
- Support tsoa config yaml format (#133)
- Support generate swagger yaml format
- Support string, number and boolan UnionType (#5)
- Support AnyKeyword (#114)
- update route schema and update type
- Change swagger enum inline type to enum definitions
- Add new rule for tslint
- Fix set status not work #94
- Fix validate
- Set query array collectionFormat, default to multi
- Create file for Circle CI
@dgreene1
Copy link
Collaborator

dgreene1 commented Aug 9, 2019

Since tsoa allows you access the request object, you explicitly set it the same way you would in express or koa:

In koa:

	@Get('{id}')
	public async getThingById(id: string, @Request() request: koa.Request): Promise<IThing> {
             const theThing = myDb.findById(id);
             if(!theThing){
                 request.ctx.status = 404;
                 request.ctx.body = { message: `could not find an item with id ${id}` };
                 return;
             }
             return theThing;
	}

But you'll notice that the first return will cause a TypeScript error since you aren't actually returning an IThing. So you have two options:

  1. return {} as unknown as IThing but since that completely subverts the type system, I'd recommend option 2.
  2. Throw an error that contains a statusCode and then use a middleware that catches it. I describe that in a separate ticket where I use Boom errors and then I demonstrate a middleware that catches it (I describe it here How can I catch errors by their status code? #382).

These solutions will work for you. In particular, I believe that the details in #382 will help you. I'm closing the ticket for house keeping purposes. If someone wants to add more information to the readme about this, that would be awesome. But for now, this issue should be easy to find by people.

@dgreene1 dgreene1 closed this as completed Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants