mikebridge commented on code in PR #44253:
URL: https://github.com/apache/superset/pull/44253#discussion_r4039271182
##########
superset/commands/version_restore.py:
##########
@@ -81,7 +83,20 @@ def run(self) -> RestoreResult:
# reference ``self.failed_exc`` — a per-subclass ClassVar that
# isn't available when this method is defined on the base (same
# pattern and rationale as ``BaseRestoreCommand.run``).
- @transaction(on_error=partial(on_error, reraise=self.failed_exc))
+ # ``catches`` widens past the SQLAlchemyError default so the
+ # restore engine's fail-closed registry guard (``LookupError``
+ # for a model missing from ``_RESTORE_RELATIONS``) maps to
+ # ``failed_exc`` → 422 instead of a raw 500 (sc-115326). The
+ # tuple is deliberately this narrow: other non-SQLAlchemy
+ # exceptions must keep passing through untouched for the
+ # endpoint to map explicitly.
+ @transaction(
+ on_error=partial(
+ on_error,
+ catches=(SQLAlchemyError, LookupError),
Review Comment:
I rechecked the current head (24d3f442768f376b5910f653261df8e508da0dc2) and
likewise have not established a naturally reachable escaping
KeyError/IndexError in this restore path. resolve_version() returns a checked
fixed-arity pair; the restore registry uses .get(); Continuum version_class()
catches its own KeyError. Its parent_class() lookup is unguarded, but the
builder populates the reverse map alongside the forward version-class map.
Malformed action metadata is caught in _inject_action_meta_record(). The 12
focused command tests pass; they test translation behavior, not proof that
arbitrary internal errors are unreachable.
Would it be worth retaining the current mapping unless we can demonstrate a
reachable counterexample? Choosing a dedicated registry exception to isolate
future programming errors would be a separate hardening policy; I have not
changed that policy or treated mock-injected KeyError as a production
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]
