msyavuz commented on code in PR #43988:
URL: https://github.com/apache/superset/pull/43988#discussion_r3959532639
##########
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:
This isn't true since #43500: `CreateChartCommand.validate` rejects anything
but `table` (create.py:84) and the legacy explore view hard-codes `table`, so
no HTTP path reaches this branch or the `getattr` fallbacks. On master the SQL
Lab save already fails with a 422 from validate, not the 500 the unit test
docstring describes. The frontend change is the fix; is this listener change
still needed?
##########
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:
This passes on master unchanged: the 422 comes from
`CreateChartCommand.validate` before `set_related_perm` runs, same path as
`test_create_chart_from_saved_query_rejected_cleanly` above it.
##########
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:
Following up on the thread above with the soft-delete case: the
`do_orm_execute` visibility filter hides a soft-deleted dataset from this
query, so any flush of one of its charts (chart trash, since Slice is also
`SoftDeleteMixin`) nulls the perms. When the dataset is restored the join
passes again but the strings stay NULL, so the chart drops out of the
permission leg of `ChartFilter` until someone re-saves it. Previously the
still-correct strings survived.
##########
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:
`MagicMock(spec=SemanticView)` returns a mock for `catalog_perm`, but the
real property returns `None` (semantic_layers/models.py:684), so the
`catalog_perm is not None` assertion for `semantic_view` is testing the mock.
--
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]
