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

Sugar response decorators for easier controller config #1672

Closed
6 tasks
virkt25 opened this issue Sep 6, 2018 · 14 comments
Closed
6 tasks

Sugar response decorators for easier controller config #1672

virkt25 opened this issue Sep 6, 2018 · 14 comments
Labels

Comments

@virkt25
Copy link
Contributor

virkt25 commented Sep 6, 2018

Description / Steps to reproduce / Feature proposal

Currently an OpenAPI response can only be set by providing the complete controller spec OR by passing it in the operation decorator. Describing a spec is very tedious and time consuming and we should provide an easier way to automagically assemble the spec.

Proposed Design

class MyController {
  @response.ok('todo model instance created')
  @post('/')
  myMethod(): Promise<Todo> {
    // implementation
  }
}

A few alternatives for the decorator:

// schema in this case can be a Model / TypeScript Type.
@response(statusCode: number, description: string, responseSchema: schema)
@response.ok(description: string, responseSchema: schema) // ok in this case would translate to a 200
@response.statusCode(description: string, responseSchema: schema) // statusCode being 200, 404, etc.
@response.ok(description: string) + @response.schema(Todo) // allows arbitrary schema registration with the OpenAPI spec.

Acceptance Criteria

  • Create a series of new "sugar" decorators to simplify OpenAPI response decorations for REST APIs. Proposed Decorators are as follows:
@response(200, 'description', 'application/custom-type', Customer)
@response(statusCode, Customer)
@response(statusCode, description, Customer)
@response(Customer) // Automagically generate statusCode, description (as done today), and assume json return type
// NOTE: Document that complex cases are dealt with via the operational decorators (@get, @post). 
  • The decorators should be "stackable", meaning the same decorator can be utilized multiple times on the same class method to specify multiple responses
class MyController {
@response(String)
@response(500, 'server error', Error)
hello() { // ... }
}
  • Update CLI controller templates to use the new decorator instead
  • Update example/todo, example/todo-list to use the new decorators
  • Tests
  • Docs
  • Blog
@virkt25 virkt25 changed the title Easier controller response decoration Sugar response decorators for easier controller config Sep 7, 2018
@bajtos
Copy link
Member

bajtos commented Sep 7, 2018

@response.statusCode(description: string, responseSchema: schema) // statusCode being 200, 404, etc.

Can we perhaps use status names from http-errors package? E.g. @response.notFound, @response.internalServerError, etc.

@virkt25
Copy link
Contributor Author

virkt25 commented Sep 7, 2018

For sure! I think the only thing would be that I would be is should the response name be capitalized as that is how it is in http-errors? Ex: @response.NotFound, @response.InternalServerError, etc.

@raymondfeng
Copy link
Contributor

It can be as simple as @response(statusCode: number, schema: SchemaObject | ReferenceObject, description='').

For example:

@response(200, {x-ts-type: Customer})
@response(400, {x-ts-type: InvalidCustomerError}, 'customer name is missing')

@virkt25
Copy link
Contributor Author

virkt25 commented Sep 7, 2018

That certainly works ... I think we can go further and just let the user pass in the type directly instead of wrapping it in an Object with the x-ts-type property. Only thing I see missing is how does a user set the content-type? Default can be application/json but if a user wanted to change it what do we do?

@bajtos
Copy link
Member

bajtos commented Sep 10, 2018

That certainly works ... I think we can go further and just let the user pass in the type directly instead of wrapping it in an Object with the x-ts-type property.

Yes please! We can always add support for arbitrary schema later, because it's easy to distinguish between as schema object ({'x-ts-type': Product}) a model constructor function (Product).

Only thing I see missing is how does a user set the content-type? Default can be application/json but if a user wanted to change it what do we do?

How about the following:

@response.json(200, Customer);
// later we can add
@response.xml(200, Customer);
@response.custom('application/custom-type', 200, Customer);

@response(statusCode: number, schema: SchemaObject | ReferenceObject, description='')

In the future, we may want to allow users to pass arbitrary extra properties besides description, e.g. {description: 'lorem ipsum', x-my-extension: true}. This can be easily supported later by using string | object type for the last parameter, so nothing to worry about right now.

@bajtos
Copy link
Member

bajtos commented Sep 10, 2018

Another option I was pondering recently was to use a Builder pattern.

@response(200).json().model(Customer).description('lorem ipsum')
@response(404).contentType('application/custom-type').schema(MySchema)

I think this can be particularly useful for parameter decorators, where we already have M times N functions, where M is the number of in values (query, path, etc.) and M is the number of types (string, integer, etc.). To distinguish between optional and required parameters, we would have to create another set of M times N functions. That does not scale well.

The builder pattern has two challenges:

  • The decorator has to return something that's both a callable decorator function that TypeScript can use AND has builder methods for further chaining.
  • Extensibility - how to allow 3rd-party extensions to contribute custom builder methods together with type information needed for TypeScript to verify correct usage.

@bajtos
Copy link
Member

bajtos commented Sep 10, 2018

I think the only thing would be that I would be is should the response name be capitalized as that is how it is in http-errors? Ex: @response.NotFound, @response.InternalServerError, etc.

I find the style @response.NotFound rather inconsistent to be honest. I am sure it is fairly easy to convert from NotFound to notFound, unless I am missing something?

@virkt25
Copy link
Contributor Author

virkt25 commented Sep 10, 2018

description is a required property of the response object while the return type is optional. I think the following would be a good start.

@response.json(200, 'description', Customer); // Sugar on `@response`
@response.xml(200, 'description', Customer);  // Sugar on `@response`
@response('application/custom-type', 200, 'description', Customer)

I like the statusCode being the first argument but at the same time think it'd be better if it was optional / defaulted to 200 as that's most likely the only response a user will describe at a minimum.

@response.json('description', Customer)

@bajtos
Copy link
Member

bajtos commented Sep 11, 2018

@response.json('description', Customer)

I love that form ☝️

@response('application/custom-type', 200, 'description', Customer)

Maybe put the code first and the content-type second? That's how the information is mapped in the spec IIRC and send down the TCP/IP socket in the HTTP response.

@response(200, 'application/custom-type', 'description', Customer)

@virkt25
Copy link
Contributor Author

virkt25 commented Sep 12, 2018

Maybe put the code first and the content-type second? That's how the information is mapped in the spec IIRC and send down the TCP/IP socket in the HTTP response.

That makes sense to me. +1 for the the @response decorator signature proposed as well -- I would just like the statusCode to default to 200 if not present for all the @response decorators.

@jannyHou
Copy link
Contributor

jannyHou commented Sep 13, 2018

  • To handle multiple descriptions for the same status code: The story owner can drive a discussion regarding how to generate the description: pick the first one or join them, etc... and document it.
  • The simplest decorator would be omitting the status code and content type, only has description(optional) and model type.

@stale
Copy link

stale bot commented Sep 17, 2019

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 stale bot added the stale label Sep 17, 2019
@stale
Copy link

stale bot commented Oct 17, 2019

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.

@stale stale bot closed this as completed Oct 17, 2019
@bajtos
Copy link
Member

bajtos commented May 11, 2020

I think this has been (partially?) implemented by #4550

Example

@model()
class SuccessModel extends Model {
  constructor(err: Partial<SuccessModel>) {
    super(err);
  }
  @property({default: 'Hi there!'})
  message: string;
}

class FooNotFound extends Model {
  @property()
  message: string;
}

class BarNotFound extends Model {
  @property()
  message: string;
}

class BazNotFound extends Model {
  @property()
  message: string;
}

class MyController {
  @oas.get('/greet/{foo}/{bar}',
  @oas.response(200, SuccessModel)
  @oas.response(404, FooNotFound, BarNotFound)
  @oas.response(404, BazNotFound)
  greet() {
    return new SuccessModel({message: 'Hello, world!'});
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants