[migration] add unique constraint on dashboard_slices table #7880

graceguo-supercat · 2019-07-16T19:53:49Z

SUMMARY

Superset didn't have unique constraint on dashboard_sliced table. In dashboard front-end code and when saving dashboard data to db, we added logic to prevent same slice id added into dashboard, but still found some duplicated entries in the dashboard_slices table.

When user tried to remove the duplicated slice_id from dashboard, they will see following error, and they cannot remove that duplicated slice:

Solution:

remove duplicated entries in many-to-many relation tbl dashboard_slices
add unique constraint
update the dashboard_slices model to include the uniqueness constraint

TEST PLAN

run following query from database, should get 0 row:

SELECT dashboard_id, slice_id, count(*)
from dashboard_slices
group by dashboard_id, slice_id
having count(*) > 1

mysql> SHOW CREATE TABLE dashboard_slices;

| dashboard_slices | CREATE TABLE `dashboard_slices` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `dashboard_id` int(11) DEFAULT NULL,
  `slice_id` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`),
  UNIQUE KEY `uq_dashboard_slice` (`dashboard_id`,`slice_id`),
  KEY `dashboard_id` (`dashboard_id`),
  KEY `slice_id` (`slice_id`),
  CONSTRAINT `dashboard_slices_ibfk_1` FOREIGN KEY (`dashboard_id`) REFERENCES `dashboards` (`id`),
  CONSTRAINT `dashboard_slices_ibfk_2` FOREIGN KEY (`slice_id`) REFERENCES `slices` (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=180250 DEFAULT CHARSET=latin1 |

ADDITIONAL INFORMATION

REVIEWERS

@mistercrunch @john-bodley @michellethomas @etr2460

codecov-io · 2019-07-16T20:03:17Z

Codecov Report

Merging #7880 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7880   +/-   ##
=======================================
  Coverage   65.83%   65.83%           
=======================================
  Files         461      461           
  Lines       22210    22210           
  Branches     2425     2425           
=======================================
  Hits        14621    14621           
  Misses       7468     7468           
  Partials      121      121

Impacted Files	Coverage Δ
superset/models/core.py	`82.95% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92eed8a...5357679. Read the comment docs.

mistercrunch · 2019-07-16T20:56:50Z

superset/migrations/versions/190188938582_adding_unique_constraint_on_dashboard_slices_tbl.py

+            "uq_dashboard_slice", "dashboard_slices", ["dashboard_id", "slice_id"]
+        )
+    except Exception:
+        pass


Maybe a logging.error / logging.exception(e)?

john-bodley · 2019-07-16T22:05:34Z

@graceguo-supercat you'll have to update the model to include the uniqueness constraint as well.

graceguo-supercat · 2019-07-17T21:42:58Z

@mistercrunch This PR always failed at a postgres unit test, even after i rebased onto master branch. But i am not sure this is related to my code change. Do you have a clue why it is failing? thanks!

======================================================================
FAIL: test_export_2_dashboards (tests.import_export_tests.ImportExportTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/apache/incubator-superset/tests/import_export_tests.py", line 259, in test_export_2_dashboards
    self.assert_dash_equals(world_health_dash, exported_dashboards[1])
  File "/home/travis/build/apache/incubator-superset/tests/import_export_tests.py", line 151, in assert_dash_equals
    self.assert_slice_equals(e_slc, a_slc)
  File "/home/travis/build/apache/incubator-superset/tests/import_export_tests.py", line 190, in assert_slice_equals
    self.assertEquals(expected_slc.viz_type, actual_slc.viz_type)
AssertionError: 'big_number' != 'filter_box'
- big_number
+ filter_box
    """Fail immediately, with the given message."""
>>  raise self.failureException("'big_number' != 'filter_box'\n- big_number\n+ filter_box\n")

-------------------- >> begin captured logging << --------------------

graceguo-supercat · 2019-07-18T00:58:59Z

trying to fix flaky unit test in #7898.

graceguo-supercat · 2019-07-19T22:52:18Z

after rebased onto #7898, no flaky failure any more!
ping @john-bodley

john-bodley · 2019-07-22T18:31:11Z

superset/migrations/versions/190188938582_adding_unique_constraint_on_dashboard_slices_tbl.py

+
+def downgrade():
+    try:
+        op.drop_constraint(u"uq_dashboard_slice", "dashboard_slices", type_="unique")


Nit. You don't need the u prefix here.

See the note above.

john-bodley · 2019-07-22T18:34:02Z

superset/migrations/versions/190188938582_adding_unique_constraint_on_dashboard_slices_tbl.py

+        )
+
+    # add unique constraint
+    try:


@graceguo-supercat if you use the batch_alter_table context manager (see here for an example) you shouldn't need to special case the the SQLite logic.

fix as suggested.

1. remove duplicated entries in many-to-many relation tbl dashboard_slices 2. add unique constraint on tbl 3. update the model to include the uniqueness constraint

) 1. remove duplicated entries in many-to-many relation tbl dashboard_slices 2. add unique constraint on tbl 3. update the model to include the uniqueness constraint

pull-request-size bot added the size/M label Jul 16, 2019

graceguo-supercat force-pushed the gg-AddUniqueConstraintDashboardSlicesTable branch from 3044d05 to 0e1f0b7 Compare July 16, 2019 19:57

mistercrunch reviewed Jul 16, 2019

View reviewed changes

graceguo-supercat force-pushed the gg-AddUniqueConstraintDashboardSlicesTable branch 2 times, most recently from a9f343e to dfc95ef Compare July 16, 2019 21:15

graceguo-supercat requested a review from mistercrunch July 16, 2019 21:42

graceguo-supercat force-pushed the gg-AddUniqueConstraintDashboardSlicesTable branch 3 times, most recently from 0b3eff4 to 5d5f49f Compare July 17, 2019 01:01

pull-request-size bot added size/L and removed size/M labels Jul 17, 2019

graceguo-supercat force-pushed the gg-AddUniqueConstraintDashboardSlicesTable branch from 5d5f49f to 6dd187a Compare July 17, 2019 21:22

graceguo-supercat force-pushed the gg-AddUniqueConstraintDashboardSlicesTable branch from 6dd187a to 3d4eda2 Compare July 19, 2019 22:39

graceguo-supercat requested a review from john-bodley July 22, 2019 18:23

john-bodley reviewed Jul 22, 2019

View reviewed changes

graceguo-supercat force-pushed the gg-AddUniqueConstraintDashboardSlicesTable branch from 3d4eda2 to d24b59f Compare July 22, 2019 22:32

[migration] add unique constraint on dashboard_slices table

5357679

1. remove duplicated entries in many-to-many relation tbl dashboard_slices 2. add unique constraint on tbl 3. update the model to include the uniqueness constraint

graceguo-supercat force-pushed the gg-AddUniqueConstraintDashboardSlicesTable branch from d24b59f to 5357679 Compare July 22, 2019 22:41

john-bodley approved these changes Jul 22, 2019

View reviewed changes

graceguo-supercat merged commit 9dd6a38 into apache:master Jul 22, 2019

graceguo-supercat deleted the gg-AddUniqueConstraintDashboardSlicesTable branch August 8, 2019 20:38

Carolinewly mentioned this pull request Jun 12, 2023

[Snyk] Upgrade deck.gl from 8.5.2 to 8.9.15 Carolinewly/superset#1

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[migration] add unique constraint on dashboard_slices table #7880

[migration] add unique constraint on dashboard_slices table #7880

graceguo-supercat commented Jul 16, 2019 •

edited

Loading

codecov-io commented Jul 16, 2019 •

edited

Loading

mistercrunch Jul 16, 2019

graceguo-supercat Jul 19, 2019

john-bodley commented Jul 16, 2019

graceguo-supercat commented Jul 17, 2019 •

edited

Loading

graceguo-supercat commented Jul 18, 2019 •

edited

Loading

graceguo-supercat commented Jul 19, 2019 •

edited

Loading

john-bodley Jul 22, 2019

john-bodley Jul 22, 2019

john-bodley Jul 22, 2019

graceguo-supercat Jul 22, 2019

[migration] add unique constraint on dashboard_slices table #7880

[migration] add unique constraint on dashboard_slices table #7880

Conversation

graceguo-supercat commented Jul 16, 2019 • edited Loading

CATEGORY

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

codecov-io commented Jul 16, 2019 • edited Loading

Codecov Report

mistercrunch Jul 16, 2019

Choose a reason for hiding this comment

graceguo-supercat Jul 19, 2019

Choose a reason for hiding this comment

john-bodley commented Jul 16, 2019

graceguo-supercat commented Jul 17, 2019 • edited Loading

graceguo-supercat commented Jul 18, 2019 • edited Loading

graceguo-supercat commented Jul 19, 2019 • edited Loading

john-bodley Jul 22, 2019

Choose a reason for hiding this comment

john-bodley Jul 22, 2019

Choose a reason for hiding this comment

john-bodley Jul 22, 2019

Choose a reason for hiding this comment

graceguo-supercat Jul 22, 2019

Choose a reason for hiding this comment

graceguo-supercat commented Jul 16, 2019 •

edited

Loading

codecov-io commented Jul 16, 2019 •

edited

Loading

graceguo-supercat commented Jul 17, 2019 •

edited

Loading

graceguo-supercat commented Jul 18, 2019 •

edited

Loading

graceguo-supercat commented Jul 19, 2019 •

edited

Loading