From 16d245b179cc58c65e7cf5ac4aa206ad75403e38 Mon Sep 17 00:00:00 2001 From: Johannes Kirschbauer Date: Wed, 15 Oct 2025 16:15:54 +0200 Subject: [PATCH 1/5] api: check deletions if possible --- .../clan-cli/clan_lib/persist/patch_engine.py | 64 ++-- .../clan_lib/persist/patch_engine_test.py | 295 +++++------------- 2 files changed, 99 insertions(+), 260 deletions(-) diff --git a/pkgs/clan-cli/clan_lib/persist/patch_engine.py b/pkgs/clan-cli/clan_lib/persist/patch_engine.py index 182298c1e..8ea13946c 100644 --- a/pkgs/clan-cli/clan_lib/persist/patch_engine.py +++ b/pkgs/clan-cli/clan_lib/persist/patch_engine.py @@ -6,16 +6,16 @@ from clan_lib.persist.path_utils import ( PathTuple, flatten_data_structured, list_difference, + path_to_string, should_skip_path, ) from clan_lib.persist.validate import ( validate_list_uniqueness, - validate_no_static_deletion, validate_patch_conflicts, validate_type_compatibility, validate_writeability, ) -from clan_lib.persist.write_rules import AttributeMap +from clan_lib.persist.write_rules import AttributeMap, PersistenceAttribute def find_deleted_paths_structured( @@ -53,37 +53,13 @@ def find_deleted_paths_structured( # deleted_paths.add(current_path) # Persisted was a dict, update is not a dict - # This can happen if user sets the value to None, explicitly. # produce 'set path None' is not a deletion + # This can happen if user sets the value to None, explicitly. + # produce 'set path None' is not a deletion pass return deleted_paths -def calculate_static_data( - all_values: dict[str, Any], persisted: dict[str, Any] -) -> dict[PathTuple, Any]: - """Calculate the static (read-only) data by finding what exists in all_values - but not in persisted data. - - This gives us a clear view of what cannot be modified/deleted. - """ - all_flat = flatten_data_structured(all_values) - persisted_flat = flatten_data_structured(persisted) - static_flat = {} - - for key, value in all_flat.items(): - if key not in persisted_flat: - # This key exists only in static data - static_flat[key] = value - elif isinstance(value, list) and isinstance(persisted_flat[key], list): - # For lists, find items that are only in all_values (static items) - static_items = list_difference(value, persisted_flat[key]) - if static_items: - static_flat[key] = static_items - - return static_flat - - def calc_patches( persisted: dict[str, Any], update: dict[str, Any], @@ -109,11 +85,8 @@ def calc_patches( ClanError: When validation fails or invalid operations are attempted """ - # Calculate static data using structured paths - static_data = calculate_static_data(all_values, persisted) - # Flatten all data structures using structured paths - # persisted_flat = flatten_data_structured(persisted) + persisted_flat = flatten_data_structured(persisted) update_flat = flatten_data_structured(update) all_values_flat = flatten_data_structured(all_values) @@ -123,13 +96,19 @@ def calc_patches( # Find paths marked for deletion delete_paths = find_deleted_paths_structured(all_values, update) - # Validate deletions don't affect static data - # TODO: We currently cannot validate this properly. - # for delete_path in delete_paths: - # for static_path in static_data: - # if path_starts_with(static_path, delete_path): - # msg = f"Cannot delete path '{path_to_string(delete_path)}' - Readonly path '{path_to_string(static_path)}' is set via .nix file" - # raise ClanError(msg) + # Validate deletions. Ensure deletions are allowed + # Prior introspection adds PersistenceAttribute.DELETE to paths that can be deleted + for delete_path in delete_paths: + if delete_path not in attribute_props: + msg = f"""Cannot delete path '{path_to_string(delete_path)}' - '{path_to_string(delete_path)}' is not a valid path. +This is an internal error that should not happen. +Please report this issue at https://git.clan.lol/clan/clan-core/issues + """ + raise ClanError(msg) + + if PersistenceAttribute.DELETE not in attribute_props.get(delete_path, {}): + msg = f"Cannot delete path '{path_to_string(delete_path)}' - '{path_to_string(delete_path)}' is set via .nix file" + raise ClanError(msg) # Get all paths that might need processing all_paths: set[PathTuple] = set(all_values_flat) | set(update_flat) @@ -154,7 +133,7 @@ def calc_patches( continue # Validate the change is allowed - validate_no_static_deletion(path, new_value, static_data) + # validate_no_static_deletion(path, new_value, static_data) validate_writeability(path, attribute_props) validate_type_compatibility(path, old_value, new_value) validate_list_uniqueness(path, new_value) @@ -162,7 +141,10 @@ def calc_patches( patch_value = new_value # Init if isinstance(new_value, list): - patch_value = list_difference(new_value, static_data.get(path, [])) + static_items = list_difference( + old_value or [], persisted_flat.get(path, []) + ) + 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 dba8bf738..12676c628 100644 --- a/pkgs/clan-cli/clan_lib/persist/patch_engine_test.py +++ b/pkgs/clan-cli/clan_lib/persist/patch_engine_test.py @@ -6,7 +6,6 @@ import pytest from clan_lib.errors import ClanError from clan_lib.persist.patch_engine import ( calc_patches, - calculate_static_data, merge_objects, ) from clan_lib.persist.path_utils import ( @@ -19,195 +18,6 @@ from clan_lib.persist.write_rules import ( compute_attribute_persistence, ) -# --- calculate_static_data --- - - -def test_calculate_static_data_basic() -> None: - all_values = { - "name": "example", - "version": 1, - "settings": { - "optionA": True, - "optionB": False, - "listSetting": [1, 2, 3, 4], - }, - "staticOnly": "staticValue", - } - persisted = { - "name": "example", - "version": 1, - "settings": { - "optionA": True, - "listSetting": [2, 3], - }, - } - - expected_static = { - ("settings", "optionB"): False, - ("settings", "listSetting"): [1, 4], - ("staticOnly",): "staticValue", - } - - static_data = calculate_static_data(all_values, persisted) - assert static_data == expected_static - - -def test_calculate_static_data_no_static() -> None: - all_values = { - "name": "example", - "version": 1, - "settings": { - "optionA": True, - "listSetting": [1, 2, 3], - }, - } - persisted = { - "name": "example", - "version": 1, - "settings": { - "optionA": True, - "listSetting": [1, 2, 3], - }, - } - - expected_static: dict = {} - - static_data = calculate_static_data(all_values, persisted) - assert static_data == expected_static - - -# See: https://git.clan.lol/clan/clan-core/issues/5231 -# Uncomment this test if the issue is resolved -# def test_calculate_static_data_no_static_sets() -> None: -# all_values = { -# "instance": { -# "hello": { -# "roles": { -# "default": { -# "machines": { -# "jon": { -# # Default -# "settings": { - -# } -# } -# } -# } -# } -# } -# } -# } -# persisted = { -# "instance": { -# "hello": { -# "roles": { -# "default": { -# "machines": { -# "jon": { - -# } -# } -# } -# } -# } -# } -# } - -# expected_static: dict = {} - -# static_data = calculate_static_data(all_values, persisted) -# assert static_data == expected_static - - -def test_calculate_static_data_all_static() -> None: - all_values = { - "name": "example", - "version": 1, - "settings": { - "optionA": True, - "listSetting": [1, 2, 3], - }, - "staticOnly": "staticValue", - } - persisted: dict = {} - - expected_static = { - ("name",): "example", - ("version",): 1, - ("settings", "optionA"): True, - ("settings", "listSetting"): [1, 2, 3], - ("staticOnly",): "staticValue", - } - - static_data = calculate_static_data(all_values, persisted) - assert static_data == expected_static - - -def test_calculate_static_data_empty_all_values() -> None: - # This should never happen in practice, but we test it for completeness. - # Maybe this should emit a warning in the future? - all_values: dict = {} - persisted = { - "name": "example", - "version": 1, - } - - expected_static: dict = {} - - static_data = calculate_static_data(all_values, persisted) - assert static_data == expected_static - - -def test_calculate_nested_dicts() -> None: - all_values = { - "level1": { - "level2": { - "staticKey": "staticValue", - "persistedKey": "persistedValue", - }, - "anotherStatic": 42, - }, - "topLevelStatic": True, - } - persisted = { - "level1": { - "level2": { - "persistedKey": "persistedValue", - }, - }, - } - - expected_static = { - ("level1", "level2", "staticKey"): "staticValue", - ("level1", "anotherStatic"): 42, - ("topLevelStatic",): True, - } - - static_data = calculate_static_data(all_values, persisted) - assert static_data == expected_static - - -def test_dot_in_keys() -> None: - all_values = { - "key.foo": "staticValue", - "key": { - "foo": "anotherStaticValue", - }, - } - persisted: dict = {} - - expected_static = { - ("key.foo",): "staticValue", - ("key", "foo"): "anotherStaticValue", - } - - static_data = calculate_static_data(all_values, persisted) - - assert static_data == expected_static - - -# --------- calc_patches --------- - def test_update_simple() -> None: prios = { @@ -379,34 +189,79 @@ def test_update_parent_non_writeable() -> None: assert "Path 'foo.bar' is readonly." in str(error.value) -# TODO: Resolve the issue https://git.clan.lol/clan/clan-core/issues/5231 -# def test_remove_non_writable_attrs() -> None: -# prios = { -# "foo": { -# "__this": {"total": True, "prio": 100}, -# # We cannot delete children because "foo" is total -# }, -# } +def test_calculate_static_data_no_static_sets() -> None: + all_values: dict = { + "instance": { + "hello": { + "static": {}, + } + } + } + persisted: dict = {"instance": {"hello": {}}} + attribute_props = { + ("instance",): {PersistenceAttribute.WRITE}, + ("instance", "hello"): { + PersistenceAttribute.WRITE, + PersistenceAttribute.DELETE, + }, + ("instance", "hello", "static"): {PersistenceAttribute.WRITE}, + } -# data_eval: dict = {"foo": {"bar": {}, "baz": {}}} + update: dict = { + "instance": { + # static is a key that is defined in nix, and cannot be deleted + # It needs to be present. + "hello": {} # Error: "static" cannot be deleted + } + } -# data_disk: dict = {} + with pytest.raises(ClanError) as error: + calc_patches( + persisted=persisted, + all_values=all_values, + update=update, + attribute_props=attribute_props, + ) -# attribute_props = compute_attribute_persistence(prios, data_eval, data_disk) + assert "Cannot delete path 'instance.hello.static'" in str(error.value) -# update: dict = { -# "foo": { -# "bar": {}, # <- user leaves this value -# # User removed "baz" -# }, -# } -# with pytest.raises(ClanError) as error: -# calc_patches( -# data_disk, update, all_values=data_eval, attribute_props=attribute_props -# ) +# Same as above, but using legacy priorities +# This allows deletion, but is slightly wrong. +# The correct behavior requires nixos >= 25.11 +def test_calculate_static_data_no_static_sets_legacy() -> None: + prios = { + "instance": { + "__prio": 100, # <- writeable: "foo" + "hello": { + "__prio": 100, + "static": {"__prio": 100}, + }, + }, + } -# assert "Cannot delete path 'foo.baz'" in str(error.value) + all_values: dict = { + "instance": { + "hello": { + "static": {}, + } + } + } + persisted: dict = {"instance": {"hello": {}}} + + attribute_props = compute_attribute_persistence(prios, all_values, persisted) + + update: dict = { + "instance": { + "hello": {}, # <- user removes "static" + }, + } + + updates, deletes = calc_patches( + persisted, update, all_values=all_values, attribute_props=attribute_props + ) + assert updates == {("instance", "hello"): {}} + assert deletes == {("instance", "hello", "static")} def test_update_list() -> None: @@ -526,27 +381,29 @@ def test_dont_persist_defaults() -> None: def test_set_null() -> None: data_eval: dict = { - "foo": {}, - "bar": {}, + "root": { + "foo": {}, + "bar": {}, + } } data_disk = data_eval # User set Foo to null # User deleted bar - update = {"foo": None} + update = {"root": {"foo": None}} patchset, delete_set = calc_patches( data_disk, update, all_values=data_eval, attribute_props=compute_attribute_persistence( - {"__prio": 100, "foo": {"__prio": 100}, "bar": {"__prio": 100}}, + {"root": {"__prio": 100, "foo": {"__prio": 100}, "bar": {"__prio": 100}}}, data_eval, data_disk, ), ) - assert patchset == {("foo",): None} - assert delete_set == {("bar",)} + assert patchset == {("root", "foo"): None} + assert delete_set == {("root", "bar")} def test_machine_delete() -> None: @@ -760,7 +617,7 @@ def test_delete_key_non_writeable() -> None: data_disk, update, all_values=data_eval, attribute_props=attribute_props ) - assert "Path 'foo' is readonly." in str(error.value) + assert "Cannot delete path 'foo.bar'" in str(error.value) # --- test merge_objects --- From a32a5151dc2efc2dd02819b0702675b2072d2f05 Mon Sep 17 00:00:00 2001 From: Johannes Kirschbauer Date: Wed, 15 Oct 2025 16:19:21 +0200 Subject: [PATCH 2/5] api: remove unused validation method --- .../clan-cli/clan_lib/persist/patch_engine.py | 4 ---- pkgs/clan-cli/clan_lib/persist/validate.py | 19 ------------------- 2 files changed, 23 deletions(-) diff --git a/pkgs/clan-cli/clan_lib/persist/patch_engine.py b/pkgs/clan-cli/clan_lib/persist/patch_engine.py index 8ea13946c..9b674fb80 100644 --- a/pkgs/clan-cli/clan_lib/persist/patch_engine.py +++ b/pkgs/clan-cli/clan_lib/persist/patch_engine.py @@ -90,9 +90,6 @@ def calc_patches( update_flat = flatten_data_structured(update) all_values_flat = flatten_data_structured(all_values) - # Early validation: ensure we're not trying to modify static-only paths - # validate_no_static_modification(update_flat, static_data) - # Find paths marked for deletion delete_paths = find_deleted_paths_structured(all_values, update) @@ -133,7 +130,6 @@ Please report this issue at https://git.clan.lol/clan/clan-core/issues continue # Validate the change is allowed - # validate_no_static_deletion(path, new_value, static_data) validate_writeability(path, attribute_props) validate_type_compatibility(path, old_value, new_value) validate_list_uniqueness(path, new_value) diff --git a/pkgs/clan-cli/clan_lib/persist/validate.py b/pkgs/clan-cli/clan_lib/persist/validate.py index 7423bb68c..6f69489c3 100644 --- a/pkgs/clan-cli/clan_lib/persist/validate.py +++ b/pkgs/clan-cli/clan_lib/persist/validate.py @@ -10,25 +10,6 @@ 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_value: Any, static_data: dict[PathTuple, Any] -) -> None: - """Validate that we're not trying to delete static data.""" - # Check if we're trying to delete a path that exists in static data - if path in static_data and new_value is None: - msg = f"Path '{path_to_string(path)}' is readonly - since its defined via a .nix file" - raise ClanError(msg) - - # For lists, check if we're trying to remove static items - if isinstance(new_value, list) and path in static_data: - static_items = static_data[path] - if isinstance(static_items, list): - missing_static = [item for item in static_items if item not in new_value] - 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): From a4839f9cf2264e56c347a7b6c6289ec8badedeaf Mon Sep 17 00:00:00 2001 From: Johannes Kirschbauer Date: Thu, 16 Oct 2025 11:57:38 +0200 Subject: [PATCH 3/5] api: persistence, fix regressions --- .../clan_lib/persist/fixtures/deferred.nix | 6 ++- .../clan_lib/persist/inventory_store.py | 1 + .../clan_lib/persist/inventory_store_test.py | 16 +++++-- .../clan-cli/clan_lib/persist/patch_engine.py | 2 + .../clan_lib/persist/patch_engine_test.py | 31 ++++++++++++++ pkgs/clan-cli/clan_lib/persist/validate.py | 10 +++++ pkgs/clan-cli/clan_lib/persist/write_rules.py | 24 ++++++++--- .../clan_lib/persist/write_rules_test.py | 42 +++++++++++++++++++ 8 files changed, 122 insertions(+), 10 deletions(-) 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}, + } From 8254d197f0843a25df2492cd30d71ce740511560 Mon Sep 17 00:00:00 2001 From: Johannes Kirschbauer Date: Thu, 16 Oct 2025 12:41:54 +0200 Subject: [PATCH 4/5] api: persistence allow path prefix --- pkgs/clan-cli/clan_lib/persist/write_rules.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkgs/clan-cli/clan_lib/persist/write_rules.py b/pkgs/clan-cli/clan_lib/persist/write_rules.py index 58e583c36..9918fe9ea 100644 --- a/pkgs/clan-cli/clan_lib/persist/write_rules.py +++ b/pkgs/clan-cli/clan_lib/persist/write_rules.py @@ -105,13 +105,12 @@ def get_inventory_exclusive(value: dict, inventory_file_name: str) -> bool | Non if "__this" not in value: return None - definition_locations = value.get("__this", {}).get("files") + definition_locations: list[str] = value.get("__this", {}).get("files") if not definition_locations: return None - return ( - len(definition_locations) == 1 - and definition_locations[0] == inventory_file_name + return len(definition_locations) == 1 and definition_locations[0].endswith( + inventory_file_name ) From d78dca47e2978b2566048f98142930b37674ba21 Mon Sep 17 00:00:00 2001 From: Johannes Kirschbauer Date: Thu, 16 Oct 2025 12:42:08 +0200 Subject: [PATCH 5/5] modules: update service test --- pkgs/clan-cli/clan_lib/services/modules_test.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkgs/clan-cli/clan_lib/services/modules_test.py b/pkgs/clan-cli/clan_lib/services/modules_test.py index ea0511a29..ff530650c 100644 --- a/pkgs/clan-cli/clan_lib/services/modules_test.py +++ b/pkgs/clan-cli/clan_lib/services/modules_test.py @@ -203,7 +203,9 @@ def test_update_service_instance( { "morning": { "machines": { - "jon": {}, # Unset the machine settings + "jon": { + "settings": {} # type: ignore[typeddict-item] + }, "sara": { "settings": { # type: ignore[typeddict-item] "greeting": "sara",