routing-controllers icon indicating copy to clipboard operation
routing-controllers copied to clipboard

Request: support class-based request definitions

Open marshall007 opened this issue 7 years ago • 14 comments

It would be nice to be able to declare the structure of your requests in an external class. Doing so would also open up the possibility of providing client-side tooling for building requests based on shared class definitions.

Example

import {JsonController, Param, QueryParam, Get} from "routing-controllers";

export class GetAllUsersRequest {
    @QueryParam("search")
    search: string;
    @QueryParam("skip")
    skip: number;
    @QueryParam("limit")
    limit: number;
}

export class GetUserRequest {
    @Param("id")
    id: number;
}

@JsonController()
export class UserController {

    @Get("/users")
    getAll(req: GetAllUsersRequest) {
       return userRepository.findAll(req);
    }

    @Get("/users/:id")
    getOne(req: GetUserRequest) {
       return userRepository.findById(req.id);
    }

}

marshall007 avatar Apr 26 '17 15:04 marshall007

As a side-note, it would also be nice if the name parameter to the various Param decorators was optional and defaulted to the property name (both in the class example above, but also function parameters). I'm pretty sure that parameter names are exposed in the TS reflection metadata, but I could be wrong.

marshall007 avatar Apr 26 '17 16:04 marshall007

sounds as a very good feature request

pleerock avatar Apr 26 '17 16:04 pleerock

It would be nice to be able to declare the structure of your requests in an external class.

Can you show more advanced example which takes full benefit of class-based requests? Because if it's about readability, currently I can do this:

import {JsonController, Param, QueryParam, Get} from "routing-controllers";

@JsonController()
export class UserController {

    @Get("/users")
    getAll(
        @QueryParam("search")
        search: string,
        @QueryParam("skip")
        skip: number,
        @QueryParam("limit")
        limit: number,
    ) {
       return userRepository.findAll({ ...arguments });
    }

    @Get("/users/:id")
    getOne(
        @Param("id")
        id: number,
    ) {
       return userRepository.findById(id);
    }

}

Only I can see is the usage with class-validator - we could check e.g. if id is a positive int.

parameter names are exposed in the TS reflection metadata, but I could be wrong

Nope, currently method parameters names are not reflected by TypeScript. But class properties are - see how class-based entities works in TypeORM. So it's the second gain from this feature :wink:

MichalLytek avatar Apr 27 '17 20:04 MichalLytek

@19majkel94 yea it's not so much about syntax or readability, though I think there is some benefit there. As you mentioned it also plays nicely into class-validator and being able to provide much more robust request validation. For me, it's mainly about the possibility of (1) having shared request definitions between the client and server and (2) auto-generating documentation.

@Get('/:customer/users')
@Returns({ type: User, array: true })
export class GetAllUsers {
  @Param()
  customer: string;

  @QueryParam()
  search: string;

  @QueryParam("skip")
  from: number;
  @QueryParam("limit")
  size: number;

  @QueryParam()
  @IsArray()
  @IsNumber({ each: true })
  flags: number[];
}

Take the above example and assume it's exposed in a shared library between client and server. Using the metadata emitted by the TS compiler, it would be possible to write a generic REST client implementation that could interact with any routing-controllers based server. It could look something like:

const client = new RoutingClient();

client.send(new GetAllUsers({
  customer: 'my-company',
  search: 'query',
  size: 50,
  flags: [1,2]
}))
.then(users: User[] => {
  // -> GET /my-company/users?search=query&size=50&flags=1&flags=2

  // client automatically:
  // - knew how to serialize request parameters
  // - validated request params pre-fetch
  // - built appropriate request URL
  // - knew how to interpret server response
  // - deserialized response into appropriate type
})

I'm making a lot of assumptions in this example, but I hope it gets my point across that there's a lot of powerful things we could do with the metadata from class-based request definitions.

marshall007 avatar Apr 27 '17 22:04 marshall007

right this feature allow to setup communication easily. Sending all params as separate query params is a big pain. @marshall007 what I do in my apps I usually have lets say /questions/ and object called QuestionFilter:

export class QuestionFilter {

         limit: number;
         offset: number;
         sort: "name"|"date";
         /// .... other properties, methods if needed
}

and then in the controller

@Get("/questions")
all(@QueryParam("filter") filter: QuestionFilter) { // filter is automatically casted to QuestionFilter
     /// ...
}

and the request is: /questions/?filter={"limit":1,"offset":0,"sort":"name"} where query param is just JSON.stringify of question filter object used on the frontend.

pleerock avatar Apr 28 '17 04:04 pleerock

I'm making a lot of assumptions in this example

That's the point. All looks beautiful with simple&clean examples covering 80% of cases. But the problem is with more complicated examples like with authorization:

@Get('/:customer/users')
@Returns({ type: User, array: true })
export class GetAllUsers {
    @JWT()
    customer: User;

    @QueryParam()
    search: string;

    @Req()
    req: Request;
}

Request object from the viewpoint of controller should have a authenticated user object extracted from jwt from header. So if you want to reuse class definition, you would have to explicitly pass the object in client.send(new GetAllUsers({ which is not convinent. The same problem would be with @Req and @Res (as we discussed in PR #119) - on the client side you would have to pass dummy object as any? It makes no sense.

So you can't reuse definition as it is, you have to parse it an generate ClientRequest definition based on backend request model. E.g. when see @JWT() in backend request, you attach the token string to header under the hood. So you can do the parsing on object used as type of request param of action or just as list of params of controllers action, it makes no difference.

being able to provide much more robust request validation

As @pleerock showed, we can easily validate query params with class-validator, with @Params() we can do the same with route params, body is already easy to validate, so what other use-cases are left? What decorators return only single value that can't be class-validated? Cookie and headers are also ready to class-define and validate.

I'm not saying that this feature is bad - it looks good but all use cases can be done now nearly as easy, so this might just go to nice-to-have features list as now they are more important things to do. The case of generic Routing-client plugin/extension is great and we should discuss it in an other issue as I have my thoughts and ideas too 😉

MichalLytek avatar May 01 '17 11:05 MichalLytek

The same problem would be with @Req and @Res - on the client side you would have to pass dummy object as any? It makes no sense.

@19majkel94 that's probably not how I would model those particular scenarios. Things like authentication are generally handled at a higher level and shouldn't really be modeled at the individual request level anyway. There's no reason you can't still inject additional parameters in your controller method the traditional way:

export class GetUserRequest {
    @Param("id")
    id: number;
}

@JsonController()
export class UserController {

    @Get("/users/:id")
    getOne(@Model() req: GetUserRequest, @JWT() jwt: Token, @Req() request: Request) {
       return userRepository.findById(req.id);
    }

}

marshall007 avatar May 01 '17 16:05 marshall007

I know that I can inject additional parameters. But with things like @JWT (or @Range ) you need to know that RoutingClient should inject token string to the Authorization header. So if it's not declared in @Model you need to parse the action params and generate code to handle it under the hood.

So basically @Model wouldn't help as much as you think, as it only combines route params, query params, header params and body in one single object, so it's so much about syntax or readability. Validating can be achieved without this feature and you can get rid of model or req param and the long syntax req.param.id instead of simple id in basic action or param.id in this sample (dummy example):

export class ReqRouteParams {
    @IsMongoId()
    userId: string;

    @IsMongoId()
    postId: string;
}

export class ReqQueryParams {
    @IsInt()
    @IsPositive()
    limit: number;

    @IsInt()
    @IsPositive()
    offset: number;
}

@JsonController()
export class SampleController {

    @Get("/users/:userId/posts/:postId/coments")
    async action(
        @Params() param: ReqRouteParams,
        @QueryParams() query: ReqQueryParams,
        @JWT() jwt: Token,
        @Req() request: Request,
    ) {
       return commentsRepository.findByPost({ id: param.postId }, query);
    }

}

But let's close this discussion for now - it's not the case of this issue, we will discuss is later in TypeStack as it would be a part of this framework and require more integration with routing-controller (even as a plugin).

MichalLytek avatar May 02 '17 17:05 MichalLytek

@19majkel94 is right that its just a nice-to-have feature and you can do same almost same way. But I really like this feature because its a bit more elegant and beauty, and for those who have lot of params in request and don't prefer to use filter pattern as I showed, it can be a useful feature. I think it can be added to routing-controllers.

pleerock avatar May 03 '17 05:05 pleerock

who don't prefer to use filter pattern as I showed

We have @QueryParams() decorators which can work with normal query params and return and object that can be validated 😉 I agree that it's a nice feature but very low prio I think. Closing & adding to the ToDo list?

MichalLytek avatar May 03 '17 12:05 MichalLytek

nope, lets left it open and wait if someone want to contribute, maybe even @marshall007

pleerock avatar May 03 '17 12:05 pleerock

I think the OP's request is a good idea, but I think it would be even better (and more flexible) to implement reusable and combinable @ParamGroups:

export class FilterParams {
    @QueryParam("genre")
    genre: string;

    @QueryParam("year")
    year: number;
}

export class PaginationParams {
    @QueryParam("pageNumber")
    pageNumber: number;

    @QueryParam("pageSize")
    pageSize: number;
}
export class MovieController {
    @Get("/movies")
    listMovies(
        @ParamGroup() filters: FilterParams,
        @ParamGroup() pagination: PaginationParams,
    ) {
        // Select with filter and pagination here 
    }
}

This way, filter params could be reused across endpoints where they make sense, and pagination params could also be reused separately across many other endpoints that use different filters.

Although the above code example doesn't show it, a @ParamGroup could also contain any combination of @Param, @QueryParam, @UploadedFile, and other such params. (If we want these sets to also include headers, body, or other non-param attributes of a request, then perhaps @ParamGroup should be named @ReqData or @ReqSubset instead.)

svicalifornia avatar Feb 18 '18 23:02 svicalifornia

Query params as class can be usable, when all get params are inherited from some shared model or for example when defining fields swagger. I've got such case now. Maybe I could help with PR? We just need to discuss how it should work finally.

michalzubkowicz avatar Aug 20 '18 10:08 michalzubkowicz

Stale issue message

github-actions[bot] avatar Feb 18 '20 00:02 github-actions[bot]