You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/10/02 22:06:51 UTC
[PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
mikebridge opened a new pull request, #44925:
URL: https://github.com/apache/superset/pull/44925
### SUMMARY
Restoring a chart version whose datasource (a semantic view or dataset) had since been deleted reported success and left the chart pointing at nothing.
- The restore now refuses with a 422 when the snapshot's datasource no longer exists, and leaves the chart unchanged: no partial write and no new version row.
- The datasource row is locked in the restore's transaction before the chart is written, so a concurrent delete cannot slip in between the check and the write. The datasource is locked before the chart, the same order a rename uses.
- A soft-deleted dataset still counts as existing, since it can be restored. When a chart is restored onto one, its permission fields are now copied from that dataset; previously they kept the previous dataset's values (a stale chart permission after restore).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend).
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/versioning/test_restore.py tests/unit_tests/versioning/test_restore_missing_datasource.py tests/unit_tests/versioning/test_restore_datasource_race.py`
- The tests run real capture-enabled restores: the missing-datasource refusal and its 422 mapping, the soft-deleted restore with all three permission fields asserted, a two-connection test where a delete races the restore on a file-backed SQLite database, and a lock-order test. Each fails without its fix.
- **Not run:** the concurrency behaviour on PostgreSQL and MySQL. SQLite serialises writers, so the row-lock path (`FOR UPDATE`) is exercised there only for statement shape.
- Known limit: a chart snapshot stores the datasource type and integer ID, not a UUID, so a purged datasource whose ID has been reused by a different row still passes the existence check.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
๐ค Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44925:
URL: https://github.com/apache/superset/pull/44925#issuecomment-5962449298
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44925?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `77.35849%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.17%. Comparing base ([`240a426`](https://app.codecov.io/gh/apache/superset/commit/240a4267f856183a4dd6bf7f00af341c5aca3d02?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7d3a998`](https://app.codecov.io/gh/apache/superset/commit/7d3a9980b218c746d6c80eb696b7287a92ca0a51?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 9 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44925?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/versioning/restore.py](https://app.codecov.io/gh/apache/superset/pull/44925?src=pr&el=tree&filepath=superset%2Fversioning%2Frestore.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmVyc2lvbmluZy9yZXN0b3JlLnB5) | 74.41% | [5 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/44925?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/versioning/api\_helpers.py](https://app.codecov.io/gh/apache/superset/pull/44925?src=pr&el=tree&filepath=superset%2Fversioning%2Fapi_helpers.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmVyc2lvbmluZy9hcGlfaGVscGVycy5weQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44925?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44925 +/- ##
===========================================
- Coverage 82.17% 66.17% -16.01%
===========================================
Files 2995 2995
Lines 184993 185064 +71
Branches 42818 42826 +8
===========================================
- Hits 152025 122463 -29562
- Misses 30209 59911 +29702
+ Partials 2759 2690 -69
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44925/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44925/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.29% <20.75%> (-0.02%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44925/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44925/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.36% <77.35%> (+<0.01%)` | :arrow_up: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44925/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.16% <20.75%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44925/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.45% <77.35%> (-30.68%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44925/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44925/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44925?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44925:
URL: https://github.com/apache/superset/pull/44925#issuecomment-5962230185
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44925:
URL: https://github.com/apache/superset/pull/44925#discussion_r4209366091
##########
superset/versioning/restore.py:
##########
@@ -135,6 +136,97 @@ def __init__(self, model_name: str, detail: str) -> None:
)
+class MissingDatasourceError(Exception):
+ """The chart version's datasource no longer exists.
+
+ A version snapshot records the chart's datasource by type and id, but
+ restoring a snapshot does not restore the datasource itself. Restoring
+ onto a deleted one would leave the chart resolving no datasource while
+ reporting success, so the restore refuses instead. The message is
+ user-facing.
+ """
+
+ def __init__(self, datasource_label: str) -> None:
+ message: str = (
+ _(
+ "This chart version can't be restored because the semantic view "
+ "it used has been permanently deleted. Restore a version that "
+ "uses an existing semantic view or dataset, or recreate the chart."
+ )
+ if datasource_label == "semantic view"
+ else _(
+ "This chart version can't be restored because the dataset it "
+ "used has been permanently deleted. Restore a version that uses "
+ "an existing dataset, or recreate the chart."
+ )
+ )
+ super().__init__(message)
+
+
+def _lock_chart_datasource(model_cls: type, target_version: Any) -> Any | None:
+ """Lock and return the datasource a chart snapshot names, or refuse.
+
+ Restoring a snapshot does not restore its datasource, so the restore
+ refuses when that datasource no longer exists. The row is locked in the
+ restore's transaction, so it cannot be deleted between this check and
+ the chart write; SQLite drops ``FOR UPDATE``, so its write lock is
+ reserved before the lookup instead.
+
+ A soft-deleted dataset still counts as existing: it is restorable from
+ the trash with its permissions intact, so the restored chart works again
+ once the dataset is restored. A snapshot without a datasource id has no
+ dependency to check and returns ``None``, as does any non-chart model.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.datasource import Datasource, DatasourceDAO
+ from superset.models.helpers import skip_visibility_filter
+ from superset.utils.core import DatasourceType
+
+ if model_cls.__name__ != "Slice":
+ return None
+ datasource_id: int | None = target_version.datasource_id
+ if not datasource_id:
+ return None
+ datasource_type: str = target_version.datasource_type
+ label: str = (
+ "semantic view"
+ if datasource_type == DatasourceType.SEMANTIC_VIEW.value
+ else "dataset"
+ )
+ src_class: type[Datasource] | None = DatasourceDAO.sources.get(datasource_type)
+ if src_class is None:
+ raise MissingDatasourceError(label)
+ _reserve_sqlite_write_lock()
+ with skip_visibility_filter(db.session, src_class):
+ # ``populate_existing`` re-reads a row the session already holds, so a
+ # rename committed before the lock is not hidden by a stale copy.
+ datasource: Datasource | None = (
+ db.session.query(src_class)
+ .populate_existing()
+ .enable_eagerloads(False)
+ .filter_by(id=datasource_id)
+ .with_for_update()
+ .one_or_none()
+ )
+ if datasource is None:
+ raise MissingDatasourceError(label)
+ return datasource
+
+
+def _sync_chart_perms(chart: Any, datasource: Any | None) -> None:
+ """Copy the restored datasource's permission fields onto the chart.
+
+ The chart's own listener resolves its datasource through the soft-delete
+ visibility filter, so for a soft-deleted dataset it would keep the
+ previous datasource's permission fields.
+ """
+ if datasource is None:
+ return
+ chart.perm = datasource.perm
+ chart.catalog_perm = datasource.catalog_perm
+ chart.schema_perm = datasource.schema_perm
Review Comment:
โ
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `78b38da`.
Permission fields are read with `getattr(..., None)`, so missing `catalog_perm` or `perm` attributes no longer raise `AttributeError`.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44925:
URL: https://github.com/apache/superset/pull/44925#discussion_r4203613160
##########
superset/versioning/restore.py:
##########
@@ -135,6 +136,97 @@ def __init__(self, model_name: str, detail: str) -> None:
)
+class MissingDatasourceError(Exception):
+ """The chart version's datasource no longer exists.
+
+ A version snapshot records the chart's datasource by type and id, but
+ restoring a snapshot does not restore the datasource itself. Restoring
+ onto a deleted one would leave the chart resolving no datasource while
+ reporting success, so the restore refuses instead. The message is
+ user-facing.
+ """
+
+ def __init__(self, datasource_label: str) -> None:
+ message: str = (
+ _(
+ "This chart version can't be restored because the semantic view "
+ "it used has been permanently deleted. Restore a version that "
+ "uses an existing semantic view or dataset, or recreate the chart."
+ )
+ if datasource_label == "semantic view"
+ else _(
+ "This chart version can't be restored because the dataset it "
+ "used has been permanently deleted. Restore a version that uses "
+ "an existing dataset, or recreate the chart."
+ )
+ )
+ super().__init__(message)
+
+
+def _lock_chart_datasource(model_cls: type, target_version: Any) -> Any | None:
+ """Lock and return the datasource a chart snapshot names, or refuse.
+
+ Restoring a snapshot does not restore its datasource, so the restore
+ refuses when that datasource no longer exists. The row is locked in the
+ restore's transaction, so it cannot be deleted between this check and
+ the chart write; SQLite drops ``FOR UPDATE``, so its write lock is
+ reserved before the lookup instead.
+
+ A soft-deleted dataset still counts as existing: it is restorable from
+ the trash with its permissions intact, so the restored chart works again
+ once the dataset is restored. A snapshot without a datasource id has no
+ dependency to check and returns ``None``, as does any non-chart model.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.datasource import Datasource, DatasourceDAO
+ from superset.models.helpers import skip_visibility_filter
+ from superset.utils.core import DatasourceType
+
+ if model_cls.__name__ != "Slice":
+ return None
+ datasource_id: int | None = target_version.datasource_id
+ if not datasource_id:
+ return None
+ datasource_type: str = target_version.datasource_type
+ label: str = (
+ "semantic view"
+ if datasource_type == DatasourceType.SEMANTIC_VIEW.value
+ else "dataset"
+ )
+ src_class: type[Datasource] | None = DatasourceDAO.sources.get(datasource_type)
+ if src_class is None:
+ raise MissingDatasourceError(label)
+ _reserve_sqlite_write_lock()
+ with skip_visibility_filter(db.session, src_class):
+ # ``populate_existing`` re-reads a row the session already holds, so a
+ # rename committed before the lock is not hidden by a stale copy.
+ datasource: Datasource | None = (
+ db.session.query(src_class)
+ .populate_existing()
+ .enable_eagerloads(False)
+ .filter_by(id=datasource_id)
+ .with_for_update()
+ .one_or_none()
+ )
+ if datasource is None:
+ raise MissingDatasourceError(label)
+ return datasource
+
+
+def _sync_chart_perms(chart: Any, datasource: Any | None) -> None:
+ """Copy the restored datasource's permission fields onto the chart.
+
+ The chart's own listener resolves its datasource through the soft-delete
+ visibility filter, so for a soft-deleted dataset it would keep the
+ previous datasource's permission fields.
+ """
+ if datasource is None:
+ return
+ chart.perm = datasource.perm
+ chart.catalog_perm = datasource.catalog_perm
+ chart.schema_perm = datasource.schema_perm
Review Comment:
Agree with codeant: `Query` lacks `catalog_perm` and `SavedQuery` lacks `perm`, so restoring a `query`/`saved_query` chart version raises a bare `AttributeError` (500) where master returns 422. `getattr` restores the 422; `set_related_perm` still raises on flush, so these restores won't complete.
```suggestion
chart.perm = getattr(datasource, "perm", None)
chart.catalog_perm = getattr(datasource, "catalog_perm", None)
chart.schema_perm = getattr(datasource, "schema_perm", None)
```
##########
superset/versioning/restore.py:
##########
@@ -135,6 +136,97 @@ def __init__(self, model_name: str, detail: str) -> None:
)
+class MissingDatasourceError(Exception):
+ """The chart version's datasource no longer exists.
+
+ A version snapshot records the chart's datasource by type and id, but
+ restoring a snapshot does not restore the datasource itself. Restoring
+ onto a deleted one would leave the chart resolving no datasource while
+ reporting success, so the restore refuses instead. The message is
+ user-facing.
+ """
+
+ def __init__(self, datasource_label: str) -> None:
+ message: str = (
+ _(
+ "This chart version can't be restored because the semantic view "
+ "it used has been permanently deleted. Restore a version that "
+ "uses an existing semantic view or dataset, or recreate the chart."
+ )
+ if datasource_label == "semantic view"
+ else _(
+ "This chart version can't be restored because the dataset it "
+ "used has been permanently deleted. Restore a version that uses "
+ "an existing dataset, or recreate the chart."
+ )
+ )
+ super().__init__(message)
+
+
+def _lock_chart_datasource(model_cls: type, target_version: Any) -> Any | None:
+ """Lock and return the datasource a chart snapshot names, or refuse.
+
+ Restoring a snapshot does not restore its datasource, so the restore
+ refuses when that datasource no longer exists. The row is locked in the
+ restore's transaction, so it cannot be deleted between this check and
+ the chart write; SQLite drops ``FOR UPDATE``, so its write lock is
+ reserved before the lookup instead.
+
+ A soft-deleted dataset still counts as existing: it is restorable from
+ the trash with its permissions intact, so the restored chart works again
+ once the dataset is restored. A snapshot without a datasource id has no
+ dependency to check and returns ``None``, as does any non-chart model.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.datasource import Datasource, DatasourceDAO
+ from superset.models.helpers import skip_visibility_filter
+ from superset.utils.core import DatasourceType
+
+ if model_cls.__name__ != "Slice":
+ return None
+ datasource_id: int | None = target_version.datasource_id
+ if not datasource_id:
+ return None
+ datasource_type: str = target_version.datasource_type
+ label: str = (
+ "semantic view"
+ if datasource_type == DatasourceType.SEMANTIC_VIEW.value
+ else "dataset"
+ )
+ src_class: type[Datasource] | None = DatasourceDAO.sources.get(datasource_type)
+ if src_class is None:
+ raise MissingDatasourceError(label)
Review Comment:
No test reaches this raise: changing it to `return None` keeps all 40 tests green, and an unknown-type snapshot then restores as success. A case with `datasource_type="nonexistent"` would pin it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44925:
URL: https://github.com/apache/superset/pull/44925#discussion_r4198790120
##########
superset/versioning/restore.py:
##########
@@ -135,6 +136,97 @@ def __init__(self, model_name: str, detail: str) -> None:
)
+class MissingDatasourceError(Exception):
+ """The chart version's datasource no longer exists.
+
+ A version snapshot records the chart's datasource by type and id, but
+ restoring a snapshot does not restore the datasource itself. Restoring
+ onto a deleted one would leave the chart resolving no datasource while
+ reporting success, so the restore refuses instead. The message is
+ user-facing.
+ """
+
+ def __init__(self, datasource_label: str) -> None:
+ message: str = (
+ _(
+ "This chart version can't be restored because the semantic view "
+ "it used has been permanently deleted. Restore a version that "
+ "uses an existing semantic view or dataset, or recreate the chart."
+ )
+ if datasource_label == "semantic view"
+ else _(
+ "This chart version can't be restored because the dataset it "
+ "used has been permanently deleted. Restore a version that uses "
+ "an existing dataset, or recreate the chart."
+ )
+ )
+ super().__init__(message)
+
+
+def _lock_chart_datasource(model_cls: type, target_version: Any) -> Any | None:
+ """Lock and return the datasource a chart snapshot names, or refuse.
+
+ Restoring a snapshot does not restore its datasource, so the restore
+ refuses when that datasource no longer exists. The row is locked in the
+ restore's transaction, so it cannot be deleted between this check and
+ the chart write; SQLite drops ``FOR UPDATE``, so its write lock is
+ reserved before the lookup instead.
+
+ A soft-deleted dataset still counts as existing: it is restorable from
+ the trash with its permissions intact, so the restored chart works again
+ once the dataset is restored. A snapshot without a datasource id has no
+ dependency to check and returns ``None``, as does any non-chart model.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.datasource import Datasource, DatasourceDAO
+ from superset.models.helpers import skip_visibility_filter
+ from superset.utils.core import DatasourceType
+
+ if model_cls.__name__ != "Slice":
+ return None
+ datasource_id: int | None = target_version.datasource_id
+ if not datasource_id:
+ return None
+ datasource_type: str = target_version.datasource_type
+ label: str = (
+ "semantic view"
+ if datasource_type == DatasourceType.SEMANTIC_VIEW.value
+ else "dataset"
+ )
+ src_class: type[Datasource] | None = DatasourceDAO.sources.get(datasource_type)
+ if src_class is None:
+ raise MissingDatasourceError(label)
+ _reserve_sqlite_write_lock()
+ with skip_visibility_filter(db.session, src_class):
+ # ``populate_existing`` re-reads a row the session already holds, so a
+ # rename committed before the lock is not hidden by a stale copy.
+ datasource: Datasource | None = (
+ db.session.query(src_class)
+ .populate_existing()
+ .enable_eagerloads(False)
+ .filter_by(id=datasource_id)
+ .with_for_update()
+ .one_or_none()
+ )
+ if datasource is None:
+ raise MissingDatasourceError(label)
+ return datasource
+
+
+def _sync_chart_perms(chart: Any, datasource: Any | None) -> None:
+ """Copy the restored datasource's permission fields onto the chart.
+
+ The chart's own listener resolves its datasource through the soft-delete
+ visibility filter, so for a soft-deleted dataset it would keep the
+ previous datasource's permission fields.
+ """
+ if datasource is None:
+ return
+ chart.perm = datasource.perm
+ chart.catalog_perm = datasource.catalog_perm
+ chart.schema_perm = datasource.schema_perm
Review Comment:
**Suggestion:** A legacy `query` datasource has no `catalog_perm`, and `SavedQuery` has no `perm`; restoring either raises `AttributeError` instead of completing.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Type error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a4f6fd729db04a6c815312cd74c3376b&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=a4f6fd729db04a6c815312cd74c3376b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/versioning/restore.py
**Line:** 225:227
**Comment:**
*Type Error: A legacy `query` datasource has no `catalog_perm`, and `SavedQuery` has no `perm`; restoring either raises `AttributeError` instead of completing.
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%2F44925&comment_hash=aca109af33c9284902ad3f65e7fcee89c3d7b42e47994263bc6f713097b75e0a&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44925&comment_hash=aca109af33c9284902ad3f65e7fcee89c3d7b42e47994263bc6f713097b75e0a&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44925:
URL: https://github.com/apache/superset/pull/44925#issuecomment-6022363363
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging.
- Chart restore copies permission fields unconditionally; legacy query or SavedQuery datasources may lack them and fail with AttributeError.
**Assessed commit:** `ec298fe27ae3`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44925:
URL: https://github.com/apache/superset/pull/44925
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44925:
URL: https://github.com/apache/superset/pull/44925#discussion_r4209371955
##########
superset/versioning/restore.py:
##########
@@ -135,6 +136,97 @@ def __init__(self, model_name: str, detail: str) -> None:
)
+class MissingDatasourceError(Exception):
+ """The chart version's datasource no longer exists.
+
+ A version snapshot records the chart's datasource by type and id, but
+ restoring a snapshot does not restore the datasource itself. Restoring
+ onto a deleted one would leave the chart resolving no datasource while
+ reporting success, so the restore refuses instead. The message is
+ user-facing.
+ """
+
+ def __init__(self, datasource_label: str) -> None:
+ message: str = (
+ _(
+ "This chart version can't be restored because the semantic view "
+ "it used has been permanently deleted. Restore a version that "
+ "uses an existing semantic view or dataset, or recreate the chart."
+ )
+ if datasource_label == "semantic view"
+ else _(
+ "This chart version can't be restored because the dataset it "
+ "used has been permanently deleted. Restore a version that uses "
+ "an existing dataset, or recreate the chart."
+ )
+ )
+ super().__init__(message)
+
+
+def _lock_chart_datasource(model_cls: type, target_version: Any) -> Any | None:
+ """Lock and return the datasource a chart snapshot names, or refuse.
+
+ Restoring a snapshot does not restore its datasource, so the restore
+ refuses when that datasource no longer exists. The row is locked in the
+ restore's transaction, so it cannot be deleted between this check and
+ the chart write; SQLite drops ``FOR UPDATE``, so its write lock is
+ reserved before the lookup instead.
+
+ A soft-deleted dataset still counts as existing: it is restorable from
+ the trash with its permissions intact, so the restored chart works again
+ once the dataset is restored. A snapshot without a datasource id has no
+ dependency to check and returns ``None``, as does any non-chart model.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.datasource import Datasource, DatasourceDAO
+ from superset.models.helpers import skip_visibility_filter
+ from superset.utils.core import DatasourceType
+
+ if model_cls.__name__ != "Slice":
+ return None
+ datasource_id: int | None = target_version.datasource_id
+ if not datasource_id:
+ return None
+ datasource_type: str = target_version.datasource_type
+ label: str = (
+ "semantic view"
+ if datasource_type == DatasourceType.SEMANTIC_VIEW.value
+ else "dataset"
+ )
+ src_class: type[Datasource] | None = DatasourceDAO.sources.get(datasource_type)
+ if src_class is None:
+ raise MissingDatasourceError(label)
Review Comment:
Good catch, thanks. The restore tests now include an actual snapshot with an unsupported datasource type, restored through `RestoreChartVersionCommand`. It has to raise `MissingDatasourceError` and leave the working chart unchanged. With the raise replaced by `return None`, the new test fails because the command reports a successful restore, and it passes with the guard restored. Added in https://github.com/apache/superset/commit/78b38da04218018eafa9394ef2c01a2c5b94ae79.
##########
superset/versioning/restore.py:
##########
@@ -135,6 +136,97 @@ def __init__(self, model_name: str, detail: str) -> None:
)
+class MissingDatasourceError(Exception):
+ """The chart version's datasource no longer exists.
+
+ A version snapshot records the chart's datasource by type and id, but
+ restoring a snapshot does not restore the datasource itself. Restoring
+ onto a deleted one would leave the chart resolving no datasource while
+ reporting success, so the restore refuses instead. The message is
+ user-facing.
+ """
+
+ def __init__(self, datasource_label: str) -> None:
+ message: str = (
+ _(
+ "This chart version can't be restored because the semantic view "
+ "it used has been permanently deleted. Restore a version that "
+ "uses an existing semantic view or dataset, or recreate the chart."
+ )
+ if datasource_label == "semantic view"
+ else _(
+ "This chart version can't be restored because the dataset it "
+ "used has been permanently deleted. Restore a version that uses "
+ "an existing dataset, or recreate the chart."
+ )
+ )
+ super().__init__(message)
+
+
+def _lock_chart_datasource(model_cls: type, target_version: Any) -> Any | None:
+ """Lock and return the datasource a chart snapshot names, or refuse.
+
+ Restoring a snapshot does not restore its datasource, so the restore
+ refuses when that datasource no longer exists. The row is locked in the
+ restore's transaction, so it cannot be deleted between this check and
+ the chart write; SQLite drops ``FOR UPDATE``, so its write lock is
+ reserved before the lookup instead.
+
+ A soft-deleted dataset still counts as existing: it is restorable from
+ the trash with its permissions intact, so the restored chart works again
+ once the dataset is restored. A snapshot without a datasource id has no
+ dependency to check and returns ``None``, as does any non-chart model.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.datasource import Datasource, DatasourceDAO
+ from superset.models.helpers import skip_visibility_filter
+ from superset.utils.core import DatasourceType
+
+ if model_cls.__name__ != "Slice":
+ return None
+ datasource_id: int | None = target_version.datasource_id
+ if not datasource_id:
+ return None
+ datasource_type: str = target_version.datasource_type
+ label: str = (
+ "semantic view"
+ if datasource_type == DatasourceType.SEMANTIC_VIEW.value
+ else "dataset"
+ )
+ src_class: type[Datasource] | None = DatasourceDAO.sources.get(datasource_type)
+ if src_class is None:
+ raise MissingDatasourceError(label)
+ _reserve_sqlite_write_lock()
+ with skip_visibility_filter(db.session, src_class):
+ # ``populate_existing`` re-reads a row the session already holds, so a
+ # rename committed before the lock is not hidden by a stale copy.
+ datasource: Datasource | None = (
+ db.session.query(src_class)
+ .populate_existing()
+ .enable_eagerloads(False)
+ .filter_by(id=datasource_id)
+ .with_for_update()
+ .one_or_none()
+ )
+ if datasource is None:
+ raise MissingDatasourceError(label)
+ return datasource
+
+
+def _sync_chart_perms(chart: Any, datasource: Any | None) -> None:
+ """Copy the restored datasource's permission fields onto the chart.
+
+ The chart's own listener resolves its datasource through the soft-delete
+ visibility filter, so for a soft-deleted dataset it would keep the
+ previous datasource's permission fields.
+ """
+ if datasource is None:
+ return
+ chart.perm = datasource.perm
+ chart.catalog_perm = datasource.catalog_perm
+ chart.schema_perm = datasource.schema_perm
Review Comment:
Good catch, thanks. Both the restore copy and the chart's flush listener (`Slice.set_related_perm`) now read `perm`, `catalog_perm` and `schema_perm` with `getattr(..., None)`, so fields a source type doesn't have are cleared instead of copied from the previous source. The listener change was needed as well because it reads those fields independently during flush. As a result, saving query and saved-query charts works again (they previously failed with an `AttributeError`), with no loosening of permissions. New command-level Query and SavedQuery restore cases failed on the original code and pass now, and they assert the restored source identity and every denormalized permission field. Added in https://github.com/apache/superset/commit/78b38da04218018eafa9394ef2c01a2c5b94ae79.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): refuse a chart restore whose datasource no longer exists [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44925:
URL: https://github.com/apache/superset/pull/44925#issuecomment-6003671907
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 68fbb09cf24a887afdae986bf02eb68ad3b41517 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ac419b10dd5530008d66977 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-44925--superset-docs-preview.netlify.app](https://deploy-preview-44925--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBCoAgEEDRu8y6AS3T6gYdIx0pyVJUiojunru_eP-FmMIRC0wADei0nGarnQ3ythNCovbB7JhsLiFZPFzO7lzxcvaunmz04ZmpHnIxgo-aM6K-7xhjA0k5KgXfDw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44925--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org