diff --git a/pkgs/clan-cli/clan_lib/persist/fixtures/deferred.nix b/pkgs/clan-cli/clan_lib/persist/fixtures/deferred.nix index 5c9c9e6ba..6c844a9ca 100644 --- a/pkgs/clan-cli/clan_lib/persist/fixtures/deferred.nix +++ b/pkgs/clan-cli/clan_lib/persist/fixtures/deferred.nix @@ -9,6 +9,8 @@ let }; } { + # Mutable data + _file = "deferred.json"; foo = { a = { }; b = { }; @@ -16,7 +18,9 @@ let } # Merge the "inventory.json" - (builtins.fromJSON (builtins.readFile ./deferred.json)) + (lib.modules.setDefaultModuleLocation "deferred.json" ( + builtins.fromJSON (builtins.readFile ./deferred.json) + )) ]; }; in diff --git a/pkgs/clan-cli/clan_lib/persist/inventory_store.py b/pkgs/clan-cli/clan_lib/persist/inventory_store.py index 99fc6dcd8..1cac77351 100644 --- a/pkgs/clan-cli/clan_lib/persist/inventory_store.py +++ b/pkgs/clan-cli/clan_lib/persist/inventory_store.py @@ -218,6 +218,7 @@ class InventoryStore: current_priority, dict(data_eval), dict(data_disk), + inventory_file_name=self.inventory_file.name, ) return WriteInfo(write_map, data_eval, data_disk) diff --git a/pkgs/clan-cli/clan_lib/persist/inventory_store_test.py b/pkgs/clan-cli/clan_lib/persist/inventory_store_test.py index 2f9b34eb8..1364e39ae 100644 --- a/pkgs/clan-cli/clan_lib/persist/inventory_store_test.py +++ b/pkgs/clan-cli/clan_lib/persist/inventory_store_test.py @@ -11,6 +11,7 @@ from clan_lib.errors import ClanError from clan_lib.nix import nix_eval from clan_lib.persist.inventory_store import InventoryStore from clan_lib.persist.path_utils import delete_by_path, set_value_by_path +from clan_lib.persist.write_rules import PersistenceAttribute class MockFlake: @@ -120,16 +121,18 @@ def test_simple_read_write(setup_test_files: Path) -> None: invalid_data = {"protected": "foo"} with pytest.raises(ClanError) as e: store.write(invalid_data, "test", commit=False) # type: ignore[arg-type] - assert "Path 'protected' is readonly" in str(e.value) + assert "Cannot delete path 'foo'" in str(e.value) # Test the data is not touched assert store.read() == data assert store._get_persisted() == {"foo": "foo"} # Remove the foo key from the persisted data - # Technically data = { } should also work + # Cannot remove keys at the root level (root is always a total submodule) data = {"protected": "protected"} # type: ignore[typeddict-unknown-key] - store.write(data, "test", commit=False) # type: ignore[arg-type] + with pytest.raises(ClanError) as e: + store.write(data, "test", commit=False) # type: ignore[arg-type] + assert "Cannot delete path 'foo'" in str(e.value) @pytest.mark.with_core @@ -150,6 +153,13 @@ def test_simple_deferred(setup_test_files: Path) -> None: _keys={"foo"}, # disable toplevel filtering ) + attribute_props = store._write_map().writeables + assert attribute_props == { + ("foo",): {PersistenceAttribute.WRITE}, + ("foo", "a"): {PersistenceAttribute.WRITE, PersistenceAttribute.DELETE}, + ("foo", "b"): {PersistenceAttribute.WRITE, PersistenceAttribute.DELETE}, + } + data = store.read() assert data == {"foo": {"a": {}, "b": {}}} diff --git a/pkgs/clan-cli/clan_lib/persist/patch_engine.py b/pkgs/clan-cli/clan_lib/persist/patch_engine.py index 9b674fb80..95bed408d 100644 --- a/pkgs/clan-cli/clan_lib/persist/patch_engine.py +++ b/pkgs/clan-cli/clan_lib/persist/patch_engine.py @@ -11,6 +11,7 @@ from clan_lib.persist.path_utils import ( ) from clan_lib.persist.validate import ( validate_list_uniqueness, + validate_no_static_deletion, validate_patch_conflicts, validate_type_compatibility, validate_writeability, @@ -140,6 +141,7 @@ Please report this issue at https://git.clan.lol/clan/clan-core/issues static_items = list_difference( old_value or [], persisted_flat.get(path, []) ) + validate_no_static_deletion(path, new_value, static_items) patch_value = list_difference(new_value, static_items) patches[path] = patch_value diff --git a/pkgs/clan-cli/clan_lib/persist/patch_engine_test.py b/pkgs/clan-cli/clan_lib/persist/patch_engine_test.py index 12676c628..f3b711b04 100644 --- a/pkgs/clan-cli/clan_lib/persist/patch_engine_test.py +++ b/pkgs/clan-cli/clan_lib/persist/patch_engine_test.py @@ -311,6 +311,37 @@ def test_update_list() -> None: assert patchset == {("foo",): []} +def test_list_delete_static() -> None: + prios = { + "foo": { + "__prio": 100, # <- writeable: "foo" + }, + } + + data_eval = { + # [ "A" ] is defined in nix. + "foo": ["A", "B"], + } + + data_disk = {"foo": ["B"]} + + attribute_props = compute_attribute_persistence(prios, data_eval, data_disk) + + assert attribute_props == { + ("foo",): {PersistenceAttribute.WRITE}, + } + + # Try to remove "A" from the list + update = {"foo": ["B"]} + + with pytest.raises(ClanError) as error: + calc_patches( + data_disk, update, all_values=data_eval, attribute_props=attribute_props + ) + + assert "Path 'foo' doesn't contain static items ['A']" in str(error.value) + + def test_update_list_duplicates() -> None: prios = { "foo": { diff --git a/pkgs/clan-cli/clan_lib/persist/validate.py b/pkgs/clan-cli/clan_lib/persist/validate.py index 6f69489c3..d00c9f8bb 100644 --- a/pkgs/clan-cli/clan_lib/persist/validate.py +++ b/pkgs/clan-cli/clan_lib/persist/validate.py @@ -10,6 +10,16 @@ from clan_lib.persist.path_utils import ( from clan_lib.persist.write_rules import AttributeMap, is_writeable_path +def validate_no_static_deletion( + path: PathTuple, new_list: list[Any], static_items: list[Any] +) -> None: + """Validate that we're not trying to delete static items from a list.""" + missing_static = [item for item in static_items if item not in new_list] + if missing_static: + msg = f"Path '{path_to_string(path)}' doesn't contain static items {missing_static} - They are readonly - since they are defined via a .nix file" + raise ClanError(msg) + + def validate_writeability(path: PathTuple, writeables: AttributeMap) -> None: """Validate that a path is writeable.""" if not is_writeable_path(path, writeables): diff --git a/pkgs/clan-cli/clan_lib/persist/write_rules.py b/pkgs/clan-cli/clan_lib/persist/write_rules.py index 56412fcce..58e583c36 100644 --- a/pkgs/clan-cli/clan_lib/persist/write_rules.py +++ b/pkgs/clan-cli/clan_lib/persist/write_rules.py @@ -101,7 +101,7 @@ def is_key_writeable( return is_mergeable_type(value_in_all) or exists_in_persisted -def get_inventory_exclusive(value: dict) -> bool | None: +def get_inventory_exclusive(value: dict, inventory_file_name: str) -> bool | None: if "__this" not in value: return None @@ -110,7 +110,8 @@ def get_inventory_exclusive(value: dict) -> bool | None: return None return ( - len(definition_locations) == 1 and definition_locations[0] == "inventory.json" + len(definition_locations) == 1 + and definition_locations[0] == inventory_file_name ) @@ -129,6 +130,8 @@ def _determine_props_recursive( parent_redonly: bool = False, results: AttributeMap | None = None, parent_total: bool = True, + *, + inventory_file_name: str, ) -> AttributeMap: """Recursively determine writeability for all paths in the priority structure. @@ -158,7 +161,7 @@ def _determine_props_recursive( # Unless there is a default that applies instead, when removed. Currently we cannot test that. # So we assume exclusive values can be removed. In reality we might need to check defaults too. (TODO) # Total parents prevent deletion of immediate children. - is_inventory_exclusive = get_inventory_exclusive(value) + is_inventory_exclusive = get_inventory_exclusive(value, inventory_file_name) if not parent_total and ( is_inventory_exclusive or is_inventory_exclusive is None ): @@ -187,7 +190,8 @@ def _determine_props_recursive( effective_priority, parent_redonly=True, results=results, - parent_total=value.get("__this", {}).get("total", False), + parent_total=get_totality(value), + inventory_file_name=inventory_file_name, ) else: # Determine writeability based on rules @@ -218,13 +222,18 @@ def _determine_props_recursive( parent_redonly=False, results=results, parent_total=get_totality(value), + inventory_file_name=inventory_file_name, ) return results def compute_attribute_persistence( - priorities: dict[str, Any], all_values: dict[str, Any], persisted: dict[str, Any] + priorities: dict[str, Any], + all_values: dict[str, Any], + persisted: dict[str, Any], + *, + inventory_file_name: str = "inventory.json", ) -> AttributeMap: """Determine writeability for all paths based on priorities and current data. @@ -236,6 +245,7 @@ def compute_attribute_persistence( priorities: The priority structure defining writeability rules. See: 'clanInternals.inventoryClass.introspection' all_values: All values in the inventory, See: 'clanInternals.inventoryClass.allValues' persisted: The current mutable state of the inventory, see: 'readFile inventory.json' + inventory_file_name: The name of the inventory file, defaults to "inventory.json" Returns: Dict with sets of writeable and non-writeable paths using tuple keys @@ -251,4 +261,6 @@ def compute_attribute_persistence( ) raise ClanError(msg) - return _determine_props_recursive(priorities, all_values, persisted) + return _determine_props_recursive( + priorities, all_values, persisted, inventory_file_name=inventory_file_name + ) diff --git a/pkgs/clan-cli/clan_lib/persist/write_rules_test.py b/pkgs/clan-cli/clan_lib/persist/write_rules_test.py index 5af3042c9..7cbb01e30 100644 --- a/pkgs/clan-cli/clan_lib/persist/write_rules_test.py +++ b/pkgs/clan-cli/clan_lib/persist/write_rules_test.py @@ -252,3 +252,45 @@ def test_attributes_totality() -> None: ("foo",): {PersistenceAttribute.WRITE}, ("foo", "a"): {PersistenceAttribute.WRITE}, } + + +def test_totality_simple() -> None: + introspection = { + "foo": { # Foo is non-total all keys are potententially deleteable + "__this": { + "files": [""], + "headType": "attrsOf", + "nullable": False, + "prio": 100, + "total": False, + }, + "a": { # Can delete "a" because it is defined somewhere else + "__this": { + "files": ["inventory.json", ""], + "headType": "deferredModule", + "nullable": False, + "prio": 100, + "total": False, + } + }, + "b": { # Can remove "b" because it is only defined in inventory.json + "__this": { + "files": ["inventory.json"], + "headType": "deferredModule", + "nullable": False, + "prio": 100, + "total": False, + } + }, + } + } + data_eval: dict = {"foo": {"a": {}, "b": {}}} + persisted: dict = {"foo": {"a": {}, "b": {}}} + + res = compute_attribute_persistence(introspection, data_eval, persisted) + + assert res == { + ("foo",): {PersistenceAttribute.WRITE}, + ("foo", "a"): {PersistenceAttribute.WRITE}, + ("foo", "b"): {PersistenceAttribute.WRITE, PersistenceAttribute.DELETE}, + }