Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically detect optional @Query #30

Open
Tracked by #94
mjgp2 opened this issue Jan 9, 2018 · 31 comments
Open
Tracked by #94

Automatically detect optional @Query #30

mjgp2 opened this issue Jan 9, 2018 · 31 comments

Comments

@mjgp2
Copy link

mjgp2 commented Jan 9, 2018

Am I right in thinking that you could look for a ? on the query function parameter to denote that it is optional, and pick this up to set the required value on the swagger document?

It would be nicer than having to add decorators like @ApiImplicitQuery({name:'offset',required:false}) to the method I think. What do you reckon?

Thanks :)

@alisherks
Copy link
Contributor

One option will be to use a class as your query parameters:

class MyParams {
  @ApiModelProperty()
  prop: number;
  @ApiModelPropertyOptional()
  optionalProp: number;
}

@Controller('MyCtrl')
class MyCtrl {
  @Get()
  async getList(@Query() params: MyParams) {
    ...
  }
}

@mjgp2
Copy link
Author

mjgp2 commented Jan 9, 2018

Sure, good idea, but I would still rather have something where the API properties can be inferred by reflection rather than more decorators as it becomes much more verbose 👍

@jaufgang
Copy link

Keep in mind that a param could also be optional if it has a default value rather than a ?. For example:

@Controller("transactions")
export class TransactionsController {

	@ApiImplicitQuery({
		name: "limit",
		description: "The maximum number of transactions to return",
		required: false,
		type: Number
	})
	@Get("recent")
	getRecentTransactions(@Query("limit", new ToIntPipe()) limit: Number = 10) {
		...
	}
}

It would be ideal if swagger could detect the default value using reflection and document that the param is optional as well as the default values.

Also, note that in this example the param type is explicitly declared as Number, but this is also not reflected in the swagger docs if we omit the @ApiImplicitQuery({type: Number}). In fact, the try it out form field will currently not accept a number without the type specified in the ApiImplicitQuery decorator.

It would be ideal if all of these things could be inferred by reflection so that you would only need to use the @ApiImplicitQuery decorator to add the description.

@cristiboariu
Copy link

@jaufgang + 1

@kvgros
Copy link

kvgros commented Jun 12, 2018

+1

@kamilmysliwiec
Copy link
Member

Reflection doesn't give us any information about optionals/default values. We'd have to use a new script that makes use of AST to properly indicate whether something is required or not.

@kunokdev
Copy link

So how do we define optional param?

@jaufgang
Copy link

@kunokdev, you specify an optional param by setting required: false in the @ApiImplicitQuery decorator. (see my sample code above)

@kamilmysliwiec
Copy link
Member

The plugin released in version 4.0.0 will automatically detect optional properties in your class https://docs.nestjs.com/recipes/swagger. However, it still doesn't check controller's methods signature (something to add in the future).

@prinzdezibel
Copy link

prinzdezibel commented Apr 23, 2020

[..]
getRecentTransactions(@Query("limit", new ToIntPipe()) limit: Number = 10) {
[..]

You mentioned the custom pipe ToIntPipe. Does this mean the Nest.js framework does not come up with a out-of-the-box solution for this trivial example (an optional query parameter)?

While it wouldn't be a big deal to write a custom pipe that doesn't throw (as opposed to ParseIntPipe), I'm inclined to think that I'm missing something here. Any thoughts, @jaufgang ?

@mareksuscak
Copy link

@prinzdezibel and everyone else looking for a solution — there is a documented way to chain multiple pipes where one of them can inject a default value. The DefaultValue pipe must precede ParseIntType or whatever other pipe you're using. Here's a direct link to the section that documents this: https://docs.nestjs.com/pipes#providing-defaults

But it's true that it took me a while to find it.

@nartc
Copy link
Contributor

nartc commented Nov 14, 2020

Thanks @mareksuscak. Closing

@nartc nartc closed this as completed Nov 14, 2020
@mjgp2
Copy link
Author

mjgp2 commented Nov 17, 2020

@nartc - that's not actually the answer to the original question, I don't think you should have closed it :)

I think as @kamilmysliwiec says, "it still doesn't check controller's methods signature (something to add in the future)."

@murbanowicz
Copy link

@kamilmysliwiec does reopening means that is being worked on?

@tamtakoe
Copy link

tamtakoe commented Feb 9, 2021

Maybe to add OptionalPipe for Swagger?

But the best way of course to use ?

getData(@Query('limit') limit?: number)

@jameskuizon13
Copy link

@tamtakoe It doesn't work for me.

@tamtakoe
Copy link

tamtakoe commented Apr 5, 2021

@jameskuizon1315 It doesn't work. It was an idea for contributors

@Burnett2k
Copy link

I'd like to +1 that supporting using ? to indicate an optional querystring parameter would be really nice. I'm converting an application to NestJs and was pretty surprised to find out that you have to do a workaround given how easy it is to use decorators for almost all other things.
I have a very straightforward endpoint which doesn't require creating a class, so I think there should be a simpler way to accomplish this. I'm about to present my application to the team and I can tell it will be awkward to explain you have to do this workaround to get an optional property. In the past I've used io-ts and they definitely support this.

@mdorda
Copy link

mdorda commented Feb 16, 2022

Wow, I was quite surprised and confused why all my optional query parameters were omitted from the swagger document. It's a shame that so great framework doesn't have support for such a pretty common thing.

I tried to invastigate it further. The problem is, that Reflect.getMetadata(constants.PARAMTYPES_METADATA, instance, method.name) returns object type for all union types (eg: string | undefined) and all object types are omitted later.
Unfortunately, all metadata are set outside of this swagger extension (probably in core module). Who knows what all it would break then if it was fixed.

@kamilmysliwiec
Copy link
Member

I tried to invastigate it further. The problem is, that Reflect.getMetadata(constants.PARAMTYPES_METADATA, instance, method.name) returns object type for all union types (eg: string | undefined) and all object types are omitted later.
Unfortunately, all metadata are set outside of this swagger extension (probably in core module). Who knows what all it would break then if it was fixed.

This is a TypeScript limitation. Not much we can do on the framework side.

@JacobMuchow
Copy link

Note to anyone checking this out today, @ApiImplicitQuery is @ApiQuery in latest versions :)

@habogay
Copy link

habogay commented Sep 19, 2022

You use @ApiQuery, set required=false and add ? before :
Like this:
@ApiQuery({ name: 'id', required: false, type: Number })
getUserById(@Req() req: any, @Query('id') id?: number)

@huykon
Copy link

huykon commented Jan 27, 2023

I tried the @habogay's way but the api always return 200 without any content, how can I fix it ?

@ermarkar
Copy link

is there any solution to not mark required on swagger docs, i tried myProp? not working

@invissiblecat
Copy link

@ermarkar it looks like only the solution above is currently working. i failed to found anything else

You use @ApiQuery, set required=false and add ? before : Like this: @ApiQuery({ name: 'id', required: false, type: Number }) getUserById(@Req() req: any, @Query('id') id?: number)

@brunodmn
Copy link

@apiquery({ name: 'id', required: false, type: Number })

This seems not to work with nestjs swagger plugin .

Snippet of my controller:

@ApiQuery({ enum: AsaasPaymentStatusQuery, required: false })
  @Get('subscriptions/:id/payments')
  listSubscriptionPayments(
    @Param('id') id: string,
    @Query('status') status?: AsaasPaymentStatusQuery,
  ) {
    return this.asaasService.listSubscriptionPayments(id, status);
  }

Swagger documentation :
image

Looks like the plugin won't override or recognize @apiquery(), if I select the optional "status" shown on the upper image, it will take no effect on my request. It works if I fill the required "status", but it is not meant to be required.

Am I doing something wrong?

@EcksDy
Copy link

EcksDy commented Nov 29, 2023

Is it possible to add options object to @Query() decorator?

Something along the lines of:

getAll(@Query(`someQuery`, { optional: true }) someQuery?: string)
getAllWithTransform(@Query(`someQuery`, new Pipe(...), { optional: true }) someQuery?: number)

That would reduce the amount of decorators needed, and while not perfect, at least should improve the overall experience.

@DrRoot-github
Copy link

@brunodmn
I'm not sure, but isn't the 'name' property missing for the 'ApiQuery'?
Since 'ApiQuery' itself is used to attach metadata to a function to associate it with its arguments, I think the 'name' property is required.
How about adding the 'name' property like this:
@ApiQuery({ enum: AsaasPaymentStatusQuery, required: false, name: 'status' })

@brunodmn
Copy link

@brunodmn I'm not sure, but isn't the 'name' property missing for the 'ApiQuery'? Since 'ApiQuery' itself is used to attach metadata to a function to associate it with its arguments, I think the 'name' property is required. How about adding the 'name' property like this: @ApiQuery({ enum: AsaasPaymentStatusQuery, required: false, name: 'status' })

Thank you @DrRoot-github, adding the name makes it work (override default plugin behavior). This is not a required param by the ApiQuery though.

@julianpoemp
Copy link

This issue is still active.

You use @ApiQuery, set required=false and add ? before : Like this: @ApiQuery({ name: 'id', required: false, type: Number }) getUserById(@Req() req: any, @Query('id') id?: number)

This workaround helped in my situation. Would be more beautiful if just using ? works.

@Stono
Copy link

Stono commented Feb 21, 2024

+100 (please just make ? work!)

@nestjs nestjs locked and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests