From 07faf4d2706c061af66e3f05e3b7df744ce0454c Mon Sep 17 00:00:00 2001 From: jannyHou Date: Mon, 29 Jul 2019 15:05:44 -0400 Subject: [PATCH 1/5] feat: simplify request body decorator --- packages/openapi-v3/_spike-request-body.md | 105 +++++++++ .../request-body.decorator.unit.ts | 16 +- .../request-body.spike.decorator.unit.ts | 223 ++++++++++++++++++ packages/openapi-v3/src/controller-spec.ts | 7 +- packages/openapi-v3/src/decorators/index.ts | 1 + .../src/decorators/request-body.decorator.ts | 8 +- .../request-body.spike.decorator.ts | 139 +++++++++++ packages/openapi-v3/src/types.ts | 2 + 8 files changed, 489 insertions(+), 12 deletions(-) create mode 100644 packages/openapi-v3/_spike-request-body.md create mode 100644 packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.spike.decorator.unit.ts create mode 100644 packages/openapi-v3/src/decorators/request-body.spike.decorator.ts diff --git a/packages/openapi-v3/_spike-request-body.md b/packages/openapi-v3/_spike-request-body.md new file mode 100644 index 000000000000..73882db94077 --- /dev/null +++ b/packages/openapi-v3/_spike-request-body.md @@ -0,0 +1,105 @@ +## Improve the UX of @requestBody() + +The original discussion is tracked in issue +[Spike: simplify requestBody annotation with schema options](https://github.com/strongloop/loopback-next/issues/2654). + +The current @requestBody() can only + +- takes in an entire request body specification with very nested media type + objects or +- generate the schema inferred from the parameter type + +To simplify the signature, this spike PR introduces two more parameters +`modelCtor` and `schemaOptions` to configure the schema. The new decorator +`requestBody2()`(let's discuss a better name later, see section +[Naming](#Naming) is written in file 'request-body.spike.decorator.ts' + +### Signature + +The new decorator's signature is +`@requestBody2(spec, modelCtor, schemaOptions)`. + +```ts +export function requestBody2( + specOrModelOrOptions?: Partial | Function | SchemaOptions, + modelOrOptions?: Function | SchemaOptions, + schemaOptions?: SchemaOptions, +) { + // implementation +} +``` + +All the 3 parameters are optional, therefore in the PoC, the real implementation +are in `_requestBody2()`, `requestBody2()` is a wrapper that resolves different +combination of the parameters. + +_Please note TypeScript doesn't support function overloading with different +number of parameters(like requestBody(spec), requestBody(model, options)). +Therefore we have to create a wrapper function to resolve different signatures +from the caller_ + +My PoC PR only adds 2 unit tests for different signatures, the real +implementation should test all the combinations. + +### Create - exclude properties + +Take "Create a new product with excluded properties" as an example: + +```ts +// The decorator takes in the option without having a nested content object +class MyController1 { + @post('/Product') + create( + @requestBody2( + // Provide the description as before + { + description: 'Create a product', + required: true, + }, + // Using advanced ts types like `Exclude<>`, `Partial<>` results in + // `MetadataInspector.getDesignTypeForMethod(target, member)` only + // returns `Object` as the param type, which loses the model type's info. + // Therefore user must provide the model type in options. + Product, + // Provide the options that configure your schema + { + // The excluded properties + exclude: ['id'], + }, + ) + product: Exclude, + ) {} +} +``` + +### Update - partial properties + +```ts +class MyController2 { + @put('/Product') + update( + @requestBody2( + {description: 'Update a product', required: true}, + Product, + {partial: true}, + ) + product: Partial, + ) {} +} +``` + +## Naming + +From @jannyHou: I think we can keep the name `requestBody` unchanged. I enabled +the existing `@requestBody()`'s tests but applied `requestBody2()` decorator, +all tests pass, which means there is no breaking change. + +## Follow-up Stories + +_I will create stories if we agree on the plan_ + +- [ ] Modify the current `@requestBody()` according to the spike code, pass + existing tests across all repos. +- [ ] Add more unit tests to verify all the signatures. +- [ ] Upgrade examples to provide the descriptive configs(model, options) using + the new decorator. diff --git a/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.decorator.unit.ts b/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.decorator.unit.ts index 1cca73f2fbd1..910fd0c97742 100644 --- a/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.decorator.unit.ts +++ b/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.decorator.unit.ts @@ -3,7 +3,7 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {post, requestBody, getControllerSpec} from '../../../..'; +import {post, requestBody2, getControllerSpec} from '../../../..'; import {expect} from '@loopback/testlab'; import {model, property} from '@loopback/repository'; @@ -16,7 +16,7 @@ describe('requestBody decorator', () => { }; class MyController { @post('/greeting') - greet(@requestBody(requestSpec) name: string) {} + greet(@requestBody2(requestSpec) name: string) {} } const requestBodySpec = getControllerSpec(MyController).paths[ @@ -35,7 +35,7 @@ describe('requestBody decorator', () => { }; class MyController { @post('/greeting') - greet(@requestBody(requestSpec) name: string) {} + greet(@requestBody2(requestSpec) name: string) {} } const requestBodySpec = getControllerSpec(MyController).paths[ @@ -60,7 +60,7 @@ describe('requestBody decorator', () => { class MyController { @post('/MyModel') createMyModel( - @requestBody({content: {'application/text': {}}}) inst: MyModel, + @requestBody2({content: {'application/text': {}}}) inst: MyModel, ) {} } @@ -81,7 +81,9 @@ describe('requestBody decorator', () => { class MyController { @post('/MyModel') - createMyModel(@requestBody({content: expectedContent}) inst: MyModel) {} + createMyModel( + @requestBody2({content: expectedContent}) inst: MyModel, + ) {} } const requestBodySpec = getControllerSpec(MyController).paths['/MyModel'][ @@ -102,7 +104,7 @@ describe('requestBody decorator', () => { class MyController { @post('/MyModel') createMyModel( - @requestBody({content: expectedContent}) inst: Partial, + @requestBody2({content: expectedContent}) inst: Partial, ) {} } @@ -115,7 +117,7 @@ describe('requestBody decorator', () => { it('reports error if more than one requestBody are found for the same method', () => { class MyController { @post('/greeting') - greet(@requestBody() name: string, @requestBody() foo: number) {} + greet(@requestBody2() name: string, @requestBody2() foo: number) {} } expect(() => getControllerSpec(MyController)).to.throwError( /An operation should only have one parameter decorated by @requestBody/, diff --git a/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.spike.decorator.unit.ts b/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.spike.decorator.unit.ts new file mode 100644 index 000000000000..0c2f0b360d08 --- /dev/null +++ b/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.spike.decorator.unit.ts @@ -0,0 +1,223 @@ +// Copyright IBM Corp. 2019. All Rights Reserved. +// Node module: @loopback/openapi-v3 +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import { + belongsTo, + Entity, + hasMany, + model, + property, +} from '@loopback/repository'; +import {expect} from '@loopback/testlab'; +import {getControllerSpec, post, put} from '../../../..'; +import {requestBody2} from '../../../../decorators/request-body.spike.decorator'; + +describe('spike - requestBody decorator', () => { + context('CRUD', () => { + @model() + class Product extends Entity { + @property({ + type: 'string', + }) + name: string; + @belongsTo(() => Category) + categoryId: number; + + constructor(data?: Partial) { + super(data); + } + } + + /** + * Navigation properties of the Product model. + */ + interface ProductRelations { + category?: CategoryWithRelations; + } + /** + * Product's own properties and navigation properties. + */ + type ProductWithRelations = Product & ProductRelations; + + @model() + class Category extends Entity { + @hasMany(() => Product) + products?: Product[]; + } + /** + * Navigation properties of the Category model. + */ + interface CategoryRelations { + products?: ProductWithRelations[]; + } + /** + * Category's own properties and navigation properties. + */ + type CategoryWithRelations = Category & CategoryRelations; + + it('create - generates schema with excluded properties', () => { + class MyController1 { + @post('/Product') + create( + @requestBody2( + {description: 'Create a product', required: true}, + Product, + {exclude: ['id']}, + ) + product: Exclude, + ) {} + } + + const spec1 = getControllerSpec(MyController1); + + const requestBodySpecForCreate = + spec1.paths['/Product']['post'].requestBody; + + const referenceSchema = spec1.components!.schemas![ + 'ProductExcluding[id]' + ]; + + expect(requestBodySpecForCreate).to.have.properties({ + description: 'Create a product', + required: true, + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/ProductExcluding[id]', + }, + }, + }, + }); + + // The feature that generates schemas according to + // different options is TO BE DONE and out of the + // scope of this spike, so that the schema `PartialProduct` + // here is still the same as `Product` + expect(referenceSchema).to.have.properties({ + title: 'ProductExcluding[id]', + properties: { + categoryId: {type: 'number'}, + name: {type: 'string'}, + }, + }); + }); + + it('update - generates schema with partial properties', () => { + class MyController2 { + @put('/Product') + update( + @requestBody2( + {description: 'Update a product', required: true}, + Product, + {partial: true}, + ) + product: Partial, + ) {} + } + + const spec2 = getControllerSpec(MyController2); + + const requestBodySpecForCreate = + spec2.paths['/Product']['put'].requestBody; + + const referenceSchema = spec2.components!.schemas!.ProductPartial; + + expect(requestBodySpecForCreate).to.have.properties({ + description: 'Update a product', + required: true, + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/ProductPartial', + }, + }, + }, + }); + + // The feature that generates schemas according to + // different options is TO BE DONE and out of the + // scope of this spike, so that the schema `PartialProduct` + // here is still the same as `Product` + expect(referenceSchema).to.have.properties({ + title: 'ProductPartial', + properties: { + categoryId: {type: 'number'}, + name: {type: 'string'}, + }, + }); + }); + }); + context( + 'different signatures(More tests TBD in the real implementation)', + () => { + @model() + class Test extends Entity { + @property({ + type: 'string', + }) + name: string; + constructor(data?: Partial) { + super(data); + } + } + it('default', () => { + class TestController { + @post('/Test') + create(@requestBody2() test: Test) {} + } + + const testSpec1 = getControllerSpec(TestController); + + const requestBodySpecForCreate = + testSpec1.paths['/Test']['post'].requestBody; + expect(requestBodySpecForCreate).to.have.properties({ + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/Test', + }, + }, + }, + }); + + const referenceSchema = testSpec1.components!.schemas!.Test; + expect(referenceSchema).to.have.properties({ + title: 'Test', + properties: { + name: {type: 'string'}, + }, + }); + }); + it('omits the 1st parameter', () => { + class TestController { + @post('/Test') + create(@requestBody2(Test, {partial: true}) test: Partial) {} + } + + const testSpec1 = getControllerSpec(TestController); + + const requestBodySpecForCreate = + testSpec1.paths['/Test']['post'].requestBody; + expect(requestBodySpecForCreate).to.have.properties({ + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/TestPartial', + }, + }, + }, + }); + + const referenceSchema = testSpec1.components!.schemas!.TestPartial; + expect(referenceSchema).to.have.properties({ + title: 'TestPartial', + properties: { + name: {type: 'string'}, + }, + }); + }); + }, + ); +}); diff --git a/packages/openapi-v3/src/controller-spec.ts b/packages/openapi-v3/src/controller-spec.ts index 8f236980f9cb..2bb4ad6488db 100644 --- a/packages/openapi-v3/src/controller-spec.ts +++ b/packages/openapi-v3/src/controller-spec.ts @@ -59,6 +59,10 @@ export interface RestEndpoint { export const TS_TYPE_KEY = 'x-ts-type'; +export function isComplexType(ctor: Function): Boolean { + return !_.includes([String, Number, Boolean, Array, Object], ctor); +} + /** * Build the api spec from class and method level decorations * @param constructor - Controller class @@ -221,9 +225,6 @@ function resolveControllerSpec(constructor: Function): ControllerSpec { ); const paramTypes = opMetadata.parameterTypes; - const isComplexType = (ctor: Function) => - !_.includes([String, Number, Boolean, Array, Object], ctor); - for (const p of paramTypes) { if (isComplexType(p)) { generateOpenAPISchema(spec, p); diff --git a/packages/openapi-v3/src/decorators/index.ts b/packages/openapi-v3/src/decorators/index.ts index 947da4a6f0e4..a688e1d77a9b 100644 --- a/packages/openapi-v3/src/decorators/index.ts +++ b/packages/openapi-v3/src/decorators/index.ts @@ -7,3 +7,4 @@ export * from './api.decorator'; export * from './operation.decorator'; export * from './parameter.decorator'; export * from './request-body.decorator'; +export * from './request-body.spike.decorator'; diff --git a/packages/openapi-v3/src/decorators/request-body.decorator.ts b/packages/openapi-v3/src/decorators/request-body.decorator.ts index 4bae7cbfdfa5..ecbf4350dc02 100644 --- a/packages/openapi-v3/src/decorators/request-body.decorator.ts +++ b/packages/openapi-v3/src/decorators/request-body.decorator.ts @@ -8,10 +8,14 @@ import * as _ from 'lodash'; import {inspect} from 'util'; import {resolveSchema} from '../generate-schema'; import {OAI3Keys} from '../keys'; -import {ReferenceObject, RequestBodyObject, SchemaObject} from '../types'; +import { + ReferenceObject, + RequestBodyObject, + SchemaObject, + REQUEST_BODY_INDEX, +} from '../types'; const debug = require('debug')('loopback:openapi3:metadata:requestbody'); -export const REQUEST_BODY_INDEX = 'x-parameter-index'; /** * Describe the request body of a Controller method parameter. diff --git a/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts b/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts new file mode 100644 index 000000000000..604f7a95ddba --- /dev/null +++ b/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts @@ -0,0 +1,139 @@ +// Copyright IBM Corp. 2018,2019. All Rights Reserved. +// Node module: @loopback/openapi-v3 +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import {MetadataInspector, ParameterDecoratorFactory} from '@loopback/context'; +import {JsonSchemaOptions} from '@loopback/repository-json-schema'; +import * as _ from 'lodash'; +import {inspect} from 'util'; +import {isComplexType, getModelSchemaRef} from '../controller-spec'; +import {resolveSchema} from '../generate-schema'; +import {OAI3Keys} from '../keys'; +import {RequestBodyObject, REQUEST_BODY_INDEX, SchemaObject} from '../types'; + +const debug = require('debug')('loopback:openapi3:metadata:requestbody'); + +function isRequestBodySpec( + spec: Partial | JsonSchemaOptions, +): spec is Partial { + return ( + (spec as Partial).description !== undefined || + (spec as Partial).required !== undefined || + (spec as Partial).content !== undefined + ); +} + +export function requestBody2( + specOrModelOrOptions?: + | Partial + | Function + | JsonSchemaOptions, + modelOrOptions?: Function | JsonSchemaOptions, + schemaOptions?: JsonSchemaOptions, +) { + if (typeof specOrModelOrOptions == 'function') { + // omit the 1st param + // @requestBody(modelCtor, schemaOptions) + if (modelOrOptions && typeof modelOrOptions == 'object') + return _requestBody2( + {}, + specOrModelOrOptions, + modelOrOptions as JsonSchemaOptions, + ); + // @requestBody(modelCtor) + return _requestBody2({}, specOrModelOrOptions); + } else if (specOrModelOrOptions && isRequestBodySpec(specOrModelOrOptions)) { + // 1st param is spec + if (modelOrOptions && typeof modelOrOptions == 'object') + // omit the 2nd param + // @requestBody(spec, schemaOptions) + return _requestBody2(specOrModelOrOptions, undefined, modelOrOptions); + // @requestBody(spec) + // @requestBody(spec, modelCtor) + // @requestBody(spec, modelCtor, schemaOptions) + return _requestBody2(specOrModelOrOptions, modelOrOptions, schemaOptions); + } else if (specOrModelOrOptions !== undefined) { + // omit 1st and 2nd params + // @requestBody(schemaOptions) + return _requestBody2( + {}, + undefined, + specOrModelOrOptions as JsonSchemaOptions, + ); + } else { + // no params + // @requestBody() + return _requestBody2(); + } +} + +// `name` is provided to avoid generating the same schema +function _requestBody2( + requestBodySpecification?: Partial, + modelCtor?: Function, + schemaOptions?: JsonSchemaOptions, +) { + return function(target: object, member: string, index: number) { + debug('@newRequestBody() on %s.%s', target.constructor.name, member); + debug(' parameter index: %s', index); + /* istanbul ignore if */ + if (debug.enabled) + debug( + ' schemaOptions: %s', + inspect(requestBodySpecification, {depth: null}), + ); + + // Use 'application/json' as default content if `requestBody` is undefined + const requestBodySpec = Object.assign( + {}, + requestBodySpecification || {content: {}}, + ); + + if (_.isEmpty(requestBodySpec.content)) + requestBodySpec.content = {'application/json': {}}; + + // Get the design time method parameter metadata + const methodSig = MetadataInspector.getDesignTypeForMethod(target, member); + const paramTypes = (methodSig && methodSig.parameterTypes) || []; + + const paramType = paramTypes[index]; + + // // preserve backward compatibility + // if (modelCtor) + // schemaOptions = Object.assign({}, schemaOptions); + + // Assumption: the paramType is always the type to be configured + let schema: SchemaObject; + + if (isComplexType(modelCtor || paramType)) { + schema = getModelSchemaRef(modelCtor || paramType, schemaOptions); + } else { + schema = resolveSchema(modelCtor || paramType); + } + + /* istanbul ignore if */ + if (debug.enabled) + debug(' inferred schema: %s', inspect(schema, {depth: null})); + requestBodySpec.content = _.mapValues(requestBodySpec.content, c => { + if (!c.schema) { + c.schema = schema; + } + return c; + }); + + // The default position for request body argument is 0 + // if not, add extension 'x-parameter-index' to specify the position + if (index !== 0) { + requestBodySpec[REQUEST_BODY_INDEX] = index; + } + + /* istanbul ignore if */ + if (debug.enabled) + debug(' final spec: ', inspect(requestBodySpec, {depth: null})); + ParameterDecoratorFactory.createDecorator( + OAI3Keys.REQUEST_BODY_KEY, + requestBodySpec as RequestBodyObject, + )(target, member, index); + }; +} diff --git a/packages/openapi-v3/src/types.ts b/packages/openapi-v3/src/types.ts index c420f4fbc7ec..1370de5d79f4 100644 --- a/packages/openapi-v3/src/types.ts +++ b/packages/openapi-v3/src/types.ts @@ -28,3 +28,5 @@ export function createEmptyApiSpec(): OpenApiSpec { servers: [{url: '/'}], }; } + +export const REQUEST_BODY_INDEX = 'x-parameter-index'; From ea24f68c841a6be2b9d90b83014fc73b277e06c9 Mon Sep 17 00:00:00 2001 From: jannyHou Date: Wed, 31 Jul 2019 10:26:56 -0400 Subject: [PATCH 2/5] style: fix lint --- packages/openapi-v3/_spike-request-body.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/openapi-v3/_spike-request-body.md b/packages/openapi-v3/_spike-request-body.md index 73882db94077..4958496ec828 100644 --- a/packages/openapi-v3/_spike-request-body.md +++ b/packages/openapi-v3/_spike-request-body.md @@ -78,11 +78,9 @@ class MyController1 { class MyController2 { @put('/Product') update( - @requestBody2( - {description: 'Update a product', required: true}, - Product, - {partial: true}, - ) + @requestBody2({description: 'Update a product', required: true}, Product, { + partial: true, + }) product: Partial, ) {} } From cd986547faa1150d9c5838f0561255ae2376db48 Mon Sep 17 00:00:00 2001 From: jannyHou Date: Wed, 31 Jul 2019 13:22:06 -0400 Subject: [PATCH 3/5] fixup!: remove unused variables --- .../src/decorators/request-body.spike.decorator.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts b/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts index 604f7a95ddba..ec14c25a3178 100644 --- a/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts +++ b/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts @@ -7,7 +7,7 @@ import {MetadataInspector, ParameterDecoratorFactory} from '@loopback/context'; import {JsonSchemaOptions} from '@loopback/repository-json-schema'; import * as _ from 'lodash'; import {inspect} from 'util'; -import {isComplexType, getModelSchemaRef} from '../controller-spec'; +import {getModelSchemaRef, isComplexType} from '../controller-spec'; import {resolveSchema} from '../generate-schema'; import {OAI3Keys} from '../keys'; import {RequestBodyObject, REQUEST_BODY_INDEX, SchemaObject} from '../types'; @@ -99,10 +99,6 @@ function _requestBody2( const paramType = paramTypes[index]; - // // preserve backward compatibility - // if (modelCtor) - // schemaOptions = Object.assign({}, schemaOptions); - // Assumption: the paramType is always the type to be configured let schema: SchemaObject; From 36b5443fc405f8a31f6374b2f31e8f332c6b1e56 Mon Sep 17 00:00:00 2001 From: jannyHou Date: Wed, 31 Jul 2019 20:41:14 -0400 Subject: [PATCH 4/5] fixup!: use overloading --- .../request-body.spike.decorator.unit.ts | 41 ++++++++++++++++ .../request-body.spike.decorator.ts | 47 ++++++++++++------- 2 files changed, 72 insertions(+), 16 deletions(-) diff --git a/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.spike.decorator.unit.ts b/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.spike.decorator.unit.ts index 0c2f0b360d08..053db72b0f0f 100644 --- a/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.spike.decorator.unit.ts +++ b/packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.spike.decorator.unit.ts @@ -18,6 +18,12 @@ describe('spike - requestBody decorator', () => { context('CRUD', () => { @model() class Product extends Entity { + @property({ + type: 'string', + id: true, + }) + id: string; + @property({ type: 'string', }) @@ -143,6 +149,7 @@ describe('spike - requestBody decorator', () => { expect(referenceSchema).to.have.properties({ title: 'ProductPartial', properties: { + id: {type: 'string'}, categoryId: {type: 'number'}, name: {type: 'string'}, }, @@ -190,6 +197,7 @@ describe('spike - requestBody decorator', () => { }, }); }); + it('omits the 1st parameter', () => { class TestController { @post('/Test') @@ -218,6 +226,39 @@ describe('spike - requestBody decorator', () => { }, }); }); + + // FIXIT: This test should fail + // The type check should detect the generic type `Random` + // is different from the second parameter `Test` + it('catches type conflict', () => { + class Random {} + class TestController { + @post('/Test') + create(@requestBody2({required: true}, Test) test: Test) {} + } + + const testSpec3 = getControllerSpec(TestController); + + const requestBodySpecForCreate = + testSpec3.paths['/Test']['post'].requestBody; + expect(requestBodySpecForCreate).to.have.properties({ + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/Test', + }, + }, + }, + }); + + const referenceSchema = testSpec3.components!.schemas!.Test; + expect(referenceSchema).to.have.properties({ + title: 'Test', + properties: { + name: {type: 'string'}, + }, + }); + }); }, ); }); diff --git a/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts b/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts index ec14c25a3178..b52f44b5d58f 100644 --- a/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts +++ b/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts @@ -7,7 +7,7 @@ import {MetadataInspector, ParameterDecoratorFactory} from '@loopback/context'; import {JsonSchemaOptions} from '@loopback/repository-json-schema'; import * as _ from 'lodash'; import {inspect} from 'util'; -import {getModelSchemaRef, isComplexType} from '../controller-spec'; +import {getModelSchemaRef, isComplexType} from '..'; import {resolveSchema} from '../generate-schema'; import {OAI3Keys} from '../keys'; import {RequestBodyObject, REQUEST_BODY_INDEX, SchemaObject} from '../types'; @@ -24,14 +24,38 @@ function isRequestBodySpec( ); } +// overload +export function requestBody2( + spec: Partial, + schemaOptions?: JsonSchemaOptions, +): (target: object, member: string, index: number) => void; + +// overload +export function requestBody2( + model: Function & {prototype: T}, + schemaOptions?: JsonSchemaOptions, +): (target: object, member: string, index: number) => void; + +// overload +export function requestBody2( + spec: Partial, + model: Function & {prototype: T}, + schemaOptions?: JsonSchemaOptions, +): (target: object, member: string, index: number) => void; + +// overload +export function requestBody2( + schemaOptions?: JsonSchemaOptions, +): (target: object, member: string, index: number) => void; + export function requestBody2( specOrModelOrOptions?: | Partial - | Function + | Function & {prototype: T} | JsonSchemaOptions, - modelOrOptions?: Function | JsonSchemaOptions, + modelOrOptions?: Function & {prototype: T} | JsonSchemaOptions, schemaOptions?: JsonSchemaOptions, -) { +): (target: object, member: string, index: number) => void { if (typeof specOrModelOrOptions == 'function') { // omit the 1st param // @requestBody(modelCtor, schemaOptions) @@ -69,11 +93,11 @@ export function requestBody2( } // `name` is provided to avoid generating the same schema -function _requestBody2( +export function _requestBody2( requestBodySpecification?: Partial, - modelCtor?: Function, + modelCtor?: Function & {prototype: T}, schemaOptions?: JsonSchemaOptions, -) { +): (target: object, member: string, index: number) => void { return function(target: object, member: string, index: number) { debug('@newRequestBody() on %s.%s', target.constructor.name, member); debug(' parameter index: %s', index); @@ -83,31 +107,24 @@ function _requestBody2( ' schemaOptions: %s', inspect(requestBodySpecification, {depth: null}), ); - // Use 'application/json' as default content if `requestBody` is undefined const requestBodySpec = Object.assign( {}, requestBodySpecification || {content: {}}, ); - if (_.isEmpty(requestBodySpec.content)) requestBodySpec.content = {'application/json': {}}; - // Get the design time method parameter metadata const methodSig = MetadataInspector.getDesignTypeForMethod(target, member); const paramTypes = (methodSig && methodSig.parameterTypes) || []; - const paramType = paramTypes[index]; - // Assumption: the paramType is always the type to be configured let schema: SchemaObject; - if (isComplexType(modelCtor || paramType)) { schema = getModelSchemaRef(modelCtor || paramType, schemaOptions); } else { schema = resolveSchema(modelCtor || paramType); } - /* istanbul ignore if */ if (debug.enabled) debug(' inferred schema: %s', inspect(schema, {depth: null})); @@ -117,13 +134,11 @@ function _requestBody2( } return c; }); - // The default position for request body argument is 0 // if not, add extension 'x-parameter-index' to specify the position if (index !== 0) { requestBodySpec[REQUEST_BODY_INDEX] = index; } - /* istanbul ignore if */ if (debug.enabled) debug(' final spec: ', inspect(requestBodySpec, {depth: null})); From 5877f0cc0201af59f2a9636a09753207c599ab26 Mon Sep 17 00:00:00 2001 From: jannyHou Date: Wed, 31 Jul 2019 21:33:00 -0400 Subject: [PATCH 5/5] fixup!: add more docs --- packages/openapi-v3/_spike-request-body.md | 106 +++++++++++++----- .../request-body.spike.decorator.ts | 22 ++-- 2 files changed, 93 insertions(+), 35 deletions(-) diff --git a/packages/openapi-v3/_spike-request-body.md b/packages/openapi-v3/_spike-request-body.md index 4958496ec828..5c3b7f71c48a 100644 --- a/packages/openapi-v3/_spike-request-body.md +++ b/packages/openapi-v3/_spike-request-body.md @@ -6,41 +6,84 @@ The original discussion is tracked in issue The current @requestBody() can only - takes in an entire request body specification with very nested media type - objects or -- generate the schema inferred from the parameter type + objects OR +- generates the schema inferred from the parameter type -To simplify the signature, this spike PR introduces two more parameters -`modelCtor` and `schemaOptions` to configure the schema. The new decorator -`requestBody2()`(let's discuss a better name later, see section -[Naming](#Naming) is written in file 'request-body.spike.decorator.ts' +It doesn't give people the convenience to configure the schema spec with `model` +and `schemaOptions` without defining them in a nested object. To simplify the +signature, this spike PR introduces two more parameters `model` and +`schemaOptions` to configure the schema. -### Signature +Please note the 2nd and 3rd parameters are only used to contribute the spec for +_default schema_(I will explain what default means with an example), any other +request body spec are still defined in the 1st parameter. -The new decorator's signature is -`@requestBody2(spec, modelCtor, schemaOptions)`. +See the example from the swagger official website: +https://swagger.io/docs/specification/describing-request-body/ + +- `application/json` and `application/xml` share the same schema + `#/components/schemas/Pet` +- `application/x-www-form-urlencoded` uses another schema + `#/components/schemas/PetForm' +- `text/plain` receives a string: `type: string` + +The new decorator allows + +- a default schema (generated from the 2nd and 3rd params, should be `Pet` in + this case) _and_ +- custom schemas (provided by the 1st parameter: partial spec, `PetForm` and + string schema) + +so that developers don't need to repeat defining the same schema in the 1st +parameter. + +The logic of the new decorator is: + +- The 1st parameter contains the pure OpenAPI request body spec +- Other parameters contribute the default schema' config, `model`, `options`. +- The decorator: + - first reads schema config + - then generate default schema spec accordingly + - if any content type is missing `schema`, then assign the default schema to + it(Our current implementation is already doing it, see + [code](https://github.com/strongloop/loopback-next/blob/master/packages/openapi-v3/src/decorators/request-body.decorator.ts#L103-L107)) + +ANY OTHER PARTS of the spec are read from the 1st parameter, the decorator only +generate and merge the default `schema`. We can implement helpers/builders for +developers to build the entire spec, like ```ts -export function requestBody2( - specOrModelOrOptions?: Partial | Function | SchemaOptions, - modelOrOptions?: Function | SchemaOptions, - schemaOptions?: SchemaOptions, -) { - // implementation -} +requestBodyBuilder + .addContent('application/x-www-form-urlencoded') + .withSchema(model, options) + .withExample(exampleSpecs); ``` +The new decorator `requestBody2()`(let's discuss a better name later, see +section [Naming](#Naming) is written in file 'request-body.spike.decorator.ts' + +### Signature + +The new decorator's signature is +`@requestBody2(spec, modelCtor, schemaOptions)`. + All the 3 parameters are optional, therefore in the PoC, the real implementation are in `_requestBody2()`, `requestBody2()` is a wrapper that resolves different combination of the parameters. -_Please note TypeScript doesn't support function overloading with different -number of parameters(like requestBody(spec), requestBody(model, options)). -Therefore we have to create a wrapper function to resolve different signatures -from the caller_ - My PoC PR only adds 2 unit tests for different signatures, the real implementation should test all the combinations. +```ts +export function _requestBody2( + specOrModelOrOptions?: Partial, + modelOrOptions?: Function & {prototype: T}, + schemaOptions?: SchemaOptions, +) { + // implementation +} +``` + ### Create - exclude properties Take "Create a new product with excluded properties" as an example: @@ -94,10 +137,21 @@ all tests pass, which means there is no breaking change. ## Follow-up Stories -_I will create stories if we agree on the plan_ - -- [ ] Modify the current `@requestBody()` according to the spike code, pass - existing tests across all repos. -- [ ] Add more unit tests to verify all the signatures. +- [ ] More discussion + + - The signature: + - `@requstBody(spec, model, options)` + - or `@requestBody(specWithOptions, model)` + - or `@requestBody(spec, getModelSchemaRef(model, options))` + - or any better signatures + - Which signatures should be disabled + +- [ ] Implementation + - Modify the current `@requestBody()` according to the spike code, pass + existing tests across all repos. + - Add more unit tests to verify all the signatures. + - Fix the type conflict detect, see test case 'catches type conflict', and + discussion in + [comment](https://github.com/strongloop/loopback-next/pull/3466/files#r308657211) - [ ] Upgrade examples to provide the descriptive configs(model, options) using the new decorator. diff --git a/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts b/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts index b52f44b5d58f..1bef2b1d2234 100644 --- a/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts +++ b/packages/openapi-v3/src/decorators/request-body.spike.decorator.ts @@ -58,24 +58,28 @@ export function requestBody2( ): (target: object, member: string, index: number) => void { if (typeof specOrModelOrOptions == 'function') { // omit the 1st param - // @requestBody(modelCtor, schemaOptions) + // @requestBody(model, schemaOptions) if (modelOrOptions && typeof modelOrOptions == 'object') return _requestBody2( {}, specOrModelOrOptions, modelOrOptions as JsonSchemaOptions, ); - // @requestBody(modelCtor) + // @requestBody(model) return _requestBody2({}, specOrModelOrOptions); } else if (specOrModelOrOptions && isRequestBodySpec(specOrModelOrOptions)) { // 1st param is spec if (modelOrOptions && typeof modelOrOptions == 'object') // omit the 2nd param // @requestBody(spec, schemaOptions) - return _requestBody2(specOrModelOrOptions, undefined, modelOrOptions); + return _requestBody2( + specOrModelOrOptions, + undefined, + modelOrOptions as JsonSchemaOptions, + ); // @requestBody(spec) - // @requestBody(spec, modelCtor) - // @requestBody(spec, modelCtor, schemaOptions) + // @requestBody(spec, model) + // @requestBody(spec, model, schemaOptions) return _requestBody2(specOrModelOrOptions, modelOrOptions, schemaOptions); } else if (specOrModelOrOptions !== undefined) { // omit 1st and 2nd params @@ -95,7 +99,7 @@ export function requestBody2( // `name` is provided to avoid generating the same schema export function _requestBody2( requestBodySpecification?: Partial, - modelCtor?: Function & {prototype: T}, + model?: Function & {prototype: T}, schemaOptions?: JsonSchemaOptions, ): (target: object, member: string, index: number) => void { return function(target: object, member: string, index: number) { @@ -120,10 +124,10 @@ export function _requestBody2( const paramType = paramTypes[index]; // Assumption: the paramType is always the type to be configured let schema: SchemaObject; - if (isComplexType(modelCtor || paramType)) { - schema = getModelSchemaRef(modelCtor || paramType, schemaOptions); + if (isComplexType(model || paramType)) { + schema = getModelSchemaRef(model || paramType, schemaOptions); } else { - schema = resolveSchema(modelCtor || paramType); + schema = resolveSchema(model || paramType); } /* istanbul ignore if */ if (debug.enabled)