aminghadersohi commented on code in PR #45061: URL: https://github.com/apache/superset/pull/45061#discussion_r4215043471
########## tests/unit_tests/semantic_layers/test_runtime_flag.py: ########## @@ -0,0 +1,211 @@ +# 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. + +"""Runtime availability applies before datasource lookup or provider access.""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock, patch + +import pytest +from flask.testing import FlaskClient +from werkzeug.test import TestResponse + +from superset.daos.datasource import DatasourceDAO +from superset.explore.utils import check_semantic_view_access +from superset.semantic_layers.access import SemanticLayersDisabledError + + [email protected]("identifier", [17, "00000000-0000-0000-0000-000000000001"]) +def test_disabled_semantic_datasource_never_queries_metadata( + identifier: int | str, +) -> None: + """Both public UUIDs and internal IDs pass through the same refusal.""" + query: MagicMock + with ( + patch("superset.feature_flag_manager.is_feature_enabled", return_value=False), + patch("superset.daos.datasource.db.session.query") as query, + pytest.raises(SemanticLayersDisabledError), + ): + DatasourceDAO.get_datasource("semantic_view", identifier) + query.assert_not_called() + + [email protected]("kind,enabled", [("semantic_view", True), ("table", False)]) +def test_available_datasource_resolves(kind: str, enabled: bool) -> None: + """Enabled semantic views and disabled-flag ordinary tables still resolve.""" + query: MagicMock + expected: MagicMock = MagicMock() + with ( + patch("superset.feature_flag_manager.is_feature_enabled", return_value=enabled), + patch("superset.daos.datasource.db.session.query") as query, + ): + query.return_value.filter.return_value.one_or_none.return_value = expected + assert DatasourceDAO.get_datasource(kind, 17) is expected + + +def test_explore_direct_semantic_lookup_is_refused() -> None: + """The direct Explore access helper cannot bypass datasource resolution.""" + find: MagicMock + with ( + patch("superset.feature_flag_manager.is_feature_enabled", return_value=False), + patch("superset.explore.utils.SemanticViewDAO.find_by_id") as find, + pytest.raises(SemanticLayersDisabledError), + ): + check_semantic_view_access(17) + find.assert_not_called() + + +def test_dashboard_omits_disabled_semantic_metadata() -> None: + """Both dashboard serializers omit semantic entries before discovery.""" + metadata: MagicMock + serialize: MagicMock + from superset.mcp_service.dashboard.schemas import dashboard_datasets_serializer + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + from superset.semantic_layers.models import SemanticView + + view: SemanticView = SemanticView(id=17, name="private metadata") + chart: Slice = Slice( + id=4, datasource_id=17, datasource_type="semantic_view", semantic_view=view + ) + dashboard: Dashboard = Dashboard(id=3, dashboard_title="test", slices=[chart]) + with ( + patch("superset.feature_flag_manager.is_feature_enabled", return_value=False), + patch.object(SemanticView, "data_for_slices") as metadata, + patch( + "superset.mcp_service.dashboard.schemas._serialize_dashboard_dataset" + ) as serialize, + ): + assert dashboard.datasets_trimmed_for_slices() == [] + assert dashboard_datasets_serializer(dashboard).datasets == [] Review Comment: This MCP half of the test can't fail: with the `SEMANTIC_VIEW` skip removed from `dashboard_datasets_serializer`, the access check drops the view instead and `datasets == []` still holds. Asserting the count goes red without the guard (verified). ```suggestion result = dashboard_datasets_serializer(dashboard) assert result.datasets == [] assert result.inaccessible_dataset_count == 0 ``` ########## docs/admin_docs/configuration/feature-flags.mdx: ########## @@ -53,6 +53,34 @@ FEATURE_FLAGS = { } ``` +## Runtime semantic-layer availability + +`SEMANTIC_LAYERS` defaults to `False`. Both semantic-layer APIs are registered +at startup, and each request evaluates the active flag decision. Hosts can use +`IS_FEATURE_ENABLED_FUNC` and `GET_FEATURE_FLAGS_FUNC` to supply runtime decisions; +changing static configuration still requires a restart. Initialize permissions +with the normal `superset init` upgrade step. Enabling the runtime flag does not +grant permissions or require another permission sync. + +When disabled, semantic API routes return 404. Semantic-view queries and metadata +return “Semantic layers are not enabled.” Saved charts remain listed, but their +data and cached images are unavailable; dashboards omit semantic-view dataset +metadata. Chart saves using semantic views are refused. Both fresh and cached dashboard screenshots/thumbnails return 404 if any chart +uses a semantic view. Legacy batch warm-up records a per-chart unavailable error +and continues processing. Dual-source MCP tools still serve ordinary datasets, +and unscoped metric listings omit semantic views. The semantic-only MCP tools +are unavailable, while chart listings mark affected charts with an +`unavailable_reason`. Ordinary dataset queries remain available, and semantic +import/export retains its existing feature-off rejection. + +Background queries evaluate availability when the worker executes them. Browser +controls reflect the bootstrap flag on the next full page load; stale pages Review Comment: (nit) `feature_flags` is built inside `cached_common_bootstrap_data`, memoized for 60s per user/locale, so with a real `CACHE_CONFIG` the next page load can still carry the old flag. ```suggestion Background queries evaluate availability when the worker executes them. Browser controls reflect the bootstrap flag on a full page load once the cached bootstrap payload expires (up to 60 seconds with a configured `CACHE_CONFIG`); stale pages ``` -- 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]
