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.

[MichalLytek]

@marshall007 @NoNameProvided
We won't confute pleerock, so I will do this as an extension package and I will link to this at the bottom of the readme here (extensions section) if you don't mind, @pleerock 😉

[marshall007]

@pleerock just to add my two cents in case in case it sways you... as @19majkel94 mentioned, there are cases where two different REST endpoints map to the same entity. In cases like this, you want to be able to validate, document, and (hopefully) execute using the same code paths to fulfill that request. Allowing the user to deserialize various request options into the same DTO is what makes this possible.

I know you're a fan of my initial proposal in #122. My argument would be that without the ability to reference multiple request options for a given class property, the value of #122 is greatly diminished. In the particular case mentioned by @19majkel94:

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

... I would actually prefer to have a single GetProductsByCategory DTO class encapsulating all the request options (and deserialization logic) and make that a parameter to ProductsController.getAll() and CategoryController.getProducts() (thus exposing the "same request" under two different controller methods). This class of use-cases would be impossible without mixed parameter support.

---

The other compelling use-case in my mind is API backwards-compatibility. In real production systems parameter names change, endpoints become deprecated in favor of new ones, etc. Supporting mixed-parameter and multiple possible names for each parameter, we give the developer a way to encapsulate this complexity and maintain backwards-compatibility.

In the future, once #122 is fully supported, these classes would become a great way to map deprecated request options to their newer equivalent. Just by looking at the class definition, you will easily be able to see how request options are being mapped. Moreover, if your class-based request definitions are shared across multiple endpoints, you don't have to go tracking down references to a param you want to deprecate. You just update that one class property and/or its decorator.

[pleerock]

Im very sorry but it did not convince me. You can do everything you wrote same easily currently and you don't need extra decorator/decorator functionality, just do what you need in the code. Im going to close this issue, we may reopen it only if it will get much more attention from other people, time will show us.

[MichalLytek]

@marshall007 @NoNameProvided
I've created the initial version:
https://github.com/19majkel94/routing-controllers-multiparam

For now the param normalization doesn't work until 0.8.x release (changes are on next branch), as well as transforming/validation (this is not a common use case I think).

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.