fix(dashboard): dashboard actions fail when bad component id exists in children array (#22323)

superset-frontend/src/dashboard/components/nativeFilters/utils.ts

@@ -146,7 +146,7 @@ export function nativeFilterGate(behaviors: Behavior[]): boolean {
 const isComponentATab = (
   dashboardLayout: DashboardLayout,
   componentId: string,
-) => dashboardLayout[componentId]?.type === TAB_TYPE;
+) => dashboardLayout?.[componentId]?.type === TAB_TYPE;
 
 const findTabsWithChartsInScopeHelper = (
   dashboardLayout: DashboardLayout,
@@ -156,13 +156,13 @@ const findTabsWithChartsInScopeHelper = (
   tabsToHighlight: Set<string>,
 ) => {
   if (
-    dashboardLayout[componentId]?.type === CHART_TYPE &&
+    dashboardLayout?.[componentId]?.type === CHART_TYPE &&
     chartsInScope.includes(dashboardLayout[componentId]?.meta?.chartId)
   ) {
     tabIds.forEach(tabsToHighlight.add, tabsToHighlight);
   }
   if (
-    dashboardLayout[componentId]?.children?.length === 0 ||
+    dashboardLayout?.[componentId]?.children?.length === 0 ||
     (isComponentATab(dashboardLayout, componentId) &&
       tabsToHighlight.has(componentId))
   ) {

superset-frontend/src/dashboard/util/updateComponentParentsList.js

@@ -1,3 +1,5 @@
+import { logging } from '@superset-ui/core';
+
 /**
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
@@ -20,16 +22,34 @@ export default function updateComponentParentsList({
   currentComponent,
   layout = {},
 }) {
-  if (currentComponent && layout[currentComponent.id]) {
+  if (currentComponent && layout) {
-    const parentsList = (currentComponent.parents || []).slice();
+    if (layout[currentComponent.id]) {
-    parentsList.push(currentComponent.id);
+      const parentsList = Array.isArray(currentComponent.parents)
+        ? currentComponent.parents.slice()
+        : [];
 
-    currentComponent.children.forEach(childId => {
+      parentsList.push(currentComponent.id);
-      layout[childId].parents = parentsList; // eslint-disable-line no-param-reassign
+
-      updateComponentParentsList({
+      if (Array.isArray(currentComponent.children)) {
-        currentComponent: layout[childId],
+        currentComponent.children.forEach(childId => {
-        layout,
+          const child = layout[childId];
-      });
+          if (child) {
-    });
+            child.parents = parentsList; // eslint-disable-line no-param-reassign
+            updateComponentParentsList({
+              currentComponent: child,
+              layout,
+            });
+          } else {
+            logging.warn(
+              `The current layout does not contain a component with the id: ${childId}.  Skipping this component`,
+            );
+          }
+        });
+      }
+    } else {
+      logging.warn(
+        `The current layout does not contain a component with the id: ${currentComponent?.id}.  Skipping this component`,
+      );
+    }
   }
 }

superset-frontend/src/dashboard/util/updateComponentParentsList.test.js

@@ -95,3 +95,68 @@ describe('updateComponentParentsList', () => {
     ]);
   });
 });
+
+describe('updateComponentParentsList with bad inputs', () => {
+  it('should handle invalid parameters and not throw error', () => {
+    updateComponentParentsList({
+      currentComponent: undefined,
+      layout: undefined,
+    });
+
+    expect(() =>
+      updateComponentParentsList({
+        currentComponent: undefined,
+        layout: undefined,
+      }),
+    ).not.toThrow();
+
+    expect(() =>
+      updateComponentParentsList({
+        currentComponent: {},
+        layout: undefined,
+      }),
+    ).not.toThrow();
+
+    /**
+     * the assignment of layout = {} only works for undefined, not null
+     * This was a missed case in the function previously.
+     * This test ensure the null check is not removed
+     */
+    expect(() =>
+      updateComponentParentsList({
+        currentComponent: {},
+        layout: null,
+      }),
+    ).not.toThrow();
+
+    /**
+     * This test catches an edge case that caused runtime error in production system where
+     * a simple logic flaw performed a dot notation lookup on an undefined object
+     */
+    expect(() =>
+      updateComponentParentsList({
+        currentComponent: { id: 'id3', children: ['id1', 'id2'] },
+        layout: { id3: {} },
+      }),
+    ).not.toThrow();
+
+    /**
+     * This test catches an edge case that causes runtime error where
+     * a simple logic flaw performed currentComponent.children.forEach without
+     * verifying currentComponent.children is an Array with a .forEach function defined
+     */
+    expect(() =>
+      updateComponentParentsList({
+        currentComponent: { id: 'id3' },
+        layout: { id3: {} },
+      }),
+    ).not.toThrow();
+
+    expect(() =>
+      updateComponentParentsList({
+        currentComponent: { id: 'id3' },
+        layout: {},
+      }),
+    ).not.toThrow();
+  });
+});