QueryParam() doesn't throw error, when trying to cast non numerical value into number

[raschan]

For example:
Get('/products') async list(@QueryParam('categoryId') categoryId: number)
categoryId will become NaN with ?categoryId=someString instead of throwing casting error

[MichalLytek]

Param type is only used to transform and validate if it's a class type. For arrays and primitive values there is no automatic validation. You should consider using @QueryParams with class-validator decorators.

[NoNameProvided]

Param type is only used to transform and validate if it's a class type.

Then shouldn't it left categoryId as it is? Instead it tries to cast someString to a number what is obviously will result in NaN.

[MichalLytek]

Casting is because query params are always a string in express/koa but they might be a number or boolean, like test-endpoint?param1=someString&param2=123&param3=true. But there was also an issue that multiple params (arrays) are not casted - test-endpoint?param=1&param=2&param=3 should have param: number[] but have string[].

[pleerock]

I think we can add error throw if validation is failed - no need to use class-validator for such simple use case. Ideally param: number[] must not be a string[] but we cannot understand using reflect-metadata that its a number array and not string array since it does not emit array type metadata.

[MichalLytek]

For me this case works as intended - dev expect number, client send string = you receive NaN because string is not a number. For other cases like casting "5" to boolean we should throw NormalizeError as it looks like the request is invalid.

I would like to do a PR with extended and better normalization than the one we have now - it was discussed already.

So to sum up:

• when boolean, we parse "true" and 1 to true, "false" and 0 to false, otherwise throw error
• when string, we do nothing (or undefined on empty string (result of parsing ?param)
• when number, we try to parse to number and throw an error if NaN
• for params like ?param we convert to true for boolean, "" for string and error for number
• we use @Type in declaration of class for @QueryParams to set the type of array to normalize them, and if single params has been send, we wrap it into array

Does it makes sense, @pleerock @NoNameProvided? 😉

[pleerock]

Sounds good. I want to comment only last proposal:

we use @type in declaration of class for @queryParams to set the type of array to normalize them, and if single params has been send, we wrap it into array

no need to introduce a new decorator, we can do @QueryParams({ type: X }) just like what we did in all other decorators(Body, QueryParam, BodyParam, etc.)

[NoNameProvided]

As I commented on the PR as well:

for params like ?param we convert to true for boolean, "" for string and error for number

?param this should be null if param exists in the query, undefined if param doesn't exists on the query

[pleerock]

Yes I agree there. Its too risky to automatically convert ?param to true`.

[MichalLytek]

no need to introduce a new decorator, we can do @queryParams({ type: X }) just like what we did in all other decorators(Body, QueryParam, BodyParam, etc.)

I am talking about this:

class ReuqestQuery {

    @IsPositive()
    limit: number;

    @IsPositive({ each: true })
    @Type(() => Number)
    cityId: number[];

}

Here we need type decorator because reflected type is generic Array which isn't enough to normalize it.
request?cityId=2&cityId=55&limit=10

[marshall007]

I really like the direction this discussion is headed. Just wanted to note that this is very similar to what was discussed in my original proposal #122, the key difference being that here only the query parameters are being taken into account. I'd suggest that we should in fact introduce a new decorator that allows you to populate parameters from anywhere in the request object (perhaps something like @RequestModel() or just @Request()).

For a given property the default lookup order could be req.params || req.body || req.query but we would also reuse the existing decorators to explicitly declare which request options are allowed to populate a given property.

export class GetAllUsers {
  // uses: req.params['q'] || req.body['q'] || req.query['q']
  @Request("q") // <- "catch all" property decorator
  search: string;

  // uses: req.params['customer'] || req.body['customer'] || req.query['customer']
  // (default behavior with no decorator is equivalent to using "catch all")
  customer: string;

  // uses: req.query['skip']
  @QueryParam("skip")
  from: number;

  // uses: req.query['limit']
  @QueryParam("limit")
  size: number;

  // uses: req.query['flags']
  @QueryParam()
  @Type(() => Number)
  flags: number[];
}

So, while I agree support for @QueryParams({ type: X }) should be added, I think we should go ahead and add a new decorator for the use case described above. For consistency with existing decorators, the new decorator would also support being used on properties (as demonstrated above) or method parameters (below).

@JsonController()
export class UserController {

  @Get("/users")
  getAll(
    // transform request object into class
    @Request({ type: GetAllUsers }) request: GetAllUsers,
    // pull `foo` field from anywhere in the request
    @Request('foo') data: string,
  ) {
      console.log(data === (req.params['foo'] || req.body['foo'] || req.query['foo'])) // -> true
      console.log(request instanceof GetAllUsers) // -> true
  }

}

[MichalLytek]

  // uses: req.params['q'] || req.body['q'] || req.query['q']
  @Request("q") // <- "catch all" property decorator

I think that this is really misleading for users.
The #122 proposal was much better but since we have @Params and @QueryParams which can validate easily, there so need for this kind of feature, so no rush with implementing it now.

The generic client idea is good but I think that we should go by the more flexible way which is generating classes code. The classes would have matching method name, so no need for client.send(new GetAllUsers({, just usersService.getAllUsers(options) that would convert the object to different ways of transporting data to rest api by http under the hood. And of course we need an inverse decorators function that instruct how to convert value to request so it would work also with 3rd-party decorators.

[marshall007]

I think that this is really misleading for users [...] since we have @Params and @QueryParams which can validate easily, there so need for this kind of feature, so no rush with implementing it now.

@19majkel94 I think support for extracting parameters from various request options is pretty important. If we can find a way to support multiple decorators on the same property I'd be fine with that, but I think it would pose challenges in certain cases. Consider:

export class GetAllUsers {
  @Param("q", { required: true })
  @QueryParam("q", { required: true })
  search: string;
}

The above scenario leads to some ambiguity as to how the parameters will be validated. We want the q request parameter to be required, while allowing it to be populated from either req.params.q or req.query.q.

export class GetAllUsers {
  @Request("q", {
    required: true,
    allow: [ ParamType.Param, ParamType.Query ]
  })
  search: string;
}

This allow parameter would specify both which param types to look at as well as the order in which to do so. As mentioned previously, this would probably default to [ ParamType.Param, ParamType.Body, ParamType.Query ]. Behind the scenes, all existing param decorators could be written as special case wrappers around the new @Request() decorator. This seems like a cleaner approach to me.

@QueryParam("q")
// equivalent to:
@Request("q", { allow: ParamType.Query })


@Param("id", { required: true })
// equivalent to:
@Request("id", { required: true, allow: ParamType.Param })