Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -240,4 +240,33 @@ export type DashboardComponentMetadata = {
dataMask: DataMaskStateWithId;
};

export interface LegacyChartCustomizationDataset {
value: number | string;
label: string;
table_name?: string;
}

export interface LegacyChartCustomizationConfig {
name: string;
dataset: string | number | LegacyChartCustomizationDataset | null;
column: string | string[] | null;
sortAscending?: boolean;
sortMetric?: string;
canSelectMultiple?: boolean;
defaultDataMask?: DataMask;
controlValues?: {
enableEmptyFilter?: boolean;
[key: string]: any;
};
description?: string;
}

export interface LegacyChartCustomizationItem {
id: string;
title?: string;
removed?: boolean;
chartId?: number;
customization: LegacyChartCustomizationConfig;
}

export default {};
8 changes: 7 additions & 1 deletion superset-frontend/src/dashboard/actions/hydrate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import { ResourceStatus } from 'src/hooks/apiResources/apiResources';
import type { DashboardChartStates } from 'src/dashboard/types/chartState';
import extractUrlParams from '../util/extractUrlParams';
import updateComponentParentsList from '../util/updateComponentParentsList';
import { migrateChartCustomizationArray } from '../util/migrateChartCustomization';
import {
DashboardLayout,
FilterBarOrientation,
Expand Down Expand Up @@ -291,8 +292,13 @@ export const hydrateDashboard =
directPathToChild.push(directLinkComponentId);
}

const chartCustomizations =
const rawChartCustomizations =
(metadata?.chart_customization_config as JsonObject[]) || [];

const chartCustomizations = migrateChartCustomizationArray(
rawChartCustomizations,
);

const filters =
(metadata?.native_filter_configuration as JsonObject[]) || [];
const combinedFilters = [...filters, ...chartCustomizations];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,10 +279,14 @@ const FilterControls: FC<FilterControlsProps> = ({
/>
);
}
const filterWithDataMask = addDataMaskToCustomization(
item,
dataMaskSelected,
);
return (
<FilterControl
key={item.id}
filter={addDataMaskToCustomization(item, dataMaskSelected)}
filter={filterWithDataMask}
dataMaskSelected={dataMaskSelected}
onFilterSelectionChange={(_, dataMask) =>
handleChartCustomizationChange(item, dataMask)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { ChartsState, RootState } from 'src/dashboard/types';
import {
NATIVE_FILTER_PREFIX,
CHART_CUSTOMIZATION_PREFIX,
LEGACY_GROUPBY_PREFIX,
isNativeFilter,
} from '../FiltersConfigModal/utils';
import { useFilterConfiguration } from '../state';
Expand Down Expand Up @@ -87,7 +88,8 @@ export const useAllAppliedDataMask = () => {
const id = String(item.id);
return (
id.startsWith(NATIVE_FILTER_PREFIX) ||
id.startsWith(CHART_CUSTOMIZATION_PREFIX)
id.startsWith(CHART_CUSTOMIZATION_PREFIX) ||
id.startsWith(LEGACY_GROUPBY_PREFIX)
);
})
.reduce(
Expand All @@ -108,10 +110,10 @@ export const useFilterUpdates = (
const dataMaskApplied = useNativeFiltersDataMask();
useEffect(() => {
Object.keys(dataMaskSelected).forEach(selectedId => {
const isChartCustomization = String(selectedId).startsWith(
CHART_CUSTOMIZATION_PREFIX,
);
if (!isChartCustomization && !filters[selectedId]) {
const isChartCustomizationItem =
String(selectedId).startsWith(CHART_CUSTOMIZATION_PREFIX) ||
String(selectedId).startsWith(LEGACY_GROUPBY_PREFIX);
if (!isChartCustomizationItem && !filters[selectedId]) {
setDataMaskSelected(draft => {
delete draft[selectedId];
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,11 @@ export const createHandleCustomizationSave =
export const CHART_CUSTOMIZATION_PREFIX = 'CHART_CUSTOMIZATION-';
export const CHART_CUSTOMIZATION_DIVIDER_PREFIX =
'CHART_CUSTOMIZATION_DIVIDER-';
export const LEGACY_GROUPBY_PREFIX = 'groupby_';

export const isChartCustomization = (id: string): boolean =>
id.startsWith(CHART_CUSTOMIZATION_PREFIX);
id.startsWith(CHART_CUSTOMIZATION_PREFIX) ||
id.startsWith(LEGACY_GROUPBY_PREFIX);

export const isChartCustomizationDivider = (id: string): boolean =>
id.startsWith(CHART_CUSTOMIZATION_DIVIDER_PREFIX);
Expand All @@ -337,7 +339,8 @@ export const isFilterId = (id: string): boolean =>

export const isChartCustomizationId = (id: string): boolean =>
id.startsWith(CHART_CUSTOMIZATION_PREFIX) ||
id.startsWith(CHART_CUSTOMIZATION_DIVIDER_PREFIX);
id.startsWith(CHART_CUSTOMIZATION_DIVIDER_PREFIX) ||
id.startsWith(LEGACY_GROUPBY_PREFIX);

export const getItemType = (id: string): ItemType => {
if (isFilterId(id)) return 'filter';
Expand Down
43 changes: 33 additions & 10 deletions superset-frontend/src/dashboard/components/nativeFilters/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ import { FilterElement } from './FilterBar/FilterControls/types';
import { ActiveTabs, DashboardLayout, RootState } from '../../types';
import { CHART_TYPE, TAB_TYPE } from '../../util/componentTypes';
import { isChartCustomizationId } from './FiltersConfigModal/utils';
import {
migrateChartCustomizationArray,
isLegacyChartCustomizationFormat,
} from '../../util/migrateChartCustomization';

const EMPTY_ARRAY: ChartCustomizationConfiguration = [];
const defaultFilterConfiguration: (Filter | Divider)[] = [];
Expand Down Expand Up @@ -93,11 +97,19 @@ const selectDashboardChartIds = createSelector(
const selectChartCustomizationConfiguration = createSelector(
[
(state: RootState) =>
state.dashboardInfo.metadata?.chart_customization_config || EMPTY_ARRAY,
state.dashboardInfo?.metadata?.chart_customization_config || EMPTY_ARRAY,
selectDashboardChartIds,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The selector accesses state.dashboardInfo.metadata without checking whether state.dashboardInfo exists; if dashboardInfo is undefined this will throw at runtime. Add optional chaining on dashboardInfo so the selector safely falls back to EMPTY_ARRAY when dashboardInfo is not present. [null pointer]

Severity Level: Critical 🚨
- ❌ Dashboard render crashes on missing dashboardInfo.
- ❌ Chart customizations sidebar fails to mount.
- ⚠️ Unit tests mocking partial store may fail.
Suggested change
selectDashboardChartIds,
state.dashboardInfo?.metadata?.chart_customization_config || EMPTY_ARRAY,
Steps of Reproduction ✅
1. Open the dashboard UI component that uses chart customizations which calls
useChartCustomizationConfiguration (see useChartCustomizationConfiguration at
superset-frontend/src/dashboard/components/nativeFilters/state.ts:127).

2. During initial render the selector selectChartCustomizationConfiguration is executed
(defined at superset-frontend/src/dashboard/components/nativeFilters/state.ts:100-101).

3. If Redux state has no dashboardInfo key (state.dashboardInfo === undefined) the
expression at superset-frontend/src/dashboard/components/nativeFilters/state.ts:101
attempts to read `.metadata` on undefined and throws a TypeError.

4. Reproduce in tests by mocking react-redux useSelector to return a store object without
dashboardInfo and rendering any component that calls useChartCustomizationConfiguration;
the component render will throw at the selector line shown above.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/dashboard/components/nativeFilters/state.ts
**Line:** 101:101
**Comment:**
	*Null Pointer: The selector accesses `state.dashboardInfo.metadata` without checking whether `state.dashboardInfo` exists; if `dashboardInfo` is undefined this will throw at runtime. Add optional chaining on `dashboardInfo` so the selector safely falls back to `EMPTY_ARRAY` when `dashboardInfo` is not present.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

],
(allCustomizations, dashboardChartIds): ChartCustomizationConfiguration =>
allCustomizations.filter(customization => {
(allCustomizations, dashboardChartIds): ChartCustomizationConfiguration => {
const hasLegacyFormat = allCustomizations.some(item =>
isLegacyChartCustomizationFormat(item),
);

const migratedCustomizations = hasLegacyFormat
? migrateChartCustomizationArray(allCustomizations)
: (allCustomizations as ChartCustomizationConfiguration);

return migratedCustomizations.filter(customization => {
if (
!customization.chartsInScope ||
customization.chartsInScope.length === 0
Expand All @@ -108,7 +120,8 @@ const selectChartCustomizationConfiguration = createSelector(
return customization.chartsInScope.some((chartId: number) =>
dashboardChartIds.has(chartId),
);
}),
});
},
);

export function useChartCustomizationConfiguration() {
Expand Down Expand Up @@ -271,10 +284,20 @@ export function useIsCustomizationInScope() {
(customization: ChartCustomization | ChartCustomizationDivider) => {
if ('title' in customization) return true;

const isChartInScope =
const hasChartsInScope =
Array.isArray(customization.chartsInScope) &&
customization.chartsInScope.length > 0 &&
customization.chartsInScope.some((chartId: number) => {
customization.chartsInScope.length > 0;
const hasTabsInScope =
Array.isArray(customization.tabsInScope) &&
customization.tabsInScope.length > 0;

if (!hasChartsInScope && !hasTabsInScope) {
return true;
}

const isChartInScope =
hasChartsInScope &&
customization.chartsInScope!.some((chartId: number) => {
const tabParents = selectChartTabParents(chartId);
return (
!tabParents ||
Expand All @@ -283,9 +306,9 @@ export function useIsCustomizationInScope() {
);
});

const isCustomizationInActiveTab = customization.tabsInScope?.some(tab =>
activeTabs.includes(tab),
);
const isCustomizationInActiveTab =
hasTabsInScope &&
customization.tabsInScope!.some(tab => activeTabs.includes(tab));

return isChartInScope || isCustomizationInActiveTab;
},
Expand Down
Loading
Loading