-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[SPIKE] feat: simplify request body decorator #3398
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
## 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 a 2nd parameter `schemaOptions` to configure the schema. The new decorator `newRequestBody1` is written in file 'request-body.options1.decorator.ts' | ||
|
||
### Create - exclude properties | ||
|
||
Take "Create a new product with excluded properties" as an example: | ||
|
||
```ts | ||
// Provide the description as before | ||
const requestBodySpec = { | ||
description: 'Create a product', | ||
required: true, | ||
}; | ||
|
||
// Provide the options that configure your schema | ||
const excludeOptions = { | ||
// 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. | ||
[TS_TYPE_KEY]: Product, | ||
// Make sure you give the custom schema a unique schema name, | ||
// this name will be used as the reference name | ||
// like `$ref: '#components/schemas/ProductWithoutID'` | ||
schemaName: 'ProductWithoutID', | ||
// The excluded properties | ||
exclude: ['id'] | ||
} | ||
|
||
// The decorator takes in the option without having a nested content object | ||
class MyController1 { | ||
@post('/Product') | ||
create(@newRequestBody1( | ||
requestBodySpec, | ||
excludeOptions | ||
) product: Exclude<Product, ['id']>) { } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To follow-up on my comment above, I am proposing to replace class MyController1 {
@post('/Product')
create(@newRequestBody1(
requestBodySpec,
getModelSchemaRef(Product, {exclude: ['id']}),
) product: Exclude<Product, ['id']>) { }
} |
||
} | ||
``` | ||
|
||
### Update - partial properties | ||
|
||
```ts | ||
const requestSpecForUpdate = { | ||
description: 'Update a product', | ||
required: true, | ||
}; | ||
|
||
const partialOptions = { | ||
[TS_TYPE_KEY]: Product, | ||
schemaName: 'PartialProduct', | ||
partial: true | ||
} | ||
|
||
class MyController2 { | ||
@put('/Product') | ||
update(@newRequestBody1( | ||
requestSpecForUpdate, | ||
partialOptions | ||
) product: Partial<Product>) { } | ||
} | ||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
// 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 { TS_TYPE_KEY } from '../../../../controller-spec'; | ||
import { newRequestBody1 } from '../../../../decorators/request-body.option1.decorator'; | ||
|
||
describe.only('spike - requestBody decorator', () => { | ||
context('proposal 1', () => { | ||
@model() | ||
class Product extends Entity { | ||
@property({ | ||
type: 'string', | ||
}) | ||
name: string; | ||
@belongsTo(() => Category) | ||
categoryId: number; | ||
|
||
constructor(data?: Partial<Product>) { | ||
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', () => { | ||
const requestBodySpec = { | ||
description: 'Create a product', | ||
required: true, | ||
}; | ||
|
||
const excludeOptions = { | ||
[TS_TYPE_KEY]: Product, | ||
schemaName: 'ProductWithoutID', | ||
exclude: ['id'] | ||
} | ||
|
||
class MyController1 { | ||
@post('/Product') | ||
create(@newRequestBody1( | ||
requestBodySpec, | ||
excludeOptions | ||
) product: Exclude<Product, ['id']>) { } | ||
} | ||
|
||
const spec1 = getControllerSpec(MyController1) | ||
|
||
const requestBodySpecForCreate = spec1.paths[ | ||
'/Product' | ||
]['post'].requestBody; | ||
|
||
const referenceSchema = spec1.components!.schemas!.ProductWithoutID; | ||
|
||
expect(requestBodySpecForCreate).to.have.properties({ | ||
description: 'Create a product', | ||
required: true, | ||
content: { | ||
'application/json': { | ||
schema: { | ||
$ref: '#/components/schemas/ProductWithoutID' | ||
} | ||
} | ||
} | ||
}); | ||
|
||
// 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: 'ProductWithoutID', | ||
properties: { | ||
categoryId: { type: 'number' }, | ||
name: { type: 'string' } | ||
} | ||
}); | ||
}) | ||
|
||
it('update - generates schema with partial properties', () => { | ||
const requestSpecForUpdate = { | ||
description: 'Update a product', | ||
required: true, | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this approach, where we need one new variable ( Please update examples/todo/src/controllers/todo.controller.ts to show your proposal in practice. |
||
|
||
const partialOptions = { | ||
[TS_TYPE_KEY]: Product, | ||
schemaName: 'PartialProduct', | ||
partial: true | ||
} | ||
|
||
class MyController2 { | ||
@put('/Product') | ||
update(@newRequestBody1( | ||
requestSpecForUpdate, | ||
partialOptions | ||
) product: Partial<Product>) { } | ||
} | ||
|
||
const spec2 = getControllerSpec(MyController2) | ||
|
||
const requestBodySpecForCreate = spec2.paths[ | ||
'/Product' | ||
]['put'].requestBody; | ||
|
||
const referenceSchema = spec2.components!.schemas!.PartialProduct; | ||
|
||
expect(requestBodySpecForCreate).to.have.properties({ | ||
description: 'Update a product', | ||
required: true, | ||
content: { | ||
'application/json': { | ||
schema: { | ||
$ref: '#/components/schemas/PartialProduct' | ||
} | ||
} | ||
} | ||
}); | ||
|
||
// 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: 'PartialProduct', | ||
properties: { | ||
categoryId: { type: 'number' }, | ||
name: { type: 'string' } | ||
} | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,29 +3,14 @@ | |
// This file is licensed under the MIT License. | ||
// License text available at https://opensource.org/licenses/MIT | ||
|
||
import {DecoratorFactory, MetadataInspector} from '@loopback/context'; | ||
import { | ||
getJsonSchema, | ||
getJsonSchemaRef, | ||
JsonSchemaOptions, | ||
} from '@loopback/repository-json-schema'; | ||
import { DecoratorFactory, MetadataInspector } from '@loopback/context'; | ||
import { getJsonSchemaRef, JsonSchemaOptions } from '@loopback/repository-json-schema'; | ||
import * as _ from 'lodash'; | ||
import {resolveSchema} from './generate-schema'; | ||
import {jsonToSchemaObject, SchemaRef} from './json-to-schema'; | ||
import {OAI3Keys} from './keys'; | ||
import { | ||
ComponentsObject, | ||
ISpecificationExtension, | ||
isReferenceObject, | ||
OperationObject, | ||
ParameterObject, | ||
PathObject, | ||
ReferenceObject, | ||
RequestBodyObject, | ||
ResponseObject, | ||
SchemaObject, | ||
SchemasObject, | ||
} from './types'; | ||
import { SchemaOptions } from './decorators/request-body.option1.decorator'; | ||
import { resolveSchema } from './generate-schema'; | ||
import { jsonToSchemaObject, SchemaRef } from './json-to-schema'; | ||
import { OAI3Keys } from './keys'; | ||
import { ComponentsObject, ISpecificationExtension, isReferenceObject, OperationObject, ParameterObject, PathObject, ReferenceObject, RequestBodyObject, ResponseObject, SchemaObject, SchemasObject } from './types'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The formatting is broken. |
||
|
||
const debug = require('debug')('loopback:openapi3:metadata:controller-spec'); | ||
|
||
|
@@ -74,7 +59,7 @@ function resolveControllerSpec(constructor: Function): ControllerSpec { | |
debug(' using class-level spec defined via @api()', spec); | ||
spec = DecoratorFactory.cloneDeep(spec); | ||
} else { | ||
spec = {paths: {}}; | ||
spec = { paths: {} }; | ||
} | ||
|
||
let endpoints = | ||
|
@@ -183,7 +168,7 @@ function resolveControllerSpec(constructor: Function): ControllerSpec { | |
|
||
const content = requestBody.content || {}; | ||
for (const mediaType in content) { | ||
processSchemaExtensions(spec, content[mediaType].schema); | ||
processSchemaExtensionsForRequestBody(spec, content[mediaType].schema); | ||
} | ||
} | ||
} | ||
|
@@ -271,31 +256,67 @@ function processSchemaExtensions( | |
} | ||
} | ||
|
||
function processSchemaExtensionsForRequestBody( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is I am reluctant to have different levels of support for schema extension depending on where the schema is coming from ( Can we find a way how leverage There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Reverted these change. |
||
spec: ControllerSpec, | ||
schema?: SchemaObject | (ReferenceObject & ISpecificationExtension), | ||
) { | ||
debug(' processing extensions in schema: %j', schema); | ||
if (!schema) return; | ||
|
||
assignRelatedSchemas(spec, schema.definitions); | ||
delete schema.definitions; | ||
|
||
const tsType = schema.options && schema.options[TS_TYPE_KEY]; | ||
debug(' %s => %o', TS_TYPE_KEY, tsType); | ||
if (tsType) { | ||
|
||
if (!schema.options.isVisited) schema = resolveSchema(tsType, schema); | ||
if (schema.$ref) generateOpenAPISchema(spec, tsType, schema.options); | ||
|
||
// We don't want a Function type in the final spec. | ||
delete schema.options; | ||
return; | ||
} | ||
if (schema.type === 'array') { | ||
processSchemaExtensionsForRequestBody(spec, schema.items); | ||
} else if (schema.type === 'object') { | ||
if (schema.properties) { | ||
for (const p in schema.properties) { | ||
processSchemaExtensionsForRequestBody(spec, schema.properties[p]); | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Generate json schema for a given x-ts-type | ||
* @param spec - Controller spec | ||
* @param tsType - TS Type | ||
*/ | ||
function generateOpenAPISchema(spec: ControllerSpec, tsType: Function) { | ||
function generateOpenAPISchema(spec: ControllerSpec, tsType: Function, options?: SchemaOptions) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this change weird. Could you please help me to better understand your intent here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I am reading through the code, IIUC, IIUC, you are trying to support There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep |
||
if (!spec.components) { | ||
spec.components = {}; | ||
} | ||
if (!spec.components.schemas) { | ||
spec.components.schemas = {}; | ||
} | ||
if (tsType.name in spec.components.schemas) { | ||
const schemaName = options && options.schemaName || tsType.name | ||
if (schemaName in spec.components.schemas) { | ||
// Preserve user-provided definitions | ||
debug(' skipping type %j as already defined', tsType.name || tsType); | ||
return; | ||
} | ||
const jsonSchema = getJsonSchema(tsType); | ||
const openapiSchema = jsonToSchemaObject(jsonSchema); | ||
|
||
const openapiSchema = getModelSchemaRef(tsType, options); | ||
// const jsonSchema = getJsonSchema(tsType); | ||
// const openapiSchema = jsonToSchemaObject(jsonSchema); | ||
delete openapiSchema.definitions.options; | ||
|
||
assignRelatedSchemas(spec, openapiSchema.definitions); | ||
delete openapiSchema.definitions; | ||
|
||
debug(' defining schema for %j: %j', tsType.name, openapiSchema); | ||
spec.components.schemas[tsType.name] = openapiSchema; | ||
// debug(' defining schema for %j: %j', tsType.name, openapiSchema); | ||
// spec.components.schemas[tsType.name] = openapiSchema; | ||
} | ||
|
||
/** | ||
|
@@ -337,7 +358,7 @@ export function getControllerSpec(constructor: Function): ControllerSpec { | |
let spec = MetadataInspector.getClassMetadata<ControllerSpec>( | ||
OAI3Keys.CONTROLLER_SPEC_KEY, | ||
constructor, | ||
{ownMetadataOnly: true}, | ||
{ ownMetadataOnly: true }, | ||
); | ||
if (!spec) { | ||
spec = resolveControllerSpec(constructor); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am quite unhappy about this design.
It's seems to be based on the old
ts-type
approach that we are slowly moving away from (see e.g. #3402) and usinggetModelSchemaRef
instead.Let's focus on a solution that would leverage
getModelSchemaRef
and allow developers using different ORM frameworks like TypeORM to use their own helper building model-schemas from their model definitions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos It's ok, I was about to switch to your proposal which uses
getModelSchemaRef
to generate the OpenAPI schemabut now I am wondering, what is the main purpose of SIMPLIFYING the decorator? To "avoid using very nested object to provide the declarative schema config(which is specific to loopback 4 core) - option1" or to "leverage the schema builder to generate the OAI3 schema that any ORM can consume - option2"?
My current design serves option1, but if our purpose is option2, I will try @bajtos's proposal.
cc @strongloop/loopback-maintainers any thought/preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the minimum way to have a valid
requestBody
spec? I assume the new decorator is designed to allow so:Product
to build the schemaexclude
oroptional
to tweak the schemaI would expect something like the following to represent
Exclude<Product, ['id']>
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raymondfeng see the spec definition
At least we need two more fields:
description
: currently requires user to provide in@requestBody
, but in the future it can be read from model's metadatarequired
: a boolean valueAnd the nested
content
object contains more fields likeexamples
besidesschema
, see comment #3398 (comment), the discussion of thecontent
spec is at the bottom.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created that user story with the intention to find a more ergonomic solution for describing request bodies. IMO, controllers methods using the default body parser should not need to repeat the fact that the request body accepts content of type
application/json
.See e.g. what Raymond proposed in his comment:
and what I proposed in elsewhere in this pull request:
The examples show a reasonably complex case. It may be not obvious that they are also making the default use case super simple - and that was the original goal!
In the future, we may add support for other content-types, e.g. XML or JSON API. It would be nice if application developers didn't need to edit all their
@requestBody
calls to add those new content types when they become supported. This isn't required, just something to be aware of.