diff --git a/superset-frontend/src/dashboard/components/nativeFilters/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/utils.ts index cffd0ba606..32a060057b 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/utils.ts +++ b/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, ) => { 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)) ) { diff --git a/superset-frontend/src/dashboard/util/updateComponentParentsList.js b/superset-frontend/src/dashboard/util/updateComponentParentsList.js index 48f01e20a6..44e6c24a19 100644 --- a/superset-frontend/src/dashboard/util/updateComponentParentsList.js +++ b/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]) { - const parentsList = (currentComponent.parents || []).slice(); - parentsList.push(currentComponent.id); + if (currentComponent && layout) { + if (layout[currentComponent.id]) { + const parentsList = Array.isArray(currentComponent.parents) + ? currentComponent.parents.slice() + : []; - currentComponent.children.forEach(childId => { - layout[childId].parents = parentsList; // eslint-disable-line no-param-reassign - updateComponentParentsList({ - currentComponent: layout[childId], - layout, - }); - }); + parentsList.push(currentComponent.id); + + if (Array.isArray(currentComponent.children)) { + currentComponent.children.forEach(childId => { + 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`, + ); + } } } diff --git a/superset-frontend/src/dashboard/util/updateComponentParentsList.test.js b/superset-frontend/src/dashboard/util/updateComponentParentsList.test.js index c8f66b1934..4a788ad35d 100644 --- a/superset-frontend/src/dashboard/util/updateComponentParentsList.test.js +++ b/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(); + }); +});