fix(dashboard): Add runtime safety checks and improved tests (#22457)

superset-frontend/src/dashboard/reducers/dashboardLayout.js

@@ -45,6 +45,39 @@ import {
 
 import { HYDRATE_DASHBOARD } from '../actions/hydrate';
 
+export function recursivelyDeleteChildren(
+  componentId,
+  componentParentId,
+  nextComponents,
+) {
+  // delete child and it's children
+  const component = nextComponents?.[componentId];
+  if (component) {
+    // eslint-disable-next-line no-param-reassign
+    delete nextComponents[componentId];
+
+    const { children = [] } = component;
+    children?.forEach?.(childId => {
+      recursivelyDeleteChildren(childId, componentId, nextComponents);
+    });
+
+    const parent = nextComponents?.[componentParentId];
+    if (Array.isArray(parent?.children)) {
+      // may have been deleted in another recursion
+      const componentIndex = parent.children.indexOf(componentId);
+      if (componentIndex > -1) {
+        const nextChildren = [...parent.children];
+        nextChildren.splice(componentIndex, 1);
+        // eslint-disable-next-line no-param-reassign
+        nextComponents[componentParentId] = {
+          ...parent,
+          children: nextChildren,
+        };
+      }
+    }
+  }
+}
+
 const actionHandlers = {
   [HYDRATE_DASHBOARD](state, action) {
     return {
@@ -71,39 +104,14 @@ const actionHandlers = {
 
     const nextComponents = { ...state };
 
-    function recursivelyDeleteChildren(componentId, componentParentId) {
-      // delete child and it's children
-      const component = nextComponents[componentId];
-      delete nextComponents[componentId];
-
-      const { children = [] } = component;
-      children.forEach(childId => {
-        recursivelyDeleteChildren(childId, componentId);
-      });
-
-      const parent = nextComponents[componentParentId];
-      if (parent) {
-        // may have been deleted in another recursion
-        const componentIndex = (parent.children || []).indexOf(componentId);
-        if (componentIndex > -1) {
-          const nextChildren = [...parent.children];
-          nextChildren.splice(componentIndex, 1);
-          nextComponents[componentParentId] = {
-            ...parent,
-            children: nextChildren,
-          };
-        }
-      }
-    }
-
-    recursivelyDeleteChildren(id, parentId);
+    recursivelyDeleteChildren(id, parentId, nextComponents);
     const nextParent = nextComponents[parentId];
-    if (nextParent.type === ROW_TYPE && nextParent.children.length === 0) {
+    if (nextParent?.type === ROW_TYPE && nextParent?.children?.length === 0) {
       const grandparentId = findParentId({
         childId: parentId,
         layout: nextComponents,
       });
-      recursivelyDeleteChildren(parentId, grandparentId);
+      recursivelyDeleteChildren(parentId, grandparentId, nextComponents);
     }
 
     return nextComponents;

superset-frontend/src/dashboard/reducers/dashboardLayout.test.js

@@ -16,7 +16,9 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import layoutReducer from 'src/dashboard/reducers/dashboardLayout';
+import layoutReducer, {
+  recursivelyDeleteChildren,
+} from 'src/dashboard/reducers/dashboardLayout';
 
 import {
   UPDATE_COMPONENTS,
@@ -455,4 +457,23 @@ describe('dashboardLayout reducer', () => {
     expect(result[DASHBOARD_GRID_ID].children).toHaveLength(2);
     expect(result[newId].type).toBe(ROW_TYPE);
   });
+
+  it('recursivelyDeleteChildren should be error proof with bad inputs', () => {
+    /*
+     ** The recursivelyDeleteChildren function was missing runtime safety checks before operating
+     ** on sub properties of object causing runtime errors when a componentId lookup returned and unexpected value
+     ** These test are to ensure this function is fault tolerant if provided any bad values while recursively looping
+     ** through the data structure of
+     */
+    const componentId = '123';
+    const componentParentId = '456';
+    const nextComponents = [];
+    expect(() => {
+      recursivelyDeleteChildren(componentId, componentParentId, nextComponents);
+    }).not.toThrow();
+
+    expect(() => {
+      recursivelyDeleteChildren(null, null, null);
+    }).not.toThrow();
+  });
 });

superset-frontend/src/dashboard/util/findParentId.test.js → superset-frontend/src/dashboard/util/findParentId.test.ts

@@ -41,4 +41,15 @@ describe('findParentId', () => {
   it('should return null if the parent cannot be found', () => {
     expect(findParentId({ childId: 'a', layout })).toBeNull();
   });
+
+  it('should not throw error and return null with bad / missing inputs', () => {
+    // @ts-ignore
+    expect(findParentId(null)).toBeNull();
+    // @ts-ignore
+    expect(findParentId({ layout })).toBeNull();
+    // @ts-ignore
+    expect(findParentId({ childId: 'a' })).toBeNull();
+    // @ts-ignore
+    expect(findParentId({ childId: 'a', layout: null })).toBeNull();
+  });
 });

superset-frontend/src/dashboard/util/findParentId.ts

@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+interface ILayoutItem {
+  [key: string]: {
+    id: string;
+    children: string[];
+  };
+}
+
+interface IStructure {
+  childId: string;
+  layout: ILayoutItem;
+}
+
+function findParentId(structure: IStructure): string | null {
+  let parentId = null;
+  if (structure) {
+    const { childId, layout = {} } = structure;
+    // default assignment to layout only works if value is undefined, not null
+    if (layout) {
+      const ids = Object.keys(layout);
+      for (let i = 0; i <= ids.length - 1; i += 1) {
+        const id = ids[i];
+        const component = layout[id] || {};
+        if (id !== childId && component?.children?.includes?.(childId)) {
+          parentId = id;
+          break;
+        }
+      }
+    }
+  }
+  return parentId;
+}
+
+const cache = {};
+export default function findParentIdWithCache(
+  structure: IStructure,
+): string | null {
+  let parentId = null;
+  if (structure) {
+    const { childId, layout = {} } = structure;
+    if (cache[childId]) {
+      const lastParent = layout?.[cache[childId]] || {};
+      if (lastParent?.children && lastParent?.children?.includes?.(childId)) {
+        return lastParent.id;
+      }
+    }
+    parentId = findParentId({ childId, layout });
+    cache[childId] = parentId;
+  }
+  return parentId;
+}