mikebridge commented on code in PR #45071:
URL: https://github.com/apache/superset/pull/45071#discussion_r4229122551
##########
superset/commands/semantic_layer/delete.py:
##########
@@ -32,11 +39,214 @@
)
from superset.commands.utils import current_user_can_modify_object
from superset.daos.semantic_layer import SemanticLayerDAO, SemanticViewDAO
+from superset.dashboards.filters import DashboardAccessFilter
from superset.exceptions import SupersetSecurityException
+from superset.models.dashboard import Dashboard, dashboard_slices
+from superset.models.slice import Slice
+from superset.reports.filters import ReportScheduleFilter
+from superset.reports.models import ReportSchedule
from superset.semantic_layers.models import SemanticLayer, SemanticView
+from superset.utils import json
from superset.utils.decorators import on_error, transaction
logger = logging.getLogger(__name__)
+_DEPENDENT_LIMIT: int = 20
+
+
+def _semantic_target_matches(target: object, view_ids: set[int]) -> bool:
+ """A missing type denotes a table, even when its integer ID collides."""
+ if not isinstance(target, dict) or target.get("datasourceType") !=
"semantic_view":
+ return False
+ raw_id: object = target.get("datasetId")
+ if isinstance(raw_id, bool) or not (
+ isinstance(raw_id, int) or (isinstance(raw_id, str) and
raw_id.isdecimal())
+ ):
+ return False
+ try:
+ return int(raw_id) in view_ids
+ except ValueError:
+ return False
+
+
+def _dashboard_targets_view(
+ metadata: str, view_ids: set[int], dashboard_id: int
+) -> bool:
+ """Match typed semantic targets in native filters or display controls."""
+ try:
+ decoded: object = json.loads(metadata)
+ except (TypeError, ValueError, RecursionError):
+ logger.debug("Skipping malformed targets on dashboard %s",
dashboard_id)
+ return False
+ if not isinstance(decoded, dict):
+ logger.debug("Skipping non-object targets on dashboard %s",
dashboard_id)
+ return False
+ for key in ("native_filter_configuration", "chart_customization_config"):
+ controls: object = decoded.get(key)
+ if not isinstance(controls, list):
+ continue
+ for control in controls:
+ if not isinstance(control, dict):
+ continue
+ targets: object = control.get("targets")
+ if isinstance(targets, list) and any(
+ _semantic_target_matches(target, view_ids) for target in
targets
+ ):
+ return True
+ return False
+
+
+def _dashboard_target_ids(view_ids: sa.Select) -> set[int]:
+ """Find live dashboards with semantic targets after a coarse SQL
prefilter."""
+ selected_ids: set[int] = set(db.session.execute(view_ids).scalars())
+ if not selected_ids:
+ return set()
+ candidates: sa.Select = sa.select(Dashboard.id,
Dashboard.json_metadata).where(
+ Dashboard.deleted_at.is_(None),
+ Dashboard.json_metadata.like("%semantic_view%"),
+ )
+ matched_ids: set[int] = set()
+ for dashboard_id, metadata in db.session.execute(
+ candidates.execution_options(yield_per=1000)
+ ):
+ if _dashboard_targets_view(metadata, selected_ids, dashboard_id):
+ matched_ids.add(dashboard_id)
+ return matched_ids
+
+
+def _dependent_assets(
+ view_ids: sa.Select,
+) -> tuple[int, list[dict[str, str | int]], int]:
+ """Count dependents, naming only assets visible through their list APIs."""
+ chart_ids: sa.Select = sa.select(Slice.id).where(
+ Slice.datasource_type == "semantic_view",
+ Slice.datasource_id.in_(view_ids),
+ Slice.deleted_at.is_(None),
+ )
+ chart_dashboard_ids: sa.Select = (
+ sa.select(dashboard_slices.c.dashboard_id)
+ .join(Slice, dashboard_slices.c.slice_id == Slice.id)
+ .join(Dashboard, dashboard_slices.c.dashboard_id == Dashboard.id)
+ .where(
+ Slice.id.in_(chart_ids),
+ Dashboard.deleted_at.is_(None),
+ )
+ .distinct()
+ )
+ dashboard_target_ids: set[int] = _dashboard_target_ids(view_ids)
+ dashboard_ids: sa.Select = sa.select(Dashboard.id).where(
+ Dashboard.deleted_at.is_(None),
+ sa.or_(
+ Dashboard.id.in_(chart_dashboard_ids),
+ Dashboard.id.in_(dashboard_target_ids),
+ ),
+ )
+ dependents: sa.Subquery = sa.union_all(
+ sa.select(
+ sa.literal("chart").label("type"),
+ Slice.id.label("id"),
+ sa.func.coalesce(Slice.slice_name,
str(_("Untitled"))).label("name"),
+ ).where(Slice.id.in_(chart_ids)),
+ sa.select(
+ sa.literal("dashboard").label("type"),
+ Dashboard.id.label("id"),
+ sa.func.coalesce(Dashboard.dashboard_title,
str(_("Untitled"))).label(
+ "name"
+ ),
+ ).where(Dashboard.id.in_(dashboard_ids)),
+ sa.select(
+ sa.func.lower(ReportSchedule.type).label("type"),
+ ReportSchedule.id.label("id"),
+ ReportSchedule.name.label("name"),
+ ).where(
+ ReportSchedule.active.is_(True),
+ sa.or_(
+ ReportSchedule.chart_id.in_(chart_ids),
+ ReportSchedule.dashboard_id.in_(dashboard_ids),
+ ),
+ ),
+ ).subquery()
+ visible_chart_ids: sa.Select = (
+ cast(
+ sa.Select,
+ ChartFilter("id", SQLAInterface(Slice, db.session))
+ .apply(db.session.query(Slice.id), None)
+ .statement,
+ )
+ if security_manager.can_access("can_read", "Chart")
+ else sa.select(Slice.id).where(sa.false())
+ )
+ visible_dashboard_ids: sa.Select = (
+ cast(
+ sa.Select,
+ DashboardAccessFilter("id", SQLAInterface(Dashboard, db.session))
+ .apply(db.session.query(Dashboard.id), None)
+ .statement,
+ )
+ if security_manager.can_access("can_read", "Dashboard")
+ else sa.select(Dashboard.id).where(sa.false())
Review Comment:
Thanks, agreed. The unreadable-dependent test now also seeds a dashboard
that contains the chart and an active report on it, with no read access to any
of them. It expects a total of 3, no names, and 3 inaccessible. With either the
Dashboard or the ReportSchedule else-arm set to `sa.true()`, that test fails.
Addressed in `7be89d9d33`.
##########
superset-frontend/src/pages/DatasetList/DatasetList.behavior.test.tsx:
##########
@@ -73,6 +73,106 @@ afterEach(async () => {
jest.restoreAllMocks();
});
+test('single semantic view delete displays the counted dependency conflict',
async () => {
+ const view = {
+ ...mockDatasets[0],
+ id: 99,
+ table_name: 'orders_semantic',
+ kind: 'semantic_view',
+ };
+ const reason =
+ 'Semantic source is used by 25 dependent assets and cannot be deleted.';
+ const addDangerToast = jest.fn();
+ mockDatasetListEndpoints({ result: [view], count: 1 });
+ fetchMock.delete('glob:*/api/v1/semantic_view/99', {
+ status: 409,
+ body: { message: reason },
+ });
+ renderDatasetList(mockAdminUser, {
+ addDangerToast,
+ addSuccessToast: jest.fn(),
+ });
+ await screen.findByText(view.table_name);
+ await userEvent.click(screen.getByTestId('dataset-row-delete'));
+ const modal = await screen.findByRole('dialog');
+ await userEvent.type(
+ within(modal).getByTestId('delete-modal-input'),
+ 'DELETE',
+ );
+ await userEvent.click(within(modal).getByRole('button', { name: 'Delete' }));
+ await waitFor(() =>
+ expect(addDangerToast).toHaveBeenCalledWith(
+ `There was an issue deleting orders_semantic: ${reason}`,
+ ),
+ );
+});
+
+test('mixed bulk delete names refused semantic views and their conflict
reason', async () => {
+ const dataset = mockDatasets[0];
+ const semanticView = {
+ ...mockDatasets[1],
+ id: 99,
+ table_name: 'orders_semantic',
+ kind: 'semantic_view',
+ };
+ const addDangerToast = jest.fn();
+ const reason =
+ 'Semantic source is used by 25 dependent assets and cannot be deleted.';
+ mockDatasetListEndpoints({ result: [dataset, semanticView], count: 2 });
+ fetchMock.get(API_ENDPOINTS.DATASET_BULK_RELATED_OBJECTS, {
+ charts: { count: 0, result: [] },
+ dashboards: { count: 0, result: [] },
+ });
+ fetchMock.delete(API_ENDPOINTS.DATASET_BULK_DELETE, { message: 'OK' });
+ fetchMock.delete('glob:*/api/v1/semantic_view/?q=*', {
+ status: 409,
+ body: {
+ message: reason,
+ total: 25,
+ dependents: [],
+ inaccessible_count: 25,
+ },
+ });
+ renderDatasetList(mockAdminUser, {
+ addDangerToast,
+ addSuccessToast: jest.fn(),
+ });
+ await screen.findByText(semanticView.table_name);
+ await userEvent.click(screen.getByRole('button', { name: /bulk select/i }));
+ const table = screen.getByTestId('listview-table');
+ for (const name of [dataset.table_name, semanticView.table_name]) {
+ const cell = await within(table).findByText(name);
+ await userEvent.click(within(cell.closest('tr')!).getByRole('checkbox'));
+ }
+ await waitFor(() =>
+ expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
+ '2 Selected',
+ ),
+ );
+ await userEvent.click(
+ within(screen.getByTestId('bulk-select-controls')).getByRole('button', {
+ name: 'Delete',
+ }),
+ );
+ const modal = await screen.findByRole('dialog');
+ await userEvent.type(
+ within(modal).getByTestId('delete-modal-input'),
+ 'DELETE',
+ );
+ await userEvent.click(within(modal).getByRole('button', { name: 'Delete' }));
+ await waitFor(() =>
+ expect(addDangerToast).toHaveBeenCalledWith(
+ `Could not delete semantic views orders_semantic: ${reason}`,
+ ),
+ );
Review Comment:
Applied, thanks: the mixed bulk case now asserts exactly one danger toast.
Dropping the `results[0].status === 'rejected'` check makes it fail with two
calls. Addressed in `7be89d9d33`.
--
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]
