EnxDev commented on PR #44307: URL: https://github.com/apache/superset/pull/44307#issuecomment-5977924946
## EnxDev's Review Agent โ apache/superset#44307 ยท HEAD af01838 **comment**: the core fix is correct, and the empty-slug gap from my earlier review is closed. One edge case now shows an OVERWRITE prompt that can never succeed. The same silent merge is also still possible through `/api/v1/assets/import/`. The `slug is None` guard is in place. I confirmed that `test_import_empty_slug_collision_is_flagged` fails (`DID NOT RAISE`) with the old `not slug` guard and passes on this head. CI is green. ### ๐ด Functional - **`superset/commands/dashboard/importers/v1/__init__.py:114`** ยท _High_: the slug branch fires even when the config's UUID already belongs to a dashboard. `import_dashboard()` only resolves by slug when `find_existing_for_import()` returns nothing (`if not existing and ...`). Take a config whose UUID is a **soft-deleted** dashboard and whose slug is now held by another active dashboard. The gate returns "already exists" and the ImportModal shows OVERWRITE. Once the user confirms, the restore path raises "Dashboard cannot be restored via re-import because its slug 'contested-slug' is now used by another active dashboard". The prompt leads nowhere. Before this PR, the same readable error came straight away. I reproduced this at this head with a unit test. Could we mirror the import precondition and `continue` when `find_existing_for_import(Dashboard, config["uuid"]) is not None`? With that guard, all 50 importer tests (including the 5 new ones) still pass locally. **regression te st:** seed a soft-deleted dashboard that has the config UUID, plus an active owner of the slug; assert that `validate()` with `overwrite=False` does not raise. ### ๐ก Should-fix - **`superset/commands/dashboard/importers/v1/__init__.py:124`**: when the UUID matches active dashboard A and the slug is owned by active dashboard B, the same file gets two "already exists" errors (reproduced: `len(_exceptions) == 2`). That contradicts the "no double reporting" claim in the description, and `test_import_slug_collision_same_uuid_not_flagged` only covers the case where the slug owner has the config UUID. The guard above fixes this as well. Worth adding an A/B test case. - **`superset/commands/importers/v1/assets.py:296`**: `_prevent_overwrite_existing_assets` still checks UUIDs only and calls the same `import_dashboard()`. So `/api/v1/assets/import/` with `overwrite=false` still silently merges a dashboard with a fresh UUID and a colliding slug into the slug owner. The description scopes out "other importers", but this one imports dashboards. Could we fix it here, or track it in a linked follow-up issue? ### ๐ต Nits - `superset/commands/dashboard/importers/v1/__init__.py:104`: "soft-deleted owners are left to the restore path" is inaccurate. The restore path only runs on a UUID match. A config with a fresh UUID whose slug matches only a soft-deleted dashboard creates a new dashboard. Reword. ### ๐ Praise - `tests/unit_tests/dashboards/commands/importers/v1/import_test.py`: the assertions pin the exact string that `isAlreadyExists()` matches, so the test protects the frontend contract and not just "something raised". <!-- enxdev-review-agent:af01838 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD af01838._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
