This is an automated email from the ASF dual-hosted git repository.
craigrueda 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 38f3fd0 Add feature flags to control query sharing, KV exposure
(#9120)
38f3fd0 is described below
commit 38f3fd0c9fab24988ba9023c114e74f6a5e5c9bb
Author: Will Barrett <w...@preset.io>
AuthorDate: Wed Feb 19 09:51:50 2020 -0800
Add feature flags to control query sharing, KV exposure (#9120)
* Add feature flags to control query sharing, KV exposure
* Add tests, fix bug
* Skip test for kv endpoints when they are disabled
* ESLint fixes
* Remove unnecessary binds
* Fix eslint errors
* Add note to UPDATING.md RE: new feature flag options
* Use expanded version of RBAC
* Enable KV_STORE and SHARE_QUERIES_VIA_KV_STORE feature flags in the test
environment
* Fix black
---
UPDATING.md | 6 +
.../javascripts/sqllab/ShareSqlLabQuery_spec.jsx | 147 +++++++++++++++------
.../src/SqlLab/components/ShareSqlLabQuery.jsx | 25 ++++
superset-frontend/src/featureFlags.ts | 1 +
superset/app.py | 5 +-
superset/config.py | 2 +
tests/core_tests.py | 14 +-
tests/superset_test_config.py | 2 +-
8 files changed, 161 insertions(+), 41 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index 9f68147..4ba13ca 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -22,6 +22,12 @@ This file documents any backwards-incompatible changes in
Superset and
assists people when migrating to a new version.
## Next
+* [9120](https://github.com/apache/incubator-superset/pull/9120): Changes the
default behavior of ad-hoc sharing of
+queries in SQLLab to one that links to the saved query rather than one that
copies the query data into the KVStore
+model and links to the record there. This is a security-related change that
makes SQLLab query
+sharing respect the existing role-based access controls. Should you wish to
retain the existing behavior, set two feature flags:
+`"KV_STORE": True` will re-enable the `/kv/` and `/kv/store/` endpoints, and
`"SHARE_QUERIES_VIA_KV_STORE": True`
+will tell the front-end to utilize them for query sharing.
* [9109](https://github.com/apache/incubator-superset/pull/9109): Expire
`filter_immune_slices` and
`filter_immune_filter_fields` to favor dashboard scoped filter metadata
`filter_scopes`.
diff --git
a/superset-frontend/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx
b/superset-frontend/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx
index c17295d..4ee8091 100644
--- a/superset-frontend/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx
+++ b/superset-frontend/spec/javascripts/sqllab/ShareSqlLabQuery_spec.jsx
@@ -21,6 +21,7 @@ import configureStore from 'redux-mock-store';
import thunk from 'redux-thunk';
import { OverlayTrigger } from 'react-bootstrap';
import fetchMock from 'fetch-mock';
+import * as featureFlags from 'src/featureFlags';
import { shallow } from 'enzyme';
import * as utils from '../../../src/utils/common';
@@ -29,8 +30,9 @@ import ShareSqlLabQuery from
'../../../src/SqlLab/components/ShareSqlLabQuery';
const mockStore = configureStore([thunk]);
const store = mockStore();
+let isFeatureEnabledMock;
-describe('ShareSqlLabQuery', () => {
+describe('ShareSqlLabQuery via /kv/store', () => {
const storeQueryUrl = 'glob:*/kv/store/';
const storeQueryMockId = '123';
@@ -50,9 +52,18 @@ describe('ShareSqlLabQuery', () => {
schema: 'query_schema',
autorun: false,
sql: 'SELECT * FROM ...',
+ remoteId: 999,
},
};
+ const storedQueryAttributes = {
+ dbId: 0,
+ title: 'query title',
+ schema: 'query_schema',
+ autorun: false,
+ sql: 'SELECT * FROM ...',
+ };
+
function setup(overrideProps) {
const wrapper = shallow(
<ShareSqlLabQuery {...defaultProps} {...overrideProps} />,
@@ -64,51 +75,113 @@ describe('ShareSqlLabQuery', () => {
return wrapper;
}
- it('renders an OverlayTrigger with Button', () => {
- const wrapper = setup();
- const trigger = wrapper.find(OverlayTrigger);
- const button = trigger.find(Button);
+ describe('via /kv/store', () => {
+ beforeAll(() => {
+ isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(() => true);
+ });
- expect(trigger).toHaveLength(1);
- expect(button).toHaveLength(1);
- });
+ afterAll(() => {
+ isFeatureEnabledMock.restore();
+ });
- it('calls storeQuery() with the query when getCopyUrl() is called and saves
the url', () => {
- expect.assertions(4);
- const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
+ it('renders an OverlayTrigger with Button', () => {
+ const wrapper = setup();
+ const trigger = wrapper.find(OverlayTrigger);
+ const button = trigger.find(Button);
- const wrapper = setup();
- const instance = wrapper.instance();
+ expect(trigger).toHaveLength(1);
+ expect(button).toHaveLength(1);
+ });
- return instance.getCopyUrl().then(() => {
- expect(storeQuerySpy.mock.calls).toHaveLength(1);
- expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1);
- expect(storeQuerySpy.mock.calls[0][0]).toMatchObject(
- defaultProps.queryEditor,
- );
- expect(instance.state.shortUrl).toContain(storeQueryMockId);
+ it('calls storeQuery() with the query when getCopyUrl() is called and
saves the url', () => {
+ expect.assertions(4);
+ const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
- return Promise.resolve();
- });
- });
+ const wrapper = setup();
+ const instance = wrapper.instance();
- it('dispatches an error toast upon fetching failure', () => {
- expect.assertions(3);
- const error = 'error';
- const addDangerToastSpy = jest.fn();
- fetchMock.post(storeQueryUrl, { throws: error }, { overwriteRoutes: true
});
- const wrapper = setup();
- wrapper.setProps({ addDangerToast: addDangerToastSpy });
-
- return wrapper
- .instance()
- .getCopyUrl()
- .then(() => {
+ return instance.getCopyUrl().then(() => {
+ expect(storeQuerySpy.mock.calls).toHaveLength(1);
expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1);
- expect(addDangerToastSpy.mock.calls).toHaveLength(1);
- expect(addDangerToastSpy.mock.calls[0][0]).toBe(error);
+ expect(storeQuerySpy.mock.calls[0][0]).toMatchObject(
+ storedQueryAttributes,
+ );
+ expect(instance.state.shortUrl).toContain(storeQueryMockId);
+
+ storeQuerySpy.mockRestore();
return Promise.resolve();
});
+ });
+
+ it('dispatches an error toast upon fetching failure', () => {
+ expect.assertions(3);
+ const error = 'error';
+ const addDangerToastSpy = jest.fn();
+ fetchMock.post(
+ storeQueryUrl,
+ { throws: error },
+ { overwriteRoutes: true },
+ );
+ const wrapper = setup();
+ wrapper.setProps({ addDangerToast: addDangerToastSpy });
+
+ return wrapper
+ .instance()
+ .getCopyUrl()
+ .then(() => {
+ expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1);
+ expect(addDangerToastSpy.mock.calls).toHaveLength(1);
+ expect(addDangerToastSpy.mock.calls[0][0]).toBe(error);
+
+ return Promise.resolve();
+ });
+ });
+ });
+ describe('via saved query', () => {
+ beforeAll(() => {
+ isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(() => false);
+ });
+
+ afterAll(() => {
+ isFeatureEnabledMock.restore();
+ });
+
+ it('renders an OverlayTrigger with Button', () => {
+ const wrapper = setup();
+ const trigger = wrapper.find(OverlayTrigger);
+ const button = trigger.find(Button);
+
+ expect(trigger).toHaveLength(1);
+ expect(button).toHaveLength(1);
+ });
+
+ it('does not call storeQuery() with the query when getCopyUrl() is
called', () => {
+ const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
+
+ const wrapper = setup();
+ const instance = wrapper.instance();
+
+ instance.getCopyUrl();
+
+ expect(storeQuerySpy.mock.calls).toHaveLength(0);
+ expect(fetchMock.calls(storeQueryUrl)).toHaveLength(0);
+ expect(instance.state.shortUrl).toContain(999);
+
+ storeQuerySpy.mockRestore();
+ });
+
+ it('shows a request to save the query when the query is not yet saved', ()
=> {
+ const wrapper = setup({ remoteId: undefined });
+ const instance = wrapper.instance();
+
+ instance.getCopyUrl();
+
+ expect(instance.state.shortUrl).toContain('save');
+ });
});
});
diff --git a/superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx
b/superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx
index 29313dc..cc1e548 100644
--- a/superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx
+++ b/superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx
@@ -20,6 +20,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import { Popover, OverlayTrigger } from 'react-bootstrap';
import { t } from '@superset-ui/translation';
+import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
import Button from '../../components/Button';
import CopyToClipboard from '../../components/CopyToClipboard';
@@ -34,6 +35,7 @@ const propTypes = {
schema: PropTypes.string,
autorun: PropTypes.bool,
sql: PropTypes.string,
+ remoteId: PropTypes.number,
}).isRequired,
addDangerToast: PropTypes.func.isRequired,
};
@@ -48,6 +50,14 @@ class ShareSqlLabQuery extends React.Component {
}
getCopyUrl() {
+ if (isFeatureEnabled(FeatureFlag.SHARE_QUERIES_VIA_KV_STORE)) {
+ return this.getCopyUrlForKvStore();
+ }
+
+ return this.getCopyUrlForSavedQuery();
+ }
+
+ getCopyUrlForKvStore() {
const { dbId, title, schema, autorun, sql } = this.props.queryEditor;
const sharedQuery = { dbId, title, schema, autorun, sql };
@@ -63,6 +73,21 @@ class ShareSqlLabQuery extends React.Component {
});
}
+ getCopyUrlForSavedQuery() {
+ let savedQueryToastContent;
+
+ if (this.props.queryEditor.remoteId) {
+ savedQueryToastContent =
+ window.location.origin +
+ window.location.pathname +
+ `?savedQueryId=${this.props.queryEditor.remoteId}`;
+ this.setState({ shortUrl: savedQueryToastContent });
+ } else {
+ savedQueryToastContent = t('Please save the query to enable sharing');
+ this.setState({ shortUrl: savedQueryToastContent });
+ }
+ }
+
renderPopover() {
return (
<Popover id="sqllab-shareurl-popover">
diff --git a/superset-frontend/src/featureFlags.ts
b/superset-frontend/src/featureFlags.ts
index e8a6287..0853578 100644
--- a/superset-frontend/src/featureFlags.ts
+++ b/superset-frontend/src/featureFlags.ts
@@ -24,6 +24,7 @@ export enum FeatureFlag {
SCHEDULED_QUERIES = 'SCHEDULED_QUERIES',
SQL_VALIDATORS_BY_ENGINE = 'SQL_VALIDATORS_BY_ENGINE',
ESTIMATE_QUERY_COST = 'ESTIMATE_QUERY_COST',
+ SHARE_QUERIES_VIA_KV_STORE = 'SHARE_QUERIES_VIA_KV_STORE',
SQLLAB_BACKEND_PERSISTENCE = 'SQLLAB_BACKEND_PERSISTENCE',
}
diff --git a/superset/app.py b/superset/app.py
index 831186d..e9d085c 100644
--- a/superset/app.py
+++ b/superset/app.py
@@ -265,7 +265,10 @@ class SupersetAppInitializer:
appbuilder.add_view_no_menu(Dashboard)
appbuilder.add_view_no_menu(DashboardModelViewAsync)
appbuilder.add_view_no_menu(Datasource)
- appbuilder.add_view_no_menu(KV)
+
+ if feature_flag_manager.is_feature_enabled("KV_STORE"):
+ appbuilder.add_view_no_menu(KV)
+
appbuilder.add_view_no_menu(R)
appbuilder.add_view_no_menu(SavedQueryView)
appbuilder.add_view_no_menu(SavedQueryViewApi)
diff --git a/superset/config.py b/superset/config.py
index 1e4756f..45237b1 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -277,7 +277,9 @@ DEFAULT_FEATURE_FLAGS = {
# Experimental feature introducing a client (browser) cache
"CLIENT_CACHE": False,
"ENABLE_EXPLORE_JSON_CSRF_PROTECTION": False,
+ "KV_STORE": False,
"PRESTO_EXPAND_DATA": False,
+ "SHARE_QUERIES_VIA_KV_STORE": False,
"TAGGING_SYSTEM": False,
}
diff --git a/tests/core_tests.py b/tests/core_tests.py
index 3e64e92..19a5ccd 100644
--- a/tests/core_tests.py
+++ b/tests/core_tests.py
@@ -30,13 +30,20 @@ import re
import string
from typing import Any, Dict
import unittest
-from unittest import mock
+from unittest import mock, skipUnless
import pandas as pd
import sqlalchemy as sqla
from tests.test_app import app
-from superset import dataframe, db, jinja_context, security_manager, sql_lab
+from superset import (
+ dataframe,
+ db,
+ jinja_context,
+ security_manager,
+ sql_lab,
+ is_feature_enabled,
+)
from superset.common.query_context import QueryContext
from superset.connectors.connector_registry import ConnectorRegistry
from superset.connectors.sqla.models import SqlaTable
@@ -497,6 +504,9 @@ class CoreTests(SupersetTestCase):
resp = self.client.post("/r/shortner/", data=dict(data=data))
assert re.search(r"\/r\/[0-9]+", resp.data.decode("utf-8"))
+ @skipUnless(
+ (is_feature_enabled("KV_STORE")), "skipping as /kv/ endpoints are not
enabled"
+ )
def test_kv(self):
self.login(username="admin")
diff --git a/tests/superset_test_config.py b/tests/superset_test_config.py
index 6e30bfd..e46df46 100644
--- a/tests/superset_test_config.py
+++ b/tests/superset_test_config.py
@@ -30,7 +30,7 @@ if "SUPERSET__SQLALCHEMY_DATABASE_URI" in os.environ:
SQL_SELECT_AS_CTA = True
SQL_MAX_ROW = 666
-FEATURE_FLAGS = {"foo": "bar"}
+FEATURE_FLAGS = {"foo": "bar", "KV_STORE": True, "SHARE_QUERIES_VIA_KV_STORE":
True}
def GET_FEATURE_FLAGS_FUNC(ff):
