From 3ba7c10543eadef7708cacc487dcffb3ec1a5fe5 Mon Sep 17 00:00:00 2001 From: Lucas Guillermou Date: Fri, 31 Jan 2025 10:27:19 +0100 Subject: [PATCH 1/6] Add migration to fix database edges state --- backend/infrahub/core/graph/__init__.py | 2 +- .../core/migrations/graph/__init__.py | 2 + .../graph/m019_restore_rels_to_time.py | 153 ++++++++++++++++++ backend/infrahub/core/relationship/model.py | 20 +-- .../infrahub/core/schema/definitions/core.py | 1 + .../unit/core/migrations/graph/test_019.py | 99 ++++++++++++ 6 files changed, 264 insertions(+), 13 deletions(-) create mode 100644 backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py create mode 100644 backend/tests/unit/core/migrations/graph/test_019.py diff --git a/backend/infrahub/core/graph/__init__.py b/backend/infrahub/core/graph/__init__.py index d549910d22..5a991cd92e 100644 --- a/backend/infrahub/core/graph/__init__.py +++ b/backend/infrahub/core/graph/__init__.py @@ -1 +1 @@ -GRAPH_VERSION = 18 +GRAPH_VERSION = 19 diff --git a/backend/infrahub/core/migrations/graph/__init__.py b/backend/infrahub/core/migrations/graph/__init__.py index f8b314ab25..99fc4a3b5e 100644 --- a/backend/infrahub/core/migrations/graph/__init__.py +++ b/backend/infrahub/core/migrations/graph/__init__.py @@ -20,6 +20,7 @@ from .m016_diff_delete_bug_fix import Migration016 from .m017_add_core_profile import Migration017 from .m018_uniqueness_nulls import Migration018 +from .m019_restore_rels_to_time import Migration019 if TYPE_CHECKING: from infrahub.core.root import Root @@ -45,6 +46,7 @@ Migration016, Migration017, Migration018, + Migration019, ] diff --git a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py new file mode 100644 index 0000000000..1e64b11956 --- /dev/null +++ b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py @@ -0,0 +1,153 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING, Any, Sequence + +from infrahub.core.migrations.shared import GraphMigration, MigrationResult +from infrahub.log import get_logger + +from ...constants import GLOBAL_BRANCH_NAME +from ...query import Query, QueryType + +if TYPE_CHECKING: + from infrahub.database import InfrahubDatabase + +log = get_logger() + + +class FixBranchAwareEdgesQuery(Query): + name = "replace_global_edges" + type = QueryType.WRITE + insert_return = False + + async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> None: + """ + Between a Node and a Relationship, if Relationship.branch_support=aware, replace any global edge + to the branch of a non-global edge leaving out of the Relationship node. Note that there can't + be multiple non-global branches on these edges, as a dedicated Relationship node would exist for that. + """ + + query = """ + MATCH (node:Node)-[global_edge:IS_RELATED {branch: $global_branch}]-(rel:Relationship) + MATCH (rel)-[non_global_edge:IS_RELATED]-(node_2: Node) + WHERE non_global_edge.branch <> $global_branch + SET global_edge.branch = non_global_edge.branch + """ + + params = {"global_branch": GLOBAL_BRANCH_NAME} + self.params.update(params) + self.add_to_query(query) + + +class SetMissingToTimeQuery(Query): + name = "set_missing_to_time" + type = QueryType.WRITE + insert_return = False + + async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> None: + """ + If both a deleted edge and an active edge with no time exist between 2 nodes on the same branch, + set `to` time of active edge using `from` time of the deleted one. This would typically happen after having + replaced a deleted edge on global branch by correct branch with above query. + """ + + query = """ + MATCH (node:Node)-[deleted_edge:IS_RELATED {status: "deleted"}]-(rel:Relationship) + MATCH (rel)-[active_edge:IS_RELATED {status: "active"}]-() + WHERE active_edge.to IS NULL AND deleted_edge.branch = active_edge.branch + SET active_edge.to = deleted_edge.from + """ + + params = {"global_branch": GLOBAL_BRANCH_NAME} + self.params.update(params) + self.add_to_query(query) + + +class DeleteNodesRelsQuery(Query): + name = "delete_relationships_of_deleted_nodes" + type = QueryType.WRITE + insert_return = False + + async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> None: + """ + Some nodes may have been deleted while having corrupted state that are fixes by above migrations. + While these nodes edges connected to Root are correctly deleted, + edges connected to other `Node` through a `Relationship` node may still be active. + Following query correctly deletes these edges by both setting correct to time and creating corresponding deleted edge. + """ + + query = """ + MATCH (deleted_node: CoreStandardGroup)-[deleted_edge:IS_PART_OF {status: "deleted"}]->(:Root) + MATCH (deleted_node)-[:IS_RELATED]-(rel:Relationship) + + // Set to time if there is an active edge on deleted edge branch + OPTIONAL MATCH (rel)-[peer_active_edge:IS_RELATED {status: "active"}]-(peer_1: Node) + WHERE peer_active_edge.branch = deleted_edge.branch AND peer_active_edge.to IS NULL + SET peer_active_edge.to = deleted_edge.from + + // Check if deleted edge exists on this branch between Relationship and any peer_2 Node connected. Create it if it doesn't. + WITH deleted_edge.branch AS branch, deleted_edge.branch_level AS branch_level, deleted_edge.from as deleted_time, rel + MATCH (rel)-[:IS_RELATED]-(peer_2:Node) + CALL { + WITH rel, peer_2, branch + OPTIONAL MATCH (rel)-[r:IS_RELATED {branch: branch}]-(peer_2) + WHERE r.status = "deleted" + RETURN r IS NOT NULL AS has_deleted_edge + } + + // The branch on which `deleted` edge might be created depends on Relationship.branch_support + WITH branch, branch_level, deleted_time, rel, has_deleted_edge, peer_2 + WHERE has_deleted_edge = FALSE // only look at rel-peer_2 couples not having a deleted edge + OPTIONAL MATCH (rel)-[active_edge:IS_RELATED {status: "active"}]-(peer_3: Node) + WHERE active_edge.branch IS NOT NULL + WITH rel, active_edge, peer_3, + CASE + WHEN rel.branch_support = "agnostic" THEN $global_branch + WHEN rel.branch_support = "aware" THEN COALESCE(active_edge.branch, NULL) + ELSE NULL // Ending up here means there is no active branch between rel its peer Node, + // so there must be a deleted edge already, and thus we will not create one. + END AS branch, + branch_level, + deleted_time, + peer_2 + + // Need 2 calls to create the edge in the correct direction. Also note that MERGE ensures we do not create multiple times. + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)-[:IS_RELATED]->(peer_2) + MERGE (rel)-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + } + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)<-[:IS_RELATED]-(peer_2) + MERGE (rel)<-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + } + """ + + params = {"global_branch": GLOBAL_BRANCH_NAME} + self.params.update(params) + self.add_to_query(query) + + +class Migration019(GraphMigration): + """ + Fix corrupted state introduced by Migration012 when duplicating a CoreAccount (branch Aware) + being part of a CoreStandardGroup (branch Agnostic). Database is corrupted at multiple points: + - Old CoreAccount node <> group_member node `active` edge has no `to` time (possibly because of #5590). + - Old CoreAccount node <> group_member node `deleted` edge is on `-global-` branch instead of `main`. + - New CoreAccount node <> group_member node `active` edge is on `-global-` branch instead of `main`. + + Also, users having deleted corresponding CoreStandardGroup will also have the following data corruption, + as deletion did not happen correctly due to above issues: + - Both CoreAccount <> group_member and CoreStandardGroup <> group_member edges + have not been deleted (ie status is `active` without `to` time and no additional `deleted` edge). + + This migration fixes all above issues to have consistent edges, and fixes IFC-1204. + """ + + name: str = "019_fix_edges_state" + minimum_version: int = 18 + queries: Sequence[type[Query]] = [FixBranchAwareEdgesQuery, SetMissingToTimeQuery, DeleteNodesRelsQuery] + + async def validate_migration(self, db: InfrahubDatabase) -> MigrationResult: + result = MigrationResult() + return result diff --git a/backend/infrahub/core/relationship/model.py b/backend/infrahub/core/relationship/model.py index ff9fe9b17f..82a928cfda 100644 --- a/backend/infrahub/core/relationship/model.py +++ b/backend/infrahub/core/relationship/model.py @@ -884,6 +884,8 @@ def get_branch_based_on_support_type(self) -> Branch: """If the attribute is branch aware, return the Branch object associated with this attribute If the attribute is branch agnostic return the Global Branch + Note that if this relationship is Aware and source node is Agnostic, it will return -global- branch. + Returns: Branch: """ @@ -959,7 +961,7 @@ async def _fetch_relationships( self.has_fetched_relationships = True for peer_id in details.peer_ids_present_local_only: - await self.remove(peer_id=peer_id, db=db) + await self.remove_locally(peer_id=peer_id, db=db) async def get(self, db: InfrahubDatabase) -> Relationship | list[Relationship] | None: rels = await self.get_relationships(db=db) @@ -1077,22 +1079,17 @@ async def resolve(self, db: InfrahubDatabase) -> None: for rel in self._relationships: await rel.resolve(db=db) - async def remove( + async def remove_locally( self, peer_id: Union[str, UUID], db: InfrahubDatabase, - update_db: bool = False, ) -> bool: - """Remove a peer id from the local relationships list, - need to investigate if and when we should update the relationship in the database.""" + """Remove a peer id from the local relationships list""" for idx, rel in enumerate(await self.get_relationships(db=db)): if str(rel.peer_id) != str(peer_id): continue - if update_db: - await rel.delete(db=db) - self._relationships.pop(idx) return True @@ -1109,14 +1106,13 @@ async def remove_in_db( # - Update the existing relationship if we are on the same branch rel_ids_per_branch = peer_data.rel_ids_per_branch() + + # In which cases do we end up here and do not want to set `to` time? if branch.name in rel_ids_per_branch: await update_relationships_to([str(ri) for ri in rel_ids_per_branch[branch.name]], to=remove_at, db=db) # - Create a new rel of type DELETED if the existing relationship is on a different branch - rel_branches: set[str] = set() - if peer_data.rels: - rel_branches = {r.branch for r in peer_data.rels} - if rel_branches == {peer_data.branch}: + if peer_data.rels and {r.branch for r in peer_data.rels} == {peer_data.branch}: return query = await RelationshipDataDeleteQuery.init( diff --git a/backend/infrahub/core/schema/definitions/core.py b/backend/infrahub/core/schema/definitions/core.py index 1acbe0e36f..9e855f015b 100644 --- a/backend/infrahub/core/schema/definitions/core.py +++ b/backend/infrahub/core/schema/definitions/core.py @@ -223,6 +223,7 @@ "optional": True, "identifier": "group_member", "cardinality": "many", + "branch": BranchSupportType.AWARE, }, { "name": "subscribers", diff --git a/backend/tests/unit/core/migrations/graph/test_019.py b/backend/tests/unit/core/migrations/graph/test_019.py new file mode 100644 index 0000000000..54a278cf47 --- /dev/null +++ b/backend/tests/unit/core/migrations/graph/test_019.py @@ -0,0 +1,99 @@ +from infrahub_sdk import InfrahubClient + +from infrahub.core.constants import GLOBAL_BRANCH_NAME +from infrahub.core.migrations.graph import Migration019 +from infrahub.core.node import Node +from infrahub.database import InfrahubDatabase +from tests.helpers.test_app import TestInfrahubApp + + +class TestMigration019(TestInfrahubApp): + async def test_migration_019( + self, + client: InfrahubClient, + db: InfrahubDatabase, + default_branch, + ): + """ + Reproduce corrupted state introduced by migration 12, and apply the migration fixing it. + """ + + test_group = await Node.init(db=db, schema="CoreStandardGroup") + await test_group.new(db=db, name="test_group") + await test_group.save(db=db) + + core_acc = await Node.init(db=db, schema="CoreAccount") + await core_acc.new(db=db, name="core_acc", account_type="User", password="def", member_of_groups=[test_group]) + await core_acc.save(db=db) + + # Delete CoreStandardGroup. This should also (correctly) update rels to CoreGenericAccount and CoreAccount + # but we will override them afterward to reproduce corrupted state. + await test_group.delete(db=db) + + async with db.start_session() as dbs: + async with dbs.start_transaction() as ts: + # Make relationship between CoreAccount <> group_member <> CoreStandardGroup active while it should have been deleted. + # and make the group_member <> CoreAccount edge part on global branch + + query = """ + MATCH (new_core_acc: CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) + MATCH (new_core_acc)-[r1:IS_RELATED]-(group_member: Relationship)-[r2:IS_RELATED]-(test_group: CoreStandardGroup) + MATCH (new_core_acc)-[active_r1]-(group_member) + MATCH (new_core_acc)-[deleted_r1]-(group_member) + MATCH (test_group)-[active_r2]-(group_member) + MATCH (test_group)-[deleted_r2]-(group_member) + + WHERE active_r1.status = 'active' AND deleted_r1.status = 'deleted' + AND active_r2.status = 'active' AND deleted_r2.status = 'deleted' + + DELETE deleted_r1 + REMOVE active_r1.to + SET active_r1.branch = '-global' + + DELETE deleted_r2 + REMOVE active_r2.to + + return new_core_acc, group_member, test_group + """ + + await ts.execute_query(query=query, name="query_1") + + # Create the old CoreAccount object - not inheriting from GenericAccount - + # sharing same attributes / relationships than above CoreAccount + + query_2 = """ + // Match the existing CoreAccount node with the specified attributes + MATCH (new_core_acc:CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) + + // Create the new CoreAccount node with the same uuid and additional properties + CREATE (new_node:CoreAccount:LineageOwner:LineageSource:Node {uuid: new_core_acc.uuid, + branch_support: new_core_acc.branch_support, namespace: new_core_acc.namespace, kind: "CoreAccount"}) + + WITH new_node, new_core_acc + + // Match the relationships of the existing CoreAccount node + MATCH (new_core_acc)-[r:IS_RELATED]->(group_member:Relationship {name: "group_member"}) + + // Create active branch with no to time on main branch + CREATE (new_node)-[:IS_RELATED {branch: "main", from: "2024-02-05T15:37:07.228145Z", status: "active"}]->(group_member) + + // Create deleted branch with no to time on global branch + CREATE (new_node)-[:IS_RELATED {branch: $global_branch, from: r.from, status: "deleted"}]->(group_member) + + // Return the new_node + RETURN new_node; + """ + + await ts.execute_query(query=query_2, name="query_2", params={"global_branch": GLOBAL_BRANCH_NAME}) + + # Make sure migration executes without error, and that we can query accounts afterwards. + # Note generated corrupted state does not trigger IFC-1204 bug, + # but a manual test confirmed migration solves this issue. + + migration = Migration019() + await migration.execute(db=db) + await migration.validate_migration(db=db) + + # Trigger e2e path to query this account + core_acc = await client.get(kind="CoreAccount", id=core_acc.id, prefetch_relationships=True) + assert core_acc.name.value == "core_acc" From 5ad182a47b14f880cd9a53921dc0eb7041dbe9a7 Mon Sep 17 00:00:00 2001 From: Lucas Guillermou Date: Wed, 12 Feb 2025 11:55:48 +0100 Subject: [PATCH 2/6] Fix queries --- .../graph/m019_restore_rels_to_time.py | 183 ++++++++++++++---- 1 file changed, 143 insertions(+), 40 deletions(-) diff --git a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py index 1e64b11956..a70af9e093 100644 --- a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py +++ b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py @@ -5,7 +5,7 @@ from infrahub.core.migrations.shared import GraphMigration, MigrationResult from infrahub.log import get_logger -from ...constants import GLOBAL_BRANCH_NAME +from ...constants import GLOBAL_BRANCH_NAME, BranchSupportType from ...query import Query, QueryType if TYPE_CHECKING: @@ -28,12 +28,18 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No query = """ MATCH (node:Node)-[global_edge:IS_RELATED {branch: $global_branch}]-(rel:Relationship) + WHERE rel.branch_support=$branch_aware MATCH (rel)-[non_global_edge:IS_RELATED]-(node_2: Node) WHERE non_global_edge.branch <> $global_branch SET global_edge.branch = non_global_edge.branch """ - params = {"global_branch": GLOBAL_BRANCH_NAME} + params = { + "global_branch": GLOBAL_BRANCH_NAME, + "branch_aware": BranchSupportType.AWARE.value, + "branch_agnostic": BranchSupportType.AGNOSTIC.value, + } + self.params.update(params) self.add_to_query(query) @@ -52,7 +58,7 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No query = """ MATCH (node:Node)-[deleted_edge:IS_RELATED {status: "deleted"}]-(rel:Relationship) - MATCH (rel)-[active_edge:IS_RELATED {status: "active"}]-() + MATCH (rel)-[active_edge:IS_RELATED {status: "active"}]-(node) WHERE active_edge.to IS NULL AND deleted_edge.branch = active_edge.branch SET active_edge.to = deleted_edge.from """ @@ -69,61 +75,158 @@ class DeleteNodesRelsQuery(Query): async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> None: """ - Some nodes may have been deleted while having corrupted state that are fixes by above migrations. + Some nodes may have been deleted while having corrupted state that are fixed by above migrations. While these nodes edges connected to Root are correctly deleted, edges connected to other `Node` through a `Relationship` node may still be active. Following query correctly deletes these edges by both setting correct to time and creating corresponding deleted edge. """ query = """ - MATCH (deleted_node: CoreStandardGroup)-[deleted_edge:IS_PART_OF {status: "deleted"}]->(:Root) + MATCH (deleted_node: Node)-[deleted_edge:IS_PART_OF {status: "deleted"}]->(:Root) MATCH (deleted_node)-[:IS_RELATED]-(rel:Relationship) - // Set to time if there is an active edge on deleted edge branch - OPTIONAL MATCH (rel)-[peer_active_edge:IS_RELATED {status: "active"}]-(peer_1: Node) - WHERE peer_active_edge.branch = deleted_edge.branch AND peer_active_edge.to IS NULL - SET peer_active_edge.to = deleted_edge.from + // exclude nodes having been deleted through migration. find those with same uuid and exclude the one with earlier + // timestamp on active branch + WHERE NOT EXISTS { + MATCH (deleted_node)-[e1:IS_RELATED]-(rel)-[e2:IS_RELATED]-(other_node) + WITH deleted_node, other_node, MIN(e1.from) AS min_e1_from, MIN(e2.from) AS min_e2_from + WHERE deleted_node <> other_node AND deleted_node.uuid = other_node.uuid AND min_e1_from < min_e2_from + } - // Check if deleted edge exists on this branch between Relationship and any peer_2 Node connected. Create it if it doesn't. - WITH deleted_edge.branch AS branch, deleted_edge.branch_level AS branch_level, deleted_edge.from as deleted_time, rel - MATCH (rel)-[:IS_RELATED]-(peer_2:Node) + // Set to time if there is an active edge on deleted edge branch CALL { - WITH rel, peer_2, branch - OPTIONAL MATCH (rel)-[r:IS_RELATED {branch: branch}]-(peer_2) - WHERE r.status = "deleted" - RETURN r IS NOT NULL AS has_deleted_edge + WITH rel, deleted_edge + OPTIONAL MATCH (rel)-[peer_active_edge {status: "active"}]-(peer_1) + WHERE peer_active_edge.branch = deleted_edge.branch AND peer_active_edge.to IS NULL + SET peer_active_edge.to = deleted_edge.from } - // The branch on which `deleted` edge might be created depends on Relationship.branch_support - WITH branch, branch_level, deleted_time, rel, has_deleted_edge, peer_2 - WHERE has_deleted_edge = FALSE // only look at rel-peer_2 couples not having a deleted edge - OPTIONAL MATCH (rel)-[active_edge:IS_RELATED {status: "active"}]-(peer_3: Node) - WHERE active_edge.branch IS NOT NULL - WITH rel, active_edge, peer_3, - CASE - WHEN rel.branch_support = "agnostic" THEN $global_branch - WHEN rel.branch_support = "aware" THEN COALESCE(active_edge.branch, NULL) - ELSE NULL // Ending up here means there is no active branch between rel its peer Node, - // so there must be a deleted edge already, and thus we will not create one. - END AS branch, - branch_level, - deleted_time, - peer_2 - - // Need 2 calls to create the edge in the correct direction. Also note that MERGE ensures we do not create multiple times. + // Get distinct rel nodes linked to a deleted node, with the time at which we should delete rel edges. + // Take the MAX time so if it does not take the deleted time of a node deleted through a duplication migration. + WITH DISTINCT rel, deleted_edge.branch AS deleted_edge_branch, + deleted_edge.branch_level AS branch_level, MAX(deleted_edge.from) as deleted_time + + + // No need to check deleted edge branch because + // If deleted_node has different branch support type (agnostic/aware) than rel type, + // there might already be a deleted edge that we would not match if we filter on deleted_edge_branch. + // If both are aware, it still works, as we would have one Relationship node for each branch on which this relationship exists. + MATCH (rel)-[]-(peer_2) + WHERE NOT exists((rel)-[{status: "deleted"}]-(peer_2)) + + + // If res is agnostic, we should delete on global branch (and we do not use deleted_edge_branch) + // If rel is aware and deleted node is aware, we should use deleted edge branch + // If rel is aware and delete node is agnostic, we need to create deleted edges for every distinct branch on which this relationship exists. + WITH DISTINCT + CASE + // Branch on which `deleted` edge should be created depends on rel.branch_support. + WHEN rel.branch_support = $branch_agnostic THEN [$global_branch] + WHEN rel.branch_aware = $branch_aware THEN + CASE + WHEN deleted_node.branch_support = $branch_aware AND exists((rel)-[{status: "active", branch: deleted_edge_branch}]-(peer_2)) + THEN [deleted_edge_branch] + WHEN deleted_node.branch_support = $branch_agnostic + THEN COLLECT { + WITH rel + MATCH (rel)-[active_edge {status: "active"}]-(peer_2) + RETURN DISTINCT active_edge.branch + } as branches + ELSE [] // deleted_node.branch_support = -local-, TODO what to do? + ELSE [] // rel.branch_support = -local-, TODO what to do? + END AS branches, + branch_level, + deleted_time, + peer_2, + rel + + UNWIND branches as branch + + // Then creates `deleted` edge. + // Below CALL subqueries are called once for each rel-peer_2 pair for which we want to create a deleted edge. + // Note that with current infrahub relationships edges design, only one of this CALL should be matched per pair. + + CALL { WITH rel, peer_2, branch, branch_level, deleted_time - MATCH (rel)-[:IS_RELATED]->(peer_2) - MERGE (rel)-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + MATCH (rel)-[e]->(peer_2) + MERGE (rel)-[:type(e) {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) } + CALL { WITH rel, peer_2, branch, branch_level, deleted_time - MATCH (rel)<-[:IS_RELATED]-(peer_2) - MERGE (rel)<-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + MATCH (rel)-[e]->(peer_2) + MERGE (rel)-[:type(e) {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) } + + + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)-[:IS_RELATED]->(peer_2) + # MERGE (rel)-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)-[:IS_PROTECTED]->(peer_2) + # MERGE (rel)-[:IS_PROTECTED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)-[:IS_VISIBLE]->(peer_2) + # MERGE (rel)-[:IS_VISIBLE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)<-[:IS_RELATED]-(peer_2) + # MERGE (rel)<-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)<-[:IS_PROTECTED]-(peer_2) + # MERGE (rel)<-[:IS_PROTECTED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)<-[:IS_VISIBLE]-(peer_2) + # MERGE (rel)<-[:IS_VISIBLE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)<-[:HAS_OWNER]-(peer_2) + # MERGE (rel)<-[:HAS_OWNER {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)<-[:HAS_SOURCE]-(peer_2) + # MERGE (rel)<-[:HAS_SOURCE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)-[:HAS_OWNER]->(peer_2) + # MERGE (rel)-[:HAS_OWNER {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + # } + # + # CALL { + # WITH rel, peer_2, branch, branch_level, deleted_time + # MATCH (rel)<-[:HAS_SOURCE]->(peer_2) + # MERGE (rel)<-[:HAS_SOURCE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + # } """ - params = {"global_branch": GLOBAL_BRANCH_NAME} + params = { + "global_branch": GLOBAL_BRANCH_NAME, + "branch_aware": BranchSupportType.AWARE.value, + "branch_agnostic": BranchSupportType.AGNOSTIC.value, + } + self.params.update(params) self.add_to_query(query) @@ -133,8 +236,8 @@ class Migration019(GraphMigration): Fix corrupted state introduced by Migration012 when duplicating a CoreAccount (branch Aware) being part of a CoreStandardGroup (branch Agnostic). Database is corrupted at multiple points: - Old CoreAccount node <> group_member node `active` edge has no `to` time (possibly because of #5590). - - Old CoreAccount node <> group_member node `deleted` edge is on `-global-` branch instead of `main`. - - New CoreAccount node <> group_member node `active` edge is on `-global-` branch instead of `main`. + - Old CoreAccount node <> group_member node `deleted` edge is on `$global_branch` branch instead of `main`. + - New CoreAccount node <> group_member node `active` edge is on `$global_branch` branch instead of `main`. Also, users having deleted corresponding CoreStandardGroup will also have the following data corruption, as deletion did not happen correctly due to above issues: From 7ee1492ba8a1f93678ccee9650c67c58d9fc92ac Mon Sep 17 00:00:00 2001 From: Lucas Guillermou Date: Fri, 14 Feb 2025 18:42:46 +0100 Subject: [PATCH 3/6] Improve computation of branch on which create the deleted edge --- .../graph/m019_restore_rels_to_time.py | 154 +++++++++--------- 1 file changed, 74 insertions(+), 80 deletions(-) diff --git a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py index a70af9e093..70e7ed1095 100644 --- a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py +++ b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py @@ -27,10 +27,10 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No """ query = """ - MATCH (node:Node)-[global_edge:IS_RELATED {branch: $global_branch}]-(rel:Relationship) - WHERE rel.branch_support=$branch_aware + MATCH (node:Node)-[global_edge:IS_RELATED {branch: "-global-"}]-(rel:Relationship) + WHERE rel.branch_support="aware" MATCH (rel)-[non_global_edge:IS_RELATED]-(node_2: Node) - WHERE non_global_edge.branch <> $global_branch + WHERE non_global_edge.branch <> "-global-" SET global_edge.branch = non_global_edge.branch """ @@ -63,8 +63,6 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No SET active_edge.to = deleted_edge.from """ - params = {"global_branch": GLOBAL_BRANCH_NAME} - self.params.update(params) self.add_to_query(query) @@ -93,18 +91,26 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No WHERE deleted_node <> other_node AND deleted_node.uuid = other_node.uuid AND min_e1_from < min_e2_from } + // Note that if an AWARE node has been deleted on a branch and relationship is AGNOSTIC, we do not "delete" this relationship + // right now as this aware node might exist on another branch. Or, should we check there is no existing active is_part_of + // to consider this node completely deleted and so we can also deleted connected agnostic relationships? + // Set to time if there is an active edge on deleted edge branch CALL { WITH rel, deleted_edge OPTIONAL MATCH (rel)-[peer_active_edge {status: "active"}]-(peer_1) - WHERE peer_active_edge.branch = deleted_edge.branch AND peer_active_edge.to IS NULL + WHERE (peer_active_edge.branch = deleted_edge.branch OR (rel.branch_support = "aware" AND deleted_edge.branch = "-global-")) + AND peer_active_edge.to IS NULL SET peer_active_edge.to = deleted_edge.from } // Get distinct rel nodes linked to a deleted node, with the time at which we should delete rel edges. // Take the MAX time so if it does not take the deleted time of a node deleted through a duplication migration. - WITH DISTINCT rel, deleted_edge.branch AS deleted_edge_branch, - deleted_edge.branch_level AS branch_level, MAX(deleted_edge.from) as deleted_time + WITH DISTINCT rel, + deleted_edge.branch AS deleted_edge_branch, + deleted_edge.branch_level AS branch_level, + MAX(deleted_edge.from) as deleted_time, + deleted_node.branch_support as deleted_node_branch_support // No need to check deleted edge branch because @@ -121,19 +127,21 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No WITH DISTINCT CASE // Branch on which `deleted` edge should be created depends on rel.branch_support. - WHEN rel.branch_support = $branch_agnostic THEN [$global_branch] - WHEN rel.branch_aware = $branch_aware THEN + WHEN rel.branch_support = "agnostic" AND deleted_node_branch_support = "agnostic" THEN ["-global-"] + WHEN rel.branch_support = "aware" THEN CASE - WHEN deleted_node.branch_support = $branch_aware AND exists((rel)-[{status: "active", branch: deleted_edge_branch}]-(peer_2)) + // in following case, if no active edge on this branch exists it means this relationship node is dedicated for another branch + WHEN deleted_node_branch_support = "aware" AND exists((rel)-[{status: "active", branch: deleted_edge_branch}]-(peer_2)) THEN [deleted_edge_branch] - WHEN deleted_node.branch_support = $branch_agnostic + WHEN deleted_node_branch_support = "agnostic" THEN COLLECT { WITH rel MATCH (rel)-[active_edge {status: "active"}]-(peer_2) RETURN DISTINCT active_edge.branch - } as branches - ELSE [] // deleted_node.branch_support = -local-, TODO what to do? - ELSE [] // rel.branch_support = -local-, TODO what to do? + } + ELSE [] // deleted_node.branch_support = -local- + END + ELSE [] // rel.branch_support = -local- END AS branches, branch_level, deleted_time, @@ -146,79 +154,65 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No // Below CALL subqueries are called once for each rel-peer_2 pair for which we want to create a deleted edge. // Note that with current infrahub relationships edges design, only one of this CALL should be matched per pair. + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)-[:IS_RELATED]->(peer_2) + MERGE (rel)-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + } + + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)-[:IS_PROTECTED]->(peer_2) + MERGE (rel)-[:IS_PROTECTED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + } + + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)-[:IS_VISIBLE]->(peer_2) + MERGE (rel)-[:IS_VISIBLE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + } CALL { WITH rel, peer_2, branch, branch_level, deleted_time - MATCH (rel)-[e]->(peer_2) - MERGE (rel)-[:type(e) {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + MATCH (rel)-[:HAS_OWNER]->(peer_2) + MERGE (rel)-[:HAS_OWNER {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) } CALL { WITH rel, peer_2, branch, branch_level, deleted_time - MATCH (rel)-[e]->(peer_2) - MERGE (rel)-[:type(e) {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) + MATCH (rel)-[:HAS_SOURCE]->(peer_2) + MERGE (rel)-[:HAS_SOURCE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) } + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)<-[:IS_RELATED]-(peer_2) + MERGE (rel)<-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + } - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)-[:IS_RELATED]->(peer_2) - # MERGE (rel)-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)-[:IS_PROTECTED]->(peer_2) - # MERGE (rel)-[:IS_PROTECTED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)-[:IS_VISIBLE]->(peer_2) - # MERGE (rel)-[:IS_VISIBLE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)<-[:IS_RELATED]-(peer_2) - # MERGE (rel)<-[:IS_RELATED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)<-[:IS_PROTECTED]-(peer_2) - # MERGE (rel)<-[:IS_PROTECTED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)<-[:IS_VISIBLE]-(peer_2) - # MERGE (rel)<-[:IS_VISIBLE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)<-[:HAS_OWNER]-(peer_2) - # MERGE (rel)<-[:HAS_OWNER {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)<-[:HAS_SOURCE]-(peer_2) - # MERGE (rel)<-[:HAS_SOURCE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)-[:HAS_OWNER]->(peer_2) - # MERGE (rel)-[:HAS_OWNER {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) - # } - # - # CALL { - # WITH rel, peer_2, branch, branch_level, deleted_time - # MATCH (rel)<-[:HAS_SOURCE]->(peer_2) - # MERGE (rel)<-[:HAS_SOURCE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]->(peer_2) - # } + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)<-[:IS_PROTECTED]-(peer_2) + MERGE (rel)<-[:IS_PROTECTED {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + } + + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)<-[:IS_VISIBLE]-(peer_2) + MERGE (rel)<-[:IS_VISIBLE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + } + + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)<-[:HAS_OWNER]-(peer_2) + MERGE (rel)<-[:HAS_OWNER {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + } + + CALL { + WITH rel, peer_2, branch, branch_level, deleted_time + MATCH (rel)<-[:HAS_SOURCE]-(peer_2) + MERGE (rel)<-[:HAS_SOURCE {status: "deleted", branch: branch, branch_level: branch_level, from: deleted_time}]-(peer_2) + } """ params = { @@ -236,8 +230,8 @@ class Migration019(GraphMigration): Fix corrupted state introduced by Migration012 when duplicating a CoreAccount (branch Aware) being part of a CoreStandardGroup (branch Agnostic). Database is corrupted at multiple points: - Old CoreAccount node <> group_member node `active` edge has no `to` time (possibly because of #5590). - - Old CoreAccount node <> group_member node `deleted` edge is on `$global_branch` branch instead of `main`. - - New CoreAccount node <> group_member node `active` edge is on `$global_branch` branch instead of `main`. + - Old CoreAccount node <> group_member node `deleted` edge is on `"-global-"` branch instead of `main`. + - New CoreAccount node <> group_member node `active` edge is on `"-global-"` branch instead of `main`. Also, users having deleted corresponding CoreStandardGroup will also have the following data corruption, as deletion did not happen correctly due to above issues: From 69f19cfa57383f9562934e4b714271a85d17e4e0 Mon Sep 17 00:00:00 2001 From: Lucas Guillermou Date: Mon, 17 Feb 2025 17:20:35 +0100 Subject: [PATCH 4/6] Same behavior for local and aware branch support type --- .../graph/m019_restore_rels_to_time.py | 52 ++++++++++--------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py index 70e7ed1095..5d30c5e44a 100644 --- a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py +++ b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py @@ -27,10 +27,10 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No """ query = """ - MATCH (node:Node)-[global_edge:IS_RELATED {branch: "-global-"}]-(rel:Relationship) - WHERE rel.branch_support="aware" + MATCH (node:Node)-[global_edge:IS_RELATED {branch: $global_branch}]-(rel:Relationship) + WHERE rel.branch_support=$branch_aware MATCH (rel)-[non_global_edge:IS_RELATED]-(node_2: Node) - WHERE non_global_edge.branch <> "-global-" + WHERE non_global_edge.branch <> $global_branch SET global_edge.branch = non_global_edge.branch """ @@ -92,14 +92,13 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No } // Note that if an AWARE node has been deleted on a branch and relationship is AGNOSTIC, we do not "delete" this relationship - // right now as this aware node might exist on another branch. Or, should we check there is no existing active is_part_of - // to consider this node completely deleted and so we can also deleted connected agnostic relationships? + // right now as this aware node might exist on another branch. // Set to time if there is an active edge on deleted edge branch CALL { WITH rel, deleted_edge OPTIONAL MATCH (rel)-[peer_active_edge {status: "active"}]-(peer_1) - WHERE (peer_active_edge.branch = deleted_edge.branch OR (rel.branch_support = "aware" AND deleted_edge.branch = "-global-")) + WHERE (peer_active_edge.branch = deleted_edge.branch OR (rel.branch_support <> $branch_agnostic AND deleted_edge.branch = $global_branch)) AND peer_active_edge.to IS NULL SET peer_active_edge.to = deleted_edge.from } @@ -121,28 +120,33 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No WHERE NOT exists((rel)-[{status: "deleted"}]-(peer_2)) - // If res is agnostic, we should delete on global branch (and we do not use deleted_edge_branch) + // If res is agnostic and delete node is agnostic, we should delete on global branch // If rel is aware and deleted node is aware, we should use deleted edge branch // If rel is aware and delete node is agnostic, we need to create deleted edges for every distinct branch on which this relationship exists. WITH DISTINCT - CASE - // Branch on which `deleted` edge should be created depends on rel.branch_support. - WHEN rel.branch_support = "agnostic" AND deleted_node_branch_support = "agnostic" THEN ["-global-"] - WHEN rel.branch_support = "aware" THEN + CASE + // Branch on which `deleted` edge should be created depends on rel.branch_support. + WHEN rel.branch_support = $branch_agnostic + THEN CASE + WHEN deleted_node_branch_support = $branch_agnostic THEN [$global_branch] + ELSE [] + END + ELSE CASE - // in following case, if no active edge on this branch exists it means this relationship node is dedicated for another branch - WHEN deleted_node_branch_support = "aware" AND exists((rel)-[{status: "active", branch: deleted_edge_branch}]-(peer_2)) - THEN [deleted_edge_branch] - WHEN deleted_node_branch_support = "agnostic" + WHEN deleted_node_branch_support = $branch_agnostic THEN COLLECT { - WITH rel - MATCH (rel)-[active_edge {status: "active"}]-(peer_2) - RETURN DISTINCT active_edge.branch - } - ELSE [] // deleted_node.branch_support = -local- + WITH rel + MATCH (rel)-[active_edge {status: "active"}]-(peer_2) + RETURN DISTINCT active_edge.branch + } + ELSE + CASE + // if no active edge on this branch exists it means this relationship node is dedicated for another branch + WHEN exists((rel)-[{status: "active", branch: deleted_edge_branch}]-(peer_2)) THEN [deleted_edge_branch] + ELSE [] + END END - ELSE [] // rel.branch_support = -local- - END AS branches, + END AS branches, branch_level, deleted_time, peer_2, @@ -230,8 +234,8 @@ class Migration019(GraphMigration): Fix corrupted state introduced by Migration012 when duplicating a CoreAccount (branch Aware) being part of a CoreStandardGroup (branch Agnostic). Database is corrupted at multiple points: - Old CoreAccount node <> group_member node `active` edge has no `to` time (possibly because of #5590). - - Old CoreAccount node <> group_member node `deleted` edge is on `"-global-"` branch instead of `main`. - - New CoreAccount node <> group_member node `active` edge is on `"-global-"` branch instead of `main`. + - Old CoreAccount node <> group_member node `deleted` edge is on `$global_branch` branch instead of `main`. + - New CoreAccount node <> group_member node `active` edge is on `$global_branch` branch instead of `main`. Also, users having deleted corresponding CoreStandardGroup will also have the following data corruption, as deletion did not happen correctly due to above issues: From a8bb971493b9768a0c6f63660509e6f96b03c16a Mon Sep 17 00:00:00 2001 From: Lucas Guillermou Date: Tue, 18 Feb 2025 10:41:35 +0100 Subject: [PATCH 5/6] Improve test --- .../graph/m019_restore_rels_to_time.py | 5 +- .../unit/core/migrations/graph/test_019.py | 117 +++++++++++------- 2 files changed, 74 insertions(+), 48 deletions(-) diff --git a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py index 5d30c5e44a..ffded6c6ab 100644 --- a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py +++ b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py @@ -73,9 +73,8 @@ class DeleteNodesRelsQuery(Query): async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> None: """ - Some nodes may have been deleted while having corrupted state that are fixed by above migrations. - While these nodes edges connected to Root are correctly deleted, - edges connected to other `Node` through a `Relationship` node may still be active. + Some nodes may have been incorrectly deleted, typically, while these nodes edges connected to Root + are correctly deleted, edges connected to other `Node` through a `Relationship` node may still be active. Following query correctly deletes these edges by both setting correct to time and creating corresponding deleted edge. """ diff --git a/backend/tests/unit/core/migrations/graph/test_019.py b/backend/tests/unit/core/migrations/graph/test_019.py index 54a278cf47..4574f38383 100644 --- a/backend/tests/unit/core/migrations/graph/test_019.py +++ b/backend/tests/unit/core/migrations/graph/test_019.py @@ -30,61 +30,60 @@ async def test_migration_019( # but we will override them afterward to reproduce corrupted state. await test_group.delete(db=db) - async with db.start_session() as dbs: - async with dbs.start_transaction() as ts: - # Make relationship between CoreAccount <> group_member <> CoreStandardGroup active while it should have been deleted. - # and make the group_member <> CoreAccount edge part on global branch - - query = """ - MATCH (new_core_acc: CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) - MATCH (new_core_acc)-[r1:IS_RELATED]-(group_member: Relationship)-[r2:IS_RELATED]-(test_group: CoreStandardGroup) - MATCH (new_core_acc)-[active_r1]-(group_member) - MATCH (new_core_acc)-[deleted_r1]-(group_member) - MATCH (test_group)-[active_r2]-(group_member) - MATCH (test_group)-[deleted_r2]-(group_member) - - WHERE active_r1.status = 'active' AND deleted_r1.status = 'deleted' - AND active_r2.status = 'active' AND deleted_r2.status = 'deleted' - - DELETE deleted_r1 - REMOVE active_r1.to - SET active_r1.branch = '-global' - - DELETE deleted_r2 - REMOVE active_r2.to - - return new_core_acc, group_member, test_group - """ + # Make relationship between CoreAccount <> group_member <> CoreStandardGroup active while it should have been deleted. + # and make the group_member <> CoreAccount edge part on global branch + + query = """ + MATCH (new_core_acc: CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) + MATCH (new_core_acc)-[r1:IS_RELATED]-(group_member: Relationship)-[r2:IS_RELATED]-(test_group: CoreStandardGroup) + MATCH (new_core_acc)-[active_r1]-(group_member) + WHERE active_r1.status = 'active' + MATCH (new_core_acc)-[deleted_r1]-(group_member) + WHERE deleted_r1.status = 'deleted' + MATCH (test_group)-[active_r2]-(group_member) + WHERE active_r2.status = 'active' + MATCH (test_group)-[deleted_r2]-(group_member) + WHERE deleted_r2.status = 'deleted' + + DELETE deleted_r1 + REMOVE active_r1.to + SET active_r1.branch = '-global-' + + DELETE deleted_r2 + REMOVE active_r2.to + + return new_core_acc, group_member, test_group + """ - await ts.execute_query(query=query, name="query_1") + await db.execute_query(query=query, name="query_1") - # Create the old CoreAccount object - not inheriting from GenericAccount - - # sharing same attributes / relationships than above CoreAccount + # Create the old CoreAccount object - not inheriting from GenericAccount - + # sharing same attributes / relationships than above CoreAccount - query_2 = """ - // Match the existing CoreAccount node with the specified attributes - MATCH (new_core_acc:CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) + query_2 = """ + // Match the existing CoreAccount node with the specified attributes + MATCH (new_core_acc:CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) - // Create the new CoreAccount node with the same uuid and additional properties - CREATE (new_node:CoreAccount:LineageOwner:LineageSource:Node {uuid: new_core_acc.uuid, - branch_support: new_core_acc.branch_support, namespace: new_core_acc.namespace, kind: "CoreAccount"}) + // Create the new CoreAccount node with the same uuid and additional properties + CREATE (new_node:CoreAccount:LineageOwner:LineageSource:Node {uuid: new_core_acc.uuid, + branch_support: new_core_acc.branch_support, namespace: new_core_acc.namespace, kind: "CoreAccount"}) - WITH new_node, new_core_acc + WITH new_node, new_core_acc - // Match the relationships of the existing CoreAccount node - MATCH (new_core_acc)-[r:IS_RELATED]->(group_member:Relationship {name: "group_member"}) + // Match the relationships of the existing CoreAccount node + MATCH (new_core_acc)-[r:IS_RELATED]->(group_member:Relationship {name: "group_member"}) - // Create active branch with no to time on main branch - CREATE (new_node)-[:IS_RELATED {branch: "main", from: "2024-02-05T15:37:07.228145Z", status: "active"}]->(group_member) + // Create active branch with no to time on main branch + CREATE (new_node)-[:IS_RELATED {branch: "main", from: "2024-02-05T15:37:07.228145Z", status: "active"}]->(group_member) - // Create deleted branch with no to time on global branch - CREATE (new_node)-[:IS_RELATED {branch: $global_branch, from: r.from, status: "deleted"}]->(group_member) + // Create deleted branch with no to time on global branch + CREATE (new_node)-[:IS_RELATED {branch: $global_branch, from: r.from, status: "deleted"}]->(group_member) - // Return the new_node - RETURN new_node; - """ + // Return the new_node + RETURN new_node; + """ - await ts.execute_query(query=query_2, name="query_2", params={"global_branch": GLOBAL_BRANCH_NAME}) + await db.execute_query(query=query_2, name="query_2", params={"global_branch": GLOBAL_BRANCH_NAME}) # Make sure migration executes without error, and that we can query accounts afterwards. # Note generated corrupted state does not trigger IFC-1204 bug, @@ -97,3 +96,31 @@ async def test_migration_019( # Trigger e2e path to query this account core_acc = await client.get(kind="CoreAccount", id=core_acc.id, prefetch_relationships=True) assert core_acc.name.value == "core_acc" + + # Verify edges are correct, ie: + # - 2 edges on main branch between old CoreAccount and group_member, 1 active with from/to set, 1 deleted + # - 2 edges on main branch between new CoreAccount and group_member, 1 active with from/to set, 1 deleted + # - 2 edges on main branch between CoreStandardGroup and group_member, 1 active with from/to set, 1 deleted + + check_deleted_edges_query = """ + MATCH (n {uuid: $uuid})-[r1:IS_RELATED]-(rel:Relationship {name: 'group_member'}) + WHERE r1.branch = 'main' + WITH n, rel, COLLECT(r1) AS edges + WITH + edges, + SIZE(edges) AS edgeCount, + [e IN edges WHERE e.status = 'active' AND e.from IS NOT NULL AND e.to IS NOT NULL] AS activeEdges, + [e IN edges WHERE e.status = 'deleted'] AS deletedEdges + WHERE edgeCount = 2 AND SIZE(activeEdges) = 1 AND SIZE(deletedEdges) = 1 AND activeEdges[0].to = deletedEdges[0].from + RETURN 'Old CoreAccount edges are correct' AS result + """ + + core_accounts_results = await db.execute_query( + query=check_deleted_edges_query, name="check_deleted_edges_query", params={"uuid": core_acc.id} + ) + assert len(core_accounts_results) == 2 + + group_results = await db.execute_query( + query=check_deleted_edges_query, name="check_deleted_edges_query", params={"uuid": test_group.id} + ) + assert len(group_results) == 1 From 84aa6e2b7fc24137ff4b256685b9c7cfdaec6f0c Mon Sep 17 00:00:00 2001 From: Lucas Guillermou Date: Tue, 25 Feb 2025 18:03:49 +0100 Subject: [PATCH 6/6] Add aware node/relationship test --- .../graph/m019_restore_rels_to_time.py | 7 +- backend/tests/conftest.py | 14 +- .../branch/test_delete_agnostic_rel.py | 10 +- backend/tests/helpers/db_validation.py | 57 +-- .../unit/core/migrations/graph/test_019.py | 359 ++++++++++++------ .../schema/test_node_kind_update.py | 2 +- .../migrations/schema/test_node_remove.py | 2 +- 7 files changed, 298 insertions(+), 153 deletions(-) diff --git a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py index ffded6c6ab..cca0e5d1f1 100644 --- a/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py +++ b/backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py @@ -93,7 +93,10 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No // Note that if an AWARE node has been deleted on a branch and relationship is AGNOSTIC, we do not "delete" this relationship // right now as this aware node might exist on another branch. - // Set to time if there is an active edge on deleted edge branch + // Set to time if there is an active edge: + // - on deleted edge branch + // - or on any branch and deleted node is agnostic + // - or deleted node is aware and rel is agnostic CALL { WITH rel, deleted_edge OPTIONAL MATCH (rel)-[peer_active_edge {status: "active"}]-(peer_1) @@ -111,7 +114,7 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No deleted_node.branch_support as deleted_node_branch_support - // No need to check deleted edge branch because + // No need to check deleted edge branch because // If deleted_node has different branch support type (agnostic/aware) than rel type, // there might already be a deleted edge that we would not match if we filter on deleted_edge_branch. // If both are aware, it still works, as we would have one Relationship node for each branch on which this relationship exists. diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index e46e9eecbf..ef72412f25 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -969,14 +969,24 @@ def car_person_branch_agnostic_schema() -> dict[str, Any]: ], "relationships": [ { - "name": "owner", + "name": "agnostic_owner", "label": "Commander of Car", "peer": "TestPerson", "optional": False, - "kind": "Parent", "cardinality": "one", "direction": "outbound", "branch": BranchSupportType.AGNOSTIC.value, + "identifier": "agnostic_owner", + }, + { + "name": "aware_owner", + "label": "Commander of Car", + "peer": "TestPerson", + "optional": True, + "cardinality": "one", + "direction": "outbound", + "branch": BranchSupportType.AWARE.value, + "identifier": "aware_owner", }, ], }, diff --git a/backend/tests/functional/branch/test_delete_agnostic_rel.py b/backend/tests/functional/branch/test_delete_agnostic_rel.py index 6e2625889b..dade8f7bdb 100644 --- a/backend/tests/functional/branch/test_delete_agnostic_rel.py +++ b/backend/tests/functional/branch/test_delete_agnostic_rel.py @@ -31,13 +31,13 @@ async def owner_2(self, client: InfrahubClient, load_schema) -> InfrahubNode: @pytest.fixture(scope="class") async def car(self, client: InfrahubClient, load_schema, owner_1: InfrahubNode) -> InfrahubNode: - car = await client.create(kind="TestCar", name="car_name", owner=owner_1) + car = await client.create(kind="TestCar", name="car_name", agnostic_owner=owner_1) await car.save() return car @pytest.fixture(scope="class") async def car_2(self, client: InfrahubClient, load_schema, owner_2: InfrahubNode) -> InfrahubNode: - car = await client.create(kind="TestCar", name="car_name_2", owner=owner_2) + car = await client.create(kind="TestCar", name="car_name_2", agnostic_owner=owner_2) await car.save() return car @@ -64,11 +64,11 @@ async def test_delete_agnostic_rel( See https://github.com/opsmill/infrahub/issues/5559. """ car = await client.get(kind="TestCar", name__value="car_name", prefetch_relationships=True) - car.owner = owner_2 + car.agnostic_owner = owner_2 await car.save() car = await client.get(kind="TestCar", name__value="car_name", prefetch_relationships=True) - assert car.owner.peer.name.value == "owner_2" + assert car.agnostic_owner.peer.name.value == "owner_2" async def test_delete_aware_mandatory_node_blocked( self, client: InfrahubClient, owner_2: InfrahubNode, car: InfrahubNode @@ -79,7 +79,7 @@ async def test_delete_aware_mandatory_node_blocked( await owner_2.delete() assert ( - f"Cannot delete TestPerson '{owner_2.id}'. It is linked to mandatory relationship owner on node TestCar '{car.id}'" + f"Cannot delete TestPerson '{owner_2.id}'. It is linked to mandatory relationship agnostic_owner on node TestCar '{car.id}'" in exc.value.message ) diff --git a/backend/tests/helpers/db_validation.py b/backend/tests/helpers/db_validation.py index aa11638b4e..c9a62398c5 100644 --- a/backend/tests/helpers/db_validation.py +++ b/backend/tests/helpers/db_validation.py @@ -1,12 +1,26 @@ from typing import Any from infrahub.core.branch import Branch +from infrahub.core.constants import GLOBAL_BRANCH_NAME, BranchSupportType from infrahub.core.node import Node from infrahub.core.query import Query, QueryType from infrahub.database import InfrahubDatabase class ValidateNodeRelationshipQuery(Query): + """ + This query will return error message if for any couple (input_node, relationship): + - If relationship type is agnostic, all edges branches should be -global- + - Else, there should not be any edge on global branch + - Considering edges on the input branch: + - Either 1 active edge without `to` + - Either 1 deleted edge, and potentially 1 active edge having `active.to` = `deleted.from` + + NOTE: This query currently validates a subset of all possible valid edge states as edges states are mainly + validated on input branch. Having a validation on any branch would require more logic + (typically, a "groupby edge.branch" like behavior) and is TODO. + """ + name: str = "validate_node_rels" type: QueryType = QueryType.READ @@ -17,16 +31,20 @@ def __init__(self, node_id: str, **kwargs: Any) -> None: async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: self.params["node_id"] = self.node_id self.params["branch"] = self.branch.name + self.params["global_branch_name"] = GLOBAL_BRANCH_NAME + self.params["branch_agnostic"] = BranchSupportType.AGNOSTIC.value query = """ // Match the pattern with specific branch conditions - MATCH (node {uuid: $node_id})-[r:IS_RELATED]-(rel:Relationship) + MATCH (input_node {uuid: $node_id})-[r:IS_RELATED]-(rel:Relationship) + WITH DISTINCT rel + MATCH (rel)-[is_related:IS_RELATED]-(node: Node) // Collect and process edges WITH node, rel, - COLLECT(r) AS edges + COLLECT(is_related) AS edges // Count and categorize edges WITH @@ -34,7 +52,8 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: rel, edges, SIZE(edges) as nb_edges, - [e IN edges WHERE e.branch = $branch] AS edges_on_correct_branch, + [e IN edges WHERE e.branch = $branch] AS edges_on_branch, + [e IN edges WHERE e.branch = $global_branch_name] AS edges_on_global_branch, [e IN edges WHERE e.status = 'active' AND e.to IS NOT NULL] AS active_with_to_edges, [e IN edges WHERE e.status = 'active' AND e.to IS NULL] AS active_no_to_edges, [e IN edges WHERE e.status = 'deleted'] AS deleted_edges @@ -47,7 +66,8 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: active_no_to_edges, deleted_edges, nb_edges, - SIZE(edges_on_correct_branch) as nb_edges_on_correct_branch, + SIZE(edges_on_branch) as nb_edges_on_branch, + SIZE(edges_on_global_branch) as nb_edges_on_global_branch, SIZE(active_with_to_edges) AS nb_active_with_to_edges, SIZE(active_no_to_edges) AS nb_active_no_to_edges, SIZE(deleted_edges) AS nb_deleted_edges @@ -55,14 +75,16 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: // Return the result based on conditions WITH CASE - WHEN nb_edges_on_correct_branch <> nb_edges - THEN "nb_edges: " + nb_edges + " VS nb_edges_on_correct_branch: " + nb_edges_on_correct_branch - WHEN nb_edges = 1 AND nb_active_no_to_edges <> 1 - THEN "1 edge but nb_active_no_to_edges: " + nb_active_no_to_edges - WHEN nb_edges = 2 AND NOT (nb_active_with_to_edges = 1 AND nb_deleted_edges = 1 - AND active_with_to_edges[0].to = deleted_edges[0].from) - THEN "2 edges but they are invalid" - ELSE "Edges state is correct" + WHEN rel.branch_support = $branch_agnostic AND nb_edges_on_global_branch <> nb_edges + THEN "Relationship is agnostic but found: " + (nb_edges - nb_edges_on_global_branch) + " aware edge(s)" + WHEN rel.branch_support <> $branch_agnostic AND nb_edges_on_global_branch > 0 + THEN "Relationship is aware but found " + nb_edges_on_global_branch + " agnostic edge(s)" + WHEN nb_edges_on_branch > 2 + THEN "More than 2 edges on a given branch between a node and a relationship" + WHEN nb_edges_on_branch = 2 AND NOT (nb_active_with_to_edges = 1 AND nb_deleted_edges = 1 + AND active_with_to_edges[0].to = deleted_edges[0].from) + THEN "Found 2 inconsistent edges between a node and a relationship" + ELSE "Edges state is correct" // It currently allows having one edge as we might not always create a `deleted` edge? END AS res """ @@ -72,16 +94,7 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: async def validate_node_relationships(node: Node, branch: Branch, db: InfrahubDatabase) -> None: """ - This function will raise an error if following conditions are not met: - - All IS_RELATED edges between this node and Relationship nodes should be on input branch - - Between this node and any Relationship node, is expected: - - Either 1 active edge without `to` - - Or 1 active edge with `to` and 1 deleted edge with `active.to` = `deleted.from` - - IMPORTANT NOTE: This function currently validates a subset of all possible valid edge states. - Typically, if between two nodes there are 1 active edge on main and 1 deleted on branch2, - this function will raise an error while this state may happen while migrating an existing object node kind - on branch2, but not on main. + Raises an error if validation conditions of the query are not met. """ query = await ValidateNodeRelationshipQuery.init(db=db, branch=branch, node_id=node.id) diff --git a/backend/tests/unit/core/migrations/graph/test_019.py b/backend/tests/unit/core/migrations/graph/test_019.py index 4574f38383..813f7fa8d2 100644 --- a/backend/tests/unit/core/migrations/graph/test_019.py +++ b/backend/tests/unit/core/migrations/graph/test_019.py @@ -1,126 +1,245 @@ -from infrahub_sdk import InfrahubClient - +from infrahub.core import registry from infrahub.core.constants import GLOBAL_BRANCH_NAME from infrahub.core.migrations.graph import Migration019 from infrahub.core.node import Node +from infrahub.core.schema import SchemaRoot, core_models +from infrahub.core.timestamp import current_timestamp from infrahub.database import InfrahubDatabase -from tests.helpers.test_app import TestInfrahubApp +from tests.helpers.db_validation import validate_node_relationships + + +async def test_migration_019( + db: InfrahubDatabase, + default_branch, +): + """ + Reproduce corrupted state introduced by migration 12, and apply the migration fixing it. + """ + + schema = SchemaRoot(**core_models) + registry.schema.register_schema(schema=schema, branch=default_branch.name) + + test_group = await Node.init(db=db, schema="CoreStandardGroup") + await test_group.new(db=db, name="test_group") + await test_group.save(db=db) + + core_acc = await Node.init(db=db, schema="CoreAccount") + await core_acc.new(db=db, name="core_acc", account_type="User", password="def", member_of_groups=[test_group]) + await core_acc.save(db=db) + + # Delete CoreStandardGroup. This should also (correctly) update rels to CoreGenericAccount and CoreAccount + # but we will override them afterward to reproduce corrupted state. + await test_group.delete(db=db) + + # Make relationship between CoreAccount <> group_member <> CoreStandardGroup active while it should have been deleted. + # and make the group_member <> CoreAccount edge part on global branch + + query = """ + MATCH (new_core_acc: CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) + MATCH (new_core_acc)-[r1:IS_RELATED]-(group_member: Relationship)-[r2:IS_RELATED]-(test_group: CoreStandardGroup) + MATCH (new_core_acc)-[active_r1]-(group_member) + WHERE active_r1.status = 'active' + MATCH (new_core_acc)-[deleted_r1]-(group_member) + WHERE deleted_r1.status = 'deleted' + MATCH (test_group)-[active_r2]-(group_member) + WHERE active_r2.status = 'active' + MATCH (test_group)-[deleted_r2]-(group_member) + WHERE deleted_r2.status = 'deleted' + + DELETE deleted_r1 + REMOVE active_r1.to + SET active_r1.branch = '-global-' + + DELETE deleted_r2 + REMOVE active_r2.to + + return new_core_acc, group_member, test_group + """ + + await db.execute_query(query=query, name="query_1") + + # Create the old CoreAccount object - not inheriting from GenericAccount - + # sharing same attributes / relationships than above CoreAccount + + query_2 = """ + // Match the existing CoreAccount node with the specified attributes + MATCH (new_core_acc:CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) + + // Create the new CoreAccount node with the same uuid and additional properties + CREATE (new_node:CoreAccount:LineageOwner:LineageSource:Node {uuid: new_core_acc.uuid, + branch_support: new_core_acc.branch_support, namespace: new_core_acc.namespace, kind: "CoreAccount"}) + + WITH new_node, new_core_acc + + // Match the relationships of the existing CoreAccount node + MATCH (new_core_acc)-[r:IS_RELATED]->(group_member:Relationship {name: "group_member"}) + + // Create active branch with no to time on main branch + CREATE (new_node)-[:IS_RELATED {branch: "main", from: "2024-02-05T15:37:07.228145Z", status: "active"}]->(group_member) + + // Create deleted branch with no to time on global branch + CREATE (new_node)-[:IS_RELATED {branch: $global_branch, from: r.from, status: "deleted"}]->(group_member) + + // Return the new_node + RETURN new_node; + """ + + await db.execute_query(query=query_2, name="query_2", params={"global_branch": GLOBAL_BRANCH_NAME}) + + # Make sure migration executes without error, and that we can query accounts afterwards. + # Note generated corrupted state does not trigger IFC-1204 bug, + # but a manual test confirmed migration solves this issue. + + migration = Migration019() + await migration.execute(db=db) + await migration.validate_migration(db=db) + + # Verify edges are correct, ie: + # - 2 edges on main branch between old CoreAccount and group_member, 1 active with from/to set, 1 deleted + # - 2 edges on main branch between new CoreAccount and group_member, 1 active with from/to set, 1 deleted + # - 2 edges on main branch between CoreStandardGroup and group_member, 1 active with from/to set, 1 deleted + + check_deleted_edges_query = """ + MATCH (n {uuid: $uuid})-[r1:IS_RELATED]-(rel:Relationship {name: 'group_member'}) + WHERE r1.branch = 'main' + WITH n, rel, COLLECT(r1) AS edges + WITH + edges, + SIZE(edges) AS edgeCount, + [e IN edges WHERE e.status = 'active' AND e.from IS NOT NULL AND e.to IS NOT NULL] AS activeEdges, + [e IN edges WHERE e.status = 'deleted'] AS deletedEdges + WHERE edgeCount = 2 AND SIZE(activeEdges) = 1 AND SIZE(deletedEdges) = 1 AND activeEdges[0].to = deletedEdges[0].from + RETURN 'Old CoreAccount edges are correct' AS result + """ + + core_accounts_results = await db.execute_query( + query=check_deleted_edges_query, name="check_deleted_edges_query", params={"uuid": core_acc.id} + ) + assert len(core_accounts_results) == 2 + + group_results = await db.execute_query( + query=check_deleted_edges_query, name="check_deleted_edges_query", params={"uuid": test_group.id} + ) + assert len(group_results) == 1 + + # Additional sanity checks + await validate_node_relationships(node=test_group, branch=default_branch, db=db) + await validate_node_relationships(node=test_group, branch=registry.get_global_branch(), db=db) + + +async def test_incorrectly_deleted_aware_nodes_and_relationship( + db: InfrahubDatabase, branch, car_person_schema_unregistered +): + """ + Reproduce a state where a branch aware node would have been incorrectly deleted, this node being + connected to another node through a branch aware relationship. + """ + + registry.schema.register_schema(schema=car_person_schema_unregistered, branch=branch.name) + + john = await Node.init(schema="TestPerson", db=db, branch=branch) + await john.new(db=db, name="John") + await john.save(db=db) + + car = await Node.init(schema="TestCar", db=db, branch=branch) + await car.new(db=db, name="test-car", owner=john) + await car.save(db=db) + + # Reproduce corrupted state by only deleting is_part_of edge + + delete_only_is_part_of_query = """ + MATCH (car:TestCar {uuid: $uuid})-[r:IS_PART_OF {status: "active"}]-(root:Root) + SET r.to = $at + CREATE (car)-[:IS_PART_OF {status: "deleted", from: $at, branch: $branch, branch_level: 2}]->(root) + + """ + + await db.execute_query( + query=delete_only_is_part_of_query, + name="delete_only_is_part_of_query", + params={"uuid": car.id, "at": current_timestamp(), "branch": branch.name}, + ) + + migration = Migration019() + await migration.execute(db=db) + await migration.validate_migration(db=db) + + await validate_node_relationships(node=car, branch=branch, db=db) + + +async def test_incorrectly_deleted_agnostic_node(db: InfrahubDatabase, branch, car_person_branch_agnostic_schema): + """ + Reproduce a state where a branch agnostic node would have been incorrectly deleted, this node being + connected to another node through 2 relationships, both aware and agnostic. + """ + + # await load_schema(db, schema=CAR_SCHEMA) + registry.schema.register_schema(schema=SchemaRoot(**car_person_branch_agnostic_schema), branch=branch.name) + + aware_person = await Node.init(schema="TestPerson", db=db, branch=branch) + await aware_person.new(db=db, name="John") + await aware_person.save(db=db) + + agnostic_car = await Node.init(schema="TestCar", db=db, branch=branch) + await agnostic_car.new(db=db, name="test-car", agnostic_owner=aware_person, aware_owner=aware_person) + await agnostic_car.save(db=db) + + # Reproduce corrupted state by only deleting is_part_of edge + + delete_only_is_part_of_query = """ + MATCH (car:TestCar {uuid: $uuid})-[r:IS_PART_OF {status: "active"}]-(root:Root) + SET r.to = $at + CREATE (car)-[:IS_PART_OF {status: "deleted", from: $at, branch: $global_branch, branch_level: 1}]->(root) + """ + + await db.execute_query( + query=delete_only_is_part_of_query, + name="delete_only_is_part_of_query", + params={"uuid": agnostic_car.id, "at": current_timestamp(), "global_branch": GLOBAL_BRANCH_NAME}, + ) + + migration = Migration019() + await migration.execute(db=db) + await migration.validate_migration(db=db) + + await validate_node_relationships(node=agnostic_car, branch=branch, db=db) + await validate_node_relationships(node=agnostic_car, branch=registry.get_global_branch(), db=db) + + +async def test_incorrectly_deleted_aware_node(db: InfrahubDatabase, branch, car_person_branch_agnostic_schema): + """ + Reproduce a state where a branch agnostic node would have been incorrectly deleted, this node being + connected to another node through 2 relationships, both aware and agnostic. + Note that, after deleting an aware node, agnostic edges of this node will not be deleted. + """ + + registry.schema.register_schema(schema=SchemaRoot(**car_person_branch_agnostic_schema), branch=branch.name) + + aware_person = await Node.init(schema="TestPerson", db=db, branch=branch) + await aware_person.new(db=db, name="John") + await aware_person.save(db=db) + + agnostic_car = await Node.init(schema="TestCar", db=db, branch=branch) + await agnostic_car.new(db=db, name="test-car", agnostic_owner=aware_person, aware_owner=aware_person) + await agnostic_car.save(db=db) + + # Reproduce corrupted state by only deleting is_part_of edge + + delete_only_is_part_of_query = """ + MATCH (person:TestPerson {uuid: $uuid})-[r:IS_PART_OF {status: "active"}]-(root:Root) + SET r.to = $at + CREATE (person)-[:IS_PART_OF {status: "deleted", from: $at, branch: $branch, branch_level: 1}]->(root) + """ + + await db.execute_query( + query=delete_only_is_part_of_query, + name="delete_only_is_part_of_query", + params={"uuid": aware_person.id, "at": current_timestamp(), "branch": branch.name}, + ) + migration = Migration019() + await migration.execute(db=db) + await migration.validate_migration(db=db) -class TestMigration019(TestInfrahubApp): - async def test_migration_019( - self, - client: InfrahubClient, - db: InfrahubDatabase, - default_branch, - ): - """ - Reproduce corrupted state introduced by migration 12, and apply the migration fixing it. - """ - - test_group = await Node.init(db=db, schema="CoreStandardGroup") - await test_group.new(db=db, name="test_group") - await test_group.save(db=db) - - core_acc = await Node.init(db=db, schema="CoreAccount") - await core_acc.new(db=db, name="core_acc", account_type="User", password="def", member_of_groups=[test_group]) - await core_acc.save(db=db) - - # Delete CoreStandardGroup. This should also (correctly) update rels to CoreGenericAccount and CoreAccount - # but we will override them afterward to reproduce corrupted state. - await test_group.delete(db=db) - - # Make relationship between CoreAccount <> group_member <> CoreStandardGroup active while it should have been deleted. - # and make the group_member <> CoreAccount edge part on global branch - - query = """ - MATCH (new_core_acc: CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) - MATCH (new_core_acc)-[r1:IS_RELATED]-(group_member: Relationship)-[r2:IS_RELATED]-(test_group: CoreStandardGroup) - MATCH (new_core_acc)-[active_r1]-(group_member) - WHERE active_r1.status = 'active' - MATCH (new_core_acc)-[deleted_r1]-(group_member) - WHERE deleted_r1.status = 'deleted' - MATCH (test_group)-[active_r2]-(group_member) - WHERE active_r2.status = 'active' - MATCH (test_group)-[deleted_r2]-(group_member) - WHERE deleted_r2.status = 'deleted' - - DELETE deleted_r1 - REMOVE active_r1.to - SET active_r1.branch = '-global-' - - DELETE deleted_r2 - REMOVE active_r2.to - - return new_core_acc, group_member, test_group - """ - - await db.execute_query(query=query, name="query_1") - - # Create the old CoreAccount object - not inheriting from GenericAccount - - # sharing same attributes / relationships than above CoreAccount - - query_2 = """ - // Match the existing CoreAccount node with the specified attributes - MATCH (new_core_acc:CoreAccount)-[:HAS_ATTRIBUTE]->(:Attribute {name: "name"})-[:HAS_VALUE]->(:AttributeValue {value: "core_acc"}) - - // Create the new CoreAccount node with the same uuid and additional properties - CREATE (new_node:CoreAccount:LineageOwner:LineageSource:Node {uuid: new_core_acc.uuid, - branch_support: new_core_acc.branch_support, namespace: new_core_acc.namespace, kind: "CoreAccount"}) - - WITH new_node, new_core_acc - - // Match the relationships of the existing CoreAccount node - MATCH (new_core_acc)-[r:IS_RELATED]->(group_member:Relationship {name: "group_member"}) - - // Create active branch with no to time on main branch - CREATE (new_node)-[:IS_RELATED {branch: "main", from: "2024-02-05T15:37:07.228145Z", status: "active"}]->(group_member) - - // Create deleted branch with no to time on global branch - CREATE (new_node)-[:IS_RELATED {branch: $global_branch, from: r.from, status: "deleted"}]->(group_member) - - // Return the new_node - RETURN new_node; - """ - - await db.execute_query(query=query_2, name="query_2", params={"global_branch": GLOBAL_BRANCH_NAME}) - - # Make sure migration executes without error, and that we can query accounts afterwards. - # Note generated corrupted state does not trigger IFC-1204 bug, - # but a manual test confirmed migration solves this issue. - - migration = Migration019() - await migration.execute(db=db) - await migration.validate_migration(db=db) - - # Trigger e2e path to query this account - core_acc = await client.get(kind="CoreAccount", id=core_acc.id, prefetch_relationships=True) - assert core_acc.name.value == "core_acc" - - # Verify edges are correct, ie: - # - 2 edges on main branch between old CoreAccount and group_member, 1 active with from/to set, 1 deleted - # - 2 edges on main branch between new CoreAccount and group_member, 1 active with from/to set, 1 deleted - # - 2 edges on main branch between CoreStandardGroup and group_member, 1 active with from/to set, 1 deleted - - check_deleted_edges_query = """ - MATCH (n {uuid: $uuid})-[r1:IS_RELATED]-(rel:Relationship {name: 'group_member'}) - WHERE r1.branch = 'main' - WITH n, rel, COLLECT(r1) AS edges - WITH - edges, - SIZE(edges) AS edgeCount, - [e IN edges WHERE e.status = 'active' AND e.from IS NOT NULL AND e.to IS NOT NULL] AS activeEdges, - [e IN edges WHERE e.status = 'deleted'] AS deletedEdges - WHERE edgeCount = 2 AND SIZE(activeEdges) = 1 AND SIZE(deletedEdges) = 1 AND activeEdges[0].to = deletedEdges[0].from - RETURN 'Old CoreAccount edges are correct' AS result - """ - - core_accounts_results = await db.execute_query( - query=check_deleted_edges_query, name="check_deleted_edges_query", params={"uuid": core_acc.id} - ) - assert len(core_accounts_results) == 2 - - group_results = await db.execute_query( - query=check_deleted_edges_query, name="check_deleted_edges_query", params={"uuid": test_group.id} - ) - assert len(group_results) == 1 + await validate_node_relationships(node=aware_person, branch=branch, db=db) + await validate_node_relationships(node=aware_person, branch=registry.get_global_branch(), db=db) diff --git a/backend/tests/unit/core/migrations/schema/test_node_kind_update.py b/backend/tests/unit/core/migrations/schema/test_node_kind_update.py index 67e81d6867..218f5d98ca 100644 --- a/backend/tests/unit/core/migrations/schema/test_node_kind_update.py +++ b/backend/tests/unit/core/migrations/schema/test_node_kind_update.py @@ -99,7 +99,7 @@ async def test_migration_agnostic_relationship( await person_john.save(db=db) car = await Node.init(db=db, schema="TestCar") - await car.new(db=db, name="yaris", owner=person_john.id) + await car.new(db=db, name="yaris", agnostic_owner=person_john.id) await car.save(db=db) schema = registry.schema.get_schema_branch(name=default_branch.name) diff --git a/backend/tests/unit/core/migrations/schema/test_node_remove.py b/backend/tests/unit/core/migrations/schema/test_node_remove.py index a63b21c2b0..1af390fa2c 100644 --- a/backend/tests/unit/core/migrations/schema/test_node_remove.py +++ b/backend/tests/unit/core/migrations/schema/test_node_remove.py @@ -115,7 +115,7 @@ async def test_migration_agnostic_relationship( await person_john.save(db=db) car = await Node.init(db=db, schema="TestCar") - await car.new(db=db, name="yaris", owner=person_john.id) + await car.new(db=db, name="yaris", agnostic_owner=person_john.id) await car.save(db=db) schema = registry.schema.get_schema_branch(name=default_branch.name)