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

feature: Add optional to show schema id instead of def-# #4

Closed

Conversation

IOsonoTAN
Copy link

This PR will improve a thing that i found on fastify-swagger about model section right now it show def-# only on schema id, so i added more option to show schema id from $id instead.

if defined $id of schema as ResponseErrorObject, the result should be like:

Actual:

"definitions": {
  "def-0": {
    "type": "object",
    "properties": {
      "code": { "type": "string" },
      "message": { "type": "string" }
    }
  }
}

Expect:

"definitions": {
  "ResponseErrorObject": {
    "type": "object",
    "properties": {
      "code": { "type": "string" },
      "message": { "type": "string" }
    }
  }
}

@Eomm
Copy link
Owner

Eomm commented Oct 6, 2020

Thks for the quick update 👍

I need a bit more to check:

  • why I opted for the generated def-? - I can't remember right now the root cause or some edge cases
  • how to integrate better with fastify-swagger since this option would not be exposed out of the box

@IOsonoTAN
Copy link
Author

Thks for the quick update 👍

I need a bit more to check:

  • why I opted for the generated def-? - I can't remember right now the root cause or some edge cases
  • how to integrate better with fastify-swagger since this option would not be exposed out of the box

I'm not sure why did you generated, might it be duplicate or something about id/key but I did tested duplicate case it not be an issue right now.

And how to integrate with fastify-swagger after this feature get merged and release as a new version, i'll create new pull-request there to add more option to enable this feature.

Copy link
Owner

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

I'm not sure why did you generated, might it be duplicate or something about id/key but I did tested duplicate case it not be an issue right now.

I got it! The $id is not mandatory so it could be:

  • duplicated: due copy paste and edit by the user
  • missing: the user prefer relative $refs

Moreover the $ref may link an external schema but we need a ONE BIG json schema that transforms the external schema to local ones (using definitions)

For this reason, it is generated.

After this review, that I hope will be helpful for you to go deeper the json schema spec, I think a better approach could be add function as an option that must generate the "local $id".

Like:

json[kRefToDef] = generateLocalReferenceId(id, rolling++)
// id may be null!!

So the user may archive fix the not so nice "def-xxx" keys

@@ -244,3 +244,59 @@ test('absolute $ref #2', t => {
t.equal(out.properties.address.$ref, '#/definitions/def-0')
t.equal(out.properties.houses.items.$ref, '#/definitions/def-0')
})

const phoneNumberSchemaId = {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you remove this const?

This is to avoid issue related to not cloned object, but always fresh ones
I would move it to an external file and load with factory

@@ -0,0 +1,18 @@
module.exports = {
$id: 'personPhoneNumbers',
Copy link
Owner

Choose a reason for hiding this comment

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

This schema is invalid due the missing local $ref
you may check here


t.deepEquals(definitions, {
definitions: {
'http://example.com/phoneNumber': phoneNumberSchemaId
Copy link
Owner

Choose a reason for hiding this comment

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

I would add a more complex test too, described here.

  • one file with an absolute $id http://example.net/root.json with 2 $ref
    • one relative ref to a local definition for the '#/definitions/phoneNumber
    • one relative ref to an external definition for the external.json#/definitions/phoneNumber
  • one file with absolute $id http://example.net/external.json

clone: true,
showSchemaId: true
})
const out = resolver.resolve(schema, {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure this is working since the out is equals to factory('relativeId-showSchemaId')
So nothing has been resolved

The purpose of this lib (in conjunction with fastify-swagger) is to transform external $ref to local definition refs.

To do it you can:

  • create a single json out of the box with the definitions
  • use $ref to reference external schema

Since this module finds a relative schema, it will no resolve the $ref since it assumes that the definitions it is already there - as it should be.

So we need to change these test to absolute references instead and fix invalid local references

@Alex-Ferreli
Copy link

Any news? This PR will fix bugs generated on other plugins (fastify-swagger for example).

@IOsonoTAN
Copy link
Author

Sorry everyone, I just come back from heavy work load and I remember i've a solution for fix $id and $ref but I didn't try yet so I'll try to finish it within this weekend and I may create some code changed again.

@anonrig
Copy link

anonrig commented Mar 17, 2021

Hey @IOsonoTAN

Is there any update on this issue? Do you need any help? Can you update us?

@Eomm
Copy link
Owner

Eomm commented Mar 17, 2021

I have planned to push this PR forward a bit
This feature is highly requested 😄

@CloudPower97
Copy link

Any updates on this?
I'm facing this issue using fastify-swagger and as of now this is a very big inconvenient for me. :/

@Eomm
Copy link
Owner

Eomm commented Aug 30, 2021

Released in 1.3.0

Closing

@Eomm Eomm closed this Aug 30, 2021
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.

5 participants