From 5cf259f1d1448c470058f4f4f2613dd158e5faa9 Mon Sep 17 00:00:00 2001 From: KaKa Date: Sat, 18 Jun 2022 03:08:55 +0800 Subject: [PATCH 1/3] fix: dedupe id in object --- index.js | 33 ++++++++++++++-- test/ref.test.js | 99 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 19529050..e3bfb2c3 100644 --- a/index.js +++ b/index.js @@ -52,6 +52,7 @@ function mergeLocation (source, dest) { const arrayItemsReferenceSerializersMap = new Map() const objectReferenceSerializersMap = new Map() const schemaReferenceMap = new Map() +const dedupRefsMap = new Map() let ajvInstance = null let contextFunctions = null @@ -234,7 +235,7 @@ function addPatternProperties (location) { let ppLocation = mergeLocation(location, { schema: pp[regex] }) if (pp[regex].$ref) { ppLocation = refFinder(pp[regex].$ref, location) - pp[regex] = ppLocation.schema + pp[regex] = dedupIdsRefs(ppLocation.schema) } try { @@ -253,6 +254,8 @@ function addPatternProperties (location) { } ` }) + dedupRefsMap.clear() + if (schema.additionalProperties) { code += additionalProperty(location) } @@ -447,9 +450,10 @@ function buildCode (location, locationPath) { Object.keys(schema.properties || {}).forEach((key) => { let propertyLocation = mergeLocation(location, { schema: schema.properties[key] }) - if (schema.properties[key].$ref) { - propertyLocation = refFinder(schema.properties[key].$ref, location) - schema.properties[key] = propertyLocation.schema + const ref = schema.properties[key].$ref + if (ref) { + propertyLocation = refFinder(ref, location) + schema.properties[key] = dedupIdsRefs(key, ref, propertyLocation.schema) } // Using obj['key'] !== undefined instead of obj.hasOwnProperty(prop) for perf reasons, @@ -484,6 +488,7 @@ function buildCode (location, locationPath) { } ` }) + dedupRefsMap.clear() for (const requiredProperty of required) { if (schema.properties && schema.properties[requiredProperty] !== undefined) continue @@ -884,6 +889,26 @@ function dereferenceOfRefs (location, type) { return locations } +function dedupIdsRefs (key, refs, schema) { + const arr = dedupRefsMap.get(refs) || [] + arr.push(key) + dedupRefsMap.set(refs, [key]) + + // we dedup the same $ref only and clear the $id after the first one. + // so, it can keep track the duplicate $id when it trying to resolve + // difference schema. + // however, it will not works for $id in nested properties. + if (arr.length > 1) { + // we need to check if we face the recursive schema + if (!objectReferenceSerializersMap.has(schema) && !arrayItemsReferenceSerializersMap.has(schema)) { + schema = clone(schema) + delete schema.$id + } + } + + return schema +} + let genFuncNameCounter = 0 function generateFuncName () { return 'anonymous' + genFuncNameCounter++ diff --git a/test/ref.test.js b/test/ref.test.js index fbf9d117..513762a7 100644 --- a/test/ref.test.js +++ b/test/ref.test.js @@ -1249,3 +1249,102 @@ test('Regression 2.5.2', t => { t.equal(output, '[{"field":"parent","sub":{"field":"joined"}}]') }) + +test('ref with same id in properties', (t) => { + t.plan(2) + + const externalSchema = { + ObjectId: { + $id: 'ObjectId', + type: 'string' + }, + File: { + $id: 'File', + type: 'object', + properties: { + _id: { $ref: 'ObjectId' }, + name: { type: 'string' }, + owner: { $ref: 'ObjectId' } + } + } + } + + t.test('anyOf', (t) => { + t.plan(1) + + const schema = { + $id: 'Article', + type: 'object', + properties: { + _id: { $ref: 'ObjectId' }, + image: { + anyOf: [ + { $ref: 'File' }, + { type: 'null' } + ] + } + } + } + + const stringify = build(schema, { schema: externalSchema }) + const output = stringify({ _id: 'foo', image: { _id: 'bar', name: 'hello', owner: 'baz' } }) + + t.equal(output, '{"_id":"foo","image":{"_id":"bar","name":"hello","owner":"baz"}}') + }) + + t.test('oneOf', (t) => { + t.plan(1) + + const schema = { + $id: 'Article', + type: 'object', + properties: { + _id: { $ref: 'ObjectId' }, + image: { + oneOf: [ + { $ref: 'File' }, + { type: 'null' } + ] + } + } + } + + const stringify = build(schema, { schema: externalSchema }) + const output = stringify({ _id: 'foo', image: { _id: 'bar', name: 'hello', owner: 'baz' } }) + + t.equal(output, '{"_id":"foo","image":{"_id":"bar","name":"hello","owner":"baz"}}') + }) +}) + +test('dedup id', (t) => { + t.plan(1) + + try { + const externalSchema = { + ObjectId: { + $id: 'ObjectId', + type: 'string' + } + } + + const schema = { + type: 'object', + properties: { + image: { + anyOf: [ + { $ref: 'ObjectId' }, + { + $id: 'ObjectId', + type: 'number' + } + ] + } + } + } + + build(schema, { schema: externalSchema }) + t.fail('should not be here') + } catch (err) { + t.pass('should fail when $id reference to multiple schema') + } +}) From f5f508589063d95276625f41a77a59f879e4a641 Mon Sep 17 00:00:00 2001 From: KaKa Date: Sat, 18 Jun 2022 04:57:22 +0800 Subject: [PATCH 2/3] fix: better dedup based on --- index.js | 22 ++++---- test/ref.test.js | 132 ++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 129 insertions(+), 25 deletions(-) diff --git a/index.js b/index.js index e3bfb2c3..72b9fd59 100644 --- a/index.js +++ b/index.js @@ -52,7 +52,7 @@ function mergeLocation (source, dest) { const arrayItemsReferenceSerializersMap = new Map() const objectReferenceSerializersMap = new Map() const schemaReferenceMap = new Map() -const dedupRefsMap = new Map() +const dedupRefsSet = new Set() let ajvInstance = null let contextFunctions = null @@ -61,6 +61,7 @@ function build (schema, options) { arrayItemsReferenceSerializersMap.clear() objectReferenceSerializersMap.clear() schemaReferenceMap.clear() + dedupRefsSet.clear() contextFunctions = [] options = options || {} @@ -151,6 +152,7 @@ function build (schema, options) { arrayItemsReferenceSerializersMap.clear() objectReferenceSerializersMap.clear() schemaReferenceMap.clear() + dedupRefsSet.clear() return stringifyFunc } @@ -235,7 +237,7 @@ function addPatternProperties (location) { let ppLocation = mergeLocation(location, { schema: pp[regex] }) if (pp[regex].$ref) { ppLocation = refFinder(pp[regex].$ref, location) - pp[regex] = dedupIdsRefs(ppLocation.schema) + pp[regex] = ppLocation.schema } try { @@ -254,7 +256,6 @@ function addPatternProperties (location) { } ` }) - dedupRefsMap.clear() if (schema.additionalProperties) { code += additionalProperty(location) @@ -329,7 +330,7 @@ function refFinder (ref, location) { if (externalSchema && externalSchema[ref]) { return { - schema: externalSchema[ref], + schema: dedupIdsRefs(ref, externalSchema[ref]), root: externalSchema[ref], externalSchema } @@ -453,7 +454,7 @@ function buildCode (location, locationPath) { const ref = schema.properties[key].$ref if (ref) { propertyLocation = refFinder(ref, location) - schema.properties[key] = dedupIdsRefs(key, ref, propertyLocation.schema) + schema.properties[key] = propertyLocation.schema } // Using obj['key'] !== undefined instead of obj.hasOwnProperty(prop) for perf reasons, @@ -488,7 +489,6 @@ function buildCode (location, locationPath) { } ` }) - dedupRefsMap.clear() for (const requiredProperty of required) { if (schema.properties && schema.properties[requiredProperty] !== undefined) continue @@ -889,16 +889,12 @@ function dereferenceOfRefs (location, type) { return locations } -function dedupIdsRefs (key, refs, schema) { - const arr = dedupRefsMap.get(refs) || [] - arr.push(key) - dedupRefsMap.set(refs, [key]) - +function dedupIdsRefs (refs, schema) { // we dedup the same $ref only and clear the $id after the first one. // so, it can keep track the duplicate $id when it trying to resolve // difference schema. // however, it will not works for $id in nested properties. - if (arr.length > 1) { + if (dedupRefsSet.has(refs)) { // we need to check if we face the recursive schema if (!objectReferenceSerializersMap.has(schema) && !arrayItemsReferenceSerializersMap.has(schema)) { schema = clone(schema) @@ -906,6 +902,8 @@ function dedupIdsRefs (key, refs, schema) { } } + dedupRefsSet.add(refs) + return schema } diff --git a/test/ref.test.js b/test/ref.test.js index 513762a7..298d960d 100644 --- a/test/ref.test.js +++ b/test/ref.test.js @@ -1316,35 +1316,141 @@ test('ref with same id in properties', (t) => { }) }) -test('dedup id', (t) => { - t.plan(1) +test('dedup refs id', (t) => { + t.plan(3) + + const externalSchema = { + ObjectId: { + $id: 'ObjectId', + type: 'string' + }, + String: { + $id: 'String', + type: 'string' + } + } + + t.test('should treat same $id but not reference by $ref as different schema', (t) => { + t.plan(1) + + try { + const schema = { + type: 'object', + properties: { + image: { + anyOf: [ + { $ref: 'ObjectId' }, + { + $id: 'ObjectId', + type: 'number' + } + ] + } + } + } + + build(schema, { schema: externalSchema }) + t.fail('should not be here') + } catch (err) { + t.pass('should fail when $id reference to multiple schema') + } + }) + + t.test('same id refs cross anyOf', (t) => { + t.plan(4) - try { const externalSchema = { ObjectId: { $id: 'ObjectId', type: 'string' + }, + String: { + $id: 'String', + type: 'string' } } const schema = { type: 'object', properties: { + _id: { $ref: 'ObjectId' }, image: { anyOf: [ - { $ref: 'ObjectId' }, - { - $id: 'ObjectId', - type: 'number' - } + { type: 'object', properties: { foo: { $ref: 'String' } }, required: ['foo'] }, + { type: 'object', properties: { bar: { $ref: 'String' } }, required: ['bar'] }, + { type: 'object', properties: { baz: { $ref: 'String' } }, required: ['baz'] }, + { $ref: 'ObjectId' } ] } } } - build(schema, { schema: externalSchema }) - t.fail('should not be here') - } catch (err) { - t.pass('should fail when $id reference to multiple schema') - } + const stringify = build(schema, { schema: externalSchema }) + + { + const output = stringify({ _id: 'foo', image: { foo: 'hello' } }) + t.equal(output, '{"_id":"foo","image":{"foo":"hello"}}') + } + { + const output = stringify({ _id: 'foo', image: { bar: 'hello' } }) + t.equal(output, '{"_id":"foo","image":{"bar":"hello"}}') + } + { + const output = stringify({ _id: 'foo', image: { baz: 'hello' } }) + t.equal(output, '{"_id":"foo","image":{"baz":"hello"}}') + } + { + const output = stringify({ _id: 'foo', image: 'hello' }) + t.equal(output, '{"_id":"foo","image":"hello"}') + } + }) + + t.test('same id refs cross oneOf', (t) => { + t.plan(4) + + const externalSchema = { + ObjectId: { + $id: 'ObjectId', + type: 'string' + }, + String: { + $id: 'String', + type: 'string' + } + } + + const schema = { + type: 'object', + properties: { + _id: { $ref: 'ObjectId' }, + image: { + oneOf: [ + { type: 'object', properties: { foo: { $ref: 'String' } }, required: ['foo'] }, + { type: 'object', properties: { bar: { $ref: 'String' } }, required: ['bar'] }, + { type: 'object', properties: { baz: { $ref: 'String' } }, required: ['baz'] }, + { $ref: 'ObjectId' } + ] + } + } + } + + const stringify = build(schema, { schema: externalSchema }) + + { + const output = stringify({ _id: 'foo', image: { foo: 'hello' } }) + t.equal(output, '{"_id":"foo","image":{"foo":"hello"}}') + } + { + const output = stringify({ _id: 'foo', image: { bar: 'hello' } }) + t.equal(output, '{"_id":"foo","image":{"bar":"hello"}}') + } + { + const output = stringify({ _id: 'foo', image: { baz: 'hello' } }) + t.equal(output, '{"_id":"foo","image":{"baz":"hello"}}') + } + { + const output = stringify({ _id: 'foo', image: 'hello' }) + t.equal(output, '{"_id":"foo","image":"hello"}') + } + }) }) From 6dee402e72e6feca2f67634f0b81f06dc970baae Mon Sep 17 00:00:00 2001 From: KaKa Date: Sat, 18 Jun 2022 05:09:53 +0800 Subject: [PATCH 3/3] fix: edge case --- index.js | 22 ++++++++++++++- test/ref.test.js | 71 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 72b9fd59..940ca841 100644 --- a/index.js +++ b/index.js @@ -53,6 +53,7 @@ const arrayItemsReferenceSerializersMap = new Map() const objectReferenceSerializersMap = new Map() const schemaReferenceMap = new Map() const dedupRefsSet = new Set() +let dedupTrack = [] let ajvInstance = null let contextFunctions = null @@ -62,6 +63,7 @@ function build (schema, options) { objectReferenceSerializersMap.clear() schemaReferenceMap.clear() dedupRefsSet.clear() + dedupTrack = [] contextFunctions = [] options = options || {} @@ -153,6 +155,7 @@ function build (schema, options) { objectReferenceSerializersMap.clear() schemaReferenceMap.clear() dedupRefsSet.clear() + dedupTrack = [] return stringifyFunc } @@ -600,6 +603,8 @@ function buildInnerObject (location, locationPath) { } function addIfThenElse (location, locationPath) { + dedupTrack.push('ifThenElse') + let code = '' const schema = location.schema @@ -645,6 +650,9 @@ function addIfThenElse (location, locationPath) { code += ` } ` + + dedupTrack.pop() + return code } @@ -889,7 +897,7 @@ function dereferenceOfRefs (location, type) { return locations } -function dedupIdsRefs (refs, schema) { +function _dedupIdsRefs (refs, schema) { // we dedup the same $ref only and clear the $id after the first one. // so, it can keep track the duplicate $id when it trying to resolve // difference schema. @@ -907,6 +915,15 @@ function dedupIdsRefs (refs, schema) { return schema } +function _dummyDedupIdsRefs (_, schema) { + return schema +} + +function dedupIdsRefs (refs, schema) { + // we should ignore the $refs in root schema + return dedupTrack.join(',') === '' ? _dummyDedupIdsRefs(refs, schema) : _dedupIdsRefs(refs, schema) +} + let genFuncNameCounter = 0 function generateFuncName () { return 'anonymous' + genFuncNameCounter++ @@ -972,6 +989,7 @@ function buildValue (locationPath, input, location) { break case undefined: if (schema.anyOf || schema.oneOf) { + dedupTrack.push(schema.anyOf ? 'anyOf' : 'oneOf') // beware: dereferenceOfRefs has side effects and changes schema.anyOf const locations = dereferenceOfRefs(location, schema.anyOf ? 'anyOf' : 'oneOf') locations.forEach((location, index) => { @@ -999,6 +1017,8 @@ function buildValue (locationPath, input, location) { ` }) + dedupTrack.pop() + code += ` else throw new Error(\`The value $\{JSON.stringify(${input})} does not match schema definition.\`) ` diff --git a/test/ref.test.js b/test/ref.test.js index 298d960d..a5f3fe6c 100644 --- a/test/ref.test.js +++ b/test/ref.test.js @@ -1317,7 +1317,7 @@ test('ref with same id in properties', (t) => { }) test('dedup refs id', (t) => { - t.plan(3) + t.plan(5) const externalSchema = { ObjectId: { @@ -1337,6 +1337,7 @@ test('dedup refs id', (t) => { const schema = { type: 'object', properties: { + _id: { $ref: 'ObjectId' }, image: { anyOf: [ { $ref: 'ObjectId' }, @@ -1453,4 +1454,72 @@ test('dedup refs id', (t) => { t.equal(output, '{"_id":"foo","image":"hello"}') } }) + + t.test('same id refs cross multiple anyOf', (t) => { + t.plan(1) + + const externalSchema = { + ObjectId: { + $id: 'ObjectId', + type: 'string' + } + } + + const schema = { + type: 'object', + properties: { + _id: { $ref: 'ObjectId' }, + image: { + anyOf: [ + { $ref: 'ObjectId' } + ] + }, + owner: { + anyOf: [ + { $ref: 'ObjectId' } + ] + } + } + } + + const stringify = build(schema, { schema: externalSchema }) + { + const output = stringify({ _id: 'foo', image: 'hello', owner: 'world' }) + t.equal(output, '{"_id":"foo","image":"hello","owner":"world"}') + } + }) + + t.test('same id refs cross multiple oneOf', (t) => { + t.plan(1) + + const externalSchema = { + ObjectId: { + $id: 'ObjectId', + type: 'string' + } + } + + const schema = { + type: 'object', + properties: { + _id: { $ref: 'ObjectId' }, + image: { + oneOf: [ + { $ref: 'ObjectId' } + ] + }, + owner: { + oneOf: [ + { $ref: 'ObjectId' } + ] + } + } + } + + const stringify = build(schema, { schema: externalSchema }) + { + const output = stringify({ _id: 'foo', image: 'hello', owner: 'world' }) + t.equal(output, '{"_id":"foo","image":"hello","owner":"world"}') + } + }) })