nest icon indicating copy to clipboard operation
nest copied to clipboard

feat(core): add option to have router module path before version

Open tbedrnik opened this issue 4 months ago • 4 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [ ] Bugfix
  • [x] Feature
  • [ ] Code style update (formatting, local variables)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Other... Please describe:

What is the current behavior?

When using RouterModule along with URI versioning, you cannot affect the order of path parts. It is always like this: /{globalAppPrefix}/{version}/{modulePath}/{controllerPath}/{methodPath}

I'm using RouterModule to separate public facing and internal routes like this:

// app.module.ts
@Module({
  imports: [
    PublicApiModule,
    InternalApiModule,
    RouterModule.register([
      {
        path: 'public',
        module: PublicApiModule,
      },
      {
        path: 'internal',
        module: InternalApiModule,
      },
    ]),
  ],
})
export class AppModule {}

// main.ts
app.setGlobalPrefix('api')
app.enableVersioning({type: VersioningType.URI})

So my routes are /api/public/... and /api/internal/.... When I enable URI versioning my routes are /api/v1/public/... and /api/v1/internal/... which breaks my proxy which enables any requests starting with /api/public/* to pass in.

Issue Number: N/A

What is the new behavior?

After adding pathBeforeVersion: true, the module path and version parts are swapped like so: /{globalAppPrefix}/{modulePath}/{version}/{controllerPath}/{methodPath}

@Module({
  imports: [
    PublicApiModule,
    InternalApiModule,
    RouterModule.register([
      {
        path: 'public',
        pathBeforeVersion: true,
        module: PublicApiModule,
      },
      {
        path: 'internal',
        pathBeforeVersion: true,
        module: InternalApiModule,
      },
    ]),
  ],
})
export class AppModule {}

My routes are now /api/public/v1/... and /api/internal/v1/... and my proxy still works.

Setting the pathBeforeVersion: false or omitting the parameter doesn't introduce any change.

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

tbedrnik avatar Mar 04 '24 15:03 tbedrnik

Pull Request Test Coverage Report for Build c036f711-58ff-4d30-9953-75a598aeb263

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 92.172%

Totals Coverage Status
Change from base Build 2bf2e4c8-5da9-4dd1-8f93-3b733029afc6: 0.01%
Covered Lines: 6747
Relevant Lines: 7320

💛 - Coveralls

coveralls avatar Mar 04 '24 17:03 coveralls

I'm not sure about the API

I found that pathBeforeVersion flag a bit hacky (like a patch over something) and not flexible.

As versioning is a first-class thing in nestjs, I think we should use this concept somehow here but I didn't have a suggestion in mind so far

micalevisk avatar Mar 04 '24 23:03 micalevisk

I'm not sure about the API

I found that pathBeforeVersion flag a bit hacky (like a patch over something) and not flexible.

As versioning is a first-class thing in nestjs, I think we should use this concept somehow here but I didn't have a suggestion in mind so far

It could be something like versionPosition: 'AFTER' | 'BEFORE'.

Or another idea would be to extend the versioning options which could give more options for the position of the version within the route, but doesn't offer the granularity of setting this per RouterModule.

export interface UriVersioningOptions {
  type: VersioningType.URI;
  /**
   * Optional prefix that will prepend the version within the URI.
   *
   * Defaults to `v`.
   *
   * Ex. Assuming a version of `1`, for `/api/v1/route`, `v` is the prefix.
   */
  prefix?: string | false;

  // Adding this:
  position?: UriVersionPosition;
}

enum UriVersionPosition {
  AFTER_GLOBAL_PREFIX,
  AFTER_MODULE,
  // Theoretically able to have these as well:
  AFTER_CONTROLLER,
  AFTER_ROUTE,
}

Actually both could be combined and RouterModule's setting would be prioritized.

I'm in to coming up with something more flexible. It's just about how flexible we want this to be. Currently before or after RouterModule's path seems enough to me, but maybe somebody would like it after their Controller's path.

tbedrnik avatar Mar 05 '24 00:03 tbedrnik

I'm not sure about the API I found that pathBeforeVersion flag a bit hacky (like a patch over something) and not flexible. As versioning is a first-class thing in nestjs, I think we should use this concept somehow here but I didn't have a suggestion in mind so far

It could be something like versionPosition: 'AFTER' | 'BEFORE'.

Or another idea would be to extend the versioning options which could give more options for the position of the version within the route, but doesn't offer the granularity of setting this per RouterModule.

export interface UriVersioningOptions {
  type: VersioningType.URI;
  /**
   * Optional prefix that will prepend the version within the URI.
   *
   * Defaults to `v`.
   *
   * Ex. Assuming a version of `1`, for `/api/v1/route`, `v` is the prefix.
   */
  prefix?: string | false;

  // Adding this:
  position?: UriVersionPosition;
}

enum UriVersionPosition {
  AFTER_GLOBAL_PREFIX,
  AFTER_MODULE,
  // Theoretically able to have these as well:
  AFTER_CONTROLLER,
  AFTER_ROUTE,
}

Actually both could be combined and RouterModule's setting would be prioritized.

I'm in to coming up with something more flexible. It's just about how flexible we want this to be. Currently before or after RouterModule's path seems enough to me, but maybe somebody would like it after their Controller's path.

This is an interesting approach, we should consider it instead of the OC solution, i mean, it seems to be permissive (Also seems a little bit useless in 99% but for NestJS as a framework, it could be better to offer this kind of change instead of just one different placement slot)

benjGam avatar Mar 12 '24 02:03 benjGam