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: customize def-0 model name #459

Merged
merged 2 commits into from
Sep 7, 2021
Merged

feat: customize def-0 model name #459

merged 2 commits into from
Sep 7, 2021

Conversation

Eomm
Copy link
Member

@Eomm Eomm commented Aug 30, 2021

This PR:

  • fixes the swagger UI layout to show the schema's $id as model name (see screenshot at the bottom). Under the hood, the def-0 renaming is still in place.
  • adds documentation to describe what is happening under the hood.
  • adds the option to customize the $ref's link into the inline schema. This gives to the user the chance to do what he/she wants and remove totally the def-0 logic.

I think this (too simple?) solution solves the case.
Feel free to give feedback about this PR

Why this change

To avoid a major bump, this PR fixes the view issue without breaking the def-0 logic that has been developed to protect users actually 😄

Let me explain.

This JSON Schema has a relative URI $id.

fastify.addSchema({
  $id: 'user',
  type: 'object',
  definitions: {
    id: {
      type: 'string',
      description: 'user id'
    }
  }
})

This is wrong from the DRAFT 07 specification:

The root schema of a JSON Schema document SHOULD contain an "$id"
keyword with an absolute-URI [RFC3986] (containing a scheme, but no
fragment), or this absolute URI but with an empty fragment.

Moreover, referencing the user schema with the $ref: 'user#/definitions/id' is wrong from the specification point of view: the reference is relative and it should be resolved using the local schema's root URI, not the external one (the $id: 'user' one to be clear).

The fastify-swagger knows that the $id field is used as a simple key/value pair actually.
For this reason, it ignores the user's $id*** and deference the schema to a def-0: to be sure to avoid conflicts to let the swagger-ui module resolve correctly the references. Otherwise, you may have different output based on the $ids the developer set.
The specification, cover a lot more use cases than I saw in the GitHub issues actually.

The last example, setting an absolute root URI tells to the swagger-ui module to fetch the schema from the link!

fastify.addSchema({
  $id: 'http://example.com/user.json',
  ...

This snippet causes the swagger-ui module to make an HTTP request to http://example.com/user.json!

Now, forcing fastify-swagger to use the $id can break the configuration of those devs that use the standard strictly.
Using the $id in place of def-0 is correct only if the developer knows that there is no conflicts or nested schema's declarations.
With this PR, the user may replace the buildLocalReference option to accomplish this. But this behaviour leads to a "convention over configuration" strategy.

Before After
image image

Fixes #337
Fixes #286
Fixes #225
Should help #432

*** when I say that this module "ignores" the $ids I mean that we know that all the $ref can be resolved locally, accessing the data added to the fastify schema via the addSchema method. If the user set an absolute URI as $id, fastify will never look up an external link.

@Eomm
Copy link
Member Author

Eomm commented Aug 30, 2021

I think CI is failing due a temporary issue:

throw new Error('Parallel webhook error:' + err + ', ' + JSON.stringify(data));

locally all is fine and 100% coverage reached

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1182925217

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1175256513: 0.0%
Covered Lines: 542
Relevant Lines: 542

💛 - Coveralls

@Eomm Eomm requested review from climba03003 and jsumners August 31, 2021 10:33
@coveralls
Copy link

coveralls commented Aug 31, 2021

Pull Request Test Coverage Report for Build 1182925217

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1175256513: 0.0%
Covered Lines: 542
Relevant Lines: 542

💛 - Coveralls

@barticus
Copy link

barticus commented Sep 2, 2021

It would be great to have some exported helper functions for use with buildLocalReference, e.g.

  • BuildLocalReferencesSequentially (default)
  • BuildLocalReferencesById (what you've shown you can do)

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

It means that we keep the original $id and move the resolution works to title field? Or the reverse way?

@Eomm
Copy link
Member Author

Eomm commented Sep 3, 2021

It means that we keep the original $id and move the resolution works to title field? Or the reverse way?

No.

We add a title to fix the visual issue: def-0 will never show up again reading the swagger documentation

The $id was and will not be changed

The $ref will continue to link relative schemas named def-0, def-1 etc.. but it can be customized and removed by the user with the new configuration.

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Eomm Eomm merged commit 560d2dc into master Sep 7, 2021
@Eomm Eomm deleted the def-0-remove branch September 7, 2021 08:51

const swaggerObject = fastify.swagger()
const api = await Swagger.validate(swaggerObject)
t.match(api.components.schemas['def-0'], schema)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this schema, which was added with an $id, not longer show the id via the api?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is explained in the first post of this pr

@cesarquiroz
Copy link

Hey guys, I kind of have a similar issue, but Im using TypeScript and it looks like that refResolver wasn't exposed on the TypeScript definition, is there a way to get this fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants