Add migration to fix deleted edges branches

Add migration to restore  time

backend/infrahub/core/graph/__init__.py

@@ -1 +1 @@
-GRAPH_VERSION = 18
+GRAPH_VERSION = 19

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,
 ]
 
 

backend/infrahub/core/migrations/graph/m019_restore_rels_to_time.py

@@ -0,0 +1,78 @@
+from __future__ import annotations
+
+from typing import TYPE_CHECKING, Sequence
+
+from infrahub.core.migrations.shared import MigrationResult
+from infrahub.log import get_logger
+
+from ..shared import InternalSchemaMigration, SchemaMigration
+
+if TYPE_CHECKING:
+    from infrahub.database import InfrahubDatabase
+
+log = get_logger()
+
+
+class Migration019(InternalSchemaMigration):
+    name: str = "019_restore_relationships_to_time"
+    minimum_version: int = 18
+    migrations: Sequence[SchemaMigration] = []
+
+    async def validate_migration(self, db: InfrahubDatabase) -> MigrationResult:
+        result = MigrationResult()
+
+        return result
+
+    async def execute(self, db: InfrahubDatabase) -> MigrationResult:
+        """
+        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 new CoreAccount node <> group_member node and CoreStandardGroup node <> group_member node 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.
+        """
+
+        query = """
+            // HAS_ATTRIBUTE links are fine because the notion of "aware" or "agnostic" is only for relationships
+            MATCH (a: CoreAccount)-[r:IS_RELATED|IS_PROTECTED|IS_VISIBLE]-(rel: Relationship)-[r3]-(c:CoreGenericAccount)
+            WITH a, rel, COUNT(rel) AS rel_count, c, r3
+            WHERE rel_count = 2 and rel.branch_support = 'aware'
+            MATCH (a)-[r1]->(rel)
+            MATCH (a)-[r2]->(rel)
+            WHERE r1.status = 'deleted' AND r1.branch = '-global-'
+            AND r2.status <> 'deleted' AND r2.branch <> '-global-' AND r2.to IS NULL
+            AND c <> a AND r3.branch = '-global-'
+
+            // Fixing missing `to` on non-global edge
+            SET r2.to = r1.from
+
+            // Fix branch of the new edge
+            SET r3.branch = r2.branch
+
+            // Remove `deleted` edge as it is not on main branch.
+            DELETE r1
+
+            WITH a, c, rel, r3
+            OPTIONAL MATCH (rel)-[r4:IS_RELATED]-(core_standard_group: CoreStandardGroup)-[is_part_of: IS_PART_OF]->(root: Root)
+
+            WHERE is_part_of.to IS NOT NULL
+            AND r3.to IS NULL AND r3.status = 'active' AND is_part_of.branch = r3.branch
+            AND r4.to IS NULL AND r4.status = 'active' AND is_part_of.branch = r4.branch
+            SET r3.to = is_part_of.to
+            SET r4.to = is_part_of.to
+            RETURN a, c, rel, core_standard_group, root
+            // maybe make sure to time is higher than from time of r3/r4 but if it was not the case if means CoreStdGroup would have been deleted
+            // beore kind migrating and thus we would not match all of these anyway
+
+        """
+
+        await db.execute_query(query=query, name="update_relationships_to")
+
+        return MigrationResult()

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(

backend/infrahub/core/schema/definitions/core.py

@@ -223,6 +223,7 @@
                     "optional": True,
                     "identifier": "group_member",
                     "cardinality": "many",
+                    "branch": BranchSupportType.AWARE,
                 },
                 {
                     "name": "subscribers",

backend/tests/unit/core/migrations/graph/test_019.py

@@ -0,0 +1,98 @@
+from infrahub_sdk import InfrahubClient
+
+from infrahub.core.migrations.graph.m019_restore_rels_to_time 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-", from: r.from, status: "deleted"}]->(group_member)
+
+                // Return the new_node
+                RETURN new_node;
+                """
+
+                await ts.execute_query(query=query_2, name="query_2")
+
+        # 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"