This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 92bc641067 fix(dashboard): dashboard actions fail when bad component
id exists in children array (#22323)
92bc641067 is described below
commit 92bc6410671d3512e66303e80ce43a77a687adab
Author: Eric Briscoe <[email protected]>
AuthorDate: Sun Dec 4 19:35:30 2022 -0800
fix(dashboard): dashboard actions fail when bad component id exists in
children array (#22323)
---
.../dashboard/components/nativeFilters/utils.ts | 6 +-
.../dashboard/util/updateComponentParentsList.js | 40 +++++++++----
.../util/updateComponentParentsList.test.js | 65 ++++++++++++++++++++++
3 files changed, 98 insertions(+), 13 deletions(-)
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<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))
) {
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()
+ : [];
+
+ parentsList.push(currentComponent.id);
- currentComponent.children.forEach(childId => {
- layout[childId].parents = parentsList; // eslint-disable-line
no-param-reassign
- updateComponentParentsList({
- currentComponent: layout[childId],
- layout,
- });
- });
+ 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();
+ });
+});
