fix(dashboard-export): Fixes datasetId is not replaced with datasetUuid in Dashboard export in 4.1.x

[fmannhardt]

SUMMARY

Fixes #30424 by duplicating the code that rewrites the native_filter_configuration to use datasetUuid instead of datasetId into the _file_content method of ExportDashboardsCommand.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before the targets attribute of native_filter_configuration items includes datasetId:

    targets:
    - datasetId: 17
      column:
        name: column_name

After it just has datasetUuid:

    targets:
    - column:
        name: column_name
      datasetUuid: 7ac464dc-315a-4c9e-b9ec-048d7bbcf382```

TESTING INSTRUCTIONS

(1) Create a dashboard with any chart
(2) Add a single value filter for any column
(3) Export the dashboard
(4) Verify that datasetUuid is used to refer to the dataset and not datasetId

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

[codecov]

Codecov Report

Attention: Patch coverage is 21.42857% with 11 lines in your changes missing coverage. Please review.

Project coverage is 83.92%. Comparing base (76d897e) to head (f8171fd).
Report is 1094 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/commands/dashboard/export.py	21.42%	11 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #30425       +/-   ##
===========================================
+ Coverage   60.48%   83.92%   +23.43%     
===========================================
  Files        1931      533     -1398     
  Lines       76236    38546    -37690     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32348    -13766     
+ Misses      28017     6198    -21819     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.99% <0.00%> (-0.17%)	⬇️
javascript	?
mysql	76.79% <21.42%> (?)
postgres	76.89% <21.42%> (?)
presto	53.48% <0.00%> (-0.32%)	⬇️
python	83.92% <21.42%> (+20.43%)	⬆️
sqlite	76.33% <21.42%> (?)
unit	60.71% <0.00%> (+3.08%)	⬆️

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.

[mistercrunch]

Could you write a unit test that would fail before and succeed after? That will help review the logic by stating the assumptions in a unit test.

[fmannhardt]

Happy to do so. Since I have no clue of the testing approach of Superset, could you help me find the correct spot to test? Would the correct file be:

superset/tests/unit_tests/commands/export_test.py

Line 62 in cc9fd88

ExportDashboardsCommand.return_value.run.return_value = [

or rather an integration test (since this also concerns code that also exports other resources:

superset/tests/integration_tests/dashboards/commands_tests.py

Line 67 in cc9fd88

class TestExportDashboardsCommand(SupersetTestCase):

[fmannhardt]

After some more searching, the example data used in the tests comes from here:
https://github.com/apache/superset/blob/master/superset/examples/world_bank.py
and are manually created via code right? Or is there somewhere examples that exists as YAML files using the import/export feature?

Searching for native in the examples folder does not find anything:
https://github.com/search?q=repo%3Aapache%2Fsuperset+path%3A%2F%5Esuperset%5C%2Fexamples%5C%2F%2F+native&type=code

So, I would need to write code to create a dashboard with native filters from scratch? Or would there be a simpler way to write such test? I would need some guidance.

[sadpandajoe]

@fmannhardt I created a PR to the covid vaccine example dashboard. I'm guessing you can probably add a test here: https://github.com/apache/superset/blob/master/tests/integration_tests/dashboards/commands_tests.py#L67 or update one of the tests to make sure when exporting native filter meta data is there.

[Vitor-Avila]

Hey @fmannhardt,

Thanks for working on this fix! 🙏

Looking at the code changes, I noticed you have re-introduced that code block to file_content, but it also remains present in the _export method. Is it required to have this block in both? For instance I see the dataset_id = target.pop("datasetId", None) in both blocks.

If we need them in both places, should we consider to move this logic to a helper/etc method to make it DRYer?

[fmannhardt]

@Vitor-Avila I think it is required in both places to also invoke the export of the related datasets via yield from ExportDatasetsCommand([dataset_id]).run(). I agree that a refactoring would make sense. However, then there are also other parts that are duplicated such as:

        payload = model.export_to_dict(
            recursive=False,
            include_parent_ref=False,
            include_defaults=True,
            export_uuids=True,

It seems to me that a completely different approach to separate the export to YAML/JSON and walking the dependency graph of the to be exported dashboard would be better. Maybe, first collecting all the dependencies and then going through the list of dependencies and exporting them. However, that would exceed the capacity that I have for contributing right now (and I am also not aware of any design decision that led to the current approach).

I will try to add the test in the next few days.

[sadpandajoe]

@fmannhardt @Vitor-Avila as this would be a regression for 4.1, let's I'd advise that we just add the tests and then if we need to refactor, refactor in a separate pr.

[fmannhardt]

It seems that I also need to write a fixture for the COVID dashboard, right? Bare with me, still exploring the setup of Superset testing and locally many other tests fail probably due to missing dependencies. I will try to look at this tomorrow.

[villebro]

@fmannhardt thanks for working on this super critical regression. I will be reviewing this today and do my best to help unblock you.

[villebro]

Note: this is a fix for a regression that was introduced by #26765

[villebro]

It seems that I also need to write a fixture for the COVID dashboard, right? Bare with me, still exploring the setup of Superset testing and locally many other tests fail probably due to missing dependencies. I will try to look at this tomorrow.

@fmannhardt you are correct - if there isn't a fixture that creates this dashboard + related assets, then you will need to add one. Let me know if you need help with this.

[fmannhardt]

Happy to help and thanks for the work on Superset. I may have time to look at this tomorrow (UTC+1). So, the existing fixtures hard code the dashboard and do not load from the examples? Just so I understand how to write it. It would be a copy of world_bank_dashboard.py but then the COVID dashboard code in there?

In case, it is easy for you to add the fixture I am also happy to update the test I wrote to work against it.

[villebro]

Happy to help and thanks for the work on Superset. I may have time to look at this tomorrow (UTC+1). So, the existing fixtures hard code the dashboard and do not load from the examples? Just so I understand how to write it. It would be a copy of world_bank_dashboard.py but then the COVID dashboard code in there?

In case, it is easy for you to add the fixture I am also happy to update the test I wrote to work against it.

@fmannhardt let me take a look how it's been implemented (my memory is rusty on these particular fixtures)..

[villebro]

@fmannhardt I took a quick look at that particular fixture, and it seems to be mostly reusing existing logic. However, the data seems to be randomly generated for some reason. I'm not sure why, but maybe it's to avoid unnecessary web traffic. Please keep an open mind while working on the fixture, and if you feel some aspect could be improved compared to the World Bank fixture, then please by all means feel free to implement accordingly. I'm in UTC-0700, so I can help/review tomorrow morning my time if needed, likely after you've had a chance to take a stab at it first.

[fmannhardt]

@villebro I took another look but this would take me much more time to understand. I don't even need to data for the test. Only the have the COVID dashboard and its dataset loaded and available to be queries by UUID f4065089-110a-41fa-8dd7-9ce98a65e250. It is probably trivial but would take me several hours. So, it would take some more days. Please feel free to add this.

[villebro]

@fmannhardt I totally understand. Feel free to disable the added test - I'll open a PR that adds the necessary fixtures and re-enables the test.

[fmannhardt]

Test is disabled and current master branch is merged.

[villebro]

LGTM, leaving open for another day so others can chime in. Note: I'll finalize the tests this week.

[villebro]

At the risk of introducing yet more regressions, I feel it would be cleaner to do the following:

if export_related:
    for target in native_filter...

The point being, no sense in iterating over all datasets, if export_related is False, as we're not doing anything else here (look at how charts are exported above).

[fmannhardt]

Done. Since this is now only to invoke the dataset export, it is fine to have the condition before iterating.

[sadpandajoe]

/testenv up

[github-actions]

@sadpandajoe Ephemeral environment spinning up at http://35.167.172.107:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[sadpandajoe]

Spun up an ephemeral and verified import/export of native filters are working so going to approve

[villebro]

I'm merging now, will follow up with those tests.

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.