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. Add a 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")
swagger-api · Nov 29, 2021 · 4d81f82 · 4d81f82
1 parent abe3e36
commit 4d81f82
Show file tree

Hide file tree

Showing 3 changed files with 221 additions and 39 deletions.
diff --git a/package.json b/package.json
@@ -112,7 +112,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
@@ -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/bugs/ui-7618.js b/test/bugs/ui-7618.js
@@ -0,0 +1,217 @@
+// https://github.com/swagger-api/swagger-ui/issues/7618
+
+import resolveSubtree from '../../src/subtree-resolver/index.js';
+
+const spec = {
+  openapi: '3.0.3',
+  info: {
+    version: '1.0.0',
+    title: 'test',
+    description: 'test',
+    contact: {
+      name: 'test',
+      email: '[email protected]',
+    },
+  },
+  servers: [
+    {
+      url: 'https://localhost/api',
+    },
+  ],
+  tags: [
+    {
+      name: 'Test',
+      description: 'Test',
+    },
+  ],
+  paths: {
+    '/one': {
+      get: {
+        description: 'test',
+        operationId: 'one',
+        tags: ['Test'],
+        parameters: [
+          {
+            name: 'test',
+            in: 'header',
+            required: true,
+            schema: {
+              type: 'string',
+            },
+          },
+        ],
+        responses: {
+          default: {
+            description: 'Response',
+            content: {
+              'text/plain': {
+                schema: {
+                  type: 'integer',
+                  enum: [401, 404, 500],
+                },
+              },
+            },
+          },
+        },
+      },
+    },
+  },
+  components: {
+    schemas: {
+      parent: {
+        type: 'object',
+        required: ['required1'],
+        properties: {
+          required1: {
+            type: 'string',
+          },
+          nested1: {
+            type: 'object',
+            required: ['nestedrequired1'],
+            properties: {
+              nestedrequired1: {
+                type: 'string',
+              },
+            },
+          },
+        },
+      },
+      child: {
+        allOf: [
+          {
+            $ref: '#/components/schemas/parent',
+          },
+          {
+            type: 'object',
+            required: ['required2'],
+            properties: {
+              required2: {
+                type: 'string',
+              },
+              nested1: {
+                type: 'object',
+                required: ['nestedrequired2'],
+                properties: {
+                  nestedrequired2: {
+                    type: 'string',
+                  },
+                },
+              },
+            },
+          },
+        ],
+      },
+    },
+  },
+};
+
+test('should resolve test case from UI-7618 correctly', async () => {
+  const res = await resolveSubtree(spec, []);
+
+  expect(res).toEqual({
+    spec: {
+      openapi: '3.0.3',
+      info: {
+        version: '1.0.0',
+        title: 'test',
+        description: 'test',
+        contact: {
+          name: 'test',
+          email: '[email protected]',
+        },
+      },
+      servers: [
+        {
+          url: 'https://localhost/api',
+        },
+      ],
+      tags: [
+        {
+          name: 'Test',
+          description: 'Test',
+        },
+      ],
+      paths: {
+        '/one': {
+          get: {
+            description: 'test',
+            operationId: 'one',
+            tags: ['Test'],
+            parameters: [
+              {
+                name: 'test',
+                in: 'header',
+                required: true,
+                schema: {
+                  type: 'string',
+                },
+              },
+            ],
+            responses: {
+              default: {
+                description: 'Response',
+                content: {
+                  'text/plain': {
+                    schema: {
+                      type: 'integer',
+                      enum: [401, 404, 500],
+                    },
+                  },
+                },
+              },
+            },
+            __originalOperationId: 'one',
+          },
+        },
+      },
+      components: {
+        schemas: {
+          parent: {
+            type: 'object',
+            required: ['required1'],
+            properties: {
+              required1: {
+                type: 'string',
+              },
+              nested1: {
+                type: 'object',
+                required: ['nestedrequired1'],
+                properties: {
+                  nestedrequired1: {
+                    type: 'string',
+                  },
+                },
+              },
+            },
+          },
+          child: {
+            type: 'object',
+            required: ['required1', 'required2'],
+            properties: {
+              required1: {
+                type: 'string',
+              },
+              nested1: {
+                type: 'object',
+                required: ['nestedrequired1', 'nestedrequired2'],
+                properties: {
+                  nestedrequired1: {
+                    type: 'string',
+                  },
+                  nestedrequired2: {
+                    type: 'string',
+                  },
+                },
+              },
+              required2: {
+                type: 'string',
+              },
+            },
+          },
+        },
+      },
+      $$normalized: true,
+    },
+    errors: [],
+  });
+});