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

HTTP methods: Support for Google-API-style custom methods paths #7412

Closed
yoshigev opened this issue Jul 6, 2021 · 2 comments
Closed

HTTP methods: Support for Google-API-style custom methods paths #7412

yoshigev opened this issue Jul 6, 2021 · 2 comments
Labels
needs triage This issue has not been looked into type: enhancement 🐺

Comments

@yoshigev
Copy link

yoshigev commented Jul 6, 2021

Feature Request

Google API design guide has a notion of custom methods.
These custom methods are denoted using a colon in the path. For example:

POST /orders:purge

Two problems arise when trying to implement such a controller:

  1. Support of colon in path that is not a route parameter.
    This was discussed in Express's ticket: How to use colon as normal character instead of route param expressjs/express#3857
    Currently the way to perform it is using regular expression, like orders[:]purge

  2. When declaring an HTTP method with a path like [:]purge, a leading slash is automatically added: /[:]purge.
    This prevents us from configuring the right path to the controller (orders).

Proposed solution

@Controller('orders')
export class OrdersController {
  constructor() { }

  @Post('\\:purge') // Full route will be equivalent to "/orders[:]purge" (and not "/orders/[:]purge")
  purge() {
    return {};
  }
}

I suggest that a "backslash+colon" in a path will perform the following:

  1. Be treated as a regular colon (not as route parameter).
    This can be achieved by replacing it with [:] behind the scenes.
  2. If appears on the start of the path - no leading slash will be added.

Drawbacks:

  • The conversion of \\: to [:] might have side affects, like an awkward looking Swagger doc.
  • Two separate behaviors are achieved using the same syntax.
    Maybe the removal of the leading slash can be controlled using a new option like: @Post('\\:purge', { noLeadingSlash: true }).

Current workaround

Currently, to achieve the desired behavior, the controller path should be removed, like:

@Controller('/')
export class OrdersController {
  constructor() { }

  @Post('orders[:]purge')
  purge() {
    return {};
  }
}

Teachability, Documentation, Adoption, Migration Strategy

I don't think that this breaks anything.
Just a new syntax to be documented (if implemented 😄).

@yoshigev yoshigev added needs triage This issue has not been looked into type: enhancement 🐺 labels Jul 6, 2021
@Tony133
Copy link
Contributor

Tony133 commented Jul 6, 2021

It might be interesting but it needs to be seen if there are plans to implement this right now.

@kamilmysliwiec
Copy link
Member

I suggest that a "backslash+colon" in a path will perform the following:
Be treated as a regular colon (not as route parameter).
This can be achieved by replacing it with [:] behind the scenes

IMO, using [:] directly is more convenient than \\:. Also, Nest simply passes down route paths to the underlying HTTP adapter (express/fastify), so I'd rather recommend reporting this issue there (we don't really want to perform any additional & custom path transformations).

As for the noLeadingSlash option, comparing the following examples:

// Proposed API
@Controller('/orders')
export class OrdersController {
  constructor() { }

  @Post('\\:purge', { noLeadingSlash: true })
  purge() {
    return {};
  }
}

and the current one:

// Current API
@Controller()
export class OrdersController {
  constructor() { }

  @Post('orders[:]purge')
  purge() {
    return {};
  }
}

the latter one looks cleaner to me.

Let me close this one as I don't think we'll be looking into implementing such a feature in the foreseeable future.

@nestjs nestjs locked and limited conversation to collaborators Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs triage This issue has not been looked into type: enhancement 🐺
Projects
None yet
Development

No branches or pull requests

3 participants