ADCM-6277 Change diff calculation algorithm (#75)

Co-authored-by: Araslanov Egor <[email protected]>

adcm_aio_client/config/_objects.py

@@ -301,7 +301,8 @@ def difference(self: Self, other: Self, *, other_is_previous: bool = True) -> Co
             current = other
 
         full_diff = find_config_difference(previous=previous.data, current=current.data, schema=self._schema)
-        return ConfigDifference.from_full_format(full_diff)
+        only_visible_fields = {k: v for k, v in full_diff.items() if self._schema.is_visible_parameter(k)}
+        return ConfigDifference(diff=only_visible_fields)
 
     # Public For Internal Use Only
 

adcm_aio_client/config/_operations.py

@@ -1,56 +1,35 @@
-from adcm_aio_client.config._types import (
-    ConfigSchema,
-    FullConfigDifference,
-    GenericConfigData,
-    LevelNames,
-    ValueChange,
-    full_name_to_level_names,
-)
+from contextlib import suppress
+
+from adcm_aio_client.config._types import ConfigSchema, GenericConfigData, LevelNames, ParameterChange
 
 
 # Difference
 def find_config_difference(
     previous: GenericConfigData, current: GenericConfigData, schema: ConfigSchema
-) -> FullConfigDifference:
-    diff = FullConfigDifference(schema=schema)
-
-    _fill_values_diff_at_level(level=(), diff=diff, previous=previous.values, current=current.values)
-    _fill_attributes_diff(diff=diff, previous=previous.attributes, current=current.attributes)
-
-    return diff
-
+) -> dict[LevelNames, ParameterChange]:
+    diff = {}
 
-def _fill_values_diff_at_level(level: LevelNames, diff: FullConfigDifference, previous: dict, current: dict) -> None:
-    missing = object()
-    for key, cur_value in current.items():
-        level_names = (*level, key)
-        prev_value = previous.get(key, missing)
+    for names, _ in schema.iterate_parameters():
+        prev = {"value": None, "attrs": {}}
+        cur = {"value": None, "attrs": {}}
 
-        if prev_value is missing:
-            # there may be collision between two None's, but for now we'll consider it a "special case"
-            diff.values[level_names] = ValueChange(previous=None, current=cur_value)
-            continue
+        # TypeError / KeyError may occur when `None` is in values
+        # (e.g. structure with dict as root item and with None value)
+        if not schema.is_group(names):
+            with suppress(TypeError, KeyError):
+                prev["value"] = previous.get_value(names)
 
-        if cur_value == prev_value:
-            continue
+            with suppress(TypeError, KeyError):
+                cur["value"] = current.get_value(names)
+        else:
+            prev.pop("value")
+            cur.pop("value")
 
-        if not (diff.schema.is_group(level_names) and isinstance(prev_value, dict) and (isinstance(cur_value, dict))):
-            diff.values[level_names] = ValueChange(previous=prev_value, current=cur_value)
-            continue
+        attr_key = f"/{'/'.join(names)}"
+        prev["attrs"] = previous.attributes.get(attr_key, {})
+        cur["attrs"] = current.attributes.get(attr_key, {})
 
-        _fill_values_diff_at_level(diff=diff, level=level_names, previous=prev_value, current=cur_value)
+        if prev != cur:
+            diff[names] = ParameterChange(previous=prev, current=cur)
 
-
-def _fill_attributes_diff(diff: FullConfigDifference, previous: dict, current: dict) -> None:
-    missing = object()
-    for full_name, cur_value in current.items():
-        prev_value = previous.get(full_name, missing)
-        if cur_value == prev_value:
-            continue
-
-        level_names = full_name_to_level_names(full_name)
-
-        if prev_value is missing:
-            prev_value = None
-
-        diff.attributes[level_names] = ValueChange(previous=prev_value, current=cur_value)
+    return diff

adcm_aio_client/config/_refresh.py

@@ -1,26 +1,17 @@
 from adcm_aio_client.config._operations import find_config_difference
-from adcm_aio_client.config._types import ConfigData, ConfigSchema, LocalConfigs
+from adcm_aio_client.config._types import ConfigData, ConfigSchema, LevelNames, LocalConfigs, ParameterChange
 
 
 def apply_local_changes(local: LocalConfigs, remote: ConfigData, schema: ConfigSchema) -> ConfigData:
     if local.initial.id == remote.id:
         return local.changed
 
     local_diff = find_config_difference(previous=local.initial, current=local.changed, schema=schema)
-    if local_diff.is_empty:
+    if not local_diff:
         # no changed, nothing to apply
         return remote
 
-    for parameter_name, value_change in local_diff.values.items():
-        remote.set_value(parameter=parameter_name, value=value_change.current)
-
-    for parameter_name, attribute_change in local_diff.attributes.items():
-        if not isinstance(attribute_change.current, dict):
-            message = f"Can't apply attribute changes of type {type(attribute_change)}, expected dict-like"
-            raise TypeError(message)
-
-        for attribute_name, value in attribute_change.current.items():
-            remote.set_attribute(parameter=parameter_name, attribute=attribute_name, value=value)
+    _apply(data=remote, changes=local_diff)
 
     return remote
 
@@ -30,41 +21,24 @@ def apply_remote_changes(local: LocalConfigs, remote: ConfigData, schema: Config
         return local.changed
 
     local_diff = find_config_difference(previous=local.initial, current=local.changed, schema=schema)
-    if local_diff.is_empty:
+    if not local_diff:
         return remote
 
     remote_diff = find_config_difference(previous=local.initial, current=remote, schema=schema)
 
-    locally_changed = set(local_diff.values.keys())
-    changed_in_both = locally_changed.intersection(remote_diff.values.keys())
-    changed_locally_only = locally_changed.difference(remote_diff.values.keys())
-
-    for parameter_name in changed_in_both:
-        remote.set_value(parameter=parameter_name, value=remote_diff.values[parameter_name].current)
-
-    for parameter_name in changed_locally_only:
-        remote.set_value(parameter=parameter_name, value=local_diff.values[parameter_name].current)
+    changed_in_remote = set(remote_diff.keys())
+    only_local_changes = {k: v for k, v in local_diff.items() if k not in changed_in_remote}
 
-    locally_changed = set(local_diff.attributes.keys())
-    changed_in_both = locally_changed.intersection(remote_diff.attributes.keys())
-    changed_locally_only = locally_changed.difference(remote_diff.attributes.keys())
+    _apply(data=remote, changes=only_local_changes)
 
-    for parameter_name in changed_in_both:
-        attribute_change = remote_diff.attributes[parameter_name]
-        if not isinstance(attribute_change.current, dict):
-            message = f"Can't apply attribute changes of type {type(attribute_change)}, expected dict-like"
-            raise TypeError(message)
-
-        for attribute_name, value in attribute_change.current.items():
-            remote.set_attribute(parameter=parameter_name, attribute=attribute_name, value=value)
+    return remote
 
-    for parameter_name in changed_locally_only:
-        attribute_change = local_diff.attributes[parameter_name]
-        if not isinstance(attribute_change.current, dict):
-            message = f"Can't apply attribute changes of type {type(attribute_change)}, expected dict-like"
-            raise TypeError(message)
 
-        for attribute_name, value in attribute_change.current.items():
-            remote.set_attribute(parameter=parameter_name, attribute=attribute_name, value=value)
+def _apply(data: ConfigData, changes: dict[LevelNames, ParameterChange]) -> None:
+    for parameter_name, change in changes.items():
+        if "value" in change.current:
+            cur_value = change.current["value"]
+            data.set_value(parameter=parameter_name, value=cur_value)
 
-    return remote
+        for name, value in change.current.get("attrs", {}).items():
+            data.set_attribute(parameter=parameter_name, attribute=name, value=value)

adcm_aio_client/config/_types.py

@@ -1,7 +1,7 @@
 from abc import ABC
 from collections import defaultdict
 from collections.abc import Callable, Iterable
-from dataclasses import dataclass, field
+from dataclasses import dataclass
 from functools import reduce
 from typing import Any, NamedTuple, Protocol, Self
 
@@ -125,62 +125,29 @@ def from_v2_response(cls: type[Self], data_in_v2_format: dict) -> Self:
 
 
 @dataclass(slots=True)
-class ValueChange:
-    previous: Any
-    current: Any
+class ParameterChange:
+    previous: dict
+    current: dict
 
 
 def recursive_defaultdict() -> defaultdict:
     return defaultdict(recursive_defaultdict)
 
 
-@dataclass(slots=True)
-class FullConfigDifference:
-    schema: "ConfigSchema"
-    values: dict[LevelNames, ValueChange] = field(default_factory=dict)
-    attributes: dict[LevelNames, ValueChange] = field(default_factory=dict)
-
-    @property
-    def is_empty(self: Self) -> bool:
-        return not bool(self.values or self.attributes)
-
-
 class ConfigDifference:
-    __slots__ = ("_schema", "_values", "_attributes")
-
-    def __init__(
-        self: Self,
-        schema: "ConfigSchema",
-        values: dict[LevelNames, ValueChange],
-        attributes: dict[LevelNames, ValueChange],
-    ) -> None:
-        self._schema = schema
-        self._values = values
-        self._attributes = attributes
+    __slots__ = ("_diff",)
 
-    @classmethod
-    def from_full_format(cls: type[Self], diff: FullConfigDifference) -> Self:
-        visible_value_changes = {k: v for k, v in diff.values.items() if not diff.schema.is_invisible(k)}
-        visible_attr_changes = {k: v for k, v in diff.attributes.items() if not diff.schema.is_invisible(k)}
-        return cls(schema=diff.schema, values=visible_value_changes, attributes=visible_attr_changes)
+    def __init__(self: Self, diff: dict[LevelNames, ParameterChange]) -> None:
+        self._diff = diff
 
     def __str__(self: Self) -> str:
-        values_nested = self._to_nested_dict(self._values)
-        attributes_nested = self._to_nested_dict(self._attributes)
-
-        if not (values_nested or attributes_nested):
+        if not self._diff:
             return "No Changes"
 
-        values_repr = f"Changed Values:\n{values_nested}" if values_nested else ""
-        attributes_repr = f"Changed Attributes:\n{attributes_nested}" if attributes_nested else ""
-
-        return "\n\n".join((values_repr, attributes_repr))
-
-    def _to_nested_dict(self: Self, changes: dict[LevelNames, ValueChange]) -> dict:
         result = recursive_defaultdict()
 
-        for names, change in changes.items():
-            changes_repr = self._prepare_change(change)
+        for names, change in self._diff.items():
+            changes_repr = self._prepare_change(previous=change.previous, current=change.current)
 
             if len(names) == 1:
                 result[names[0]] = changes_repr
@@ -192,22 +159,24 @@ def _to_nested_dict(self: Self, changes: dict[LevelNames, ValueChange]) -> dict:
 
         # get rid of `defaultdict` in favor of `dict`
         # may be not optimal
-        return self._simplify_dict(result)
+        print_ready_dict = self._simplify_dict(result)
+
+        return str(print_ready_dict)
 
-    def _prepare_change(self: Self, change: ValueChange) -> tuple | dict:
-        if not (isinstance(change.previous, dict) and isinstance(change.current, dict)):
-            return (change.previous, change.current)
+    def _prepare_change(self: Self, previous: Any, current: Any) -> tuple | dict:  # noqa: ANN401
+        if not (isinstance(previous, dict) and isinstance(current, dict)):
+            return (previous, current)
 
         dict_diff = {}
 
-        for key, cur_value in change.current.items():
-            prev_value = change.previous.get(key)
+        for key, cur_value in current.items():
+            prev_value = previous.get(key)
             if prev_value != cur_value:
-                dict_diff[key] = self._prepare_change(change=ValueChange(previous=prev_value, current=cur_value))
+                dict_diff[key] = self._prepare_change(previous=prev_value, current=cur_value)
 
-        missing_in_current = set(change.previous.keys()).difference(change.current.keys())
+        missing_in_current = set(previous.keys()).difference(current.keys())
         for key in missing_in_current:
-            dict_diff[key] = self._prepare_change(change=ValueChange(previous=change.previous[key], current=None))
+            dict_diff[key] = self._prepare_change(previous=previous[key], current=None)
 
         return dict_diff
 
@@ -272,6 +241,20 @@ def get_default(self: Self, parameter_name: LevelNames) -> Any:  # noqa: ANN401
 
         return {child_name: self.get_default((*parameter_name, child_name)) for child_name in param_spec["properties"]}
 
+    def iterate_parameters(self: Self) -> Iterable[tuple[LevelNames, dict]]:
+        yield from self._iterate_parameters(object_schema=self._raw)
+
+    def _iterate_parameters(self: Self, object_schema: dict) -> Iterable[tuple[LevelNames, dict]]:
+        for level_name, optional_attrs in object_schema["properties"].items():
+            attributes = self._unwrap_optional(optional_attrs)
+
+            yield (level_name,), attributes
+
+            if is_group_v2(attributes):
+                for inner_level, inner_optional_attrs in self._iterate_parameters(attributes):
+                    inner_attributes = self._unwrap_optional(inner_optional_attrs)
+                    yield (level_name, *inner_level), inner_attributes
+
     def _analyze_schema(self: Self) -> None:
         for level_names, param_spec in self._iterate_parameters(object_schema=self._raw):
             if is_group_v2(param_spec):
@@ -297,17 +280,6 @@ def _retrieve_name_type_mapping(self: Self) -> dict[LevelNames, str]:
             for level_names, param_spec in self._iterate_parameters(object_schema=self._raw)
         }
 
-    def _iterate_parameters(self: Self, object_schema: dict) -> Iterable[tuple[LevelNames, dict]]:
-        for level_name, optional_attrs in object_schema["properties"].items():
-            attributes = self._unwrap_optional(optional_attrs)
-
-            yield (level_name,), attributes
-
-            if is_group_v2(attributes):
-                for inner_level, inner_optional_attrs in self._iterate_parameters(attributes):
-                    inner_attributes = self._unwrap_optional(inner_optional_attrs)
-                    yield (level_name, *inner_level), inner_attributes
-
     def _unwrap_optional(self: Self, attributes: dict) -> dict:
         if "oneOf" not in attributes:
             return attributes

tests/integration/test_config.py

@@ -96,8 +96,8 @@ async def test_invisible_fields(cluster: Cluster) -> None:
     second_config = await service.config_history[-1]
 
     diff = first_config.difference(second_config)
-    assert len(diff._values) == 1
-    assert ("very_important_flag",) in diff._values
+    assert len(diff._diff) == 1
+    assert ("very_important_flag",) in diff._diff
     assert first_config.data._values["cant_find"] != second_config.data._values["cant_find"]
 
 
@@ -232,11 +232,7 @@ async def test_config(cluster: Cluster) -> None:
     assert earliest_config.id == pre_save_id
 
     diff = latest_config.difference(earliest_config)
-    # group was activated, then deactivated, so returned to initial state
-    # => no diff
-    assert len(diff._attributes) == 0
-    # field values changed from earliest to latest
-    assert len(diff._values) == 6
+    assert len(diff._diff) == 6
 
 
 async def test_host_group_config(cluster: Cluster) -> None:
@@ -263,6 +259,8 @@ async def test_host_group_config(cluster: Cluster) -> None:
     assert all(v == person_default for v in values)
 
     req_val_1, req_val_2 = 12, 44
+    complexity_default = 4
+    complexity_changed = 10
     person_val_1 = {"name": "Boss", "age": "unknown"}
     person_val_2 = {"name": "Moss", "awesome": "yes"}
     strange_val_1 = {"custom": [1, 2, 3]}
@@ -273,6 +271,7 @@ async def test_host_group_config(cluster: Cluster) -> None:
     main_config["from_doc"]["person"].set(person_val_1)  # type: ignore
     main_config["more"]["strange"].set(strange_val_1)  # type: ignore
     main_config["Optional", ActivatableParameterGroup].activate()
+    main_config["complexity_level", Parameter].set(complexity_changed)
     # todo fix working with structures here
     # sag = main_config["A lot of text"]["sag"]
     # sag["quantity"].set(4)
@@ -283,10 +282,12 @@ async def test_host_group_config(cluster: Cluster) -> None:
     config_1["from_doc"]["person"].set(person_val_2)  # type: ignore
     config_1["more"]["strange"].set(strange_val_2)  # type: ignore
     config_1["Optional", ActivatableParameterGroupHG].desync()
+    config_1["complexity_level", ParameterHG].desync()
 
     config_2["from_doc"]["person"].set(person_val_2)  # type: ignore
     config_2["more"]["strange"].set(strange_val_3)  # type: ignore
     config_2["Optional", ActivatableParameterGroupHG].desync()
+    config_2["complexity_level", ParameterHG].desync()
 
     await main_config.save()
     await config_1.refresh(strategy=apply_local_changes)
@@ -302,11 +303,15 @@ async def test_host_group_config(cluster: Cluster) -> None:
     main_val, c1_val, c2_val = get_field_value("from_doc", "person", configs=configs)
     assert main_val == c2_val == person_val_1
     assert c1_val == person_val_2
+    values = get_field_value("complexity_level", configs=configs)
+    assert values == (complexity_changed, complexity_default, complexity_changed)
     # since attributes are compared as a whole, desync is considered a change
     # => priority of local change
+    assert not config_1.data.attributes["/complexity_level"]["isSynchronized"]
     assert not config_1.data.attributes["/agroup"]["isActive"]
     assert not config_1.data.attributes["/agroup"]["isSynchronized"]
     # the opposite situation when we "desynced", but changes overriten
+    assert config_2.data.attributes["/complexity_level"]["isSynchronized"]
     assert config_2.data.attributes["/agroup"]["isActive"]
     assert config_2.data.attributes["/agroup"]["isSynchronized"]