darraghoriordan / eslint-plugin-nestjs-typed

Some eslint rules for working with NestJs projects
http://www.darraghoriordan.com
174 stars 34 forks source link

Swagger: Rule for enforcing @ApiParam and @ApiQuery of @Param("...") or @Query("...") is used. #118

Closed SunsetFi closed 11 months ago

SunsetFi commented 1 year ago

We would like to enforce well-defined swagger docs for all queries and params. It would be great if any presence of a @Param() or @Query() decorator on an argument could enforce a @ApiParam or @ApiQuery which match the parameter name.

Chances are good I will be submitting a PR for this myself. We are currently in the trial phases of checking out nestjs and this is a big sticking point to us. It might be a while before I can get to it though, so I wanted to bring this up early to check out if its something that this library would be interested in having.

darraghoriordan commented 11 months ago

yea sounds like a great idea to me. I guess it would be an optional one to turn on though (not in the recommended rule set).

What swagger docs do you set in the @apiParam/@queryparam decorators? Descriptions and such?

SunsetFi commented 11 months ago

It seems our usage of NestJS is falling through, so unfortunately I do not have the capacity to contribute these rules. However, for those who come after me, I can give a summary of our original intent.

The goal was to create linter rules that enforce enough metadata on nest controllers that sensible swagger docs would be generated. This proved to be a pretty complex problem, and quite a bit beyond my original post.

Our initial thought was to enforce that all @Param(name) instances have a matching @ApiParam(name, ...) entry (and same with @Query, and @Headers). Descriptions would certainly be a candidate to enforce but possibly a bit heavy handed for all use cases.

We would also have needed enforcement of @ApiBody wherever nest could not infer the type, which would be wherever arrays or non-class types were used for @Body annotated arguments. We would need the same with response objects, especially where the function returns a promise and nest cannot infer that. This could be something like requiring at least one @ApiResponse (or variant decorators). I remember there being a companion decorator to inform nest of the native return type for swagger purposes, but I am failing to find that in the documentation at the moment.

Even with that, we still had a problem of the hand-rolled validation logic not being kept in sync with the api decorators, so we moved onto using DTOs. This is because nestjs is nominally capable of generating swagger from the class-validator validation decorators on the DTOs. However, this would bring its own set of rules in order to ensure the process works correctly:

All in all, the effort to enforce robust swagger on a nestjs app ended up being too high of a cost for adoption, and we ended up adapting an older library of ours for an openapi-first approach.

For anyone still looking to do this, the closest we got to unifying the swagger docs and nestjs validation was to abandon nest's class-validator validation altogether and use express-openapi-validator fed with nest's own runtime-generated swagger. This at least gave us a what the swagger says is what is enforced assurance, but the linter rules described here would still be essential in ensuring that the swagger is generated correctly.

I am closing this issue since we no longer have an interest or need for this feature, but hopefully anyone else seeking to wrangle swagger with nest can pick this up for inspiration.

darraghoriordan commented 11 months ago

Sorry you've probably already subscribed and this is unrelated to the issue so no need to reply if you don't have time.

But I'm keen to hear what you're using instead of nest? I work in consulting type stuff and havent used nestjs in a year at work i think. It's all NextJS or serverless these days?

SunsetFi commented 11 months ago

So we had one big advocate for Nest on the team, but in the end the amount of effort it would take to get accurate OpenAPI specs and prevent misuse wasn't worth what switching over to it would buy us. OpenAPI accuracy and non-optional validation was a particular sticking point, as these are public APIs that need to be robustly validated, documented, and monitored for breaking changes.

We had already made a decorator-based controller library in the past to solve our validation woes with json schema, and it essentially produced OpenAPI specs as an afterthought. We took that design, kept the api, and inverted it. The decorators now directly generate OpenAPI describing the operation, with additional extension properties to record the handler function and positional arguments. Then the resulting spec is used to generate a router where all requirements defined by the spec are enforced before the handler method is called.

The new library lives at SunsetFi/simply-openapi for now, with GitBook documentation. There's a minimalist example, but I really need to put together a more significant demonstration showing off how the spec can be leveraged with regard to tests and linting. For example, we have jest snapshot tests being auto generated for all endpoints to capture any api changes, and we are using Spectral with a custom ruleset to enforce our API design guidelines.

It's framework agnostic, we have it running with an Inversify DI base right now, but as it essentially emits express middleware it can be used anywhere express is. The engine is very nearly generic enough to be abstracted to run on koa or fastly too, but that's not going to be a priority for a while.

darraghoriordan commented 11 months ago

hah this is interesting yea. i'm working on a client project at the moment and the team has a custom decorator open api spec generator for controllers. its for aws lamda handlers though.

we use zod for validation and use a zod to open api spec library for the open api model generation

Interesting how there is value in the spec but the breadth of javascript ecosystem means the tooling might never converge on one framework/implementation. good and bad i supose.

best of luck with the project anyways 🫡