Import / export of the dashboards. #1197

bkyryliuk · 2016-09-26T16:16:59Z

Provides import export functionality for the dashboards using pickle library to serialize the objects. It requires the tables to have the same names to be able to hook slices up to them.
Import overrides the existing dashboards and slices. Newly added metrics and table column will stay, old definition will be overridden.

Git Issues: #1237. #1173

Exports:

table datasources
metrics
column definitions
dashboards
slices

Code:

stored additional metadata for slices, datasources and dashboards remote_id, database_name to be able to locate and recreated the dependencies
creates the migration to add metadata field to the tables
implements Import mixin with copy and override functions
removes table_columns relation
- uses subqueryload instead of joinedload as it's more efficient

Future works:

support druid / generic datasource
make alter_params mixin function too, requires standard params for the models

Tested:

multiple cases manually
extensive unit testing

Reviewers:

CC:

@jefffeng

ascott · 2016-09-27T01:54:33Z

what is the use case for importing/exporting slices and dashboards?

ascott · 2016-09-28T01:45:08Z

caravel/templates/caravel/export_dashboards.html

+<script>
+    window.onload = function() {
+        window.open(window.location += '&action=go');
+        window.location = '{{ dashboards_url }}';


could we do this redirect in the views.py rather than in an html template?

the challenge here is to download a file and redirect right after, flask doesn't provide an easy way to do that :(

ascott · 2016-09-28T01:49:53Z

caravel/templates/caravel/import_dashboards.html

+    <div class="container">
+        <title>Import the dashboards.</title>
+        <h1>Import the dashboards.</h1>
+        <form action="" method=post enctype=multipart/form-data>


since action="" you can leave it out altogether and it will still post to the current url.

ascott · 2016-09-28T01:50:21Z

caravel/templates/caravel/import_dashboards.html

+        <h1>Import the dashboards.</h1>
+        <form action="" method=post enctype=multipart/form-data>
+          <p><input type=file name=file>
+             <input type=submit value=Upload>


missing closing </p> tag

ascott · 2016-09-28T02:07:35Z

caravel/models.py

+        session = db.session
+        make_transient(slc)
+        slc.dashboards = []
+        id_pos = slc.perm.find('(id') if slc.perm else -1


is this a typo? find('(id') or find('id')

added a comment describing how the perm string is created.

vera-liu · 2016-09-28T21:04:01Z

caravel/views.py

+appbuilder.add_link(
+    'Import Dashboards',
+    href='/caravel/import_dashboards',
+    icon="fa-flask",


I think fa-flask is for SQL Lab, here should be some file-import icon?

Thanks, changed the icon.
Here is the list: http://fontawesome.io/icons/

maybe this one:
http://fontawesome.io/icon/files-o/

oops, missed your comment @bkyryliuk 👍

mistercrunch · 2016-10-03T18:34:36Z

caravel/models.py

+        make_transient(slc)
+        slc.dashboards = []
+        # perms are named [{db_name}].[{datasource_name}](id:{id})
+        id_pos = slc.perm.find('(id') if slc.perm else -1


let's add a perm_slice_id property to Slice, I think some other place in the code used a regex to do something like this, let's always use that new property instead.

That property should raise if the perm is malformed.

refactored in a way that there is no need to use the permission anymore

mistercrunch · 2016-10-03T18:37:50Z

caravel/models.py

+
+        perm_prefix = slc.perm[:id_pos]
+        datasource_class = SourceRegistry.sources[slc.datasource_type]
+        datasources = session.query(datasource_class).all()


filter_by missing?,

we may want a shortcut function SourceRegistry.get_datasource(type, id, session=None)

filter by doesn't work for lookups by name, created function SourceRegistry.get_datasource_by_name

mistercrunch · 2016-10-03T18:38:40Z

caravel/models.py

+        datasource_class = SourceRegistry.sources[slc.datasource_type]
+        datasources = session.query(datasource_class).all()
+        if not datasources:
+            logging.warning('Datasource {} was not found for the slice {}.'


If we assume that the datasource should always exist we sould just raise.

mistercrunch · 2016-10-03T18:41:30Z

caravel/models.py

+            return None
+
+        params_dict = {}
+        if slc.params:


a new method Slice.alert_params(**kwargs) could be useful here, resulting in a simple call:
slc.alert_params(remote_id=slc.id, import_time=import_time)

mistercrunch · 2016-10-03T18:44:07Z

caravel/models.py

+        # find if the slice was already imported
+        slc_to_override = (
+            session.query(Slice)
+            .filter(Slice.params.ilike('%"remote_id": {}%'.format(slc.id)))


seems brittle, you should at least have a comma after the {} otherwise slice_id=5 would match with 5.*

mistercrunch · 2016-10-03T19:12:45Z

tests/core_tests.py

+            return session.query(models.Dashboard).filter_by(
+                id=dash_id).first()
+
+        def get_slc(name, table, slc_id):


create_slice

mistercrunch · 2016-10-03T19:15:12Z

tests/core_tests.py

+    def test_import_dashboard(self):
+        session = db.session
+
+        def get_dashboard(title, id=0, slcs=[]):


create_dashboard

mistercrunch · 2016-10-03T19:15:43Z

tests/core_tests.py

+                slug='{}_imported'.format(title.lower()),
+            )
+
+        def find_dash(dash_id):


mistercrunch · 2016-10-03T19:15:51Z

tests/core_tests.py

+
+        def find_dash(dash_id):
+            return session.query(models.Dashboard).filter_by(
+                id=dash_id).first()


mistercrunch · 2016-10-03T19:18:01Z

tests/core_tests.py

+        imported_dash_id_4 = models.Dashboard.import_dashboard(
+            dash_with_2_slices_new, import_time=1992)
+        imported_dash_4 = find_dash(imported_dash_id_4)
+        self.assertEquals('dash_with_2_slices_new',


assert imported_dash_4.dashboard_title == imported_dash_2.dashboard_title instead of hard coding the title twice

imported_dash_2 doesn't exist when this check is done, sqlalchemy magic.

bkyryliuk · 2016-10-07T03:38:05Z

@mistercrunch - please take another look. PR is getting bigger :(
There is quite some code duplication, but I can't find an easy way to tackle that in this PR.

mistercrunch · 2016-10-07T22:02:42Z

caravel/models.py

@@ -283,6 +310,48 @@ def get_viz(self, url_params_multidict=None):
            slice_=self
        )

+    def alter_params(self, **kwargs):
+        params_dict = {}


d = self.params_dict d.update(kwargs) self.params = json.dumps(d)

@mistercrunch - done

mistercrunch · 2016-10-10T16:13:48Z

caravel/models.py

+
+        # find if the slice was already imported
+        slc_to_override = None
+        for s in session.query(Slice).all():


NIT (optional): by convention s is used for short lived strings

mistercrunch · 2016-10-10T16:15:17Z

caravel/models.py

+                slc_to_override = s
+
+        slc.id = None
+        params = json.loads(slc.params)


params = slc.params_dict

mistercrunch · 2016-10-10T16:17:44Z

caravel/source_registry.py

+    def get_datasource_by_name(cls, session, datasource_type, datasource_name,
+                               schema, database_name):
+        datasource_class = SourceRegistry.sources[datasource_type]
+        datasources = session.query(datasource_class).all()


Why returning everything and filtering on the server? You should just use a filter in session.query and use .one()

I wish that was that easy. I need to use properties for filtering, otherwise I have to implement 2 separate queries, as druid and table reference databases using different attributes. @mistercrunch

mistercrunch · 2016-10-10T16:22:33Z

caravel/views.py

@@ -26,6 +29,8 @@
 from flask_appbuilder.models.sqla.filters import BaseFilter

 from sqlalchemy import create_engine
+from werkzeug import secure_filename
+from werkzeug.datastructures import ImmutableMultiDict


dup with line 35

mistercrunch · 2016-10-10T16:25:33Z

caravel/views.py


 appbuilder.add_view(
    DashboardModelView,
    "Dashboards",
-    label=__("Dashboards"),
+    label=__("Dashboards List"),


Dashboard List

mistercrunch · 2016-10-10T16:47:52Z

tests/import_export_tests.py

+        self.assertEquals(
+            2, len([c for c in [t.columns for t in exported_tables_2]]))
+        self.assertEquals(
+            2, len([m for m in [t.metrics for t in exported_tables_2]]))


the outer comprehension doesn't do anything

mistercrunch · 2016-10-10T16:48:42Z

tests/import_export_tests.py

+            return session.query(models.SqlaTable).filter_by(
+                id=table_id).first()
+
+        # Case 1. Import table with no metrics and columns


NIT: I personally prefer having a smaller atomicity of tests and no number-suffixed variables. Shared items can be refactored into the class or module scope, methods won't run if it doesn't start with test_.

If case 5 fails here there's just more stuff to look at, understand whether case 5 is tangled with other tests or not.

yeah, in this case test cases are build upon each other. I'll try to separate them.

mistercrunch · 2016-10-10T16:49:00Z

tests/import_export_tests.py

+    """Testing export import functionality for dashboards"""
+
+    examples_loaded = False
+


requires_examples should be set to True here as you assume examples have been loaded

leftovers, sorry

mistercrunch · 2016-10-10T17:22:53Z

tests/import_export_tests.py

+            {'new_metric1', 'metric1', 'metric2'},
+            set([m.metric_name for m in imported_table_5.metrics]))
+
+        # cleanup


Would if make sense to cleanup at the beginning to setup to test? if the test fails half way, it's nice to be able to assume that you can just re-run the test itself (not the full suite) to fix the state. Idempotent, atomic tests ftw!

mistercrunch · 2016-10-10T17:28:03Z

caravel/models.py

+        return {}
+
+    @classmethod
+    def import_obj(cls, datasource_to_import, import_time=None):


There's a lot in common in all these import_obj methods. It'd be nice to distill some of it into re-usable chunks, maybe adding a CaravelModel base class and adding a get_or_create @classmethod. The second answer here is inspiring:
http://stackoverflow.com/questions/2546207/does-sqlalchemy-have-an-equivalent-of-djangos-get-or-create

And maybe a generic import_obj if that's reasonable though that might be tricky with relationships, so not a requirement for this PR...

@mistercrunch - making query to retrieve the object that have remote id is not possible as "remote_id": {id} could be in the end of the json and there will be no comma after it. Json doesn't support trailing commas. Because of it I loop through all slices / dashboards to find the object to override.
I see 2 possible solutions:

use "remote_id": {id}{prefix}instead

add remote_id field to the model

What would you suggest?
Would it be fine to have it in the separate PR ?

mistercrunch · 2016-10-11T23:48:31Z

LGTM!

bkyryliuk · 2016-10-12T00:54:33Z

thanks @mistercrunch !

bkyryliuk changed the title ~~Dashboards export. Work in progress~~ Dashboards export. Sep 28, 2016

ascott reviewed Sep 28, 2016

View reviewed changes

bkyryliuk changed the title ~~Dashboards export.~~ Import / export of the dashboards. Sep 28, 2016

vera-liu reviewed Sep 28, 2016

View reviewed changes

mistercrunch requested changes Oct 3, 2016

View reviewed changes

mistercrunch reviewed Oct 7, 2016

View reviewed changes

mistercrunch requested changes Oct 10, 2016

View reviewed changes

Bogdan Kyryliuk added 13 commits October 11, 2016 16:49

Implement import / export dashboard functionality.

d9c2b77

Address comments from discussion.

0b9a9f3

Add function descriptions.

8d26061

Minor fixes

3a3e0d6

Fix tests for python 3.

0035fe3

Export datasources.

3e506bd

Implement tables import.

6a8ee22

Json.loads does not support trailing commas.

045dd1c

Improve alter_dict func

0cb1231

Resolve comments.

06c18b1

Refactor tests

c78a38a

Move params_dict and alter_params to the ImportMixin

784d885

Fix flask menues.

813b3a3

bkyryliuk merged commit 73cd2ea into apache:master Oct 12, 2016

dpgaspar mentioned this pull request Dec 9, 2019

Bump FAB to 2.2.1 #8793

Merged

12 tasks

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

		"""Testing export import functionality for dashboards"""

		examples_loaded = False

Import / export of the dashboards. #1197

Import / export of the dashboards. #1197

Conversation

bkyryliuk commented Sep 26, 2016 • edited Loading

ascott commented Sep 27, 2016

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

mistercrunch Oct 3, 2016 • 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

mistercrunch Oct 3, 2016 • 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

bkyryliuk commented Oct 7, 2016

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 Oct 11, 2016 • 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

Choose a reason for hiding this comment

mistercrunch commented Oct 11, 2016

bkyryliuk commented Oct 12, 2016

bkyryliuk commented Sep 26, 2016 •

edited

Loading

mistercrunch Oct 3, 2016 •

edited

Loading

mistercrunch Oct 3, 2016 •

edited

Loading

bkyryliuk Oct 11, 2016 •

edited

Loading