From commits-return-2678-archive-asf-public=cust-asf.ponee.io@superset.incubator.apache.org Wed May 15 22:32:52 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id DAC96180621 for ; Thu, 16 May 2019 00:32:51 +0200 (CEST) Received: (qmail 96851 invoked by uid 500); 15 May 2019 22:32:51 -0000 Mailing-List: contact commits-help@superset.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@superset.incubator.apache.org Delivered-To: mailing list commits@superset.incubator.apache.org Received: (qmail 96805 invoked by uid 99); 15 May 2019 22:32:51 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 15 May 2019 22:32:51 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 34B81873CE; Wed, 15 May 2019 22:32:51 +0000 (UTC) Date: Wed, 15 May 2019 22:32:50 +0000 To: "commits@superset.apache.org" Subject: [incubator-superset] branch master updated: feat: Live query validation in the SQL Lab UI (#7461) (#7516) (#7518) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <155795957059.7570.16738516321201068657@gitbox.apache.org> From: christine@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: incubator-superset X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Oldrev: fda10026548169b507a2ccad3049bd3305c7978b X-Git-Newrev: 156b0aaa0705e5b3900aa3b43d50a2fef01aee54 X-Git-Rev: 156b0aaa0705e5b3900aa3b43d50a2fef01aee54 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated This is an automated email from the ASF dual-hosted git repository. christine pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-superset.git The following commit(s) were added to refs/heads/master by this push: new 156b0aa feat: Live query validation in the SQL Lab UI (#7461) (#7516) (#7518) 156b0aa is described below commit 156b0aaa0705e5b3900aa3b43d50a2fef01aee54 Author: Alex Berghage AuthorDate: Wed May 15 16:32:40 2019 -0600 feat: Live query validation in the SQL Lab UI (#7461) (#7516) (#7518) * [WIP] Live query validation, where supported This builds on #7422 to build check-as-you-type sql query validation in Sql Lab. This closes #6707 too. It adds a (debounced) call to the validate_sql_json API endpoint with the querytext, and on Lyft infra is able to return feedback to the user (end to end) in $TBD seconds. At present feedback is provided only through the "annotations" mechanism build in to ACE, although I'd be open to adding full text elsewhere on the page if there's interest. * fix: Unbreak lints and tests --- superset/assets/src/SqlLab/actions/sqlLab.js | 54 ++++++++++++++++++ .../src/SqlLab/components/AceEditorWrapper.jsx | 15 +++++ .../assets/src/SqlLab/components/SqlEditor.jsx | 34 +++++++++++ .../assets/src/SqlLab/reducers/getInitialState.js | 5 ++ superset/assets/src/SqlLab/reducers/sqlLab.js | 65 ++++++++++++++++++++++ superset/assets/src/featureFlags.ts | 1 + superset/views/core.py | 5 +- 7 files changed, 176 insertions(+), 3 deletions(-) diff --git a/superset/assets/src/SqlLab/actions/sqlLab.js b/superset/assets/src/SqlLab/actions/sqlLab.js index f6ac455..81c8e8d 100644 --- a/superset/assets/src/SqlLab/actions/sqlLab.js +++ b/superset/assets/src/SqlLab/actions/sqlLab.js @@ -65,6 +65,10 @@ 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'; +export const START_QUERY_VALIDATION = 'START_QUERY_VALIDATION'; +export const QUERY_VALIDATION_RETURNED = 'QUERY_VALIDATION_RETURNED'; +export const QUERY_VALIDATION_FAILED = 'QUERY_VALIDATION_FAILED'; + export const CREATE_DATASOURCE_STARTED = 'CREATE_DATASOURCE_STARTED'; export const CREATE_DATASOURCE_SUCCESS = 'CREATE_DATASOURCE_SUCCESS'; export const CREATE_DATASOURCE_FAILED = 'CREATE_DATASOURCE_FAILED'; @@ -77,6 +81,21 @@ export function resetState() { return { type: RESET_STATE }; } +export function startQueryValidation(query) { + Object.assign(query, { + id: query.id ? query.id : shortid.generate(), + }); + return { type: START_QUERY_VALIDATION, query }; +} + +export function queryValidationReturned(query, results) { + return { type: QUERY_VALIDATION_RETURNED, query, results }; +} + +export function queryValidationFailed(query, message, error) { + return { type: QUERY_VALIDATION_FAILED, query, message, error }; +} + export function saveQuery(query) { return dispatch => SupersetClient.post({ @@ -187,6 +206,41 @@ export function runQuery(query) { }; } +export function validateQuery(query) { + return function (dispatch) { + dispatch(startQueryValidation(query)); + + const postPayload = { + client_id: query.id, + database_id: query.dbId, + json: true, + schema: query.schema, + sql: query.sql, + sql_editor_id: query.sqlEditorId, + templateParams: query.templateParams, + validate_only: true, + }; + + return SupersetClient.post({ + endpoint: `/superset/validate_sql_json/${window.location.search}`, + postPayload, + stringify: false, + }) + .then(({ json }) => { + dispatch(queryValidationReturned(query, json)); + }) + .catch(response => + getClientErrorObject(response).then((error) => { + let message = error.error || error.statusText || t('Unknown error'); + if (message.includes('CSRF token')) { + message = t(COMMON_ERR_MESSAGES.SESSION_TIMED_OUT); + } + dispatch(queryValidationFailed(query, message, error)); + }), + ); + }; +} + export function postStopQuery(query) { return function (dispatch) { return SupersetClient.post({ diff --git a/superset/assets/src/SqlLab/components/AceEditorWrapper.jsx b/superset/assets/src/SqlLab/components/AceEditorWrapper.jsx index 14a9775..6c87ec2 100644 --- a/superset/assets/src/SqlLab/components/AceEditorWrapper.jsx +++ b/superset/assets/src/SqlLab/components/AceEditorWrapper.jsx @@ -157,6 +157,20 @@ class AceEditorWrapper extends React.PureComponent { } }); } + getAceAnnotations() { + const validationResult = this.props.queryEditor.validationResult; + const resultIsReady = (validationResult && validationResult.completed); + if (resultIsReady && validationResult.errors.length > 0) { + const errors = validationResult.errors.map(err => ({ + type: 'error', + row: err.line_number - 1, + column: err.start_column - 1, + text: err.message, + })); + return errors; + } + return []; + } render() { return ( ); } diff --git a/superset/assets/src/SqlLab/components/SqlEditor.jsx b/superset/assets/src/SqlLab/components/SqlEditor.jsx index f4495af..0b25dcf 100644 --- a/superset/assets/src/SqlLab/components/SqlEditor.jsx +++ b/superset/assets/src/SqlLab/components/SqlEditor.jsx @@ -30,6 +30,7 @@ import { } from 'react-bootstrap'; import Split from 'react-split'; import { t } from '@superset-ui/translation'; +import debounce from 'lodash/debounce'; import Button from '../../components/Button'; import LimitControl from './LimitControl'; @@ -52,6 +53,7 @@ const GUTTER_HEIGHT = 5; const GUTTER_MARGIN = 3; const INITIAL_NORTH_PERCENT = 30; const INITIAL_SOUTH_PERCENT = 70; +const VALIDATION_DEBOUNCE_MS = 600; const propTypes = { actions: PropTypes.object.isRequired, @@ -88,6 +90,7 @@ class SqlEditor extends React.PureComponent { this.elementStyle = this.elementStyle.bind(this); this.onResizeStart = this.onResizeStart.bind(this); this.onResizeEnd = this.onResizeEnd.bind(this); + this.canValidateQuery = this.canValidateQuery.bind(this); this.runQuery = this.runQuery.bind(this); this.stopQuery = this.stopQuery.bind(this); this.onSqlChanged = this.onSqlChanged.bind(this); @@ -95,6 +98,10 @@ class SqlEditor extends React.PureComponent { this.queryPane = this.queryPane.bind(this); this.getAceEditorAndSouthPaneHeights = this.getAceEditorAndSouthPaneHeights.bind(this); this.getSqlEditorHeight = this.getSqlEditorHeight.bind(this); + this.requestValidation = debounce( + this.requestValidation.bind(this), + VALIDATION_DEBOUNCE_MS, + ); } componentWillMount() { if (this.state.autorun) { @@ -126,6 +133,11 @@ class SqlEditor extends React.PureComponent { } onSqlChanged(sql) { this.setState({ sql }); + // Request server-side validation of the query text + if (this.canValidateQuery()) { + // NB. requestValidation is debounced + this.requestValidation(); + } } // One layer of abstraction for easy spying in unit tests getSqlEditorHeight() { @@ -186,6 +198,28 @@ class SqlEditor extends React.PureComponent { [dimension]: `calc(${elementSize}% - ${gutterSize + GUTTER_MARGIN}px)`, }; } + requestValidation() { + if (this.props.database) { + const qe = this.props.queryEditor; + const query = { + dbId: qe.dbId, + sql: this.state.sql, + sqlEditorId: qe.id, + schema: qe.schema, + templateParams: qe.templateParams, + }; + this.props.actions.validateQuery(query); + } + } + canValidateQuery() { + // Check whether or not we can validate the current query based on whether + // or not the backend has a validator configured for it. + const validatorMap = window.featureFlags.SQL_VALIDATORS_BY_ENGINE; + if (this.props.database && validatorMap != null) { + return validatorMap.hasOwnProperty(this.props.database.backend); + } + return false; + } runQuery() { if (this.props.database) { this.startQuery(this.props.database.allow_run_async); diff --git a/superset/assets/src/SqlLab/reducers/getInitialState.js b/superset/assets/src/SqlLab/reducers/getInitialState.js index adb3db4..535cb3d 100644 --- a/superset/assets/src/SqlLab/reducers/getInitialState.js +++ b/superset/assets/src/SqlLab/reducers/getInitialState.js @@ -30,6 +30,11 @@ export default function getInitialState({ defaultDbId, ...restBootstrapData }) { autorun: false, dbId: defaultDbId, queryLimit: restBootstrapData.common.conf.DEFAULT_SQLLAB_LIMIT, + validationResult: { + id: null, + errors: [], + completed: false, + }, }; return { diff --git a/superset/assets/src/SqlLab/reducers/sqlLab.js b/superset/assets/src/SqlLab/reducers/sqlLab.js index 93ff325..a94e817 100644 --- a/superset/assets/src/SqlLab/reducers/sqlLab.js +++ b/superset/assets/src/SqlLab/reducers/sqlLab.js @@ -141,6 +141,71 @@ export default function sqlLabReducer(state = {}, action) { [actions.REMOVE_TABLE]() { return removeFromArr(state, 'tables', action.table); }, + [actions.START_QUERY_VALIDATION]() { + let newState = Object.assign({}, state); + const sqlEditor = { id: action.query.sqlEditorId }; + newState = alterInArr(newState, 'queryEditors', sqlEditor, { + validationResult: { + id: action.query.id, + errors: [], + completed: false, + }, + }); + return newState; + }, + [actions.QUERY_VALIDATION_RETURNED]() { + // If the server is very slow about answering us, we might get validation + // responses back out of order. This check confirms the response we're + // handling corresponds to the most recently dispatched request. + // + // We don't care about any but the most recent because validations are + // only valid for the SQL text they correspond to -- once the SQL has + // changed, the old validation doesn't tell us anything useful anymore. + const qe = getFromArr(state.queryEditors, action.query.sqlEditorId); + if (qe.validationResult.id !== action.query.id) { + return state; + } + // Otherwise, persist the results on the queryEditor state + let newState = Object.assign({}, state); + const sqlEditor = { id: action.query.sqlEditorId }; + newState = alterInArr(newState, 'queryEditors', sqlEditor, { + validationResult: { + id: action.query.id, + errors: action.results, + completed: true, + }, + }); + return newState; + }, + [actions.QUERY_VALIDATION_FAILED]() { + // If the server is very slow about answering us, we might get validation + // responses back out of order. This check confirms the response we're + // handling corresponds to the most recently dispatched request. + // + // We don't care about any but the most recent because validations are + // only valid for the SQL text they correspond to -- once the SQL has + // changed, the old validation doesn't tell us anything useful anymore. + const qe = getFromArr(state.queryEditors, action.query.sqlEditorId); + if (qe.validationResult.id !== action.query.id) { + return state; + } + // Otherwise, persist the results on the queryEditor state + let newState = Object.assign({}, state); + const sqlEditor = { id: action.query.sqlEditorId }; + newState = alterInArr(newState, 'queryEditors', sqlEditor, { + validationResult: { + id: action.query.id, + errors: [{ + line_number: 1, + start_column: 1, + end_column: 1, + message: `The server failed to validate your query.\n${action.message}`, + }], + completed: true, + }, + }); + return newState; + }, [actions.START_QUERY]() { let newState = Object.assign({}, state); if (action.query.sqlEditorId) { diff --git a/superset/assets/src/featureFlags.ts b/superset/assets/src/featureFlags.ts index 450ad2c..bd0855e 100644 --- a/superset/assets/src/featureFlags.ts +++ b/superset/assets/src/featureFlags.ts @@ -23,6 +23,7 @@ export enum FeatureFlag { OMNIBAR = 'OMNIBAR', CLIENT_CACHE = 'CLIENT_CACHE', SCHEDULED_QUERIES = 'SCHEDULED_QUERIES', + SQL_VALIDATORS_BY_ENGINE = 'SQL_VALIDATORS_BY_ENGINE', } export type FeatureFlagMap = { diff --git a/superset/views/core.py b/superset/views/core.py index 468ea58..e79de11 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2579,9 +2579,8 @@ class Superset(BaseSupersetView): except Exception as e: logging.exception(e) msg = _( - 'Failed to validate your SQL query text. Please check that ' - f'you have configured the {validator.name} validator ' - 'correctly and that any services it depends on are up. ' + f'{validator.name} was unable to check your query.\nPlease ' + 'make sure that any services it depends on are available\n' f'Exception: {e}') return json_error_response(f'{msg}')