codeant-ai-for-open-source[bot] commented on code in PR #40128:
URL: https://github.com/apache/superset/pull/40128#discussion_r3397959176
##########
tests/integration_tests/base_tests.py:
##########
@@ -594,10 +595,17 @@ def insert_dashboard(
role_obj = db.session.query(security_manager.role_model).get(role)
obj_roles.append(role_obj)
- # Defensive cleanup: remove any existing dashboard with the same slug
+ # Defensive cleanup: remove any existing dashboard with the same slug.
+ # Bypass the soft-delete visibility filter so a soft-deleted row from
+ # a prior test still gets cleared — without the bypass the lookup
+ # returns ``None`` and the INSERT below trips the unique constraint
+ # on ``slug`` against the soft-deleted (but hidden) row.
if slug:
existing_dashboard = (
Review Comment:
**Suggestion:** The cleanup query only deletes the first dashboard found for
a slug. With soft-delete enabled, multiple rows can share the same slug
(deleted + active), so deleting a single arbitrary row can leave an active
duplicate behind and make the next insert fail with a unique-slug constraint
error. Delete all matching rows (or explicitly the active one) before
inserting. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset drill_info tests fail with IntegrityError.
- ❌ Dashboard API integration tests flake on insert.
- ⚠️ Any helper reuse of slugs becomes brittle.
- ⚠️ Soft-delete scenarios harder to regression-test reliably.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure the dashboards soft-delete migration is applied on a
partial-index-capable
backend (PostgreSQL/MySQL 8+), as described in
`superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py:17-45`,
which explicitly allows an active row and a soft-deleted row to share the
same `slug`.
2. In a Superset shell or test, create a first dashboard with a slug used by
tests, for
example:
- Use the `Dashboard` model defined in
`superset/models/dashboard.py:135-155` to create
`dash1 = Dashboard(dashboard_title="Embedded Test Dashboard",
slug="embedded-test-dashboard")`, then `db.session.add(dash1);
db.session.commit()`.
- Soft-delete this dashboard via the `SoftDeleteMixin.soft_delete()`
method implemented
in `superset/models/helpers.py:59-65` by calling `dash1.soft_delete();
db.session.commit()`, so `dash1.deleted_at` is non-null.
3. Create a second, active dashboard with the same slug:
- Again use `Dashboard` (same file/lines as above) to create `dash2 =
Dashboard(dashboard_title="Active Embedded Test Dashboard",
slug="embedded-test-dashboard")`, then `db.session.add(dash2);
db.session.commit()`.
- This is permitted because the partial unique index defined in
`9e1f3b8c4d2a_add_deleted_at_to_dashboards.py:119-151` enforces
uniqueness only for
rows where `deleted_at IS NULL`, so one soft-deleted and one active row
can share the
slug.
4. Invoke the `insert_dashboard` helper that contains the cleanup logic:
- `insert_dashboard` is defined in
`tests/integration_tests/base_tests.py:54-111` and
called from multiple integration tests such as `TestDatasetApi` in
`tests/integration_tests/datasets/api_tests.py:3190-3568` (e.g. lines
3190-3195,
3236-3264, 3351-3590).
- Call `self.insert_dashboard("Embedded Test Dashboard",
"embedded-test-dashboard", [],
slices=[chart])` from a test method.
- Inside `insert_dashboard`, the cleanup block at
`tests/integration_tests/base_tests.py:79-93` runs:
- It executes
`db.session.query(Dashboard).execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES:
{Dashboard}}).filter_by(slug=slug).first()`.
- Because there are two rows (`dash1` soft-deleted, `dash2` active),
`.first()`
returns an arbitrary one, which can be `dash1` (the older row)
depending on query
ordering.
- Only this single `existing_dashboard` is deleted via
`db.session.delete(existing_dashboard); db.session.commit()`, leaving
the active
`dash2` with the same slug in place.
- `insert_dashboard` then constructs a new `Dashboard` with the same
`slug` and calls
`db.session.add(dashboard); db.session.commit()` (lines 95-110).
- On commit, the partial unique index `ix_dashboards_active_slug` still
sees `dash2`
(active) plus the new dashboard, and raises an `IntegrityError` for
duplicate active
`slug`, causing the integration test (for example,
`test_get_drill_info_embedded_user_with_dashboard_id` in
`datasets/api_tests.py:36-88`)
to fail.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9139cb70449043c682bf6671992b67bb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9139cb70449043c682bf6671992b67bb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/base_tests.py
**Line:** 604:612
**Comment:**
*Logic Error: The cleanup query only deletes the first dashboard found
for a slug. With soft-delete enabled, multiple rows can share the same slug
(deleted + active), so deleting a single arbitrary row can leave an active
duplicate behind and make the next insert fail with a unique-slug constraint
error. Delete all matching rows (or explicitly the active one) before inserting.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=6ab0f95fe7d88ceb1723585f22a79b8d4918b8dbc6ad8e860d33804f1fec4e78&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=6ab0f95fe7d88ceb1723585f22a79b8d4918b8dbc6ad8e860d33804f1fec4e78&reaction=dislike'>👎</a>
--
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]
