Spike/new requestbody #3466

jannyHou · 2019-07-29T19:47:08Z

Description

connect to #2654
A new draft PR created after all the discussions in #3398.
The PoC in #3398 is over complicated, and the rebase effort is also big after introducing the generic type in interface JsonSchemaOptions, so I created this PR to replace it.

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 · 2019-07-29T19:51:00Z

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

+
+    const paramType = paramTypes[index];
+
+    // preserve backward compatibility


FYI: compared with the current decorator function, the difference of the new decorator is from Line 81 to Line 91, other code stay the same.

raymondfeng · 2019-07-29T19:59:33Z

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

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


Formatting is broken.

@raymondfeng Fixed :)

bajtos · 2019-07-30T10:37:04Z

packages/openapi-v3/_spike-request-body.md

+_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_


I am afraid this is not true. The following code seems to compile correctly for me.

export function requestBody2( spec: Partial<RequestBodyObject>, ): (target: object, member: string, index: number) => void; export function requestBody2( spec: Partial<RequestBodyObject>, model: Function, schemaOptions?: SchemaOptions, ): (target: object, member: string, index: number) => void; export function requestBody2( spec: Partial<RequestBodyObject>, schemaOptions?: SchemaOptions, ): (target: object, member: string, index: number) => void; export function requestBody2( schemaOptions?: SchemaOptions, ): (target: object, member: string, index: number) => void; export function requestBody2( specOrModelOrOptions?: Partial<RequestBodyObject> | Function | SchemaOptions, modelOrOptions?: Function | SchemaOptions, schemaOptions?: SchemaOptions, ): (target: object, member: string, index: number) => void { return (target: object, member: string, index: number): void => { // implementation }; }

@bajtos This is weird, not sure if you read the comment in #3398 (comment)
yesterday I double checked it with Dom and I believe the conclusion is overloading with different number of parameters is not supported, and my editor still complains with your code:

What version of TypeScript are you using in the editor? I vaguely remember that VS Code sometimes prefers to use the built-in version instead of the version installed in node_modules. Could that explain the difference?

@bajtos I have the same problem with 3.5.3, see:

The overloaded functions work for me in VSCode and npm run build:

export function requestBody2( spec: Partial<RequestBodyObject>, ): (target: object, member: string, index: number) => void; export function requestBody2<T extends object>( spec: Partial<RequestBodyObject>, model: Function, schemaOptions?: JsonSchemaOptions<T>, ): (target: object, member: string, index: number) => void; export function requestBody2<T extends object>( spec: Partial<RequestBodyObject>, schemaOptions?: JsonSchemaOptions<T>, ): (target: object, member: string, index: number) => void; export function requestBody2<T extends object>( schemaOptions?: JsonSchemaOptions<T>, ): (target: object, member: string, index: number) => void; export function requestBody2<T extends object>( specOrModelOrOptions?: | Partial<RequestBodyObject> | Function | JsonSchemaOptions<T>, modelOrOptions?: Function | JsonSchemaOptions<T>, schemaOptions?: JsonSchemaOptions<T>, ) { // }

Seems like it works for 2/3 of us, so this would be a great addition/change to us having to determine the arguments from the callee.

k...finally get rid of the error after several times' code comment/uncomment 👏 , but still have no clue why that error happened...

bajtos

The proposal looks much better now!

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

bajtos · 2019-07-30T10:39:23Z

packages/openapi-v3/_spike-request-body.md

+  schemaOptions?: SchemaOptions,
+) {
+  // implementation
+}


Please update the signature to match the code.

export function requestBody2<T extends object>( specOrModelOrOptions?: | Partial<RequestBodyObject> | Function | JsonSchemaOptions<T>, modelOrOptions?: Function | JsonSchemaOptions<T>, schemaOptions?: JsonSchemaOptions<T>, ) {

The reason why I put the private function _requestBody2()'s signature(instead of the wrapper's) here is to avoid the confusion caused by manually resolve the caller's input.

Let me change the name to the private one.

bajtos · 2019-07-30T10:49:02Z

packages/openapi-v3/_spike-request-body.md

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


I would like to tighten the API little bit, to make it more difficult for our users to do something wrong or confusing.

(1)
When the spec contains content property, then we should not allow model & schemaOptions.

I'd like to make the following example invalid & rejected by the compiler.

requestBody({ content: { 'application/json': { schema: getModelSchemaRef(MyModel) } }, MyModelData, // notice a different class! );

Proposed signature (overload):

export function requestBody2<T>( spec: Omit<Partial<RequestBodyObject>, 'content'>, model: Function, schemaOptions?: JsonSchemaOptions<T>, ): (target: object, member: string, index: number) => void;

(2)
As discussed in #3398 (comment), we may want to add a new overload in the future that will allow users to a different ORM framework supply the MediaTypeObject in requestBody parameters, for example:

@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'd like our current proposal to be forward compatible and allow us to add support for the example above as an incremental & backwards-compatible change in the future.

IIUC the current proposal, it supports the following form:

@requestBody( // RequestBodyObject {description: 'description of the request body parameter'}, // JsonSchemaOptions {partial: true} )

I am concerned that if we want to support MediaTypeObject for the second argument too, then it will be difficult to determine what did the user mean - did they provide JsonSchemaOptions or MediaTypeObject?

To avoid that problem, I am proposing to forbid the following form:

@requestBody( // RequestBodyObject {description: 'description of the request body parameter'}, // JsonSchemaOptions {partial: true} )

and always require users to provide the model constructor if they want to use JsonSchemaOptions too.

@requestBody( // RequestBodyObject {description: 'description of the request body parameter'}, MyModel, // JsonSchemaOptions {partial: true} )

(2)
I am proposing to forbid the following form
and always require users to provide the model constructor if they want to use JsonSchemaOptions

My point (2) becomes irrelevant if we decide to nest schemaOptions as a new property of the first argument, see #3466 (comment),

@bajtos I don't think excluding the content for the first parameterspec is a good idea, the 2nd and 3rd parameters are there to describe the schema, but content has more stuff than schema, and user should still have their own flexibility to

provide an entire content including schema without using the 2nd and 3rd params

if the 2nd+3rd params decide the default schema, user can override it for special media types with different schemas, like application/x-www-form-urlencoded in this example

So honestly, IMO the request spec(especially content) is nested in nature, unless we provide a bunch of fluent APIs to help users build it, like requestBodyBuilder.addContent('application/x-www-form-urlencoded').withSchema(model, options).withExample(exampleSpecs), there is not that much to simplify for the UX...and if we implement those builders, the purpose is still generating a complete requestBody spec, which can be provided as the 1st parameter in @requestBody(), and still fits my current proposal.

My point of view:

If users want to override special media types, then they provide full content object in the first argument.

The 2nd+3rd params are only for the case when they want LB to generate default content object for the given model & schema options.

If you like to go forward with your proposal, then please document the rules we are going to merge the (deep) fields in spec.content with the fields created from model + schemaOptions.

I am also interested how are developers going to learn about those merge rules and more importantly, when they do a mistake, how are they going to figure out why they are getting different schema from what they were expected to see?

Let's start with the example I have shown before:

requestBody({ content: { 'application/json': { schema: getModelSchemaRef(MyModel) } }, MyModelData, // notice a different class! );

Another case to consider:

requestBody({ content: { 'application/json': { example: {/* some model data */ } }, MyModel, );

Are we going to print debug logs or perhaps regular warnings to notify developers when the framework is overwriting spec in a way that may be unexpected?

@bajtos

When the spec contains content property, then we should not allow model & schemaOptions.

Let's still discuss with 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

My point is we could allow

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.
And our current implementation is already doing it, see code:

we generate a default schema according to the interred type

iterate through the contents, if schema is missing, content.schema = defaultSchema.

Back to your use case:

requestBody({ content: { 'application/json': { schema: getModelSchemaRef(MyModel) } }, MyModelData, // notice a different class! );

This is a VALID use case IMO.
cc @strongloop/sq-lb-apex @raymondfeng more opinions are welcomed here ^^ :)

With your proposal which merges the JsonSchemaOptions into the 1st parameter, everything would be the same...

@bajtos hope my comment #3466 (comment) answers your question in #3466 (comment)

As discussed in [SPIKE] feat: simplify request body decorator #3398 (comment), we may want to add a new overload in the future that will allow users to a different ORM framework supply the MediaTypeObject in requestBody parameters...

@bajtos I would suggest NOT achieve this by adding more signatures by overloading, we can introduce fluent spec builder functions to generate any part of the requestBody spec, they would be much more straightforward to use than providing partial specs in some parameters and the decorator function resolves different parts from different places.

The logic in the current decorator, or my new proposal is pretty straightforward to understand IMO:

The 1st parameter contains the pure OpenAPI request body spec

Other parameters contribute the default schema' config, model, options...

The decorator:

reads schema config

generate default schema spec accordingly

if any content type is missing schema, then assign the default schema to it

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

requestBodyBuilder.addContent('application/x-www-form-urlencoded').withSchema(model, options).withExample(exampleSpecs)

After discussing with Janny, I think her proposal makes sense to me high-level(ly). Especially the idea of that we can have some build functions that similar to this that allows users to add/build spec object if they want to pass in more information.

But I am not sure if @jannyHou 's proposal could address the following requirement that @bajtos mentioned above:

(2)
As discussed in #3398 (comment), we may want to add a new overload in the future that will allow users to a different ORM framework supply the MediaTypeObject in requestBody parameters..

If yes, I am fine with the proposal.

I agree with @agnes512, but for the first iteration, let's not exclude the content for the first parameter. I am not knowledgable to know if the use-case pointed by @bajtos is either valid or not valid since we have differing opinions, so I think we should explore it further outside the scope of this PR.

bajtos · 2019-07-30T10:53:11Z

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

+    | Partial<RequestBodyObject>
+    | Function
+    | JsonSchemaOptions<T>,
+  modelOrOptions?: Function | JsonSchemaOptions<T>,


This is not type safe, we allow users to provide MyModel as the model ctor but then provide JsonSchemaOptions<OtherModel>.

I think we should use Function & {prototype: T} to require the callers to provide a constructor function of T instead of arbitrary function.

I checked the source code, this problem is present in the current implementation of modelToJsonSchema, getJsonSchema, getJsonSchemaRef and getModelSchemaRef functions too. Let's fix that in a standalone pull request, before we start any work on a better @requestBody decorator - see #3470

I am able to catch the non-existing fields in exclude by using Function & {prototype: T}, but

@requestBody<RandomModel>(spec, Product) test: Product

still passes.
I added a test case for it, let's fix it in the implementation.

nabdelgadir

The proposal at a high level LGTM, especially using the default schema. I agree with @bajtos' point about very clearly documenting the merge rules so that developers are aware of how it works. As for using a different class in e.g.:

requestBody({
  content: {
    'application/json': {
      schema: getModelSchemaRef(MyModel)
    }
  },
  MyModelData, // notice a different class!
);

At first glance, I thought it should be rejected, but from your POV why do you think it should be a valid use case?

b-admike

Nice job Janny. LGTM overall, left my views on some of the discussions.

b-admike · 2019-07-31T21:24:29Z

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

+        },
+      });
+
+      // The feature that generates schemas according to


Can we include this in the list of follow up stories in the markdown file you have (disregard if already included)?

@b-admike Seems this feature is already supported.

b-admike · 2019-07-31T21:25:11Z

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

+
+      const spec2 = getControllerSpec(MyController2);
+
+      const requestBodySpecForCreate =


nitpick: requestBodySpecForUpdate

b-admike · 2019-07-31T21:38:08Z

packages/openapi-v3/_spike-request-body.md

+_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_


Seems like it works for 2/3 of us, so this would be a great addition/change to us having to determine the arguments from the callee.

b-admike · 2019-07-31T21:42:50Z

packages/openapi-v3/_spike-request-body.md

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


I agree with @agnes512, but for the first iteration, let's not exclude the content for the first parameter. I am not knowledgable to know if the use-case pointed by @bajtos is either valid or not valid since we have differing opinions, so I think we should explore it further outside the scope of this PR.

jannyHou · 2019-08-01T01:41:44Z

@nabdelgadir

At first glance, I thought it should be rejected, but from your POC why do you think it should be a valid use case?

See my explanation in https://github.com/strongloop/loopback-next/pull/3466/files#diff-12798cc8f728c5affaa88ab994dbf31fR21, and also comment.

IMO user should have the flexibility to define a default schema using the 2nd and 3rd parameters, but override it with custom ones provided in the 1st parameter.

agnes512

Thank you for the explanations 👍

nabdelgadir

LGTM

b-admike

LGTM

jannyHou · 2019-08-14T17:32:55Z

Subsequent tasks see #2654 (comment)
Closing.
Feel free to copy paste the code in this PR if we decide to use it.

jannyHou commented Jul 29, 2019

View reviewed changes

jannyHou force-pushed the spike/new-requestbody branch from fa2b2c7 to f1b596f Compare July 29, 2019 19:57

raymondfeng reviewed Jul 29, 2019

View reviewed changes

jannyHou force-pushed the spike/new-requestbody branch from 05c63dc to 9c142bb Compare July 29, 2019 20:30

jannyHou mentioned this pull request Jul 29, 2019

[SPIKE] feat: simplify request body decorator #3398

Closed

7 tasks

jannyHou force-pushed the spike/new-requestbody branch from 9c142bb to 0a8df75 Compare July 29, 2019 21:01

bajtos reviewed Jul 30, 2019

View reviewed changes

bajtos mentioned this pull request Jul 30, 2019

fix: enforce JsonSchemaOptions type when building model schema #3470

Merged

7 tasks

bajtos reviewed Jul 30, 2019

View reviewed changes

jannyHou force-pushed the spike/new-requestbody branch 2 times, most recently from 66ffa8f to 655a173 Compare July 31, 2019 14:27

jannyHou added 3 commits July 31, 2019 13:24

feat: simplify request body decorator

07faf4d

style: fix lint

ea24f68

fixup!: remove unused variables

cd98654

nabdelgadir reviewed Jul 31, 2019

View reviewed changes

b-admike reviewed Jul 31, 2019

View reviewed changes

jannyHou added 2 commits July 31, 2019 20:41

fixup!: use overloading

36b5443

fixup!: add more docs

5877f0c

jannyHou force-pushed the spike/new-requestbody branch from 655a173 to 5877f0c Compare August 1, 2019 01:33

agnes512 approved these changes Aug 1, 2019

View reviewed changes

nabdelgadir approved these changes Aug 1, 2019

View reviewed changes

b-admike approved these changes Aug 1, 2019

View reviewed changes

raymondfeng approved these changes Aug 1, 2019

View reviewed changes

This was referenced Aug 1, 2019

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

Closed

Spike: simplify requestBody annotation with schema options #2654

Closed

dhmlau assigned jannyHou Aug 14, 2019

jannyHou closed this Aug 14, 2019

jannyHou deleted the spike/new-requestbody branch September 12, 2019 21:01

dhmlau mentioned this pull request Oct 4, 2019

[Epic] Simplify requestBody annotation with schema options #3872

Closed

bajtos mentioned this pull request Jun 5, 2020

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

Closed

5 tasks


		const paramType = paramTypes[index];

		// preserve backward compatibility


		const spec2 = getControllerSpec(MyController2);

		const requestBodySpecForCreate =

Spike/new requestbody #3466

Spike/new requestbody #3466

Conversation

jannyHou commented Jul 29, 2019 • edited Loading

Description

Checklist

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

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jul 30, 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

jannyHou Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

nabdelgadir left a comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Aug 1, 2019

agnes512 left a comment • edited Loading

Choose a reason for hiding this comment

nabdelgadir left a comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

jannyHou commented Aug 14, 2019 • edited Loading

jannyHou commented Jul 29, 2019 •

edited

Loading

jannyHou Jul 29, 2019 •

edited

Loading

jannyHou Jul 30, 2019 •

edited

Loading

jannyHou Jul 30, 2019 •

edited

Loading

jannyHou Aug 1, 2019 •

edited

Loading

agnes512 left a comment •

edited

Loading

jannyHou commented Aug 14, 2019 •

edited

Loading