codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3492945333
##########
tests/integration_tests/security_tests.py:
##########
@@ -560,6 +560,72 @@ def
test_after_update_database__perm_datasource_access(self):
db.session.delete(tmp_db1)
db.session.commit()
+ def test_after_update_database__perm_datasource_access_soft_deleted(self):
+ """A soft-deleted dataset's perm strings must still be rewritten on a
+ database rename. The maintenance query bypasses the soft-delete
+ visibility filter so hidden datasets are updated too; otherwise
+ restoring the dataset later would resurrect stale
dataset/schema/catalog
+ permission strings pointing at the old database name.
+ """
+ from datetime import datetime # noqa: PLC0415
+
+ from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES # noqa:
PLC0415
+
+ security_manager.on_view_menu_after_update = Mock()
+
+ tmp_db1 = Database(database_name="tmp_db1", sqlalchemy_uri="sqlite://")
+ db.session.add(tmp_db1)
+ db.session.commit()
+
+ table1 = SqlaTable(
+ schema="tmp_schema",
+ table_name="tmp_table1",
+ database=tmp_db1,
+ )
+ db.session.add(table1)
+ db.session.commit()
+ table1 =
db.session.query(SqlaTable).filter_by(table_name="tmp_table1").one()
+ table1_id = table1.id
Review Comment:
**Suggestion:** Add a concrete type annotation for this new scalar variable
assignment to satisfy the type-hint requirement for relevant variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added scalar local variable whose type is readily
annotatable as an int. The custom rule requires type hints on relevant
variables that can be annotated, so this omission is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=048d5a9ee1964f28bc1664f7da7df6b4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=048d5a9ee1964f28bc1664f7da7df6b4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/security_tests.py
**Line:** 588:588
**Comment:**
*Custom Rule: Add a concrete type annotation for this new scalar
variable assignment to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=78a19dbb5ed4076660260e8000b7b5711b90620eaa7d4a79770adb05f12be0f1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=78a19dbb5ed4076660260e8000b7b5711b90620eaa7d4a79770adb05f12be0f1&reaction=dislike'>👎</a>
##########
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
+#
+# 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.
+"""Unit tests for RestoreDatasetCommand."""
+
+from __future__ import annotations
+
+import uuid
+from datetime import datetime, timezone
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+# Stable UUIDs make the test boundary realistic: the production command
+# loads by UUID, not integer ID. The test only mocks the DAO lookup, but
+# the argument shape should still match.
+_DATASET_UUID = str(uuid.uuid4())
+_MISSING_UUID = str(uuid.uuid4())
Review Comment:
**Suggestion:** Add explicit type annotations for the module-level UUID
constants to satisfy the type-hint requirement for relevant variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
These module-level constants are newly introduced Python variables that can
be annotated with `str`; they currently omit type hints, so this matches the
requirement to flag relevant variables missing annotations.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=61ff63e163754317b4c0c5f66b043741&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=61ff63e163754317b4c0c5f66b043741&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/dataset/restore_test.py
**Line:** 30:31
**Comment:**
*Custom Rule: Add explicit type annotations for the module-level UUID
constants to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=8ef6fcd61873c34abb80a45a9cbd66d08e3e920c21e850c029918a47a809d713&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=8ef6fcd61873c34abb80a45a9cbd66d08e3e920c21e850c029918a47a809d713&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -101,3 +102,66 @@ def test_duplicate_dataset_access_check_passes_through()
-> None:
command.validate() # should not raise
# Confirm access check was called with the base dataset
mock_access.assert_called_once_with(datasource=mock_dataset)
+
+
+def test_duplicate_dataset_blocked_by_soft_deleted_twin(session: Session) ->
None:
+ """validate() rejects a duplicate whose name matches a soft-deleted
+ dataset at the same (database, schema).
+
+ The previous name-only ``find_one_or_none`` lookup ran through the
+ soft-delete visibility filter, so a soft-deleted twin was invisible and
+ the duplicate proceeded — hitting whichever DB constraint applies as an
+ opaque IntegrityError, or creating an active twin that permanently
+ blocks restore where none does. The shared ``validate_uniqueness``
+ check bypasses the filter and refuses with a clean validation error.
+ """
+ from datetime import datetime, timezone
+
+ from superset import db
+ from superset.commands.dataset.duplicate import DuplicateDatasetCommand
+ from superset.commands.dataset.exceptions import DatasetInvalidError
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.core import Database
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+ database = Database(database_name="dup_db", sqlalchemy_uri="sqlite://")
+ base = SqlaTable(
+ table_name="base_view",
+ schema="main",
+ database=database,
+ sql="select 1",
+ )
+ hidden_twin = SqlaTable(
+ table_name="dup_name",
+ schema="main",
+ database=database,
+ deleted_at=datetime(2026, 1, 1, tzinfo=timezone.utc),
+ )
Review Comment:
**Suggestion:** Add explicit type annotations for newly introduced local
variables in this test to comply with the type-hint requirement. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The added test code introduces new local variables (`database`, `base`, and
`hidden_twin`) without type annotations, and they are clearly annotatable. This
matches the rule requiring type hints on new or modified Python code where
applicable.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4dda67b7380046a383ab40d4a411c5a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4dda67b7380046a383ab40d4a411c5a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/dataset/test_duplicate.py
**Line:** 128:140
**Comment:**
*Custom Rule: Add explicit type annotations for newly introduced local
variables in this test to comply with the type-hint requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=84b86188137cbafca873351bd52e31f6201de713e0ae3745796f3b0b3d035138&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=84b86188137cbafca873351bd52e31f6201de713e0ae3745796f3b0b3d035138&reaction=dislike'>👎</a>
##########
superset/commands/database/delete.py:
##########
@@ -61,6 +61,28 @@ def validate(self) -> None:
report_names=",".join(report_names),
)
)
- # Check if there are datasets for this database
- if self._model.tables:
+ # Check if there are datasets for this database. ``self._model.tables``
+ # would now hide soft-deleted datasets (``SqlaTable`` inherits
+ # ``SoftDeleteMixin``, so the relationship lazy-load applies the
+ # visibility filter), letting a database whose datasets are all
+ # soft-deleted look empty and be hard-deleted while
``tables.database_id``
+ # rows still reference it. Count with the visibility filter bypassed so
+ # soft-deleted datasets still block the delete.
+ from superset.connectors.sqla.models import ( # pylint:
disable=import-outside-toplevel
+ SqlaTable,
+ )
+ from superset.extensions import ( # pylint:
disable=import-outside-toplevel
+ db,
+ )
+ from superset.models.helpers import ( # pylint:
disable=import-outside-toplevel
+ skip_visibility_filter,
+ )
+
+ with skip_visibility_filter(db.session, SqlaTable):
+ has_datasets = db.session.query(
+ db.session.query(SqlaTable.id)
+ .filter(SqlaTable.database_id == self._model_id)
+ .exists()
+ ).scalar()
Review Comment:
**Suggestion:** Add an explicit boolean type annotation for this newly
introduced variable to satisfy the type-hint requirement for relevant
annotatable variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new local variable `has_datasets` is introduced without a type hint.
Since this is new Python code and the value is clearly a boolean-like result
from `.scalar()`, it fits the custom rule requiring type hints on annotatable
variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=56c1ae644fd547268aba5f90638cf8ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=56c1ae644fd547268aba5f90638cf8ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/database/delete.py
**Line:** 81:86
**Comment:**
*Custom Rule: Add an explicit boolean type annotation for this newly
introduced variable to satisfy the type-hint requirement for relevant
annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=09b622dd25b3fe6bb32250912d9545c492bbef68dd0112ba2c98206046403dec&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=09b622dd25b3fe6bb32250912d9545c492bbef68dd0112ba2c98206046403dec&reaction=dislike'>👎</a>
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -200,27 +203,159 @@ def import_dataset( # noqa: C901
force_data: bool = False,
ignore_permissions: bool = False,
) -> SqlaTable:
+ """Import a dataset from a config dict, handling existing matches.
+
+ Permission model for an existing UUID match:
+
+ +--------------+---------------+---------------------+-----------------+
+ | Existing row | overwrite arg | Caller has perms? | Outcome |
+ +==============+===============+=====================+=================+
+ | alive | False | (n/a) | return existing |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write + owner | UPDATE in place |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write + owner | restore + UPDATE|
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | not can_write | raise (Case B) |
+ +--------------+---------------+---------------------+-----------------+
+
+ Re-importing a soft-deleted UUID is implicitly a restore-with-update:
+ the user is bringing the dataset back by uploading it again. We apply
+ the same ownership check as the explicit overwrite path so non-owners
+ cannot resurrect via re-import, and we raise rather than silently
+ returning a soft-deleted row to callers without write permission
+ (which would let them reattach charts/dashboards to a deleted dataset).
+ """
can_write = ignore_permissions or security_manager.can_access(
"can_write",
"Dataset",
)
- existing =
db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).first()
+ # `user` is None for background / example-loader paths (no Flask request
+ # user). Combined with ``can_write=True`` (typically from
+ # ``ignore_permissions=True``), the ownership checks in the restore /
+ # overwrite branches below are intentionally skipped because the caller has
+ # already established trust at the command level.
user = get_user()
- if existing:
- if overwrite and can_write and user:
- if user not in existing.owners and not security_manager.is_admin():
+ # Tracks whether we entered the soft-deleted mutation path so the
+ # downstream `sync` decision (below) can reflect that an
+ # implicit-restore re-import is a clean replacement, not a merge.
+ is_soft_deleted_match = False
Review Comment:
**Suggestion:** Add an explicit boolean type annotation for this newly
introduced state flag to satisfy the type-hint requirement for relevant
variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced local state flag in Python that can clearly be
annotated as a bool. The custom rule requires type hints on relevant variables
that can be annotated, so omitting the annotation is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cb6992b280164e8d85b2642b5b01cc9b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cb6992b280164e8d85b2642b5b01cc9b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 248:248
**Comment:**
*Custom Rule: Add an explicit boolean type annotation for this newly
introduced state flag to satisfy the type-hint requirement for relevant
variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=efdd3e8ee1ea3c45b464df91f504e686c3ee9c1f13faafa3d1d2eaef6af4fa5d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=efdd3e8ee1ea3c45b464df91f504e686c3ee9c1f13faafa3d1d2eaef6af4fa5d&reaction=dislike'>👎</a>
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -200,27 +203,159 @@ def import_dataset( # noqa: C901
force_data: bool = False,
ignore_permissions: bool = False,
) -> SqlaTable:
+ """Import a dataset from a config dict, handling existing matches.
+
+ Permission model for an existing UUID match:
+
+ +--------------+---------------+---------------------+-----------------+
+ | Existing row | overwrite arg | Caller has perms? | Outcome |
+ +==============+===============+=====================+=================+
+ | alive | False | (n/a) | return existing |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write + owner | UPDATE in place |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write + owner | restore + UPDATE|
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | not can_write | raise (Case B) |
+ +--------------+---------------+---------------------+-----------------+
+
+ Re-importing a soft-deleted UUID is implicitly a restore-with-update:
+ the user is bringing the dataset back by uploading it again. We apply
+ the same ownership check as the explicit overwrite path so non-owners
+ cannot resurrect via re-import, and we raise rather than silently
+ returning a soft-deleted row to callers without write permission
+ (which would let them reattach charts/dashboards to a deleted dataset).
+ """
can_write = ignore_permissions or security_manager.can_access(
"can_write",
"Dataset",
)
- existing =
db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).first()
+ # `user` is None for background / example-loader paths (no Flask request
+ # user). Combined with ``can_write=True`` (typically from
+ # ``ignore_permissions=True``), the ownership checks in the restore /
+ # overwrite branches below are intentionally skipped because the caller has
+ # already established trust at the command level.
user = get_user()
- if existing:
- if overwrite and can_write and user:
- if user not in existing.owners and not security_manager.is_admin():
+ # Tracks whether we entered the soft-deleted mutation path so the
+ # downstream `sync` decision (below) can reflect that an
+ # implicit-restore re-import is a clean replacement, not a merge.
+ is_soft_deleted_match = False
+
+ if existing := find_existing_for_import(SqlaTable, config["uuid"]):
+ if existing.deleted_at is not None:
+ # RESTORE path — re-importing a soft-deleted UUID is an implicit
+ # restore-with-update, a distinct operation from overwriting an
+ # alive row, so it is handled in its own branch.
+ if not can_write:
+ # Case B: don't silently return a soft-deleted row to a caller
+ # without write permission — that would let the importer
+ # reattach charts/dashboards to a deleted dataset and produce
+ # broken charts.
+ raise ImportFailedError(
+ "Dataset was deleted and re-import requires can_write "
+ "permission to restore it"
+ )
+ # ``user`` is None on background / example-loader paths; combined
+ # with ``can_write`` (typically from ``ignore_permissions=True``)
+ # the ownership check is intentionally skipped because the caller
+ # already established trust.
+ if user and (
+ user not in existing.owners and not security_manager.is_admin()
+ ):
raise ImportFailedError(
- "A dataset already exists and user doesn't "
- "have permissions to overwrite it"
+ "A dataset already exists and user doesn't have "
+ "permissions to restore it"
)
- if not overwrite or not can_write:
- return existing
- config["id"] = existing.id
+ # Before clearing ``deleted_at``, refuse if another active dataset
+ # already references the same physical table. Otherwise the
+ # restore would produce two live ``SqlaTable`` rows for one
+ # physical table, breaking the logical-uniqueness contract. The
+ # shared DAO helper relies on the SoftDeleteMixin listener to
+ # consider only active rows, excludes ``existing`` itself via
+ # ``id !=``, and normalizes the catalog the same way create/update
+ # uniqueness does.
+ if DatasetDAO.has_active_logical_duplicate(existing):
+ raise ImportFailedError(
+ "Dataset cannot be restored via re-import because another "
+ "active dataset already references the same physical table"
+ )
+ # Restore in place (clear ``deleted_at``) rather than
+ # hard-delete-and-replace: a hard delete would cascade through the
+ # chart back-reference and the delete-orphan child relationships
+ # (table_columns, sql_metrics), which the import would then need to
+ # reconstruct.
+ #
+ # How the restore lands as an UPDATE: clearing
+ # ``existing.deleted_at`` marks the in-session row dirty and the
+ # explicit flush emits the ``deleted_at = NULL`` UPDATE before
+ # ``SqlaTable.import_from_dict`` (below) does its own query-by-uuid
+ # lookup. Without the flush we would be relying on autoflush ahead
+ # of that internal query — correct under default session config but
+ # a hidden contract; the explicit flush makes it robust. The lookup
+ # then finds the now-live row (the listener filters ``deleted_at IS
+ # NULL``) and ``import_from_dict`` applies the config as field
+ # updates on the existing object, preserving the PK. Note:
+ # ``config["id"]`` is set defensively, but
+ # ``ImportExportMixin.import_from_dict`` strips it because
+ # ``SqlaTable.export_fields`` does not contain "id"; what actually
+ # binds to the existing row is the uuid uniqueness constraint used
+ # inside ``import_from_dict``.
+ #
+ # Snapshot ``deleted_at`` first so we can roll back the clear if
the
+ # downstream ``import_from_dict`` hits the legacy
+ # ``MultipleResultsFound`` fallback. Without the rollback, an
+ # ambiguous import would leave the dataset half-restored
+ # (``deleted_at = None`` but upload contents unapplied).
+ original_deleted_at = existing.deleted_at
+ existing.restore()
+ db.session.flush()
+ is_soft_deleted_match = True
+ config["id"] = existing.id
+ else:
+ # OVERWRITE path — existing alive row. Without ``overwrite`` or
+ # write permission, return it unchanged (the pre-soft-delete
+ # overwrite-without-permission behaviour).
+ if not overwrite or not can_write:
+ return existing
+ if user and (
+ user not in existing.owners and not security_manager.is_admin()
+ ):
+ raise ImportFailedError(
+ "A dataset already exists and user doesn't have "
+ "permissions to overwrite it"
+ )
+ config["id"] = existing.id
elif not can_write:
raise ImportFailedError(
"Dataset doesn't exist and user doesn't have permission to create
datasets"
)
+ else:
+ # Creating a brand-new dataset (no UUID match). A soft-deleted dataset
+ # may still claim this physical table; ``import_from_dict`` cannot see
it
+ # (the visibility filter hides soft-deleted rows), so without this
guard
+ # the import would create an active twin of a hidden dataset. The REST
+ # create path blocks the same collision via ``validate_uniqueness`` —
+ # mirror it here and direct the user to restore the existing dataset
+ # instead of leaving two rows for one physical table.
+ database = (
+
db.session.query(Database).filter_by(id=config["database_id"]).first()
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly added query
result variable to comply with the rule requiring type hints on relevant
variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new local lookup can be annotated as an optional Database reference.
The custom rule specifically requires type hints on relevant variables that can
be annotated, so this omission is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d8f7620201dc4bd9bfb60120501f598a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d8f7620201dc4bd9bfb60120501f598a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 347:349
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly added
query result variable to comply with the rule requiring type hints on relevant
variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=bc11c4fdf777a4cfc044aa0e5b61bcae0a6bd5f294b8165e9c5575b0362ba930&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=bc11c4fdf777a4cfc044aa0e5b61bcae0a6bd5f294b8165e9c5575b0362ba930&reaction=dislike'>👎</a>
##########
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
Review Comment:
**Suggestion:** Add an explicit class-variable type annotation for `dao` to
satisfy the type-hint requirement for newly introduced variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced class variable in Python and it is left
unannotated. The custom rule requires type hints on relevant variables that can
be annotated, so this omission is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=02e92168d879465e8dc52e97d3b3ec0c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=02e92168d879465e8dc52e97d3b3ec0c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/restore.py
**Line:** 46:46
**Comment:**
*Custom Rule: Add an explicit class-variable type annotation for `dao`
to satisfy the type-hint requirement for newly introduced variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=fe1fc76a6e9bf3f73bd7bf479816001d4599184064d9d7b97a8678e92046e47b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=fe1fc76a6e9bf3f73bd7bf479816001d4599184064d9d7b97a8678e92046e47b&reaction=dislike'>👎</a>
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -200,27 +203,159 @@ def import_dataset( # noqa: C901
force_data: bool = False,
ignore_permissions: bool = False,
) -> SqlaTable:
+ """Import a dataset from a config dict, handling existing matches.
+
+ Permission model for an existing UUID match:
+
+ +--------------+---------------+---------------------+-----------------+
+ | Existing row | overwrite arg | Caller has perms? | Outcome |
+ +==============+===============+=====================+=================+
+ | alive | False | (n/a) | return existing |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write + owner | UPDATE in place |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write + owner | restore + UPDATE|
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | not can_write | raise (Case B) |
+ +--------------+---------------+---------------------+-----------------+
+
+ Re-importing a soft-deleted UUID is implicitly a restore-with-update:
+ the user is bringing the dataset back by uploading it again. We apply
+ the same ownership check as the explicit overwrite path so non-owners
+ cannot resurrect via re-import, and we raise rather than silently
+ returning a soft-deleted row to callers without write permission
+ (which would let them reattach charts/dashboards to a deleted dataset).
+ """
can_write = ignore_permissions or security_manager.can_access(
"can_write",
"Dataset",
)
- existing =
db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).first()
+ # `user` is None for background / example-loader paths (no Flask request
+ # user). Combined with ``can_write=True`` (typically from
+ # ``ignore_permissions=True``), the ownership checks in the restore /
+ # overwrite branches below are intentionally skipped because the caller has
+ # already established trust at the command level.
user = get_user()
- if existing:
- if overwrite and can_write and user:
- if user not in existing.owners and not security_manager.is_admin():
+ # Tracks whether we entered the soft-deleted mutation path so the
+ # downstream `sync` decision (below) can reflect that an
+ # implicit-restore re-import is a clean replacement, not a merge.
+ is_soft_deleted_match = False
+
+ if existing := find_existing_for_import(SqlaTable, config["uuid"]):
+ if existing.deleted_at is not None:
+ # RESTORE path — re-importing a soft-deleted UUID is an implicit
+ # restore-with-update, a distinct operation from overwriting an
+ # alive row, so it is handled in its own branch.
+ if not can_write:
+ # Case B: don't silently return a soft-deleted row to a caller
+ # without write permission — that would let the importer
+ # reattach charts/dashboards to a deleted dataset and produce
+ # broken charts.
+ raise ImportFailedError(
+ "Dataset was deleted and re-import requires can_write "
+ "permission to restore it"
+ )
+ # ``user`` is None on background / example-loader paths; combined
+ # with ``can_write`` (typically from ``ignore_permissions=True``)
+ # the ownership check is intentionally skipped because the caller
+ # already established trust.
+ if user and (
+ user not in existing.owners and not security_manager.is_admin()
+ ):
raise ImportFailedError(
- "A dataset already exists and user doesn't "
- "have permissions to overwrite it"
+ "A dataset already exists and user doesn't have "
+ "permissions to restore it"
)
- if not overwrite or not can_write:
- return existing
- config["id"] = existing.id
+ # Before clearing ``deleted_at``, refuse if another active dataset
+ # already references the same physical table. Otherwise the
+ # restore would produce two live ``SqlaTable`` rows for one
+ # physical table, breaking the logical-uniqueness contract. The
+ # shared DAO helper relies on the SoftDeleteMixin listener to
+ # consider only active rows, excludes ``existing`` itself via
+ # ``id !=``, and normalizes the catalog the same way create/update
+ # uniqueness does.
+ if DatasetDAO.has_active_logical_duplicate(existing):
+ raise ImportFailedError(
+ "Dataset cannot be restored via re-import because another "
+ "active dataset already references the same physical table"
+ )
+ # Restore in place (clear ``deleted_at``) rather than
+ # hard-delete-and-replace: a hard delete would cascade through the
+ # chart back-reference and the delete-orphan child relationships
+ # (table_columns, sql_metrics), which the import would then need to
+ # reconstruct.
+ #
+ # How the restore lands as an UPDATE: clearing
+ # ``existing.deleted_at`` marks the in-session row dirty and the
+ # explicit flush emits the ``deleted_at = NULL`` UPDATE before
+ # ``SqlaTable.import_from_dict`` (below) does its own query-by-uuid
+ # lookup. Without the flush we would be relying on autoflush ahead
+ # of that internal query — correct under default session config but
+ # a hidden contract; the explicit flush makes it robust. The lookup
+ # then finds the now-live row (the listener filters ``deleted_at IS
+ # NULL``) and ``import_from_dict`` applies the config as field
+ # updates on the existing object, preserving the PK. Note:
+ # ``config["id"]`` is set defensively, but
+ # ``ImportExportMixin.import_from_dict`` strips it because
+ # ``SqlaTable.export_fields`` does not contain "id"; what actually
+ # binds to the existing row is the uuid uniqueness constraint used
+ # inside ``import_from_dict``.
+ #
+ # Snapshot ``deleted_at`` first so we can roll back the clear if
the
+ # downstream ``import_from_dict`` hits the legacy
+ # ``MultipleResultsFound`` fallback. Without the rollback, an
+ # ambiguous import would leave the dataset half-restored
+ # (``deleted_at = None`` but upload contents unapplied).
+ original_deleted_at = existing.deleted_at
Review Comment:
**Suggestion:** Add a type annotation for this restored timestamp snapshot
variable (for example an optional datetime type) so the new variable is
explicitly typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new local variable stores a deleted_at snapshot and can be annotated
with an optional datetime type. Since the rule flags relevant variables that
can be annotated, the lack of a type hint is a valid violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=82e7f24b9d9541b5aa2a025836fd0f5c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=82e7f24b9d9541b5aa2a025836fd0f5c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 315:315
**Comment:**
*Custom Rule: Add a type annotation for this restored timestamp
snapshot variable (for example an optional datetime type) so the new variable
is explicitly typed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=c1f2d59fe30b82a9e20051e7e9864c026f6be72843633bf9ee044625b1819427&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=c1f2d59fe30b82a9e20051e7e9864c026f6be72843633bf9ee044625b1819427&reaction=dislike'>👎</a>
##########
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
Review Comment:
**Suggestion:** Add an explicit class-variable type annotation for
`restore_failed_exc` rather than relying on implicit typing. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is an untyped class attribute in newly added Python code. It can be
annotated, so it falls under the type-hint requirement.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4f3ad69e3b0c4b319269bc7a410e74cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4f3ad69e3b0c4b319269bc7a410e74cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/restore.py
**Line:** 49:49
**Comment:**
*Custom Rule: Add an explicit class-variable type annotation for
`restore_failed_exc` rather than relying on implicit typing.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=1fd5a338b6e183b6870bfdd288f26907b77b29118d688f1ee5004f9692010b3c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=1fd5a338b6e183b6870bfdd288f26907b77b29118d688f1ee5004f9692010b3c&reaction=dislike'>👎</a>
##########
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]
+ model = super().validate()
Review Comment:
**Suggestion:** Add a local variable annotation for `model` when assigning
the result of `super().validate()`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This local variable is introduced in modified Python code and is a clear
candidate for annotation. The custom rule requires type hints for relevant
variables that can be annotated, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b377d405de464182af8b96834394cbd8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b377d405de464182af8b96834394cbd8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/restore.py
**Line:** 52:52
**Comment:**
*Custom Rule: Add a local variable annotation for `model` when
assigning the result of `super().validate()`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=d5800e51437c9cca6ea4c16816b2e302b218de8f0400cdc390b6538f0505c21f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=d5800e51437c9cca6ea4c16816b2e302b218de8f0400cdc390b6538f0505c21f&reaction=dislike'>👎</a>
##########
superset/daos/dataset.py:
##########
@@ -163,20 +188,45 @@ def validate_uniqueness(
) -> bool:
# The catalog might not be set even if the database supports catalogs,
in case
# multi-catalog is disabled.
- catalog = table.catalog or database.get_default_catalog()
-
- dataset_query = db.session.query(SqlaTable).filter(
- SqlaTable.table_name == table.table,
- SqlaTable.schema == table.schema,
- SqlaTable.catalog == catalog,
- SqlaTable.database_id == database.id,
+ default_catalog = database.get_default_catalog()
+ catalog = table.catalog or default_catalog
Review Comment:
**Suggestion:** Add explicit type annotations for the newly introduced local
variables that hold catalog normalization values in this method. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new code introduces local variables that hold catalog normalization
values, and they are not type-annotated even though their types can be
expressed. This matches the rule requiring type hints on relevant variables in
modified Python code.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bc52bd8212454c0eb2c3e44574f6ce3e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bc52bd8212454c0eb2c3e44574f6ce3e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 191:192
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced
local variables that hold catalog normalization values in this method.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=e6fee46c1bca5a837e28891afb27a12fa5e0653624bf297573902952217545c7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=e6fee46c1bca5a837e28891afb27a12fa5e0653624bf297573902952217545c7&reaction=dislike'>👎</a>
##########
superset/daos/dataset.py:
##########
@@ -186,16 +236,111 @@ def validate_update_uniqueness(
) -> bool:
# The catalog might not be set even if the database supports catalogs,
in case
# multi-catalog is disabled.
- catalog = table.catalog or database.get_default_catalog()
-
- dataset_query = db.session.query(SqlaTable).filter(
- SqlaTable.table_name == table.table,
- SqlaTable.database_id == database.id,
- SqlaTable.schema == table.schema,
- SqlaTable.catalog == catalog,
- SqlaTable.id != dataset_id,
+ default_catalog = database.get_default_catalog()
+ catalog = table.catalog or default_catalog
Review Comment:
**Suggestion:** Add explicit type annotations for the newly introduced local
variables before building the soft-deleted duplicate query. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The code in the final file introduces local catalog-normalization variables
without explicit type hints, which falls under the stated requirement to
annotate relevant variables when possible.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dafa660c433a4c658dfdc151b5ad40a9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dafa660c433a4c658dfdc151b5ad40a9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 239:240
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced
local variables before building the soft-deleted duplicate query.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=f2e89b1d8319299fd28470b34e779e2e914b1f3d19fc1e8fa274ab2d742f7030&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=f2e89b1d8319299fd28470b34e779e2e914b1f3d19fc1e8fa274ab2d742f7030&reaction=dislike'>👎</a>
##########
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:
+ query = super().apply(query, value)
+ normalized = str(value).lower().strip() if value is not None else ""
Review Comment:
**Suggestion:** Add an explicit type annotation for this derived local value
to satisfy the required variable type-hint rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is new Python code adding a derived local variable without any type
annotation. The custom rule explicitly requires type hints for relevant
variables that can be annotated, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0d0720a2f80d429fa6c5c64001f16939&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0d0720a2f80d429fa6c5c64001f16939&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/datasets/filters.py
**Line:** 83:83
**Comment:**
*Custom Rule: Add an explicit type annotation for this derived local
value to satisfy the required variable type-hint rule.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=18c157936bcca6521b6e085155ef00ed915310e5569d1b76162df77f32cf3c5e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=18c157936bcca6521b6e085155ef00ed915310e5569d1b76162df77f32cf3c5e&reaction=dislike'>👎</a>
##########
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:
+ query = super().apply(query, value)
+ normalized = str(value).lower().strip() if value is not None else ""
+ if normalized not in {"include", "only"} or
security_manager.is_admin():
+ return query
+
+ # Non-admins may only see soft-deleted datasets they own. ``any()``
emits
+ # an EXISTS subquery so it composes with the base access filter without
+ # producing duplicate rows from a join.
+ owned = SqlaTable.owners.any(security_manager.user_model.id ==
get_user_id())
Review Comment:
**Suggestion:** Add a concrete type annotation for this SQLAlchemy
expression variable so newly added relevant variables are typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is also a newly introduced local variable in Python code and it lacks
an explicit type hint. Since it is a relevant variable that can be annotated,
it violates the type-hint rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=35e68edda4bc45ff846dc38f1fc6e600&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=35e68edda4bc45ff846dc38f1fc6e600&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/datasets/filters.py
**Line:** 90:90
**Comment:**
*Custom Rule: Add a concrete type annotation for this SQLAlchemy
expression variable so newly added relevant variables are typed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=d92a34c7f282c962e9e17344e61d6ddd45b73d868fafb76da2161ffabf3c6596&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=d92a34c7f282c962e9e17344e61d6ddd45b73d868fafb76da2161ffabf3c6596&reaction=dislike'>👎</a>
##########
superset/daos/dataset.py:
##########
@@ -186,16 +236,111 @@ def validate_update_uniqueness(
) -> bool:
# The catalog might not be set even if the database supports catalogs,
in case
# multi-catalog is disabled.
- catalog = table.catalog or database.get_default_catalog()
-
- dataset_query = db.session.query(SqlaTable).filter(
- SqlaTable.table_name == table.table,
- SqlaTable.database_id == database.id,
- SqlaTable.schema == table.schema,
- SqlaTable.catalog == catalog,
- SqlaTable.id != dataset_id,
+ default_catalog = database.get_default_catalog()
+ catalog = table.catalog or default_catalog
+
+ # Same rationale as ``validate_uniqueness`` above: a soft-deleted
+ # twin must block the update rather than being silently ignored.
+ # The bypass is session-scoped, not per-query, because the EXISTS
+ # subquery pattern below leaves the inner Query's execution_options
+ # invisible to the outer execute (where the listener fires).
+ #
+ # avoid app-init regression: see ``validate_uniqueness`` above — a
+ # module-top import from ``superset.models.helpers`` raises
+ # "App not initialized yet" outside an app context (see PR #40573).
+ from superset.models.helpers import ( # pylint:
disable=import-outside-toplevel
+ skip_visibility_filter,
)
- return not db.session.query(dataset_query.exists()).scalar()
+
+ with skip_visibility_filter(db.session, SqlaTable):
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == table.table,
+ SqlaTable.database_id == database.id,
+ SqlaTable.schema == table.schema,
+ DatasetDAO._catalog_identity_filter(catalog, default_catalog),
+ SqlaTable.id != dataset_id,
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def has_active_logical_duplicate(model: SqlaTable) -> bool:
+ """Return True iff another *active* dataset shares model's physical
table.
+
+ Physical identity is ``(database_id, catalog, schema, table_name)``.
+ ``model``'s catalog is normalized to the database default when unset —
+ the same rule ``validate_uniqueness``/``validate_update_uniqueness``
+ apply — so create, update, restore, and re-import agree on what counts
+ as the same physical table. Catalog matching is null-aware via
+ ``_catalog_identity_filter``: when the normalized catalog is the
database
+ default, both ``catalog = default`` and ``catalog IS NULL`` rows match,
+ so a default-catalog twin is caught whichever way either row stored its
+ catalog.
+
+ Unlike ``validate_uniqueness``, this does NOT use
+ ``skip_visibility_filter``: it relies on the ``SoftDeleteMixin``
listener
+ to auto-append ``deleted_at IS NULL`` so only *active* rows match. Do
not
+ add the bypass here — it would broaden the check to soft-deleted rows
and
+ silently refuse legitimate restores. ``id != model.id`` excludes the
row
+ itself.
+
+ Assumes an active app context and a session-attached ``model`` so
+ ``db.session`` and the lazy ``model.database`` relationship resolve.
+ """
+ # The catalog might not be set even if the database supports catalogs,
+ # in case multi-catalog is disabled.
+ default_catalog = model.database.get_default_catalog()
+ catalog = model.catalog or default_catalog
Review Comment:
**Suggestion:** Type-annotate the newly added local variables in this
duplicate-check method to comply with the rule on variable hints. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This duplicate-check helper introduces unannotated local variables for
catalog normalization. Since the rule explicitly covers relevant variables in
new or modified Python code, this is a verified violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5ff7f8965cbe483fb8baf0ad7957eb26&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5ff7f8965cbe483fb8baf0ad7957eb26&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 291:292
**Comment:**
*Custom Rule: Type-annotate the newly added local variables in this
duplicate-check method to comply with the rule on variable hints.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=f8e62317b4c40e9afbbd52b32a7b570eff97469b0d209f215a9afe8b8966f59c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=f8e62317b4c40e9afbbd52b32a7b570eff97469b0d209f215a9afe8b8966f59c&reaction=dislike'>👎</a>
--
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]
