diff --git a/superset/assets/spec/javascripts/sqllab/LimitControl_spec.jsx b/superset/assets/spec/javascripts/sqllab/LimitControl_spec.jsx new file mode 100644 index 0000000000000..7869eae227e6d --- /dev/null +++ b/superset/assets/spec/javascripts/sqllab/LimitControl_spec.jsx @@ -0,0 +1,63 @@ +import React from 'react'; +import { shallow } from 'enzyme'; + +import { Label } from 'react-bootstrap'; +import LimitControl from '../../../src/SqlLab/components/LimitControl'; +import ControlHeader from '../../../src/explore/components/ControlHeader'; + +describe('LimitControl', () => { + const defaultProps = { + value: 0, + defaultQueryLimit: 1000, + maxRow: 100000, + onChange: () => {}, + }; + let wrapper; + const factory = o => ; + beforeEach(() => { + wrapper = shallow(factory(defaultProps)); + }); + it('is a valid element', () => { + expect(React.isValidElement()).toEqual(true); + }); + it('renders a Label', () => { + expect(wrapper.find(Label)).toHaveLength(1); + }); + it('loads the correct state', () => { + const value = 100; + wrapper = shallow(factory({ ...defaultProps, value })); + expect(wrapper.state().textValue).toEqual(value.toString()); + wrapper.find(Label).first().simulate('click'); + expect(wrapper.state().showOverlay).toBe(true); + expect(wrapper.find(ControlHeader).props().validationErrors).toHaveLength(0); + }); + it('handles invalid value', () => { + wrapper.find(Label).first().simulate('click'); + wrapper.setState({ textValue: 'invalid' }); + expect(wrapper.find(ControlHeader).props().validationErrors).toHaveLength(1); + }); + it('handles negative value', () => { + wrapper.find(Label).first().simulate('click'); + wrapper.setState({ textValue: '-1' }); + expect(wrapper.find(ControlHeader).props().validationErrors).toHaveLength(1); + }); + it('handles value above max row', () => { + wrapper.find(Label).first().simulate('click'); + wrapper.setState({ textValue: (defaultProps.maxRow + 1).toString() }); + expect(wrapper.find(ControlHeader).props().validationErrors).toHaveLength(1); + }); + it('opens and closes', () => { + wrapper.find(Label).first().simulate('click'); + expect(wrapper.state().showOverlay).toBe(true); + wrapper.find('.ok').first().simulate('click'); + expect(wrapper.state().showOverlay).toBe(false); + }); + it('resets and closes', () => { + const value = 100; + wrapper = shallow(factory({ ...defaultProps, value })); + wrapper.find(Label).first().simulate('click'); + expect(wrapper.state().textValue).toEqual(value.toString()); + wrapper.find('.reset').simulate('click'); + expect(wrapper.state().textValue).toEqual(defaultProps.defaultQueryLimit.toString()); + }); +}); diff --git a/superset/assets/spec/javascripts/sqllab/SqlEditor_spec.jsx b/superset/assets/spec/javascripts/sqllab/SqlEditor_spec.jsx index 822192dd2f87d..95db9b05dea06 100644 --- a/superset/assets/spec/javascripts/sqllab/SqlEditor_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/SqlEditor_spec.jsx @@ -1,7 +1,8 @@ import React from 'react'; import { shallow } from 'enzyme'; -import { initialState, queries, table } from './fixtures'; +import { defaultQueryEditor, initialState, queries, table } from './fixtures'; +import LimitControl from '../../../src/SqlLab/components/LimitControl'; import SqlEditor from '../../../src/SqlLab/components/SqlEditor'; import SqlEditorLeftBar from '../../../src/SqlLab/components/SqlEditorLeftBar'; @@ -16,6 +17,8 @@ describe('SqlEditor', () => { getHeight: () => ('100px'), editorQueries: [], dataPreviewQueries: [], + defaultQueryLimit: 1000, + maxRow: 100000, }; it('is valid', () => { expect( @@ -26,4 +29,18 @@ describe('SqlEditor', () => { const wrapper = shallow(); expect(wrapper.find(SqlEditorLeftBar)).toHaveLength(1); }); + it('render a LimitControl with default limit', () => { + const defaultQueryLimit = 101; + const updatedProps = { ...mockedProps, defaultQueryLimit }; + const wrapper = shallow(); + expect(wrapper.find(LimitControl)).toHaveLength(1); + expect(wrapper.find(LimitControl).props().value).toEqual(defaultQueryLimit); + }); + it('render a LimitControl with existing limit', () => { + const queryEditor = { ...defaultQueryEditor, queryLimit: 101 }; + const updatedProps = { ...mockedProps, queryEditor }; + const wrapper = shallow(); + expect(wrapper.find(LimitControl)).toHaveLength(1); + expect(wrapper.find(LimitControl).props().value).toEqual(queryEditor.queryLimit); + }); }); diff --git a/superset/assets/spec/javascripts/sqllab/TabbedSqlEditors_spec.jsx b/superset/assets/spec/javascripts/sqllab/TabbedSqlEditors_spec.jsx index 33d1e476af9c9..111aba9499fed 100644 --- a/superset/assets/spec/javascripts/sqllab/TabbedSqlEditors_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/TabbedSqlEditors_spec.jsx @@ -52,6 +52,8 @@ describe('TabbedSqlEditors', () => { editorHeight: '', getHeight: () => ('100px'), database: {}, + defaultQueryLimit: 1000, + maxRow: 100000, }; const getWrapper = () => ( shallow(, { diff --git a/superset/assets/spec/javascripts/sqllab/fixtures.js b/superset/assets/spec/javascripts/sqllab/fixtures.js index 02a6c127f49aa..99ba6a8e6814d 100644 --- a/superset/assets/spec/javascripts/sqllab/fixtures.js +++ b/superset/assets/spec/javascripts/sqllab/fixtures.js @@ -367,6 +367,12 @@ export const initialState = { activeSouthPaneTab: 'Results', }, messageToasts: [], + common: { + conf: { + DEFAULT_SQLLAB_LIMIT: 1000, + SQL_MAX_ROW: 100000, + }, + }, }; export const query = { diff --git a/superset/assets/spec/javascripts/sqllab/reducers/sqlLab_spec.js b/superset/assets/spec/javascripts/sqllab/reducers/sqlLab_spec.js index 964daa8fbd927..ba189ca0488ea 100644 --- a/superset/assets/spec/javascripts/sqllab/reducers/sqlLab_spec.js +++ b/superset/assets/spec/javascripts/sqllab/reducers/sqlLab_spec.js @@ -81,6 +81,11 @@ describe('sqlLabReducer', () => { newState = sqlLabReducer(newState, actions.queryEditorSetSql(qe, sql)); expect(newState.queryEditors[1].sql).toBe(sql); }); + it('should not fail while setting queryLimit', () => { + const queryLimit = 101; + newState = sqlLabReducer(newState, actions.queryEditorSetQueryLimit(qe, queryLimit)); + expect(newState.queryEditors[1].queryLimit).toEqual(queryLimit); + }); it('should set selectedText', () => { const selectedText = 'TEST'; expect(newState.queryEditors[0].selectedText).toBeNull(); diff --git a/superset/assets/src/SqlLab/actions/sqlLab.js b/superset/assets/src/SqlLab/actions/sqlLab.js index 17e022b8c723c..084aaea80fa4c 100644 --- a/superset/assets/src/SqlLab/actions/sqlLab.js +++ b/superset/assets/src/SqlLab/actions/sqlLab.js @@ -27,6 +27,7 @@ export const QUERY_EDITOR_SET_SCHEMA = 'QUERY_EDITOR_SET_SCHEMA'; export const QUERY_EDITOR_SET_TITLE = 'QUERY_EDITOR_SET_TITLE'; export const QUERY_EDITOR_SET_AUTORUN = 'QUERY_EDITOR_SET_AUTORUN'; export const QUERY_EDITOR_SET_SQL = 'QUERY_EDITOR_SET_SQL'; +export const QUERY_EDITOR_SET_QUERY_LIMIT = 'QUERY_EDITOR_SET_QUERY_LIMIT'; export const QUERY_EDITOR_SET_TEMPLATE_PARAMS = 'QUERY_EDITOR_SET_TEMPLATE_PARAMS'; export const QUERY_EDITOR_SET_SELECTED_TEXT = 'QUERY_EDITOR_SET_SELECTED_TEXT'; export const QUERY_EDITOR_PERSIST_HEIGHT = 'QUERY_EDITOR_PERSIST_HEIGHT'; @@ -141,6 +142,7 @@ export function runQuery(query) { tmp_table_name: query.tempTableName, select_as_cta: query.ctas, templateParams: query.templateParams, + queryLimit: query.queryLimit, }; return SupersetClient.post({ @@ -230,6 +232,10 @@ export function queryEditorSetSql(queryEditor, sql) { return { type: QUERY_EDITOR_SET_SQL, queryEditor, sql }; } +export function queryEditorSetQueryLimit(queryEditor, queryLimit) { + return { type: QUERY_EDITOR_SET_QUERY_LIMIT, queryEditor, queryLimit }; +} + export function queryEditorSetTemplateParams(queryEditor, templateParams) { return { type: QUERY_EDITOR_SET_TEMPLATE_PARAMS, queryEditor, templateParams }; } @@ -325,6 +331,7 @@ export function reFetchQueryResults(query) { tab: '', runAsync: false, ctas: false, + queryLimit: query.queryLimit, }; dispatch(runQuery(newQuery)); dispatch(changeDataPreviewId(query.id, newQuery)); diff --git a/superset/assets/src/SqlLab/components/LimitControl.jsx b/superset/assets/src/SqlLab/components/LimitControl.jsx new file mode 100644 index 0000000000000..cdb1f5750de5b --- /dev/null +++ b/superset/assets/src/SqlLab/components/LimitControl.jsx @@ -0,0 +1,126 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { + Button, + Label, + FormGroup, + FormControl, + Overlay, + Popover, +} from 'react-bootstrap'; +import { t } from '@superset-ui/translation'; + +import ControlHeader from '../../explore/components/ControlHeader'; + +const propTypes = { + value: PropTypes.number, + defaultQueryLimit: PropTypes.number.isRequired, + maxRow: PropTypes.number.isRequired, + onChange: PropTypes.func.isRequired, +}; + +export default class LimitControl extends React.PureComponent { + constructor(props) { + super(props); + const { value, defaultQueryLimit } = props; + this.state = { + textValue: value.toString() || defaultQueryLimit.toString(), + showOverlay: false, + }; + this.handleHide = this.handleHide.bind(this); + this.handleToggle = this.handleToggle.bind(this); + this.submitAndClose = this.submitAndClose.bind(this); + } + + setValueAndClose(val) { + this.setState({ textValue: val }, this.submitAndClose); + } + + submitAndClose() { + const value = parseInt(this.state.textValue, 10) || this.props.defaultQueryLimit; + this.props.onChange(value); + this.setState({ showOverlay: false }); + } + + isValidLimit(limit) { + const value = parseInt(limit, 10); + return !(Number.isNaN(value) || value <= 0 || (this.props.maxRow && value > this.props.maxRow)); + } + + handleToggle() { + this.setState({ showOverlay: !this.state.showOverlay }); + } + + handleHide() { + this.setState({ showOverlay: false }); + } + + renderPopover() { + const textValue = this.state.textValue; + const isValid = this.isValidLimit(textValue); + const errorMsg = 'Row limit must be positive integer' + + (this.props.maxRow ? ` and not greater than ${this.props.maxRow}` : ''); + return ( + +
+ + + this.setState({ textValue: e.target.value })} + /> + +
+ + +
+
+ + ); + } + + render() { + return ( +
+ + + {this.renderPopover()} + +
+ ); + } +} + +LimitControl.propTypes = propTypes; diff --git a/superset/assets/src/SqlLab/components/SqlEditor.jsx b/superset/assets/src/SqlLab/components/SqlEditor.jsx index e739efa518f71..7a387707c0ba4 100644 --- a/superset/assets/src/SqlLab/components/SqlEditor.jsx +++ b/superset/assets/src/SqlLab/components/SqlEditor.jsx @@ -17,6 +17,7 @@ import SplitPane from 'react-split-pane'; import { t } from '@superset-ui/translation'; import Button from '../../components/Button'; +import LimitControl from './LimitControl'; import TemplateParamsEditor from './TemplateParamsEditor'; import SouthPane from './SouthPane'; import SaveQuery from './SaveQuery'; @@ -38,6 +39,8 @@ const propTypes = { dataPreviewQueries: PropTypes.array.isRequired, queryEditor: PropTypes.object.isRequired, hideLeftBar: PropTypes.bool, + defaultQueryLimit: PropTypes.number.isRequired, + maxRow: PropTypes.number.isRequired, }; const defaultProps = { @@ -130,6 +133,9 @@ class SqlEditor extends React.PureComponent { setQueryEditorSql(sql) { this.props.actions.queryEditorSetSql(this.props.queryEditor, sql); } + setQueryLimit(queryLimit) { + this.props.actions.queryEditorSetQueryLimit(this.props.queryEditor, queryLimit); + } runQuery() { this.startQuery(!(this.props.database || {}).allow_run_sync); } @@ -143,6 +149,7 @@ class SqlEditor extends React.PureComponent { schema: qe.schema, tempTableName: ctas ? this.state.ctas : '', templateParams: qe.templateParams, + queryLimit: qe.queryLimit, runAsync, ctas, }; @@ -236,7 +243,18 @@ class SqlEditor extends React.PureComponent { - {ctasControls} + + {ctasControls} + + + + )} @@ -243,7 +247,7 @@ class TabbedSqlEditors extends React.PureComponent { TabbedSqlEditors.propTypes = propTypes; TabbedSqlEditors.defaultProps = defaultProps; -function mapStateToProps({ sqlLab }) { +function mapStateToProps({ sqlLab, common }) { return { databases: sqlLab.databases, queryEditors: sqlLab.queryEditors, @@ -252,6 +256,8 @@ function mapStateToProps({ sqlLab }) { tables: sqlLab.tables, defaultDbId: sqlLab.defaultDbId, offline: sqlLab.offline, + defaultQueryLimit: common.conf.DEFAULT_SQLLAB_LIMIT, + maxRow: common.conf.SQL_MAX_ROW, }; } function mapDispatchToProps(dispatch) { diff --git a/superset/assets/src/SqlLab/reducers/getInitialState.js b/superset/assets/src/SqlLab/reducers/getInitialState.js index 14bd677d39d8a..3351a22a5c03c 100644 --- a/superset/assets/src/SqlLab/reducers/getInitialState.js +++ b/superset/assets/src/SqlLab/reducers/getInitialState.js @@ -11,6 +11,7 @@ export default function getInitialState({ defaultDbId, ...restBootstrapData }) { latestQueryId: null, autorun: false, dbId: defaultDbId, + queryLimit: restBootstrapData.common.conf.DEFAULT_SQLLAB_LIMIT, }; return { diff --git a/superset/assets/src/SqlLab/reducers/sqlLab.js b/superset/assets/src/SqlLab/reducers/sqlLab.js index b8a27a97d4153..ff881c327a3ee 100644 --- a/superset/assets/src/SqlLab/reducers/sqlLab.js +++ b/superset/assets/src/SqlLab/reducers/sqlLab.js @@ -32,6 +32,8 @@ export default function sqlLabReducer(state = {}, action) { schema: action.query.schema ? action.query.schema : null, autorun: true, sql: action.query.sql, + queryLimit: action.query.queryLimit, + maxRow: action.query.maxRow, }; return sqlLabReducer(state, actions.addQueryEditor(qe)); @@ -204,6 +206,9 @@ export default function sqlLabReducer(state = {}, action) { [actions.QUERY_EDITOR_SET_SQL]() { return alterInArr(state, 'queryEditors', action.queryEditor, { sql: action.sql }); }, + [actions.QUERY_EDITOR_SET_QUERY_LIMIT]() { + return alterInArr(state, 'queryEditors', action.queryEditor, { queryLimit: action.queryLimit }); + }, [actions.QUERY_EDITOR_SET_TEMPLATE_PARAMS]() { return alterInArr(state, 'queryEditors', action.queryEditor, { templateParams: action.templateParams, diff --git a/superset/config.py b/superset/config.py index c94373969f52e..a34e5d96f436b 100644 --- a/superset/config.py +++ b/superset/config.py @@ -282,8 +282,8 @@ # in the results backend. This also becomes the limit when exporting CSVs SQL_MAX_ROW = 100000 -# Limit to be returned to the frontend. -DISPLAY_MAX_ROW = 1000 +# Default row limit for SQL Lab queries +DEFAULT_SQLLAB_LIMIT = 1000 # Maximum number of tables/views displayed in the dropdown window in SQL Lab. MAX_TABLE_NAMES = 3000 diff --git a/superset/sql_lab.py b/superset/sql_lab.py index d211f025c5b73..1d4171b175780 100644 --- a/superset/sql_lab.py +++ b/superset/sql_lab.py @@ -150,10 +150,11 @@ def handle_error(msg): query.user_id, start_dttm.strftime('%Y_%m_%d_%H_%M_%S')) executed_sql = superset_query.as_create_table(query.tmp_table_name) query.select_as_cta_used = True - if (superset_query.is_select() and SQL_MAX_ROWS and - (not query.limit or query.limit > SQL_MAX_ROWS)): - query.limit = SQL_MAX_ROWS - executed_sql = database.apply_limit_to_sql(executed_sql, query.limit) + if superset_query.is_select(): + if SQL_MAX_ROWS and (not query.limit or query.limit > SQL_MAX_ROWS): + query.limit = SQL_MAX_ROWS + if query.limit: + executed_sql = database.apply_limit_to_sql(executed_sql, query.limit) # Hook to allow environment-specific mutation (usually comments) to the SQL SQL_QUERY_MUTATOR = config.get('SQL_QUERY_MUTATOR') diff --git a/superset/views/base.py b/superset/views/base.py index 4cba8b23df888..4b4926c959cab 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -24,6 +24,8 @@ 'SUPERSET_WEBSERVER_TIMEOUT', 'SUPERSET_DASHBOARD_POSITION_DATA_LIMIT', 'ENABLE_JAVASCRIPT_CONTROLS', + 'DEFAULT_SQLLAB_LIMIT', + 'SQL_MAX_ROW', ) diff --git a/superset/views/core.py b/superset/views/core.py index a16bceb4c289a..1f11e704b4e7b 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2456,7 +2456,7 @@ def results(self, key): '{}'.format(rejected_tables)), status=403) payload = utils.zlib_decompress_to_string(blob) - display_limit = app.config.get('DISPLAY_MAX_ROW', None) + display_limit = app.config.get('DEFAULT_SQLLAB_LIMIT', None) if display_limit: payload_json = json.loads(payload) payload_json['data'] = payload_json['data'][:display_limit] @@ -2495,6 +2495,12 @@ def sql_json(self): schema = request.form.get('schema') or None template_params = json.loads( request.form.get('templateParams') or '{}') + limit = int(request.form.get('queryLimit', 0)) + if limit < 0: + logging.warning( + 'Invalid limit of {} specified. Defaulting to max limit.'.format(limit)) + limit = 0 + limit = limit or app.config.get('SQL_MAX_ROW') session = db.session() mydb = session.query(models.Database).filter_by(id=database_id).first() @@ -2520,10 +2526,10 @@ def sql_json(self): ) client_id = request.form.get('client_id') or utils.shortid()[:10] - + limits = [mydb.db_engine_spec.get_limit_from_sql(sql), limit] query = Query( database_id=int(database_id), - limit=mydb.db_engine_spec.get_limit_from_sql(sql), + limit=min(lim for lim in limits if lim is not None), sql=sql, schema=schema, select_as_cta=request.form.get('select_as_cta') == 'true', diff --git a/tests/base_tests.py b/tests/base_tests.py index b9b4649a50dbe..7ff1036e321d5 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -154,7 +154,8 @@ def revoke_public_access_to_table(self, table): perm.view_menu and table.perm in perm.view_menu.name): security_manager.del_permission_role(public_role, perm) - def run_sql(self, sql, client_id=None, user_name=None, raise_on_error=False): + def run_sql(self, sql, client_id=None, user_name=None, raise_on_error=False, + query_limit=None): if user_name: self.logout() self.login(username=(user_name if user_name else 'admin')) @@ -163,7 +164,7 @@ def run_sql(self, sql, client_id=None, user_name=None, raise_on_error=False): '/superset/sql_json/', raise_on_error=False, data=dict(database_id=dbid, sql=sql, select_as_create_as=False, - client_id=client_id), + client_id=client_id, queryLimit=query_limit), ) if raise_on_error and 'error' in resp: raise Exception('run_sql failed') diff --git a/tests/sqllab_tests.py b/tests/sqllab_tests.py index ca2063cf12d3c..95e3fbc144fd0 100644 --- a/tests/sqllab_tests.py +++ b/tests/sqllab_tests.py @@ -260,6 +260,29 @@ def test_sqllab_viz(self): resp = self.get_json_resp('/superset/sqllab_viz/', data=data) self.assertIn('table_id', resp) + def test_sql_limit(self): + self.login('admin') + test_limit = 1 + data = self.run_sql( + 'SELECT * FROM ab_user', + client_id='sql_limit_1') + self.assertGreater(len(data['data']), test_limit) + data = self.run_sql( + 'SELECT * FROM ab_user', + client_id='sql_limit_2', + query_limit=test_limit) + self.assertEquals(len(data['data']), test_limit) + data = self.run_sql( + 'SELECT * FROM ab_user LIMIT {}'.format(test_limit), + client_id='sql_limit_3', + query_limit=test_limit + 1) + self.assertEquals(len(data['data']), test_limit) + data = self.run_sql( + 'SELECT * FROM ab_user LIMIT {}'.format(test_limit + 1), + client_id='sql_limit_4', + query_limit=test_limit) + self.assertEquals(len(data['data']), test_limit) + if __name__ == '__main__': unittest.main()