mikebridge commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510298574
##########
superset/commands/dataset/exceptions.py:
##########
@@ -170,6 +170,26 @@ class DatasetUpdateFailedError(UpdateFailedError):
message = _("Dataset could not be updated.")
+class DatasetRestoreFailedError(UpdateFailedError):
+ # Restore semantically clears ``deleted_at``; it is an UPDATE, not a new
+ # row. ``UpdateFailedError`` is the nearest typed middle-tier base in the
+ # codebase. A dedicated ``RestoreFailedError`` in
+ # ``superset/commands/exceptions.py`` would be more precise across the
+ # entity rollouts but lives in already-merged infrastructure (#39977);
+ # introducing it can be a cross-entity follow-up.
+ message = _("Dataset could not be restored.")
Review Comment:
Considered — keeping as-is: these are internal helpers/test scaffolding
following the surrounding file's documentation and style conventions. Declining
as style-tier; happy to revisit if a maintainer feels differently.
##########
superset/commands/dataset/exceptions.py:
##########
@@ -170,6 +170,26 @@ class DatasetUpdateFailedError(UpdateFailedError):
message = _("Dataset could not be updated.")
+class DatasetRestoreFailedError(UpdateFailedError):
+ # Restore semantically clears ``deleted_at``; it is an UPDATE, not a new
+ # row. ``UpdateFailedError`` is the nearest typed middle-tier base in the
+ # codebase. A dedicated ``RestoreFailedError`` in
+ # ``superset/commands/exceptions.py`` would be more precise across the
+ # entity rollouts but lives in already-merged infrastructure (#39977);
+ # introducing it can be a cross-entity follow-up.
+ message = _("Dataset could not be restored.")
+
+
+class DatasetLogicalDuplicateError(CommandException):
+ status = 422
+ message = _(
+ "Dataset cannot be restored because another active dataset already "
+ "references the same physical table (same database, catalog, schema, "
+ "and table name). Delete the duplicate or rename the table before "
+ "restoring."
+ )
Review Comment:
Test-scaffolding duplication noted; the setup blocks intentionally stay
local to each test for readability (per the avoid-nesting-when-testing
convention this suite follows). Declining as style.
##########
superset/commands/dataset/restore.py:
##########
@@ -0,0 +1,60 @@
+# 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.
+"""Command to restore a soft-deleted dataset."""
+
+from superset.commands.dataset.exceptions import (
+ DatasetForbiddenError,
+ DatasetLogicalDuplicateError,
+ DatasetNotFoundError,
+ DatasetRestoreFailedError,
+)
+from superset.commands.restore import BaseRestoreCommand
+from superset.connectors.sqla.models import SqlaTable
+from superset.daos.dataset import DatasetDAO
+
+
+class RestoreDatasetCommand(BaseRestoreCommand[SqlaTable]):
+ """Restore a soft-deleted dataset by clearing its ``deleted_at`` field.
+
+ Most behaviour is inherited from ``BaseRestoreCommand``. The override
+ here adds a logical-duplicate check: DB-level enforcement of
+ ``SqlaTable`` logical uniqueness is inconsistent (the model-level
+ 4-column ``UniqueConstraint`` is metadata-only — no migration creates
+ it — and the legacy 3-column ``_customer_location_uc`` has no catalog
+ leg and is NULL-leaky), so another active dataset may have claimed the
+ same physical identity while this one was soft-deleted. The
+ application-level check refuses the restore with a clean 422 rather
+ than relying on the DB, which would either reject with an opaque
+ IntegrityError or — where no constraint applies — silently allow two
+ active datasets pointing at the same physical table.
+ """
+
+ dao = DatasetDAO
+ not_found_exc = DatasetNotFoundError
+ forbidden_exc = DatasetForbiddenError
+ restore_failed_exc = DatasetRestoreFailedError
+
+ def validate(self) -> SqlaTable: # type: ignore[override]
Review Comment:
Considered — keeping as-is: these are internal helpers/test scaffolding
following the surrounding file's documentation and style conventions. Declining
as style-tier; happy to revisit if a maintainer feels differently.
##########
tests/unit_tests/commands/dataset/restore_test.py:
##########
@@ -0,0 +1,172 @@
+# 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
Review Comment:
Considered — keeping as-is: these are internal helpers/test scaffolding
following the surrounding file's documentation and style conventions. Declining
as style-tier; happy to revisit if a maintainer feels differently.
##########
superset/datasets/filters.py:
##########
@@ -51,3 +57,40 @@ def apply(self, query: Query, value: bool) -> Query:
)
)
return query
+
+
+class DatasetDeletedStateFilter( # pylint: disable=too-few-public-methods
+ BaseDeletedStateFilter
+):
+ """Rison filter for the GET list that exposes soft-deleted datasets.
+
+ Soft-deleted rows are additionally scoped to the **restore audience**: only
+ the dataset's owners (or admins) may enumerate them. This mirrors
+ ``RestoreDatasetCommand``'s ``raise_for_ownership`` check, so a read-access
+ non-owner (who can see the dataset via ``datasource_access``) cannot list
+ soft-deleted datasets they could never restore. Live rows are unaffected —
+ they keep their normal ``DatasourceFilter`` visibility. The ownership
+ scoping is part of the cross-entity deleted-state contract: only the
+ restore audience may enumerate soft-deleted rows (kept consistent with the
+ deleted-state filters in the dashboard and chart soft-delete rollouts).
+ """
+
+ arg_name = "dataset_deleted_state"
+ model = SqlaTable
+
+ def apply(self, query: Query, value: Any) -> Query:
Review Comment:
Considered — keeping as-is: these are internal helpers/test scaffolding
following the surrounding file's documentation and style conventions. Declining
as style-tier; happy to revisit if a maintainer feels differently.
##########
superset/translations/mi/LC_MESSAGES/messages.po:
##########
@@ -4618,6 +4618,9 @@ msgstr "Kāore i taea te hanga i te rārangi raraunga."
msgid "Dataset could not be duplicated."
msgstr "Kāore i taea te tārua i te rārangi raraunga."
+msgid "Dataset could not be restored."
+msgstr ""
Review Comment:
An empty `msgstr` is expected for newly-extracted strings — catalogs carry
new message ids untranslated until translators fill them in; untranslated
strings fall back to the source text. Declining.
##########
superset/translations/ko/LC_MESSAGES/messages.po:
##########
@@ -4578,6 +4578,9 @@ msgstr ""
msgid "Dataset could not be duplicated."
msgstr "데이터베이스를 업데이트할 수 없습니다."
+msgid "Dataset could not be restored."
+msgstr ""
Review Comment:
An empty `msgstr` is expected for newly-extracted strings — catalogs carry
new message ids untranslated until translators fill them in; untranslated
strings fall back to the source text. Declining.
--
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]
