[SPIKE] feat: simplify request body decorator #3398

jannyHou · 2019-07-19T17:08:13Z

connect to #2654

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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 · 2019-07-19T20:24:03Z

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

+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';


The formatting is broken.

raymondfeng · 2019-07-19T20:29:14Z

packages/openapi-v3/src/decorators/request-body.option1.decorator.ts

+  // generated schema as `schemaName`
+  [TS_TYPE_KEY]?: Function,
+  isVisited?: boolean,
+}


Maybe the following is better:

export interface SchemaOptions extends JsonSchemaOptions { tsType: { type: Function, exclude?: string[], optional?: string[], } isVisited?: boolean, }

I agree 👍

Eventually I am able to use JsonSchemaOptions without creating a new interface/type. So removed it in the new PR #3466 :)

bajtos

Thank you @jannyHou for starting this work ❤️

I appreciate the spike.md document with a high-level overview of the proposal 👍

Besides my comments below, I think the important part of this spike is to find a good name for newRequestBody1�.

bajtos · 2019-07-22T09:19:29Z

packages/openapi-v3/spike.md

+  schemaName: 'ProductWithoutID',
+  // The excluded properties
+  exclude: ['id']
+}


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 using getModelSchemaRef 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.

@bajtos It's ok, I was about to switch to your proposal which uses getModelSchemaRef to generate the OpenAPI schema

class MyController1 { @post('/Product') create(@newRequestBody1( requestBodySpec, getModelSchemaRef(Product, {exclude: ['id']}), ) product: Exclude<Product, ['id']>) { } }

but 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?

What's the minimum way to have a valid requestBody spec? I assume the new decorator is designed to allow so:

Use a TS class such as Product to build the schema

Use options such as exclude or optional to tweak the schema

I would expect something like the following to represent Exclude<Product, ['id']>:

class MyController1 { @post('/Product') create(@requestBody.fromType(Product, {exclude: ['id']}) product: Exclude<Product, ['id']>) { } }

@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 metadata

required: a boolean value

And the nested content object contains more fields like examples besides schema, see comment #3398 (comment), the discussion of the content spec is at the bottom.

what is the main purpose of SIMPLIFYING the decorator?

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:

@requestBody.fromType(Product, {exclude: ['id']})

and what I proposed in elsewhere in this pull request:

@requestBody(MyModel, { // parameter-definition options description: 'description of the request body parameter', required: true, // schema options partial: true, optional: ['categoryId'], exclude: ['id'], })

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!

@requestBody.fromType(Product); // or @requestBody(Product)

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.

bajtos · 2019-07-22T09:21:44Z

packages/openapi-v3/spike.md

+  create(@newRequestBody1(
+    requestBodySpec,
+    excludeOptions
+  ) product: Exclude<Product, ['id']>) { }


To follow-up on my comment above, I am proposing to replace excludeOptions with calling getModelSchemaRef to obtain the schema.

class MyController1 { @post('/Product') create(@newRequestBody1( requestBodySpec, getModelSchemaRef(Product, {exclude: ['id']}), ) product: Exclude<Product, ['id']>) { } }

bajtos · 2019-07-22T09:24:19Z

...s/openapi-v3/src/__tests__/unit/decorators/request-body/spike.request-body.decorator.unit.ts

+      const requestSpecForUpdate = {
+        description: 'Update a product',
+        required: true,
+      };


I find this approach, where we need one new variable (requestSpecForUpdate, requestSpecForCreate) for each endpoint accepting a request body, as not very ergonomic.

Please update examples/todo/src/controllers/todo.controller.ts to show your proposal in practice.

bajtos · 2019-07-22T09:58:25Z

While this proposal does simplify request body annotation, it seems like a too small improvement to me. It makes me wonder if we could find a more radical simplification?

For example, this could be a super-simple API for people using LB4 with our ORM:

@requestBody(MyModel, {
  // parameter-definition options
  description: 'description of the request body parameter',
  required: true, // is it required?
  // schema options
  partial: true,
  optional: ['categoryId'],
  exclude: ['id'],
})

// alternatively, nest `schema` options for better readability
@requestBody(MyModel, {
  // parameter-definition options
  description: 'description of the request body parameter',
  required: true, // is it required?
  schemaOptions: {
    partial: true,
    optional: ['categoryId'],
    exclude: ['id'],
  },
})

API allowing people to provide their own model schema:

@requestBody(
  // fields from RequestBodyObject
  {
    description: 'description of the request body parameter',
    required: true,
  },
  // schema to use for populating `content` entries
  getModelSchemaRef(Product, {exclude: ['id']}),
)

I think this can be easily extended to support x-ts-type too, if we like so:

@requestBody(
  // fields from RequestBodyObject
  {
    description: 'description of the request body parameter',
    required: true,
  },
  // schema to use for populating `content` entries
  {'x-ts-type': Todo},
)

And finally the current API stays supported too:

@requestBody({
  description: 'description of the request body parameter',
  required: true, 
  content: {
    'application/json': {schema: {'x-ts-type': Todo}},
  }
)

Here is a mock-up declaration of requestBody decorator function:

// Current API
export function requestBody(requestBodySpec?: Partial<RequestBodyObject>): void;

// Simplified API - content is filled from schema
export function requestBody(
  requestBodySpec: Exclude<RequestBodyObject, 'content'>,
  schema: SchemaObject | ReferenceObject,
): void;

// Even simpler API - content schema is created from Model 
export function requestBody<T extends object>(
  modelCtor: Function & {prototype: T},
  paramAndSchemaOptions: Exclude<RequestBodyObject, 'content'> &
    JsonSchemaOptions<T>,
): void;

One important aspect to consider: content-specific entries can provide more than just the schema property, for example users can provide one or more examples. See Request Body Object spec and TypeScript interfaces related to RequestBodyObject:

export interface RequestBodyObject extends ISpecificationExtension {
    description?: string;
    content: ContentObject;
    required?: boolean;
}
export interface ContentObject {
    [mediatype: string]: MediaTypeObject;
}
export interface MediaTypeObject extends ISpecificationExtension {
    schema?: SchemaObject | ReferenceObject;
    examples?: ExamplesObject;
    example?: any;
    encoding?: EncodingObject;
}

I feel it's important for the new API to allow developers to provide examples. I am not so sure about the encoding, according to the example in OpenAPI spec, this fields seems to be relevant mostly for multipart (file-upload) requests. I think the short-hard form cannot be used for multipart requests and therefore we don't have to worry about encoding field. However, users may want to add custom extensions to MediaTypeObject values, so I think we will eventually need to support that too. Not necessarily as part of this PR, but it would be great to come up with a design that's flexible enough to support that use case.

One option that comes to my mind is to accept MediaTypeObject instead of SchemaObject | ReferenceObject:

@requestBody(
  // fields from RequestBodyObject
  {
    description: 'description of the request body parameter',
    required: true,
  },
  // MediaTypeObject to use for populating `content` entries
  {schema: getModelSchemaRef(Product, {exclude: ['id']})}
)

Let's spend more time on experimentation with different API flavors, so that we can find one that will be significantly easier to use.

/cc @strongloop/loopback-maintainers @strongloop/loopback-next

jannyHou · 2019-07-23T05:09:45Z

@bajtos Thank you for the very detailed comment :)

For example, this could be a super-simple API for people using LB4 with our ORM:

@requestBody(MyModel, {
// parameter-definition options
description: 'description of the request body parameter',
required: true, // is it required?
// schema options
partial: true,
optional: ['categoryId'],
exclude: ['id'],
})

// alternatively, nest schema options for better readability
@requestBody(MyModel, {
// parameter-definition options
description: 'description of the request body parameter',
required: true, // is it required?
schemaOptions: {
partial: true,
optional: ['categoryId'],
exclude: ['id'],
},
})

I am a little bit reluctant to mix our options with the request body spec(description, required)...it means the request body decorator will contain cumbersome logic to decide which ones are options and which are specs.

@requestBody(
  // fields from RequestBodyObject
  {
    description: 'description of the request body parameter',
    required: true,
  },
  // schema to use for populating `content` entries
  getModelSchemaRef(Product, {exclude: ['id']}),
)

I like this proposal if we decide to pass the entire OAI schema into the requestBody decorator, and the improved version below is better 👇

One important aspect to consider: content-specific entries can provide more than just the schema property....

I agree with "accepting the MediaTypeObject" 👍 , if there are multiple media types the decorator should be able to take in an array of the MediaTypeObject.

@requestBody(
  // fields from RequestBodyObject
  {
    description: 'description of the request body parameter',
    required: true,
  },
  // MediaTypeObject to use for populating `content` entries
  {schema: getModelSchemaRef(Product, {exclude: ['id']})}
)

@bajtos I post a comment in https://github.com/strongloop/loopback-next/pull/3398/files#r305981537, let's take some time to decide which direction to go first :)

If we decide to leverage builder functions like getModelSchemaRef to give the decorator a complete schema/mediaTypeObject, then I would vote for:

@requestBody(
  // fields from RequestBodyObject
  {
    description: 'description of the request body parameter',
    required: true,
  },
  // MediaTypeObject to use for populating `content` entries
  {schema: getModelSchemaRef(Product, {exclude: ['id']})}
)

bajtos · 2019-07-23T13:20:37Z

I am a little bit reluctant to mix our options with the request body spec(description, required)...it means the request body decorator will contain cumbersome logic to decide which ones are options and which are specs.

Sure, that's why I proposed the second option where schema options are nested in a new property.

@requestBody(MyModel, {
  description: 'description of the request body parameter',
  required: true,
  schemaOptions: {
    partial: true,
    optional: ['categoryId'],
    exclude: ['id'],
  },
})

In this design, requestBody provides spec.schemaOptions to getModelSchemaRef and copies all other properties to the spec.

I agree with "accepting the MediaTypeObject" 👍 , if there are multiple media types the decorator should be able to take in an array of the MediaTypeObject.

Array of MediaObjectType won't work, each MediaTypeObject requires a key (the content type, e.g. application/json). When the developer needs to specify more than a single MediaObjectType, then they should use the current flavor of @requestBody and build the entire request-body spec themselves.

If we decide to leverage builder functions like getModelSchemaRef to give the decorator a complete schema/mediaTypeObject, then I would vote for:

@requestBody(
  // fields from RequestBodyObject
  {
    description: 'description of the request body parameter',
    required: true,
  },
  // MediaTypeObject to use for populating `content` entries
  {schema: getModelSchemaRef(Product, {exclude: ['id']})}
)

I agree with you that this is better than my older proposals.

As discussed in #3398 (comment), I think this it not enough of an improvement. I think it makes a lot of sense to implement this flavor as the first step, I just would like us to implement also the more simple flavor in the second step.

For example:

// a mock-up, in reality we will merge requestBodyX with requestBody
function requestBodyX(
  modelCtor: Function, 
  options: Exclude<RequestBodyObject, 'content'> & {schemaOptions: JsonSchemaOptions} = {},
) {
  const schema = getModelSchemaRef(modelCtor, options.schemaOptions);

  const spec = {...options};
  delete spec.schemaOptions;

  return requestBody(spec, {schema});
}

jannyHou · 2019-07-23T21:09:15Z

@bajtos @raymondfeng according to the discussion, I feel people like the signature as @requestBody(modelCtor, otherSpecsAndSchemaOptions), I am thinking, if we adjust the order of those two params:
@requestBody(otherSpcesAndSchemaOptions, modelCtor), it simplifies the nested content object and doesn't differ that much from the current syntax 🥇 it's not even a breaking change, the new decorator is compatible with the current one.

The improved decorator would be:

// We can preserve the current name `requestBody` since it's not a breaking change
function requestBody(
  // Let's don't exclude the `content` object:
  // - user can still provide the entire content, we only generate the schema when content is empty
  // - it doesn't break the current user's code
  otherSpecsAndSchemaOptions: RequestBodyObject & {schemaOptions: JsonSchemaOptions} = {},
  modelCtor: Function, 
) { }

// add `exclude: string[]` to JsonSchemaOptions
export interface SchemaOptions extends JsonSchemaOptions {
   // preserve the current usage
   `x-ts-type`?: Function
   isVisited?: boolean,
}

As discussed in #3398 (comment), I think this it not enough of an improvement. I think it makes a lot of sense to implement this flavor as the first step, I just would like us to implement also the more simple flavor in the second step.

The "more simple flavor in the second step" is easier to implement actually... lol so if no objections, I will go with the proposal above.

raymondfeng · 2019-07-23T22:35:51Z

I don't think @requestBody(...otherSpcesAndSchemaOptions, modelCtor) is valid as the REST parameter has to be the last argument.

@requestBody(modelCtor?: Constructor<>, ...otherSpcesAndSchemaOptions, modelCtor) works for me. We can make modelCtor optional and only honors it if the 1st arg is Function.

jannyHou · 2019-07-24T16:16:01Z

@raymondfeng oops my bad, we don't need the spread operator 😅 , I updated my last comment #3398 (comment), does it look good now?

raymondfeng · 2019-07-24T16:18:36Z

@jannyHou Can we make the 1st arg optional so that we can do @requestBody(Product)?

jannyHou · 2019-07-24T16:34:45Z

@raymondfeng They can both be optional:

function requestBody(
  otherSpecsAndSchemaOptions?: RequestBodyObject & {schemaOptions: JsonSchemaOptions},
  modelCtor?: Function, 
) { }

We support @requestBody() product: Product, if no model constructor provided, the decorator infers it from the parameter's type, which is Product in this case.

People only provide a custom model constructor when they need to configure the default one, which means the schema options must come with the modelCtor IMO.

raymondfeng · 2019-07-24T16:49:54Z

With otherSpecsAndSchemaOptions?: RequestBodyObject & {schemaOptions: JsonSchemaOptions},, do we need to delete schemaOptions when the request body spec is built?

jannyHou · 2019-07-24T17:22:14Z

Just had a talk with @raymondfeng , here is a summary of the use cases we could support:

Use case 1: @requestBody() product: Product
Use case 2: @requestBody(Product, {partial: true}) product: Partial<Product>
Use case 3: @requestBody({description: 'some desc'}) product: Product
Use case 4: @requestBody({description: 'some desc'}, Product, {partial: true}) product: Partial<Product>
User case 5: if people want to build the entire request body spec, they can leverage builder functions like getModelSchemaRef. We can create a spec builder with fluent APIs.

I am going to implement the new design.

bajtos · 2019-07-25T10:29:54Z

Use case 1: @requestBody() product: Product

Use case 2: @requestBody(Product, {partial: true}) product: Partial<Product>

Use case 3: @requestBody({description: 'some desc'}) product: Product

Use case 4: @requestBody({description: 'some desc'}, Product, {partial: true}) product: Partial<Product>

User case 5: if people want to build the entire request body spec, they can leverage builder

I agree with the use cases described above 👍

(1) Do we want to support the following variant too?

@requestBody({description: 'some desc'}, Product) product: Product

(2) I am concerned about ergonomic of the proposed API. Please verify how is Prettier formatting the code. I am concerned that the proposal for use case 4 may lead to code that's contains too much whitespace noise compared to the actual information.

// Your proposed syntax: 10 lines
@requestBody(
  {
   description: 'some desc',
  },
  Product,
  {
    partial: true,
  },
)
product: Partial<Product>,

// Compare with my proposal (7 lines):
@requestBody(Product, {
  description: 'some desc',
  schemaOptions: {
    partial: true,
  },
})
product: Partial<Product>

(3) In the use case 3, are you expecting that Product model will be automatically inferred by the decorator from the design:type metadata provided by TypeScript? That would be nice 👍 As far as I am concerned, model inference can be omitted from this story if it turns out to be non-trivial to implement.

jannyHou · 2019-07-26T15:26:49Z

(1) Do we want to support the following variant too?
@requestBody({description: 'some desc'}, Product) product: Product

@bajtos This is a valid call, but the 2nd parameter is only needed when it differs from the inferred argument type.

(2) Please verify how is Prettier formatting the code.

Good catch, trying it.

(3) In the use case 3, are you expecting that Product model will be automatically inferred by the decorator from the design:type metadata provided by TypeScript?

Yep, type inference is something we already support :) and the model type precedence is just one line's change, should be simple to include in this spike.

Since the new signature is

requestBody2(
  requestBodySpecification?: Partial<RequestBodyObject>,
  modelCtor?: Function,
  schemaOptions?: SchemaOptions
) {
// implementation
}

There are 3 optional parameters, I am adding test cases to verify all the possible combinations, and see what's the refactor effort to upgrade from the current one.

bajtos

@jannyHou I find the current PoC implementation as too complex and making changes in too many places. Let's find a simpler solution please. Ideally, most (if not all) changes should be done in requestBody decorator only.

bajtos · 2019-07-29T09:54:28Z

packages/openapi-v3/src/decorators/request-body.spike.decorator.ts

+  specOrModelOrOptions?: Partial<RequestBodyObject> | Function | SchemaOptions,
+  modelOrOptions?: Function | SchemaOptions,
+  schemaOptions?: SchemaOptions
+) {


This type definition allows the following invocation:

const opts1: SchemaOptions = //...; const opts2: SchemaOptions = //...; const opts3: SchemaOptions = //...; @requestBody2(opts1, opts2, opts3)

Please use function overloads instead.

export function requestBody2( spec: Partial<RequestBodyObject>, ); export function requestBody2( spec: Partial<RequestBodyObject>, model: Function, schemaOptions?: SchemaOptions ); export function requestBody2( spec: Partial<RequestBodyObject>, schemaOptions?: SchemaOptions ); export function requestBody2( schemaOptions?: SchemaOptions ); export function requestBody2( spec: Partial<RequestBodyObject>, model: Function schemaOptions?: SchemaOptions ) { // the implementation }

Also to avoid confusion about which parameter takes precedence (spec.content['application/json'].schema vs. model + schemaOptions), I am proposing to accept model only when spec DOES NOT contain content property.

export function requestBody2( spec: Omit<Partial<RequestBodyObject>, 'content'>, model: Function, schemaOptions?: SchemaOptions );

@bajtos Typescript doesn't support "different number of parameters" in function overloading,
see https://www.tutorialsteacher.com/typescript/function-overloading

So we still need the wrapper function to resolve the caller's arguments.

@jannyHou TypeScript cannot distinguish between spec and options from function overloading perspective.

Also to avoid confusion about which parameter takes precedence (spec.content['application/json'].schema vs. model + schemaOptions), I am proposing to accept model only when spec DOES NOT contain content property.

+1, let's implement it in the real PR.

bajtos · 2019-07-29T09:57:58Z

packages/openapi-v3/src/decorators/request-body.spike.decorator.ts

+export type SchemaOptions = JsonSchemaOptions & {
+  // To support the current flavor using 'x-ts-type',
+  // preserve backward compatibility
+  [TS_TYPE_KEY]?: Function,


It is really necessary to include TS_TYPE_KEY in the options? AFAICT, the current implementation is expecting TS_TYPE_KEY inside SchemaObject. I don't understand why do we need to add TS_TYPE_KEY to SchemaOptions?

Ideally, I'd like the new decorator to use vanilla JsonSchemaOptions.

It is really necessary to include TS_TYPE_KEY in the options?

Nope 😅

bajtos · 2019-07-29T10:00:29Z

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

@@ -271,31 +256,62 @@ function processSchemaExtensions(
  }
 }

+function processSchemaExtensionsForRequestBody(


How is processSchemaExtensionsForRequestBody different from the current processSchemaExtensions function?

I am reluctant to have different levels of support for schema extension depending on where the schema is coming from (@param vs. @requestBody).

Can we find a way how leverage processSchemaExtensions for the new decorator syntax?

How is processSchemaExtensionsForRequestBody different from the current processSchemaExtensions function?

Reverted these change.

bajtos · 2019-07-29T10:03:34Z

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

 /**
 * 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) {


I find this change weird. spec if of type ControllerSpec, which means it contains specification of multiple controller methods. Typically, each controller method accepts a slightly different request body: create accepts model data excluding the PK property, patchById accepts model data with all properties optional, and so on. I don't see how a single SchemaOptions is going to work for that.

Could you please help me to better understand your intent here?

As I am reading through the code, IIUC, generateOpenAPISchema is adding schema for tsType to spec.components.schemas - I guess that starts to explain why you are adding options argument.

IIUC, you are trying to support x-ts-type together with schemaOptions. This is not something we support now, and personally I'd prefer to not support it in the future either.

spec if of type ControllerSpec, which means it contains specification of multiple controller methods. Typically, each controller method accepts a slightly different request body: create accepts model data excluding the PK property, patchById accepts model data with all properties optional, and so on. I don't see how a single SchemaOptions is going to work for that.

Yep generateOpenAPISchema generates json schema for a given x-ts-type and add it to spec.components.schemas

bajtos · 2019-07-29T10:10:48Z

packages/openapi-v3/src/generate-schema.ts

    }
  }

-  return Object.assign(schema, resolvedSchema);
+  return Object.assign(schema, resolvedSchema, { options: Object.assign(options, { isVisited: true }) });


I find it suspicious that you are setting isVisited: true here in a function that's not actually reading isVisited at all. Please move that change to the function that's reading isVisited value.

that flag is not needed any more in the new PoC #3466

bajtos · 2019-07-29T10:18:30Z

packages/openapi-v3/src/decorators/request-body.spike.decorator.ts

+    const paramType = paramTypes[index];
+
+    // preserve backward compatibility
+    if (modelCtor) schemaOptions = Object.assign({}, schemaOptions, { [TS_TYPE_KEY]: modelCtor });


Let's not use TS_TYPE_KEY please and find a way how to leverage getModelSchemaRef instead.

Ideally, I'd like all of the changes to stay inside the request-body decorator, we should not need to change other functions like resolveSchema.

Would the following work?

const ctor = modelCtor || paramCtor; if (!ctor) { // No model to infer the schema from. This means the user is supposed // to describe content schema in requestBodySpecification. // Print a warning or throw an error if they did not do so, // because the OpenAPI spec is not very useful without content schema return; } const schema = getModelSchemaRef(ctor, schemaOptions); requestBodySpec.content = _.mapValues(requestBodySpec.content, c => { if (!c.schema) { c.schema = schema; } return c; });

reverted this code as well in #3466

jannyHou · 2019-07-29T13:59:33Z

@bajtos

Please use function overloads instead.

I first tried function overload but it doesn't recognize the call with omitted params, maybe I missed some signatures, let me try again.

Let's not use TS_TYPE_KEY please and find a way how to leverage getModelSchemaRef instead.
Ideally, I'd like all of the changes to stay inside the request-body decorator, we should not need to change other functions like resolveSchema.

getModelSchemaRef is not called anywhere, I guess we should replace these two lines to call it first.

processSchemaExtensions reads the TS_TYPE_KEY.

So IMO "Let's not use TS_TYPE_KEY", "leverage getModelSchemaRef instead" and "all of the changes to stay inside the request-body decorator" could not be achieved at the same time...but I will try to simplify the change in my PR.

raymondfeng · 2019-07-29T17:15:43Z

packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.decorator.unit.ts

-import {model, property} from '@loopback/repository';
+import { model, property } from '@loopback/repository';
+import { expect } from '@loopback/testlab';
+import { getControllerSpec, post, requestBody2 } from '../../../..';


The formatting changes should be reverted.

sure. Sorry I didn't make it clear in the PR title, this is a PoC PR, not the real implementation. I should have created this PR as a draft one.

raymondfeng · 2019-07-29T17:16:45Z

packages/openapi-v3/src/__tests__/unit/decorators/request-body/request-body.decorator.unit.ts

@@ -16,7 +16,7 @@ describe('requestBody decorator', () => {
      };
      class MyController {
        @post('/greeting')
-        greet(@requestBody(requestSpec) name: string) {}
+        greet(@requestBody2(requestSpec) name: string) { }


It would be better to use @requestBody or @requestBody.* for all flavors.

@raymondfeng see the section "Naming" in the spike.md file, I started a discussion there and proposed staying with @requestBody() since it doesn't break the existing usage.

jannyHou · 2019-07-29T20:47:35Z

@bajtos All the complicated code change begin with I mixed getModelSchemaRef() function and the code here, thought they do the same thing while they are actually different...

I rewrote the code change in a new draft PR #3466, and all the changes are inside requestBody decorator.

cc @strongloop/loopback-maintainers I am closing this PR and let's continue the discussion in the much cleaner PoC #3466

jannyHou requested review from bajtos and raymondfeng as code owners July 19, 2019 17:08

feat: simplify request body decorator

2834b26

jannyHou force-pushed the spike/improve-request-body-decorator branch from 57dc19e to 2834b26 Compare July 19, 2019 17:15

raymondfeng reviewed Jul 19, 2019

View reviewed changes

bajtos suggested changes Jul 22, 2019

View reviewed changes

bajtos added developer-experience Issues affecting ease of use and overall experience of LB users feature OpenAPI REST Issues related to @loopback/rest package and REST transport in general labels Jul 25, 2019

jannyHou added 2 commits July 26, 2019 15:55

feat: the improved poc

2573ee7

fixup!: lint

74c3461

bajtos suggested changes Jul 29, 2019

View reviewed changes

raymondfeng reviewed Jul 29, 2019

View reviewed changes

jannyHou changed the title ~~feat: simplify request body decorator~~ [SPIKE] feat: simplify request body decorator Jul 29, 2019

jannyHou mentioned this pull request Jul 29, 2019

Spike/new requestbody #3466

Closed

7 tasks

jannyHou closed this Jul 29, 2019

jannyHou mentioned this pull request Aug 1, 2019

[Spike/Discussion] The proposal of the new requestBody decorator #3493

Closed

2 tasks

jannyHou deleted the spike/improve-request-body-decorator branch September 12, 2019 21:01

bajtos mentioned this pull request Jun 5, 2020

Syntactic sugar for requestBody() with custom schema options #3257

Closed

5 tasks

[SPIKE] feat: simplify request body decorator #3398

[SPIKE] feat: simplify request body decorator #3398

Conversation

jannyHou commented Jul 19, 2019

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

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jul 22, 2019 • 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

bajtos commented Jul 22, 2019

jannyHou commented Jul 23, 2019 • edited Loading

bajtos commented Jul 23, 2019

jannyHou commented Jul 23, 2019 • edited Loading

raymondfeng commented Jul 23, 2019

jannyHou commented Jul 24, 2019

raymondfeng commented Jul 24, 2019

jannyHou commented Jul 24, 2019

raymondfeng commented Jul 24, 2019

jannyHou commented Jul 24, 2019

bajtos commented Jul 25, 2019

jannyHou commented Jul 26, 2019

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

jannyHou Jul 29, 2019 • 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

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

jannyHou commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Jul 29, 2019 • edited Loading

jannyHou Jul 22, 2019 •

edited

Loading

jannyHou commented Jul 23, 2019 •

edited

Loading

jannyHou commented Jul 23, 2019 •

edited

Loading

jannyHou Jul 29, 2019 •

edited

Loading

raymondfeng Jul 29, 2019 •

edited

Loading

jannyHou commented Jul 29, 2019 •

edited

Loading