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..1d7a0ebb99 --- /dev/null +++ b/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() 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..f8bc044b65 --- /dev/null +++ b/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"