feat(api): Add option to enable sync on import

[reesercollins]

SUMMARY

In order to bring the API more in line with the CLI, I have added the ability to specify sync_columns and sync_metrics when using the API.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Set sync_columns or sync_metrics to true. The API should behave like the CLI when those flags are set.

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

Merging #20312 (225521a) into master (f1fbaf8) will decrease coverage by 0.35%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #20312      +/-   ##
==========================================
- Coverage   66.82%   66.47%   -0.36%     
==========================================
  Files        1799     1763      -36     
  Lines       68884    67720    -1164     
  Branches     7319     7052     -267     
==========================================
- Hits        46031    45014    -1017     
+ Misses      20966    20907      -59     
+ Partials     1887     1799      -88     

Flag	Coverage Δ
hive	52.91% <0.00%> (-0.01%)	⬇️
mysql	78.25% <100.00%> (?)
postgres	78.31% <100.00%> (+<0.01%)	⬆️
presto	52.81% <0.00%> (-0.01%)	⬇️
python	81.45% <100.00%> (+0.04%)	⬆️
sqlite	76.79% <0.00%> (-0.01%)	⬇️
unit	50.95% <0.00%> (-0.01%)	⬇️

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

Impacted Files	Coverage Δ
superset/datasets/api.py	87.39% <100.00%> (+0.10%)	⬆️
superset-frontend/src/setup/setupFormatters.ts	0.00% <0.00%> (-100.00%)	⬇️
...-chart-table/src/DataTable/utils/useMountedMemo.ts	0.00% <0.00%> (-87.50%)	⬇️
.../shared-controls/components/RadioButtonControl.tsx	0.00% <0.00%> (-83.34%)	⬇️
...hart-table/src/DataTable/utils/getScrollBarSize.ts	7.14% <0.00%> (-78.58%)	⬇️
...t-frontend/src/components/DropdownButton/index.tsx	10.00% <0.00%> (-75.00%)	⬇️
superset-frontend/src/setup/setupClient.ts	0.00% <0.00%> (-70.00%)	⬇️
...qlLab/components/EstimateQueryCostButton/index.tsx	5.26% <0.00%> (-58.38%)	⬇️
...ugin-chart-table/src/DataTable/hooks/useSticky.tsx	4.21% <0.00%> (-47.37%)	⬇️
superset-frontend/src/preamble.ts	0.00% <0.00%> (-44.00%)	⬇️
... and 324 more

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

[eschutho]

Side note, I see that V0.ImportDatasetsCommand will use the sync kwargs but not V1.

[eschutho]

cc @betodealmeida is that intentional? ☝️ Or do we need to port over the sync functionality to V1 as well?

[reesercollins]

It appears that V1 uses the overwrite flag to denote syncing columns and metrics.

[betodealmeida]

I'm a bit confused here... passing sync_columns and sync_metrics to ImportDatasetsCommand has no effect, as far as I can see. Can you explain more the use case for this, @reesercollins?

[reesercollins]

sync_columns and sync_metrics means the import command will remove columns and metrics which don't appear in the dataset YAML.

superset/superset/datasets/commands/importers/v0.py

Lines 249 to 252 in 76d6a9a

	if kwargs.get("sync_columns"):
	self.sync.append("columns")
	if kwargs.get("sync_metrics"):
	self.sync.append("metrics")

[betodealmeida]

Ah, that makes sense, it's for the v0 API! Yeah, in this case it should be fine... the v1 API will ignore these extra arguments, and should be fine.

But then my next question is... any reason why you're still using the v0 API, instead of enabling the VERSIONED_EXPORT feature flag? The v0 import IMHO is very buggy, the code is incomplete and non-standard across asset types, and we should deprecated it as soon as possible.

[reesercollins]

We have some automation we haven't gotten around to updating which generates the YAML files. We very rarely do any exporting.

[eschutho]

@reesercollins This looks good. Do you mind writing a test that validates that passing in this kwarg to the api will work? 🙏

[rusackas]

@eschutho looks like this might be ready to merge if it meets your standards :)

[eschutho]

Thanks @reesercollins!

[rusackas]

@reesercollins sorry, this slipped under my/our radar again 🤦
If you could rebase/resolve the conflicts, we'll merge the thing pronto.