feat(openapi-v3): responses decorator #1655

virkt25 · 2018-08-29T16:37:46Z

connected to #1531

This PR:

Ensures we set a default responses property on a Controller methods OpenAPI Spec if one isn't given
Extends the OpenAPI spec by introducing a x-ts-type property that can be used to specify the schema of a response object. Can also be used to specify the type of array items.

content: {'x-ts-type': Todo} => content: {schema: {$ref: '#components/schemas/Todo'}}
content: {schema: {type: 'array', items: {x-ts-type: Todo}}} => content: {schema: {type: 'array', items: {$ref: '#components/schemas/Todo'}}}

Refactors generate-schema to have a single function to resolve schema from a Function.
Updates CLI Controller template to set a responses object based on the Model
Updates example-todo / example-todo-list to set responses object

~~This PR introduces:~~
~~- a new @responses decorator that can be used to decorate a Controller method with the appropriate OpenAPI responses object.~~
~~- for controller's not decorated with this property, or via the @operation and family (get, post, etc.) of decorators, we generate a default responses object to be OpenAPI 3.0 compliant~~
- controller-spec generator can set Model as $ref for {schema: Model} or {schema: {type: 'array', items: Model}}}in the responses object.~ ~- Decorateexample-todowith@Responses()` decorator as an example. Once this PR is agreed upon, a follow up PR/commit will add decoration to CLI template + remaining examples + docs

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

raymondfeng · 2018-08-29T16:54:18Z

examples/todo/src/controllers/todo.controller.ts

+      description: 'Todo model instance',
+      content: {'application/json': {schema: Todo}},
+    },
+  })


Looks good. Very intuitive to folks who are familiar with OpenAPI spec.

raymondfeng · 2018-08-29T16:56:06Z

examples/todo/src/controllers/todo.controller.ts

+  @responses({
+    '200': {
+      description: 'Put success',
+      content: {'application/json': {schema: {type: 'boolean'}}},


We might want to allow {schema: Boolean}.

This may need story #1112 to happen first.

raymondfeng · 2018-08-29T16:57:38Z

packages/openapi-v3/src/controller-spec.ts

+            'responses schema of type array detected with items as a model / function',
+          );
+          operationSpec.responses[code].content[c].schema.items = getModelRef(
+            operationSpec.responses[code].content[c].schema.items,


Refactor operationSpec.responses[code].content[c].schema to be a variable.

raymondfeng · 2018-08-29T16:58:30Z

packages/openapi-v3/src/controller-spec.ts

@@ -239,3 +285,7 @@ export function getControllerSpec(constructor: Function): ControllerSpec {
  }
  return spec;
 }
+
+function getModelRef(fn: Function) {
+  return {$ref: `#/components/schemas/${fn.name}`};


If fn is a Model class with @model({name: 'my-model'}), we should use my-model.

It would be great if we could share the implementation of "Model class to $ref" algorithm with openapi-v3 package, see https://github.com/strongloop/loopback-next/blob/8499862ed7bbb615d8bcdf2c2590fcdfaa624e8f/packages/openapi-v3/src/generate-schema.ts#L54-L65

jannyHou

👍 LGTM so far. Just a few nitpicks.

jannyHou · 2018-08-29T17:53:48Z

packages/openapi-v3/src/decorators/responses.decorator.ts

@@ -0,0 +1,100 @@
+// Copyright IBM Corp. 2018. All Rights Reserved.


You could add this file in https://github.com/strongloop/loopback-next/blob/master/packages/openapi-v3/docs.json so the tsdoc generates API doc for it :)

jannyHou · 2018-08-29T17:58:17Z

packages/openapi-v3/src/decorators/responses.decorator.ts

+    descriptor: TypedPropertyDescriptor<any>,
+  ) {
+    debug('@responses() on %s.%s', target.constructor.name, member);
+    debug('  responsesObject: %s', inspect(responsesObject, {depth: null}));


nitpick: is the space before responsesObject intended?

jannyHou · 2018-08-29T18:11:41Z

packages/openapi-v3/src/controller-spec.ts

+
+    debug('  decorated responses for method %s: %o', op, responses);
+
+    const defaultResponse = {


nitpick: how about declare it before Line97 to reuse the defaultResponse in

if (!operationSpec) { // The operation was defined via @operation(verb, path) with no spec operationSpec = { responses: defaultResponse }; }

As I commented earlier, I am proposing to remove @responses decorator from this pull request.

If we decide to keep it around, then I agree too that the default response spec should be reused between these two places.

jannyHou · 2018-08-29T18:22:21Z

examples/todo/src/controllers/todo.controller.ts

+  @responses({
+    '200': {
+      description: 'Put success',
+      content: {'application/json': {schema: {type: 'boolean'}}},


This may need story #1112 to happen first.

bajtos · 2018-08-30T11:14:53Z

The proposed decorator API looks great for advanced users. I have a question though - how is @responses different from specifying the responses object via @post options? To me, it does not bring enough benefits to justify the implementation/maintenance cost and more importantly possible confusion between @responses and @reponse that is described as the next decorator to be implemented.

Current proposal:

  @post('/todos')
  @responses({
     '200': {
       description: 'Todo model instance',
       content: {'application/json': {schema: Todo}},
     },
   })

Existing solution working OOTB right now:

  @post('/todos', {
    responses: {
      description: 'Todo model instance',
      content: {'application/json': {schema: Todo}},
    },
  })

}
If our main goal is to get a valid OpenAPI spec regardless of the developer experience, then I am proposing to simply fix our example apps and CLI templates to use the existing solution described above.

When it comes to adding a new decorator API, I would like to see a simpler API that will hide away the complexity of OpenAPI format and allow developers to focus on the important parts only.

For example:

@post()
@returnsJson(Todo, 'Todo model instance')
async createTodo(@requestBody() todo: Todo): Promise<Todo> {
  // ...
}

I picked returnsJson as the first name that comes to my mind, we can find a more suited one that supports all different flavors we want to provide. I can imagine the following signatures:

@returnsJson(201, Todo, 'Todo model instance')
@returnsJson(201, Todo, {description: 'Todo model instance', headers: {/*...*/}, links: {/*...*/}})
@response(201, {/* ResponseObject *})

I am proposing to remove @responses decorator from this pull request and focus on

Modifying examples and CLI templates so that controller methods define responses via @get/@post/etc. options as shown below.
Implementing the logic inferring the schema from the return value type provided by TypeScript.

I am not sure whether schema inference is usable. IIUC, we are not able to infer Todo type from the controller method return value Promise<Todo>. Since most controller methods are going to return promises, they will have to provide the schema explicitly and thus the schema inference is not worth the implementation & maintenance costs IMO.

On the other hand, if we are able to infer the schema from existing controller methods in our examples, then I would expect that no changes to our examples and CLI templates should be needed?

Either way, it seems to me that we need to implement only one of these two changes.

Thoughts?

UPDATE
The list of tasks above is missing the following item, which probably should be done in all cases:

Introduce a default responses object on the generated OpenAPI Spec (responses: {200: {description: ''}})

bajtos

Please see my comment above first, it explains why I disagree with the current direction and offers a different approach instead.

Besides the comments below, this PR is also missing an update of CLI templates, tutorial docs and general documentation related to writing Controllers. You are probably aware of that already, but the pull request description does not make it clear.

bajtos · 2018-08-30T11:19:20Z

packages/openapi-spec-builder/src/openapi-spec-builder.ts

@@ -120,7 +120,7 @@ export class OpenApiSpecBuilder extends BuilderBase<OpenApiSpec> {
 export class OperationSpecBuilder extends BuilderBase<OperationObject> {
  constructor() {
    super({
-      responses: {},
+      responses: {'200': {description: ''}},


Let's use a non-empty string please. For example description: 'An undocumented response body.'

PTAL, this comment hasn't been addressed yet.

Let's use a non-empty string please. For example description: 'An undocumented response body.'

bajtos · 2018-08-30T11:22:21Z

packages/openapi-v3/src/controller-spec.ts

-        responses: {},
+        responses: {
+          '200': {
+            description: '',


Let's use a non-empty default value please. For example, we can use the controller name and method name to build a more descriptive text:

description: `Return value of controller method ${fullMethodName}`

bajtos · 2018-08-30T11:23:48Z

packages/openapi-v3/src/controller-spec.ts

+
+    debug('  decorated responses for method %s: %o', op, responses);
+
+    const defaultResponse = {


As I commented earlier, I am proposing to remove @responses decorator from this pull request.

If we decide to keep it around, then I agree too that the default response spec should be reused between these two places.

bajtos · 2018-08-30T11:27:17Z

packages/openapi-v3/src/controller-spec.ts

@@ -239,3 +285,7 @@ export function getControllerSpec(constructor: Function): ControllerSpec {
  }
  return spec;
 }
+
+function getModelRef(fn: Function) {
+  return {$ref: `#/components/schemas/${fn.name}`};


It would be great if we could share the implementation of "Model class to $ref" algorithm with openapi-v3 package, see https://github.com/strongloop/loopback-next/blob/8499862ed7bbb615d8bcdf2c2590fcdfaa624e8f/packages/openapi-v3/src/generate-schema.ts#L54-L65

raymondfeng · 2018-08-30T15:08:01Z

Please note that @operation accepts OperationObject, which can contain ResponsesObject. The SchemaObject does not allow usage of TypeScript classes.

Here is an idea I have to mitigate the issue:

For a return type such as Promise<Customer>, we can use {$ref: '#components.schemas.Customer'}.
Introduce a @schemas decorator for the controller class to register model classes for schemas. Such as:

@schemas({Customer, MyOrder: Order})
export class MyController {}

Or a @schema decorator to map between TS and OpenAPI spec for parameters or return value, for example:

@schema(Customer)
async findById(id: string) {}

async create(@schema(Customer) customer: DeepPartial<Customer>) {}

With this approach, we can have an OpenAPI compatible spec for parameters and responses while allowing developers to map TS types to schema definitions.

In the long run, we can possibly use AST to pre-process the TypeScript file to automate these if necessary.

raymondfeng · 2018-08-30T17:21:45Z

I had an offline chat with @virkt25 and we think the PR can move forward as follows:

Remove @responses decorator as @operation already supports it.
Add x-typescript-type or x-schema so that we can use TypeScript classes for schema objects while keeping the spec compliance.
Have follow-up discussion on the proposal from feat(openapi-v3): responses decorator #1655 (comment) to evaluate if new decorators should be introduced.

bajtos · 2018-08-31T07:24:52Z

Please note that @operation accepts OperationObject, which can contain ResponsesObject. The SchemaObject does not allow usage of TypeScript classes.

Ah, good catch about SchemaObject not allowing TypeScript classes (model)!

Remove @responses decorator as @operation already supports it.

Add x-typescript-type or x-schema so that we can use TypeScript classes for schema objects while keeping the spec compliance.

Have follow-up discussion on the proposal from feat(openapi-v3): responses decorator #1655 (comment) to evaluate if new decorators should be introduced.

The plan sound good to me 👍

Let's pick a different name than x-schema please, I find it too similar to existing schema property.

On the other hand, how about introducing a helper function that can convert a class into a schema value? I think that will be easier to use and give us more flexibility for the future.

import {modelRef} from '@loopback/openapi-v3'; // or from @loopback/rest

@post('/todos', {
  responses: {
    description: 'Todo model instance',
    content: {'application/json': {schema: modelRef(Todo)}},
  },
})

Under the hood, modelRef have several options what value to return:

it can leverage the proposed x-typescript-type to attach a ref to a model class
return {$ref: '#/components/schemas/' + model.name}
return a schema generated for the model. (We don't want this to avoid duplicate definitions of the same model schema, I am mentioning this option for completeness.)

raymondfeng · 2018-08-31T16:15:25Z

+1 to have modelToRef or typeRef utility. But I don't think it will work well as part of the decoration, which happens when the class is built.

Todo can be undefined. We might have to use typeResolver such as modelRef(() => Todo).
modelRef does not have access to x-typescript-type

virkt25 · 2018-09-07T03:57:03Z

UPDATED:

This PR:

Ensures we set a default responses property on a Controller methods OpenAPI Spec if one isn't given
Extends the OpenAPI spec by introducing a x-ts-type property that can be used to specify the schema of a response object. Can also be used to specify the type of array items.

content: {'x-ts-type': Todo} => content: {schema: {$ref: '#components/schemas/Todo'}}
content: {schema: {type: 'array', items: {x-ts-type: Todo}}} => content: {schema: {type: 'array', items: {$ref: '#components/schemas/Todo'}}}

Refactors generate-schema to have a single function to resolve schema from a Function.
Updates CLI Controller template to set a responses object based on the Model
Updates example-todo / example-todo-list to set responses object

I am purposely choosing to not introduce a modelRef() function as I think it's easier for controller-spec to resolve the schema from x-ts-type.
This is a first step, next we will need to look into adding sugar response decorators as proposed in Sugar response decorators for easier controller config #1672

bajtos

The patch looks pretty good at high level now.

I have few comments related to implementation details, see below.

bajtos · 2018-09-10T08:23:48Z

examples/todo-list/src/controllers/todo-list-todo.controller.ts

+    responses: {
+      '200': {
+        description: 'TodoList.Todo model instance',
+        content: {'application/json': {'x-ts-type': Todo}},


Just in case it was not clear, if we added modelRef(ctor) helper as a sugar API returning {'x-ts-type': ctor}, then the line above could look like this:

content: {'application/json': modelRef(Todo)},

Having wrote that, I am not sure how much better that is and whether the benefits justify maintenance costs. I guess what we really want is to implement the new decorator.

I'd rather see new decorators than introduce a helper utility that we have to maintain.

bajtos · 2018-09-10T08:25:51Z

examples/todo-list/src/controllers/todo-list-todo.controller.ts

+    responses: {
+      '200': {
+        description: 'TodoList.Todo PATCH success count',
+        content: {'application/json': {'x-ts-type': Number}},


Hmm, a response returning just a number is not very usual in REST/JSON world, is it?

I think it would be better to return an object with a count property instead, such response is easier to extend with additional properties while preserving backwards compatibility.

Having said that, such change is out of scope of this pull request.

I agree it's unusual but it's what our Repository returns. I'll make a follow up issue for us to return an object instead.

bajtos · 2018-09-10T08:27:27Z

packages/openapi-spec-builder/src/openapi-spec-builder.ts

@@ -120,7 +120,7 @@ export class OpenApiSpecBuilder extends BuilderBase<OpenApiSpec> {
 export class OperationSpecBuilder extends BuilderBase<OperationObject> {
  constructor() {
    super({
-      responses: {},
+      responses: {'200': {description: ''}},


PTAL, this comment hasn't been addressed yet.

Let's use a non-empty string please. For example description: 'An undocumented response body.'

bajtos · 2018-09-10T08:29:53Z

packages/openapi-v3/src/controller-spec.ts

+    const TS_TYPE_KEY = 'x-ts-type';
+
+    for (const code in operationSpec.responses) {
+      for (const c in operationSpec.responses[code].content) {


IIRC, content is an optional property. Please add a test to verify what happens when the OAI spec does not provide any value for content, and fix the implementation if needed.

content is an optional property and definitely took me a minute to realize what was happening here since I coded it.

The for (const c in operationSpec.responses[code].content) loop is only entered if the operationSpec.responses[code] object had a content property. If the property is not defined, the loop is never entered as there are no properties on undefined -- making the implementation correct and sound.

I also verified this behaviour on this page: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in

By changing the code to be as follows:

var string1 = ""; var object1 = {a: 1, b: 2, c: 3}; // var object1 = {a: 1, b: 2, c: 3, d: {x:1, y:2}}; for (var property1 in object1.d) { console.log('in loop') string1 = string1 + property1; } console.log(string1); // expected output: "123"

The above will log "" as is and uncommenting line 3 should give "xy" after 2x in loop

bajtos · 2018-09-10T08:30:15Z

packages/openapi-v3/src/controller-spec.ts

+    for (const code in operationSpec.responses) {
+      for (const c in operationSpec.responses[code].content) {
+        debug('  evaluating response code %s with content: %o', code, c);
+        const tsType = operationSpec.responses[code].content[c][TS_TYPE_KEY];


Please extract operationSpec.responses[code].content[c] into a shared variable to avoid repetition.

bajtos · 2018-09-10T08:34:25Z

packages/openapi-v3/test/integration/controller-spec.integration.ts

+            responses: {
+              '200': {
+                description: 'Return value of FooController.create',
+              },


Interesting. Is this the test verifying how a response status with no content is handled?

IIRC, content is an optional property. Please add a test to verify what happens when the OAI spec does not provide any value for content, and fix the implementation if needed.

It makes me wonder why this test passes, when the code processing the schema is not checking whether .content is set. Are we filling the missing content property somewhere else?

See #1655 (comment)

bajtos

LGTM 👍

Please clean up the commit history before landing.

Co-Authored-By: Raymond Feng <[email protected]>

virkt25 requested review from bajtos and raymondfeng as code owners August 29, 2018 16:37

virkt25 self-assigned this Aug 29, 2018

virkt25 added this to the August Milestone milestone Aug 29, 2018

virkt25 requested a review from a team August 29, 2018 16:38

raymondfeng reviewed Aug 29, 2018

View reviewed changes

jannyHou reviewed Aug 29, 2018

View reviewed changes

bajtos mentioned this pull request Aug 29, 2018

Monthly Milestone - August 2018 🍉⛱ #1581

Closed

31 tasks

bajtos mentioned this pull request Aug 30, 2018

Unable to Explore API (generated OpenAPI spec is not valid) #1531

Closed

bajtos suggested changes Aug 30, 2018

View reviewed changes

bajtos modified the milestones: August Milestone, September Milestone Sep 3, 2018

bajtos mentioned this pull request Sep 4, 2018

Monthly Milestone - September 2018 🌾🌽🍂 #1653

Closed

20 tasks

virkt25 force-pushed the openapi-responses branch from 0dbf2e1 to 4391523 Compare September 7, 2018 03:44

bajtos suggested changes Sep 10, 2018

View reviewed changes

virkt25 force-pushed the openapi-responses branch from 4391523 to c27b10b Compare September 12, 2018 03:07

bajtos approved these changes Sep 13, 2018

View reviewed changes

virkt25 force-pushed the openapi-responses branch 2 times, most recently from 7534e00 to eda80ca Compare September 13, 2018 14:52

virkt25 and others added 2 commits September 13, 2018 13:56

feat(openapi-v3): add support for openapi responses

a2f6c44

Co-Authored-By: Raymond Feng <[email protected]>

fix(cli): update template with responses object

7d42f54

virkt25 added 3 commits September 13, 2018 13:56

refactor(boot): import decorators from @loopback/rest

d1ae573

fix(example-todo): add responses to controllers

0050b1b

fix(example-todo-list): add responses to controllers

f9309a5

virkt25 force-pushed the openapi-responses branch from eda80ca to f9309a5 Compare September 13, 2018 17:56

virkt25 merged commit bf32971 into master Sep 13, 2018

virkt25 deleted the openapi-responses branch September 13, 2018 19:02

		@@ -0,0 +1,100 @@
		// Copyright IBM Corp. 2018. All Rights Reserved.


		debug(' decorated responses for method %s: %o', op, responses);

		const defaultResponse = {

feat(openapi-v3): responses decorator #1655

feat(openapi-v3): responses decorator #1655

Conversation

virkt25 commented Aug 29, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Aug 30, 2018 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Aug 30, 2018

raymondfeng commented Aug 30, 2018

bajtos commented Aug 31, 2018

raymondfeng commented Aug 31, 2018

virkt25 commented Sep 7, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

virkt25 commented Aug 29, 2018 •

edited

Loading

bajtos commented Aug 30, 2018 •

edited

Loading

bajtos Aug 30, 2018 •

edited

Loading

virkt25 Sep 12, 2018 •

edited

Loading