jsonSchema composition for inheritance and includeRelations

[InvictusMB]

Implement a workaround for enum duplication in a generated OpenAPI spec mentioned here.
As of @loopback/repository-json-schema@2.4.2 both inheritance and getModelSchemaRef(SomeModel, {includeRelations: true}) produce duplication in definitions. If an enum type property is present in SomeModel and a client is generated from such a spec (via typescript-fetch in my case), SomeModel and SomeModelWithRelations will have incompatible types in generated code. Same happens if a model inherits another one with an enum in it.
This PR replaces all inherited props with allOf composition and a $ref to a parent schema in both cases.

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 👈

[Smixi]

Hi,
I just got the same problem with enums and Typescript Axios generator for my client.
I'm kinda new to Loopback, but I was wondering if your workaround will change the usage on mentioned show in this post or you plan to go for a reusable ref, so it will not only check for inheritance.
Because I think if two different model share the same enum, then the OpenAPI spec will still have two of them and, it will also produce duplicates in the generated code for the clients (and also incompatible types).

[InvictusMB]

@Smixi It won't address reusable enums yet. Also OpenAPI generators are not good at enums either.
But it is possible work to around to some extent if a shared enum property is extracted into a common ancestor model. This is essentially the use case I'm aiming to enable here.

[Smixi]

@Smixi It won't address reusable enums yet. Also OpenAPI generators are not good at enums either.
But it is possible work to around to some extent if a shared enum property is extracted into a common ancestor model. This is essentially the use case I'm aiming to enable here.

Ok thanks for the answer !
For my side your use case is the same as mine so I'm happy for your fix :) !
For now, client side, I'm just casting to targeted model, but it will be in fact a lot cleaner with what you're fixing.
I guess it will improve overtime for both OpenAPI and LB4 for handling such items

[InvictusMB]

@Smixi You could already test this PR on your setup via patch-package. See example here. Please let me know if there is any unexpected behavior.

[Smixi]

@InvictusMB It seems my LB4 version is one minor version more than yours, and the patch thus cannot be applied (2.4.2 v 2.4.3), i'll try to downgrade if they are no breaking changes I'll tell you if that works fine.

[Smixi]

@InvictusMB I just Installed the patch but unfortunately it seems to keep the inlining of the enums.

Here is a property of my model Weapon :

export enum CategoryType {
  A = "A",
  B = "B",
  C = "C",
  D = "D",
}

  @property({
    type: 'string',
    required: false,
    postgresql: { columnName: 'category', dataType: 'text', dataLength: null, dataPrecision: null, dataScale: null, nullable: 'YES' },
    jsonSchema: {
      type: ["string", "null"],
      enum: Object.values(CategoryType)
    }
  })
  category?: CategoryType;

Here is a part of the controller :

@authenticate('jwt')
  @post('/weapons', {
    responses: {
      '200': {
        description: 'Weapon model instance',
        content: { 'application/json': { schema: getModelSchemaRef(Weapon) } },
      },
    },
  })
  async create(
    @requestBody({
      content: {
        'application/json': {
          schema: getModelSchemaRef(Weapon, {
            title: 'NewWeapon',
            exclude: ['id'],
          }),
        },
      },
    })
    weapon: Omit<Weapon, 'id'>,
  ): Promise<Weapon> {...}

  @authenticate("jwt")
  @get('/weapons/{id}', {
    responses: {
      '200': {
        description: 'Weapon model instance',
        content: {
          'application/json': {
            schema: getModelSchemaRef(Weapon, { includeRelations: true }),
          },
        },
      },
    },
  })
  async findById(
    @param.path.string('id') id: string,
    @param.query.object('filter', getFilterSchemaFor(Weapon)) filter?: Filter<Weapon>
  ): Promise<Weapon> {
    return this.weaponRepository.findById(id, filter);
  }

And here is the associated part of the JSON

"NewWeapon":{   "title":"NewWeapon",
   "description":"(tsType: Omit<Weapon, 'id'>, schemaOptions: { title: 'NewWeapon', exclude: [ 'id' ] })",
   "properties":{      "serialnumber":{
         "type":"string"
      
},
      "model":{
         "type":"string"
      
},
      "available":{
         "type":"boolean"
      
},
      "description":{
         "type":"string"
      
},
      "deleted":{
         "type":"boolean"
      
},
      "handtype":{         "type":"string",
         "enum":[
            "rightHanded",
            "leftHanded",
            "ambidextrous"
         
]
      
},
      "weaponType":{         "type":"string",
         "enum":[
            "rifle",
            "pistol",
            "cannon",
            "carbine",
            "crossbow"
         
]
      
},
      "caliber":{         "type":"string",
         "enum":[
            "4.5mm",
            "7.5mm",
            ".22LR",
            ".38",
            "arrow",
            "lever"
         
]
      
},
      "category":{         "type":"string",
         "enum":[
            "A",
            "B",
            "C",
            "D"
         
]
      
},
      "price":{
         "type":"string"
      
},
      "clubNumber":{
         "type":"string"
      
}
   
},
   "required":[
      "serialnumber"
   
],
   "additionalProperties":false,
   "x-typescript-type":"Omit<Weapon, 'id'>"
},
"WeaponPartial":{   "title":"WeaponPartial",
   "description":"(tsType: Partial<Weapon>, schemaOptions: { partial: true })",
   "properties":{      "id":{
         "type":"string"
      
},
      "serialnumber":{
         "type":"string"
      
},
      "model":{
         "type":"string"
      
},
      "available":{
         "type":"boolean"
      
},
      "description":{
         "type":"string"
      
},
      "deleted":{
         "type":"boolean"
      
},
      "handtype":{         "type":"string",
         "enum":[
            "rightHanded",
            "leftHanded",
            "ambidextrous"
         
]
      
},
      "weaponType":{         "type":"string",
         "enum":[
            "rifle",
            "pistol",
            "cannon",
            "carbine",
            "crossbow"
         
]
      
},
      "caliber":{         "type":"string",
         "enum":[
            "4.5mm",
            "7.5mm",
            ".22LR",
            ".38",
            "arrow",
            "lever"
         
]
      
},
      "category":{         "type":"string",
         "enum":[
            "A",
            "B",
            "C",
            "D"
         
]
      
},
      "price":{
         "type":"string"
      
},
      "clubNumber":{
         "type":"string"
      
}
   
},
   "additionalProperties":false,
   "x-typescript-type":"Partial<Weapon>"
}

Am I doing something wrong ?
If you need more info don't hesitate :) !

[InvictusMB]

This is a slightly different use case.
You have schemaOptions: { partial: true } and schemaOptions: { exclude: [ 'id' ] }. This requires a more complicated logic.

I've only added special handling for schemaOptions: {includeRelations: true} so far.

[Smixi]

@InvictusMB Ok I see ! Thus Weapon and WeaponWithRelation should have the same enum, I'll check this. Otherwise sorry, I got in fact confused with the use case fixed by your PR. Thanks you :)

[bajtos]

I find it very difficult to review this part of the changeset. Is there a way how to preserve the old order of definitions entries, to keep the amount of changed lines as small as possible please?

[InvictusMB]

@bajtos I don't know how to proceed with this PR tbh.
The original goal was to eliminate all duplication that blows up taxonomy in OpenAPI definitions. The reason is that each model creates 4+ definitions (Thing, ThingPartial, ThingWithRelations, NewThing, etc.) and this causes issues in the client-side generated code as TypeScript does not consider all those incarnations of the same model compatible.

This code seems to work fine on my projects but I've observed more cases where extra definitions are created. Other variations on JsonSchemaOptions (e.g. excluding properties) create new definitions. I'm not sure if this code is future-proof. A proper fix would be to construct an inheritance chain/tree in terms of OpenAPI schema but I don't have enough experience with lb4 to account for all scenarios.

Also I'm not sure if the isInherited check and getParentModel implemented here are sufficient.

My suggestion for moving this forward would be to expose modelToJsonSchema to userland customization/override instead. So that we can explore the advanced scenarios and come up with patterns that work before writing them in stone.

[bajtos]

A proper fix would be to construct an inheritance chain/tree in terms of OpenAPI schema but I don't have enough experience with lb4 to account for all scenarios.

I like the idea of using inheritance for different variants of model schemas. I think Thing, ThingWithRelations and NewThing may be reasonably easy to implement (NewThing -> Thing -> ThingWithRelations). I am not sure if it's possible to include ThingPartial in the inheritance chain though.

My suggestion for moving this forward would be to expose modelToJsonSchema to userland customization/override instead. So that we can explore the advanced scenarios and come up with patterns that work before writing them in stone.

I think that's the best approach to get started 👍🏻 . Would you like to open a new pull request for that?

Another idea to consider: #4365 landed a feature that consolidates OpenAPI schema to move repeated in-place schema definitions to shared #/components/schemas. Can we perhaps enhance the consolidator to also rework NewThing -> Thing -> ThingWithRelations to use inheritance?

[bajtos]

Can we perhaps enhance the consolidator to also rework NewThing -> Thing -> ThingWithRelations to use inheritance?

On the second thought, I think it's better to implement this feature as a new OASEnhancer that should be executed after ConsolidationEnhancer finished its work.

[dhmlau]

We just switch the contribution method from CLA to DCO, making your contribution easier in the future. Please sign the commits with DCO by amending your commit messages with -s flag and push the changes again. If you're using the command line, you can:

git commit --amend -s
git push --force-with-lease

Please refer to this docs page for details. Thanks!

[stale]

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

[stale]

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

[stale]

This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one.