Proposal: generic `@Param()` decorators

[marshall007]

This spawned out of the discussion following #234 (comment). Pending the removal of the existing @Param(s) decorators in #289. I would like to propose that we repurpose them as generic decorators that allow you to populate from arbitrary request options.

enum ParamType {
  Params = 'params',
  Query = 'query',
  Body = 'body',
  Header = 'headers',
  // ...
}

interface ParamOptions {
  required?: boolean = true,
  allow?: ParamType | ParamType[] | { [key: ParamType]: string | string[] | boolean },
}

function Param(options: ParamOptions);
function Params(type: Type, validate: boolean | ValidatorOptions = true)

Parameter Usage

@JsonController()
class ProductsController {

  @Get("/products")
  @Get("/categories/:categoryId/products")
  getProducts(
    @Param({
      required: false,
      allow: ParamType.Query,
    })
    tagId: number, // req.query.tagId

    @Param({
      // required: true,
      allow: [ ParamType.Path, ParamType.Query ],
    })
    categoryId: number, // req.params.categoryId || req.query.categoryId

    @Param({
      // required: true,
      allow: {
        [ParamType.Query]: [ 'api_key', 'apiKey' ],
        [ParamType.Header]: 'X-Auth-Api-Key',
      }
    })
    apiKey: string, // req.query.api_key || req.query.apiKey || req.header('x-auth-api-key')
  ) { /* ... */ }

}

Class-Based Usage

We can start by splitting out our authentication/session related parameters as it's likely that many services will require them:

class AuthParams {
  @Param({
    required: false,
    allow: ParamType.Session,
  })
  user: UserSession;

  @Param({
    allow: {
      [ParamType.Query]: [ 'api_key', 'apiKey' ],
      [ParamType.Header]: 'X-Auth-Api-Key',
    }
  })
  apiKey: string;
}

Next create a class to encapsulate your request definition (ideally these classes could support all class-validator decorators as well):

class GetProductsByCategory {
  @Param({
    required: false,
    allow: ParamType.Query,
  })
  tagId: number;

  @Param({
    allow: [ ParamType.Path, ParamType.Query ],
  })
  categoryId: number;

  @Params() // could support nesting?
  auth: AuthParams;
}

Now we can inject shared, class-based request definitions into our controller methods:

@JsonController()
class ProductsController {
  @Get("/products")
  @Get("/categories/:categoryId/products")
  getProducts(
    @Params(GetProductsByCategory) req: GetProductsByCategory,
    @Params(AuthParams) auth: AuthParams,
  ) {  }
}

[MichalLytek]

I am into Parameter Usage, however we should change the allow name to something more descriptive, as we can also pass [ParamType.Query]: [ 'api_key', 'apiKey' ] which describes how/from where to get the param. I would also rewrite the current @HeaderParam and other decorators to internally use new @Param decorator to simplify our internal handling logic.

However class based request definition are quite complicate and I think we shouldn't did this right now all together, just step by step after discussion about design that will allow to generate rest client code for frontent, based on controller classes with decorators.

[MichalLytek]

@NoNameProvided I would like to hear your opinion before I start to make the PR - what parts of the proposal you like, with which ones you disagree? 😉

[NoNameProvided]

I like the overall idea of this. Curious about how well this can be implemented currently. A few notes:

@param() vs @params()

The two name is too similar, while @Params() meaning would be quite different with this new implementation. I think this will cause some confusion. Does anyone have a better name for it?

[ParamType.Query]: [ 'api_key', 'apiKey' ]

I am kind of against allowing to receive params with multiple names. So instead of

allow: {
      [ParamType.Query]: [ 'api_key', 'apiKey' ],
      [ParamType.Header]: 'X-Auth-Api-Key',
    }

I would only use the

allow: {
      [ParamType.Query]: 'api_key',
      [ParamType.Header]: 'X-Auth-Api-Key',
    }

It's not because we cannot implement it, but because I don't see the reason why we should add this? I find this bad practice.

[MichalLytek]

The two name is too similar, while @params() meaning would be quite different with this new implementation. I think this will cause some confusion. Does anyone have a better name for it?

Accepting class with definied params should be called @Model or @ReqModel.

I will try to implement the multi-source @Param decorator, the model feature will be discussed later. However, I wonder if it wouldn't be more universal if we allow to pass the function which would extract the data from request? Like:

  @Param(req => req.query.api_key || req.query.apiKey || req.header['X-Auth-Api-Key'])
  apiKey: string;

Instead of:

  @Param({
    allow: {
      [ParamType.Query]: [ 'api_key', 'apiKey' ],
      [ParamType.Header]: 'X-Auth-Api-Key',
    }
  })
  apiKey: string;

It could be also @Param(params => params.query.api_key ... to be more universal across drivers.

[pleerock]

@marshall007 I really like what you suggested in your original proposal #122 . But I really don't like (just like others) what you suggested in #234 with @Request() and mixed parameters. If you do so in your project... well, its not a good practice I believe to do so, and I recommend you to rethink and change your approaches. But if you really want to do so - then simply create a new decorator in your own codebase, it shouldn't be so hard. Such functionality will not become a part of routing-controllers because its not generic. And in this issue you are prosing same, but in a bit different form. I don't think we need functionality prosed here as well.

[MichalLytek]

I don't think we need functionality prosed here as well.

Yes we need. Short example:

@JsonController()
class ProductsController {

    @Get("/products")
    @Get("/categories/:categoryId/products")
    getProductsByCategory(
        @Param("categoryId", { allow: [ParamType.Query, ParamType.Path] })
        categoryId: number,
    ) {
        return this.productsRepository.getAll({ categoryId });
    }
}

This method can be achieved with two canonical REST ways:

GET /products?categoryId=5
GET /categories/5/products

Without this kind of feature I could support only one way which without documentation might be misleading for api consumers. Or copy-paste the code and maintain two versions of the method in different resource controllers. Which is bad and I would really like to be able to do this simple things.

Such functionality will not become a part of routing-controllers because its not generic.

Generic? It supports all of ours params decorators. What we need more, custom decorators? It's supposed to work with params, not DB or communicating with microservices.

But if you really want to do so - then simply create a new decorator in your own codebase, it shouldn't be so hard.

Why we have to write everything by ourself? Maybe we should fork routing-controllers to make it better and more usable? 😆

[pleerock]

Yeah, do two separate actions if you have such edge-case scenario. Or you also can include @Param and @QueryParam and then make const categoryId = paramCategoryId || queryCategoryId;.

Its not a general scenario to do such controllers:

@Get("/products")
@Get("/categories/:categoryId/products")
getProductsByCategory

First, rare cases when you need two routes that does same thing. It should be avoided until you have no other choice. Second, even if you do so, I would recommend you to move second route into CategoryController or even CategoryProductController.

[MichalLytek]

First, rare cases when you need two routes that does same thing.

You don't know if it's rare. Paraphrasing you: "The fact that you don't do that doesn't mean that others don't need it" 😉

It should be avoided until you have no other choice.

Why it should be avoided? We have decorators to provide abstraction and be able to easily map path and params to our method, not to repeat our code.

It is useful, we have discovered the use cases and we have people that like this idea and want this feature. It doesn't force you to use this and it can be implemented as a built-in custom decorator, so no need for changes in the core codebase. It's a win-win, so no need to refrain from this feature.

[pleerock]

You don't know if it's rare.

No, I do know, I have quite a good experience in this area to know how to design a good and bad api.

Paraphrasing you: "The fact that you don't do that doesn't mean that others don't need it"

Paraphrasing myself: "Every rule has its own place" 😉

Why it should be avoided? We have decorators to provide abstraction and be able to easily map path and params to our method, not to repeat our code.
It is useful, we have discovered the use cases and we have people that like this idea and want this feature. It doesn't force you to use this and it can be implemented as a built-in custom decorator, so no need for changes in the core codebase. It's a win-win, so no need to refrain from this feature.

Proposal in previous author's issue proposes a bad practice. I don't see point in adding things that will lead to bad practices just to cover somebody specific use-cases. This proposal basically suggests same.