loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.97k stars 1.07k forks source link

Overriding title in `getModelSchemaRef` causes duplication in OpenAPI schema #5645

Open InvictusMB opened 4 years ago

InvictusMB commented 4 years ago

Steps to reproduce

Define a controller with getModelSchemaRef(SomeModel, {title: 'TitleOverride'}).

Controller definition ```typescript export class UserController { @post('/users', { security: OPERATION_SECURITY_SPEC, responses: { '200': { description: 'User', content: { 'application/json': { schema: { 'x-ts-type': User, }, }, }, }, }, }) async create( @requestBody({ content: { 'application/json': { schema: getModelSchemaRef(NewUserRequest, { title: 'NewUser', }), }, }, }) newUserRequest: NewUserRequest, ): Promise { // implementation } } ```

Current Behavior

The controller above emits schema definitions for both NewUserRequest and NewUser. While NewUserRequest is not referenced anywhere else in the schema.

Expected Behavior

Only NewUser is present in OpenAPI schema.

Additional information

@loopback/repository-json-schema@2.4.2 @loopback/openapi-v3@3.4.1

I presume NewUserRequest is generated from getModelSchemaRef as it should while NewUser comes from parsing paramTypes where it causes a cache miss due to overridden title.

A probable fix could exclude a parameter corresponding to requestBody from enumeration here

https://github.com/strongloop/loopback-next/blob/7f8d8356946dc236dd4daecbfae12e0a0662cf1c/packages/openapi-v3/src/controller-spec.ts#L329

Acceptance Criteria

A solution I can think of is: https://github.com/strongloop/loopback-next/blob/7f8d8356946dc236dd4daecbfae12e0a0662cf1c/packages/openapi-v3/src/controller-spec.ts#L329

should search through the content objects in the request body spec, if all contents' schemas exist in reference, then skip generating the one inferred from model ctor.

jannyHou commented 4 years ago

Hi @InvictusMB I assume you are trying example like todo-jwt. A quick confirm:

This is the OpenAPI spec I get from that app:

Controller definition

"NewUser": {
        "title": "NewUser",
        "description": "(tsType: NewUserRequest, schemaOptions: { title: 'NewUser' })",
        "properties": {
          "id": {
            "type": "string"
          },
          "realm": {
            "type": "string"
          },
          "username": {
            "type": "string"
          },
          "email": {
            "type": "string"
          },
          "emailVerified": {
            "type": "boolean"
          },
          "verificationToken": {
            "type": "string"
          },
          "password": {
            "type": "string"
          }
        },
        "required": [
          "email",
          "password"
        ],
        "additionalProperties": true,
        "x-typescript-type": "NewUserRequest"
      },
      "NewUserRequest": {
        "title": "NewUserRequest",
        "properties": {
          "id": {
            "type": "string"
          },
          "realm": {
            "type": "string"
          },
          "username": {
            "type": "string"
          },
          "email": {
            "type": "string"
          },
          "emailVerified": {
            "type": "boolean"
          },
          "verificationToken": {
            "type": "string"
          },
          "password": {
            "type": "string"
          }
        },
        "required": [
          "email",
          "password"
        ],
        "additionalProperties": true
      },

Do you mean the NewUserRequest schema shouldn't exist? And ONLY NewUser is expected.

InvictusMB commented 4 years ago

Hi @jannyHou Yes, NewUserRequest should not exist if it is not referenced anywhere else as in my case.

The fix should be something like

for (const p of paramTypes) {
  if (isComplexType(p) && !isRequestBody(p)) {
    generateOpenAPISchema(spec, p);
  }
}
jannyHou commented 4 years ago

@InvictusMB I double checked the @requestBody decorator, feel it's hard to tell should the schema generation be skipped or not, because in some situation(e.g. if no spec found in the decorator), it must infer the schema from model ctor. For example

see the doc of @requestBody

// no schema provided inside decorator, 
// therefore #L329 has to generate the schema from `User`
@requestBody() user: User

While your report is correct, it shouldn't always generate that unused schema.

A solution I can think of is: https://github.com/strongloop/loopback-next/blob/7f8d8356946dc236dd4daecbfae12e0a0662cf1c/packages/openapi-v3/src/controller-spec.ts#L329

should search through the content objects in the request body spec, if all contents' schemas exist in reference, then skip generating the one inferred from model ctor.

dhmlau commented 4 years ago

@InvictusMB, would you be interested to contribute a PR? Thanks.

InvictusMB commented 4 years ago

@dhmlau Not anytime soon. I have a bunch of fat PRs to finalize first.

stale[bot] commented 3 years ago

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

InvictusMB commented 3 years ago

I believe this is still relevant.

stale[bot] commented 3 years ago

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.