Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow controllers to provide definitions of models referenced in operation spec #2629

Closed
2 tasks
bajtos opened this issue Mar 22, 2019 · 6 comments · Fixed by #2898
Closed
2 tasks

Allow controllers to provide definitions of models referenced in operation spec #2629

bajtos opened this issue Mar 22, 2019 · 6 comments · Fixed by #2898
Assignees
Labels
feature OpenAPI Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package

Comments

@bajtos
Copy link
Member

bajtos commented Mar 22, 2019

Description / Steps to reproduce / Feature proposal

Allow controller methods to reference model schema directly via OpenAPI spec, without requiring them to use x-ts-type extension. Extend support for propagating definitions of references models, so that this feature is available to all operations using $ref instead of x-ts-type.

Consider the following test in @loopback/openapi-v3:

  it('allows operations to provide definition of referenced models', () => {
    class MyController {
      @get('/todos', {
        responses: {
          '200': {
            description: 'Array of Category model instances',
            content: {
              'application/json': {
                schema: {
                  $ref: '#/definitions/Todo',
                  definitions: {
                    Todo: {
                      title: 'Todo',
                      properties: {
                        title: {type: 'string'},
                      },
                    },
                  },
                },
              },
            },
          },
        },
      })
      async find(): Promise<object[]> {
        return []; // dummy implementation, it's never called
      }
    }

    const spec = getControllerSpec(MyController);
    const opSpec: OperationObject = spec.paths['/todos'].get;
    const responseSpec = opSpec.responses['200'].content['application/json'];
    expect(responseSpec.schema).to.deepEqual({
     $ref: '#/definitions/Todo',
    });

    const globalSchemas = (spec.components || {}).schemas;
    expect(globalSchemas).to.deepEqual({
      Todo: {
        title: 'Todo',
        properties: {
          title: {
            type: 'string',
          },
        },
      },
    });
  });

Current Behavior

The test fails:

 + expected - actual

       {
         "$ref": "#/components/schemas/Todo"
      -  "definitions": {
      -    "Todo": {
      -      "properties": {
      -        "title": {
      -          "type": "string"
      -        }
      -      }
      -      "title": "Todo"
      -    }
      -  }
       }

Expected Behavior

The test passes.

See the spike #2592 and especially the commit d93dff7 for more details . Please note the commit shows changes before the review feedback was applied. You need to review the final pull request diff of the PoC before copying any code over.

Acceptance criteria

  • Make the changes necessary to make the above test pass
  • Add additional test coverage at unit-test and integration level as necessary
@emonddr
Copy link
Contributor

emonddr commented Apr 2, 2019

@bajtos , we would like to know if the definitions is actually needed:

        responses: {
          '200': {
            description: 'Array of Category model instances',
            content: {
              'application/json': {
                schema: {
                  $ref: '#/definitions/Todo',
                  definitions: {
                    Todo: {
                      title: 'Todo',
                      properties: {
                        title: {type: 'string'},
                      },
                    },
                  },
                },
              },
            },
          },
        },

Shouldn't $ref: '#/definitions/Todo' be $ref: '#/components/schemas/Todo', ?

@bajtos bajtos added the p1 label Apr 9, 2019
@emonddr
Copy link
Contributor

emonddr commented Apr 11, 2019

@bajtos ^ . Thank you.

@emonddr
Copy link
Contributor

emonddr commented Apr 16, 2019

@bajtos , please see our question above. Thank you :)

@bajtos
Copy link
Member Author

bajtos commented Apr 18, 2019

we would like to know if the definitions is actually needed
Shouldn't $ref: '#/definitions/Todo' be $ref: '#/components/schemas/Todo'?

Ah, this is a very good question indeed!

The high-level goal of this story is to allow controller authors to tell LoopBack that the method wants to reference a shared model schema instead of inlining the schema in every place it's used. For example, both find and findById operations use the same schema to describe data of an individual model instance. find returns an array of instances, findById returns a single instance. We don't want to duplicate model schema in each method, we want to use $ref and point to a shared schema.

Now in OpenAPI, shared schemas are stored in components.schemas.{title} and referenced via $ref: '#/components/schemas/{title}'. In JSON Schema, shared schemas are stored in definitions.{title} and referenced via $ref: '#/definitions/{title}'.

I don't recall now why did I use definitions instead of components.schema in my spike. I vaguely remember that

  1. OpenAPI interfaces does not allow components inside schema object, while JSON Schema does allow definitions.
  2. The current implementation of x-ts-type was using definitions from JSON Schema as an extension field added to the standard OpenAPI schema object.

Personally, I don't really mind which approach (definitions vs. components.schemas we choose), as long as it's easy to understand, does not create confusion and preferably preserves backwards compatibility with whatever we already have in place.

I am proposing to make this decision as part of the implementation.

If we decide to change the schema format, then we need to update the description of getJsonSchemaRef helpers in #2631, they will be building on top of this story.

@emonddr
Copy link
Contributor

emonddr commented Apr 18, 2019

The proposal only allows operation-level definitions. We should also allow class level definitions available for re-use across multiple operations.

@bajtos
Copy link
Member Author

bajtos commented Apr 29, 2019

The proposal only allows operation-level definitions. We should also allow class level definitions available for re-use across multiple operations.

That's out of scope of this user story, unless it's trivial to implement. Let's avoid scope creep please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature OpenAPI Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants