loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.95k stars 1.07k forks source link

Allow multiple routes decorated to a controller method #6707

Closed f-w closed 3 years ago

f-w commented 3 years ago

Suggestion

lb3 allows a custom remote method to handle multiple routes. lb4 limits # of operation decorators to 1 for a controller method. Even though it can be mitigated by creating separate methods calling common code, the API spec in parameter decorators of these methods have to be duplicated.

Use Cases

Consider a self managed opt-in opt-out email subscription API server, user can unsubscribe by calling DELETE /subscriptions/{id} endpoint. But it is also a common practice to provide a GET endpoint in order to unsubscribe by clicking the unsubscribe link in email. Parameters and underlying business logic should be the same for both GET and DELETE endpoints.

Examples

In lb3 this can be achieved using array for http property

  "methods": {
    "prototype.deleteItemById": {
      ...
      "http": [
        {
          "path": "/",
          "verb": "delete"
        },
        {
          "path": "/unsubscribe",
          "verb": "get"
        }
      ]
    },
  },

lb4 equivalent would be either

  @operation([{verb: 'GET', path: '/subscriptions/{id}/unsubscribe'}, 
              {verb: 'DELETE', path: '/subscriptions/{id}'}], 
             {...}: OperationObject)
  async deleteById(...){...}

or

  @del('/subscriptions/{id}', {...}: OperationObject)
  @get('/subscriptions/{id}/unsubscribe', {...}: OperationObject)
  async deleteById(...){...}

in controller class. The first option is preferred because OperationObject spec can also be shared.

mdbetancourt commented 3 years ago

yes i get some problems with this too but is mainly caused because currently loopback decorators doesnt allow to decorate the same function, property (like models) twice

raymondfeng commented 3 years ago

It can be achieved as follows:

@del('/subscriptions/{id}')
async deleteById(...){...}

@get('/subscriptions/{id}/unsubscribe')
async deleteById2(...){
  return this.deleteById(...);
}

There are some complexity to mark a single method with multiple @operation or sugar decorators as we need to find out how to merge specs.

Maybe one compromise is to allow something like aliasPaths to allow extra routes for @operation

f-w commented 3 years ago

It can be achieved as follows:

@del('/subscriptions/{id}')
async deleteById(...){...}

@get('/subscriptions/{id}/unsubscribe')
async deleteById2(...){
  return this.deleteById(...);
}

The drawback is explained in Suggestion section.

raymondfeng commented 3 years ago

@f-w If you are willing to contribute a patch to allow alternative verb/path for a given operation, we are happy to review.

f-w commented 3 years ago

@raymondfeng , is this alternative less complex?

@operation([{verb: 'GET', path: '/subscriptions/{id}/unsubscribe'}, 
              {verb: 'DELETE', path: '/subscriptions/{id}'}], 
             {...}: OperationObject)
  async deleteById(...){...}
mdbetancourt commented 3 years ago

@raymondfeng , is this alternative less complex?

@operation([{verb: 'GET', path: '/subscriptions/{id}/unsubscribe'}, 
              {verb: 'DELETE', path: '/subscriptions/{id}'}], 
             {...}: OperationObject)
  async deleteById(...){...}

i dont think so xd

raymondfeng commented 3 years ago

What I have in mind is something like:

@path('get', '/subscriptions/{id}/unsubscribe') // it only accepts verb and path, not spec
@delete('/subscriptions/{id}' , spec)

The @path is a placeholder. Maybe @route or @alias is better.

f-w commented 3 years ago

What I have in mind is something like:

@path('get', '/subscriptions/{id}/unsubscribe') // it only accepts verb and path, not spec
@delete('/subscriptions/{id}' , spec)

The @path is a placeholder. Maybe @route or @alias is better.

It limits max # of routes to a method to 2.

raymondfeng commented 3 years ago

Not really. The @path can be applied more than once to the same method.

achrinza commented 3 years ago

My main concern with @path() is that it may be too restrictive. For example, these endpoints might have a different description or schema. Furthermore, it can get quite complex if we have multiple sets:

@path('get', '/users2/{id}')
@get('/users/{id}' , spec)
@path('get', '/subscriptions/{id}/unsubscribe')
@delete('/subscriptions/{id}' , spec)

One way this may be interpreted is:

  1. subscriptions/{id}/unsubscribe will inherit the spec from /subscriptions/{id}
  2. /users2/{id} will inherit the spec from /users/{id}

It's not a good example of a real-life use-case, but I don't think LoopBack 4 should allow developers to make their controllers overly-complex. Developers shouldn't need to think of the direction that decorators are interpreted and how they would behave.


I favour supporting "stacking" @operation(), @get(), etc.

Also potentially allow the decorators to accept an array:

@get([
  {'/users/{id}' , spec},
  {'/users2/{id}' , spec},
])

This would enable developers to group their requests by their RESTful operations.

mdbetancourt commented 3 years ago

+1 to use @get() @post()

raymondfeng commented 3 years ago

My goal was to minimize the merging complexity. It will only allow 0-N @path in addition to one @operation (including @get, @post, etc). Merging specs is not fun.

@path('get', '/users2/{id}') // alias path 1
@path('get', '/subscriptions/{id}/unsubscribe') // alias path 2
@delete('/subscriptions/{id}' , spec) // official spec
f-w commented 3 years ago

Not really. The @path can be applied more than once to the same method.

@raymondfeng , are you sure? The limitation of applying a decorator function maximum once to a method is imposed by decorator-factory rather than openapi-v3. If it's easy to allow multiple path decorators, then it's also easy to allow multiple operation decorators.

raymondfeng commented 3 years ago

@f-w See https://github.com/strongloop/loopback-next/blob/afb99ebcbea9924b590f211c1db33c9b0d570152/packages/metadata/src/decorator-factory.ts#L823. We have MethodMultiDecoratorFactory<T> which is used by @response

stale[bot] commented 3 years ago

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

stale[bot] commented 3 years ago

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.