mikebridge commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510299750
##########
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:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
##########
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:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. 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
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. 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
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. 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]
+ model = super().validate()
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
##########
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:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
##########
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:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
--
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]
