sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4148280148
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -383,8 +553,63 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ // Skip persistence for unauthenticated/guest users: they have no stable
+ // identity to scope the key to, and restoring filter state across
+ // guest-token sessions would leak selections between unrelated sessions.
+ // Use == null (not !userId) so a valid userId of 0 is not treated as
+ // anonymous.
+ // Also skip when a historical version preview is active: the dashboard
+ // rehydrates with the snapshot's filter defaults, and every other guard
+ // would pass, causing those defaults to overwrite the user's live mask.
+ if (
+ !id ||
+ userId == null ||
+ isVersionPreviewActive ||
+ hydratedDashboardId !== id ||
+ !isDashboardHydrated.current
+ )
+ return;
+
+ if (isRestoringUrlFilters.current) {
Review Comment:
This guard only blocks the very next save-effect run after a URL-driven
hydration (permalink, native_filters_key, or legacy Rison `?f=`), then clears
itself. Any later, unrelated re-render that changes `nativeFilters` (for
example the dashboard-builder container's scope-status dispatch on mount, or
the filter bar's auto-apply/default-selection dispatch) re-runs this effect
with the ref already false, so it persists the URL-derived mask under the
user's own storage key, overwriting whatever selection they actually had saved
for this dashboard. Anyone who opens a shared permalink/`?f=` link once and
later returns normally gets the link's filters instead of their own. Could the
guard track whether the live native-filter mask has actually diverged from the
URL-derived one it just hydrated, rather than expiring after exactly one effect
run?
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +630,543 @@ test('clears undo history after hydrating the dashboard',
async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is
present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code
rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format:
dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned
validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('skips localStorage restore for guest/embedded users (userId is
undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__1');
+});
+
+test('scopes localStorage key to userId when user is authenticated', async ()
=> {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['2024'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'year' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // key is scoped to userId=7 and dashboardId=1
+ localStorage.setItem(
+ 'dashboard__native_filters__7__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID in native_filter_configuration so versioned
validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'year' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 7 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-xyz': expect.objectContaining({
+ filterState: { value: ['2024'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__7__1');
+});
+
+test('does not restore localStorage filters when a nativeFiltersKey is in the
URL', async () => {
+ // Put something in localStorage that should be ignored because the URL key
takes priority
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify({ 'NATIVE_FILTER-abc': { filterState: { value: ['X'] } } }),
+ );
+
+ const { getFilterValue } = jest.requireMock(
+ 'src/dashboard/components/nativeFilters/FilterBar/keyValue',
+ );
+ (getFilterValue as jest.Mock).mockResolvedValueOnce({
+ 'NATIVE_FILTER-abc': { filterState: { value: ['FromURL'] } },
+ });
+
+ mockGetUrlParam.mockImplementation((param: { name: string }) => {
+ if (param.name === 'native_filters_key') return 'some-key';
+ return null;
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // hydrateDashboard should use the URL-resolved value, not localStorage
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc': expect.objectContaining({
+ filterState: { value: ['FromURL'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__1');
+});
+
+test('ignores corrupted localStorage data (array) and uses empty dataMask',
async () => {
+ // An array is not a valid dataMask shape and must be rejected
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify([1, 2, 3]),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — the corrupted array value must not be used
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('restores versioned localStorage filters and drops them if targets
change', async () => {
+ const savedVersionedData = {
+ dataMask: {
+ 'NATIVE_FILTER-versioned': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-versioned': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+
+ // Use an authenticated user key — restoration skips unauthenticated users.
+ localStorage.setItem(
+ 'dashboard__native_filters__5__1',
+ JSON.stringify(savedVersionedData),
+ );
+
+ // 1. Simulate the dashboard where the target matches
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ const { render } = jest.requireActual('spec/helpers/testing-library');
+ const { unmount } = render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should be called with the restored value since target
matches
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ unmount();
+ (hydrateDashboard as jest.Mock).mockClear();
+
+ // 2. Simulate the dashboard where the target has changed
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'country' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should NOT have the dropped filter since target mismatch
+ expect(hydrateDashboard).not.toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__5__1');
+});
+
+test('skips localStorage restore when ?f= Rison link is present in URL', async
() => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['California'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ // Mock getUrlParam to simulate ?f= presence
+ jest.spyOn(require('src/dashboard/util/risonFilters'),
'getRisonFilterParam').mockReturnValue('{}');
+
+ render(
+ <React.Suspense fallback="loading">
Review Comment:
Both new tests here (and the matching block further down) render with
`<React.Suspense>`, but this file only imports the named `Suspense` value and
the `ReactNode` type — there's no default `React` import, so `React` is
undefined at runtime and these two tests throw before reaching their
assertions. That matches the current lint-frontend/pre-commit failures on this
PR. Swap both occurrences for the already-imported `<Suspense>`.
Separately, this file's other tests each clean up their own localStorage key
with a trailing `removeItem` call instead of a shared teardown, so a thrown
assertion in one test can leave its key behind and affect an unrelated later
test (one of these two new tests asserts that same key is absent). Worth a
shared `afterEach(() => localStorage.clear())`?
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -375,6 +375,14 @@ const RightMenu = ({
try {
window.localStorage.removeItem('redux');
window.sessionStorage.removeItem('login_attempted');
+ // Clear dashboard native filters persistence to prevent leaking saved
+ // filter selections (which may contain business-sensitive data) to the
+ // next user logging in on the same browser profile.
+ Object.keys(window.localStorage).forEach(key => {
Review Comment:
This cleanup only runs through the menu item's onClick handler, but the same
element renders a real `<a href>` to the logout URL. Opening that link through
a browser context-menu action (for example "open link in new tab") or
navigating to it directly skips onClick entirely, so these dashboard-filter
keys are never cleared and the next person to use this browser profile can
still read the previous user's saved filter selections from localStorage. Could
this cleanup instead run from an entry point that fires regardless of how the
logout navigation is triggered?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -228,6 +313,93 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else if (getRisonFilterParam()) {
+ // A Rison ?f= filter in the URL encodes an explicit filter intent
+ // (e.g. from a shared link). Loading localStorage state on top would
+ // silently inject the viewer's saved selections (e.g. region=EMEA)
+ // and return narrower data than the URL encodes. Skip the fallback so
+ // the Rison filters below are applied to a clean dataMask.
+ } else if (userId != null) {
+ // Skip localStorage restore for unauthenticated/guest users: they have
+ // no stable identity and reading localStorage here would share filter
+ // state across different guest-token sessions. Use != null (not
!!userId)
+ // so a valid userId of 0 is not treated as anonymous.
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ // Reject unversioned entries: the storage key has not shipped, so
+ // there are no legitimate unversioned values in the wild. Falling
+ // back to an ID-only check on an unversioned entry would restore
+ // stale extraFormData for any retargeted filter, so we discard it
+ // outright and let the dashboard open with its configured defaults.
+ const isVersioned =
+ 'dataMask' in savedFilters && 'filterDefinitions' in savedFilters;
+ if (!isVersioned) {
+ // Nothing to restore — proceed with URL/default state
+ } else {
+ const maskToRestore = savedFilters.dataMask;
+ const savedDefinitions = savedFilters.filterDefinitions;
+
+ // Validate that the nested dataMask itself is a non-null plain
+ // object before calling Object.entries. A versioned entry whose
+ // dataMask field is null (e.g. corrupted storage) would otherwise
+ // throw here and prevent the dashboard from hydrating.
+ if (
+ maskToRestore != null &&
+ typeof maskToRestore === 'object' &&
+ !Array.isArray(maskToRestore)
+ ) {
+ // Filter out any null/falsey legacy entries in
+ // native_filter_configuration before looking up IDs, to avoid
+ // dereferencing null when a dashboard has legacy null config
rows.
+ const currentFilters = (
+ (dashboard?.metadata?.native_filter_configuration ??
+ []) as Array<NativeFilterConfigEntry | null | undefined>
+ ).filter(
+ (f): f is NativeFilterConfigEntry => f != null && !!f.id,
+ );
+
+ // Only restore entries whose filter ID still exists in the
current
+ // native filter configuration. Because this is a versioned
entry,
+ // we also verify that the filter's target columns/datasets and
type
+ // have not changed. This prevents stale extraFormData from a
+ // retargeted filter from being hydrated.
+ const validatedFilters = Object.fromEntries(
+ Object.entries(maskToRestore).filter(([filterId]) => {
+ const currentConfig = currentFilters.find(
+ f => f.id === filterId,
+ );
+ if (!currentConfig) return false;
+
+ const savedDef = savedDefinitions?.[filterId];
+ if (!savedDef) return false;
+
+ // Validate that the target column(s) and filter type have
not changed
+ const currentTargets = JSON.stringify(currentConfig.targets);
+ const savedTargets = JSON.stringify(savedDef.targets);
+
+ if (
Review Comment:
This check verifies the filter's `targets` and `filterType` haven't changed,
but not other settings that determine the saved `extraFormData`. If an editor
changes a filter's comparator (for example from an exact match to a "contains"
match) without changing its target or type, a returning user's restored
selection still applies the old comparator's `extraFormData`, producing
incorrect query results until they reapply the filter. Should the comparison
also cover filter-type-specific settings such as the comparator, not only
target and filterType?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
