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

Request: support class-based request definitions #122

Open
marshall007 opened this issue Apr 26, 2017 · 14 comments
Open

Request: support class-based request definitions #122

marshall007 opened this issue Apr 26, 2017 · 14 comments
Labels
status: awaiting answer Awaiting answer from the author of issue or PR. type: feature Issues related to new features.

Comments

@marshall007
Copy link

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
Copy link
Author

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.

@pleerock
Copy link
Contributor

sounds as a very good feature request

@pleerock pleerock added the type: feature Issues related to new features. label Apr 26, 2017
@marshall007 marshall007 mentioned this issue Apr 27, 2017
@MichalLytek
Copy link
Contributor

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 😉

@marshall007
Copy link
Author

marshall007 commented Apr 27, 2017

@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.

@pleerock
Copy link
Contributor

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.

@MichalLytek
Copy link
Contributor

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 😉

@marshall007
Copy link
Author

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);
    }

}

@MichalLytek
Copy link
Contributor

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).

@pleerock
Copy link
Contributor

pleerock commented May 3, 2017

@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.

@MichalLytek
Copy link
Contributor

MichalLytek commented May 3, 2017

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?

@pleerock
Copy link
Contributor

pleerock commented May 3, 2017

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

@svicalifornia
Copy link

svicalifornia commented Feb 18, 2018

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.)

@michalzubkowicz
Copy link

michalzubkowicz commented Aug 20, 2018

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.

@github-actions
Copy link

Stale issue message

@github-actions github-actions bot added the status: awaiting answer Awaiting answer from the author of issue or PR. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting answer Awaiting answer from the author of issue or PR. type: feature Issues related to new features.
Development

No branches or pull requests

6 participants