fix previous value bug for multiple updates on branch

backend/infrahub/core/diff/combiner.py

@@ -3,7 +3,7 @@
 from typing import Iterable
 from uuid import uuid4
 
-from infrahub.core.constants import DiffAction, RelationshipCardinality
+from infrahub.core.constants import NULL_VALUE, DiffAction, RelationshipCardinality
 from infrahub.core.constants.database import DatabaseEdgeType
 
 from .model.path import (
@@ -151,7 +151,10 @@ def _combine_properties(
             if earlier_property.action is DiffAction.ADDED and later_property.action is DiffAction.REMOVED:
                 continue
             combined_action = self._combine_actions(earlier=earlier_property.action, later=later_property.action)
-            if earlier_property.previous_value == later_property.new_value:
+            if earlier_property.previous_value == later_property.new_value or {
+                earlier_property.previous_value,
+                later_property.new_value,
+            } <= {None, NULL_VALUE}:
                 combined_action = DiffAction.UNCHANGED
             combined_conflict = self.combine_conflicts(earlier=earlier_property.conflict, later=later_property.conflict)
             combined_properties.add(

backend/infrahub/core/diff/conflicts_enricher.py

@@ -1,6 +1,6 @@
 from uuid import uuid4
 
-from infrahub.core.constants import DiffAction, RelationshipCardinality
+from infrahub.core.constants import NULL_VALUE, DiffAction, RelationshipCardinality
 from infrahub.core.constants.database import DatabaseEdgeType
 
 from .model.path import (
@@ -249,7 +249,7 @@ def _add_property_conflict(
     def _have_same_value(self, base_property: EnrichedDiffProperty, branch_property: EnrichedDiffProperty) -> bool:
         if base_property.new_value == branch_property.new_value:
             return True
-        if {base_property.new_value, branch_property.new_value} <= {"NULL", None}:
+        if {base_property.new_value, branch_property.new_value} <= {NULL_VALUE, None}:
             return True
         if (
             base_property.action is DiffAction.UNCHANGED

backend/infrahub/core/diff/enricher/cardinality_one.py

@@ -1,6 +1,6 @@
 from typing import TYPE_CHECKING, Any
 
-from infrahub.core.constants import DiffAction, RelationshipCardinality
+from infrahub.core.constants import NULL_VALUE, DiffAction, RelationshipCardinality
 from infrahub.core.constants.database import DatabaseEdgeType
 from infrahub.database import InfrahubDatabase
 
@@ -45,9 +45,9 @@ async def enrich(self, enriched_diff_root: EnrichedDiffRoot, calculated_diffs: C
     def _determine_action(self, previous_value: Any, new_value: Any) -> DiffAction:
         if previous_value == new_value:
             return DiffAction.UNCHANGED
-        if previous_value in (None, "NULL"):
+        if previous_value in (None, NULL_VALUE):
             return DiffAction.ADDED
-        if new_value in (None, "NULL"):
+        if new_value in (None, NULL_VALUE):
             return DiffAction.REMOVED
         return DiffAction.UPDATED
 

backend/infrahub/core/diff/merger/serializer.py

@@ -97,8 +97,8 @@ def _convert_property_value(
         ):
             return raw_value.lower() == "true"
         # this must be HAS_VALUE
-        if raw_value in (None, "NULL"):
-            return "NULL"
+        if raw_value in (None, NULL_VALUE):
+            return NULL_VALUE
         if value_type:
             if value_type is bool and isinstance(raw_value, str):
                 return raw_value.lower() == "true"

backend/infrahub/core/diff/query_parser.py

@@ -4,7 +4,13 @@
 from typing import TYPE_CHECKING, Any, Optional
 from uuid import uuid4
 
-from infrahub.core.constants import BranchSupportType, DiffAction, RelationshipCardinality, RelationshipStatus
+from infrahub.core.constants import (
+    NULL_VALUE,
+    BranchSupportType,
+    DiffAction,
+    RelationshipCardinality,
+    RelationshipStatus,
+)
 from infrahub.core.constants.database import DatabaseEdgeType
 from infrahub.core.timestamp import Timestamp
 
@@ -119,22 +125,25 @@ def get_property_details(self, from_time: Timestamp) -> tuple[DiffAction, Timest
                 action = DiffAction.REMOVED
                 previous = lone_value.value
             return (action, lone_value.changed_at, previous, new)
-        previous_value = ordered_values[0].value
-        previous_status = ordered_values[0].status
+        earliest_value = ordered_values[0]
         new_diff_value = ordered_values[-1]
         new_value = new_diff_value.value
         new_status = new_diff_value.status
         action = DiffAction.UPDATED
-        if previous_value in (None, "NULL") and new_value not in (None, "NULL"):
+        previous_value = earliest_value.value
+        if earliest_value.changed_at > from_time:
+            action = DiffAction.ADDED
+            previous_value = None
+        elif earliest_value.value in (None, NULL_VALUE) and new_value not in (None, NULL_VALUE):
             action = DiffAction.ADDED
-        elif (previous_value not in (None, "NULL") and new_value in (None, "NULL")) or (
-            previous_status,
+        elif (earliest_value.value not in (None, NULL_VALUE) and new_value in (None, NULL_VALUE)) or (
+            earliest_value.status,
             new_status,
         ) == (RelationshipStatus.ACTIVE, RelationshipStatus.DELETED):
             action = DiffAction.REMOVED
-            if new_value != "NULL":
+            if new_value != NULL_VALUE:
                 new_value = None
-        elif previous_value == new_value or {previous_value, new_value} <= {None, "NULL"}:
+        elif earliest_value.value == new_value or {earliest_value.value, new_value} <= {None, NULL_VALUE}:
             action = DiffAction.UNCHANGED
         return (action, new_diff_value.changed_at, previous_value, new_value)
 
@@ -252,11 +261,11 @@ def _get_single_relationship_final_property(
         action = DiffAction.UPDATED
         if last_diff_prop.changed_at < from_time:
             action = DiffAction.UNCHANGED
-        elif previous_value in (None, "NULL") and new_value not in (None, "NULL"):
+        elif previous_value in (None, NULL_VALUE) and new_value not in (None, NULL_VALUE):
             action = DiffAction.ADDED
-        elif previous_value not in (None, "NULL") and new_value in (None, "NULL"):
+        elif previous_value not in (None, NULL_VALUE) and new_value in (None, NULL_VALUE):
             action = DiffAction.REMOVED
-        elif previous_value == new_value or {previous_value, new_value} <= {None, "NULL"}:
+        elif previous_value == new_value or {previous_value, new_value} <= {None, NULL_VALUE}:
             action = DiffAction.UNCHANGED
         return DiffProperty(
             property_type=property_type,
@@ -335,6 +344,7 @@ def add_path(self, database_path: DatabasePath, diff_from_time: Timestamp, diff_
             )
         )
         to_time = database_path.property_to_time
+        # if the to time was set in the time frame, then it is effectively a delete
         if (
             to_time
             and database_path.property_from_time < diff_from_time <= to_time <= diff_to_time
@@ -513,6 +523,11 @@ def parse(self, include_unchanged: bool = False) -> None:
         self._finalize(include_unchanged=include_unchanged)
 
     def _parse_path(self, database_path: DatabasePath) -> None:
+        to_time = database_path.property_to_time
+        # this path was added and removed within the timeframe, so we ignore it
+        if to_time and self.from_time <= database_path.property_from_time <= to_time <= self.to_time:
+            return
+
         diff_root = self._get_diff_root(database_path=database_path)
         diff_node = self._get_diff_node(database_path=database_path, diff_root=diff_root)
         self._update_attribute_level(database_path=database_path, diff_node=diff_node)

backend/tests/unit/core/diff/test_diff_calculator.py

@@ -166,6 +166,49 @@ async def test_attribute_branch_set_null(db: InfrahubDatabase, default_branch: B
     assert before_change < property_diff.changed_at < after_change
 
 
+async def test_attribute_branch_update_from_null(db: InfrahubDatabase, default_branch: Branch, person_john_main: Node):
+    car = await Node.init(db=db, schema="TestCar", branch=default_branch)
+    await car.new(db=db, name="accord", is_electric=False, owner=person_john_main.id)
+    await car.save(db=db)
+    branch = await create_branch(db=db, branch_name="branch")
+    from_time = Timestamp(branch.created_at)
+    car_branch = await NodeManager.get_one(db=db, branch=branch, id=car.id)
+    car_branch.nbr_seats.value = 5
+    before_change = Timestamp()
+    await car_branch.save(db=db)
+    after_change = Timestamp()
+
+    diff_calculator = DiffCalculator(db=db)
+    calculated_diffs = await diff_calculator.calculate_diff(
+        base_branch=default_branch,
+        diff_branch=branch,
+        from_time=from_time,
+        to_time=Timestamp(),
+        include_unchanged=False,
+    )
+
+    base_root_path = calculated_diffs.base_branch_diff
+    assert base_root_path.nodes == []
+    branch_root_path = calculated_diffs.diff_branch_diff
+    assert branch_root_path.branch == branch.name
+    assert len(branch_root_path.nodes) == 1
+    node_diff = branch_root_path.nodes[0]
+    assert node_diff.uuid == car.id
+    assert node_diff.kind == "TestCar"
+    assert node_diff.action is DiffAction.UPDATED
+    assert len(node_diff.attributes) == 1
+    attribute_diff = node_diff.attributes[0]
+    assert attribute_diff.name == "nbr_seats"
+    assert attribute_diff.action is DiffAction.UPDATED
+    assert len(attribute_diff.properties) == 1
+    property_diff = attribute_diff.properties[0]
+    assert property_diff.property_type == DatabaseEdgeType.HAS_VALUE
+    assert property_diff.previous_value == "NULL"
+    assert property_diff.new_value == 5
+    assert property_diff.action is DiffAction.ADDED
+    assert before_change < property_diff.changed_at < after_change
+
+
 @pytest.mark.parametrize("use_branch", [True, False])
 async def test_node_delete(db: InfrahubDatabase, default_branch: Branch, car_accord_main, person_john_main, use_branch):
     if use_branch:
@@ -369,6 +412,87 @@ async def test_attribute_property_multiple_branch_updates(
     assert before_last_change < property_diff.changed_at < after_last_change
 
 
+async def test_attribute_property_branch_create_multiple_updates(
+    db: InfrahubDatabase, default_branch: Branch, person_alfred_main, person_john_main, car_accord_main
+):
+    branch = await create_branch(db=db, branch_name="branch")
+    from_time = Timestamp(branch.created_at)
+    person = await Node.init(db=db, schema="TestPerson", branch=branch)
+    await person.new(db=db, name="Gerald", height=160)
+    await person.save(db=db)
+    after_create = Timestamp()
+    person.name.value = "Gerald Two"
+    await person.save(db=db)
+    person.name.value = "Gerald Three"
+    await person.save(db=db)
+    before_last_change = Timestamp()
+    person.name.value = "Gerald Four"
+    await person.save(db=db)
+    after_last_change = Timestamp()
+
+    diff_calculator = DiffCalculator(db=db)
+    calculated_diffs = await diff_calculator.calculate_diff(
+        base_branch=default_branch,
+        diff_branch=branch,
+        from_time=from_time,
+        to_time=Timestamp(),
+        include_unchanged=False,
+    )
+
+    base_root_path = calculated_diffs.base_branch_diff
+    assert base_root_path.nodes == []
+    root_path = calculated_diffs.diff_branch_diff
+    assert root_path.branch == branch.name
+    assert len(root_path.nodes) == 1
+    node_diff = root_path.nodes[0]
+    assert node_diff.uuid == person.id
+    assert node_diff.kind == "TestPerson"
+    assert node_diff.action is DiffAction.ADDED
+    attributes_by_name = {a.name: a for a in node_diff.attributes}
+    assert set(attributes_by_name.keys()) == {"name", "height"}
+    # name attribute
+    attribute_diff = attributes_by_name["name"]
+    assert attribute_diff.action is DiffAction.ADDED
+    prop_diffs_by_type = {p.property_type: p for p in attribute_diff.properties}
+    assert set(prop_diffs_by_type.keys()) == {
+        DatabaseEdgeType.HAS_VALUE,
+        DatabaseEdgeType.IS_VISIBLE,
+        DatabaseEdgeType.IS_PROTECTED,
+    }
+    for prop_type, new_value in (
+        (DatabaseEdgeType.HAS_VALUE, "Gerald Four"),
+        (DatabaseEdgeType.IS_VISIBLE, True),
+        (DatabaseEdgeType.IS_PROTECTED, False),
+    ):
+        property_diff = prop_diffs_by_type[prop_type]
+        assert property_diff.action is DiffAction.ADDED
+        assert property_diff.previous_value is None
+        assert property_diff.new_value == new_value
+        if prop_type is DatabaseEdgeType.HAS_VALUE:
+            assert before_last_change < property_diff.changed_at < after_last_change
+        else:
+            assert from_time < property_diff.changed_at < after_create
+    # height attribute
+    attribute_diff = attributes_by_name["height"]
+    assert attribute_diff.action is DiffAction.ADDED
+    prop_diffs_by_type = {p.property_type: p for p in attribute_diff.properties}
+    assert set(prop_diffs_by_type.keys()) == {
+        DatabaseEdgeType.HAS_VALUE,
+        DatabaseEdgeType.IS_VISIBLE,
+        DatabaseEdgeType.IS_PROTECTED,
+    }
+    for prop_type, new_value in (
+        (DatabaseEdgeType.HAS_VALUE, 160),
+        (DatabaseEdgeType.IS_VISIBLE, True),
+        (DatabaseEdgeType.IS_PROTECTED, False),
+    ):
+        property_diff = prop_diffs_by_type[prop_type]
+        assert property_diff.action is DiffAction.ADDED
+        assert property_diff.previous_value is None
+        assert property_diff.new_value == new_value
+        assert from_time < property_diff.changed_at < after_create
+
+
 async def test_relationship_one_peer_branch_and_main_update(
     db: InfrahubDatabase,
     default_branch: Branch,
@@ -764,8 +888,11 @@ async def test_add_node_branch(
     branch = await create_branch(db=db, branch_name="branch")
     from_time = Timestamp(branch.created_at)
     new_car = await Node.init(db=db, branch=branch, schema="TestCar")
-    await new_car.new(db=db, name="Batmobile", color="#000000", owner=person_jane_main)
+    await new_car.new(db=db, name="Batmobile", color="#000000", owner=person_alfred_main)
     await new_car.save(db=db)
+    fresh_new_car = await NodeManager.get_one(db=db, branch=branch, id=new_car.id)
+    await fresh_new_car.owner.update(db=db, data=person_jane_main)
+    await fresh_new_car.save(db=db)
 
     diff_calculator = DiffCalculator(db=db)
     calculated_diffs = await diff_calculator.calculate_diff(
@@ -2022,6 +2149,64 @@ async def test_branch_relationship_delete_with_property_update(
         assert before_branch_change < prop_diff.changed_at < after_branch_change
 
 
+async def test_node_deleted_on_base_update_on_branch(
+    db: InfrahubDatabase, default_branch: Branch, person_alfred_main, person_john_main, car_accord_main
+):
+    branch = await create_branch(db=db, branch_name="branch")
+    from_time = Timestamp(branch.created_at)
+    alfred_main = await NodeManager.get_one(db=db, branch=default_branch, id=person_alfred_main.id)
+    await alfred_main.delete(db=db)
+    alfred_branch = await NodeManager.get_one(db=db, branch=branch, id=person_alfred_main.id)
+    alfred_branch.name.value = "Still Alfred"
+    await alfred_branch.save(db=db)
+
+    diff_calculator = DiffCalculator(db=db)
+    calculated_diffs = await diff_calculator.calculate_diff(
+        base_branch=default_branch,
+        diff_branch=branch,
+        from_time=from_time,
+        to_time=Timestamp(),
+        include_unchanged=False,
+    )
+
+    # deleted on base
+    diff_root = calculated_diffs.base_branch_diff
+    assert len(diff_root.nodes) == 1
+    diff_node = diff_root.nodes.pop()
+    assert diff_node.action is DiffAction.REMOVED
+    assert diff_node.uuid == person_alfred_main.id
+    attributes_by_name = {a.name: a for a in diff_node.attributes}
+    assert set(attributes_by_name.keys()) == {"name", "height"}
+    for attr_diff in diff_node.attributes:
+        assert attr_diff.action is DiffAction.REMOVED
+        props_by_type = {p.property_type: p for p in attr_diff.properties}
+        assert set(props_by_type.keys()) == {
+            DatabaseEdgeType.HAS_VALUE,
+            DatabaseEdgeType.IS_PROTECTED,
+            DatabaseEdgeType.IS_VISIBLE,
+        }
+        for prop_diff in attr_diff.properties:
+            assert prop_diff.action is DiffAction.REMOVED
+    assert len(diff_node.relationships) == 0
+    # update on branch
+    diff_root = calculated_diffs.diff_branch_diff
+    assert len(diff_root.nodes) == 1
+    diff_node = diff_root.nodes.pop()
+    assert diff_node.action is DiffAction.UPDATED
+    assert diff_node.uuid == person_alfred_main.id
+    attributes_by_name = {a.name: a for a in diff_node.attributes}
+    assert set(attributes_by_name.keys()) == {"name"}
+    attr_diff = diff_node.attributes.pop()
+    assert attr_diff.action is DiffAction.UPDATED
+    props_by_type = {p.property_type: p for p in attr_diff.properties}
+    assert set(props_by_type.keys()) == {DatabaseEdgeType.HAS_VALUE}
+    prop_diff = attr_diff.properties.pop()
+    assert prop_diff.action is DiffAction.UPDATED
+    assert prop_diff.previous_value == "Alfred"
+    assert prop_diff.new_value == "Still Alfred"
+    assert len(diff_node.relationships) == 0
+
+
 async def test_node_deleted_on_both(
     db: InfrahubDatabase, default_branch: Branch, person_alfred_main, person_john_main, car_accord_main
 ):