From f4863c1410a892153189dabbd7963524e8a2e1fc Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Fri, 30 Oct 2026 12:20:44 -0600 Subject: [PATCH 1/2] fix(semantic-layer): preserve metadata generation on unchanged configuration Compare effective configuration content before invalidating metadata. Omit unchanged configuration writes so a stale no-op save cannot overwrite a concurrent edit without a new generation. Preserve explicit clearing and invalidation for actual changes. --- superset/commands/semantic_layer/update.py | 14 ++++- .../simple_cache_clear_test.py | 62 ++++++++++++++++++- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/superset/commands/semantic_layer/update.py b/superset/commands/semantic_layer/update.py index 3689ca315d93..d2b756f3ea18 100644 --- a/superset/commands/semantic_layer/update.py +++ b/superset/commands/semantic_layer/update.py @@ -219,7 +219,19 @@ def run(self) -> Model: self._properties["configuration"] ) if "configuration" in self._properties: - self._model.clear_metadata_cache() + try: + unchanged: bool = json.dumps( + json.loads(self._model.configuration or "{}"), sort_keys=True + ) == json.dumps( + json.loads(self._properties["configuration"]), sort_keys=True + ) + except (TypeError, ValueError): + unchanged = False + if unchanged: + # Avoid overwriting a concurrent edit with this reader's old config. + self._properties.pop("configuration") + else: + self._model.clear_metadata_cache() return SemanticLayerDAO.update(self._model, attributes=self._properties) def validate(self) -> None: diff --git a/tests/unit_tests/semantic_layers/simple_cache_clear_test.py b/tests/unit_tests/semantic_layers/simple_cache_clear_test.py index 8b71bf732314..77fbfede503d 100644 --- a/tests/unit_tests/semantic_layers/simple_cache_clear_test.py +++ b/tests/unit_tests/semantic_layers/simple_cache_clear_test.py @@ -69,18 +69,19 @@ def layer( @pytest.mark.parametrize( "properties,expected", [ - ({"configuration": {}}, 1), + ({"configuration": {}}, 0), + ({"configuration": {"value": "new"}}, 1), ({"description": "changed"}, 0), ], ) -def test_save_rotates_only_submitted_configuration( +def test_save_rotates_only_changed_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.""" + """Save rotates generation only when the effective configuration changes.""" command: UpdateSemanticLayerCommand = UpdateSemanticLayerCommand( str(layer.uuid), properties ) @@ -91,6 +92,61 @@ def test_save_rotates_only_submitted_configuration( assert session.get(SemanticLayer, layer.uuid).cache_version == expected +@pytest.mark.parametrize( + "stored,submitted,expected", + [ + ('{ "b": 2, "a": {"x": 1} }', {"a": {"x": 1}, "b": 2}, 0), + ('{"value": true}', {"value": 1}, 1), + ("invalid JSON", {"value": "repaired"}, 1), + ('{"items": [1, 2]}', {"items": [2, 1]}, 1), + ], +) +def test_save_compares_configuration_content( + app: Flask, + session: Session, + layer: SemanticLayer, + stored: str, + submitted: dict[str, Any], + expected: int, +) -> None: + """Ignore JSON formatting/key order, but preserve scalar types and list order.""" + layer.configuration = stored + session.commit() + command: UpdateSemanticLayerCommand = UpdateSemanticLayerCommand( + str(layer.uuid), {"configuration": submitted} + ) + command._model = layer + with app.app_context(), patch.object(command, "validate"): + command.run() + session.expire_all() + assert session.get(SemanticLayer, layer.uuid).cache_version == expected + + +def test_noop_save_preserves_a_concurrent_configuration_change( + app: Flask, + session: Session, + layer: SemanticLayer, +) -> None: + """A stale no-op reader must not overwrite a newer configuration/generation.""" + layer.configuration = '{ "a": 1, "b": 2 }' + session.commit() + assert layer.configuration == '{ "a": 1, "b": 2 }' + session.execute( + SemanticLayer.__table__.update() + .where(SemanticLayer.uuid == layer.uuid) + .values(configuration='{"new": true}', cache_version=1) + ) + command: UpdateSemanticLayerCommand = UpdateSemanticLayerCommand( + str(layer.uuid), {"configuration": {"b": 2, "a": 1}} + ) + command._model = layer + with app.app_context(), patch.object(command, "validate"): + command.run() + session.expire_all() + assert layer.configuration == '{"new": true}' + assert layer.cache_version == 1 + + def test_failed_save_rolls_back_configuration_and_version( app: Flask, session: Session, From 520429f4ef8912b771836124141f2746411110ea Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Sat, 31 Oct 2026 19:43:37 -0600 Subject: [PATCH 2/2] test(semantic-layer): expect masked no-op saves to preserve stored configuration The update command validates restored credentials but omits unchanged configuration writes to preserve generation and avoid stale-reader overwrites. Assert that contract instead of requiring a redundant rewrite, retaining checks for restored-secret validation and no cache invalidation. --- .../commands/semantic_layer/update_test.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/unit_tests/commands/semantic_layer/update_test.py b/tests/unit_tests/commands/semantic_layer/update_test.py index 2f2e3d995478..59bcc42c15f0 100644 --- a/tests/unit_tests/commands/semantic_layer/update_test.py +++ b/tests/unit_tests/commands/semantic_layer/update_test.py @@ -35,7 +35,6 @@ ) from superset.constants import PASSWORD_MASK from superset.exceptions import SupersetSecurityException -from superset.utils import json def test_update_semantic_view_success(mocker: MockerFixture) -> None: @@ -648,11 +647,11 @@ def test_update_semantic_layer_preserves_masked_secret_end_to_end( ) -> None: """A name-only PUT that round-trips the masked GET response does not overwrite the stored credential with the literal mask.""" - mock_model = MagicMock() + mock_model: MagicMock = MagicMock() mock_model.type = "snowflake" mock_model.configuration = '{"account": "test", "password": "hunter2"}' - dao = mocker.patch( + dao: MagicMock = mocker.patch( "superset.commands.semantic_layer.update.SemanticLayerDAO", ) dao.find_by_uuid.return_value = mock_model @@ -662,7 +661,7 @@ def test_update_semantic_layer_preserves_masked_secret_end_to_end( "superset.commands.semantic_layer.update.current_user_can_modify_object", ) - mock_cls = MagicMock() + mock_cls: MagicMock = MagicMock() mock_cls.get_configuration_schema.return_value = { "properties": {"password": {"type": "string", "writeOnly": True}} } @@ -672,7 +671,7 @@ def test_update_semantic_layer_preserves_masked_secret_end_to_end( clear=True, ) - data = { + data: dict[str, Any] = { "name": "Renamed", "configuration": {"account": "test", "password": PASSWORD_MASK}, } @@ -683,11 +682,10 @@ def test_update_semantic_layer_preserves_masked_secret_end_to_end( ) dao.update.assert_called_once_with( mock_model, - attributes={ - "name": "Renamed", - "configuration": json.dumps({"account": "test", "password": "hunter2"}), - }, + attributes={"name": "Renamed"}, ) + assert mock_model.configuration == '{"account": "test", "password": "hunter2"}' + mock_model.clear_metadata_cache.assert_not_called() @pytest.mark.parametrize("registered", [True, False])