diff --git a/backend/infrahub/core/diff/combiner.py b/backend/infrahub/core/diff/combiner.py index 880b8fb170..bacbf8e486 100644 --- a/backend/infrahub/core/diff/combiner.py +++ b/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( diff --git a/backend/infrahub/core/diff/conflicts_enricher.py b/backend/infrahub/core/diff/conflicts_enricher.py index d4bf47bf66..62ef6179cd 100644 --- a/backend/infrahub/core/diff/conflicts_enricher.py +++ b/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 diff --git a/backend/infrahub/core/diff/enricher/cardinality_one.py b/backend/infrahub/core/diff/enricher/cardinality_one.py index f1fcffd89d..9678e9200a 100644 --- a/backend/infrahub/core/diff/enricher/cardinality_one.py +++ b/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 diff --git a/backend/infrahub/core/diff/merger/serializer.py b/backend/infrahub/core/diff/merger/serializer.py index 85755fd0f1..ad2d700529 100644 --- a/backend/infrahub/core/diff/merger/serializer.py +++ b/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" diff --git a/backend/infrahub/core/diff/query_parser.py b/backend/infrahub/core/diff/query_parser.py index 74122b77d4..bbf7446e7f 100644 --- a/backend/infrahub/core/diff/query_parser.py +++ b/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) diff --git a/backend/tests/unit/core/diff/test_diff_calculator.py b/backend/tests/unit/core/diff/test_diff_calculator.py index b2526954c6..298d909998 100644 --- a/backend/tests/unit/core/diff/test_diff_calculator.py +++ b/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 ):