yadunut/clan-core
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Merge pull request 'api: check deletions if possible' (#5538) from fix-deletions into main

Browse Source 
Reviewed-on: https://git.clan.lol/clan/clan-core/pulls/5538
This commit is contained in:
hsjobeki
2025-10-16 10:50:15 +00:00
parent bb4b43f5be d78dca47e2
commit 3d436b3c6b
9 changed files with 218 additions and 289 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
pkgs/clan-cli/clan_lib/persist/fixtures/deferred.nix
View File

@@ -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

1
pkgs/clan-cli/clan_lib/persist/inventory_store.py
View File

@@ -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)

16
pkgs/clan-cli/clan_lib/persist/inventory_store_test.py
View File

@@ -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": {}}}

66
pkgs/clan-cli/clan_lib/persist/patch_engine.py
View File

@@ -6,6 +6,7 @@ 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 (
@@ -15,7 +16,7 @@ from clan_lib.persist.validate import (
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 +54,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,27 +86,27 @@ 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)
# 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)
# 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 +131,6 @@ def calc_patches(
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)
@@ -162,7 +138,11 @@ 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, [])
)
validate_no_static_deletion(path, new_value, static_items)
patch_value = list_difference(new_value, static_items)
patches[path] = patch_value

326
pkgs/clan-cli/clan_lib/persist/patch_engine_test.py
View File

@@ -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:
@@ -456,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": {
@@ -526,27 +412,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 +648,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 ---

19
pkgs/clan-cli/clan_lib/persist/validate.py
View File

@@ -11,23 +11,14 @@ 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]
path: PathTuple, new_list: list[Any], static_items: list[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"
"""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)
# 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."""

27
pkgs/clan-cli/clan_lib/persist/write_rules.py
View File

@@ -101,16 +101,16 @@ 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
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.json"
return len(definition_locations) == 1 and definition_locations[0].endswith(
inventory_file_name
)
@@ -129,6 +129,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 +160,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 +189,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 +221,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 +244,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 +260,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
)

42
pkgs/clan-cli/clan_lib/persist/write_rules_test.py
View File

@@ -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": ["<unknown-file>"],
"headType": "attrsOf",
"nullable": False,
"prio": 100,
"total": False,
},
"a": { # Can delete "a" because it is defined somewhere else
"__this": {
"files": ["inventory.json", "<unknown-file>"],
"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},
}

4
pkgs/clan-cli/clan_lib/services/modules_test.py
View File

@@ -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",