Use a key-value store model for sharing long queries #1951

vera-liu · 2017-01-11T19:41:23Z

Issue:

with extremely long queries, share query link would break as it exceeds maximal number of characters permitted in a url

Solution:

adding a new key-value store model for us to store text associated with IDs
store query string in key-value store, use id in search params in url
make asynchronous call to get query string after TabbedSqlEditors is mounted
added tests

needs-review @ascott @mistercrunch @bkyryliuk

use it for storing shared queries

vera-liu · 2017-01-11T22:18:22Z

superset/views.py

@@ -1747,7 +1775,7 @@ def tables(self, db_id, schema):
        )
        tables = [t for t in database.all_table_names(schema) if
                  self.datasource_access_by_name(database, t, schema=schema)]
-        views = [v for v in database.all_table_names(schema) if
+        views = [v for v in database.all_view_names(schema) if


a minor typo, figured it's convenient just to change it here

👍 thanks !

bkyryliuk · 2017-01-13T18:23:23Z

superset/migrations/versions/bcf3126872fc_add_keyvalue.py

+def upgrade():
+    ### commands auto generated by Alembic - please adjust! ###
+    op.create_table('keyvalue',
+    sa.Column('created_on', sa.DateTime(), nullable=True),


I don't think we need audit on it

bkyryliuk · 2017-01-13T18:28:34Z

superset/models.py

@@ -210,6 +210,15 @@ class Url(Model, AuditMixinNullable):
    url = Column(Text)


+class KeyValue(Model, AuditMixinNullable):


don't think you need this: AuditMixinNullable

bkyryliuk · 2017-01-13T18:28:52Z

superset/views.py

@@ -1104,6 +1104,34 @@ def ping():
    return "OK"


+class KV(BaseSupersetView):
+
+    """used for storing and retrieving key value pairs"""


s/used/Used

bkyryliuk · 2017-01-13T18:30:14Z

superset/views.py

+        obj = models.KeyValue(value=value)
+        db.session.add(obj)
+        db.session.commit()
+        return("http://{request.headers[Host]}/{base_url}?id={obj.id}".format(


can u add a comment what's happening here, I do not fully understand?

base_url is for generalizing this model. If we directly return /kv/id like we do in shortener, sqllab redux app won't catch it on pre-rendering because url is not marking to /sqllab. Adding base_url here hooks it to the right url with the right redux app we want

I am not sure if it's safe to return it that way. urls in flask should be handled by url_for() function and redirects - via redirect function.
@mistercrunch ^^

Avoiding redirects is why we opened this PR in the first place, because we want to avoid including queries in url params. We return the url in the same pattern in shortener: https://github.com/airbnb/superset/blob/cdbd2f850706825f16a08afc40682237d6f6d54f/superset/views.py#L1127

@vera-liu - at least this is not very secure as headers can be spoofed, let's chat about the approach in person.

bkyryliuk · 2017-01-13T18:31:06Z

superset/views.py

+            request=request, base_url=base_url, obj=obj))
+
+    @log_this
+    @expose("/<key_id>/")


get or post?

bkyryliuk · 2017-01-13T18:32:52Z

tests/core_tests.py

+        resp = self.client.post('/kv/store/', data=dict(data=json.dumps(data)))
+        kv = db.session.query(models.KeyValue).first()
+        kv_value = kv.value
+        assert 'this is a test' in kv_value


use self.assertIn

bkyryliuk · 2017-01-13T18:33:03Z

tests/core_tests.py

+        kv_value = kv.value
+        assert 'this is a test' in kv_value
+        kv_id = 'id={}'.format(kv.id)
+        assert kv_id in resp.data.decode('utf-8')


bkyryliuk · 2017-01-13T18:33:34Z

tests/core_tests.py

@@ -256,6 +256,19 @@ def test_shortner(self):
        resp = self.client.post('/r/shortner/', data=data)
        assert '/r/' in resp.data.decode('utf-8')

+    def test_kv(self):


test corner cases: with null base_url and null data

ascott · 2017-01-25T00:16:04Z

superset/assets/javascripts/SqlLab/components/QueryTable.jsx

-            className="btn btn-primary btn-xs"
+          <button
+            className="btn btn-link btn-xs"
+            onClick={this.openQuery.bind(this, q.schema, q.dbId, q.sql)}


openQuery should be called with the args in this order openQuery(dbId, schema, sql)
https://github.com/airbnb/superset/pull/1951/files#diff-080302e539cb3e2903e1456909802fddR44

ascott · 2017-01-25T00:18:31Z

superset/assets/javascripts/SqlLab/components/TabbedSqlEditors.jsx

@@ -28,12 +29,32 @@ let queryCount = 1;
 class TabbedSqlEditors extends React.PureComponent {
  constructor(props) {
    super(props);
-    const uri = window.location.toString();
+    const cleanUri = '/superset/sqllab';


would sqlLabUrl make more sense than cleanUri here?

bkyryliuk · 2017-01-25T15:47:53Z

superset/assets/javascripts/SqlLab/components/CopyQueryTabUrl.jsx

-    const queryLink = window.location.pathname + '?' + queryString;
-    getShortUrl(queryLink, callback);
+    const sharedQuery = {
+      dbId: qe.dbId,


What if we store query object inside qe ?

QueryEditor is already pretty big, I don't think we need this temp query in redux store

qe already has those variables as you pull them from it.
Question was will it make sense to package them into the query object this PR introduces this concept.

bkyryliuk · 2017-01-25T15:51:12Z

superset/assets/javascripts/SqlLab/components/TabbedSqlEditors.jsx

-    const query = search.substring(1);
+    if (search) {
+      const queryString = search.substring(1);
+      const urlId = getParamFromQuery(queryString, 'id');


since we plan to do it generic, could u move this function to utils ?

ascott · 2017-01-26T22:04:28Z

superset/assets/javascripts/SqlLab/components/TabbedSqlEditors.jsx

+        $.ajax({
+          type: 'GET',
+          url: `/kv/${urlId}`,
+          success(data) {


you could get rid of
const self = this;

if you use a fat arrow method for success

if (urlId) { $.ajax({ type: 'GET', url: `/kv/${urlId}`, success: (data) => { this.popNewTab(JSON.parse(data)); }, }); }

ascott · 2017-01-26T22:17:01Z

superset/assets/javascripts/SqlLab/components/TabbedSqlEditors.jsx

+      const urlId = getParamFromQuery(queryString, 'id');
+      const self = this;
+      if (urlId) {
+        $.ajax({


seems to me that this fetching should probably be contained in an action...

ascott · 2017-01-26T22:46:05Z

superset/assets/javascripts/SqlLab/components/TabbedSqlEditors.jsx

    }
  }
+  popNewTab(newQuery) {
+    queryCount++;


what is queryCount used for?

title: Untitled Query ${queryCount},

bkyryliuk · 2017-01-26T22:56:12Z

superset/views.py

+        obj = models.KeyValue(value=value)
+        db.session.add(obj)
+        db.session.commit()
+        return url_for(base_view, id=obj.id, _external=True)


try / catch around url_for

bkyryliuk · 2017-01-26T22:57:06Z

superset/views.py

+            return Response(
+                __("Error: data can not be empty"), status=404)
+        base_view = request.form.get('baseView')
+        if not base_view:


let's not check on base_view and data validity, just try to fetch the value and return the exception message in case of error

bkyryliuk · 2017-01-26T22:57:18Z

superset/views.py

+        base_view = request.form.get('baseView')
+        if not base_view:
+            return Response(
+                __("Error: please provide a base view"), status=404)


use json_error instead

bkyryliuk · 2017-01-26T22:57:51Z

superset/views.py

+            kv = db.session.query(models.KeyValue).filter_by(id=key_id).one()
+        except Exception as e:
+            logging.exception(e)
+            return Response(


return json_error(utils.error_msg_from_exception(e), status=404)

bkyryliuk · 2017-01-26T22:59:02Z

tests/core_tests.py

+
+        value = {'data': 'this is a test'}
+        resp = self.client.post('/kv/store/', data=dict(data=json.dumps(value)))
+        self.assertEqual(resp.status_code, 200)


s/ data={'data': 'this is a test'}

bkyryliuk · 2017-01-26T22:59:22Z

tests/core_tests.py

+        self.assertEqual(resp.status_code, 200)
+        kv = db.session.query(models.KeyValue).first()
+        kv_value = kv.value
+        self.assertIn('this is a test', kv_value)


self.assertEquals

bkyryliuk · 2017-01-26T22:59:39Z

tests/core_tests.py

+
+        resp = self.client.get('/kv/{}/'.format(kv.id))
+        self.assertEqual(resp.status_code, 200)
+        self.assertIn('this is a test', resp.data.decode('utf-8'))


ditto, assertEquals

ascott · 2017-01-26T23:48:47Z

superset/assets/utils/common.js

+    url: '/kv/store/',
+    async: false,
+    data: {
+      baseView: 'Superset.sqllab',


we don't need to pass baseView anymore with your new changes...

bkyryliuk · 2017-01-27T00:16:11Z

tests/core_tests.py

+        self.login(username='admin')
+
+        try:
+            resp = self.client.post('/kv/store/', data=dict())


resp her should be 500

bkyryliuk · 2017-01-27T00:18:13Z

LGTM once @ascott comment is resolved

ascott · 2017-01-27T01:18:54Z

LGTM 🚢

* Add KeyValue model for storing id-value pairs use it for storing shared queries * Change string to text and added test * Put getQueryLink in one place * Changed migration down version * Changes based on comments * Update bcf3126872fc_add_keyvalue.py

vera-liu added 3 commits January 11, 2017 10:47

Add KeyValue model for storing id-value pairs

89f39dc

use it for storing shared queries

Change string to text and added test

065a9fc

Put getQueryLink in one place

22a4c21

vera-liu force-pushed the vliu_share_long_query branch 2 times, most recently from f595ee3 to 0d7ab39 Compare January 11, 2017 19:45

vera-liu changed the title ~~Use a key-value store model for sharing long queries~~ [WIP] Use a key-value store model for sharing long queries Jan 11, 2017

vera-liu force-pushed the vliu_share_long_query branch from fc01575 to 412bb17 Compare January 11, 2017 22:17

vera-liu commented Jan 11, 2017

View reviewed changes

vera-liu force-pushed the vliu_share_long_query branch 2 times, most recently from 7eb71c6 to 17fb74e Compare January 11, 2017 22:47

Changed migration down version

7719e5a

vera-liu force-pushed the vliu_share_long_query branch from 17fb74e to 7719e5a Compare January 11, 2017 23:24

vera-liu changed the title ~~[WIP] Use a key-value store model for sharing long queries~~ Use a key-value store model for sharing long queries Jan 11, 2017

bkyryliuk reviewed Jan 13, 2017

View reviewed changes

superset/views.py

request=request, base_url=base_url, obj=obj))

@log_this

@expose("/<key_id>/")

Copy link

Member

bkyryliuk Jan 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get or post?

bkyryliuk reviewed Jan 13, 2017

View reviewed changes

vera-liu force-pushed the vliu_share_long_query branch 2 times, most recently from b6eb346 to 3c037f5 Compare January 23, 2017 18:00

vera-liu force-pushed the vliu_share_long_query branch 2 times, most recently from d2671d4 to 42946f5 Compare January 24, 2017 23:44

ascott reviewed Jan 25, 2017

View reviewed changes

vera-liu force-pushed the vliu_share_long_query branch from 42946f5 to 70622fc Compare January 25, 2017 01:30

bkyryliuk reviewed Jan 25, 2017

View reviewed changes

vera-liu force-pushed the vliu_share_long_query branch 2 times, most recently from 4106b0a to 537f2ba Compare January 26, 2017 21:44

ascott reviewed Jan 26, 2017

View reviewed changes

vera-liu force-pushed the vliu_share_long_query branch from 537f2ba to d64131c Compare January 26, 2017 22:21

ascott reviewed Jan 26, 2017

View reviewed changes

bkyryliuk reviewed Jan 26, 2017

View reviewed changes

vera-liu force-pushed the vliu_share_long_query branch from d64131c to 766a76a Compare January 26, 2017 22:57

bkyryliuk reviewed Jan 26, 2017

View reviewed changes

vera-liu force-pushed the vliu_share_long_query branch from 766a76a to 570e8b2 Compare January 26, 2017 23:33

ascott reviewed Jan 26, 2017

View reviewed changes

bkyryliuk reviewed Jan 27, 2017

View reviewed changes

vera-liu force-pushed the vliu_share_long_query branch from 570e8b2 to 1d77b69 Compare January 27, 2017 00:22

Changes based on comments

d509e95

vera-liu force-pushed the vliu_share_long_query branch from 1d77b69 to d509e95 Compare January 27, 2017 00:25

Update bcf3126872fc_add_keyvalue.py

0a0328a

vera-liu merged commit 8580662 into apache:master Jan 27, 2017

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.16.0 labels Feb 20, 2024

		@@ -210,6 +210,15 @@ class Url(Model, AuditMixinNullable):
		url = Column(Text)


		class KeyValue(Model, AuditMixinNullable):

Use a key-value store model for sharing long queries #1951

Use a key-value store model for sharing long queries #1951

Conversation

vera-liu commented Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vera-liu Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

bkyryliuk Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascott Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

vera-liu Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

bkyryliuk commented Jan 27, 2017

ascott commented Jan 27, 2017

vera-liu commented Jan 11, 2017 •

edited

Loading

bkyryliuk Jan 13, 2017 •

edited

Loading

vera-liu Jan 13, 2017 •

edited

Loading

bkyryliuk Jan 25, 2017 •

edited

Loading

ascott Jan 25, 2017 •

edited

Loading

bkyryliuk Jan 25, 2017 •

edited

Loading

vera-liu Jan 25, 2017 •

edited

Loading

bkyryliuk Jan 26, 2017 •

edited

Loading

bkyryliuk Jan 27, 2017 •

edited

Loading