OpenAPI schema generation and model inheritance

[ricky92]

When using inheritance in model definitions, the behaviour is inconsistent.

Steps to reproduce

• Define a model that inherits from another Model/Entity
• Use the aforementioned model in any controller, so that the OpenAPI spec references it
• Run the server
• Check the schemas in the API Explorer

Current Behaviour

As noted, the behaviour is inconsistent, and depends upon the order of occurrence of the parent/child models.

• If the parent model is referenced first, the child model is represented as a 1:1 copy (even the name is identical) of the parent.
• If the child model is referenced first and schema-related functions are used somewhere in the code (most notably in decorators, as I suspect they are executed before anything else), both the models are represented as having the child's name and the parent's properties.
• If the child model is referenced first, and schema-related functions (e.g. getModelSchemaRef) are not used on the parent model, the schemas are generated correctly.

My guess is there is some sort of caching mechanism (which makes a lot of sense) for the generation of OpenAPI schemas. If so, this mechanism is broken when dealing with inheritance.

Expected Behaviour

Correct generation of both child and parent models' schemas, regardless of their occurrences' positions in controllers.

Acceptance Criteria

• [ ] Correct generation of both child and parent models' schemas, regardless of their occurrences' positions in controllers.
• [ ] Add tests to exercise this area
• [ ] Update docs accordingly.

Link to reproduction sandbox

https://github.com/ricky92/loopback-next/tree/model-inheritance

Additional information

Platform and node version: darwin x64 8.16.0

@loopback/example-todo@1.6.5
├── @loopback/boot@1.4.4
├── @loopback/context@1.20.2
├── @loopback/core@1.8.5
├── @loopback/openapi-v3@1.7.0
├── @loopback/repository@1.8.2
├── @loopback/rest@1.16.3
├── @loopback/rest-explorer@1.2.5
├── @loopback/service-proxy@1.2.5
└── loopback-connector-rest@3.4.1

[bajtos]

Thank you @ricky92 for reporting the bug, I am able to reproduce it using your sandbox.

I did a quick check and I agree with your conclusion that our schema caching is not working correctly.

Here is the problematic code:

https://github.com/strongloop/loopback-next/blob/6bbc44f769bf5ed2aaa2012ef4b64c30d68ee928/packages/repository-json-schema/src/build-schema.ts#L63

The caching problem seems to be fixed when I add {ownMetadataOnly: true} options as follows:

const cached = MetadataInspector.getClassMetadata(JSON_SCHEMA_KEY, ctor, {
  ownMetadataOnly: true,
});

Observed schema:

{
  "title": "TodoEx",
  "properties": {
    "id": { "type": "number" },
    "title": { "type": "string" },
    "desc": { "type": "string" },
    "isComplete": { "type": "boolean" },
    "remindAtAddress": { "type": "string" },
    "remindAtGeo": { "type": "string" }
  },
  "required": [ "title" ]
}

I am not sure what is causing this second problem.

Anyhow, would you like to investigate this issue further and perhaps contribute a pull request to fix it? See Contributing code in LoopBack 4 and Submitting a pull request to LoopBack 4 to get started.

[mgabeler-lee-6rs]

I ran into a similar symptom which, in my case at least (not sure if it's exactly the same as here), seems to be caused by the caching of MODEL_WITH_PROPERTIES_KEY in ModelMetadataHelper.getModelMetadata.

What's happening is that the call to MetadataInspector.getClassMetadata calls Reflector.getMetadata which calls Reflect.getMetadata which finally calls Reflect.OrdinaryGetMetadata which is this:

            var hasOwn = OrdinaryHasOwnMetadata(MetadataKey, O, P);
            if (hasOwn)
                return OrdinaryGetOwnMetadata(MetadataKey, O, P);
            var parent = OrdinaryGetPrototypeOf(O);
            if (!IsNull(parent))
                return OrdinaryGetMetadata(MetadataKey, parent, P);
            return undefined;

And so what happens is that hasOwn comes out false (the MODEL_WITH_PROPERTIES_KEY cache is not built yet for this class), so it tries to fetch it from the parent, and the parent is cached already, and so we return the cached value from the parent instead of building the cached value for the derived class.

I think the fix may then be to have getModelMetadata always pass {ownMetadataOnly: true} in the InspectionOptions argument to MetadataInspector.getClassMetadata for this specific case.

---

Workaround I used: Put all the properties in an abstract base class that does not have a model decorator. Put the model decorator only on final leaf classes (one of which may have no properties of its own).

[Harmonickey]

@mgabeler-lee-6rs that workaround solution worked for me. As a side-note for those of whom are going about this workaround for now. Make sure your relations are in your final leaf classes, not the base abstract class. Otherwise loopback will complain about the relation metadata not compiling correctly.

Something like... Unhandled error in POST /users: 500 TypeError: Cannot read property 'idProperties' of undefined at Function.getIdProperties (...\@loopback\repository\src\model.ts:271:28) at Object.resolveHasOneMetadata (...\@loopback\repository\src\relations\has-one\has-one.helpers.ts:61:27) at Object.createHasOneRepositoryFactory (...\@loopback\repository\src\relations\has-one\has-one-repository.factory.ts:52:16)

[emonddr]

Possible causes decorator function maybe introspecting the class too early

Possible Solution Decorator should reference the raw metadata when the function is applied. We could have a late phase to do full introspection.

We need test cases to confirm this problem, and afterwards confirm that we've fixed the issue.

[bajtos]

We need test cases to confirm this problem, and afterwards confirm that we've fixed the issue.

Based on https://github.com/strongloop/loopback-next/issues/3293#issuecomment-507643117, the problem can be reproduced using the sanbox provided in the bug report, see https://github.com/ricky92/loopback-next/tree/model-inheritance