aminghadersohi commented on code in PR #40130: URL: https://github.com/apache/superset/pull/40130#discussion_r3342302277
########## superset/commands/dataset/restore.py: ########## @@ -0,0 +1,43 @@ +# 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.""" + +import logging + +from superset.commands.dataset.exceptions import ( + DatasetForbiddenError, + DatasetNotFoundError, + DatasetRestoreFailedError, +) +from superset.commands.restore import BaseRestoreCommand +from superset.connectors.sqla.models import SqlaTable +from superset.daos.dataset import DatasetDAO + +logger = logging.getLogger(__name__) + + +class RestoreDatasetCommand(BaseRestoreCommand[SqlaTable]): + """Restore a soft-deleted dataset by clearing its ``deleted_at`` field. + + All transactional wrapping and validation lives in + ``BaseRestoreCommand``; this subclass is purely declarative. + """ + + dao = DatasetDAO + not_found_exc = DatasetNotFoundError + forbidden_exc = DatasetForbiddenError + restore_failed_exc = DatasetRestoreFailedError Review Comment: **HIGH — `RestoreDatasetCommand` does not check for live logical duplicates before restoring** `validate()` (in `BaseRestoreCommand`) confirms the row exists, is soft-deleted, and the caller owns it — but it does **not** check whether a live dataset with the same `(database_id, catalog, schema, table_name)` was created while this one was soft-deleted. Because `SqlaTable` uniqueness is logical-only (not a DB constraint — see the comment at `models.py:1249`), and `DatasetDAO.validate_uniqueness` now excludes soft-deleted rows via the ORM listener, the following sequence is valid today: 1. `DELETE /api/v1/dataset/{pk}` — soft-deletes dataset A (`db.public.users`) 2. `POST /api/v1/dataset/` — creates dataset B, same physical table — uniqueness check misses soft-deleted A 3. `POST /api/v1/dataset/{uuid_A}/restore` — restores A Result: **two live `SqlaTable` rows for the same physical table**, producing chart-lookup ambiguity and undefined behavior in anything that assumes one dataset per physical table. Fix: add a dataset-specific `validate()` override (or a pre-restore hook) that queries for a live row with the same logical identity before clearing `deleted_at`. Also fix `DatasetDAO.validate_uniqueness` / `validate_update_uniqueness` to bypass the visibility filter so soft-deleted rows block re-creation at the API level. This is the root cause of the issue richardfogaca noted at `api.py:963`. ########## superset/commands/dataset/exceptions.py: ########## @@ -170,6 +170,10 @@ class DatasetUpdateFailedError(UpdateFailedError): message = _("Dataset could not be updated.") +class DatasetRestoreFailedError(CommandException): + message = _("Dataset could not be restored.") Review Comment: **MEDIUM — `DatasetRestoreFailedError` should inherit a domain-specific base, not `CommandException` directly** All sibling exceptions use typed middle-tier bases: - `DatasetDeleteFailedError(DeleteFailedError)` - `DatasetUpdateFailedError(UpdateFailedError)` - `DatasetRefreshFailedError(UpdateFailedError)` (nearest analogy) Inheriting `CommandException` directly skips the tier. Either define `RestoreFailedError` in `superset/commands/exceptions.py` (mirroring `DeleteFailedError` / `UpdateFailedError`) and inherit from that, or inherit from the nearest analogy (`UpdateFailedError`) with a comment explaining the choice. -- 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]
