Controllers have state

[nenadalm]

If I want to set response code, I have to extend tsoa controller which have method setStatus. This requires controller to be instantiated on each request and leads to bugs if somebody forgets to do that (e.g. by specifying wrong scope in dependency injection container.

Wouldn't it be better if status and possibly other values were returned by the action so that controllers could be stateless? You could get inspiration from ring-clojure https://github.com/ring-clojure/ring/wiki/Concepts#responses.

So the response could look like:

interface Response<T> {
    status?: Number;
    body: T;
    headers?: {[key: string]: string};
}

?

[egandro]

You can have this with Dependency Injection / IoC.

In the Inversify example we have:

let provideSingleton = function(
  identifier: string | symbol | interfaces.Newable<any> | interfaces.Abstract<any>
) {
    return fluentProvider(identifier)
      .inSingletonScope()
      .done();
};

Add this:

import { Controller } from 'tsoa';
import { decorate, injectable } from 'inversify';
decorate(injectable(), Controller);

let provideTransient = function (
  identifier: string | symbol | interfaces.Newable<any> | interfaces.Abstract<any>
) {
  return fluentProvider(identifier)
    .inTransientScope()
    .done();
};

And decorate your controller this way:

@Route('foo')
@provideTransient (FooController)
export class FooController extends Controller {
  ...
}

Every Controller get's created per call. Please note - because of current limitations of tsoa you have to decorate all controllers.

[nenadalm]

@egandro sure, but my proposal is to make controllers stateless.

Currently it works something like:

class Controller extends TsoaController {
    public action() {
        this.setStatus(200); // storing status in magical place

        return {
            name: 'John Doe',
            count: 5,
        };
    }
}

const controller = new Controller();
const body = controller->action();
const status = controller.getStatus(); // retrieving status from magical place
response.status(status).send(JSON.stringify(body));

I propose to make it work like:

class Controller extends TsoaController {
    public action() {
        return {
            status: 200,
            body: {
                name: 'John Doe',
                count: 5,
            },
        };
    }
}

const controller = new Controller();
const response = controller->action();
response.status(response.status).send(JSON.stringify(response.body));

Advantage is that action returns required data for response (instead of retrieving part of them and storing other part in some magical place). So as for me this is more clear + it's less error prone (see my previous post).

If you don't like my idea, feel free to close the ticket. Reason I created it is not that I couldn't make it work, reason is that I think it could work in better way.

(as a side note, #94 is about wrong status code in some cases. As status code would be part of the response here, there would be no way you could have such a bug and reading status before resolving the promise...).

[egandro]

Ok I understand what you like.

A sort of Return Type (derived from an interface). If this interface contains status and body it will raw-pass this to the result.

I like this approach.

We also need this the Respond() Attribute is used on the method and this differs from the Return value.

Question. Would it be ok for you to have it in this way?

@Response('200', 'My Info Text', MyResultType)
@Response('400', 'My Error Text', MyOtherResultType)
@DefaultResponse<ErrorResponse>('Unexpected error')
@Get('Response')
public async getResponse(): Promise<TsoaResponse> {
         const resultType: MyResultType = {
                name: 'John Doe',
                count: 5,
         });

         return Promise<TsoaResponse>.resolve( {
            status: 200,
            body: resultType,
           // message: 'Overwrite here - or get the default'
        });
}

[nenadalm]

yes, that looks awesome.

[egandro]

Let's discuss 2 more things.

1. I am not so happy with that TsoaResponse interface. That might create some other issues or limits. Let's collect some more opinions.

2. I'd really like to have one Default Error class for every routes and for all exceptions. This is something swagger does not support by specification, but we might be able to emulate this with a global "DefaultResponse" - but this forces every result type to be part of TsoaResonse.

Any ideas on this?

[nenadalm]

1. I am not so happy with that TsoaResponse interface. That might create some other issues or limits. Let's collect some more opinions.

Do you have some concrete examples in mind? Status and body are required for each response probably, if user wants to have other fields (like headers, ...) they can create their own compatible interface and override the template generating the handler code.

Extending of the handlebar template could be made easier probably. I am not familiar with handlebars, but there seems to be way to define blocks https://www.npmjs.com/package/handlebars-extend-block so instead of copy pasting the whole template and changing some parts, template could be split into blocks and user could then extend the base template and override only code handling the response (so on updates, they would have latest changes in their template except the block they changed ofc.
Another approach to make extending easier would be trying to get rid of the handlebar template (which shouldn't be solved as part of this issue probably).

2. I'd really like to have one Default Error class for every routes and for all exceptions. This is something swagger does not support by specification, but we might be able to emulate this with a global "DefaultResponse" - but this forces every result type to be part of TsoaResonse.

That would be nice. I guess the code could look something like:

// -- code in tsoa --
interface TsoaResponse<T> {
    status: number;
    body: T;
}

// somewhere in the handler
new Controller()
    .action()
    .then((res: TsoaResponse<any>) => // let's make sure that user returns compatible interface
        response.status(res.status).send(JSON.stringify(res.body))
    );





// -- user code --
@DefaultGlobalErrorResponse('400')
interface ErrorResponse {
    status?: string;
    code?: string;
    title?: string;
}

/**
 * This interface is created by user so that he doesn't have to type
 * TsoaResponse<ErrorResponse | UserResponse> each time.
 */
interface CustomTsoaResponse<T> {
    status: number;
    body: T | ErrorResponse;
}

interface UserResponse {
    name: string;
    count: number;
}

let successResponse: CustomTsoaResponse<UserResponse> = {
    status: 200,
    body: {
        name: 'John Doe',
        count: 5,
    },
};

let errorResponse: CustomTsoaResponse<ErrorResponse> = {
    status: 400,
    body: {
        title: 'Bad request',
    },
};

class Controller extends TsoaController {
    public async action(): Promise<CustomTsoaResponse<UserResponse>> {
        return res.isSuccess() ? successResponse : errorResponse;
    }
}

?

[egandro]

Hello Nenadalem,

I had the same Idea as you while taking a shower :)

Extending the Controller with a default Error reporter.

This solves the issue with the Base Interface for the Result.

Nice one!

We have to figure our a way to tell that the swagger file, as I am working with Client Code Generators that are swagger based.

From point of view there is no swagger specification for the default error.

We might (!) add this to the global Description Field in the swagger File.

%%DefaultErrorClass%% NamespaceOfIt.DefaultErrorClass

Let's carefully think before implementing this ;)

[nenadalm]

As I see my example now

class Controller extends TsoaController

should be just

class Controller

(without extending the TsoaController as it is no longer needed - I forgot to remove it when copy pasting)

From point of view there is no swagger specification for the default error.

As I was thinking about it - I thought it would be added into each response:

{
    "paths": {
        "$path": {
            "$method": {
                "responses": {
                    "400": {
                        "description": "Bad request",
                        "schema": {
                            "$ref": "#/definitions/ErrorResponse"
                        }
                    }
                }
            }
        }
    }
}

where $path and $method stands for placeholders. So it would work the same way as if people put @Response<ErrorResponse>('400', 'Bad request') on each action.

Putting @Response<ErrorResponse>('400', 'Bad request') on each action is what I do for my current project as all responses have the same structure - description varies though sometimes, but in worst case I can override it in tsoa.json (alternative would be that if I decorate action with same status code as the global one, local one is used instead. So I would have the global decorator @DefaultGlobalErrorResponse('400') which would be used for all actions where I didn't specify same error code via @Response<ErrorResponse>('400', 'Bad request') (in which case latter would be used).

@DefaultGlobalErrorResponse('400', 'Bad request')
interface ErrorResponse {
    status?: string;
    code?: string;
    title?: string;
}

class Controller {
    public async action(): Promise<CustomTsoaResponse<UserResponse>> {
        // ...
    }

    @Response<ErrorResponse>('400', 'Overriden message')
    public async action2(): Promise<CustomTsoaResponse<UserResponse>> {
        // ...
    }
}

would then generate responses for actions

• action (action for which we didn't specify 400 error response)
  • code: 400, description: 'Bad request'
• action2 (action for which we specified 400 error response despite existing global one)
  • code: 400, description: 'Overriden message'

[egandro]

1. Nice one

What about

Promise<CustomTsoaResponse> can also become

TsoaPromise<UserResponse>

2. What about throws?

e.g. throw new Error("the error")

we might need some

throw new ErrorResponse( 400, "my message");

[nenadalm]

Promise<CustomTsoaResponse> can also become TsoaPromise<UserResponse>

I guess that TsoaPromise has nothing to do with Promise except that it works in similar way (you don't really have the value now, but you'll get it later through the object.

In which case you need to override the handler

tsoa/src/routeGeneration/templates/express.ts

Line 91 in d03c94f

function promiseHandler(promise: any, statusCode: any, response: any, next: any) {

So in case current handler would look like (taken from my previous example):

// somewhere in the handler
new Controller()
    .action()
    .then((res: TsoaResponse<any>) => // let's make sure that user returns compatible interface
        response.status(res.status).send(JSON.stringify(res.body))
    );

Your custom handler for TsoaPromise could look like:

// somewhere in the handler
new Controller()
    .action()
    .customThen((res: TsoaResponse<any>) => // let's make sure that user returns compatible interface
        response.status(res.status).send(JSON.stringify(res.body))
    );

(or whatever else - notice the customThen method which is equivalent to then in standard Promise)

see #130 (comment) (Extending of the handlebar template... paragraph) on some very initial idea on how extending of the templates could be made easier (as it's not really related to the controllers - new ticket could be created if change there is desired - so that we don't mix too much stuff here).

As for the UserResponse instead of TsoaResponse - you need to return object compatible with interface TsoaResponse - UserResponse is not compatible. See the 2 below:

interface TsoaResponse<T> {
    status: number;
    body: T;
}

interface UserResponse {
    name: string;
    count: number;
}

You need to return it in following shape: TsoaPromise<TsoaResponse<UserResponse>> (probably I named UserResponse badly - but it's not really response - it is body of the response. Having just one single interface for response object (where keys can be optional if desired) is I think better than allowing multiple interfaces and thinking about how to deal with that (both in handler and generation of swagger). By this single interface - I mean the TsoaResponse not the Promise which can be whatever probably (as long as it has one type argument compatible with TsoaResponse.

What about throws?

What about them? In my current project, I am not throwing uncaught errors in tsoa controllers (not intentionally at least). I am converting the into ErrorResponse and return is as js object (same way as I would return successful response).

I am using express library with tsoa. It means that If I want to handle errors, I can do something like:

import {RegisterRoutes} from 'pathToTheDir/routes';
import * as express from 'express';

// generated router
export const apiRouter = express.Router();

// registering routes to the apiRouter
RegisterRoutes(apiRouter);

// custom middleware to handle errors extending base error
// which I use for showing errors
apiRouter.use((err, req, res, next) => {
    if (err instanceof ApiError) {
        res.status(err.status).send(err.message);
    } else {
        next(); // we don't know what to do with the error - let's do nothing about it then, and leave it to next error handler
    }
});

and then I can register my router in the index file or where I created the express app:

import {apiRouter} from './router/api';
import * as express from 'express';

const app = express();
app.use('/api/v1', apiRouter); // now let's use the previously created router for api

// ...

so as for me there is no need to add additional code to handle errors somehow (anybody can do that easilly using middleware or whatever else their routing library is using). In your case, the error would be object extending ApiError (which you created as ancestor of all errors handled by the middleware I showed you). So it would be handled by the middleware. (This could be added into docs if it is some common usecase which should be highlighted).

[nenadalm]

I'll try to look at it and send PR.

[egandro]

Yes also agree with that - I have this middleware errorhandler ;)

Let me print this page when I arrive in my office.

Let's make a branch and try this!

Do you have Write Access to this Repo?

[nenadalm]

@egandro I don't have write access.

[egandro]

I'll ask Luke.

[egandro]

Done but it might take a day or two.

[nenadalm]

@egandro please review: #131

[egandro]

Luke just gave you write access. Lets make a branch for this.

[nenadalm]

thanks. I've created new pr #132 and closed old one.

[lukeautry]

Looks like this was resolved with the linked PR - if this is still an issue let me know.