feat: add database dropdown to dashboard import #10118

mistercrunch · 2020-06-20T15:46:23Z

Currently, when importing a database from a JSON file, the process
looks at the database name from the source (the info is in the file)
and matches the datasources to that name. If no database by that name
exists, it simply fails.

With this PR, we add a database dropdown that allows the user to specify
which databases the datasources should target as the get upserted.

I want to stress that the code in this area is not in a great shape,
and that the challenge of serializing/deser the nested objects is
challenging, but that there should be a much better way to do this.
One of the improvement (out of scope for this PR) that would allow to
simplify those import/export would be to use UUIDs for
importable/exportable objects.

Another identified issue is the indirections between
utils/import_expor_{model}.py on top of {Model}.import_object. Not
addressing that here.

Next topic is the MVC stuff. Decided to stick with it for now as this is
more of a [obious missing feat:] than a rewrite.

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

mistercrunch · 2020-06-20T15:48:12Z

superset-frontend/src/components/FlashProvider.tsx

@@ -42,7 +43,9 @@ class FlashProvider extends React.PureComponent<Props> {
    flashMessages.forEach(message => {
      const [type, text] = message;
      const flash = flashObj[type];
-      this.props[flash](text);
+      if (this.props[flash]) {


Caught this bug as bycatch, this fails for the commonly used "alert" flashes.

mistercrunch · 2020-06-20T15:50:44Z

superset/models/dashboard.py

@@ -311,6 +311,10 @@ def alter_positions(
        # copy slices object as Slice.import_slice will mutate the slice
        # and will remove the existing dashboard - slice association
        slices = copy(dashboard_to_import.slices)
+
+        # Clearing the slug to avoid conflicts
+        dashboard_to_import.slug = None


This was raising when loading the same dashboard (with a slug) twice. Currently the authority on whether to insert or update is the remote_id and didn't want to tangle the slug into that. Imported dashboards loose their slugs. We could lookup whether there will be a conflict or not and keep the slug / suffix it, but for now I'm just fixing this bug.

Removing the slug might be unexpected behavior for the end user. I wonder if we should raise an alert toast if the slug has been removed until we figure out a more permanent solution?

So let's say I raise Slug already exists! user should do what? Go to change the slug from the source and re-export? I feel like we'd need to wizard that up and be like "Want to overwrite or create a new one?". Currently it'd be hard to do that.

I guess the form could offer "overwrite based on slug" checkbox or similar... ?

TBH this is one of those things where it'd be really valuable to get usage statistics on how much slugs are used, how often slugged dashboards are exported + the slug is expected to work upon import. My guess is not so often, but who knows. But for the time being I'm comfortable with clearing the slug to avoid getting errors on repeated imports (idempotency FTW).

superset/models/dashboard.py

mistercrunch

Some PR notes

superset/utils/log.py

eugeniamz · 2020-06-20T23:47:43Z

Do you think that doing this will resolve the issue that if Dashboard to import is using legacy Druid Driver?

mistercrunch · 2020-06-21T00:59:24Z

@eugeniamz we're more heading towards deprecation at this time, I'd rather help people migrate off the legacy connector and onto SQL druid than investing in that direction.

In theory we could make the dropdown include databases + druid clusters, but that's one more thing to untangle/deprecate...

villebro

A few comments.

superset/connectors/sqla/models.py

villebro · 2020-06-22T05:32:12Z

superset/models/dashboard.py

@@ -311,6 +311,10 @@ def alter_positions(
        # copy slices object as Slice.import_slice will mutate the slice
        # and will remove the existing dashboard - slice association
        slices = copy(dashboard_to_import.slices)
+
+        # Clearing the slug to avoid conflicts
+        dashboard_to_import.slug = None


Removing the slug might be unexpected behavior for the end user. I wonder if we should raise an alert toast if the slug has been removed until we figure out a more permanent solution?

dpgaspar · 2020-06-23T08:03:32Z

superset/utils/log.py

+            sesh = current_app.appbuilder.get_session
+            sesh.bulk_save_objects(logs)
+            sesh.commit()
+        except Exception as ex:  # pylint: disable=broad-except


except SQLAlchemyError as ex:

superset/views/core.py

mistercrunch · 2020-06-24T15:54:36Z

@dpgaspar @villebro , addressed most comments. Clearly export/import needs more attention, but this tackles important shortcoming of lacking the ability to target a database, and fixes the slug bug.

About next step, I think we'll need:

uuid as the authority for upsert logic
add a try-to-update VS insert new checkbox option
uuids relationships where possible so we don't need all this lookup logic!

villebro · 2020-06-24T16:03:07Z

superset/models/dashboard.py

@@ -311,6 +311,10 @@ def alter_positions(
        # copy slices object as Slice.import_slice will mutate the slice
        # and will remove the existing dashboard - slice association
        slices = copy(dashboard_to_import.slices)
+
+        # Clearing the slug to avoid conflicts
+        dashboard_to_import.slug = None


TBH this is one of those things where it'd be really valuable to get usage statistics on how much slugs are used, how often slugged dashboards are exported + the slug is expected to work upon import. My guess is not so often, but who knows. But for the time being I'm comfortable with clearing the slug to avoid getting errors on repeated imports (idempotency FTW).

willbarrett · 2020-06-24T21:10:06Z

FYI @mistercrunch this needs a rebase.

Currently, when importing a database from a JSON file, the process looks at the database name from the source (the info is in the file) and matches the datasources to that name. If no database by that name exists, it simply fails. With this PR, we add a database dropdown that allows the user to specify which databases the datasources should target as the get upserted. I want to stress that the code in this area is not in a great shape, and that the challenge of serializing/deser the nested objects is challenging, but that there should be a much better way to do this. One of the improvement (out of scope for this PR) that would allow to simplify those import/export would be to use UUIDs for importable/exportable objects. Another identified issue is the indirections between `utils/import_expor_{model}.py` on top of `{Model}.import_object`. Not addressing that here. Next topic is the MVC stuff. Decided to stick with it for now as this is more of a [obious missing feat:] than a rewrite.

* feat: add database dropdown to dashboard import Currently, when importing a database from a JSON file, the process looks at the database name from the source (the info is in the file) and matches the datasources to that name. If no database by that name exists, it simply fails. With this PR, we add a database dropdown that allows the user to specify which databases the datasources should target as the get upserted. I want to stress that the code in this area is not in a great shape, and that the challenge of serializing/deser the nested objects is challenging, but that there should be a much better way to do this. One of the improvement (out of scope for this PR) that would allow to simplify those import/export would be to use UUIDs for importable/exportable objects. Another identified issue is the indirections between `utils/import_expor_{model}.py` on top of `{Model}.import_object`. Not addressing that here. Next topic is the MVC stuff. Decided to stick with it for now as this is more of a [obious missing feat:] than a rewrite. * isort \!? 0%^$%Y$&?%$^?%0^? * fix tests * pre-committing to py3.6 * address dpgaspar's comments * revert isort

superset-github-bot bot added the preset-io label Jun 20, 2020

pull-request-size bot added the size/L label Jun 20, 2020

mistercrunch commented Jun 20, 2020

View reviewed changes

superset/models/dashboard.py Show resolved Hide resolved

mistercrunch force-pushed the import_dash branch from 052eae5 to 953d853 Compare June 20, 2020 15:55

mistercrunch commented Jun 20, 2020

View reviewed changes

superset/utils/log.py Show resolved Hide resolved

mistercrunch force-pushed the import_dash branch 4 times, most recently from 701e892 to 1005537 Compare June 20, 2020 19:35

mistercrunch marked this pull request as ready for review June 20, 2020 21:26

mistercrunch requested a review from villebro June 20, 2020 21:26

villebro reviewed Jun 22, 2020

View reviewed changes

dpgaspar reviewed Jun 23, 2020

View reviewed changes

superset/views/core.py Show resolved Hide resolved

villebro approved these changes Jun 24, 2020

View reviewed changes

willbarrett approved these changes Jun 24, 2020

View reviewed changes

mistercrunch added 5 commits July 5, 2020 14:33

isort \!? 0%^$%Y$&?%$^?%0^?

aa07e39

fix tests

ae8019c

pre-committing to py3.6

5687673

address dpgaspar's comments

8967e8c

mistercrunch added the v0.37 label Jul 5, 2020

mistercrunch force-pushed the import_dash branch from 3209429 to dadb346 Compare July 5, 2020 21:45

revert isort

8e4c873

mistercrunch force-pushed the import_dash branch from dadb346 to 8e4c873 Compare July 5, 2020 21:50

mistercrunch merged commit 2314aad into apache:master Jul 5, 2020

mistercrunch deleted the import_dash branch July 5, 2020 22:08

villebro mentioned this pull request Jul 30, 2020

dashboard slug is cleared on import #10479

Closed

3 tasks

betodealmeida mentioned this pull request Oct 6, 2020

[SIP-54] Proposal to improve export/import of objects #11167

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add database dropdown to dashboard import #10118

feat: add database dropdown to dashboard import #10118

mistercrunch commented Jun 20, 2020

mistercrunch Jun 20, 2020

mistercrunch Jun 20, 2020

villebro Jun 22, 2020

mistercrunch Jun 24, 2020

villebro Jun 24, 2020

mistercrunch left a comment

eugeniamz commented Jun 20, 2020

mistercrunch commented Jun 21, 2020

villebro left a comment

villebro Jun 22, 2020

dpgaspar Jun 23, 2020

mistercrunch commented Jun 24, 2020 •

edited

villebro Jun 24, 2020

willbarrett commented Jun 24, 2020

feat: add database dropdown to dashboard import #10118

feat: add database dropdown to dashboard import #10118

Conversation

mistercrunch commented Jun 20, 2020

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

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 left a comment

Choose a reason for hiding this comment

eugeniamz commented Jun 20, 2020

mistercrunch commented Jun 21, 2020

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Jun 24, 2020 • edited

Choose a reason for hiding this comment

willbarrett commented Jun 24, 2020

mistercrunch commented Jun 24, 2020 •

edited