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

feat(openapi-v3): allow controller to reference models via openapispec #2898

Merged
merged 1 commit into from
May 17, 2019

Conversation

nabdelgadir
Copy link
Contributor

Closes #2629.

Controller methods can now reference model schema directly via OpenAPI spec and without using the x-ts-type extension

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 👈

@nabdelgadir nabdelgadir self-assigned this May 14, 2019
});
});

it('returns an empty object when it cannot find definition of referenced model', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we report it as an error for unresolved $ref or defer it to other definitions outside of the controller?

},
});
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test case to allow @api at controller class level to supply definitions so that they can be shared by multiple methods in the controller?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was talking with @nabdelgadir , I think the @api() decorator takes in a whole OpenAPI spec described by interface ControllerSpec, so if a user wants to provide the model schema, the spec should be sth like:

{
  components: {
    schemas: {
      Todo: {
        // The Todo schema
      }
    }
  },
  paths: {
    // some path spec refers to the Todo schema provided in the root level property 
    // `components`, instead of providing the definition in itself.
  }
}

Copy link
Contributor Author

@nabdelgadir nabdelgadir May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test in this commit, although I'm not sure it's what you meant. I thought that OpenAPI spec had shared definitions in #/components/schemas? Can you please provide an example of what you meant?

Edit: oops posted this before I saw Janny's comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannyHou Good point. We already have the ability to define schema at @api level. With that in mind, what's the additional value to allow definitions? Is it just for full support of json schema?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raymondfeng I think definitions is the way to add schema reference in the method level metadata, the reason is explained in Miroslav's comment: #2629 (comment).
@api() doesn't have that limitation :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Schema references can be used at method level regardless where the target schema is provided. With this PR, we allow the target schema to be defined in two ways:

  1. At @api level - inside components.schemas.<target>
  2. Use definitions with $ref:
schema: {
  $ref: #/definitions/Todo,
  definitions: {/* ... */} // [raymond] I assume the schema definitions are going to be visible to other methods?
}

Copy link
Contributor Author

@nabdelgadir nabdelgadir May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raymondfeng Since the definition will become a part of #/components/schemas, they should be visible to other methods.

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, got a question and +1 to @raymondfeng's comments.

packages/openapi-v3/src/controller-spec.ts Show resolved Hide resolved
@nabdelgadir nabdelgadir force-pushed the operation-spec-ref branch from 7e75867 to e738d63 Compare May 15, 2019 20:12
@@ -332,6 +333,66 @@ describe('controller spec', () => {
expect(globalSchemas).to.be.empty();
});

it('allows a class to provide definitions of referenced models through #/components/schemas', () => {
@api({
paths: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is to only have schemas at @api level for the controller class, not individual operations.

@nabdelgadir nabdelgadir force-pushed the operation-spec-ref branch 2 times, most recently from 0ccf8d2 to 8172c46 Compare May 16, 2019 13:54
Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash all commits before landing.

@nabdelgadir nabdelgadir force-pushed the operation-spec-ref branch from 3eec6a5 to ea45a99 Compare May 17, 2019 17:15
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice additions from @api() decorator 💯

Controller methods can now reference model schema through OpenAPI spec without the need to use the `x-ts-type` extension.

Co-authored-by: Miroslav Bajtoš <[email protected]>
@nabdelgadir nabdelgadir force-pushed the operation-spec-ref branch from ea45a99 to 656aed8 Compare May 17, 2019 18:31
@nabdelgadir nabdelgadir merged commit d57f272 into master May 17, 2019
@nabdelgadir nabdelgadir deleted the operation-spec-ref branch May 17, 2019 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow controllers to provide definitions of models referenced in operation spec
4 participants