fix: Allow chart import to update the dataset an existing chart points to

[jfrag1]

SUMMARY

Currently, attempting to overwrite a chart via import fails when the chart in the import file points to a dataset other than the one the chart currently points at.

This is because of the following filter which gets added to the query that checks if the chart being imported already exists:

filters.extend([getattr(cls, k) == dict_rep.get(k) for k in parent_refs.keys()])

which for charts evalulates to

filters.extend([Slice.datasource_id == dict_rep.get("datasource_id")])

When the chart's dataset has been changed, the query fails to find the chart even if a chart with the same UUID exists. It then attempts to create the chart since it thinks it doesn't exist, which obviously fails the UUID uniqueness constraint.

This PR allows for charts to be "re-parented" to another dataset on import by removing this filter for chart imports, which can be useful when importing and exporting assets between environments after making changes, or for managing assets as code.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

1. Have a database connection with a dataset
2. Create a virtual dataset identical to the dataset (SELECT * FROM <physical_dataset>)
3. Create a chart for each dataset
4. Do a bulk export with both of these charts
5. Extract the export file
6. Navigate to the charts folder and swap the dataset_uuid values for the charts
7. Compress the file
8. Import it back into the Superset instance
9. See the charts have swapped datasets

ADDITIONAL INFORMATION

• Has associated issue: Import failed on duplicated key when importing existing chart that its dataset was changed #22506
• 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

[codecov]

Codecov Report

Merging #24821 (3ef4ddf) into master (ba508a7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 3ef4ddf differs from pull request most recent head 0d14ddf. Consider uploading reports for the commit 0d14ddf to get more accurate results

@@           Coverage Diff           @@
##           master   #24821   +/-   ##
=======================================
  Coverage   68.98%   68.98%           
=======================================
  Files        1903     1903           
  Lines       74007    74008    +1     
  Branches     8193     8193           
=======================================
+ Hits        51052    51053    +1     
  Misses      20834    20834           
  Partials     2121     2121           

Flag	Coverage Δ
hive	54.16% <20.00%> (-0.01%)	⬇️
mysql	79.19% <100.00%> (+<0.01%)	⬆️
postgres	79.29% <100.00%> (+<0.01%)	⬆️
presto	54.06% <20.00%> (-0.01%)	⬇️
python	83.35% <100.00%> (+<0.01%)	⬆️
sqlite	77.86% <100.00%> (+<0.01%)	⬆️
unit	54.95% <100.00%> (+<0.01%)	⬆️

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

Files Changed	Coverage Δ
superset/charts/commands/importers/v1/utils.py	91.66% <100.00%> (ø)
superset/models/helpers.py	70.78% <100.00%> (+0.03%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Vitor-Avila]

This is awesome! Thanks!!!

[michael-s-molina]

@jfrag1 I saw yesterday that someone publish a SIP about this topic. Should we wait for the SIP before making this change? In the SIP, they are proposing to require explicit confirmation from the user before overriding associated assets. Is this PR going to conflict with the SIP?

[rusackas]

The SIP not only proposes the UI to allow choice in the cascade of overrides, but also having a checkbox or similar introduces a place to check for or add ownership/RBAC constraints. I'll add this comment (and others) to the sip as well.

[jfrag1]

@jfrag1 I saw yesterday that someone publish a SIP about this topic. Should we wait for the SIP before making this change? In the SIP, they are proposing to require explicit confirmation from the user before overriding associated assets. Is this PR going to conflict with the SIP?

I don't believe this PR is in conflict with that SIP, or that this functionality is relevant to that SIP. This isn't doing any cascading updates to child objects, it simply allows pointing a chart at a different dataset than it currently does. Also note that there is already an explicit confirmation that has to be performed when overwriting an asset via import

[Vitor-Avila]

I think this would only conflict with the SIP in case the chart dataset update also happens when importing a dashboard. Currently the overwrite check is only "applied" to the actual asset being imported -- for example, if you import a dashboard with charts, the dashboard will be overwritten but charts wouldn't. So if this change is only applied when importing a chart directly (or via the /api/v1/assets/import/ endpoint) I don't think it conflicts.

[betodealmeida]

LGTM!

@michael-s-molina this is not directly related to the SIP you mentioned; it's not doing any cascade imports but just point the chart to a different dataset (which could be new or existing).

[michael-s-molina]

Got it. Thanks @jfrag1 @Vitor-Avila and @betodealmeida for the additional context.

[SharonCastel]

@jfrag1 finally someone who understands the issue and know how to fix it! You are the best! Can't wait for it...