Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug in merge logic that caused duplicate edges in the databae #5605

Merged
merged 1 commit into from
Jan 29, 2025
Merged

fix bug in merge logic that caused duplicate edges in the databae #5605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions backend/infrahub/core/diff/merger/serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,10 +294,6 @@ def _serialize_relationship_element(
actions_and_peers = self._get_actions_and_peers(relationship_diff=relationship_diff)
added_peer_ids = [peer_id for action, peer_id in actions_and_peers if action is DiffAction.ADDED]
removed_peer_ids = [peer_id for action, peer_id in actions_and_peers if action is DiffAction.REMOVED]
if not added_peer_ids:
added_peer_ids = [relationship_diff.peer_id]
if not removed_peer_ids:
removed_peer_ids = [relationship_diff.peer_id]
for action, peer_id in actions_and_peers:
if (
peer_id
Expand All @@ -314,6 +310,7 @@ def _serialize_relationship_element(
relationship_diff_properties=relationship_diff.properties,
added_peer_ids=added_peer_ids,
removed_peer_ids=removed_peer_ids,
unchanged_peer_id=relationship_diff.peer_id,
)
return relationship_dicts, relationship_property_dicts

Expand All @@ -324,9 +321,14 @@ def _serialize_relationship_properties(
relationship_diff_properties: set[EnrichedDiffProperty],
added_peer_ids: list[str],
removed_peer_ids: list[str],
unchanged_peer_id: str,
) -> list[RelationshipPropertyMergeDict]:
added_property_dicts = self._get_default_property_merge_dicts(action=DiffAction.ADDED)
removed_property_dicts = self._get_default_property_merge_dicts(action=DiffAction.REMOVED)
added_property_dicts = {}
removed_property_dicts = {}
if added_peer_ids:
added_property_dicts = self._get_default_property_merge_dicts(action=DiffAction.ADDED)
if removed_peer_ids:
removed_property_dicts = self._get_default_property_merge_dicts(action=DiffAction.REMOVED)
for property_diff in relationship_diff_properties:
if property_diff.property_type is DatabaseEdgeType.IS_RELATED:
# handled above
Expand All @@ -348,10 +350,15 @@ def _serialize_relationship_properties(
elif action is DiffAction.REMOVED:
removed_property_dicts[property_diff.property_type] = property_dict
relationship_property_dicts = []
peers_and_property_dicts: list[tuple[str, dict[DatabaseEdgeType, PropertyMergeDict]]] = []
if added_property_dicts:
peers_and_property_dicts = [(peer_id, added_property_dicts) for peer_id in added_peer_ids]
peers_and_property_dicts += [
(peer_id, added_property_dicts) for peer_id in (added_peer_ids or [unchanged_peer_id])
]
if removed_property_dicts:
peers_and_property_dicts += [(peer_id, removed_property_dicts) for peer_id in removed_peer_ids]
peers_and_property_dicts += [
(peer_id, removed_property_dicts) for peer_id in (removed_peer_ids or [unchanged_peer_id])
]
for peer_id, property_dicts in peers_and_property_dicts:
if (
peer_id
Expand Down
66 changes: 66 additions & 0 deletions backend/tests/unit/core/diff/test_diff_and_merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,29 @@
from tests.unit.core.test_utils import verify_all_linked_edges_deleted


async def verify_no_duplicate_paths(db: InfrahubDatabase) -> None:
"""Verify that no duplicate paths exist at the database level"""
query = """
MATCH path = (p)-[e]->(q)
WITH COALESCE(p.uuid, p.value) AS node_id1, e.branch AS branch, e.from AS from_time, type(e) AS edge_type, COALESCE(q.uuid, q.value) AS node_id2, path
WHERE node_id1 IS NOT NULL AND node_id2 IS NOT NULL
WITH node_id1, branch, from_time, edge_type, node_id2, size(collect(path)) AS num_paths
WHERE num_paths > 1
RETURN node_id1, branch, from_time, edge_type, node_id2, num_paths
"""
records = await db.execute_query(query=query)
for record in records:
node_id1 = record.get("node_id1")
branch = record.get("branch")
from_time = record.get("from_time")
edge_type = record.get("edge_type")
node_id2 = record.get("node_id2")
num_paths = record.get("num_paths")
raise ValueError(
f"{num_paths} paths ({branch=},{edge_type=},{from_time=}) between nodes '{node_id1}' and '{node_id2}'"
)


class TestDiffAndMerge:
@pytest.fixture
async def diff_repository(self, db: InfrahubDatabase, default_branch: Branch) -> DiffRepository:
Expand Down Expand Up @@ -57,6 +80,7 @@ async def test_diff_and_merge_with_list_attribute(

updated_node = await NodeManager.get_one(db=db, branch=default_branch, id=new_node.id)
assert updated_node.mylist.value == ["c", "d", 3, 4]
await verify_no_duplicate_paths(db=db)

async def test_diff_and_merge_schema_with_default_values(
self,
Expand Down Expand Up @@ -105,6 +129,7 @@ async def test_diff_and_merge_schema_with_default_values(
assert "num_cupholders" not in attribute_names
assert "is_cool" not in attribute_names
assert "nickname" not in attribute_names
await verify_no_duplicate_paths(db=db)

@pytest.mark.parametrize(
"conflict_selection,expected_value",
Expand Down Expand Up @@ -149,6 +174,7 @@ async def test_diff_and_merge_with_attribute_value_conflict(

rolled_back_john = await NodeManager.get_one(db=db, id=person_john_main.id)
assert rolled_back_john.name.value == "John-main"
await verify_no_duplicate_paths(db=db)

@pytest.mark.parametrize(
"conflict_selection",
Expand Down Expand Up @@ -198,6 +224,7 @@ async def test_diff_and_merge_with_relationship_conflict(
rolled_back_car = await NodeManager.get_one(db=db, id=car_accord_main.id)
owner_rel = await rolled_back_car.owner.get(db=db)
assert owner_rel.peer_id == person_alfred_main.id
await verify_no_duplicate_paths(db=db)

@pytest.mark.parametrize(
"conflict_selection",
Expand Down Expand Up @@ -247,6 +274,7 @@ async def test_diff_and_merge_with_attribute_property_conflict(
rolled_back_john = await NodeManager.get_one(db=db, id=person_john_main.id, include_source=True)
attr_source = await rolled_back_john.name.get_source(db=db)
assert attr_source.id == person_alfred_main.id
await verify_no_duplicate_paths(db=db)

@pytest.mark.parametrize(
"conflict_selection",
Expand Down Expand Up @@ -299,6 +327,7 @@ async def test_diff_and_merge_with_relationship_property_conflict(
owner_rel = await rolled_back_car.owner.get(db=db)
owner_prop = await owner_rel.get_owner(db=db)
assert owner_prop.id == person_alfred_main.id
await verify_no_duplicate_paths(db=db)

@pytest.mark.parametrize("new_height", (0, 1000, None))
async def test_single_attribute_update(
Expand All @@ -321,6 +350,35 @@ async def test_single_attribute_update(

updated_person = await NodeManager.get_one(db=db, id=person_jane_main.id)
assert updated_person.height.value == new_height
await verify_no_duplicate_paths(db=db)

async def test_one_many_relationship_added(
self, db: InfrahubDatabase, default_branch: Branch, person_john_main, person_jane_main, car_camry_main
):
branch2 = await create_branch(db=db, branch_name="branch2")
branch_car = await Node.init(db=db, schema="TestCar", branch=branch2)
await branch_car.new(db=db, name="new camry", nbr_seats=5, is_electric=False, owner=person_jane_main.id)
await branch_car.save(db=db)

diff_coordinator = await self._get_diff_coordinator(db=db, branch=branch2)
enriched_diff = await diff_coordinator.update_branch_diff_and_return(
base_branch=default_branch, diff_branch=branch2
)
car_node = enriched_diff.get_node(node_uuid=branch_car.id)
assert car_node.action is DiffAction.ADDED
person_node = enriched_diff.get_node(node_uuid=person_jane_main.id)
assert person_node.action is DiffAction.UPDATED

diff_merger = await self._get_diff_merger(db=db, branch=branch2)
await diff_merger.merge_graph(at=Timestamp())

updated_car = await NodeManager.get_one(db=db, id=branch_car.id)
assert updated_car.name.value == "new camry"
assert updated_car.nbr_seats.value == 5
assert updated_car.is_electric.value is False
owner_rel = await updated_car.owner.get(db=db)
assert owner_rel.peer_id == person_jane_main.id
await verify_no_duplicate_paths(db=db)

async def test_relationship_set_to_null(self, db: InfrahubDatabase, default_branch: Branch, animal_person_schema):
person_main = await Node.init(db=db, schema="TestPerson")
Expand Down Expand Up @@ -356,6 +414,7 @@ async def test_relationship_set_to_null(self, db: InfrahubDatabase, default_bran
updated_friend = await NodeManager.get_one(db=db, id=friend_main.id)
best_friend_rels = await updated_friend.best_friends.get_relationships(db=db)
assert len(best_friend_rels) == 0
await verify_no_duplicate_paths(db=db)

async def test_local_and_aware_nodes_added_on_branch(
self, db: InfrahubDatabase, default_branch: Branch, car_person_schema_branch_local: SchemaBranch
Expand Down Expand Up @@ -423,6 +482,7 @@ async def test_local_and_aware_nodes_added_on_branch(
)
assert len(owner_rels) == 1
assert owner_rels[0].peer_id == person.id
await verify_no_duplicate_paths(db=db)

async def test_agnostic_and_aware_nodes_added_on_branch(
self, db: InfrahubDatabase, default_branch: Branch, car_person_schema_global
Expand Down Expand Up @@ -499,6 +559,7 @@ async def test_agnostic_and_aware_nodes_added_on_branch(
)
assert len(owner_rels) == 1
assert owner_rels[0].peer_id == person.id
await verify_no_duplicate_paths(db=db)

async def test_update_individual_relationship_properties_one_at_a_time(
self,
Expand Down Expand Up @@ -542,6 +603,7 @@ async def test_update_individual_relationship_properties_one_at_a_time(
assert owner_rel.peer_id == person_john_main.id
assert owner_rel.is_protected is False
assert owner_rel.is_visible is True
await verify_no_duplicate_paths(db=db)

async def test_branch_delete_with_added_base_relationship(
self,
Expand Down Expand Up @@ -604,6 +666,7 @@ async def test_branch_delete_with_added_base_relationship(
assert owner_rel.peer_id == person_john_main.id
assert owner_rel.is_protected is False
assert owner_rel.is_visible is True
await verify_no_duplicate_paths(db=db)

async def test_base_delete_with_added_branch_relationship(
self,
Expand Down Expand Up @@ -663,6 +726,7 @@ async def test_base_delete_with_added_branch_relationship(
# validate that car remains deleted after rollback
rolled_back_car = await NodeManager.get_one(db=db, id=car_accord_main.id)
assert rolled_back_car is None
await verify_no_duplicate_paths(db=db)

async def test_delete_with_many_relationship_added(
self, db: InfrahubDatabase, default_branch: Branch, car_person_schema_unregistered: SchemaRoot
Expand Down Expand Up @@ -713,6 +777,7 @@ async def test_delete_with_many_relationship_added(
# including the relationship connecting car_1 and person_1 is deleted,
# requires a special query b/c TestCar has no relationship to TestPerson in the schema
await verify_all_linked_edges_deleted(db=db, node_uuid=person_1.id, branch_name=default_branch.name)
await verify_no_duplicate_paths(db=db)

@pytest.mark.parametrize("selection", [ConflictSelection.BASE_BRANCH, ConflictSelection.DIFF_BRANCH])
async def test_attribute_update_with_conflict(
Expand Down Expand Up @@ -754,3 +819,4 @@ async def test_attribute_update_with_conflict(
assert updated_person.height.value == branch_value
else:
assert updated_person.height.value == main_value
await verify_no_duplicate_paths(db=db)
Loading