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,