mikebridge commented on code in PR #42469: URL: https://github.com/apache/superset/pull/42469#discussion_r3673626953
########## superset/versioning/restore.py: ########## @@ -0,0 +1,254 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Write-side: restore a versioned entity to an earlier state. + +Companion to :mod:`superset.versioning.queries`. The +``BaseRestoreVersionCommand`` in :mod:`superset.commands.version_restore` +is the only intended caller; the backward-compat ``VersionDAO`` façade +in :mod:`superset.daos.version` re-exports ``restore_version``. + +Restore semantics are strictly per-entity: a restore rewrites the target +entity's own fields (and, for datasets, its own columns/metrics — the +aggregate's internal parts), never the content of other entities. A +dashboard restore reattaches membership to charts that still exist; +charts that have been deleted since the snapshot stay deleted and are +reported as skipped rather than revived or dangling. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass, field +from datetime import datetime +from typing import Any +from uuid import UUID + +from sqlalchemy_continuum import version_class + +from superset.extensions import db +from superset.versioning.baseline import OPERATION_DELETE +from superset.versioning.queries import find_active_by_uuid +from superset.versioning.utils import single_flush_scope + +logger = logging.getLogger(__name__) + +# A DELETE version row (``OPERATION_DELETE``) is never a valid restore +# target: Continuum's ``Reverter`` would delete the live entity and report +# success — the opposite of the non-destructive contract — so the engine +# treats it as not-found. + + +# Per-model relationships that Continuum's Reverter recurses into during a +# restore — deliberately limited to the entity's OWN aggregate parts +# (``TableColumn`` / ``SqlMetric`` on ``SqlaTable``). ``Dashboard`` is NOT +# given ``slices`` here: recursing into the M2M would run a full child +# revert on every member chart, overwriting live charts' content with +# historical values (charts are shared entities with their own restore), +# and re-creating hard-deleted charts. Dashboard membership is instead +# reconstructed by :func:`_restore_dashboard_membership`. +# +# Unknown models fail closed (``LookupError``) rather than defaulting to a +# relation-less restore — a silently partial restore is worse than a loud +# failure (mirrors ``_RAISE_FOR_ACCESS_KWARG`` in ``api_helpers``). +_RESTORE_RELATIONS: dict[str, list[str]] = { + "SqlaTable": ["columns", "metrics"], + "Dashboard": [], + "Slice": [], +} Review Comment: Leaving the string keying as-is — it is deliberate, and the fragility you describe is handled at runtime rather than by the type checker. Two reasons: 1. **It fails closed.** `_RESTORE_RELATIONS.get(...)` returning `None` raises `LookupError` with a message naming the unregistered model, rather than defaulting to a relation-less restore. A rename therefore produces a loud, immediate failure the first time that model is restored — not a silent partial restore. That is called out in the comment above the dict and pinned by `test_restore_version_fails_closed_for_unregistered_model`. 2. **Keying on the class would require importing the models here.** This module deliberately avoids module-level model imports — see the `# pylint: disable=import-outside-toplevel` local imports elsewhere in the file, which exist because `utils.core` and `models.slice` pull in the feature-flag manager and need the initialised app. Turning `dict[str, ...]` into `dict[type, ...]` reintroduces exactly that cycle. Agreed a subclass would not be caught, but restore is dispatched from three explicit endpoints on three concrete models, so there is no polymorphic entry point for one to arrive through. ########## superset/versioning/restore.py: ########## @@ -0,0 +1,254 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Write-side: restore a versioned entity to an earlier state. + +Companion to :mod:`superset.versioning.queries`. The +``BaseRestoreVersionCommand`` in :mod:`superset.commands.version_restore` +is the only intended caller; the backward-compat ``VersionDAO`` façade +in :mod:`superset.daos.version` re-exports ``restore_version``. + +Restore semantics are strictly per-entity: a restore rewrites the target +entity's own fields (and, for datasets, its own columns/metrics — the +aggregate's internal parts), never the content of other entities. A +dashboard restore reattaches membership to charts that still exist; +charts that have been deleted since the snapshot stay deleted and are +reported as skipped rather than revived or dangling. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass, field +from datetime import datetime +from typing import Any +from uuid import UUID + +from sqlalchemy_continuum import version_class + +from superset.extensions import db +from superset.versioning.baseline import OPERATION_DELETE +from superset.versioning.queries import find_active_by_uuid +from superset.versioning.utils import single_flush_scope + +logger = logging.getLogger(__name__) + +# A DELETE version row (``OPERATION_DELETE``) is never a valid restore +# target: Continuum's ``Reverter`` would delete the live entity and report +# success — the opposite of the non-destructive contract — so the engine +# treats it as not-found. + + +# Per-model relationships that Continuum's Reverter recurses into during a +# restore — deliberately limited to the entity's OWN aggregate parts +# (``TableColumn`` / ``SqlMetric`` on ``SqlaTable``). ``Dashboard`` is NOT +# given ``slices`` here: recursing into the M2M would run a full child +# revert on every member chart, overwriting live charts' content with +# historical values (charts are shared entities with their own restore), +# and re-creating hard-deleted charts. Dashboard membership is instead +# reconstructed by :func:`_restore_dashboard_membership`. +# +# Unknown models fail closed (``LookupError``) rather than defaulting to a +# relation-less restore — a silently partial restore is worse than a loud +# failure (mirrors ``_RAISE_FOR_ACCESS_KWARG`` in ``api_helpers``). +_RESTORE_RELATIONS: dict[str, list[str]] = { + "SqlaTable": ["columns", "metrics"], + "Dashboard": [], + "Slice": [], +} + + +@dataclass +class RestoreResult: + """Outcome of a successful restore. + + ``skipped_slice_ids`` is only ever populated for dashboard restores: + member charts referenced by the snapshot that no longer exist and were + therefore not reattached (they stay deleted — restore never revives + entities). + """ + + entity: Any + skipped_slice_ids: list[int] = field(default_factory=list) + + +def restore_version( + model_cls: type, + entity_uuid: UUID, + transaction_id: int, + *, + entity: Any | None = None, +) -> RestoreResult | None: + """Restore the entity identified by *entity_uuid* to the state captured + at *transaction_id* (the stable identifier resolved from a + ``version_uuid`` by :func:`superset.versioning.queries.resolve_version`). + + Returns a :class:`RestoreResult` wrapping the live entity, or ``None`` + when the UUID does not match an active entity, no version row exists at + *transaction_id*, or the target row is a DELETE — callers should + translate all three to a 404. + + Pass *entity* to skip the ``find_active_by_uuid`` lookup when the + caller has already loaded the row (the command's ``validate()`` has). + + Uses SQLAlchemy-Continuum's native ``version_obj.revert(relations=...)`` + and delegates commit to the caller (expected to be a command decorated + with ``@transaction()``). The ``relations`` list depends on the model + type and is looked up in :data:`_RESTORE_RELATIONS`; unknown models + raise ``LookupError`` rather than silently restoring without children. + + Within the same flush, ``changed_on`` / ``changed_by_fk`` are + re-stamped with the current time and the restoring user's id so the + new version row produced by the restoring commit reflects who clicked + Restore, not the original author. ``created_on`` / ``created_by_fk`` + are left alone. + """ + if entity is None: + entity = find_active_by_uuid(model_cls, entity_uuid) + if entity is None: + return None + + ver_cls = version_class(model_cls) + target_version = ( + db.session.query(ver_cls) + .filter( + ver_cls.id == entity.id, + ver_cls.transaction_id == transaction_id, + ) + .one_or_none() + ) + if target_version is None or target_version.operation_type == OPERATION_DELETE: + return None + + relations = _RESTORE_RELATIONS.get(model_cls.__name__) Review Comment: Same as the thread on line 71 — fail-closed `LookupError` on an unregistered model, and keying on the class would reintroduce the model-import cycle this module works around. Leaving as-is. ########## superset/versioning/restore.py: ########## @@ -0,0 +1,254 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Write-side: restore a versioned entity to an earlier state. + +Companion to :mod:`superset.versioning.queries`. The +``BaseRestoreVersionCommand`` in :mod:`superset.commands.version_restore` +is the only intended caller; the backward-compat ``VersionDAO`` façade +in :mod:`superset.daos.version` re-exports ``restore_version``. + +Restore semantics are strictly per-entity: a restore rewrites the target +entity's own fields (and, for datasets, its own columns/metrics — the +aggregate's internal parts), never the content of other entities. A +dashboard restore reattaches membership to charts that still exist; +charts that have been deleted since the snapshot stay deleted and are +reported as skipped rather than revived or dangling. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass, field +from datetime import datetime +from typing import Any +from uuid import UUID + +from sqlalchemy_continuum import version_class + +from superset.extensions import db +from superset.versioning.baseline import OPERATION_DELETE +from superset.versioning.queries import find_active_by_uuid +from superset.versioning.utils import single_flush_scope + +logger = logging.getLogger(__name__) + +# A DELETE version row (``OPERATION_DELETE``) is never a valid restore +# target: Continuum's ``Reverter`` would delete the live entity and report +# success — the opposite of the non-destructive contract — so the engine +# treats it as not-found. + + +# Per-model relationships that Continuum's Reverter recurses into during a +# restore — deliberately limited to the entity's OWN aggregate parts +# (``TableColumn`` / ``SqlMetric`` on ``SqlaTable``). ``Dashboard`` is NOT +# given ``slices`` here: recursing into the M2M would run a full child +# revert on every member chart, overwriting live charts' content with +# historical values (charts are shared entities with their own restore), +# and re-creating hard-deleted charts. Dashboard membership is instead +# reconstructed by :func:`_restore_dashboard_membership`. +# +# Unknown models fail closed (``LookupError``) rather than defaulting to a +# relation-less restore — a silently partial restore is worse than a loud +# failure (mirrors ``_RAISE_FOR_ACCESS_KWARG`` in ``api_helpers``). +_RESTORE_RELATIONS: dict[str, list[str]] = { + "SqlaTable": ["columns", "metrics"], + "Dashboard": [], + "Slice": [], +} + + +@dataclass +class RestoreResult: + """Outcome of a successful restore. + + ``skipped_slice_ids`` is only ever populated for dashboard restores: + member charts referenced by the snapshot that no longer exist and were + therefore not reattached (they stay deleted — restore never revives + entities). + """ + + entity: Any + skipped_slice_ids: list[int] = field(default_factory=list) + + +def restore_version( + model_cls: type, + entity_uuid: UUID, + transaction_id: int, + *, + entity: Any | None = None, +) -> RestoreResult | None: + """Restore the entity identified by *entity_uuid* to the state captured + at *transaction_id* (the stable identifier resolved from a + ``version_uuid`` by :func:`superset.versioning.queries.resolve_version`). + + Returns a :class:`RestoreResult` wrapping the live entity, or ``None`` + when the UUID does not match an active entity, no version row exists at + *transaction_id*, or the target row is a DELETE — callers should + translate all three to a 404. + + Pass *entity* to skip the ``find_active_by_uuid`` lookup when the + caller has already loaded the row (the command's ``validate()`` has). + + Uses SQLAlchemy-Continuum's native ``version_obj.revert(relations=...)`` + and delegates commit to the caller (expected to be a command decorated + with ``@transaction()``). The ``relations`` list depends on the model + type and is looked up in :data:`_RESTORE_RELATIONS`; unknown models + raise ``LookupError`` rather than silently restoring without children. + + Within the same flush, ``changed_on`` / ``changed_by_fk`` are + re-stamped with the current time and the restoring user's id so the + new version row produced by the restoring commit reflects who clicked + Restore, not the original author. ``created_on`` / ``created_by_fk`` + are left alone. + """ + if entity is None: + entity = find_active_by_uuid(model_cls, entity_uuid) + if entity is None: + return None + + ver_cls = version_class(model_cls) + target_version = ( + db.session.query(ver_cls) + .filter( + ver_cls.id == entity.id, + ver_cls.transaction_id == transaction_id, + ) + .one_or_none() + ) + if target_version is None or target_version.operation_type == OPERATION_DELETE: + return None + + relations = _RESTORE_RELATIONS.get(model_cls.__name__) + if relations is None: + raise LookupError( + f"No restore relations registered for {model_cls.__name__!r}; " + "register the model in _RESTORE_RELATIONS before wiring a " + "restore command for it." + ) + + # Run the whole revert — including membership reconstruction and audit + # stamping — inside a single flush scope so SQLAlchemy-Continuum's + # ``Reverter`` can iterate relations without tripping its autoflush + # race, and so the change-records listener sees the complete state in + # one ``after_flush`` pass. See ``single_flush_scope`` for the full + # rationale. + skipped_slice_ids: list[int] = [] + try: + with single_flush_scope(db.session): + target_version.revert(relations=relations) + if model_cls.__name__ == "Dashboard": Review Comment: Same as the thread on line 71. Leaving as-is. -- 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]
