Params fixes and features

[MichalLytek]

This PR makes multiple changes:

• restore/introduce @QueryParams() and @Params() missing decorators options (they are needed for validation purposes)
• enhance params normalization - converting from string to primitive types is now more strict and can throw ParamNormalizationError, e.g. when number is expected but the invalid string (NaN) has been received
• normalize param object properties - only "queries", "headers", "params" and "cookies" - so you can easily validate query params and path params using class-validator

Closes #234, fixes #160.

NOTE:
Due to potentially breaking changes (all tests green of course) it has to be released as major release (0.8.x). Should we create the release branch and make some git-flow on this repo?

[MichalLytek]

As we introduce breaking changes, I would also propose renaming misleading @Param and @Params decorators to more meaningful @PathParam and @PathParams. It is really confusing as we have @BodyParam, @QueryParam, @HeaderParam, @CookieParam but the path param is simple @Param. For me it's not intuitive and new developers might think that @Param decorator is a generic one like @Request decorator described here.

@pleerock Can I do this also in this PR?

[NoNameProvided]

I agree! I wanted to bring this to attention as well! Green light from me. We can revert it later if @pleerock says otherwise.

[MichalLytek]

@NoNameProvided done in dd4f710 😉
Please review also the rest of the PR.

[NoNameProvided]

A typo + a minor change. Logic is LGTM so I approve. We can merge after pushing that two change.

[NoNameProvided]

.some is easier to read than .indexOf so please ["queries", "headers", "path-params", "cookies"].indexOf(param.type) !== -1 => ["queries", "headers", "path-params", "cookies"].some(type => type == param.type)

[NoNameProvided]

demand => demanded

[MichalLytek]

@NoNameProvided done in d7fe496. We should merge it to new major release branch to collect all pending breaking changes and to not block hot fixes.

[NoNameProvided]

Ok, so how about having a master what HEAD's always at the latest stable major release, having a develop branch what is always released as routing-controllers@next? Or do you have some better idea?

[MichalLytek]

Yes, we would merge it to development branch, and it can be released as routing-controllers@next pointing to the v0.8.0.beta-1 release 😉

Master would be then stable, all hotfix/bugfix land here (and merged to development from master), new breaking changes features to the development.

When we group together all breaking changes, we can merge it to master and release as 0.8.0 stable.

[MichalLytek]

@NoNameProvided
Merged into next branch 😃

Please take a look at the project board:
https://github.com/pleerock/routing-controllers/projects/3
We have to use it to keep track what is done, what is released and on which tag 😉

[pleerock]

rename @Param and @Params decorators to @PathParam and @PathParams

no. I understood you both want to make it consistent with other decorator names, but I don't find that its necessary to do so, if choice is between make it consistent and call "@PathParam" or make it inconsistent but call it "@param" then second choice absolutely wins because of its simplicity and everyone addiction. Everyone use .params (including express, angular, etc.) for router params and its already a accepted standard.

Also "PathParam" sounds odd and "path" does not tell me anything meaningful (path - associations with filesystem, not a route, better name would be a "@RouteParam", but Im not agree with it either.

Sorry, my bad for late respond here, but please be careful next time before merging such serious changes.

[pleerock]

in ParamNormalizationError I don't like Normalization word. It does not tell user so much, user may think - what is "normalization?". Normalization is more implementation-specific, what is going on in this code. I think it will be better user experience if we replace Normalization with at least Validation

[MichalLytek]

Normalization is a "process that makes something more normal or regular". However the better name would be transforming, because we transform string to numbers or boolean, however I don't know if it wouldn't collide with the error that "class-transformer" throws (if any?).

[pleerock]

Normalization is a "process that makes something more normal or regular".

right, thats why I called this method "normalize". But its routing-controllers internals, we need something more user-friendly for users since they will see this message. Same with tranformation. We shall tell users "dude you have a wrong parameter" instead of "dude transormation of your parameter failed" or "dude normalization of your parameter failed".

[MichalLytek]

It's just an internal name. Have you checked the source code of this error?

`Given parameter ${parameterName} is invalid. Value (${JSON.stringify(value)}) cannot be parsed into ${parameterType}.`

Isn't it a user-friendly message?

[pleerock]

just internal name

no, error name is used as well.

Isn't it a user-friendly message?

yes, it is. And we can clearly admin that class name is a short name of message inside it. And message inside it does not tell anything about normalization or transformation

[MichalLytek]

So you suggest that it should be InvalidParamError?

[MichalLytek]

please be careful next time before merging such serious changes

Don't worry, it's only merged into next branch 😉 We can rename it to @RouteParam but @PathParam is commonly used in Java world.

I understood you both want to make it consistent with other decorator names, but I don't find that its necessary to do so,

It has to be consistent and general @Param is confusing because about which param we are talking? We should provide abstraction over express/koa, no to stick with the original names of the req/ctx properties.

General @Param will be more usefull with proposal #291 which would be a multi-source param decorator, so the name makes more sense that coupling param name with path/route params.

[pleerock]

Don't worry, it's only merged into next branch 😉

yeah I know, but anyway you did redundant job as well.

We can rename it to @RouteParam but @PathParam is commonly used in Java world.

thats why Java sucks. It has too much redundancy.

It has to be consistent and general @param is confusing because about which param we are talking?

if we talk about params its about @Param, if we talk about query params, its a @QueryParam, if we talk about body params, its a @BodyParam, I don't find issues here.

We should provide abstraction over express/koa, no to stick with the original names of the req/ctx properties.

thats different. Im not talking "to name everything like in express because we are abstraction layer over express". Im talking to use @Param because its simply better then @PathParam or @RouteParam or anything else. Plus everyone uses params as router param in any framework so its kinda quite common practice and its really intuitive

General @param will be more usefull with proposal #291 which would be a multi-source param decorator, so the name makes more sense that coupling param name with path/route params.

my thoughts about #291 are already there

[NoNameProvided]

@pleerock I fail to see how you see simplicity in breaking a convention for one type.

If there is multiple types of xParam I will expect route params to be called @RouteParam. We have auto-completion so it doesn't change typing speed but for developer experience it does matter as we would like to provide a convenient/expectable naming.

if we talk about params its

Your statement just justify the PR, as a reader I need to ask what kind of param we talk about. Param can be any type you just decided to use it for route params.

We can rename it to @RouteParam but @PathParam

Yeah @RouteParam is better.

[pleerock]

you just decided to use it for route params.

hey, hey, not me! Its an accepted practice - just "params" are route params. "query params" are "query params". Veu, angular, react, express, koa any node.js and even some php frameworks I know use "params" as router params, and if there query params as well they are called "query params", and nobody confused. I don't know why you both are confused.

@19majkel94 please revert Param decorator name.

[MichalLytek]

Vue, angular, react,

Frontend frameworks are not a good example, as there's no body param, cookie param, header param or session param, only "path and query"... and even this not always - react-router there's no query params parse. Should we also strip of query feature to be equal to other frameworks and libs?
Also, e.g. in Angular 2+ we use this.route.params, so it makes no sense to name it redundantly this.route.routeParams - it doesn't sound good. So comparing to others makes no sense, we can only go backward instead of expand and grow up.

So there are only two solutions:

1. We specify the type of the param: @QueryParam, @SessionParam, @RouteParam
2. We strip of the param from other decorators: @Param, @Query, @Header

Naming inconsistency is very confusing, @Param related to route params is misleading for me, also for my coworker who recently started to make node.js backend.

[pleerock]

Angular 2+ we use this.route.params, so it makes no sense to name it redundantly this.route.routeParams - it doesn't sound good

same I think about change in routing-controllers. RouteParam does not sound good. Also, why in angular2 you aren't saying about this.route.params and this.route.queryParams ?! Noone really confused about this.

We strip of the param from other decorators: @param, @query, @Header

You can't since at least Header is busy for action decorator.

Naming inconsistency is very confusing, @param related to route params is misleading for me, also for my coworker who recently started to make node.js backend.

You missed third solution. Simply left as it is. I asked 3 my coworkers and they are prefer Param over RouteParam as well.

[MichalLytek]

same I think about change in routing-controllers. RouteParam does not sound good.

I can't believe 😆 What is the same, redundancy?

class ProductsController {

  @Get("/products/:categoryId")
  getProductsByCategory(
    @RouteParam("categoryId") categoryId: number,
    @QueryParam("limit") limit: number,
) {
    // ...
  }

}

Also, why in angular2 you aren't saying about this.route.params and this.route.queryParams ?! Noone really confused about this.

Because you have only 2 (two, t-w-o) param sources, so it's not as confusing like here, where we have 6 and maybe more.

You missed third solution. Simply left as it is. I asked 3 my coworkers and they are prefer Param over RouteParam as well.

So maybe let the users decide? Will you allow them to vote or you know better what they want because you have huge experience and know better what is good and what is bad?

[pleerock]

Because you have only 2 (two, t-w-o) param sources, so it's not as confusing like here, where we have 6 and maybe more.

actually only param and query param are very useful, others a bit specific and less common used. And yeah, even they use it they aren't confused because each of them has its own prefix.

So maybe let the users decide? Will you allow them to vote or you know better what they want because you have huge experience and know better what is good and what is bad?

okay lets do a democracy 😆 But we'll need enough people to vote (lets say at least 20-30 people). I'll buy as much people as I can to promote my idea 😈 . Make @Param Great Again!

Naming is always subject of disputes, especially for me who is picky to naming things.

[NoNameProvided]

+1 for RouteParam

[pleerock]

yeah I know there are two votes on this already 😆

[MichalLytek]

Ok, so how we should organize the voting? We should elaborate a mechanism that could be used also for koa support case or changes in middlewares support/api.

[IAMtheIAM]

I vote for RouteParam, just because it is a little more clear than simply Param, and because we are indeed referring to Route Parameters.

[github-actions]

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