EnxDev commented on code in PR #43988:
URL: https://github.com/apache/superset/pull/43988#discussion_r3965706724
##########
superset/models/slice.py:
##########
@@ -496,13 +496,36 @@ def set_related_perm(_mapper: Mapper, _connection:
Connection, target: Slice) ->
# pylint: disable=import-outside-toplevel
from superset.daos.datasource import DatasourceDAO
- src_class = DatasourceDAO.sources[target.datasource_type]
+ # This listener also runs on updates, so clear the derived values before
+ # attempting to resolve the datasource. Otherwise an unmapped or deleted
+ # datasource can retain permission strings copied from a previous one.
+ target.perm = None
Review Comment:
Good catch. I had only considered a permanently deleted datasource, not the
soft-delete and restore flow. Nulling the permission strings would break that
flow exactly as you described, so I reverted the clearing behavior and kept the
existing listener unchanged in `103d165181`.
##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -704,6 +704,44 @@ def
test_create_chart_from_saved_query_rejected_cleanly(self):
db.session.delete(db.session.query(SavedQuery).get(saved_query_id))
db.session.commit()
+ def test_create_chart_from_sql_lab_query_rejected_cleanly(self):
Review Comment:
Agreed. This test only repeated validation that already exists on `master`,
so it did not cover the regression fixed by this PR. I removed it in
`103d165181`; the focused SaveModal test remains as the regression coverage.
##########
superset/models/slice.py:
##########
@@ -496,13 +496,36 @@ def set_related_perm(_mapper: Mapper, _connection:
Connection, target: Slice) ->
# pylint: disable=import-outside-toplevel
from superset.daos.datasource import DatasourceDAO
- src_class = DatasourceDAO.sources[target.datasource_type]
+ # This listener also runs on updates, so clear the derived values before
+ # attempting to resolve the datasource. Otherwise an unmapped or deleted
+ # datasource can retain permission strings copied from a previous one.
+ target.perm = None
+ target.catalog_perm = None
+ target.schema_perm = None
+
+ src_class = DatasourceDAO.sources.get(target.datasource_type)
+ if src_class is None:
+ # The chart API accepts every ``DatasourceType``, but only some of them
Review Comment:
Yes, you are right. I was reasoning from the behavior before #43500, but
`CreateChartCommand.validate` now rejects every non-table datasource before the
listener can run. The frontend ordering change is the actual fix, so I removed
the listener change and its unit coverage in `103d165181`.
##########
superset/models/slice.py:
##########
@@ -496,13 +496,36 @@ def set_related_perm(_mapper: Mapper, _connection:
Connection, target: Slice) ->
# pylint: disable=import-outside-toplevel
from superset.daos.datasource import DatasourceDAO
- src_class = DatasourceDAO.sources[target.datasource_type]
+ # This listener also runs on updates, so clear the derived values before
+ # attempting to resolve the datasource. Otherwise an unmapped or deleted
+ # datasource can retain permission strings copied from a previous one.
+ target.perm = None
Review Comment:
You were right to question this. The soft-delete and restore path means the
stored permission strings can still be valid and needed later, so clearing them
here is unsafe. I reverted the permission clearing and the rest of the listener
change in `103d165181`; the PR is focused on the SaveModal ordering fix now.
##########
tests/unit_tests/models/slice_test.py:
##########
@@ -229,6 +229,107 @@ def test_icons_escapes_datasource_html(self):
assert '"onmouseover' not in html
+def _run_set_related_perm(
+ datasource_type: str, *, with_existing_perms: bool = False
+) -> Slice:
+ """Run the perm-denormalizing listener against a stand-in datasource.
+
+ The stand-in is specced against the model class registered for
+ ``datasource_type``, so it exposes exactly the perm attributes that class
+ really defines -- the whole point being that they differ per type.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.datasource import DatasourceDAO
+
+ target = Slice()
+ target.datasource_type = datasource_type
+ target.datasource_id = 1
+ if with_existing_perms:
+ target.perm = "old-perm"
+ target.catalog_perm = "old-catalog-perm"
+ target.schema_perm = "old-schema-perm"
+
+ src_class = DatasourceDAO.sources.get(datasource_type)
+ datasource = MagicMock(spec=src_class) if src_class else None
+
+ with patch("superset.models.slice.db") as mock_db:
+ query = mock_db.session.query.return_value.filter_by.return_value
+ query.first.return_value = datasource
+ set_related_perm(None, None, target)
+
+ return target
+
+
[email protected]("datasource_type", ["table", "semantic_view"])
Review Comment:
Correct. That assertion was observing the `MagicMock`, not the real
`SemanticView.catalog_perm` behavior. Since the listener change was
unnecessary, I removed that test and the rest of the listener-specific coverage
in `103d165181`.
--
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]
