codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510301404
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for local variables whose types
are unambiguous from context, especially in test-scaffolding or similarly small
helper code.
**Applied to:**
- `superset/commands/database/**`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for obvious local variables/state
flags in this importer utility area when the type is unambiguous from context;
keep the code concise where inference is sufficient.
**Applied to:**
- `superset/commands/dataset/importers/v1/**`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing type annotations on local/test-scaffolding variables
when the type is already clear from context and the annotation would only add
noise without improving mypy coverage.
**Applied to:**
- `superset/commands/dataset/importers/v1/**`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
--
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]
