From 48a80950de06dee1d0af5b11b33401108395f04d Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Mon, 14 Feb 2022 17:09:06 -0300 Subject: [PATCH] feat: Improve state key generation for dashboards and charts (#18576) * feat: Improve state key generation for dashboards and charts --- .../integration/dashboard/key_value.test.ts | 4 +- .../dashboard/nativeFilters.test.ts | 2 +- superset-frontend/package-lock.json | 132 ++++++++++++++--- superset-frontend/package.json | 1 + superset-frontend/spec/helpers/shim.ts | 7 + .../components/gridComponents/Chart.jsx | 5 + .../components/menu/ShareMenuItems/index.tsx | 5 + .../nativeFilters/FilterBar/index.tsx | 92 +++++++----- .../nativeFilters/FilterBar/keyValue.tsx | 39 +++-- .../components/ExploreViewContainer.test.jsx | 92 ------------ .../components/ExploreViewContainer/index.jsx | 19 ++- .../src/explore/exploreUtils/formData.ts | 17 ++- superset-frontend/src/hooks/useTabId.ts | 73 ++++++++++ superset/dashboards/filter_state/api.py | 18 ++- .../filter_state/commands/create.py | 17 ++- .../filter_state/commands/delete.py | 5 + .../filter_state/commands/update.py | 30 +++- superset/explore/form_data/api.py | 19 ++- superset/explore/form_data/commands/create.py | 10 +- superset/explore/form_data/commands/delete.py | 11 +- .../explore/form_data/commands/parameters.py | 1 + superset/explore/form_data/commands/update.py | 20 ++- superset/key_value/api.py | 23 ++- superset/key_value/commands/create.py | 8 +- superset/key_value/commands/parameters.py | 4 +- superset/key_value/commands/update.py | 5 +- superset/key_value/utils.py | 5 + .../dashboards/filter_state/api_tests.py | 128 +++++++++++++++-- .../explore/form_data/api_tests.py | 136 ++++++++++++++++++ 29 files changed, 694 insertions(+), 234 deletions(-) delete mode 100644 superset-frontend/src/explore/components/ExploreViewContainer.test.jsx create mode 100644 superset-frontend/src/hooks/useTabId.ts diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/key_value.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/key_value.test.ts index 6bd34373f2acf..ba27bf30163a2 100644 --- a/superset-frontend/cypress-base/cypress/integration/dashboard/key_value.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/dashboard/key_value.test.ts @@ -33,9 +33,7 @@ describe('nativefiler url param key', () => { cy.login(); cy.visit(WORLD_HEALTH_DASHBOARD); WORLD_HEALTH_CHARTS.forEach(waitForChartLoad); - }); - beforeEach(() => { - cy.login(); + cy.wait(1000); // wait for key to be published (debounced) }); let initialFilterKey: string; it('should have cachekey in nativefilter param', () => { diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts index 7a9cfe092305a..74785d05df5bf 100644 --- a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts @@ -151,7 +151,7 @@ describe('Nativefilters Sanity test', () => { cy.location().then(loc => { const queryParams = qs.parse(removeFirstChar(loc.search)); const newfilterKey = queryParams.native_filters_key; - expect(newfilterKey).not.eq(filterKey); + expect(newfilterKey).eq(filterKey); }); cy.wait(3000); cy.get(nativeFilters.modal.container).should('not.exist'); diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index a3dc019f6acff..f9f0d0c2306a1 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -57,6 +57,7 @@ "bootstrap": "^3.4.1", "bootstrap-slider": "^10.0.0", "brace": "^0.11.1", + "broadcast-channel": "^4.10.0", "chrono-node": "^2.2.6", "classnames": "^2.2.5", "core-js": "^3.6.5", @@ -2418,9 +2419,9 @@ } }, "node_modules/@babel/runtime": { - "version": "7.15.4", - "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.15.4.tgz", - "integrity": "sha512-99catp6bHCaxr4sJ/DbTGgHS4+Rs2RVd2g7iOap6SLGPDknRK9ztKNsE/Fg6QhSeh1FGE5f6gHGQmvvn3I3xhw==", + "version": "7.17.2", + "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.17.2.tgz", + "integrity": "sha512-hzeyJyMA1YGdJTuWU0e/j4wKXrU4OMFvY2MSlaI9B7VQb0r5cxTE3EAIS2Q7Tn2RIcDkRvTA/v2JsAEhxe99uw==", "dependencies": { "regenerator-runtime": "^0.13.4" }, @@ -25690,6 +25691,14 @@ "node": ">8.0.0" } }, + "node_modules/big-integer": { + "version": "1.6.51", + "resolved": "https://registry.npmjs.org/big-integer/-/big-integer-1.6.51.tgz", + "integrity": "sha512-GPEid2Y9QU1Exl1rpO9B2IPJGHPSupF5GnVIP0blYvNOMer2bTvSWs1jGOUg04hTmu67nmLsQ9TBo1puaotBHg==", + "engines": { + "node": ">=0.6" + } + }, "node_modules/big.js": { "version": "5.2.2", "resolved": "https://registry.npmjs.org/big.js/-/big.js-5.2.2.tgz", @@ -26048,6 +26057,21 @@ "brfs": "bin/cmd.js" } }, + "node_modules/broadcast-channel": { + "version": "4.10.0", + "resolved": "https://registry.npmjs.org/broadcast-channel/-/broadcast-channel-4.10.0.tgz", + "integrity": "sha512-hOUh312XyHk6JTVyX9cyXaH1UYs+2gHVtnW16oQAu9FL7ALcXGXc/YoJWqlkV8vUn14URQPMmRi4A9q4UrwVEQ==", + "dependencies": { + "@babel/runtime": "^7.16.0", + "detect-node": "^2.1.0", + "microseconds": "0.2.0", + "nano-time": "1.0.0", + "oblivious-set": "1.0.0", + "p-queue": "6.6.2", + "rimraf": "3.0.2", + "unload": "2.3.1" + } + }, "node_modules/brorand": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/brorand/-/brorand-1.1.0.tgz", @@ -31271,10 +31295,9 @@ } }, "node_modules/detect-node": { - "version": "2.0.4", - "resolved": "https://registry.npmjs.org/detect-node/-/detect-node-2.0.4.tgz", - "integrity": "sha512-ZIzRpLJrOj7jjP2miAtgqIfmzbxa4ZOr5jJc601zklsfEx9oTzmmj2nVpIPRpNlRTIh8lc1kyViIY7BWSGNmKw==", - "devOptional": true + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/detect-node/-/detect-node-2.1.0.tgz", + "integrity": "sha512-T0NIuQpnTvFDATNuHN5roPwSBG83rFsuO+MXXH9/3N1eFbn4wcPjttvjMLEPWJ0RGUYgQE7cGgS3tNxbqCGM7g==" }, "node_modules/detect-port": { "version": "1.3.0", @@ -33557,8 +33580,7 @@ "node_modules/eventemitter3": { "version": "4.0.7", "resolved": "https://registry.npmjs.org/eventemitter3/-/eventemitter3-4.0.7.tgz", - "integrity": "sha512-8guHBZCwKnFhYdHr2ysuRWErTwhoN2X8XELRlrRwpmfeY2jjuUN4taQMsULKUVo1K4DvZl+0pgfyoysHxvmvEw==", - "devOptional": true + "integrity": "sha512-8guHBZCwKnFhYdHr2ysuRWErTwhoN2X8XELRlrRwpmfeY2jjuUN4taQMsULKUVo1K4DvZl+0pgfyoysHxvmvEw==" }, "node_modules/events": { "version": "3.2.0", @@ -43659,6 +43681,11 @@ "node": ">=0.10.0" } }, + "node_modules/microseconds": { + "version": "0.2.0", + "resolved": "https://registry.npmjs.org/microseconds/-/microseconds-0.2.0.tgz", + "integrity": "sha512-n7DHHMjR1avBbSpsTBj6fmMGh2AGrifVV4e+WYc3Q9lO+xnSZ3NyhcBND3vzzatt05LFhoKFRxrIyklmLlUtyA==" + }, "node_modules/miller-rabin": { "version": "4.0.1", "resolved": "https://registry.npmjs.org/miller-rabin/-/miller-rabin-4.0.1.tgz", @@ -44271,6 +44298,14 @@ "integrity": "sha512-INOFj37C7k3AfaNTtX8RhsTw7qRy7eLET14cROi9+5HAVbbHuIWUHEauBv5qT4Av2tWasiTY1Jw6puUNqRJXQg==", "optional": true }, + "node_modules/nano-time": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/nano-time/-/nano-time-1.0.0.tgz", + "integrity": "sha1-sFVPaa2J4i0JB/ehKwmTpdlhN+8=", + "dependencies": { + "big-integer": "^1.6.16" + } + }, "node_modules/nanocolors": { "version": "0.1.12", "resolved": "https://registry.npmjs.org/nanocolors/-/nanocolors-0.1.12.tgz", @@ -45453,6 +45488,11 @@ "resolved": "https://registry.npmjs.org/objectorarray/-/objectorarray-1.0.5.tgz", "integrity": "sha512-eJJDYkhJFFbBBAxeh8xW+weHlkI28n2ZdQV/J/DNfWfSKlGEf2xcfAbZTv3riEXHAhL9SVOTs2pRmXiSTf78xg==" }, + "node_modules/oblivious-set": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/oblivious-set/-/oblivious-set-1.0.0.tgz", + "integrity": "sha512-z+pI07qxo4c2CulUHCDf9lcqDlMSo72N/4rLUpRXf6fu+q8vjt8y0xS+Tlf8NTJDdTXHbdeO1n3MlbctwEoXZw==" + }, "node_modules/obuf": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/obuf/-/obuf-1.1.2.tgz", @@ -45821,7 +45861,6 @@ "version": "6.6.2", "resolved": "https://registry.npmjs.org/p-queue/-/p-queue-6.6.2.tgz", "integrity": "sha512-RwFpb72c/BhQLEXIZ5K2e+AhgNVmIejGlTgiB9MzZ0e93GRvqZ7uSi0dvRF7/XIXDeNkra2fNHBxTyPDGySpjQ==", - "dev": true, "dependencies": { "eventemitter3": "^4.0.4", "p-timeout": "^3.2.0" @@ -54467,6 +54506,15 @@ "node": ">= 10.0.0" } }, + "node_modules/unload": { + "version": "2.3.1", + "resolved": "https://registry.npmjs.org/unload/-/unload-2.3.1.tgz", + "integrity": "sha512-MUZEiDqvAN9AIDRbbBnVYVvfcR6DrjCqeU2YQMmliFZl9uaBUjTkhuDQkBiyAy8ad5bx1TXVbqZ3gg7namsWjA==", + "dependencies": { + "@babel/runtime": "^7.6.2", + "detect-node": "2.1.0" + } + }, "node_modules/unpipe": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/unpipe/-/unpipe-1.0.0.tgz", @@ -61413,9 +61461,9 @@ } }, "@babel/runtime": { - "version": "7.15.4", - "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.15.4.tgz", - "integrity": "sha512-99catp6bHCaxr4sJ/DbTGgHS4+Rs2RVd2g7iOap6SLGPDknRK9ztKNsE/Fg6QhSeh1FGE5f6gHGQmvvn3I3xhw==", + "version": "7.17.2", + "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.17.2.tgz", + "integrity": "sha512-hzeyJyMA1YGdJTuWU0e/j4wKXrU4OMFvY2MSlaI9B7VQb0r5cxTE3EAIS2Q7Tn2RIcDkRvTA/v2JsAEhxe99uw==", "requires": { "regenerator-runtime": "^0.13.4" } @@ -80194,6 +80242,11 @@ "open": "^7.0.3" } }, + "big-integer": { + "version": "1.6.51", + "resolved": "https://registry.npmjs.org/big-integer/-/big-integer-1.6.51.tgz", + "integrity": "sha512-GPEid2Y9QU1Exl1rpO9B2IPJGHPSupF5GnVIP0blYvNOMer2bTvSWs1jGOUg04hTmu67nmLsQ9TBo1puaotBHg==" + }, "big.js": { "version": "5.2.2", "resolved": "https://registry.npmjs.org/big.js/-/big.js-5.2.2.tgz", @@ -80478,6 +80531,21 @@ "through2": "^2.0.0" } }, + "broadcast-channel": { + "version": "4.10.0", + "resolved": "https://registry.npmjs.org/broadcast-channel/-/broadcast-channel-4.10.0.tgz", + "integrity": "sha512-hOUh312XyHk6JTVyX9cyXaH1UYs+2gHVtnW16oQAu9FL7ALcXGXc/YoJWqlkV8vUn14URQPMmRi4A9q4UrwVEQ==", + "requires": { + "@babel/runtime": "^7.16.0", + "detect-node": "^2.1.0", + "microseconds": "0.2.0", + "nano-time": "1.0.0", + "oblivious-set": "1.0.0", + "p-queue": "6.6.2", + "rimraf": "3.0.2", + "unload": "2.3.1" + } + }, "brorand": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/brorand/-/brorand-1.1.0.tgz", @@ -84502,10 +84570,9 @@ "dev": true }, "detect-node": { - "version": "2.0.4", - "resolved": "https://registry.npmjs.org/detect-node/-/detect-node-2.0.4.tgz", - "integrity": "sha512-ZIzRpLJrOj7jjP2miAtgqIfmzbxa4ZOr5jJc601zklsfEx9oTzmmj2nVpIPRpNlRTIh8lc1kyViIY7BWSGNmKw==", - "devOptional": true + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/detect-node/-/detect-node-2.1.0.tgz", + "integrity": "sha512-T0NIuQpnTvFDATNuHN5roPwSBG83rFsuO+MXXH9/3N1eFbn4wcPjttvjMLEPWJ0RGUYgQE7cGgS3tNxbqCGM7g==" }, "detect-port": { "version": "1.3.0", @@ -86294,8 +86361,7 @@ "eventemitter3": { "version": "4.0.7", "resolved": "https://registry.npmjs.org/eventemitter3/-/eventemitter3-4.0.7.tgz", - "integrity": "sha512-8guHBZCwKnFhYdHr2ysuRWErTwhoN2X8XELRlrRwpmfeY2jjuUN4taQMsULKUVo1K4DvZl+0pgfyoysHxvmvEw==", - "devOptional": true + "integrity": "sha512-8guHBZCwKnFhYdHr2ysuRWErTwhoN2X8XELRlrRwpmfeY2jjuUN4taQMsULKUVo1K4DvZl+0pgfyoysHxvmvEw==" }, "events": { "version": "3.2.0", @@ -94128,6 +94194,11 @@ } } }, + "microseconds": { + "version": "0.2.0", + "resolved": "https://registry.npmjs.org/microseconds/-/microseconds-0.2.0.tgz", + "integrity": "sha512-n7DHHMjR1avBbSpsTBj6fmMGh2AGrifVV4e+WYc3Q9lO+xnSZ3NyhcBND3vzzatt05LFhoKFRxrIyklmLlUtyA==" + }, "miller-rabin": { "version": "4.0.1", "resolved": "https://registry.npmjs.org/miller-rabin/-/miller-rabin-4.0.1.tgz", @@ -94610,6 +94681,14 @@ "integrity": "sha512-INOFj37C7k3AfaNTtX8RhsTw7qRy7eLET14cROi9+5HAVbbHuIWUHEauBv5qT4Av2tWasiTY1Jw6puUNqRJXQg==", "optional": true }, + "nano-time": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/nano-time/-/nano-time-1.0.0.tgz", + "integrity": "sha1-sFVPaa2J4i0JB/ehKwmTpdlhN+8=", + "requires": { + "big-integer": "^1.6.16" + } + }, "nanocolors": { "version": "0.1.12", "resolved": "https://registry.npmjs.org/nanocolors/-/nanocolors-0.1.12.tgz", @@ -95565,6 +95644,11 @@ "resolved": "https://registry.npmjs.org/objectorarray/-/objectorarray-1.0.5.tgz", "integrity": "sha512-eJJDYkhJFFbBBAxeh8xW+weHlkI28n2ZdQV/J/DNfWfSKlGEf2xcfAbZTv3riEXHAhL9SVOTs2pRmXiSTf78xg==" }, + "oblivious-set": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/oblivious-set/-/oblivious-set-1.0.0.tgz", + "integrity": "sha512-z+pI07qxo4c2CulUHCDf9lcqDlMSo72N/4rLUpRXf6fu+q8vjt8y0xS+Tlf8NTJDdTXHbdeO1n3MlbctwEoXZw==" + }, "obuf": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/obuf/-/obuf-1.1.2.tgz", @@ -95841,7 +95925,6 @@ "version": "6.6.2", "resolved": "https://registry.npmjs.org/p-queue/-/p-queue-6.6.2.tgz", "integrity": "sha512-RwFpb72c/BhQLEXIZ5K2e+AhgNVmIejGlTgiB9MzZ0e93GRvqZ7uSi0dvRF7/XIXDeNkra2fNHBxTyPDGySpjQ==", - "dev": true, "requires": { "eventemitter3": "^4.0.4", "p-timeout": "^3.2.0" @@ -102595,6 +102678,15 @@ "resolved": "https://registry.npmjs.org/universalify/-/universalify-2.0.0.tgz", "integrity": "sha512-hAZsKq7Yy11Zu1DE0OzWjw7nnLZmJZYTDZZyEFHZdUhV8FkH5MCfoU1XMaxXovpyW5nq5scPqq0ZDP9Zyl04oQ==" }, + "unload": { + "version": "2.3.1", + "resolved": "https://registry.npmjs.org/unload/-/unload-2.3.1.tgz", + "integrity": "sha512-MUZEiDqvAN9AIDRbbBnVYVvfcR6DrjCqeU2YQMmliFZl9uaBUjTkhuDQkBiyAy8ad5bx1TXVbqZ3gg7namsWjA==", + "requires": { + "@babel/runtime": "^7.6.2", + "detect-node": "2.1.0" + } + }, "unpipe": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/unpipe/-/unpipe-1.0.0.tgz", diff --git a/superset-frontend/package.json b/superset-frontend/package.json index 90e2122b00665..a982865eb2fe4 100644 --- a/superset-frontend/package.json +++ b/superset-frontend/package.json @@ -117,6 +117,7 @@ "bootstrap": "^3.4.1", "bootstrap-slider": "^10.0.0", "brace": "^0.11.1", + "broadcast-channel": "^4.10.0", "chrono-node": "^2.2.6", "classnames": "^2.2.5", "core-js": "^3.6.5", diff --git a/superset-frontend/spec/helpers/shim.ts b/superset-frontend/spec/helpers/shim.ts index 8543e98ce8bff..2d3e943b73519 100644 --- a/superset-frontend/spec/helpers/shim.ts +++ b/superset-frontend/spec/helpers/shim.ts @@ -74,3 +74,10 @@ g.$ = jQuery(g.window); configureTranslation(); setupSupersetClient(); + +// The useTabId hook depends on BroadcastChannel. Jest has a memory leak problem when +// dealing with native modules. See https://chanind.github.io/javascript/2019/10/12/jest-tests-memory-leak.html +// and https://github.com/facebook/jest/issues/6814 for more information. +jest.mock('src/hooks/useTabId', () => ({ + useTabId: () => 1, +})); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index ba455188f70c2..d434155df105f 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -242,10 +242,15 @@ export default class Chart extends React.Component { onExploreChart = async () => { try { + const lastTabId = window.localStorage.getItem('last_tab_id'); + const nextTabId = lastTabId + ? String(Number.parseInt(lastTabId, 10) + 1) + : undefined; const key = await postFormData( this.props.datasource.id, this.props.formData, this.props.slice.slice_id, + nextTabId, ); const url = mountExploreUrl(null, { [URL_PARAMS.formDataKey.name]: key, diff --git a/superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx b/superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx index 9518e3a4d62d3..9013369d7ac71 100644 --- a/superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx +++ b/superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx @@ -23,6 +23,7 @@ import { t, logging } from '@superset-ui/core'; import { Menu } from 'src/common/components'; import { getUrlParam } from 'src/utils/urlUtils'; import { postFormData } from 'src/explore/exploreUtils/formData'; +import { useTabId } from 'src/hooks/useTabId'; import { URL_PARAMS } from 'src/constants'; import { mountExploreUrl } from 'src/explore/exploreUtils'; import { @@ -56,6 +57,8 @@ const ShareMenuItems = (props: ShareMenuItemProps) => { ...rest } = props; + const tabId = useTabId(); + const getShortUrl = useUrlShortener(url || ''); async function getCopyUrl() { @@ -68,6 +71,7 @@ const ShareMenuItems = (props: ShareMenuItemProps) => { const newDataMaskKey = await createFilterKey( dashboardId, JSON.stringify(prevData), + tabId, ); const newUrl = new URL(`${window.location.origin}${url}`); newUrl.searchParams.set(URL_PARAMS.nativeFilters.name, newDataMaskKey); @@ -80,6 +84,7 @@ const ShareMenuItems = (props: ShareMenuItemProps) => { parseInt(formData.datasource.split('_')[0], 10), formData, formData.slice_id, + tabId, ); return `${window.location.origin}${mountExploreUrl(null, { [URL_PARAMS.formDataKey.name]: key, diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx index b8d5a4b6dc43c..6eba6d565b21f 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx @@ -27,6 +27,7 @@ import { HandlerFunction, styled, t, + SLOW_DEBOUNCE, } from '@superset-ui/core'; import React, { useEffect, useState, useCallback, useMemo } from 'react'; import { useDispatch, useSelector } from 'react-redux'; @@ -38,13 +39,14 @@ import { usePrevious } from 'src/hooks/usePrevious'; import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; import { updateDataMask, clearDataMask } from 'src/dataMask/actions'; import { useImmer } from 'use-immer'; -import { isEmpty, isEqual } from 'lodash'; +import { isEmpty, isEqual, debounce } from 'lodash'; import { testWithId } from 'src/utils/testUtils'; import Loading from 'src/components/Loading'; import { getInitialDataMask } from 'src/dataMask/reducer'; import { URL_PARAMS } from 'src/constants'; import { getUrlParam } from 'src/utils/urlUtils'; import { EmptyStateSmall } from 'src/components/EmptyState'; +import { useTabId } from 'src/hooks/useTabId'; import { checkIsApplyDisabled, TabIds } from './utils'; import FilterSets from './FilterSets'; import { @@ -150,6 +152,53 @@ export interface FiltersBarProps { offset: number; } +const publishDataMask = debounce( + async ( + history, + dashboardId, + updateKey, + dataMaskSelected: DataMaskStateWithId, + tabId, + ) => { + const { location } = history; + const { search } = location; + const previousParams = new URLSearchParams(search); + const newParams = new URLSearchParams(); + let dataMaskKey = ''; + previousParams.forEach((value, key) => { + if (key !== URL_PARAMS.nativeFilters.name) { + newParams.append(key, value); + } + }); + + const nativeFiltersCacheKey = getUrlParam(URL_PARAMS.nativeFiltersKey); + const dataMask = JSON.stringify(dataMaskSelected); + if ( + updateKey && + nativeFiltersCacheKey && + (await updateFilterKey( + dashboardId, + dataMask, + nativeFiltersCacheKey, + tabId, + )) + ) { + dataMaskKey = nativeFiltersCacheKey; + } else { + dataMaskKey = await createFilterKey(dashboardId, dataMask, tabId); + } + newParams.set(URL_PARAMS.nativeFiltersKey.name, dataMaskKey); + + // pathname could be updated somewhere else through window.history + // keep react router history in sync with window history + history.location.pathname = window.location.pathname; + history.replace({ + search: newParams.toString(), + }); + }, + SLOW_DEBOUNCE, +); + const FilterBar: React.FC = ({ filtersOpen, toggleFiltersBar, @@ -165,6 +214,7 @@ const FilterBar: React.FC = ({ useImmer(dataMaskApplied); const dispatch = useDispatch(); const [updateKey, setUpdateKey] = useState(0); + const tabId = useTabId(); const filterSets = useFilterSets(); const filterSetFilterValues = Object.values(filterSets); const [tab, setTab] = useState(TabIds.AllFilters); @@ -203,42 +253,6 @@ const FilterBar: React.FC = ({ [dataMaskSelected, dispatch, setDataMaskSelected, tab], ); - const publishDataMask = useCallback( - async (dataMaskSelected: DataMaskStateWithId) => { - const { location } = history; - const { search } = location; - const previousParams = new URLSearchParams(search); - const newParams = new URLSearchParams(); - let dataMaskKey = ''; - previousParams.forEach((value, key) => { - if (key !== URL_PARAMS.nativeFilters.name) { - newParams.append(key, value); - } - }); - - const nativeFiltersCacheKey = getUrlParam(URL_PARAMS.nativeFiltersKey); - const dataMask = JSON.stringify(dataMaskSelected); - if ( - updateKey && - nativeFiltersCacheKey && - (await updateFilterKey(dashboardId, dataMask, nativeFiltersCacheKey)) - ) { - dataMaskKey = nativeFiltersCacheKey; - } else { - dataMaskKey = await createFilterKey(dashboardId, dataMask); - } - newParams.set(URL_PARAMS.nativeFiltersKey.name, dataMaskKey); - - // pathname could be updated somewhere else through window.history - // keep react router history in sync with window history - history.location.pathname = window.location.pathname; - history.replace({ - search: newParams.toString(), - }); - }, - [history, updateKey], - ); - useEffect(() => { if (previousFilters) { const updates = {}; @@ -272,9 +286,9 @@ const FilterBar: React.FC = ({ const dataMaskAppliedText = JSON.stringify(dataMaskApplied); useEffect(() => { - publishDataMask(dataMaskApplied); + publishDataMask(history, dashboardId, updateKey, dataMaskApplied, tabId); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [dataMaskAppliedText, publishDataMask]); + }, [dashboardId, dataMaskAppliedText, history, updateKey, tabId]); const handleApply = useCallback(() => { const filterIds = Object.keys(dataMaskSelected); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/keyValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/keyValue.tsx index ef8cd68875321..9682fdb7b8f0e 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/keyValue.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/keyValue.tsx @@ -18,9 +18,29 @@ */ import { SupersetClient, logging } from '@superset-ui/core'; -export const updateFilterKey = (dashId: string, value: string, key: string) => +const assembleEndpoint = ( + dashId: string | number, + key?: string, + tabId?: string, +) => { + let endpoint = `api/v1/dashboard/${dashId}/filter_state`; + if (key) { + endpoint = endpoint.concat(`/${key}`); + } + if (tabId) { + endpoint = endpoint.concat(`?tab_id=${tabId}`); + } + return endpoint; +}; + +export const updateFilterKey = ( + dashId: string, + value: string, + key: string, + tabId?: string, +) => SupersetClient.put({ - endpoint: `api/v1/dashboard/${dashId}/filter_state/${key}/`, + endpoint: assembleEndpoint(dashId, key, tabId), jsonPayload: { value }, }) .then(r => r.json.message) @@ -29,9 +49,13 @@ export const updateFilterKey = (dashId: string, value: string, key: string) => return null; }); -export const createFilterKey = (dashId: string | number, value: string) => +export const createFilterKey = ( + dashId: string | number, + value: string, + tabId?: string, +) => SupersetClient.post({ - endpoint: `api/v1/dashboard/${dashId}/filter_state`, + endpoint: assembleEndpoint(dashId, undefined, tabId), jsonPayload: { value }, }) .then(r => r.json.key) @@ -40,12 +64,9 @@ export const createFilterKey = (dashId: string | number, value: string) => return null; }); -export const getFilterValue = ( - dashId: string | number | undefined, - key: string, -) => +export const getFilterValue = (dashId: string | number, key: string) => SupersetClient.get({ - endpoint: `api/v1/dashboard/${dashId}/filter_state/${key}/`, + endpoint: assembleEndpoint(dashId, key), }) .then(({ json }) => JSON.parse(json.value)) .catch(err => { diff --git a/superset-frontend/src/explore/components/ExploreViewContainer.test.jsx b/superset-frontend/src/explore/components/ExploreViewContainer.test.jsx deleted file mode 100644 index 4727a98cadfeb..0000000000000 --- a/superset-frontend/src/explore/components/ExploreViewContainer.test.jsx +++ /dev/null @@ -1,92 +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. - */ -import React from 'react'; -import configureStore from 'redux-mock-store'; -import thunk from 'redux-thunk'; -import { shallow } from 'enzyme'; - -import getInitialState from 'src/explore/reducers/getInitialState'; -import ExploreViewContainer from 'src/explore/components/ExploreViewContainer'; -import QueryAndSaveBtns from 'src/explore/components/QueryAndSaveBtns'; -import ConnectedControlPanelsContainer from 'src/explore/components/ControlPanelsContainer'; -import ChartContainer from 'src/explore/components/ExploreChartPanel'; -import * as featureFlags from 'src/featureFlags'; - -// I added .skip to this entire suite because none of these tests -// are actually testing particularly useful things, -// and too many hacks were needed to get enzyme to play well with context. -// Leaving it here in the hopes that someone can salvage this. -describe.skip('ExploreViewContainer', () => { - const middlewares = [thunk]; - const mockStore = configureStore(middlewares); - let store; - let wrapper; - let isFeatureEnabledMock; - - // jest.spyOn(ReactAll, 'useContext').mockImplementation(() => { - // return { - // store, - // subscription: new Subscription(store), - // }; - // }); - - beforeAll(() => { - isFeatureEnabledMock = jest - .spyOn(featureFlags, 'isFeatureEnabled') - .mockReturnValue(false); - - const bootstrapData = { - common: { - conf: {}, - }, - datasource: { - columns: [], - }, - }; - store = mockStore(getInitialState(bootstrapData), {}); - }); - - afterAll(() => { - isFeatureEnabledMock.mockRestore(); - }); - - beforeEach(() => { - wrapper = shallow(, { - disableLifecycleMethods: true, - }) - .dive() - .dive(); - }); - - it('renders', () => { - expect(React.isValidElement()).toBe(true); - }); - - it('renders QueryAndSaveButtons', () => { - expect(wrapper.find(QueryAndSaveBtns)).toExist(); - }); - - it('renders ControlPanelsContainer', () => { - expect(wrapper.find(ConnectedControlPanelsContainer)).toExist(); - }); - - it('renders ChartContainer', () => { - expect(wrapper.find(ChartContainer)).toExist(); - }); -}); diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index 07c93253849a4..b14ca36c734b1 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -24,7 +24,7 @@ import { connect } from 'react-redux'; import { styled, t, css, useTheme, logging } from '@superset-ui/core'; import { debounce } from 'lodash'; import { Resizable } from 're-resizable'; - +import { useChangeEffect } from 'src/hooks/useChangeEffect'; import { usePluginContext } from 'src/components/DynamicPlugins'; import { Global } from '@emotion/react'; import { Tooltip } from 'src/components/Tooltip'; @@ -44,6 +44,7 @@ import { fetchDatasourceMetadata } from 'src/dashboard/actions/datasources'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import { mergeExtraFormData } from 'src/dashboard/components/nativeFilters/utils'; import { postFormData, putFormData } from 'src/explore/exploreUtils/formData'; +import { useTabId } from 'src/hooks/useTabId'; import ExploreChartPanel from '../ExploreChartPanel'; import ConnectedControlPanelsContainer from '../ControlPanelsContainer'; import SaveModal from '../SaveModal'; @@ -164,7 +165,7 @@ function useWindowSize({ delayMs = 250 } = {}) { } const updateHistory = debounce( - async (formData, datasetId, isReplace, standalone, force, title) => { + async (formData, datasetId, isReplace, standalone, force, title, tabId) => { const payload = { ...formData }; const chartId = formData.slice_id; const additionalParam = {}; @@ -178,11 +179,11 @@ const updateHistory = debounce( let key; let stateModifier; if (isReplace) { - key = await postFormData(datasetId, formData, chartId); + key = await postFormData(datasetId, formData, chartId, tabId); stateModifier = 'replaceState'; } else { key = getUrlParam(URL_PARAMS.formDataKey); - await putFormData(datasetId, key, formData, chartId); + await putFormData(datasetId, key, formData, chartId, tabId); stateModifier = 'pushState'; } const url = mountExploreUrl( @@ -218,6 +219,7 @@ function ExploreViewContainer(props) { const [showingModal, setShowingModal] = useState(false); const [isCollapsed, setIsCollapsed] = useState(false); const [shouldForceUpdate, setShouldForceUpdate] = useState(-1); + const tabId = useTabId(); const theme = useTheme(); const width = `${windowSize.width}px`; @@ -248,6 +250,7 @@ function ExploreViewContainer(props) { props.standalone, props.force, title, + tabId, ); }, [ @@ -256,6 +259,7 @@ function ExploreViewContainer(props) { props.datasource.id, props.standalone, props.force, + tabId, ], ); @@ -322,7 +326,12 @@ function ExploreViewContainer(props) { useComponentDidMount(() => { props.actions.logEvent(LOG_ACTIONS_MOUNT_EXPLORER); - addHistory({ isReplace: true }); + }); + + useChangeEffect(tabId, (previous, current) => { + if (current) { + addHistory({ isReplace: true }); + } }); const previousHandlePopstate = usePrevious(handlePopstate); diff --git a/superset-frontend/src/explore/exploreUtils/formData.ts b/superset-frontend/src/explore/exploreUtils/formData.ts index 25e77c33fefd3..38637c1019623 100644 --- a/superset-frontend/src/explore/exploreUtils/formData.ts +++ b/superset-frontend/src/explore/exploreUtils/formData.ts @@ -24,6 +24,17 @@ type Payload = { chart_id?: number; }; +const assembleEndpoint = (key?: string, tabId?: string) => { + let endpoint = 'api/v1/explore/form_data'; + if (key) { + endpoint = endpoint.concat(`/${key}`); + } + if (tabId) { + endpoint = endpoint.concat(`?tab_id=${tabId}`); + } + return endpoint; +}; + const assemblePayload = ( datasetId: number, form_data: JsonObject, @@ -43,9 +54,10 @@ export const postFormData = ( datasetId: number, form_data: JsonObject, chartId?: number, + tabId?: string, ): Promise => SupersetClient.post({ - endpoint: 'api/v1/explore/form_data', + endpoint: assembleEndpoint(undefined, tabId), jsonPayload: assemblePayload(datasetId, form_data, chartId), }).then(r => r.json.key); @@ -54,8 +66,9 @@ export const putFormData = ( key: string, form_data: JsonObject, chartId?: number, + tabId?: string, ): Promise => SupersetClient.put({ - endpoint: `api/v1/explore/form_data/${key}`, + endpoint: assembleEndpoint(key, tabId), jsonPayload: assemblePayload(datasetId, form_data, chartId), }).then(r => r.json.message); diff --git a/superset-frontend/src/hooks/useTabId.ts b/superset-frontend/src/hooks/useTabId.ts new file mode 100644 index 0000000000000..1bd0df3a36aa7 --- /dev/null +++ b/superset-frontend/src/hooks/useTabId.ts @@ -0,0 +1,73 @@ +/** + * 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. + */ +import { useEffect, useState } from 'react'; +import { BroadcastChannel } from 'broadcast-channel'; + +interface TabIdChannelMessage { + type: 'REQUESTING_TAB_ID' | 'TAB_ID_DENIED'; + tabId: string; +} + +// TODO: We are using broadcast-channel to support Safari. +// The native BroadcastChannel API will be supported in Safari version 15.4. +// After that, we should remove this dependency and use the native API. +const channel = new BroadcastChannel('tab_id_channel'); + +export function useTabId() { + const [tabId, setTabId] = useState(); + + useEffect(() => { + const updateTabId = () => { + const lastTabId = window.localStorage.getItem('last_tab_id'); + const newTabId = String( + lastTabId ? Number.parseInt(lastTabId, 10) + 1 : 1, + ); + window.sessionStorage.setItem('tab_id', newTabId); + window.localStorage.setItem('last_tab_id', newTabId); + setTabId(newTabId); + }; + + const storedTabId = window.sessionStorage.getItem('tab_id'); + if (storedTabId) { + channel.postMessage({ + type: 'REQUESTING_TAB_ID', + tabId: storedTabId, + }); + setTabId(storedTabId); + } else { + updateTabId(); + } + + channel.onmessage = messageEvent => { + if (messageEvent.tabId === tabId) { + if (messageEvent.type === 'REQUESTING_TAB_ID') { + const message: TabIdChannelMessage = { + type: 'TAB_ID_DENIED', + tabId: messageEvent.tabId, + }; + channel.postMessage(message); + } else if (messageEvent.type === 'TAB_ID_DENIED') { + updateTabId(); + } + } + }; + }, [tabId]); + + return tabId; +} diff --git a/superset/dashboards/filter_state/api.py b/superset/dashboards/filter_state/api.py index 68209f3e7d672..6add94558fc6a 100644 --- a/superset/dashboards/filter_state/api.py +++ b/superset/dashboards/filter_state/api.py @@ -65,6 +65,10 @@ def post(self, pk: int) -> Response: schema: type: integer name: pk + - in: query + schema: + type: integer + name: tab_id requestBody: required: true content: @@ -93,7 +97,7 @@ def post(self, pk: int) -> Response: """ return super().post(pk) - @expose("//filter_state//", methods=["PUT"]) + @expose("//filter_state/", methods=["PUT"]) @protect() @safe @event_logger.log_this_with_context( @@ -115,6 +119,10 @@ def put(self, pk: int, key: str) -> Response: schema: type: string name: key + - in: query + schema: + type: integer + name: tab_id requestBody: required: true content: @@ -129,9 +137,9 @@ def put(self, pk: int, key: str) -> Response: schema: type: object properties: - message: + key: type: string - description: The result of the operation + description: The key to retrieve the value. 400: $ref: '#/components/responses/400' 401: @@ -145,7 +153,7 @@ def put(self, pk: int, key: str) -> Response: """ return super().put(pk, key) - @expose("//filter_state//", methods=["GET"]) + @expose("//filter_state/", methods=["GET"]) @protect() @safe @event_logger.log_this_with_context( @@ -191,7 +199,7 @@ def get(self, pk: int, key: str) -> Response: """ return super().get(pk, key) - @expose("//filter_state//", methods=["DELETE"]) + @expose("//filter_state/", methods=["DELETE"]) @protect() @safe @event_logger.log_this_with_context( diff --git a/superset/dashboards/filter_state/commands/create.py b/superset/dashboards/filter_state/commands/create.py index 40a70ba27ce16..f1abe8a5f0e92 100644 --- a/superset/dashboards/filter_state/commands/create.py +++ b/superset/dashboards/filter_state/commands/create.py @@ -14,22 +14,29 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from flask import session + from superset.dashboards.dao import DashboardDAO from superset.extensions import cache_manager from superset.key_value.commands.create import CreateKeyValueCommand from superset.key_value.commands.entry import Entry from superset.key_value.commands.parameters import CommandParameters -from superset.key_value.utils import cache_key +from superset.key_value.utils import cache_key, random_key class CreateFilterStateCommand(CreateKeyValueCommand): - def create(self, cmd_params: CommandParameters) -> bool: + def create(self, cmd_params: CommandParameters) -> str: resource_id = cmd_params.resource_id actor = cmd_params.actor - key = cache_key(resource_id, cmd_params.key) + tab_id = cmd_params.tab_id + contextual_key = cache_key(session.get("_id"), tab_id, resource_id) + key = cache_manager.filter_state_cache.get(contextual_key) + if not key or not tab_id: + key = random_key() value = cmd_params.value dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard and value: entry: Entry = {"owner": actor.get_user_id(), "value": value} - return cache_manager.filter_state_cache.set(key, entry) - return False + cache_manager.filter_state_cache.set(cache_key(resource_id, key), entry) + cache_manager.filter_state_cache.set(contextual_key, key) + return key diff --git a/superset/dashboards/filter_state/commands/delete.py b/superset/dashboards/filter_state/commands/delete.py index 225dd8aa63cd0..1ad3f5e547367 100644 --- a/superset/dashboards/filter_state/commands/delete.py +++ b/superset/dashboards/filter_state/commands/delete.py @@ -14,6 +14,8 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from flask import session + from superset.dashboards.dao import DashboardDAO from superset.extensions import cache_manager from superset.key_value.commands.delete import DeleteKeyValueCommand @@ -34,5 +36,8 @@ def delete(self, cmd_params: CommandParameters) -> bool: if entry: if entry["owner"] != actor.get_user_id(): raise KeyValueAccessDeniedError() + tab_id = cmd_params.tab_id + contextual_key = cache_key(session.get("_id"), tab_id, resource_id) + cache_manager.filter_state_cache.delete(contextual_key) return cache_manager.filter_state_cache.delete(key) return False diff --git a/superset/dashboards/filter_state/commands/update.py b/superset/dashboards/filter_state/commands/update.py index e8d0606f672d3..5d8d9151dd8cd 100644 --- a/superset/dashboards/filter_state/commands/update.py +++ b/superset/dashboards/filter_state/commands/update.py @@ -14,28 +14,46 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from typing import Optional + +from flask import session + from superset.dashboards.dao import DashboardDAO from superset.extensions import cache_manager from superset.key_value.commands.entry import Entry from superset.key_value.commands.exceptions import KeyValueAccessDeniedError from superset.key_value.commands.parameters import CommandParameters from superset.key_value.commands.update import UpdateKeyValueCommand -from superset.key_value.utils import cache_key +from superset.key_value.utils import cache_key, random_key class UpdateFilterStateCommand(UpdateKeyValueCommand): - def update(self, cmd_params: CommandParameters) -> bool: + def update(self, cmd_params: CommandParameters) -> Optional[str]: resource_id = cmd_params.resource_id actor = cmd_params.actor - key = cache_key(resource_id, cmd_params.key) + key = cmd_params.key value = cmd_params.value dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard and value: - entry: Entry = cache_manager.filter_state_cache.get(key) + entry: Entry = cache_manager.filter_state_cache.get( + cache_key(resource_id, key) + ) if entry: user_id = actor.get_user_id() if entry["owner"] != user_id: raise KeyValueAccessDeniedError() + + # Generate a new key if tab_id changes or equals 0 + contextual_key = cache_key( + session.get("_id"), cmd_params.tab_id, resource_id + ) + key = cache_manager.filter_state_cache.get(contextual_key) + if not key or not cmd_params.tab_id: + key = random_key() + cache_manager.filter_state_cache.set(contextual_key, key) + new_entry: Entry = {"owner": actor.get_user_id(), "value": value} - return cache_manager.filter_state_cache.set(key, new_entry) - return False + cache_manager.filter_state_cache.set( + cache_key(resource_id, key), new_entry + ) + return key diff --git a/superset/explore/form_data/api.py b/superset/explore/form_data/api.py index fe73ed0d3af92..7cbb22715c700 100644 --- a/superset/explore/form_data/api.py +++ b/superset/explore/form_data/api.py @@ -73,6 +73,11 @@ def post(self) -> Response: post: description: >- Stores a new form_data. + parameters: + - in: query + schema: + type: integer + name: tab_id requestBody: required: true content: @@ -101,10 +106,12 @@ def post(self) -> Response: """ try: item = self.add_model_schema.load(request.json) + tab_id = request.args.get("tab_id") args = CommandParameters( actor=g.user, dataset_id=item["dataset_id"], chart_id=item.get("chart_id"), + tab_id=tab_id, form_data=item["form_data"], ) key = CreateFormDataCommand(args).run() @@ -139,6 +146,10 @@ def put(self, key: str) -> Response: schema: type: string name: key + - in: query + schema: + type: integer + name: tab_id requestBody: required: true content: @@ -153,9 +164,9 @@ def put(self, key: str) -> Response: schema: type: object properties: - message: + key: type: string - description: The result of the operation + description: The key to retrieve the form_data. 400: $ref: '#/components/responses/400' 401: @@ -169,17 +180,19 @@ def put(self, key: str) -> Response: """ try: item = self.edit_model_schema.load(request.json) + tab_id = request.args.get("tab_id") args = CommandParameters( actor=g.user, dataset_id=item["dataset_id"], chart_id=item.get("chart_id"), + tab_id=tab_id, key=key, form_data=item["form_data"], ) result = UpdateFormDataCommand(args).run() if not result: return self.response_404() - return self.response(200, message="Value updated successfully.") + return self.response(200, key=result) except ValidationError as ex: return self.response(400, message=ex.messages) except ( diff --git a/superset/explore/form_data/commands/create.py b/superset/explore/form_data/commands/create.py index a2325f0be924e..c1fa61e14a31b 100644 --- a/superset/explore/form_data/commands/create.py +++ b/superset/explore/form_data/commands/create.py @@ -15,8 +15,8 @@ # specific language governing permissions and limitations # under the License. import logging -from secrets import token_urlsafe +from flask import session from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand @@ -25,6 +25,7 @@ from superset.explore.form_data.utils import check_access from superset.extensions import cache_manager from superset.key_value.commands.exceptions import KeyValueCreateFailedError +from superset.key_value.utils import cache_key, random_key logger = logging.getLogger(__name__) @@ -37,10 +38,14 @@ def run(self) -> str: try: dataset_id = self._cmd_params.dataset_id chart_id = self._cmd_params.chart_id + tab_id = self._cmd_params.tab_id actor = self._cmd_params.actor form_data = self._cmd_params.form_data check_access(dataset_id, chart_id, actor) - key = token_urlsafe(48) + contextual_key = cache_key(session.get("_id"), tab_id, dataset_id, chart_id) + key = cache_manager.explore_form_data_cache.get(contextual_key) + if not key or not tab_id: + key = random_key() if form_data: state: TemporaryExploreState = { "owner": actor.get_user_id(), @@ -49,6 +54,7 @@ def run(self) -> str: "form_data": form_data, } cache_manager.explore_form_data_cache.set(key, state) + cache_manager.explore_form_data_cache.set(contextual_key, key) return key except SQLAlchemyError as ex: logger.exception("Error running create command") diff --git a/superset/explore/form_data/commands/delete.py b/superset/explore/form_data/commands/delete.py index 8dc414ee14cd2..80193186ea311 100644 --- a/superset/explore/form_data/commands/delete.py +++ b/superset/explore/form_data/commands/delete.py @@ -17,6 +17,7 @@ import logging from abc import ABC +from flask import session from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand @@ -28,6 +29,7 @@ KeyValueAccessDeniedError, KeyValueDeleteFailedError, ) +from superset.key_value.utils import cache_key logger = logging.getLogger(__name__) @@ -44,9 +46,16 @@ def run(self) -> bool: key ) if state: - check_access(state["dataset_id"], state["chart_id"], actor) + dataset_id = state["dataset_id"] + chart_id = state["chart_id"] + check_access(dataset_id, chart_id, actor) if state["owner"] != actor.get_user_id(): raise KeyValueAccessDeniedError() + tab_id = self._cmd_params.tab_id + contextual_key = cache_key( + session.get("_id"), tab_id, dataset_id, chart_id + ) + cache_manager.explore_form_data_cache.delete(contextual_key) return cache_manager.explore_form_data_cache.delete(key) return False except SQLAlchemyError as ex: diff --git a/superset/explore/form_data/commands/parameters.py b/superset/explore/form_data/commands/parameters.py index c4d41a184a781..3e830810b5000 100644 --- a/superset/explore/form_data/commands/parameters.py +++ b/superset/explore/form_data/commands/parameters.py @@ -25,5 +25,6 @@ class CommandParameters: actor: User dataset_id: int = 0 chart_id: int = 0 + tab_id: Optional[int] = None key: Optional[str] = None form_data: Optional[str] = None diff --git a/superset/explore/form_data/commands/update.py b/superset/explore/form_data/commands/update.py index 8b69ae885f714..8c8b6a500bcbc 100644 --- a/superset/explore/form_data/commands/update.py +++ b/superset/explore/form_data/commands/update.py @@ -16,7 +16,9 @@ # under the License. import logging from abc import ABC +from typing import Optional +from flask import session from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand @@ -28,6 +30,7 @@ KeyValueAccessDeniedError, KeyValueUpdateFailedError, ) +from superset.key_value.utils import cache_key, random_key logger = logging.getLogger(__name__) @@ -38,7 +41,7 @@ def __init__( ): self._cmd_params = cmd_params - def run(self) -> bool: + def run(self) -> Optional[str]: try: dataset_id = self._cmd_params.dataset_id chart_id = self._cmd_params.chart_id @@ -53,14 +56,25 @@ def run(self) -> bool: user_id = actor.get_user_id() if state["owner"] != user_id: raise KeyValueAccessDeniedError() + + # Generate a new key if tab_id changes or equals 0 + tab_id = self._cmd_params.tab_id + contextual_key = cache_key( + session.get("_id"), tab_id, dataset_id, chart_id + ) + key = cache_manager.explore_form_data_cache.get(contextual_key) + if not key or not tab_id: + key = random_key() + cache_manager.explore_form_data_cache.set(contextual_key, key) + new_state: TemporaryExploreState = { "owner": actor.get_user_id(), "dataset_id": dataset_id, "chart_id": chart_id, "form_data": form_data, } - return cache_manager.explore_form_data_cache.set(key, new_state) - return False + cache_manager.explore_form_data_cache.set(key, new_state) + return key except SQLAlchemyError as ex: logger.exception("Error running update command") raise KeyValueUpdateFailedError() from ex diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 8f6a0d2d06364..f01cb363e2e6d 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -73,11 +73,9 @@ def add_apispec_components(self, api_spec: APISpec) -> None: def post(self, pk: int) -> Response: try: item = self.add_model_schema.load(request.json) + tab_id = request.args.get("tab_id") args = CommandParameters( - actor=g.user, - resource_id=pk, - value=item["value"], - query_params=request.args, + actor=g.user, resource_id=pk, value=item["value"], tab_id=tab_id ) key = self.get_create_command()(args).run() return self.response(201, key=key) @@ -97,17 +95,16 @@ def post(self, pk: int) -> Response: def put(self, pk: int, key: str) -> Response: try: item = self.edit_model_schema.load(request.json) + tab_id = request.args.get("tab_id") args = CommandParameters( actor=g.user, resource_id=pk, key=key, value=item["value"], - query_params=request.args, + tab_id=tab_id, ) - result = self.get_update_command()(args).run() - if not result: - return self.response_404() - return self.response(200, message="Value updated successfully.") + key = self.get_update_command()(args).run() + return self.response(200, key=key) except ValidationError as ex: return self.response(400, message=ex.messages) except ( @@ -122,9 +119,7 @@ def put(self, pk: int, key: str) -> Response: def get(self, pk: int, key: str) -> Response: try: - args = CommandParameters( - actor=g.user, resource_id=pk, key=key, query_params=request.args - ) + args = CommandParameters(actor=g.user, resource_id=pk, key=key) value = self.get_get_command()(args).run() if not value: return self.response_404() @@ -141,9 +136,7 @@ def get(self, pk: int, key: str) -> Response: def delete(self, pk: int, key: str) -> Response: try: - args = CommandParameters( - actor=g.user, resource_id=pk, key=key, query_params=request.args - ) + args = CommandParameters(actor=g.user, resource_id=pk, key=key) result = self.get_delete_command()(args).run() if not result: return self.response_404() diff --git a/superset/key_value/commands/create.py b/superset/key_value/commands/create.py index 8a7eec9298c0b..987c02bee6cb2 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -16,7 +16,6 @@ # under the License. import logging from abc import ABC, abstractmethod -from secrets import token_urlsafe from sqlalchemy.exc import SQLAlchemyError @@ -33,10 +32,7 @@ def __init__(self, cmd_params: CommandParameters): def run(self) -> str: try: - key = token_urlsafe(48) - self._cmd_params.key = key - self.create(self._cmd_params) - return key + return self.create(self._cmd_params) except SQLAlchemyError as ex: logger.exception("Error running create command") raise KeyValueCreateFailedError() from ex @@ -45,5 +41,5 @@ def validate(self) -> None: pass @abstractmethod - def create(self, cmd_params: CommandParameters) -> bool: + def create(self, cmd_params: CommandParameters) -> str: ... diff --git a/superset/key_value/commands/parameters.py b/superset/key_value/commands/parameters.py index 00a933c67c713..4d98167c37231 100644 --- a/superset/key_value/commands/parameters.py +++ b/superset/key_value/commands/parameters.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. from dataclasses import dataclass -from typing import Dict, Optional +from typing import Optional from flask_appbuilder.security.sqla.models import User @@ -24,6 +24,6 @@ class CommandParameters: actor: User resource_id: int - query_params: Dict[str, str] + tab_id: Optional[int] = None key: Optional[str] = None value: Optional[str] = None diff --git a/superset/key_value/commands/update.py b/superset/key_value/commands/update.py index 7990d2125272f..b0949193064ac 100644 --- a/superset/key_value/commands/update.py +++ b/superset/key_value/commands/update.py @@ -16,6 +16,7 @@ # under the License. import logging from abc import ABC, abstractmethod +from typing import Optional from sqlalchemy.exc import SQLAlchemyError @@ -32,7 +33,7 @@ def __init__( ): self._parameters = cmd_params - def run(self) -> bool: + def run(self) -> Optional[str]: try: return self.update(self._parameters) except SQLAlchemyError as ex: @@ -43,5 +44,5 @@ def validate(self) -> None: pass @abstractmethod - def update(self, cmd_params: CommandParameters) -> bool: + def update(self, cmd_params: CommandParameters) -> Optional[str]: ... diff --git a/superset/key_value/utils.py b/superset/key_value/utils.py index 9ba2a8d036077..2f2f71f957e08 100644 --- a/superset/key_value/utils.py +++ b/superset/key_value/utils.py @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from secrets import token_urlsafe from typing import Any SEPARATOR = ";" @@ -21,3 +22,7 @@ def cache_key(*args: Any) -> str: return SEPARATOR.join(str(arg) for arg in args) + + +def random_key() -> str: + return token_urlsafe(48) diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index bec6dfc5f220e..f89efce29f3f7 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -82,9 +82,7 @@ def test_post_bad_request(client, dashboard_id: int): payload = { "value": 1234, } - resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload - ) + resp = client.post(f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload) assert resp.status_code == 400 @@ -99,24 +97,128 @@ def test_post_access_denied(mock_raise_for_dashboard_access, client, dashboard_i assert resp.status_code == 403 +def test_post_same_key_for_same_tab_id(client, dashboard_id: int): + login(client, "admin") + payload = { + "value": value, + } + resp = client.post( + f"api/v1/dashboard/{dashboard_id}/filter_state?tab_id=1", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.post( + f"api/v1/dashboard/{dashboard_id}/filter_state?tab_id=1", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key == second_key + + +def test_post_different_key_for_different_tab_id(client, dashboard_id: int): + login(client, "admin") + payload = { + "value": value, + } + resp = client.post( + f"api/v1/dashboard/{dashboard_id}/filter_state?tab_id=1", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.post( + f"api/v1/dashboard/{dashboard_id}/filter_state?tab_id=2", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + +def test_post_different_key_for_no_tab_id(client, dashboard_id: int): + login(client, "admin") + payload = { + "value": value, + } + resp = client.post(f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.post(f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + def test_put(client, dashboard_id: int): login(client, "admin") payload = { "value": "new value", } resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}", json=payload ) assert resp.status_code == 200 +def test_put_same_key_for_same_tab_id(client, dashboard_id: int): + login(client, "admin") + payload = { + "value": value, + } + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}?tab_id=1", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}?tab_id=1", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key == second_key + + +def test_put_different_key_for_different_tab_id(client, dashboard_id: int): + login(client, "admin") + payload = { + "value": value, + } + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}?tab_id=1", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}?tab_id=2", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + +def test_put_different_key_for_no_tab_id(client, dashboard_id: int): + login(client, "admin") + payload = { + "value": value, + } + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}", json=payload + ) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + def test_put_bad_request(client, dashboard_id: int): login(client, "admin") payload = { "value": 1234, } resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}", json=payload ) assert resp.status_code == 400 @@ -129,7 +231,7 @@ def test_put_access_denied(mock_raise_for_dashboard_access, client, dashboard_id "value": "new value", } resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}", json=payload ) assert resp.status_code == 403 @@ -140,7 +242,7 @@ def test_put_not_owner(client, dashboard_id: int): "value": "new value", } resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}", json=payload ) assert resp.status_code == 403 @@ -153,13 +255,13 @@ def test_get_key_not_found(client, dashboard_id: int): def test_get_dashboard_not_found(client): login(client, "admin") - resp = client.get(f"api/v1/dashboard/{-1}/filter_state/{key}/") + resp = client.get(f"api/v1/dashboard/{-1}/filter_state/{key}") assert resp.status_code == 404 def test_get(client, dashboard_id: int): login(client, "admin") - resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}") assert resp.status_code == 200 data = json.loads(resp.data.decode("utf-8")) assert value == data.get("value") @@ -169,13 +271,13 @@ def test_get(client, dashboard_id: int): def test_get_access_denied(mock_raise_for_dashboard_access, client, dashboard_id): login(client, "admin") mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}") assert resp.status_code == 403 def test_delete(client, dashboard_id: int): login(client, "admin") - resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}") assert resp.status_code == 200 @@ -185,11 +287,11 @@ def test_delete_access_denied( ): login(client, "admin") mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}") assert resp.status_code == 403 def test_delete_not_owner(client, dashboard_id: int): login(client, "gamma") - resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}") assert resp.status_code == 403 diff --git a/tests/integration_tests/explore/form_data/api_tests.py b/tests/integration_tests/explore/form_data/api_tests.py index bf0fc6cab4bec..5e97aae6b7654 100644 --- a/tests/integration_tests/explore/form_data/api_tests.py +++ b/tests/integration_tests/explore/form_data/api_tests.py @@ -118,6 +118,94 @@ def test_post_access_denied(client, chart_id: int, dataset_id: int): assert resp.status_code == 404 +def test_post_same_key_for_same_context(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, + "form_data": "new form_data", + } + resp = client.post("api/v1/explore/form_data?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.post("api/v1/explore/form_data?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key == second_key + + +def test_post_different_key_for_different_context( + client, chart_id: int, dataset_id: int +): + login(client, "admin") + payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, + "form_data": "new form_data", + } + resp = client.post("api/v1/explore/form_data?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + payload = { + "dataset_id": dataset_id, + "form_data": "new form_data", + } + resp = client.post("api/v1/explore/form_data?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + +def test_post_same_key_for_same_tab_id(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, + "form_data": "new form_data", + } + resp = client.post("api/v1/explore/form_data?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.post("api/v1/explore/form_data?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key == second_key + + +def test_post_different_key_for_different_tab_id( + client, chart_id: int, dataset_id: int +): + login(client, "admin") + payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, + "form_data": "new form_data", + } + resp = client.post("api/v1/explore/form_data?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.post("api/v1/explore/form_data?tab_id=2", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + +def test_post_different_key_for_no_tab_id(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, + "form_data": "new form_data", + } + resp = client.post("api/v1/explore/form_data", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.post("api/v1/explore/form_data", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + def test_put(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { @@ -129,6 +217,54 @@ def test_put(client, chart_id: int, dataset_id: int): assert resp.status_code == 200 +def test_put_same_key_for_same_tab_id(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, + "form_data": "new form_data", + } + resp = client.put(f"api/v1/explore/form_data/{key}?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.put(f"api/v1/explore/form_data/{key}?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key == second_key + + +def test_put_different_key_for_different_tab_id(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, + "form_data": "new form_data", + } + resp = client.put(f"api/v1/explore/form_data/{key}?tab_id=1", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.put(f"api/v1/explore/form_data/{key}?tab_id=2", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + +def test_put_different_key_for_no_tab_id(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, + "form_data": "new form_data", + } + resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) + data = json.loads(resp.data.decode("utf-8")) + first_key = data.get("key") + resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) + data = json.loads(resp.data.decode("utf-8")) + second_key = data.get("key") + assert first_key != second_key + + def test_put_bad_request(client, chart_id: int, dataset_id: int): login(client, "admin") payload = {