nest icon indicating copy to clipboard operation
nest copied to clipboard

Enhance forRoutes RouteInfo object to specify API versions

Open NitnekB opened this issue 2 years ago • 7 comments

Is there an existing issue that is already proposing this?

  • [X] I have searched the existing issues

Is your feature request related to a problem? Please describe it

I currently use NestJS versioning feature, which is nice BTW 🚀

I also use a custom middleware the most common way:

consumer
      .apply(CustomMiddleware)
      .forRoutes(PricesController, ProductsController);

But this caused a regression and my middleware doesn't works anymore.

The only way to make it works is to specify each route, or exclude the one we doesn't want:

consumer
      .apply(CustomMiddleware)
      .exclude({ path: '/v0/products', method: RequestMethod.POST })
      .forRoutes({ path: '/*', method: RequestMethod.ALL });

This works well, but we have to fill in the version for each route: /v0/products, ...

Describe the solution you'd like

I dont' know if we should consider this as a bug, but I think we could use another parameter versions when declaring routes on forRoutes method enhancing RouteInfo type.

Teachability, documentation, adoption, migration strategy

Here an example of what I'm thinking about:

consumer
      .apply(CustomMiddleware)
      .exclude({ path: '/products', method: RequestMethod.POST, versions: ['0', '1'] })
      .forRoutes({ path: '/*', method: RequestMethod.ALL });

What is the motivation / use case for changing the behavior?

Prevent lots of works introducing or deprecating API versions

// products.controller.ts

export const PRODUCTS_API_VERSIONS = ['0', '1'];

@Controller({ path: 'products', version: PRODUCTS_API_VERSIONS })
export class ProductsController {
}
// app.module.ts

consumer
      .apply(CustomMiddleware)
      .exclude({ path: '/products', method: RequestMethod.POST, versions: PRODUCTS_API_VERSIONS })
      .forRoutes({ path: '/*', method: RequestMethod.ALL });

NitnekB avatar Dec 22 '21 10:12 NitnekB

I believe this feat will help on making HTTP versioning a first-class citizen on NestJS. That's cool.


@NitnekB As we can use multiple versions on version/defaultVersion field already, wouldn't be better to use the singular here as well?

.exclude({ ..., version: '1' })

.exclude({ ..., version: ['1'] })

micalevisk avatar Dec 26 '21 17:12 micalevisk

@micalevisk Seems legit to me 👍

NitnekB avatar Jan 20 '22 08:01 NitnekB

May I ask for any update on this issue?

Cyril-zip avatar Mar 23 '22 06:03 Cyril-zip

@chanpaul1234572 I didn't have time to try to implement this :/

PRs are more than welcomed

micalevisk avatar Mar 23 '22 11:03 micalevisk

@kamilmysliwiec I would like to implement this.

princechauhan1992 avatar Jun 02 '22 16:06 princechauhan1992

@princechauhan1992 PRs are more than welcomed :smile_cat:

micalevisk avatar Jun 02 '22 16:06 micalevisk

Guys, I will get this one! 😁

ologbonowiwi avatar Aug 05 '22 23:08 ologbonowiwi

I'm taking this one, @kamilmysliwiec. I've been already able to reproduce the issue in an integration test.

thiagomini avatar Oct 26 '22 21:10 thiagomini

Sounds great @thiagomini!

kamilmysliwiec avatar Oct 27 '22 06:10 kamilmysliwiec

I couldn't find the original PR that introduced version-aware middleware but the implementation is currently buggy when global prefix is configured.

It produced:

/v1/api/users/:userId

instead of:

/api/v1/users/:userId

I.e. the version number is prepended before the global prefix, not the other way around. I'll create an issue.

mareksuscak avatar Nov 15 '22 12:11 mareksuscak

Hey @mareksuscak, this is the PR that introduced that. Feel free to add the necessary fix to it! I can work on that as well as soon as I have the time. Thanks for lettings us know 😊

thiagomini avatar Nov 15 '22 12:11 thiagomini

My approach to resolving this issue would be to reproduce the same problem in this test. As soon as you get the same error, work on your way down the layers to make it pass with the least amount of changes. Then, you can refactor it to a cleaner solution.

thiagomini avatar Nov 15 '22 12:11 thiagomini

Here's the ticket: https://github.com/nestjs/nest/issues/10566

mareksuscak avatar Nov 15 '22 12:11 mareksuscak