From 0c0d2b38a672bd2fef8dad75d0bffe78e8a5b80e Mon Sep 17 00:00:00 2001 From: "JUST.in DO IT" Date: Tue, 18 Apr 2023 12:26:29 -0700 Subject: [PATCH 01/47] fix(sqllab): infinite running state on disconnect (#23669) --- .../src/SqlLab/actions/sqlLab.js | 5 + .../src/SqlLab/components/App/index.jsx | 3 +- .../QueryAutoRefresh.test.tsx | 127 +++++++++++++++--- .../components/QueryAutoRefresh/index.tsx | 46 ++++--- .../src/SqlLab/reducers/sqlLab.js | 15 +++ 5 files changed, 158 insertions(+), 38 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index 260f9944f9b0..e4cc78ef0f94 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -80,6 +80,7 @@ export const STOP_QUERY = 'STOP_QUERY'; export const REQUEST_QUERY_RESULTS = 'REQUEST_QUERY_RESULTS'; export const QUERY_SUCCESS = 'QUERY_SUCCESS'; export const QUERY_FAILED = 'QUERY_FAILED'; +export const CLEAR_INACTIVE_QUERIES = 'CLEAR_INACTIVE_QUERIES'; export const CLEAR_QUERY_RESULTS = 'CLEAR_QUERY_RESULTS'; export const REMOVE_DATA_PREVIEW = 'REMOVE_DATA_PREVIEW'; export const CHANGE_DATA_PREVIEW_ID = 'CHANGE_DATA_PREVIEW_ID'; @@ -219,6 +220,10 @@ export function estimateQueryCost(queryEditor) { }; } +export function clearInactiveQueries() { + return { type: CLEAR_INACTIVE_QUERIES }; +} + export function startQuery(query) { Object.assign(query, { id: query.id ? query.id : shortid.generate(), diff --git a/superset-frontend/src/SqlLab/components/App/index.jsx b/superset-frontend/src/SqlLab/components/App/index.jsx index 4689f8ec2191..bbd1bba9aead 100644 --- a/superset-frontend/src/SqlLab/components/App/index.jsx +++ b/superset-frontend/src/SqlLab/components/App/index.jsx @@ -168,7 +168,7 @@ class App extends React.PureComponent { } render() { - const { queries, actions, queriesLastUpdate } = this.props; + const { queries, queriesLastUpdate } = this.props; if (this.state.hash && this.state.hash === '#search') { return window.location.replace('/superset/sqllab/history/'); } @@ -176,7 +176,6 @@ class App extends React.PureComponent { diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx index 32bf401f2213..9b2c1feaefda 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx @@ -16,15 +16,26 @@ * specific language governing permissions and limitations * under the License. */ +import fetchMock from 'fetch-mock'; import React from 'react'; -import { render } from '@testing-library/react'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { render, waitFor } from 'spec/helpers/testing-library'; +import { + CLEAR_INACTIVE_QUERIES, + REFRESH_QUERIES, +} from 'src/SqlLab/actions/sqlLab'; import QueryAutoRefresh, { isQueryRunning, shouldCheckForQueries, + QUERY_UPDATE_FREQ, } from 'src/SqlLab/components/QueryAutoRefresh'; import { successfulQuery, runningQuery } from 'src/SqlLab/fixtures'; import { QueryDictionary } from 'src/SqlLab/types'; +const middlewares = [thunk]; +const mockStore = configureStore(middlewares); + // NOTE: The uses of @ts-ignore in this file is to enable testing of bad inputs to verify the // function / component handles bad data elegantly describe('QueryAutoRefresh', () => { @@ -34,10 +45,14 @@ describe('QueryAutoRefresh', () => { const successfulQueries: QueryDictionary = {}; successfulQueries[successfulQuery.id] = successfulQuery; - const refreshQueries = jest.fn(); - const queriesLastUpdate = Date.now(); + const refreshApi = 'glob:*/api/v1/query/updated_since?*'; + + afterEach(() => { + fetchMock.reset(); + }); + it('isQueryRunning returns true for valid running query', () => { const running = isQueryRunning(runningQuery); expect(running).toBe(true); @@ -91,43 +106,119 @@ describe('QueryAutoRefresh', () => { ).toBe(false); }); - it('Attempts to refresh when given pending query', () => { + it('Attempts to refresh when given pending query', async () => { + const store = mockStore(); + fetchMock.get(refreshApi, { + result: [ + { + id: runningQuery.id, + status: 'success', + }, + ], + }); + render( , + { useRedux: true, store }, + ); + await waitFor( + () => + expect(store.getActions()).toContainEqual( + expect.objectContaining({ + type: REFRESH_QUERIES, + }), + ), + { timeout: QUERY_UPDATE_FREQ + 100 }, ); - setTimeout(() => { - expect(refreshQueries).toHaveBeenCalled(); - }, 1000); }); - it('Does not fail and attempts to refresh when given pending query and invlaid query', () => { + it('Attempts to clear inactive queries when updated queries are empty', async () => { + const store = mockStore(); + fetchMock.get(refreshApi, { + result: [], + }); + + render( + , + { useRedux: true, store }, + ); + await waitFor( + () => + expect(store.getActions()).toContainEqual( + expect.objectContaining({ + type: CLEAR_INACTIVE_QUERIES, + }), + ), + { timeout: QUERY_UPDATE_FREQ + 100 }, + ); + expect( + store.getActions().filter(({ type }) => type === REFRESH_QUERIES), + ).toHaveLength(0); + expect(fetchMock.calls(refreshApi)).toHaveLength(1); + }); + + it('Does not fail and attempts to refresh when given pending query and invlaid query', async () => { + const store = mockStore(); + fetchMock.get(refreshApi, { + result: [ + { + id: runningQuery.id, + status: 'success', + }, + ], + }); + render( , + { useRedux: true, store }, + ); + await waitFor( + () => + expect(store.getActions()).toContainEqual( + expect.objectContaining({ + type: REFRESH_QUERIES, + }), + ), + { timeout: QUERY_UPDATE_FREQ + 100 }, ); - setTimeout(() => { - expect(refreshQueries).toHaveBeenCalled(); - }, 1000); }); - it('Does NOT Attempt to refresh when given only completed queries', () => { + it('Does NOT Attempt to refresh when given only completed queries', async () => { + const store = mockStore(); + fetchMock.get(refreshApi, { + result: [ + { + id: runningQuery.id, + status: 'success', + }, + ], + }); render( , + { useRedux: true, store }, + ); + await waitFor( + () => + expect(store.getActions()).toContainEqual( + expect.objectContaining({ + type: CLEAR_INACTIVE_QUERIES, + }), + ), + { timeout: QUERY_UPDATE_FREQ + 100 }, ); - setTimeout(() => { - expect(refreshQueries).not.toHaveBeenCalled(); - }, 1000); + expect(fetchMock.calls(refreshApi)).toHaveLength(0); }); }); diff --git a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx index 2d01e724e247..65a6d11a1d1a 100644 --- a/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx +++ b/superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx @@ -16,7 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { useState } from 'react'; +import { useRef } from 'react'; +import { useDispatch } from 'react-redux'; import { isObject } from 'lodash'; import rison from 'rison'; import { @@ -27,19 +28,18 @@ import { } from '@superset-ui/core'; import { QueryDictionary } from 'src/SqlLab/types'; import useInterval from 'src/SqlLab/utils/useInterval'; +import { + refreshQueries, + clearInactiveQueries, +} from 'src/SqlLab/actions/sqlLab'; -const QUERY_UPDATE_FREQ = 2000; +export const QUERY_UPDATE_FREQ = 2000; const QUERY_UPDATE_BUFFER_MS = 5000; const MAX_QUERY_AGE_TO_POLL = 21600000; const QUERY_TIMEOUT_LIMIT = 10000; -interface RefreshQueriesFunc { - (alteredQueries: any): any; -} - export interface QueryAutoRefreshProps { queries: QueryDictionary; - refreshQueries: RefreshQueriesFunc; queriesLastUpdate: number; } @@ -61,20 +61,22 @@ export const shouldCheckForQueries = (queryList: QueryDictionary): boolean => { function QueryAutoRefresh({ queries, - refreshQueries, queriesLastUpdate, }: QueryAutoRefreshProps) { // We do not want to spam requests in the case of slow connections and potentially receive responses out of order // pendingRequest check ensures we only have one active http call to check for query statuses - const [pendingRequest, setPendingRequest] = useState(false); + const pendingRequestRef = useRef(false); + const cleanInactiveRequestRef = useRef(false); + const dispatch = useDispatch(); const checkForRefresh = () => { - if (!pendingRequest && shouldCheckForQueries(queries)) { + const shouldRequestChecking = shouldCheckForQueries(queries); + if (!pendingRequestRef.current && shouldRequestChecking) { const params = rison.encode({ last_updated_ms: queriesLastUpdate - QUERY_UPDATE_BUFFER_MS, }); - setPendingRequest(true); + pendingRequestRef.current = true; SupersetClient.get({ endpoint: `/api/v1/query/updated_since?q=${params}`, timeout: QUERY_TIMEOUT_LIMIT, @@ -82,19 +84,27 @@ function QueryAutoRefresh({ .then(({ json }) => { if (json) { const jsonPayload = json as { result?: QueryResponse[] }; - const queries = - jsonPayload?.result?.reduce((acc, current) => { - acc[current.id] = current; - return acc; - }, {}) ?? {}; - refreshQueries?.(queries); + if (jsonPayload?.result?.length) { + const queries = + jsonPayload?.result?.reduce((acc, current) => { + acc[current.id] = current; + return acc; + }, {}) ?? {}; + dispatch(refreshQueries(queries)); + } else { + dispatch(clearInactiveQueries()); + } } }) .catch(() => {}) .finally(() => { - setPendingRequest(false); + pendingRequestRef.current = false; }); } + if (!cleanInactiveRequestRef.current && !shouldRequestChecking) { + dispatch(clearInactiveQueries()); + cleanInactiveRequestRef.current = true; + } }; // Solves issue where direct usage of setInterval in function components diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.js b/superset-frontend/src/SqlLab/reducers/sqlLab.js index ff2cb340bbd0..03b239e56b69 100644 --- a/superset-frontend/src/SqlLab/reducers/sqlLab.js +++ b/superset-frontend/src/SqlLab/reducers/sqlLab.js @@ -742,6 +742,21 @@ export default function sqlLabReducer(state = {}, action) { } return { ...state, queries: newQueries, queriesLastUpdate }; }, + [actions.CLEAR_INACTIVE_QUERIES]() { + const { queries } = state; + const cleanedQueries = Object.fromEntries( + Object.entries(queries).filter(([, query]) => { + if ( + ['running', 'pending'].includes(query.state) && + query.progress === 0 + ) { + return false; + } + return true; + }), + ); + return { ...state, queries: cleanedQueries }; + }, [actions.SET_USER_OFFLINE]() { return { ...state, offline: action.offline }; }, From d6b6d9eae654d7d57a20b9c52d9b9b956627877a Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Tue, 18 Apr 2023 16:31:09 -0300 Subject: [PATCH 02/47] feat: Makes "Add to dashboard" in Save chart modal paginated (#23634) --- .../cypress/integration/dashboard/utils.ts | 4 - .../cypress/integration/explore/utils.ts | 7 +- .../src/explore/actions/saveModalActions.js | 37 --- .../explore/actions/saveModalActions.test.js | 68 ---- .../src/explore/components/SaveModal.test.jsx | 24 +- .../src/explore/components/SaveModal.tsx | 294 ++++++++++-------- 6 files changed, 170 insertions(+), 264 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/utils.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/utils.ts index 0b8776b06c1a..44322e1c42ce 100644 --- a/superset-frontend/cypress-base/cypress/integration/dashboard/utils.ts +++ b/superset-frontend/cypress-base/cypress/integration/dashboard/utils.ts @@ -163,10 +163,6 @@ export function interceptDatasets() { cy.intercept('GET', `/api/v1/dashboard/*/datasets`).as('getDatasets'); } -export function interceptDashboardasync() { - cy.intercept('GET', `/dashboardasync/api/read*`).as('getDashboardasync'); -} - export function interceptFilterState() { cy.intercept('POST', `/api/v1/dashboard/*/filter_state*`).as( 'postFilterState', diff --git a/superset-frontend/cypress-base/cypress/integration/explore/utils.ts b/superset-frontend/cypress-base/cypress/integration/explore/utils.ts index 15e7dcba1b6f..04cf1f181998 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/utils.ts +++ b/superset-frontend/cypress-base/cypress/integration/explore/utils.ts @@ -17,10 +17,7 @@ * under the License. */ -import { - interceptGet as interceptDashboardGet, - interceptDashboardasync, -} from '../dashboard/utils'; +import { interceptGet as interceptDashboardGet } from '../dashboard/utils'; export function interceptFiltering() { cy.intercept('GET', `/api/v1/chart/?q=*`).as('filtering'); @@ -61,12 +58,10 @@ export function setFilter(filter: string, option: string) { export function saveChartToDashboard(dashboardName: string) { interceptDashboardGet(); - interceptDashboardasync(); interceptUpdate(); interceptExploreGet(); cy.getBySel('query-save-button').click(); - cy.wait('@getDashboardasync'); cy.getBySelLike('chart-modal').should('be.visible'); cy.get( '[data-test="save-chart-modal-select-dashboard-form"] [aria-label="Select a dashboard"]', diff --git a/superset-frontend/src/explore/actions/saveModalActions.js b/superset-frontend/src/explore/actions/saveModalActions.js index 1c3c3b765f78..9bd516139153 100644 --- a/superset-frontend/src/explore/actions/saveModalActions.js +++ b/superset-frontend/src/explore/actions/saveModalActions.js @@ -40,29 +40,6 @@ export function setSaveChartModalVisibility(isVisible) { return { type: SET_SAVE_CHART_MODAL_VISIBILITY, isVisible }; } -export function fetchDashboards(userId) { - return function fetchDashboardsThunk(dispatch) { - return SupersetClient.get({ - endpoint: `/dashboardasync/api/read?_flt_0_owners=${userId}`, - }) - .then(({ json }) => { - const choices = json.pks.map((id, index) => ({ - value: id, - label: (json.result[index] || {}).dashboard_title, - })); - choices.sort((a, b) => - a.label.localeCompare(b.label, { - sensitivity: 'base', - numeric: true, - }), - ); - - return dispatch(fetchDashboardsSucceeded(choices)); - }) - .catch(() => dispatch(fetchDashboardsFailed(userId))); - }; -} - export const SAVE_SLICE_FAILED = 'SAVE_SLICE_FAILED'; export function saveSliceFailed() { return { type: SAVE_SLICE_FAILED }; @@ -241,20 +218,6 @@ export const createDashboard = dashboardName => async dispatch => { } }; -// Get existing dashboard from ID -export const getDashboard = dashboardId => async dispatch => { - try { - const response = await SupersetClient.get({ - endpoint: `/api/v1/dashboard/${dashboardId}`, - }); - - return response.json; - } catch (error) { - dispatch(saveSliceFailed()); - throw error; - } -}; - // Get dashboards the slice is added to export const getSliceDashboards = slice => async dispatch => { try { diff --git a/superset-frontend/src/explore/actions/saveModalActions.test.js b/superset-frontend/src/explore/actions/saveModalActions.test.js index f89729f5ff98..1662ead63e5f 100644 --- a/superset-frontend/src/explore/actions/saveModalActions.test.js +++ b/superset-frontend/src/explore/actions/saveModalActions.test.js @@ -23,10 +23,6 @@ import { ADD_TOAST } from 'src/components/MessageToasts/actions'; import { createDashboard, createSlice, - fetchDashboards, - FETCH_DASHBOARDS_FAILED, - FETCH_DASHBOARDS_SUCCEEDED, - getDashboard, getSliceDashboards, SAVE_SLICE_FAILED, SAVE_SLICE_SUCCESS, @@ -34,37 +30,6 @@ import { getSlicePayload, } from './saveModalActions'; -/** - * Tests fetchDashboards action - */ - -const userId = 1; -const fetchDashboardsEndpoint = `glob:*/dashboardasync/api/read?_flt_0_owners=${1}`; -const mockDashboardData = { - pks: ['id'], - result: [{ id: 'id', dashboard_title: 'dashboard title' }], -}; - -test('fetchDashboards handles success', async () => { - fetchMock.reset(); - fetchMock.get(fetchDashboardsEndpoint, mockDashboardData); - const dispatch = sinon.spy(); - await fetchDashboards(userId)(dispatch); - expect(fetchMock.calls(fetchDashboardsEndpoint)).toHaveLength(1); - expect(dispatch.callCount).toBe(1); - expect(dispatch.getCall(0).args[0].type).toBe(FETCH_DASHBOARDS_SUCCEEDED); -}); - -test('fetchDashboards handles failure', async () => { - fetchMock.reset(); - fetchMock.get(fetchDashboardsEndpoint, { throws: 'error' }); - const dispatch = sinon.spy(); - await fetchDashboards(userId)(dispatch); - expect(fetchMock.calls(fetchDashboardsEndpoint)).toHaveLength(4); // 3 retries - expect(dispatch.callCount).toBe(1); - expect(dispatch.getCall(0).args[0].type).toBe(FETCH_DASHBOARDS_FAILED); -}); - const sliceId = 10; const sliceName = 'New chart'; const vizType = 'sample_viz_type'; @@ -176,7 +141,6 @@ test('createSlice handles failure', async () => { expect(dispatch.getCall(0).args[0].type).toBe(SAVE_SLICE_FAILED); }); -const dashboardId = 14; const dashboardName = 'New dashboard'; const dashboardResponsePayload = { id: 14, @@ -214,38 +178,6 @@ test('createDashboard handles failure', async () => { expect(dispatch.getCall(0).args[0].type).toBe(SAVE_SLICE_FAILED); }); -/** - * Tests getDashboard action - */ - -const getDashboardEndpoint = `glob:*/api/v1/dashboard/${dashboardId}`; -test('getDashboard handles success', async () => { - fetchMock.reset(); - fetchMock.get(getDashboardEndpoint, dashboardResponsePayload); - const dispatch = sinon.spy(); - const dashboard = await getDashboard(dashboardId)(dispatch); - expect(fetchMock.calls(getDashboardEndpoint)).toHaveLength(1); - expect(dispatch.callCount).toBe(0); - expect(dashboard).toEqual(dashboardResponsePayload); -}); - -test('getDashboard handles failure', async () => { - fetchMock.reset(); - fetchMock.get(getDashboardEndpoint, { throws: sampleError }); - const dispatch = sinon.spy(); - let caughtError; - try { - await getDashboard(dashboardId)(dispatch); - } catch (error) { - caughtError = error; - } - - expect(caughtError).toEqual(sampleError); - expect(fetchMock.calls(getDashboardEndpoint)).toHaveLength(4); - expect(dispatch.callCount).toBe(1); - expect(dispatch.getCall(0).args[0].type).toBe(SAVE_SLICE_FAILED); -}); - test('updateSlice with add to new dashboard handles success', async () => { fetchMock.reset(); fetchMock.put(updateSliceEndpoint, sliceResponsePayload); diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx index 15bfc64e7588..74d1c1199c75 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.jsx +++ b/superset-frontend/src/explore/components/SaveModal.test.jsx @@ -20,10 +20,8 @@ import React from 'react'; import configureStore from 'redux-mock-store'; import thunk from 'redux-thunk'; import { bindActionCreators } from 'redux'; -import { Provider } from 'react-redux'; import { shallow } from 'enzyme'; -import { styledMount as mount } from 'spec/helpers/theming'; import { Radio } from 'src/components/Radio'; import Button from 'src/components/Button'; import sinon from 'sinon'; @@ -39,6 +37,7 @@ const initialState = { chart: {}, saveModal: { dashboards: [], + isVisible: true, }, explore: { datasource: {}, @@ -57,6 +56,7 @@ const initialState = { const initialStore = mockStore(initialState); const defaultProps = { + addDangerToast: jest.fn(), onHide: () => ({}), actions: bindActionCreators(saveModalActions, arg => { if (typeof arg === 'function') { @@ -83,6 +83,7 @@ const queryStore = mockStore({ chart: {}, saveModal: { dashboards: [], + isVisible: true, }, explore: { datasource: { name: 'test', type: 'query' }, @@ -144,8 +145,7 @@ test('renders the right footer buttons when existing dashboard selected', () => test('renders the right footer buttons when new dashboard selected', () => { const wrapper = getWrapper(); wrapper.setState({ - saveToDashboardId: null, - newDashboardName: 'Test new dashboard', + dashboard: { label: 'Test new dashboard', value: 'Test new dashboard' }, }); const footerWrapper = shallow(wrapper.find(StyledModal).props().footer); const saveAndGoDash = footerWrapper @@ -186,18 +186,6 @@ test('sets action when overwriting slice', () => { expect(wrapperForOverwrite.state().action).toBe('overwrite'); }); -test('fetches dashboards on component mount', () => { - sinon.spy(defaultProps.actions, 'fetchDashboards'); - mount( - - - , - ); - expect(defaultProps.actions.fetchDashboards.calledOnce).toBe(true); - - defaultProps.actions.fetchDashboards.restore(); -}); - test('updates slice name and selected dashboard', () => { const wrapper = getWrapper(); const dashboardId = mockEvent.value; @@ -205,8 +193,8 @@ test('updates slice name and selected dashboard', () => { wrapper.instance().onSliceNameChange(mockEvent); expect(wrapper.state().newSliceName).toBe(mockEvent.target.value); - wrapper.instance().onDashboardSelectChange(dashboardId); - expect(wrapper.state().saveToDashboardId).toBe(dashboardId); + wrapper.instance().onDashboardChange({ value: dashboardId }); + expect(wrapper.state().dashboard.value).toBe(dashboardId); }); test('removes alert', () => { diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 9e63f10b6148..849baa1417b0 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -19,17 +19,18 @@ /* eslint camelcase: 0 */ import React from 'react'; import { Dispatch } from 'redux'; -import { SelectValue } from 'antd/lib/select'; +import { isFeatureEnabled } from 'src/featureFlags'; +import rison from 'rison'; import { connect } from 'react-redux'; import { withRouter, RouteComponentProps } from 'react-router-dom'; import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls'; import { css, DatasourceType, - ensureIsArray, FeatureFlag, isDefined, styled, + SupersetClient, t, } from '@superset-ui/core'; import { Input } from 'src/components/Input'; @@ -38,11 +39,10 @@ import Alert from 'src/components/Alert'; import Modal from 'src/components/Modal'; import { Radio } from 'src/components/Radio'; import Button from 'src/components/Button'; -import { Select } from 'src/components'; +import { AsyncSelect } from 'src/components'; import Loading from 'src/components/Loading'; import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; import { SaveActionType } from 'src/explore/types'; -import { isFeatureEnabled } from 'src/featureFlags'; // Session storage key for recent dashboard const SK_DASHBOARD_ID = 'save_chart_recent_dashboard'; @@ -52,7 +52,6 @@ interface SaveModalProps extends RouteComponentProps { actions: Record; form_data?: Record; userId: number; - dashboards: Array; alert?: string; sliceName?: string; slice?: Record; @@ -63,15 +62,14 @@ interface SaveModalProps extends RouteComponentProps { } type SaveModalState = { - saveToDashboardId: number | string | null; newSliceName?: string; - newDashboardName?: string; datasetName: string; alert: string | null; action: SaveActionType; isLoading: boolean; saveStatus?: string | null; vizType?: string; + dashboard?: { label: string; value: string | number }; }; export const StyledModal = styled(Modal)` @@ -89,15 +87,15 @@ class SaveModal extends React.Component { constructor(props: SaveModalProps) { super(props); this.state = { - saveToDashboardId: null, newSliceName: props.sliceName, datasetName: props.datasource?.name, alert: null, action: this.canOverwriteSlice() ? 'overwrite' : 'saveas', isLoading: false, vizType: props.form_data?.viz_type, + dashboard: undefined, }; - this.onDashboardSelectChange = this.onDashboardSelectChange.bind(this); + this.onDashboardChange = this.onDashboardChange.bind(this); this.onSliceNameChange = this.onSliceNameChange.bind(this); this.changeAction = this.changeAction.bind(this); this.saveOrOverwrite = this.saveOrOverwrite.bind(this); @@ -107,7 +105,8 @@ class SaveModal extends React.Component { } isNewDashboard(): boolean { - return !!(!this.state.saveToDashboardId && this.state.newDashboardName); + const { dashboard } = this.state; + return typeof dashboard?.value === 'string'; } canOverwriteSlice(): boolean { @@ -117,30 +116,26 @@ class SaveModal extends React.Component { ); } - componentDidMount() { - this.props.actions.fetchDashboards(this.props.userId).then(() => { - if (ensureIsArray(this.props.dashboards).length === 0) { - return; - } - const dashboardIds = this.props.dashboards?.map( - dashboard => dashboard.value, - ); + async componentDidMount() { + let { dashboardId } = this.props; + if (!dashboardId) { const lastDashboard = sessionStorage.getItem(SK_DASHBOARD_ID); - let recentDashboard = lastDashboard && parseInt(lastDashboard, 10); - - if (this.props.dashboardId) { - recentDashboard = this.props.dashboardId; - } - - if ( - recentDashboard !== null && - dashboardIds.indexOf(recentDashboard) !== -1 - ) { - this.setState({ - saveToDashboardId: recentDashboard, - }); + dashboardId = lastDashboard && parseInt(lastDashboard, 10); + } + if (dashboardId) { + try { + const result = await this.loadDashboard(dashboardId); + if (result) { + this.setState({ + dashboard: { label: result.dashboard_title, value: result.id }, + }); + } + } catch (error) { + this.props.actions.addDangerToast( + t('An error occurred while loading dashboard information.'), + ); } - }); + } } handleDatasetNameChange = (e: React.FormEvent) => { @@ -152,11 +147,8 @@ class SaveModal extends React.Component { this.setState({ newSliceName: event.target.value }); } - onDashboardSelectChange(selected: SelectValue) { - const newDashboardName = selected ? String(selected) : undefined; - const saveToDashboardId = - selected && typeof selected === 'number' ? selected : null; - this.setState({ saveToDashboardId, newDashboardName }); + onDashboardChange(dashboard: { label: string; value: string | number }) { + this.setState({ dashboard }); } changeAction(action: SaveActionType) { @@ -206,19 +198,22 @@ class SaveModal extends React.Component { delete formData.url_params; let dashboard: DashboardGetResponse | null = null; - if (this.state.newDashboardName || this.state.saveToDashboardId) { - let saveToDashboardId = this.state.saveToDashboardId || null; - if (!this.state.saveToDashboardId) { + if (this.state.dashboard) { + let validId = this.state.dashboard.value; + if (this.isNewDashboard()) { const response = await this.props.actions.createDashboard( - this.state.newDashboardName, + this.state.dashboard.label, ); - saveToDashboardId = response.id; + validId = response.id; + } + + try { + dashboard = await this.loadDashboard(validId as number); + } catch (error) { + this.props.actions.saveSliceFailed(); + return; } - const response = await this.props.actions.getDashboard( - saveToDashboardId, - ); - dashboard = response.result; if (isDefined(dashboard) && isDefined(dashboard?.id)) { sliceDashboards = sliceDashboards.includes(dashboard.id) ? sliceDashboards @@ -240,7 +235,7 @@ class SaveModal extends React.Component { dashboard ? { title: dashboard.dashboard_title, - new: !this.state.saveToDashboardId, + new: this.isNewDashboard(), } : null, ); @@ -251,7 +246,7 @@ class SaveModal extends React.Component { dashboard ? { title: dashboard.dashboard_title, - new: !this.state.saveToDashboardId, + new: this.isNewDashboard(), } : null, ); @@ -284,94 +279,131 @@ class SaveModal extends React.Component { } } - renderSaveChartModal = () => { - const dashboardSelectValue = - this.state.saveToDashboardId || this.state.newDashboardName; + loadDashboard = async (id: number) => { + const response = await SupersetClient.get({ + endpoint: `/api/v1/dashboard/${id}`, + }); + return response.json.result; + }; - return ( -
- {(this.state.alert || this.props.alert) && ( - { + const queryParams = rison.encode({ + columns: ['id', 'dashboard_title'], + filters: [ + { + col: 'dashboard_title', + opr: 'ct', + value: search, + }, + { + col: 'owners', + opr: 'rel_m_m', + value: this.props.userId, + }, + ], + page, + page_size: pageSize, + order_column: 'dashboard_title', + }); + + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/dashboard/?q=${queryParams}`, + }); + const { result, count } = json; + return { + data: result.map( + (dashboard: { id: number; dashboard_title: string }) => ({ + value: dashboard.id, + label: dashboard.dashboard_title, + }), + ), + totalCount: count, + }; + }; + + renderSaveChartModal = () => ( + + {(this.state.alert || this.props.alert) && ( + + )} + + this.changeAction('overwrite')} + data-test="save-overwrite-radio" + > + {t('Save (Overwrite)')} + + this.changeAction('saveas')} + > + {t('Save as...')} + + +
+ + + + {this.props.datasource?.type === 'query' && ( + + - )} - - this.changeAction('overwrite')} - data-test="save-overwrite-radio" - > - {t('Save (Overwrite)')} - - this.changeAction('saveas')} - > - {t('Save as...')} - - -
- - {this.props.datasource?.type === 'query' && ( - - - - - )} - {!( - isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) && - this.state.vizType === 'filter_box' - ) && ( - - .*?)' expecting (?P.*?)\)" +) +TABLE_DOES_NOT_EXIST_REGEX = re.compile( + r"The referenced table or view '(?P.*?)' does not exist" +) +COLUMN_DOES_NOT_EXIST_REGEX = re.compile( + r"The reference to column '(?P.*?)' is not valid" +) + + +# Custom datatype conversion functions + + +def _to_hex(data: bytes) -> str: + """ + Converts the bytes object into a string of hexadecimal digits. + + :param data: the bytes object + :returns: string of hexadecimal digits representing the bytes + """ + return data.hex() + + +def _polygon_to_json(polygon: Any) -> str: + """ + Converts the _STPolygon object into its JSON representation. + + :param data: the polygon object + :returns: JSON representation of the polygon + """ + json_value = f"{str([[p.long, p.lat] for p in polygon.exterior])}" + if polygon.holes: + for hole in polygon.holes: + json_value += f", {str([[p.long, p.lat] for p in hole])}" + json_value = f"[{json_value}]" + return json_value + + +def _linestring_to_json(linestring: Any) -> str: + """ + Converts the _STLinestring object into its JSON representation. + + :param data: the linestring object + :returns: JSON representation of the linestring + """ + return f"{str([[p.long, p.lat] for p in linestring.points])}" + + +def _point_to_comma_delimited(point: Any) -> str: + """ + Returns the x and y coordinates as a comma delimited string. + + :param data: the point object + :returns: the x and y coordinates as a comma delimited string + """ + return f"{point.long}, {point.lat}" + + +# Sanitization function for column values +SanitizeFunc = Callable[[Any], Any] + +# Represents a pair of a column index and the sanitization function +# to apply to its values. +PlacedSanitizeFunc = NamedTuple( + "PlacedSanitizeFunc", + [ + ("column_index", int), + ("sanitize_func", SanitizeFunc), + ], +) + +# This map contains functions used to sanitize values for column types +# that cannot be processed natively by Superset. +# +# Superset serializes temporal objects using a custom serializer +# defined in superset/utils/core.py (#json_int_dttm_ser(...)). Other +# are serialized by the default JSON encoder. +# +# Need to try-catch here because pyocient may not be installed +try: + from pyocient import TypeCodes + + _sanitized_ocient_type_codes: Dict[int, SanitizeFunc] = { + TypeCodes.BINARY: _to_hex, + TypeCodes.ST_POINT: _point_to_comma_delimited, + TypeCodes.IP: str, + TypeCodes.IPV4: str, + TypeCodes.ST_LINESTRING: _linestring_to_json, + TypeCodes.ST_POLYGON: _polygon_to_json, + } +except ImportError as e: + _sanitized_ocient_type_codes = {} + + +def _find_columns_to_sanitize(cursor: Any) -> List[PlacedSanitizeFunc]: + """ + Cleans the column value for consumption by Superset. + + :param cursor: the result set cursor + :returns: the list of tuples consisting of the column index and sanitization function + """ + return [ + PlacedSanitizeFunc(i, _sanitized_ocient_type_codes[cursor.description[i][1]]) + for i in range(len(cursor.description)) + if cursor.description[i][1] in _sanitized_ocient_type_codes + ] + + +class OcientEngineSpec(BaseEngineSpec): + engine = "ocient" + engine_name = "Ocient" + # limit_method = LimitMethod.WRAP_SQL + force_column_alias_quotes = True + max_column_name_length = 30 + + allows_cte_in_subquery = False + # Ocient does not support cte names starting with underscores + cte_alias = "cte__" + # Store mapping of superset Query id -> Ocient ID + # These are inserted into the cache when executing the query + # They are then removed, either upon cancellation or query completion + query_id_mapping: Dict[str, str] = dict() + query_id_mapping_lock = threading.Lock() + + custom_errors: Dict[Pattern[str], Tuple[str, SupersetErrorType, Dict[str, Any]]] = { + CONNECTION_INVALID_USERNAME_REGEX: ( + __('The username "%(username)s" does not exist.'), + SupersetErrorType.CONNECTION_INVALID_USERNAME_ERROR, + {}, + ), + CONNECTION_INVALID_PASSWORD_REGEX: ( + __( + "The user/password combination is not valid" + " (Incorrect password for user)." + ), + SupersetErrorType.CONNECTION_INVALID_PASSWORD_ERROR, + {}, + ), + CONNECTION_UNKNOWN_DATABASE_REGEX: ( + __('Could not connect to database: "%(database)s"'), + SupersetErrorType.CONNECTION_UNKNOWN_DATABASE_ERROR, + {}, + ), + CONNECTION_INVALID_HOSTNAME_REGEX: ( + __('Could not resolve hostname: "%(host)s".'), + SupersetErrorType.CONNECTION_INVALID_HOSTNAME_ERROR, + {}, + ), + CONNECTION_INVALID_PORT_ERROR: ( + __("Port out of range 0-65535"), + SupersetErrorType.CONNECTION_INVALID_PORT_ERROR, + {}, + ), + INVALID_CONNECTION_STRING_REGEX: ( + __( + "Invalid Connection String: Expecting String of" + " the form 'ocient://user:pass@host:port/database'." + ), + SupersetErrorType.GENERIC_DB_ENGINE_ERROR, + {}, + ), + SYNTAX_ERROR_REGEX: ( + __('Syntax Error: %(qualifier)s input "%(input)s" expecting "%(expected)s'), + SupersetErrorType.SYNTAX_ERROR, + {}, + ), + TABLE_DOES_NOT_EXIST_REGEX: ( + __('Table or View "%(table)s" does not exist.'), + SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR, + {}, + ), + COLUMN_DOES_NOT_EXIST_REGEX: ( + __('Invalid reference to column: "%(column)s"'), + SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR, + {}, + ), + } + _time_grain_expressions = { + None: "{col}", + "PT1S": "ROUND({col}, 'SECOND')", + "PT1M": "ROUND({col}, 'MINUTE')", + "PT1H": "ROUND({col}, 'HOUR')", + "P1D": "ROUND({col}, 'DAY')", + "P1W": "ROUND({col}, 'WEEK')", + "P1M": "ROUND({col}, 'MONTH')", + "P0.25Y": "ROUND({col}, 'QUARTER')", + "P1Y": "ROUND({col}, 'YEAR')", + } + + @classmethod + def get_table_names( + cls, database: Database, inspector: Inspector, schema: Optional[str] + ) -> Set[str]: + return inspector.get_table_names(schema) + + @classmethod + def fetch_data( + cls, cursor: Any, limit: Optional[int] = None + ) -> List[Tuple[Any, ...]]: + try: + rows: List[Tuple[Any, ...]] = super(OcientEngineSpec, cls).fetch_data( + cursor, limit + ) + except Exception as exception: + with OcientEngineSpec.query_id_mapping_lock: + del OcientEngineSpec.query_id_mapping[ + getattr(cursor, "superset_query_id") + ] + raise exception + + # TODO: Unsure if we need to verify that we are receiving rows: + if len(rows) > 0 and type(rows[0]).__name__ == "Row": + # Peek at the schema to determine which column values, if any, + # require sanitization. + columns_to_sanitize: List[PlacedSanitizeFunc] = _find_columns_to_sanitize( + cursor + ) + + if columns_to_sanitize: + # At least 1 column has to be sanitized. + + def identity(x: Any) -> Any: + return x + + # Use the identity function if the column type doesn't need to be + # sanitized. + sanitization_functions: List[SanitizeFunc] = [ + identity for _ in range(len(cursor.description)) + ] + for info in columns_to_sanitize: + sanitization_functions[info.column_index] = info.sanitize_func + + # pyocient returns a list of NamedTuple objects which represent a + # single row. We have to do this copy because that data type is + # NamedTuple's are immutable. + rows = [ + tuple( + sanitize_func(val) + for sanitize_func, val in zip(sanitization_functions, row) + ) + for row in rows + ] + return rows + + @classmethod + def epoch_to_dttm(cls) -> str: + return "DATEADD(S, {col}, '1970-01-01')" + + @classmethod + def epoch_ms_to_dttm(cls) -> str: + return "DATEADD(MS, {col}, '1970-01-01')" + + @classmethod + def get_cancel_query_id(cls, cursor: Any, query: Query) -> Optional[str]: + # Return a Non-None value + # If None is returned, Superset will not call cancel_query + return "DUMMY_VALUE" + + @classmethod + def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None: + with OcientEngineSpec.query_id_mapping_lock: + OcientEngineSpec.query_id_mapping[query.id] = cursor.query_id + + # Add the query id to the cursor + setattr(cursor, "superset_query_id", query.id) + return super().handle_cursor(cursor, query, session) + + @classmethod + def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool: + with OcientEngineSpec.query_id_mapping_lock: + if query.id in OcientEngineSpec.query_id_mapping: + cursor.execute(f"CANCEL {OcientEngineSpec.query_id_mapping[query.id]}") + # Query has been cancelled, so we can safely remove the cursor from + # the cache + del OcientEngineSpec.query_id_mapping[query.id] + return True + # If the query is not in the cache, it must have either been cancelled + # elsewhere or completed + return False diff --git a/tests/unit_tests/db_engine_specs/test_ocient.py b/tests/unit_tests/db_engine_specs/test_ocient.py new file mode 100644 index 000000000000..c5578fa93727 --- /dev/null +++ b/tests/unit_tests/db_engine_specs/test_ocient.py @@ -0,0 +1,215 @@ +# 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. + +# pylint: disable=import-outside-toplevel + +from datetime import datetime +from typing import Dict, List, Tuple + +import pytest + +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType + +# (msg,expected) +MARSHALED_OCIENT_ERRORS: List[Tuple[str, SupersetError]] = [ + ( + "The referenced user does not exist (User 'mj' not found)", + SupersetError( + message='The username "mj" does not exist.', + error_type=SupersetErrorType.CONNECTION_INVALID_USERNAME_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1012, + "message": "Issue 1012 - The username provided when connecting to a database is not valid.", + } + ], + }, + ), + ), + ( + "The userid/password combination was not valid (Incorrect password for user)", + SupersetError( + message="The user/password combination is not valid (Incorrect password for user).", + error_type=SupersetErrorType.CONNECTION_INVALID_PASSWORD_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1013, + "message": "Issue 1013 - The password provided when connecting to a database is not valid.", + } + ], + }, + ), + ), + ( + "No database named 'bulls' exists", + SupersetError( + message='Could not connect to database: "bulls"', + error_type=SupersetErrorType.CONNECTION_UNKNOWN_DATABASE_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1015, + "message": "Issue 1015 - Either the database is spelled incorrectly or does not exist.", + } + ], + }, + ), + ), + ( + "Unable to connect to unitedcenter.com:4050", + SupersetError( + message='Could not resolve hostname: "unitedcenter.com".', + error_type=SupersetErrorType.CONNECTION_INVALID_HOSTNAME_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1007, + "message": "Issue 1007 - The hostname provided can't be resolved.", + } + ], + }, + ), + ), + ( + "Port out of range 0-65535", + SupersetError( + message="Port out of range 0-65535", + error_type=SupersetErrorType.CONNECTION_INVALID_PORT_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1034, + "message": "Issue 1034 - The port number is invalid.", + } + ], + }, + ), + ), + ( + "An invalid connection string attribute was specified (failed to decrypt cipher text)", + SupersetError( + message="Invalid Connection String: Expecting String of the form 'ocient://user:pass@host:port/database'.", + error_type=SupersetErrorType.GENERIC_DB_ENGINE_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1002, + "message": "Issue 1002 - The database returned an unexpected error.", + } + ], + }, + ), + ), + ( + "There is a syntax error in your statement (extraneous input 'foo bar baz' expecting {, 'trace', 'using'})", + SupersetError( + message="Syntax Error: extraneous input \"foo bar baz\" expecting \"{, 'trace', 'using'}", + error_type=SupersetErrorType.SYNTAX_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1030, + "message": "Issue 1030 - The query has a syntax error.", + } + ], + }, + ), + ), + ( + "There is a syntax error in your statement (mismatched input 'to' expecting {, 'trace', 'using'})", + SupersetError( + message="Syntax Error: mismatched input \"to\" expecting \"{, 'trace', 'using'}", + error_type=SupersetErrorType.SYNTAX_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1030, + "message": "Issue 1030 - The query has a syntax error.", + } + ], + }, + ), + ), + ( + "The referenced table or view 'goats' does not exist", + SupersetError( + message='Table or View "goats" does not exist.', + error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1003, + "message": "Issue 1003 - There is a syntax error in the SQL query. Perhaps there was a misspelling or a typo.", + }, + { + "code": 1005, + "message": "Issue 1005 - The table was deleted or renamed in the database.", + }, + ], + }, + ), + ), + ( + "The reference to column 'goats' is not valid", + SupersetError( + message='Invalid reference to column: "goats"', + error_type=SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR, + level=ErrorLevel.ERROR, + extra={ + "engine_name": "Ocient", + "issue_codes": [ + { + "code": 1003, + "message": "Issue 1003 - There is a syntax error in the SQL query. Perhaps there was a misspelling or a typo.", + }, + { + "code": 1004, + "message": "Issue 1004 - The column was deleted or renamed in the database.", + }, + ], + }, + ), + ), +] + + +@pytest.mark.parametrize("msg,expected", MARSHALED_OCIENT_ERRORS) +def test_connection_errors(msg: str, expected: SupersetError) -> None: + from superset.db_engine_specs.ocient import OcientEngineSpec + + result = OcientEngineSpec.extract_errors(Exception(msg)) + assert result == [expected] From 6ae5388dcf0205e89d4abcc5cefcb644e8c7cdbd Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Sun, 23 Apr 2023 15:44:21 +0100 Subject: [PATCH 31/47] fix: allow db driver distinction on enforced URI params (#23769) --- superset/db_engine_specs/base.py | 18 +++++++----- superset/db_engine_specs/mysql.py | 10 +++++-- tests/integration_tests/model_tests.py | 12 +++++++- .../unit_tests/db_engine_specs/test_mysql.py | 28 +++++++++++++++++++ 4 files changed, 58 insertions(+), 10 deletions(-) diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index f1dda401afc1..a21c0b41006a 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -355,10 +355,11 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods # This set will give the keywords for data limit statements # to consider for the engines with TOP SQL parsing top_keywords: Set[str] = {"TOP"} - # A set of disallowed connection query parameters - disallow_uri_query_params: Set[str] = set() + # A set of disallowed connection query parameters by driver name + disallow_uri_query_params: Dict[str, Set[str]] = {} # A Dict of query parameters that will always be used on every connection - enforce_uri_query_params: Dict[str, Any] = {} + # by driver name + enforce_uri_query_params: Dict[str, Dict[str, Any]] = {} force_column_alias_quotes = False arraysize = 0 @@ -1099,7 +1100,10 @@ def adjust_engine_params( # pylint: disable=unused-argument This is important because DB engine specs can be installed from 3rd party packages. """ - return uri, {**connect_args, **cls.enforce_uri_query_params} + return uri, { + **connect_args, + **cls.enforce_uri_query_params.get(uri.get_driver_name(), {}), + } @classmethod def patch(cls) -> None: @@ -1853,9 +1857,9 @@ def validate_database_uri(cls, sqlalchemy_uri: URL) -> None: :param sqlalchemy_uri: """ - if existing_disallowed := cls.disallow_uri_query_params.intersection( - sqlalchemy_uri.query - ): + if existing_disallowed := cls.disallow_uri_query_params.get( + sqlalchemy_uri.get_driver_name(), set() + ).intersection(sqlalchemy_uri.query): raise ValueError(f"Forbidden query parameter(s): {existing_disallowed}") diff --git a/superset/db_engine_specs/mysql.py b/superset/db_engine_specs/mysql.py index 07d2aea36286..9c5bd0034a5b 100644 --- a/superset/db_engine_specs/mysql.py +++ b/superset/db_engine_specs/mysql.py @@ -175,8 +175,14 @@ class MySQLEngineSpec(BaseEngineSpec, BasicParametersMixin): {}, ), } - disallow_uri_query_params = {"local_infile"} - enforce_uri_query_params = {"local_infile": 0} + disallow_uri_query_params = { + "mysqldb": {"local_infile"}, + "mysqlconnector": {"allow_local_infile"}, + } + enforce_uri_query_params = { + "mysqldb": {"local_infile": 0}, + "mysqlconnector": {"allow_local_infile": 0}, + } @classmethod def convert_dttm( diff --git a/tests/integration_tests/model_tests.py b/tests/integration_tests/model_tests.py index 35dbcc0a6bb3..d5684b1b6210 100644 --- a/tests/integration_tests/model_tests.py +++ b/tests/integration_tests/model_tests.py @@ -194,7 +194,7 @@ def test_impersonate_user_presto(self, mocked_create_engine): @mock.patch("superset.models.core.create_engine") def test_adjust_engine_params_mysql(self, mocked_create_engine): model = Database( - database_name="test_database", + database_name="test_database1", sqlalchemy_uri="mysql://user:password@localhost", ) model._get_sqla_engine() @@ -203,6 +203,16 @@ def test_adjust_engine_params_mysql(self, mocked_create_engine): assert str(call_args[0][0]) == "mysql://user:password@localhost" assert call_args[1]["connect_args"]["local_infile"] == 0 + model = Database( + database_name="test_database2", + sqlalchemy_uri="mysql+mysqlconnector://user:password@localhost", + ) + model._get_sqla_engine() + call_args = mocked_create_engine.call_args + + assert str(call_args[0][0]) == "mysql+mysqlconnector://user:password@localhost" + assert call_args[1]["connect_args"]["allow_local_infile"] == 0 + @mock.patch("superset.models.core.create_engine") def test_impersonate_user_trino(self, mocked_create_engine): principal_user = security_manager.find_user(username="gamma") diff --git a/tests/unit_tests/db_engine_specs/test_mysql.py b/tests/unit_tests/db_engine_specs/test_mysql.py index 31e01ace5855..07ce6838fc20 100644 --- a/tests/unit_tests/db_engine_specs/test_mysql.py +++ b/tests/unit_tests/db_engine_specs/test_mysql.py @@ -104,8 +104,11 @@ def test_convert_dttm( "sqlalchemy_uri,error", [ ("mysql://user:password@host/db1?local_infile=1", True), + ("mysql+mysqlconnector://user:password@host/db1?allow_local_infile=1", True), ("mysql://user:password@host/db1?local_infile=0", True), + ("mysql+mysqlconnector://user:password@host/db1?allow_local_infile=0", True), ("mysql://user:password@host/db1", False), + ("mysql+mysqlconnector://user:password@host/db1", False), ], ) def test_validate_database_uri(sqlalchemy_uri: str, error: bool) -> None: @@ -123,18 +126,43 @@ def test_validate_database_uri(sqlalchemy_uri: str, error: bool) -> None: "sqlalchemy_uri,connect_args,returns", [ ("mysql://user:password@host/db1", {"local_infile": 1}, {"local_infile": 0}), + ( + "mysql+mysqlconnector://user:password@host/db1", + {"allow_local_infile": 1}, + {"allow_local_infile": 0}, + ), ("mysql://user:password@host/db1", {"local_infile": -1}, {"local_infile": 0}), + ( + "mysql+mysqlconnector://user:password@host/db1", + {"allow_local_infile": -1}, + {"allow_local_infile": 0}, + ), ("mysql://user:password@host/db1", {"local_infile": 0}, {"local_infile": 0}), + ( + "mysql+mysqlconnector://user:password@host/db1", + {"allow_local_infile": 0}, + {"allow_local_infile": 0}, + ), ( "mysql://user:password@host/db1", {"param1": "some_value"}, {"local_infile": 0, "param1": "some_value"}, ), + ( + "mysql+mysqlconnector://user:password@host/db1", + {"param1": "some_value"}, + {"allow_local_infile": 0, "param1": "some_value"}, + ), ( "mysql://user:password@host/db1", {"local_infile": 1, "param1": "some_value"}, {"local_infile": 0, "param1": "some_value"}, ), + ( + "mysql+mysqlconnector://user:password@host/db1", + {"allow_local_infile": 1, "param1": "some_value"}, + {"allow_local_infile": 0, "param1": "some_value"}, + ), ], ) def test_adjust_engine_params( From d4c0ae34f4e23d1172d2ae3335f73873b0b37c1e Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Mon, 24 Apr 2023 17:42:56 +0200 Subject: [PATCH 32/47] fix: Context menu crashing when there is no dimension in Echarts Series charts (#23797) --- .../plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx index 516255de0cbd..202d62756965 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx @@ -205,7 +205,7 @@ export default function EchartsTimeseries({ const pointerEvent = eventParams.event.event; const values = [ ...(eventParams.name ? [eventParams.name] : []), - ...labelMap[seriesName], + ...(labelMap[seriesName] ?? []), ]; if (data && xAxis.type === AxisType.time) { drillToDetailFilters.push({ From c536d92ade3b60e7cac75f85bcc2d6bad7e8c884 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Mon, 24 Apr 2023 13:04:47 -0300 Subject: [PATCH 33/47] fix: Docker ephemeral env (#23786) --- .github/workflows/docker-ephemeral-env.yml | 124 ++++++++++----------- 1 file changed, 62 insertions(+), 62 deletions(-) diff --git a/.github/workflows/docker-ephemeral-env.yml b/.github/workflows/docker-ephemeral-env.yml index 544c1c8b1f42..e3ca7473d2aa 100644 --- a/.github/workflows/docker-ephemeral-env.yml +++ b/.github/workflows/docker-ephemeral-env.yml @@ -1,4 +1,4 @@ -name: Push ephmereral env image +name: Push ephemeral env image on: workflow_run: @@ -17,14 +17,14 @@ jobs: id: check shell: bash run: | - aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} - aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} - if [ -n "${{ (secrets.AWS_ACCESS_KEY_ID != '' && - secrets.AWS_ACCESS_KEY_ID != '' && - secrets.AWS_SECRET_ACCESS_KEY != '' && - secrets.AWS_SECRET_ACCESS_KEY != '') || '' }}" ]; then - echo "has-secrets=1" >> "$GITHUB_OUTPUT" - fi + aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} + aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} + if [ -n "${{ (secrets.AWS_ACCESS_KEY_ID != '' && + secrets.AWS_ACCESS_KEY_ID != '' && + secrets.AWS_SECRET_ACCESS_KEY != '' && + secrets.AWS_SECRET_ACCESS_KEY != '') || '' }}" ]; then + echo "has-secrets=1" >> "$GITHUB_OUTPUT" + fi docker_ephemeral_env: needs: config @@ -33,66 +33,66 @@ jobs: runs-on: ubuntu-latest steps: - - name: 'Download artifact' - uses: actions/github-script@v3.1.0 - with: - script: | - const artifacts = await github.actions.listWorkflowRunArtifacts({ - owner: context.repo.owner, - repo: context.repo.repo, - run_id: ${{ github.event.workflow_run.id }}, - }); + - name: "Download artifact" + uses: actions/github-script@v3.1.0 + with: + script: | + const artifacts = await github.actions.listWorkflowRunArtifacts({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: ${{ github.event.workflow_run.id }}, + }); - core.info('*** artifacts') - core.info(JSON.stringify(artifacts)) + core.info('*** artifacts') + core.info(JSON.stringify(artifacts)) - const matchArtifact = artifacts.data.artifacts.filter((artifact) => { - return artifact.name == "build" - })[0]; - if(!matchArtifact) return core.setFailed("Build artifacts not found") + const matchArtifact = artifacts.data.artifacts.filter((artifact) => { + return artifact.name == "build" + })[0]; + if(!matchArtifact) return core.setFailed("Build artifacts not found") - const download = await github.actions.downloadArtifact({ - owner: context.repo.owner, - repo: context.repo.repo, - artifact_id: matchArtifact.id, - archive_format: 'zip', - }); - var fs = require('fs'); - fs.writeFileSync('${{github.workspace}}/build.zip', Buffer.from(download.data)); + const download = await github.actions.downloadArtifact({ + owner: context.repo.owner, + repo: context.repo.repo, + artifact_id: matchArtifact.id, + archive_format: 'zip', + }); + var fs = require('fs'); + fs.writeFileSync('${{github.workspace}}/build.zip', Buffer.from(download.data)); - - run: unzip build.zip + - run: unzip build.zip - - name: Display downloaded files (debug) - run: ls -la + - name: Display downloaded files (debug) + run: ls -la - - name: Get SHA - id: get-sha - run: echo "::set-output name=sha::$(cat ./SHA)" + - name: Get SHA + id: get-sha + run: echo "::set-output name=sha::$(cat ./SHA)" - - name: Get PR - id: get-pr - run: echo "::set-output name=num::$(cat ./PR-NUM)" + - name: Get PR + id: get-pr + run: echo "::set-output name=num::$(cat ./PR-NUM)" - - name: Configure AWS credentials - uses: aws-actions/configure-aws-credentials@v1 - with: - aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} - aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} - aws-region: us-west-2 + - name: Configure AWS credentials + uses: aws-actions/configure-aws-credentials@v1 + with: + aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} + aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} + aws-region: us-west-2 - - name: Login to Amazon ECR - id: login-ecr - uses: aws-actions/amazon-ecr-login@v1 + - name: Login to Amazon ECR + id: login-ecr + uses: aws-actions/amazon-ecr-login@v1 - - name: Load, tag and push image to ECR - id: push-image - env: - ECR_REGISTRY: ${{ steps.login-ecr.outputs.registry }} - ECR_REPOSITORY: superset-ci - SHA: ${{ steps.get-sha.outputs.sha }} - IMAGE_TAG: pr-${{ steps.get-pr.outputs.num }} - run: | - docker load < $SHA.tar.gz - docker tag $SHA $ECR_REGISTRY/$ECR_REPOSITORY:$IMAGE_TAG - docker tag $SHA $ECR_REGISTRY/$ECR_REPOSITORY:$SHA - docker push -a $ECR_REGISTRY/$ECR_REPOSITORY + - name: Load, tag and push image to ECR + id: push-image + env: + ECR_REGISTRY: ${{ steps.login-ecr.outputs.registry }} + ECR_REPOSITORY: superset-ci + SHA: ${{ steps.get-sha.outputs.sha }} + IMAGE_TAG: pr-${{ steps.get-pr.outputs.num }} + run: | + docker load < $SHA.tar.gz + docker tag $SHA $ECR_REGISTRY/$ECR_REPOSITORY:$IMAGE_TAG + docker tag $SHA $ECR_REGISTRY/$ECR_REPOSITORY:$SHA + docker push -a $ECR_REGISTRY/$ECR_REPOSITORY From f7810b602025512166266c55c38587aa87b26d64 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Mon, 24 Apr 2023 13:10:58 -0300 Subject: [PATCH 34/47] feat(revert): Re-introduces the RLS page (#23777) --- UPDATING.md | 1 + .../rls/RowLevelSecurityModal.test.tsx | 295 +++++++++++ .../features/rls/RowLevelSecurityModal.tsx | 479 ++++++++++++++++++ .../src/features/rls/constants.ts | 31 ++ superset-frontend/src/features/rls/types.ts | 51 ++ .../RowLevelSecurityList.test.tsx | 259 ++++++++++ .../src/pages/RowLevelSecurityList/index.tsx | 350 +++++++++++++ superset-frontend/src/views/routes.tsx | 11 + superset/config.py | 14 +- superset/connectors/sqla/views.py | 117 +---- superset/dao/base.py | 11 + superset/initialization/__init__.py | 22 +- superset/row_level_security/api.py | 349 +++++++++++++ .../row_level_security/commands/__init__.py | 16 + .../commands/bulk_delete.py | 52 ++ .../row_level_security/commands/create.py | 57 +++ .../row_level_security/commands/exceptions.py | 29 ++ .../row_level_security/commands/update.py | 63 +++ superset/row_level_security/dao.py | 23 + superset/row_level_security/schemas.py | 154 ++++++ .../templates/superset/models/rls/list.html | 96 ---- .../security/row_level_security_tests.py | 419 +++++++++++++-- 22 files changed, 2642 insertions(+), 257 deletions(-) create mode 100644 superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx create mode 100644 superset-frontend/src/features/rls/RowLevelSecurityModal.tsx create mode 100644 superset-frontend/src/features/rls/constants.ts create mode 100644 superset-frontend/src/features/rls/types.ts create mode 100644 superset-frontend/src/pages/RowLevelSecurityList/RowLevelSecurityList.test.tsx create mode 100644 superset-frontend/src/pages/RowLevelSecurityList/index.tsx create mode 100644 superset/row_level_security/api.py create mode 100644 superset/row_level_security/commands/__init__.py create mode 100644 superset/row_level_security/commands/bulk_delete.py create mode 100644 superset/row_level_security/commands/create.py create mode 100644 superset/row_level_security/commands/exceptions.py create mode 100644 superset/row_level_security/commands/update.py create mode 100644 superset/row_level_security/dao.py create mode 100644 superset/row_level_security/schemas.py delete mode 100644 superset/templates/superset/models/rls/list.html diff --git a/UPDATING.md b/UPDATING.md index 9afd8d353fd1..12e69b3738a5 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -53,6 +53,7 @@ assists people when migrating to a new version. - [22798](https://github.com/apache/superset/pull/22798): To make the welcome page more relevant in production environments, the last tab on the welcome page has been changed from to feature all charts/dashboards the user has access to (previously only examples were shown). To keep current behavior unchanged, add the following to your `superset_config.py`: `WELCOME_PAGE_LAST_TAB = "examples"` - [22328](https://github.com/apache/superset/pull/22328): For deployments that have enabled the "THUMBNAILS" feature flag, the function that calculates dashboard digests has been updated to consider additional properties to more accurately identify changes in the dashboard metadata. This change will invalidate all currently cached dashboard thumbnails. - [21765](https://github.com/apache/superset/pull/21765): For deployments that have enabled the "ALERT_REPORTS" feature flag, Gamma users will no longer have read and write access to Alerts & Reports by default. To give Gamma users the ability to schedule reports from the Dashboard and Explore view like before, create an additional role with "can read on ReportSchedule" and "can write on ReportSchedule" permissions. To further give Gamma users access to the "Alerts & Reports" menu and CRUD view, add "menu access on Manage" and "menu access on Alerts & Report" permissions to the role. +- [22325](https://github.com/apache/superset/pull/22325): "RLS_FORM_QUERY_REL_FIELDS" is replaced by "RLS_BASE_RELATED_FIELD_FILTERS" feature flag. Its value format stays same. ### Potential Downtime diff --git a/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx b/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx new file mode 100644 index 000000000000..6253c42c8275 --- /dev/null +++ b/superset-frontend/src/features/rls/RowLevelSecurityModal.test.tsx @@ -0,0 +1,295 @@ +/** + * 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 fetchMock from 'fetch-mock'; +import { render, screen, waitFor, within } from 'spec/helpers/testing-library'; +import { act } from 'react-dom/test-utils'; +import userEvent from '@testing-library/user-event'; +import RowLevelSecurityModal, { + RowLevelSecurityModalProps, +} from './RowLevelSecurityModal'; +import { FilterType } from './types'; + +const getRuleEndpoint = 'glob:*/api/v1/rowlevelsecurity/1'; +const getRelatedRolesEndpoint = + 'glob:*/api/v1/rowlevelsecurity/related/roles?q*'; +const getRelatedTablesEndpoint = + 'glob:*/api/v1/rowlevelsecurity/related/tables?q*'; +const postRuleEndpoint = 'glob:*/api/v1/rowlevelsecurity/*'; +const putRuleEndpoint = 'glob:*/api/v1/rowlevelsecurity/1'; + +const mockGetRuleResult = { + description_columns: {}, + id: 1, + label_columns: { + clause: 'Clause', + description: 'Description', + filter_type: 'Filter Type', + group_key: 'Group Key', + name: 'Name', + 'roles.id': 'Roles Id', + 'roles.name': 'Roles Name', + 'tables.id': 'Tables Id', + 'tables.table_name': 'Tables Table Name', + }, + result: { + clause: 'gender="girl"', + description: 'test rls rule with RTL', + filter_type: 'Base', + group_key: 'g1', + id: 1, + name: 'rls 1', + roles: [ + { + id: 1, + name: 'Admin', + }, + ], + tables: [ + { + id: 2, + table_name: 'birth_names', + }, + ], + }, + show_columns: [ + 'name', + 'description', + 'filter_type', + 'tables.id', + 'tables.table_name', + 'roles.id', + 'roles.name', + 'group_key', + 'clause', + ], + show_title: 'Show Row Level Security Filter', +}; + +const mockGetRolesResult = { + count: 3, + result: [ + { + extra: {}, + text: 'Admin', + value: 1, + }, + { + extra: {}, + text: 'Public', + value: 2, + }, + { + extra: {}, + text: 'Alpha', + value: 3, + }, + ], +}; + +const mockGetTablesResult = { + count: 3, + result: [ + { + extra: {}, + text: 'wb_health_population', + value: 1, + }, + { + extra: {}, + text: 'birth_names', + value: 2, + }, + { + extra: {}, + text: 'long_lat', + value: 3, + }, + ], +}; + +fetchMock.get(getRuleEndpoint, mockGetRuleResult); +fetchMock.get(getRelatedRolesEndpoint, mockGetRolesResult); +fetchMock.get(getRelatedTablesEndpoint, mockGetTablesResult); +fetchMock.post(postRuleEndpoint, {}); +fetchMock.put(putRuleEndpoint, {}); + +global.URL.createObjectURL = jest.fn(); + +const NOOP = () => {}; + +const addNewRuleDefaultProps: RowLevelSecurityModalProps = { + addDangerToast: NOOP, + addSuccessToast: NOOP, + show: true, + rule: null, + onHide: NOOP, +}; + +describe('Rule modal', () => { + async function renderAndWait(props: RowLevelSecurityModalProps) { + const mounted = act(async () => { + render(, { useRedux: true }); + }); + return mounted; + } + + it('Sets correct title for adding new rule', async () => { + await renderAndWait(addNewRuleDefaultProps); + const title = screen.getByText('Add Rule'); + expect(title).toBeInTheDocument(); + expect(fetchMock.calls(getRuleEndpoint)).toHaveLength(0); + expect(fetchMock.calls(getRelatedTablesEndpoint)).toHaveLength(0); + expect(fetchMock.calls(getRelatedRolesEndpoint)).toHaveLength(0); + }); + + it('Sets correct title for editing existing rule', async () => { + await renderAndWait({ + ...addNewRuleDefaultProps, + rule: { + id: 1, + name: 'test rule', + filter_type: FilterType.BASE, + tables: [{ key: 1, id: 1, value: 'birth_names' }], + roles: [], + }, + }); + const title = screen.getByText('Edit Rule'); + expect(title).toBeInTheDocument(); + expect(fetchMock.calls(getRuleEndpoint)).toHaveLength(1); + expect(fetchMock.calls(getRelatedTablesEndpoint)).toHaveLength(0); + expect(fetchMock.calls(getRelatedRolesEndpoint)).toHaveLength(0); + }); + + it('Fills correct values when editing rule', async () => { + await renderAndWait({ + ...addNewRuleDefaultProps, + rule: { + id: 1, + name: 'rls 1', + filter_type: FilterType.BASE, + }, + }); + + const name = await screen.findByTestId('rule-name-test'); + expect(name).toHaveDisplayValue('rls 1'); + userEvent.type(name, 'rls 2'); + expect(name).toHaveDisplayValue('rls 2'); + + const filterType = await screen.findByText('Base'); + expect(filterType).toBeInTheDocument(); + + const roles = await screen.findByText('Admin'); + expect(roles).toBeInTheDocument(); + + const tables = await screen.findByText('birth_names'); + expect(tables).toBeInTheDocument(); + + const groupKey = await screen.findByTestId('group-key-test'); + expect(groupKey).toHaveValue('g1'); + userEvent.clear(groupKey); + userEvent.type(groupKey, 'g2'); + expect(groupKey).toHaveValue('g2'); + + const clause = await screen.findByTestId('clause-test'); + expect(clause).toHaveValue('gender="girl"'); + userEvent.clear(clause); + userEvent.type(clause, 'gender="boy"'); + expect(clause).toHaveValue('gender="boy"'); + + const description = await screen.findByTestId('description-test'); + expect(description).toHaveValue('test rls rule with RTL'); + userEvent.clear(description); + userEvent.type(description, 'test description'); + expect(description).toHaveValue('test description'); + }); + + it('Does not allow to create rule without name, tables and clause', async () => { + await renderAndWait(addNewRuleDefaultProps); + + const addButton = screen.getByRole('button', { name: /add/i }); + expect(addButton).toBeDisabled(); + + const nameTextBox = screen.getByTestId('rule-name-test'); + userEvent.type(nameTextBox, 'name'); + + expect(addButton).toBeDisabled(); + + const getSelect = () => screen.getByRole('combobox', { name: 'Tables' }); + const getElementByClassName = (className: string) => + document.querySelector(className)! as HTMLElement; + + const findSelectOption = (text: string) => + waitFor(() => + within(getElementByClassName('.rc-virtual-list')).getByText(text), + ); + const open = () => waitFor(() => userEvent.click(getSelect())); + await open(); + userEvent.click(await findSelectOption('birth_names')); + expect(addButton).toBeDisabled(); + + const clause = await screen.findByTestId('clause-test'); + userEvent.type(clause, 'gender="girl"'); + + expect(addButton).toBeEnabled(); + }); + + it('Creates a new rule', async () => { + await renderAndWait(addNewRuleDefaultProps); + + const addButton = screen.getByRole('button', { name: /add/i }); + + const nameTextBox = screen.getByTestId('rule-name-test'); + userEvent.type(nameTextBox, 'name'); + + const getSelect = () => screen.getByRole('combobox', { name: 'Tables' }); + const getElementByClassName = (className: string) => + document.querySelector(className)! as HTMLElement; + + const findSelectOption = (text: string) => + waitFor(() => + within(getElementByClassName('.rc-virtual-list')).getByText(text), + ); + const open = () => waitFor(() => userEvent.click(getSelect())); + await open(); + userEvent.click(await findSelectOption('birth_names')); + + const clause = await screen.findByTestId('clause-test'); + userEvent.type(clause, 'gender="girl"'); + + await waitFor(() => userEvent.click(addButton)); + + expect(fetchMock.calls(postRuleEndpoint)).toHaveLength(1); + }); + + it('Updates existing rule', async () => { + await renderAndWait({ + ...addNewRuleDefaultProps, + rule: { + id: 1, + name: 'rls 1', + filter_type: FilterType.BASE, + }, + }); + + const addButton = screen.getByRole('button', { name: /save/i }); + await waitFor(() => userEvent.click(addButton)); + expect(fetchMock.calls(putRuleEndpoint)).toHaveLength(4); + }); +}); diff --git a/superset-frontend/src/features/rls/RowLevelSecurityModal.tsx b/superset-frontend/src/features/rls/RowLevelSecurityModal.tsx new file mode 100644 index 000000000000..1fc0b597dd31 --- /dev/null +++ b/superset-frontend/src/features/rls/RowLevelSecurityModal.tsx @@ -0,0 +1,479 @@ +/** + * 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 { + css, + styled, + SupersetClient, + SupersetTheme, + t, +} from '@superset-ui/core'; +import Modal from 'src/components/Modal'; +import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import Icons from 'src/components/Icons'; +import Select from 'src/components/Select/Select'; +import AsyncSelect from 'src/components/Select/AsyncSelect'; +import rison from 'rison'; +import { LabeledErrorBoundInput } from 'src/components/Form'; +import { noBottomMargin } from 'src/components/ReportModal/styles'; +import InfoTooltip from 'src/components/InfoTooltip'; +import { useSingleViewResource } from 'src/views/CRUD/hooks'; +import { FilterOptions } from './constants'; +import { FilterType, RLSObject, RoleObject, TableObject } from './types'; + +const StyledModal = styled(Modal)` + max-width: 1200px; + width: 100%; + .ant-modal-body { + overflow: initial; + } +`; +const StyledIcon = (theme: SupersetTheme) => css` + margin: auto ${theme.gridUnit * 2}px auto 0; + color: ${theme.colors.grayscale.base}; +`; + +const StyledSectionContainer = styled.div` + display: flex; + flex-direction: column; + padding: ${({ theme }) => + `${theme.gridUnit * 3}px ${theme.gridUnit * 4}px ${theme.gridUnit * 2}px`}; + + label { + font-size: ${({ theme }) => theme.typography.sizes.s}px; + color: ${({ theme }) => theme.colors.grayscale.light1}; + } +`; + +const StyledInputContainer = styled.div` + display: flex; + flex-direction: column; + margin: ${({ theme }) => theme.gridUnit}px; + + margin-bottom: ${({ theme }) => theme.gridUnit * 4}px; + + .input-container { + display: flex; + align-items: center; + + > div { + width: 100%; + } + + label { + display: flex; + margin-right: ${({ theme }) => theme.gridUnit * 2}px; + } + } + + input, + textarea { + flex: 1 1 auto; + } + + textarea { + height: 100px; + resize: none; + } + + .required { + margin-left: ${({ theme }) => theme.gridUnit / 2}px; + color: ${({ theme }) => theme.colors.error.base}; + } +`; + +export interface RowLevelSecurityModalProps { + rule: RLSObject | null; + addSuccessToast: (msg: string) => void; + addDangerToast: (msg: string) => void; + onAdd?: (alert?: any) => void; + onHide: () => void; + show: boolean; +} + +const DEAFULT_RULE = { + name: '', + filter_type: FilterType.REGULAR, + tables: [], + roles: [], + clause: '', + group_key: '', + description: '', +}; + +function RowLevelSecurityModal(props: RowLevelSecurityModalProps) { + const { rule, addDangerToast, addSuccessToast, onHide, show } = props; + + const [currentRule, setCurrentRule] = useState({ + ...DEAFULT_RULE, + }); + const [disableSave, setDisableSave] = useState(true); + + const isEditMode = rule !== null; + + // * hooks * + const { + state: { loading, resource, error: fetchError }, + fetchResource, + createResource, + updateResource, + clearError, + } = useSingleViewResource( + `rowlevelsecurity`, + t('rowlevelsecurity'), + addDangerToast, + ); + + // initialize + useEffect(() => { + if (!isEditMode) { + setCurrentRule({ ...DEAFULT_RULE }); + } else if (rule?.id !== null && !loading && !fetchError) { + fetchResource(rule.id as number); + } + }, [rule]); + + useEffect(() => { + if (resource) { + setCurrentRule({ ...resource, id: rule?.id }); + const selectedTableAndRoles = getSelectedData(); + updateRuleState('tables', selectedTableAndRoles?.tables || []); + updateRuleState('roles', selectedTableAndRoles?.roles || []); + } + }, [resource]); + + // find selected tables and roles + const getSelectedData = useCallback(() => { + if (!resource) { + return null; + } + const tables: TableObject[] = []; + const roles: RoleObject[] = []; + + resource.tables?.forEach(selectedTable => { + tables.push({ + key: selectedTable.id, + label: selectedTable.schema + ? `${selectedTable.schema}.${selectedTable.table_name}` + : selectedTable.table_name, + value: selectedTable.id, + }); + }); + + resource.roles?.forEach(selectedRole => { + roles.push({ + key: selectedRole.id, + label: selectedRole.name, + value: selectedRole.id, + }); + }); + + return { tables, roles }; + }, [resource?.tables, resource?.roles]); + + // validate + const currentRuleSafe = currentRule || {}; + useEffect(() => { + validate(); + }, [currentRuleSafe.name, currentRuleSafe.clause, currentRuleSafe?.tables]); + + // * event handlers * + type SelectValue = { + value: string; + label: string; + }; + + const updateRuleState = (name: string, value: any) => { + setCurrentRule(currentRuleData => ({ + ...currentRuleData, + [name]: value, + })); + }; + + const onTextChange = (target: HTMLInputElement | HTMLTextAreaElement) => { + updateRuleState(target.name, target.value); + }; + + const onFilterChange = (type: string) => { + updateRuleState('filter_type', type); + }; + + const onTablesChange = (tables: Array) => { + updateRuleState('tables', tables || []); + }; + + const onRolesChange = (roles: Array) => { + updateRuleState('roles', roles || []); + }; + + const hide = () => { + clearError(); + setCurrentRule({ ...DEAFULT_RULE }); + onHide(); + }; + + const onSave = () => { + const tables: number[] = []; + const roles: number[] = []; + + currentRule.tables?.forEach(table => tables.push(table.key)); + currentRule.roles?.forEach(role => roles.push(role.key)); + + const data: any = { ...currentRule, tables, roles }; + + if (isEditMode && currentRule.id) { + const updateId = currentRule.id; + delete data.id; + updateResource(updateId, data).then(response => { + if (!response) { + return; + } + addSuccessToast(`Rule updated`); + hide(); + }); + } else if (currentRule) { + createResource(data).then(response => { + if (!response) return; + addSuccessToast(t('Rule added')); + hide(); + }); + } + }; + + // * data loaders * + const loadTableOptions = useMemo( + () => + (input = '', page: number, pageSize: number) => { + const query = rison.encode({ + filter: input, + page, + page_size: pageSize, + }); + return SupersetClient.get({ + endpoint: `/api/v1/rowlevelsecurity/related/tables?q=${query}`, + }).then(response => { + const list = response.json.result.map( + (item: { value: number; text: string }) => ({ + label: item.text, + value: item.value, + }), + ); + return { data: list, totalCount: response.json.count }; + }); + }, + [], + ); + + const loadRoleOptions = useMemo( + () => + (input = '', page: number, pageSize: number) => { + const query = rison.encode({ + filter: input, + page, + page_size: pageSize, + }); + return SupersetClient.get({ + endpoint: `/api/v1/rowlevelsecurity/related/roles?q=${query}`, + }).then(response => { + const list = response.json.result.map( + (item: { value: number; text: string }) => ({ + label: item.text, + value: item.value, + }), + ); + return { data: list, totalCount: response.json.count }; + }); + }, + [], + ); + + // * state validators * + const validate = () => { + if ( + currentRule?.name && + currentRule?.clause && + currentRule.tables?.length + ) { + setDisableSave(false); + } else { + setDisableSave(true); + } + }; + + return ( + + {isEditMode ? ( + + ) : ( + + )} + {isEditMode ? t('Edit Rule') : t('Add Rule')} + + } + > + +
+ + + onTextChange(target), + }} + css={noBottomMargin} + label={t('Rule Name')} + data-test="rule-name-test" + /> + + + +
+ {t('Filter Type')}{' '} + +
+
+