Loopback-next: OpenAPI decorator does not properly generate schemas of type `Partial`

[shimks]

Description / Steps to reproduce / Feature proposal

From todo.controllers.ts in example-getting-started:

    async createTodo(@requestBody() todo: Todo) {
    // TODO(bajtos) This should be handled by the framework
    // See https://github.com/strongloop/loopback-next/issues/118
      if (!todo.title) {
        return Promise.reject(new HttpErrors.BadRequest('title is required'));
      }
      return await this.todoRepo.create(todo);
    }

It should be noted that the spec generated by @requestBody is not correct since the argumnet that the operation takes in _does not_ need an id property. One may think that Partial<Todo> may be the next best thing, but it's not possible at the moment to specify which properties are optional and which aren't.

We should find a way to generate the schema for a model when used as a parameter so that id property is not included in the generated schema

Proposed behavior:

@post('/')
makeStuff(@requestBody(Note) note: Partial<Note>) {}

• Whenever requestBody is used on a model that extends Entity, always generate (or modify) its spec without the property that's been assigned as id.
• pros:
  
  
  
  • simple ux
  
  
• cons:
  
  
  
  • Not very flexible
  
  
  • requestBody becomes very overloaded
  
  

Acceptance Criteria:

@post('/exclude')
postWithout(@requestBody(Note, {excludeProperties: ['id']}) note: Partial<Note>) {}
@post('/include')
PostWith(@requestbody(Note, {includeProperties: ['title', 'description']) note: Partial<Note>) {}

• [ ] explore more options from TypeScript (like Partial) to see if they can infer or provide us with additional data/metadata
• [ ] allow requestBody (or other new decorator to create the appropriate metadata) to take in the model type and schema generation options to produce a custom schema
  
  
  
  • [ ] specifying excludeProperties should generate an OpenAPI Schema that lacks the given keys and their values when converting from the JSON Schema generated in rest
  
  
  • [ ] specifying includeProperties should generate an OpenAPI Schema that contains only the given keys and their values when converted from the JSON Schema generated in rest
  
  

• [ ] Backwards compatibility must be preserved (@requestBody(spec))

• related to https://github.com/strongloop/loopback-next/pull/1121
  

_See Reporting Issues for more tips on writing good issues_

[bajtos]

I don't think it's a good idea to interpret Partial<Note> as a data-type omitting the primary key (id property) because not all applications are generating ids on the server/in the database. In some cases, the clients are responsible to generate a unique id (typically a GUID/UUID). This is important for example for offline-first implementation.

I think we need a way how to tweak the auto-magic algorithm that's currently in use to build JSON Schema/OpenAPI Schema description of requestBody payload from our model definition. The we should give the application developer control of requestBody schema generated for each controller method (API endpoint). Please let's not make this even more magical than it already is!

A possible API:

@post('/')
makeStuff(@requestBody(Note, {propertyBlacklist: ['id']}) note: Partial<Note>) {}

(propertyBlacklist allows us to also implement propertyWhitelist. Alternative naming scheme: exceptProperties & onlyProperties).

[virkt25]

+1 to having a way to tweak the auto-magic algorithm but I also think we should have a default "magic" fallback that we default to and then a way to tweak it if needed. I have another alternative name for propertyBlacklist & friends ... includeProperties / excludeProperties where we can even abbreviate Properties with Props

[jannyHou]

+1 for the discussion above.
And I would prefer to have the property inclusion/exclusion functions as Model static methods instead of processing them in the decorator function. So we don't need to duplicate the implementation every time a new artifact needs it.

---

We can also implement some schema combination methods in package @loopback/repository-json-schema to support allof, oneof, anyof, not: https://spacetelescope.github.io/understanding-json-schema/reference/combining.html. Especially oneof, which would be useful in polymorphic relations. While the proposal is off topic in this issue.

[bajtos]

BTW, this is a problem we faced in LoopBack 3.x too, see https://github.com/strongloop/loopback/issues/2924

Another example of a property that must be omitted when creating a new model is _rev property used by Cloudant and CouchDB.

The solution in LB 3.x touched several layers:

• Juggler properties can be flagged as "updateOnly", see https://github.com/strongloop/loopback-datasource-juggler/pull/1453. Since LB4 is using the same juggler under the hood, we may be able to leverage this feature too.
• LoopBack detects whether a model has any "updateOnly" instances. If it does, then it sets a special flag "createOnlyInstance" in the remoting metadata of model'screate method, see https://github.com/strongloop/loopback/pull/3548
• The swagger generator recognizes "createOnlyInstance" and emits a different schema for the request body payload of such method, see https://github.com/strongloop/loopback-swagger/pull/92

Maybe we can get some inspiration from this existing solution and perhaps leverage certain parts?

[raymondfeng]

+1 for the discussion above.
And I would prefer to have the property inclusion/exclusion functions as Model static methods instead of processing them in the decorator function. So we don't need to duplicate the implementation every time a new artifact needs it.

The overlapping between parameter value and model instance validation is interesting:

For example, CustomerController.create(customer) probably hints two types of validations here:

1. Make sure customer parameter is valid. One use case is to disallow customer.id to be passed in during creation.
2. Make sure customer is a valid instance of Customer model, such as email is required.

I start to lean toward that a parameter can impose additional constraints beyond the model type checking as such validations only make sense in the method invocation context. For the model itself, it's hard to describe when id should be supplied or not.

[bajtos]

I start to lean toward that a parameter can impose additional constraints beyond the model type checking as such validations only make sense in the method invocation context. For the model itself, it's hard to describe when id should be supplied or not.

This makes a lot of sense to me too. I think it's also in line with includeProperties / excludeProperties we discussed earlier.

[shimks]

~I think this issue is ready for estimation unless anyone disagrees with the given acceptance criteria~
~I'm thinking now that the issue is more closely tied to the result from the spike #1057 and that whatever implementation is used achieve the UX we've settled on will need to leverage it.
In short, the acceptance criteria cannot be decided as of yet because it's blocked by #1057.~

[bajtos]

_(cross-posted @raymondfeng's https://github.com/strongloop/loopback-next/issues/1722#issuecomment-422439405)_

I'm proposing to introduce an @schema decorator or extend existing decorators so that a schema can be something like:
```js
{
schema: {
'x-ts-type': Order,
'x-ts-type-options': {
partial: true,
includes: ['p1', 'p2'],
excludes: []
}
}

[bajtos]

Closing in favor of #2652 _Emit schema with all model properties optional_ and #2653 _Emit schema excluding certain model properties_.