fix: Remedy logic for UpdateDatasetCommand uniqueness check #28341

john-bodley · 2024-05-04T03:28:44Z

SUMMARY

At Airbnb we detected a number of duplicate datasets even though, per the UpdateDatasetCommand.validate() method, there is a check to ensure that the new dataset name is unique. The issue was that it wrongfully used the schema of the existing (source) table as opposed to the target schema.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Added unit tests.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

john-bodley · 2024-05-04T03:29:32Z

superset/commands/dataset/create.py


        # Validate uniqueness
        if not DatasetDAO.validate_uniqueness(database_id, table):
-            exceptions.append(DatasetExistsValidationError(table_name))
+            exceptions.append(DatasetExistsValidationError(table))


We should be using a Table object rather than the table name which is too terse, i.e., doesn't contain the schema or catalog.

john-bodley · 2024-05-04T03:30:24Z

superset/commands/dataset/update.py

        # Validate uniqueness
        if not DatasetDAO.validate_update_uniqueness(
            self._model.database_id,
-            Table(table_name, self._model.schema, self._model.catalog),


Here's the bug. The schema should be the schema defined in the properties and not the schema associated with the model.

john-bodley · 2024-05-04T03:32:04Z

tests/unit_tests/commands/dataset/test_update.py

@@ -0,0 +1,43 @@
+from unittest.mock import MagicMock


I was _shocked that there were zero unit or integration tests for the UpdateDatasetCommand.

Though this test isn't pretty, it's a start.

john-bodley · 2024-05-04T03:32:47Z

tests/unit_tests/dao/dataset_test.py

@@ -51,7 +51,7 @@ def test_validate_update_uniqueness(session: Session) -> None:
    db.session.add_all([database, dataset1, dataset2])
    db.session.flush()

-    # same table name, different schema


These comments were ordered wrongly (AFAICT) and thus misleading. It's probably best to not have comments if the logic is clear from the code.

codecov-commenter · 2024-05-04T03:38:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.21%. Comparing base (2e5f3ed) to head (debeea5).
Report is 38 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #28341       +/-   ##
===========================================
+ Coverage   60.49%   83.21%   +22.71%     
===========================================
  Files        1931      521     -1410     
  Lines       76241    37179    -39062     
  Branches     8566        0     -8566     
===========================================
- Hits        46122    30938    -15184     
+ Misses      28015     6241    -21774     
+ Partials     2104        0     -2104

Flag	Coverage Δ
hive	`49.11% <35.71%> (-0.05%)`	⬇️
javascript	`?`
mysql	`77.23% <100.00%> (?)`
postgres	`77.35% <100.00%> (?)`
presto	`53.71% <35.71%> (-0.10%)`	⬇️
python	`83.21% <100.00%> (+19.72%)`	⬆️
sqlite	`76.81% <100.00%> (?)`
unit	`58.23% <71.42%> (+0.60%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

john-bodley · 2024-05-04T06:27:39Z

tests/integration_tests/datasets/api_tests.py

-            "message": {"table_name": ["Dataset energy_usage already exists"]}
+            "message": {
+                "table": [
+                    f"Dataset {Table(energy_usage_ds.table_name, schema)} already exists"


The schema differs between SQLite and PostgreSQL and thus this is parameterized.

john-bodley · 2024-05-04T06:40:55Z

superset/commands/dataset/update.py

@@ -86,15 +86,21 @@ def validate(self) -> None:
        except SupersetSecurityException as ex:
            raise DatasetForbiddenError() from ex

-        database_id = self._properties.get("database", None)


The default for dict.get() is None and thus there's no need to explicitly define it.

john-bodley · 2024-05-04T06:41:13Z

tests/integration_tests/datasets/api_tests.py

-            "message": {"table_name": ["Dataset energy_usage already exists"]}
+            "message": {
+                "table": [
+                    f"Dataset {Table(energy_usage_ds.table_name, schema)} already exists"


See previous comment.

john-bodley · 2024-05-04T15:37:30Z

superset/commands/dataset/update.py

+        table = Table(
+            self._properties.get("table_name"),  # type: ignore
+            self._properties.get("schema"),
+            self._properties.get("catalog"),


This doesn't exist in the schema, though is consistent with other logic. See 6cf681d#r141673924 for details.

michael-s-molina · 2024-05-06T11:16:11Z

tests/unit_tests/dao/dataset_test.py

@@ -51,7 +51,7 @@ def test_validate_update_uniqueness(session: Session) -> None:
    db.session.add_all([database, dataset1, dataset2])
    db.session.flush()

-    # same table name, different schema
+    #


Suggested change

#

michael-s-molina · 2024-05-06T11:17:49Z

superset/commands/dataset/create.py

        schema = self._properties.get("schema")
        catalog = self._properties.get("catalog")
        sql = self._properties.get("sql")
        owner_ids: Optional[list[int]] = self._properties.get("owners")

-        table = Table(table_name, schema, catalog)
+        table = Table(self._properties["table_name"], schema, catalog)


I think the previous version is more readable as it groups variables definition in just one place.

Suggested change

table = Table(self._properties["table_name"], schema, catalog)

table = Table(table_name, schema, catalog)

@michael-s-molina personally I try to remove single use variables (where possible) as I find it easier to read/grok the code.

…8341)

pull-request-size bot added the size/M label May 4, 2024

john-bodley force-pushed the john-bodley--fix-update-dataset-command-uniqueness-check branch from 3b626e2 to 62ae9d2 Compare May 4, 2024 03:33

john-bodley commented May 4, 2024

View reviewed changes

john-bodley force-pushed the john-bodley--fix-update-dataset-command-uniqueness-check branch 2 times, most recently from 214d13d to 96217ed Compare May 4, 2024 04:28

pull-request-size bot added size/L and removed size/M labels May 4, 2024

john-bodley force-pushed the john-bodley--fix-update-dataset-command-uniqueness-check branch 6 times, most recently from 999fb95 to 39b95b0 Compare May 4, 2024 06:20

john-bodley commented May 4, 2024

View reviewed changes

john-bodley requested review from betodealmeida, dpgaspar and michael-s-molina May 4, 2024 06:28

john-bodley marked this pull request as ready for review May 4, 2024 06:39

john-bodley commented May 4, 2024

View reviewed changes

john-bodley force-pushed the john-bodley--fix-update-dataset-command-uniqueness-check branch from 39b95b0 to d90b0be Compare May 4, 2024 15:35

john-bodley commented May 4, 2024

View reviewed changes

michael-s-molina reviewed May 6, 2024

View reviewed changes

john-bodley force-pushed the john-bodley--fix-update-dataset-command-uniqueness-check branch 2 times, most recently from b77077f to f48e663 Compare May 6, 2024 18:39

fix: Remedy logic for UpdateDatasetCommand uniqueness check

debeea5

john-bodley force-pushed the john-bodley--fix-update-dataset-command-uniqueness-check branch from f48e663 to debeea5 Compare May 6, 2024 19:03

john-bodley requested a review from michael-s-molina May 6, 2024 23:13

michael-s-molina approved these changes May 7, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label May 7, 2024

dpgaspar approved these changes May 7, 2024

View reviewed changes

john-bodley merged commit 467e612 into apache:master May 7, 2024
29 checks passed

michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label May 8, 2024

michael-s-molina pushed a commit that referenced this pull request May 8, 2024

fix: Remedy logic for UpdateDatasetCommand uniqueness check (#28341)

dc0a97c

imancrsrk pushed a commit to imancrsrk/superset that referenced this pull request May 10, 2024

fix: Remedy logic for UpdateDatasetCommand uniqueness check (apache#2…

2991191

…8341)

jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024

fix: Remedy logic for UpdateDatasetCommand uniqueness check (apache#2…

ab9ec2d

…8341)

EnxDev pushed a commit to EnxDev/superset that referenced this pull request May 31, 2024

fix: Remedy logic for UpdateDatasetCommand uniqueness check (apache#2…

ed69676

…8341)

mistercrunch added 🍒 4.0.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 24, 2024

irublev pushed a commit to HighviewPower/superset that referenced this pull request Oct 29, 2024

fix: Remedy logic for UpdateDatasetCommand uniqueness check (apache#2…

6481936

…8341)

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix: Remedy logic for UpdateDatasetCommand uniqueness check (apache#2…

281d7f1

…8341)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Remedy logic for UpdateDatasetCommand uniqueness check #28341

fix: Remedy logic for UpdateDatasetCommand uniqueness check #28341

john-bodley commented May 4, 2024 •

edited

Loading

john-bodley May 4, 2024

john-bodley May 4, 2024

john-bodley May 4, 2024 •

edited

Loading

john-bodley May 4, 2024 •

edited

Loading

codecov-commenter commented May 4, 2024 •

edited

Loading

john-bodley May 4, 2024

john-bodley May 4, 2024

john-bodley May 4, 2024

john-bodley May 4, 2024

michael-s-molina May 6, 2024

john-bodley May 6, 2024

michael-s-molina May 6, 2024

john-bodley May 6, 2024

	table = Table(self._properties["table_name"], schema, catalog)
	table = Table(table_name, schema, catalog)

fix: Remedy logic for UpdateDatasetCommand uniqueness check #28341

fix: Remedy logic for UpdateDatasetCommand uniqueness check #28341

Conversation

john-bodley commented May 4, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley May 4, 2024 • edited Loading

Choose a reason for hiding this comment

john-bodley May 4, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented May 4, 2024 • edited Loading

Codecov Report

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

john-bodley commented May 4, 2024 •

edited

Loading

john-bodley May 4, 2024 •

edited

Loading

john-bodley May 4, 2024 •

edited

Loading

codecov-commenter commented May 4, 2024 •

edited

Loading