feat: add id with references on schema #77
Conversation
|
Hi @FabioReact! Thank you very much for your PR! Could you adjust the Readme.md as well? And do you think, that this change could be a breaking change? I don‘t think so. But I would like to double check with you! Thanks a lot! 🙏 |
| @@ -60,7 +60,7 @@ function getFormatByDMMFType(fieldType: string): string | undefined { | |||
|
|
|||
| function getJSONSchemaForPropertyReference(field: DMMF.Field): JSONSchema7 { | |||
| const notNullable = field.isRequired || field.isList | |||
| const ref = { $ref: `${DEFINITIONS_ROOT}${field.type}` } | |||
| const ref = { $ref: `${JSON_SCHEMA_ID}${DEFINITIONS_ROOT}${field.type}` } | |||
There was a problem hiding this comment.
Is it really necessary to change the internal references to include the main $id reference? Let‘s have a look at https://json-schema.org/understanding-json-schema/structuring.html#id2. Although $id is set, it is not necessary to include $id in $refs. (bundling section)
“ You should also see that "$ref": "#/definitions/state" resolves to the definitions keyword in the address schema rather than the one at the top level schema like it would if the embedded schema wasn’t used.
Each Schema Resource is evaluated independently and may use different JSON Schema dialects. The example above has the address Schema Resource using Draft 7 while the customer Schema Resource uses Draft 2019-09. If no $schema is declared in an embedded schema, it defaults using to the dialect of the parent schema.“
There was a problem hiding this comment.
Sorry to answer just now. Got caught up by work.
I thought just adding the $id would be enough, but in case of someone using ajv-merge-patch (like i am), if the user do not specify it the $id in the reference, the reference doesn't work as expected.
So i guess in some cases, the precision might be unnecessary/superfluous, but in other cases, it might be needed.
|
|
||
| function getPropertyDefinition( | ||
| model: DMMF.Model, | ||
| ): [name: string, reference: JSONSchema7Definition] { | ||
| return [ | ||
| toCamelCase(model.name), | ||
| { | ||
| $ref: `${DEFINITIONS_ROOT}${model.name}`, | ||
| $ref: `${JSON_SCHEMA_ID}${DEFINITIONS_ROOT}${model.name}`, |
There was a problem hiding this comment.
Same reason as above.
If in original json file i have a reference and i merge it with an extended json, the new schema will think a reference in original file is referencing the extended file (which is not). Precising the $id on reference completely solves the problem.
| @@ -1 +1,2 @@ | |||
| export const DEFINITIONS_ROOT = '#/definitions/' | |||
| export const JSON_SCHEMA_ID = 'prisma-json-schema' | |||
There was a problem hiding this comment.
Do you think it might be necessary or it might be a nice feature to configure the id via generator settings? Then, JSON_SCHEMA_ID is the default one but another value can be passed in via generator options.
There was a problem hiding this comment.
I think it would be great. If no id is provided, the library would have no $id (like it is now). And if someone needs an id, he would just need to specify it. I can make another PR for a feature like that next week.
|
Hey @FabioReact Based upon your changes I have created another PR, but with the option to make the id optional. See here #105. Therefore I will close this one. Nevertheless, thank you for your help! :) |
#73 Hi,
This is my PR to add an id to the generated schema. I hardcoded it in
constants.tsasprisma-json-schema.I also had to update the generated refs for it to work properly if we have to reference the file.
Now, people cam generated schemas and extend it with refs/merge/patch keyword without problems.
Thanks for this library mate ;)