Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPIKE] feat: simplify request body decorator #3398

Closed
wants to merge 3 commits into from

Conversation

@jannyHou
Copy link
Contributor

commented Jul 19, 2019

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

馃憠 Check out how to submit a PR 馃憟

@jannyHou jannyHou requested review from bajtos and raymondfeng as code owners Jul 19, 2019

@jannyHou jannyHou force-pushed the spike/improve-request-body-decorator branch from 57dc19e to 2834b26 Jul 19, 2019

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

The formatting is broken.

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

Maybe the following is better:

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

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 23, 2019

Author Contributor

I agree 馃憤

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

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

@bajtos
Copy link
Member

left a comment

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锟.

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

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.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 22, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 22, 2019

Member

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']>) { }
}

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 23, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 23, 2019

Member

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.

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

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']>) { }
}
const requestSpecForUpdate = {
description: 'Update a product',
required: true,
};

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

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

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

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

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

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

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

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

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

  • 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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

(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.

jannyHou added some commits Jul 26, 2019

@bajtos
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 29, 2019

Member

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
}

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 29, 2019

Member

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
);

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 29, 2019

Member

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

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 30, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 29, 2019

Member

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.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

It is really necessary to include TS_TYPE_KEY in the options?

Nope 馃槄

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

function processSchemaExtensionsForRequestBody(

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 29, 2019

Member

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?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

How is processSchemaExtensionsForRequestBody different from the current processSchemaExtensions function?

Reverted these change.

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 29, 2019

Member

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?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 29, 2019

Member

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.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

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

}
}

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 29, 2019

Member

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.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

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

const paramType = paramTypes[index];

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 29, 2019

Member

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;
});

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

reverted this code as well in #3466

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

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

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 29, 2019

Member

The formatting changes should be reverted.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 29, 2019

Member

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

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 29, 2019

Author Contributor

@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 jannyHou changed the title feat: simplify request body decorator [SPIKE] feat: simplify request body decorator Jul 29, 2019

@jannyHou jannyHou referenced this pull request Jul 29, 2019
2 of 7 tasks complete
@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.