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

[MichalLytek]

@Request is also a misleading name as someone may use it as a express request object:

import { Request } from "routing-controllers";
import { Request } from "express";
//...
method(@Request() request: Request) {
   const user = request.user;
   //...
}

And I think that using the same name for query/body/route parameter is an awful idea. I don't know who does it and in what use cases it might help.

[marshall007]

@19majkel94 totally open to suggestions on the name, I agree @Request() is ambiguous.

And I think that using the same name for query/body/route parameter is an awful idea. I don't know who does it and in what use cases it might help.

We do this a lot, actually. Examples include:

• search endpoints that respond to GET requests with query parameters and POST requests with more complex, but overlapping, filtering options in the body
• GET requests where path and query params are interchangable
  • i.e. GET /products/:foo/:bar and GET /products?foo=...&bar=...
• PATCH, PUT, and POST endpoints where the document id can either be a route param or in the request body
• token based auth schemes where your "API key" can be included as a header, query param, etc

That said, I agree using the exact same name for various param types is probably the less common case, so I think the signature should be something like this instead:

@Request({
  required: boolean,
  allow: ParamType | ParamType[] | { [key: ParamType]: string | string[] | boolean }
})

class {
  @Request({
    required: true,
    allow: ParamType.Query
  })
  public apiKey: string; // req.query.apiKey

  @Request({
    required: true,
    allow: {
      [ParamType.Query]: true
    }
  })
  public apiKey: string; // req.query.apiKey (same as previous)

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

[NoNameProvided]

We do this a lot, actually. Examples include:

I would call that a bad design, everything should have one source of truth. Encouraging to mix things up is not a good idea in my opinion.

[marshall007]

I would call that a bad design, everything should have one source of truth. Encouraging to mix things up is not a good idea in my opinion.

A better example than the ones I originally provided is when you need to support both nested and top-level routes that map to the same request:

• GET /foos/:foo_id/bars
• GET /bars?foo_id=...

@NoNameProvided as mentioned above, I agree that this is not the common case. I think the modified proposal above (#234 (comment)) better encapsulates all use cases by requiring you to specify the allowed name(s) for each param type. By removing the optional "name" as the first parameter, it no longer "encourages mixing things up", but does still support that use case when needed.

[MichalLytek]

totally open to suggestions on the name, I agree @request() is ambiguous.

With #289 the @Param decorator might be reused.

So your use case is that you have resource (e.g. category) and some other resource (e.g. product) has a many-to-one relation with this category. So your rest api support two ways of fetching data:

GET /category/5/products
GET /products?categoryId=5

where both endpoints return the list of products of category with id 5.

And you create controllers not per path but per feature, so you would like to achieve:

@JsonController()
class ProductsController {

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

I think it's a good use case that allow us to reuse controllers actions. @marshall007 go ahead and create a proposal in new issue where we could discuss details.

[MichalLytek]

Closing this via #289. Will be released in 0.8.0-beta.1 - routing-controllers@next, stay tuned! 😉

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