mikebridge commented on code in PR #45100: URL: https://github.com/apache/superset/pull/45100#discussion_r4227201523
########## superset/migrations/versions/2026-10-08_00-00_c179af013f48_add_semantic_layer_cache_version.py: ########## @@ -0,0 +1,42 @@ +# 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. + +"""Add a transactionally rotated semantic-layer cache generation. + +Revision ID: c179af013f48 +Revises: 00fab727cd0a +""" + +import sqlalchemy as sa + +from superset.migrations.shared.utils import add_columns, drop_columns + +revision: str = "c179af013f48" +down_revision: str = "00fab727cd0a" Review Comment: Good catch, thanks. `c179af013f48` now revises `e4a7c2b9d1f3` (ed8d2651c4), so the branch has a single Alembic head again. `superset db heads` reports only `c179af013f48`, and the single-head migration tests pass. The upgrade and downgrade bodies are unchanged; only the parent revision and its docstring moved. ########## tests/unit_tests/semantic_layers/simple_cache_clear_test.py: ########## @@ -0,0 +1,587 @@ +# 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. + +from __future__ import annotations + +from typing import Any +from unittest.mock import Mock, patch +from uuid import uuid4 + +import pytest +from flask import Flask +from sqlalchemy import select +from sqlalchemy.orm import Session + +from superset.commands.semantic_layer.exceptions import SemanticLayerUpdateFailedError +from superset.commands.semantic_layer.update import UpdateSemanticLayerCommand +from superset.semantic_layers.models import SemanticLayer, SemanticView + +pytestmark: pytest.MarkDecorator = pytest.mark.parametrize( + "app", [{"FEATURE_FLAGS": {"SEMANTIC_LAYERS": True}}], indirect=True +) + + [email protected] +def layer(session: Session) -> SemanticLayer: + """Persist a layer without involving a provider or permission event callbacks.""" + SemanticLayer.metadata.create_all(session.get_bind()) + session.execute( + SemanticLayer.__table__.insert().values( + uuid=uuid4(), + name="test", + type="test", + configuration="{}", + ) + ) + session.commit() + return session.scalars(select(SemanticLayer)).one() + + [email protected]( + "properties,expected", + [ + ({"configuration": {}}, 1), + ({"description": "changed"}, 0), + ], +) +def test_save_rotates_only_submitted_configuration( + app: Flask, + session: Session, + layer: SemanticLayer, + properties: dict[str, Any], + expected: int, +) -> None: + """Save writes its generation in the same transaction, even for unchanged config.""" + command: UpdateSemanticLayerCommand = UpdateSemanticLayerCommand( + str(layer.uuid), properties + ) + with app.app_context(), patch.object(command, "validate"): + command._model = layer + command.run() + session.expire_all() + assert session.get(SemanticLayer, layer.uuid).cache_version == expected + + +def test_failed_save_rolls_back_configuration_and_version( + app: Flask, + session: Session, + layer: SemanticLayer, +) -> None: + """Commit failure cannot publish a generation without the corresponding Save.""" + command: UpdateSemanticLayerCommand = UpdateSemanticLayerCommand( + str(layer.uuid), + {"configuration": {"value": "new"}}, + ) + command._model = layer + with ( + app.app_context(), + patch.object(command, "validate"), + patch.object( + session, + "commit", + side_effect=ValueError("failed commit"), + ), + pytest.raises(SemanticLayerUpdateFailedError), + ): + command.run() + session.expire_all() + assert layer.configuration == "{}" + assert layer.cache_version == 0 + + +def test_metadata_generation_is_captured_and_namespaced( + app: Flask, + session: Session, + layer: SemanticLayer, +) -> None: + """An operation cannot label old discovery with a newly rotated generation.""" + with app.app_context(): + first: str = layer.metadata_generation + layer.cache_version = 1 + assert layer.metadata_generation == first + session.info.clear() + second: str = layer.metadata_generation + assert second != first + app.config["SEMANTIC_LAYER_CACHE_NAMESPACE"] = "other-workspace" + session.info.clear() + assert layer.metadata_generation != second + app.config.pop("SEMANTIC_LAYER_CACHE_NAMESPACE") + + +def test_generation_binding_precedes_provider_construction( + app: Flask, + session: Session, + layer: SemanticLayer, +) -> None: + """The optional factory hook receives identity before eager discovery can run.""" + factory: Mock = Mock() + with ( + app.app_context(), + patch.dict("superset.semantic_layers.models.registry", {"test": factory}), + ): + assert ( + layer.implementation + is factory.from_configuration_with_cache_token.return_value + ) + factory.from_configuration_with_cache_token.assert_called_once_with( + {}, + cache_token=layer.metadata_generation, + ) + view: SemanticView = SemanticView(name="view", semantic_layer=layer) + assert view.metadata_generation == layer.metadata_generation + assert view.get_extra_cache_keys({}) == [layer.metadata_generation] + + +def test_host_generation_is_named_apart_from_provider_token( + app: Flask, + session: Session, + layer: SemanticLayer, +) -> None: + """Host keys use the metadata generation, never the provider's echoed token.""" + with app.app_context(): + view: SemanticView = SemanticView(name="view", semantic_layer=layer) + view.__dict__["implementation"] = Mock( + metadata_cache_token="provider-echo" # noqa: S106 + ) + assert view.metadata_generation == layer.metadata_generation + # Containment (#42760) still reads the host generation by its older name. + assert view.metadata_cache_token == view.metadata_generation + assert view.get_extra_cache_keys({}) == [layer.metadata_generation] + + [email protected]( + "allowed,guest,modifiable", + [ + (True, False, True), + (False, False, True), + (True, True, True), + (True, False, False), + ], +) +def test_clear_requires_management_authority_without_provider_work( + app: Flask, + session: Session, + layer: SemanticLayer, + allowed: bool, + guest: bool, + modifiable: bool, +) -> None: + """Clear uses the management gate and commits only for an authorized owner.""" + from superset.commands.semantic_layer.exceptions import SemanticLayerForbiddenError + from superset.commands.semantic_layer.update import ClearSemanticLayerCacheCommand + + security: Mock + with ( + app.app_context(), + patch( + "superset.commands.semantic_layer.update.security_manager", new=Mock() + ) as security, + patch( + "superset.commands.semantic_layer.update.current_user_can_modify_object", + return_value=modifiable, + ), + patch( + "superset.commands.semantic_layer.update.SemanticLayerDAO.find_by_uuid", + return_value=layer, + ), + patch.object( + layer, + "raise_for_access", + ), + patch.dict("superset.semantic_layers.models.registry", {}, clear=True), + ): + security.is_guest_user.return_value = guest + security.can_access.return_value = allowed + command: ClearSemanticLayerCacheCommand = ClearSemanticLayerCacheCommand( + str(layer.uuid) + ) + if allowed and not guest and modifiable: + command.run() + assert layer.cache_version == 1 + else: + with pytest.raises(SemanticLayerForbiddenError): + command.run() + assert layer.cache_version == 0 + assert "implementation" not in layer.__dict__ + + [email protected]( + "payload,status", [({}, 200), ({"version": 4}, 400), ([], 400)] +) +def test_clear_route_requires_empty_object( + client: Any, + full_api_access: None, + payload: Any, + status: int, +) -> None: + """The clear endpoint does not accept arbitrary configuration or versions.""" + command: Mock + with ( + patch("superset.semantic_layers.api.is_feature_enabled", return_value=True), + patch( + "superset.semantic_layers.api.ClearSemanticLayerCacheCommand", + create=True, + ) as command, + ): + response: Any = client.post( + f"/api/v1/semantic_layer/{uuid4()}/clear_cache", json=payload + ) + assert response.status_code == status + assert command.return_value.run.call_count == (status == 200) + + [email protected]("route", ["compatible", "column/category/values/"]) +def test_suggestion_cache_rotates_with_metadata_generation( + client: Any, + full_api_access: None, + route: str, +) -> None: + """Both endpoint caches partition generations without changing their query scope.""" + view: Mock = Mock(spec=SemanticView) + view.uid = "view" + view.cache_timeout = 60 + view.changed_on = None + view.type = "semantic_view" + view.metadata_generation = "generation-zero" + view.implementation.selection_identity_version = None + view.get_compatible_metrics.return_value = [] + view.get_compatible_dimensions.return_value = [] + view.values_for_column.return_value = ["one"] + cache: Mock + with ( + patch( + "superset.datasource.api.DatasourceDAO.get_datasource", return_value=view + ), + patch( + "superset.datasource.api.cache_manager", + ) as cache, + patch( + "superset.datasource.api.security_manager.get_rls_cache_key", + return_value=[], + ), + ): + cache.data_cache.get.return_value = None + endpoint: str = f"/api/v1/datasource/semantic_view/1/{route}" + + def request() -> Any: + return ( + client.post(endpoint, json={}) + if route == "compatible" + else client.get(endpoint) + ) + + assert request().status_code == 200 + first: str = cache.data_cache.get.call_args.args[0] + view.metadata_generation = "generation-one" + assert request().status_code == 200 + assert cache.data_cache.get.call_args.args[0] != first + + +def test_annotation_result_key_uses_saved_source_generation( + app: Flask, + session: Session, + layer: SemanticLayer, +) -> None: + """Annotation versions follow the datasource in the saved query context.""" + from superset.common.query_context_processor import QueryContextProcessor + + view: SemanticView = SemanticView(name="view", semantic_layer=layer) + chart: Mock = Mock(query_context='{"datasource":{"id":17,"type":"semantic_view"}}') + query: Mock = Mock(annotation_layers=[{"sourceType": "line", "value": 7}]) + processor: QueryContextProcessor = QueryContextProcessor(Mock()) + with ( + app.app_context(), + patch( + "superset.common.query_context_processor.ChartDAO.find_by_id", + return_value=chart, + ), + patch( + "superset.daos.datasource.DatasourceDAO.get_datasource", Review Comment: Agreed, the test could not tell the two datasources apart. It now captures the patched `get_datasource` and asserts it was called with `("semantic_view", 17)` (737b40e3a3). `_annotation_cache_context` runs twice in that test (before and after the generation bump), so it checks the full call list (`[call("semantic_view", 17)] * 2`) rather than `assert_called_once_with`. I checked that it fails when the code resolves the chart's own datasource instead of the one in the saved query context. -- 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]
