Repository navigation
feat(soft-delete): translate unique collisions with soft-deleted rows into restore guidance - #44184
Conversation
… into restore guidance A slug or table slot held by a soft-deleted row fails opaquely today: dashboards die at flush with a raw IntegrityError 422 on the full-constraint dialects (SQLite, MariaDB, MySQL < 8.0 — the partial-index dialects free the slot per migration 9e1f3b8c4d2a), and a dataset PUT that collides with a hidden twin says only "already exists" about a row the caller cannot see. Introduce superset/commands/soft_delete_collisions.py — a shared finder that bypasses the visibility filter (session-scoped, matching deleted rows only) plus a dashboard-slug translator — and wire it in: - dashboard create/update commands flush inside run() and translate an IntegrityError into a 422 naming the deleted holder's uuid and its restore endpoint; any conflict NOT caused by a soft-deleted row re-raises unchanged - dataset update gains the same hidden-twin branch the create path already has (DatasetSoftDeletedTwinExistsError, wired into PUT) - dataset PUT's refresh/update failure handlers merged (identical behavior) to stay within the complexity budget The integration control introspects the schema for whether a deleted row still reserves the slug, creating the full unique index in-test on a create-all SQLite database so the collision path is exercised the way a migration-built database behaves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
github-actions
Bot
added
i18n
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44184 +/- ##
=======================================
Coverage 80.41% 80.41%
=======================================
Files 2932 2933 +1
Lines 174621 174688 +67
Branches 40559 40568 +9
=======================================
+ Hits 140415 140478 +63
- Misses 31539 31543 +4
Partials 2667 2667
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The custom pylint plugin flags any raw session.rollback(); these two are required — after the failed flush the session cannot run the soft-deleted-holder query until it is rolled back. Same inline-disable pattern as force_purge.py and deletion_retention.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…mpty-string slugs Codex round (committer + sqlalchemy lenses): - The translator no longer advises restoring an archived namesake when a LIVE dashboard holds the slug: under concurrency another request can commit the same slug between this request's uniqueness precheck and its flush, and the failed constraint is then the live row's — a restore of the archived holder could never resolve it. A live-holder check now runs first; the original IntegrityError stands. - `if not slug` swallowed empty strings — the PUT schema accepts a zero-length slug and the uniqueness prechecks compare every non-None value, so "" is now probed like any other value; only None (no slug sent) skips. - The two import-outside-toplevel disables carry their reason (app-init chain / model registry, per the constitution). - Annotations: -> None on the integration test, typed locals, and the guidance error's holder is a Dashboard (TYPE_CHECKING import), not Any. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Code Review Agent Run #226d21Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Reviewed 7dcab08cf3d3.
P2 — unrelated integrity errors can become incorrect slug restore guidance. soft_delete_collisions.py:105–119 does not inspect the failing constraint. If an archived dashboard holds a reusable slug and a create request uses that slug plus another dashboard's UUID, the UUID constraint fails, but the helper recommends restoring the archived dashboard or choosing another slug. Neither fixes the duplicate UUID. Could we narrowly identify a slug uniqueness violation before translating, preserve unrelated/ambiguous failures, and add this negative test? Source-verified; not runtime-reproduced.
The archived-slug translator caught every flush `IntegrityError` and asked only which row holds the requested slug, never which constraint actually failed. So with an archived dashboard holding slug S and a live dashboard holding uuid U under a different slug, a create with S and U fails on the uuid constraint and was still answered with "restore the archived dashboard" guidance — advice that cannot resolve a uuid conflict. The create schema accepts a client-supplied uuid, so the path is reachable. Require positive identification that the caught error is a dashboard slug uniqueness violation before translating: PostgreSQL by SQLSTATE 23505 plus `diag.constraint_name`, MySQL/MariaDB by errno 1062 plus the quoted key name (optionally table-qualified, as MySQL 8 emits), SQLite by its exact `UNIQUE constraint failed: dashboards.slug` diagnostic. Anything missing, ambiguous, or belonging to another constraint returns untranslated and the caller re-raises the original error. Both accepted names are matched: `idx_unique_slug` (the original full constraint) and `ix_dashboards_active_slug` (the live-rows-only partial index that replaced it on PostgreSQL and MySQL 8.0.13+). The diagnostic is read from the driver exception (`ex.orig`), never from the SQLAlchemy wrapper's `str()`, which embeds the statement and its bound parameters — a dashboard whose slug or title text contained a constraint name would otherwise be enough to fool the matcher. A regression pins that specific trap. The live-holder guard and the `slug is None` semantics are unchanged and still run after the new gate. Reported by Richard Fogaça in review of apache#44184; the uuid variant is distinct from the live-holder race fixed earlier on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
|
You're right, and it's distinct from the live-holder fix — that guard only ever looks at the requested slug, so with archived slug S and a live dashboard holding uuid U under a different slug, there's no live S holder to find and the uuid failure still comes back as restore-the-archived-dashboard guidance. Since the create schema accepts a client-supplied uuid, it's reachable. Fixed in 96a620a: the translator now requires positive identification that the caught error is a dashboard slug uniqueness violation before it translates — PostgreSQL by SQLSTATE 23505 plus |
Code Review Agent Run #6828eeActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
apache#44025 landed the update-path is_managed_externally gate today and updated every test in this file that existed then. This branch's soft-deleted-twin test was written on a separate branch, so the two only met when master was merged in: its bare MagicMock returns a truthy attribute, the new gate reads that as externally managed, and the dataset is refused before the uniqueness guidance it exists to assert. Matches the pattern the other ten tests in this file already use. Control: with the stub removed the test fails with DatasetForbiddenError; restored, the file's 32 tests pass. Both states asserted by file content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4MGEkUcDpyLReqjQBhPy
Code Review Agent Run #89e2f4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Graded the nine claims in your reply by execution at 9513dc18 — all land; the ex.orig regression is precisely pinned (only cause12 separates it from a str(orig) control). One inline.
Also: bito's uncommitted-DDL note on api_tests.py doesn't reproduce — pysqlite autocommits DDL.
| pgcode: str | None = getattr(orig, "pgcode", None) or getattr( | ||
| orig, "sqlstate", None | ||
| ) | ||
| if pgcode is not None: | ||
| constraint_name: str | None = getattr( | ||
| getattr(orig, "diag", None), "constraint_name", None | ||
| ) | ||
| return pgcode == "23505" and constraint_name in _SLUG_UNIQUE_NAMES |
There was a problem hiding this comment.
PyMySQL 1.2.0 (pinned) and mysql-connector set .sqlstate on the DBAPI exception, so orig takes this PostgreSQL branch, '23000' != '23505' → False, and the MySQL branch below is unreachable. Measured on live MySQL 8. Gating on diag (psycopg-only) preserves every current verdict:
| pgcode: str | None = getattr(orig, "pgcode", None) or getattr( | |
| orig, "sqlstate", None | |
| ) | |
| if pgcode is not None: | |
| constraint_name: str | None = getattr( | |
| getattr(orig, "diag", None), "constraint_name", None | |
| ) | |
| return pgcode == "23505" and constraint_name in _SLUG_UNIQUE_NAMES | |
| diag: Any | None = getattr(orig, "diag", None) | |
| if diag is not None: | |
| pgcode: str | None = getattr(orig, "pgcode", None) or getattr( | |
| orig, "sqlstate", None | |
| ) | |
| constraint_name: str | None = getattr(diag, "constraint_name", None) | |
| return pgcode == "23505" and constraint_name in _SLUG_UNIQUE_NAMES |
There was a problem hiding this comment.
Fixed in 46fa114. The integer MySQL errno is checked before PostgreSQL SQLSTATE, so errno 1062 plus sqlstate 23000 reaches the narrow slug-key check. Added combined-attribute positives for bare, table-qualified and active-index slug keys, plus wrong-key/wrong-errno negatives; the three positives failed against the old order. Also verified the installed PyMySQL 1.2.0 error-packet parser produces the combined attributes (constructed packets, not a live MySQL server run). The 64 affected unit tests and all applicable changed-file hooks pass; independent final-snapshot review has no open findings. The no-holder probe assertion and traceback-comment nit are included. Live metadata integration/CI remains a separate gate.
Code Review Agent Run #56ef17Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
CI refresh: merged Apache master containing the already-merged migration repair #44364 into this branch, published at 70d7ab2. No duplicate migration was added. The fresh PR merge ref includes repair commit 4c77b0b; its migration directory matches that repaired baseline. Local verification: 65 focused tests passed (including the single-migration-head regression), all applicable pre-commit hooks passed against the complete branch-changed file list, and the final source snapshot received independent Python review. Fresh CI has been triggered; this is not an all-green claim. Live database integration remains a remote-CI gate. The older migration failures tested pre-repair merge snapshots and are not evidence about this new head. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if soft_twin := DatasetDAO.find_soft_deleted_logical_duplicate(db, table): | ||
| raise DatasetSoftDeletedTwinExistsError(str(soft_twin.uuid)) |
There was a problem hiding this comment.
Suggestion: update_dataset_metric does not catch this command exception, so a soft-deleted collision escapes as an unexpected MCP failure instead of the structured validation response used previously.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dataset/update.py
**Line:** 160:161
**Comment:**
*Api Mismatch: `update_dataset_metric` does not catch this command exception, so a soft-deleted collision escapes as an unexpected MCP failure instead of the structured validation response used previously.
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 fixThere was a problem hiding this comment.
Confirmed and fixed in acf39cd. DatasetSoftDeletedTwinExistsError is a separate command exception, so the previous catch-all re-raised it as an unexpected tool error. update_dataset_metric now catches it specifically and returns UpdateDatasetMetricResponse(error=...) with the existing guidance.
The new FastMCP-client regression failed before the handler with ToolError and passes afterward, asserting the structured error, absent metric and restore pointer. All 84 affected MCP/command cases and all applicable branch-file pre-commit hooks pass; independent final-source Python review approved the exact tree. Fresh CI on this new head is pending, not inherited from the previous green head.
| except IntegrityError as ex: | ||
| db.session.rollback() # pylint: disable=consider-using-transaction | ||
| raise_for_soft_deleted_slug_collision(self._properties.get("slug"), ex) |
There was a problem hiding this comment.
Suggestion: When this command runs inside an existing g.in_transaction, rollback() aborts unrelated pending changes owned by the outer operation.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Transaction/lifecycle
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/create.py
**Line:** 59:61
**Comment:**
*Transaction Lifecycle: When this command runs inside an existing `g.in_transaction`, `rollback()` aborts unrelated pending changes owned by the outer operation.
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 fixThere was a problem hiding this comment.
Checked this against the transaction decorator and a real in-memory SQLite/SQLAlchemy flush failure. g.in_transaction shares one all-or-nothing transaction; it does not create a savepoint. A failed flush already rolls back the database transaction and leaves the Session inactive. In the probe, a previously flushed unrelated tag was already absent before the explicit Session.rollback().
The explicit rollback here resets that inactive Session so the slug-diagnostic queries can run; removing it produces PendingRollbackError rather than preserving outer writes. The command still raises either the translated validation error or the original integrity error. Current callers propagate the failure rather than recovering a partial outer unit of work. No new rollback loss was reproduced, and independent source review agrees. Supporting a nested caller that catches failures and continues would need a separately designed savepoint/transaction contract; this PR does not introduce that policy. The runtime probe was SQLite-only, not a live cross-dialect test.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
In this dashboard creation command, retain the explicit session rollback after a failed flush so the session can be reused for slug-diagnostic queries; do not flag it as losing unrelated outer changes, since the failed flush already invalidates the shared transaction.
Applied to:
superset/commands/dashboard/create.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| except IntegrityError as ex: | ||
| db.session.rollback() # pylint: disable=consider-using-transaction | ||
| raise_for_soft_deleted_slug_collision(self._properties.get("slug"), ex) |
There was a problem hiding this comment.
Suggestion: When this command runs inside an existing g.in_transaction, rollback() discards unrelated staged dashboard, tag, and relationship changes from the outer operation.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Transaction/lifecycle
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/update.py
**Line:** 124:126
**Comment:**
*Transaction Lifecycle: When this command runs inside an existing `g.in_transaction`, `rollback()` discards unrelated staged dashboard, tag, and relationship changes from the outer operation.
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 fixThere was a problem hiding this comment.
The same shared-transaction analysis applies to the update path: g.in_transaction is not a savepoint, and a failed flush already aborts the database transaction before this explicit rollback. The reset is needed for the diagnostic query; both the translated and untranslated paths still raise. A real SQLite probe confirmed earlier writes were already absent while the Session was inactive, before explicit rollback.
No partial-success recovery contract is provided by transaction(), and current callers propagate the failure. Independent source review found no new rollback defect here. I have left this transaction policy unchanged; safely continuing an outer operation after a nested failure would require separately scoped savepoint design. This disposition does not claim a live MySQL/PostgreSQL reproduction.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag the explicit rollback after a failed flush in this command; it is required to reset the session for diagnostic queries, and failures are propagated rather than used for partial-success recovery.
Applied to:
superset/commands/dashboard/update.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
Code Review Agent Run #ddfbc6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Confirmed fixed at acf39cd4 by execution: against live PyMySQL 1.2.0, mysql-connector, psycopg2, psycopg3 and sqlite3 all 10 slug/uuid cases classify correctly, and there is no mirror defect — PG drivers put a str in args[0]. My round-1 thread can be resolved.
|
Synced with master at ffd4739; no functional change |
SUMMARY
Soft delete keeps rows in the database, so unique slots held by deleted rows can block new work with errors that never mention the invisible holder (sc-107581, part 1):
9e1f3b8c4d2agave PostgreSQL and MySQL 8.0+ a partial unique index scoped to live rows, so a deleted dashboard frees its slug there. On the full-constraint dialects (SQLite, MariaDB, MySQL < 8.0.13) the insert still dies at flush with a rawIntegrityError422 that names nobody.DatasetSoftDeletedTwinExistsErrornaming the hidden twin and its restore endpoint, but the update path still says only "already exists" about a row the caller cannot see in any listing.This PR adds a small shared abstraction,
superset/commands/soft_delete_collisions.py:find_soft_deleted_slot_holder(model_cls, *criteria)— looks up the deleted holder, bypassing the soft-delete visibility filter with the session-scoped context manager (a per-query execution option never reaches thedo_orm_executelistener) and matching deleted rows only, so a live-row conflict is never misreported as restorable.raise_for_soft_deleted_slug_collision(slug, cause)— translates a dashboard-slug collision into a 422 naming the deleted holder's uuid andPOST /api/v1/dashboard/<uuid>/restore; returns (so the caller re-raises the original error unmasked) when no slug was sent (None— an empty string is a storable value and IS probed), when a LIVE dashboard holds the slug (the concurrent-commit race: restoring an archived namesake could never resolve that conflict), or when no deleted holder exists. (Review round via Codex: live-holder-wins + empty-string semantics, both pinned.)Wired in:
run()(the DAO onlysession.adds; without the explicit flush theIntegrityErrorfires at the@transactiondecorator's commit, outside any command-levelexcept) and translate the collision. Any integrity failure not caused by a soft-deleted row re-raises unchanged.DatasetSoftDeletedTwinExistsError; the dataset PUT handler maps it to a 422 exactly like POST already does.DatasetRefreshFailedError/DatasetUpdateFailedErrorhandlers were behaviorally identical (same 422 body;logger.exception≡logger.error(..., exc_info=True)) and are merged into one, keepingputunder the C901 limit with the new except added.Deliberately not changed (already covered on master): dashboard/dataset importers resolve or restore soft-deleted collisions themselves; dataset create twin guidance; the dashboard partial-index dialects, where a deleted slug is intentionally reclaimable and no error exists to translate. Part 2 of the ticket (restore UX surfacing) is out of scope per the dispatch.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
API-only change. Before (full-constraint dialects):
{"message": "Dashboard could not be created."}After:
{"message": {"slug": ["Slug q1-report belongs to the soft-deleted dashboard <uuid>. Restore it via POST /api/v1/dashboard/<uuid>/restore, or use a different slug."]}}TESTING INSTRUCTIONS
pytest tests/unit_tests/commands/dashboard/slug_collision_test.py— translation with the deleted holder's uuid + restore endpoint and__cause__chained; no-holder returns so the caller re-raises (a conflict NOT caused by a soft-deleted row is never masked); no-slug skips the probe entirely.pytest tests/unit_tests/commands/dataset/update_test.py— the update twin branch raises the shared guidance error; the plain exists case still raisesDatasetExistsValidationError.pytest "tests/integration_tests/dashboards/api_tests.py::TestDashboardApi::test_create_dashboard_slug_held_by_soft_deleted"— end-to-end: create, soft-delete, re-create the same slug. The test introspects whether the schema reserves the slug for deleted rows (partial-index schemas → 201, slot freed; full-constraint schemas → 422 with guidance) and, on acreate_all-built SQLite database (which has neither, since the model no longer declares the column unique), creates the full unique index in-test so the collision path is exercised the way a migration-built database behaves.ADDITIONAL INFORMATION
🤖 Generated with Claude Code
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW