-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(repository-json-schema): resolve the circular reference #2690
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -591,6 +591,45 @@ describe('build-schema', () => { | |
| }); | ||
| }); | ||
|
|
||
| context('model conversion', () => { | ||
| @model() | ||
| class Category { | ||
| @property.array(() => Product) | ||
| products?: Product[]; | ||
| } | ||
|
|
||
| @model() | ||
| class Product { | ||
| @property(() => Category) | ||
| category?: Category; | ||
| } | ||
|
|
||
| const expectedSchema = { | ||
| title: 'Category', | ||
| properties: { | ||
| products: { | ||
| type: 'array', | ||
| items: {$ref: '#/definitions/Product'}, | ||
| }, | ||
| }, | ||
| definitions: { | ||
| Product: { | ||
| title: 'Product', | ||
| properties: { | ||
| category: { | ||
| $ref: '#/definitions/Category', | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| it('handles circular references', () => { | ||
| const schema = modelToJsonSchema(Category); | ||
| expect(schema).to.deepEqual(expectedSchema); | ||
| }); | ||
| }); | ||
|
|
||
| function expectValidJsonSchema(schema: JsonSchema) { | ||
| const ajv = new Ajv(); | ||
| const validate = ajv.compile( | ||
|
|
@@ -641,5 +680,54 @@ describe('build-schema', () => { | |
| }, | ||
| }); | ||
| }); | ||
| it('does not pollute the JSON schema options', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we can treat this as a unit test instead. Feel free to ignore. |
||
| @model() | ||
| class Category { | ||
| @property() | ||
| name: string; | ||
| } | ||
|
|
||
| const JSON_SCHEMA_OPTIONS = {}; | ||
| getJsonSchema(Category, JSON_SCHEMA_OPTIONS); | ||
| expect(JSON_SCHEMA_OPTIONS).to.be.empty(); | ||
| }); | ||
| context('circular reference', () => { | ||
| @model() | ||
| class Category { | ||
| @property.array(() => Product) | ||
| products?: Product[]; | ||
| } | ||
|
|
||
| @model() | ||
| class Product { | ||
| @property(() => Category) | ||
| category?: Category; | ||
| } | ||
|
|
||
| const expectedSchemaForCategory = { | ||
| title: 'Category', | ||
| properties: { | ||
| products: { | ||
| type: 'array', | ||
| items: {$ref: '#/definitions/Product'}, | ||
| }, | ||
| }, | ||
| definitions: { | ||
| Product: { | ||
| title: 'Product', | ||
| properties: { | ||
| category: { | ||
| $ref: '#/definitions/Category', | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| it('generates the schema without running into infinite loop', () => { | ||
| const schema = getJsonSchema(Category); | ||
| expect(schema).to.deepEqual(expectedSchemaForCategory); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,18 +14,27 @@ import { | |
| import {JSONSchema6 as JSONSchema} from 'json-schema'; | ||
| import {JSON_SCHEMA_KEY} from './keys'; | ||
|
|
||
| export interface JsonSchemaOptions { | ||
| visited?: {[key: string]: JSONSchema}; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the JSON Schema of a TypeScript model/class by seeing if one exists | ||
| * in a cache. If not, one is generated and then cached. | ||
| * @param ctor Contructor of class to get JSON Schema from | ||
| */ | ||
| export function getJsonSchema(ctor: Function): JSONSchema { | ||
| // NOTE(shimks) currently impossible to dynamically update | ||
| const jsonSchema = MetadataInspector.getClassMetadata(JSON_SCHEMA_KEY, ctor); | ||
| if (jsonSchema) { | ||
| return jsonSchema; | ||
| export function getJsonSchema( | ||
| ctor: Function, | ||
| options?: JsonSchemaOptions, | ||
| ): JSONSchema { | ||
| // In the near future the metadata will be an object with | ||
| // different titles as keys | ||
| const cached = MetadataInspector.getClassMetadata(JSON_SCHEMA_KEY, ctor); | ||
|
|
||
| if (cached) { | ||
| return cached; | ||
| } else { | ||
| const newSchema = modelToJsonSchema(ctor); | ||
| const newSchema = modelToJsonSchema(ctor, options); | ||
| MetadataInspector.defineMetadata(JSON_SCHEMA_KEY.key, newSchema, ctor); | ||
| return newSchema; | ||
| } | ||
|
|
@@ -142,16 +151,26 @@ export function metaToJsonProperty(meta: PropertyDefinition): JSONSchema { | |
| * reflection API | ||
| * @param ctor Constructor of class to convert from | ||
| */ | ||
| export function modelToJsonSchema(ctor: Function): JSONSchema { | ||
| export function modelToJsonSchema( | ||
| ctor: Function, | ||
| jsonSchemaOptions: JsonSchemaOptions = {}, | ||
| ): JSONSchema { | ||
| const options = {...jsonSchemaOptions}; | ||
| options.visited = options.visited || {}; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't modify original
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. applied and new test case added. |
||
|
|
||
| const meta: ModelDefinition | {} = ModelMetadataHelper.getModelMetadata(ctor); | ||
| const result: JSONSchema = {}; | ||
|
|
||
| // returns an empty object if metadata is an empty object | ||
| if (!(meta instanceof ModelDefinition)) { | ||
| return {}; | ||
| } | ||
|
|
||
| result.title = meta.title || ctor.name; | ||
| const title = meta.title || ctor.name; | ||
|
|
||
| if (options.visited[title]) return options.visited[title]; | ||
|
|
||
| const result: JSONSchema = {title}; | ||
| options.visited[title] = result; | ||
|
|
||
| if (meta.description) { | ||
| result.description = meta.description; | ||
|
|
@@ -190,20 +209,24 @@ export function modelToJsonSchema(ctor: Function): JSONSchema { | |
| continue; | ||
| } | ||
|
|
||
| const propSchema = getJsonSchema(referenceType); | ||
| const propSchema = getJsonSchema(referenceType, options); | ||
|
|
||
| includeReferencedSchema(referenceType.name, propSchema); | ||
|
|
||
| if (propSchema && Object.keys(propSchema).length > 0) { | ||
| function includeReferencedSchema(name: string, schema: JSONSchema) { | ||
| if (!schema || !Object.keys(schema).length) return; | ||
| result.definitions = result.definitions || {}; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One idea: since we are effectively storing the schema of visited models in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hmm, do you mean storing the schema of the current model in the Like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, only schema of models used by properties (i.e. |
||
|
|
||
| // delete nested definition | ||
| if (propSchema.definitions) { | ||
| for (const key in propSchema.definitions) { | ||
| result.definitions[key] = propSchema.definitions[key]; | ||
| // promote nested definition to the top level | ||
| if (schema.definitions) { | ||
| for (const key in schema.definitions) { | ||
| if (key === title) continue; | ||
| result.definitions[key] = schema.definitions[key]; | ||
| } | ||
| delete propSchema.definitions; | ||
| delete schema.definitions; | ||
| } | ||
|
|
||
| result.definitions[referenceType.name] = propSchema; | ||
| result.definitions[name] = schema; | ||
| } | ||
| } | ||
| return result; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test to verify
getJsonSchematoo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize
getJsonSchemais the entry point of the schema generation, thought it starts withmodelToJsonSchema.Good catch! Tests added.