Skip to content

Commit

Permalink
fix: deep array merge
Browse files Browse the repository at this point in the history
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
swagger-api/swagger-ui#7618

Fixes: 2f5bb86 ("Fix and test for swagger-ui #3328: swagger-api/swagger-ui#3328. Added manual logic to merge arrays after calling deepExtend within `mergeDeep` function")
  • Loading branch information
lipnitsk committed Jan 4, 2022
1 parent 45d2375 commit 538b180
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 57 deletions.
19 changes: 2 additions & 17 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
41 changes: 3 additions & 38 deletions src/specmap/lib/index.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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.
Expand Down
26 changes: 25 additions & 1 deletion test/specmap/all-of.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -450,6 +450,12 @@ describe('allOf', () => {
{
type: 'object',
required: ['a', 'b'],
properties: {
nested: {
type: 'object',
required: ['e'],
},
},
},
],
},
Expand All @@ -458,6 +464,12 @@ describe('allOf', () => {
{
type: 'object',
required: ['c', 'd'],
properties: {
nested: {
type: 'object',
required: ['f'],
},
},
},
],
},
Expand All @@ -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'],
},
},
},
},
});
Expand Down

0 comments on commit 538b180

Please sign in to comment.