mikebridge commented on code in PR #44184:
URL: https://github.com/apache/superset/pull/44184#discussion_r4033277054
##########
superset/commands/dataset/update.py:
##########
@@ -151,6 +152,13 @@ def _validate_dataset_source(self, exceptions:
list[ValidationError]) -> None:
table,
self._model_id,
):
+ # Same hidden-twin guidance as the create path: when the blocking
+ # row is a SOFT-DELETED dataset, raise the targeted 422 naming
+ # the twin's uuid and the restore pointer instead of the opaque
+ # "already exists" (the twin is invisible in the caller's list).
+ soft_twin: SqlaTable | None
+ if soft_twin := DatasetDAO.find_soft_deleted_logical_duplicate(db,
table):
+ raise DatasetSoftDeletedTwinExistsError(str(soft_twin.uuid))
Review Comment:
Confirmed and fixed in acf39cd4d3e5679b67a96415958a6a030005039b.
`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.
##########
superset/commands/dashboard/create.py:
##########
@@ -44,6 +49,17 @@ def __init__(self, data: dict[str, Any]) -> None:
def run(self) -> Model:
self.validate()
dashboard = DashboardDAO.create(attributes=self._properties)
+ # Surface the INSERT here rather than at the transaction decorator's
+ # commit, so a slug collision with a SOFT-DELETED dashboard (possible
+ # only on the full-constraint dialects; the partial-index dialects
+ # free the slot) can be translated into restore guidance. Any other
+ # integrity failure re-raises unchanged.
+ try:
+ db.session.flush()
+ except IntegrityError as ex:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+
raise_for_soft_deleted_slug_collision(self._properties.get("slug"), ex)
Review 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.
##########
superset/commands/dashboard/update.py:
##########
@@ -112,6 +116,15 @@ def run(self) -> Model:
self._model,
{k: v for k, v in self._properties.items() if k !=
"json_metadata"},
)
+ # See CreateDashboardCommand.run: translate a slug collision
+ # with a soft-deleted dashboard (full-constraint dialects) into
+ # restore guidance; anything else re-raises unchanged.
+ try:
+ db.session.flush()
+ except IntegrityError as ex:
+ db.session.rollback() # pylint:
disable=consider-using-transaction
+
raise_for_soft_deleted_slug_collision(self._properties.get("slug"), ex)
Review 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.
--
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]
