From 538b18030e9cd5f3950ea323bd3e4862ae061105 Mon Sep 17 00:00:00 2001 From: Ilya Lipnitskiy Date: Sat, 6 Nov 2021 14:01:01 -0700 Subject: [PATCH] fix: deep array merge deep-extend does not support array merge. There was special code added to merge top-level arrays, but that was a shallow merge. Use deepmerge instead of deep-extend to merge arrays also. Default merge settings seem to work well - all tests pass. Extend all-of merge test case based on https://github.com/swagger-api/swagger-ui/issues/7618 Fixes: 2f5bb86c9b88 ("Fix and test for swagger-ui #3328: https://github.com/swagger-api/swagger-ui/issues/3328. Added manual logic to merge arrays after calling deepExtend within `mergeDeep` function") --- package-lock.json | 19 ++----------------- package.json | 2 +- src/specmap/lib/index.js | 41 +++------------------------------------- test/specmap/all-of.js | 26 ++++++++++++++++++++++++- 4 files changed, 31 insertions(+), 57 deletions(-) diff --git a/package-lock.json b/package-lock.json index 394ca8e5f..52ba5d210 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,7 +13,7 @@ "btoa": "^1.2.1", "cookie": "~0.4.1", "cross-fetch": "^3.1.4", - "deep-extend": "~0.6.0", + "deepmerge": "~4.2.2", "fast-json-patch": "^3.0.0-1", "form-data-encoder": "^1.4.3", "formdata-node": "^4.0.0", @@ -9307,14 +9307,6 @@ "integrity": "sha1-JJXduvbrh0q7Dhvp3yLS5aVEMmw=", "dev": true }, - "node_modules/deep-extend": { - "version": "0.6.0", - "resolved": "https://registry.npmjs.org/deep-extend/-/deep-extend-0.6.0.tgz", - "integrity": "sha512-LOHxIOaPYdHlJRtCQfDIVZtfw/ufM8+rVj649RIHzcm/vGwQRXFt6OPqIFWsm2XEMrNIEtWR64sY1LEKD2vAOA==", - "engines": { - "node": ">=4.0.0" - } - }, "node_modules/deep-is": { "version": "0.1.4", "resolved": "https://registry.npmjs.org/deep-is/-/deep-is-0.1.4.tgz", @@ -9325,7 +9317,6 @@ "version": "4.2.2", "resolved": "https://registry.npmjs.org/deepmerge/-/deepmerge-4.2.2.tgz", "integrity": "sha512-FJ3UgI4gIl+PHZm53knsuSFpE+nESMr7M4v9QcgB7S63Kj/6WqMiFQJpBBYz1Pt+66bZpP3Q7Lye0Oo9MPKEdg==", - "dev": true, "engines": { "node": ">=0.10.0" } @@ -24099,11 +24090,6 @@ "integrity": "sha1-JJXduvbrh0q7Dhvp3yLS5aVEMmw=", "dev": true }, - "deep-extend": { - "version": "0.6.0", - "resolved": "https://registry.npmjs.org/deep-extend/-/deep-extend-0.6.0.tgz", - "integrity": "sha512-LOHxIOaPYdHlJRtCQfDIVZtfw/ufM8+rVj649RIHzcm/vGwQRXFt6OPqIFWsm2XEMrNIEtWR64sY1LEKD2vAOA==" - }, "deep-is": { "version": "0.1.4", "resolved": "https://registry.npmjs.org/deep-is/-/deep-is-0.1.4.tgz", @@ -24113,8 +24099,7 @@ "deepmerge": { "version": "4.2.2", "resolved": "https://registry.npmjs.org/deepmerge/-/deepmerge-4.2.2.tgz", - "integrity": "sha512-FJ3UgI4gIl+PHZm53knsuSFpE+nESMr7M4v9QcgB7S63Kj/6WqMiFQJpBBYz1Pt+66bZpP3Q7Lye0Oo9MPKEdg==", - "dev": true + "integrity": "sha512-FJ3UgI4gIl+PHZm53knsuSFpE+nESMr7M4v9QcgB7S63Kj/6WqMiFQJpBBYz1Pt+66bZpP3Q7Lye0Oo9MPKEdg==" }, "define-properties": { "version": "1.1.3", diff --git a/package.json b/package.json index 6d9602945..7c5e17415 100644 --- a/package.json +++ b/package.json @@ -109,7 +109,7 @@ "btoa": "^1.2.1", "cookie": "~0.4.1", "cross-fetch": "^3.1.4", - "deep-extend": "~0.6.0", + "deepmerge": "~4.2.2", "fast-json-patch": "^3.0.0-1", "form-data-encoder": "^1.4.3", "formdata-node": "^4.0.0", diff --git a/src/specmap/lib/index.js b/src/specmap/lib/index.js index a1f5bc79f..159318864 100644 --- a/src/specmap/lib/index.js +++ b/src/specmap/lib/index.js @@ -1,6 +1,5 @@ import * as jsonPatch from 'fast-json-patch'; -import deepExtend from 'deep-extend'; -import cloneDeep from 'lodash/cloneDeep'; +import deepmerge from 'deepmerge'; export default { add, @@ -40,42 +39,8 @@ function applyPatch(obj, patch, opts) { jsonPatch.applyPatch(obj, [replace(patch.path, newValue)]); } else if (patch.op === 'mergeDeep') { const currentValue = getInByJsonPath(obj, patch.path); - - // Iterate the properties of the patch - // eslint-disable-next-line no-restricted-syntax, guard-for-in - for (const prop in patch.value) { - const propVal = patch.value[prop]; - const isArray = Array.isArray(propVal); - if (isArray) { - // deepExtend doesn't merge arrays, so we will do it manually - const existing = currentValue[prop] || []; - currentValue[prop] = existing.concat(propVal); - } else if (isObject(propVal) && !isArray) { - // If it's an object, iterate it's keys and merge - // if there are conflicting keys, merge deep, otherwise shallow merge - let currentObj = { ...currentValue[prop] }; - // eslint-disable-next-line no-restricted-syntax - for (const key in propVal) { - if (Object.prototype.hasOwnProperty.call(currentObj, key)) { - // if there is a single conflicting key, just deepExtend the entire value - // and break from the loop (since all future keys are also merged) - // We do this because we can't deepExtend two primitives - // (currentObj[key] & propVal[key] may be primitives). - // - // we also deeply assign here, since we aren't in control of - // how deepExtend affects existing nested objects - currentObj = deepExtend(cloneDeep(currentObj), propVal); - break; - } else { - Object.assign(currentObj, { [key]: propVal[key] }); - } - } - currentValue[prop] = currentObj; - } else { - // It's a primitive, just replace existing - currentValue[prop] = propVal; - } - } + const newValue = deepmerge(currentValue, patch.value); + obj = jsonPatch.applyPatch(obj, [replace(patch.path, newValue)]).newDocument; } else if (patch.op === 'add' && patch.path === '' && isObject(patch.value)) { // { op: 'add', path: '', value: { a: 1, b: 2 }} // has no effect: json patch refuses to do anything. diff --git a/test/specmap/all-of.js b/test/specmap/all-of.js index ef85b9425..9e663ad99 100644 --- a/test/specmap/all-of.js +++ b/test/specmap/all-of.js @@ -436,7 +436,7 @@ describe('allOf', () => { }); }); - test('merges arrays inside of an `allOf`', () => + test('deepmerges arrays inside of an `allOf`', () => mapSpec({ plugins: [plugins.refs, plugins.allOf], showDebug: true, @@ -450,6 +450,12 @@ describe('allOf', () => { { type: 'object', required: ['a', 'b'], + properties: { + nested: { + type: 'object', + required: ['e'], + }, + }, }, ], }, @@ -458,6 +464,12 @@ describe('allOf', () => { { type: 'object', required: ['c', 'd'], + properties: { + nested: { + type: 'object', + required: ['f'], + }, + }, }, ], }, @@ -470,10 +482,22 @@ describe('allOf', () => { one: { type: 'object', required: ['c', 'd', 'a', 'b'], + properties: { + nested: { + type: 'object', + required: ['f', 'e'], + }, + }, }, two: { type: 'object', required: ['c', 'd'], + properties: { + nested: { + type: 'object', + required: ['f'], + }, + }, }, }, });