You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2023/01/06 22:45:57 UTC
[superset] branch master updated: fix(dashboard): Add runtime safety checks and improved tests (#22457)
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 fad873c100 fix(dashboard): Add runtime safety checks and improved tests (#22457)
fad873c100 is described below
commit fad873c100cb35912308a5c700a2d49831506e3a
Author: Eric Briscoe <er...@gmail.com>
AuthorDate: Fri Jan 6 14:45:49 2023 -0800
fix(dashboard): Add runtime safety checks and improved tests (#22457)
---
.../src/dashboard/reducers/dashboardLayout.js | 64 +++++++++++---------
.../src/dashboard/reducers/dashboardLayout.test.js | 23 +++++++-
.../src/dashboard/util/findParentId.js | 49 ----------------
.../{findParentId.test.js => findParentId.test.ts} | 11 ++++
.../src/dashboard/util/findParentId.ts | 68 ++++++++++++++++++++++
5 files changed, 137 insertions(+), 78 deletions(-)
diff --git a/superset-frontend/src/dashboard/reducers/dashboardLayout.js b/superset-frontend/src/dashboard/reducers/dashboardLayout.js
index 30ad33c62b..6ba297be91 100644
--- a/superset-frontend/src/dashboard/reducers/dashboardLayout.js
+++ b/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;
diff --git a/superset-frontend/src/dashboard/reducers/dashboardLayout.test.js b/superset-frontend/src/dashboard/reducers/dashboardLayout.test.js
index f9b504acf7..f0f4d8b1bc 100644
--- a/superset-frontend/src/dashboard/reducers/dashboardLayout.test.js
+++ b/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();
+ });
});
diff --git a/superset-frontend/src/dashboard/util/findParentId.js b/superset-frontend/src/dashboard/util/findParentId.js
deleted file mode 100644
index 4d24bb761d..0000000000
--- a/superset-frontend/src/dashboard/util/findParentId.js
+++ /dev/null
@@ -1,49 +0,0 @@
-/**
- * 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.
- */
-function findParentId({ childId, layout = {} }) {
- let parentId = null;
-
- 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 &&
- component.children.includes(childId)
- ) {
- parentId = id;
- break;
- }
- }
-
- return parentId;
-}
-
-const cache = {};
-export default function findParentIdWithCache({ childId, layout = {} }) {
- if (cache[childId]) {
- const lastParent = layout[cache[childId]] || {};
- if (lastParent.children && lastParent.children.includes(childId)) {
- return lastParent.id;
- }
- }
- cache[childId] = findParentId({ childId, layout });
- return cache[childId];
-}
diff --git a/superset-frontend/src/dashboard/util/findParentId.test.js b/superset-frontend/src/dashboard/util/findParentId.test.ts
similarity index 79%
rename from superset-frontend/src/dashboard/util/findParentId.test.js
rename to superset-frontend/src/dashboard/util/findParentId.test.ts
index d63be40107..b5fe36c59d 100644
--- a/superset-frontend/src/dashboard/util/findParentId.test.js
+++ b/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();
+ });
});
diff --git a/superset-frontend/src/dashboard/util/findParentId.ts b/superset-frontend/src/dashboard/util/findParentId.ts
new file mode 100644
index 0000000000..d8dbb4dbf2
--- /dev/null
+++ b/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;
+}