Skip to content

Commit 5d4efad

Browse files
ajtmccartyajtmccarty
authored
fix attribute add migration for deleted nodes (#6569)
* fix attribute add migration for deleted nodes * add some database validation to the end of every schema integration test * update one more test * unit test cleanup * add migration and a unit test * add changelog * remove comment, add line to test
1 parent 481b8b1 commit 5d4efad

23 files changed

+467
-13
lines changed

‎backend/infrahub/core/graph/__init__.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
GRAPH_VERSION = 29
1+
GRAPH_VERSION = 30

‎backend/infrahub/core/migrations/graph/__init__.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
from .m027_delete_isolated_nodes import Migration027
3232
from .m028_delete_diffs import Migration028
3333
from .m029_duplicates_cleanup import Migration029
34+
from .m030_illegal_edges import Migration030
3435

3536
if TYPE_CHECKING:
3637
from infrahub.core.root import Root
@@ -67,6 +68,7 @@
6768
Migration027,
6869
Migration028,
6970
Migration029,
71+
Migration030,
7072
]
7173

7274

‎backend/infrahub/core/migrations/graph/m030_illegal_edges.py‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
from __future__ import annotations
2+
3+
from typing import TYPE_CHECKING, Any, Sequence
4+
5+
from infrahub.core.migrations.shared import GraphMigration, MigrationResult
6+
from infrahub.log import get_logger
7+
8+
from ...query import Query, QueryType
9+
10+
if TYPE_CHECKING:
11+
from infrahub.database import InfrahubDatabase
12+
13+
log = get_logger()
14+
15+
16+
class DeletePosthumousEdges(Query):
17+
name = "delete_posthumous_edges_query"
18+
type = QueryType.WRITE
19+
insert_return = False
20+
21+
async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> None: # noqa: ARG002
22+
query = """
23+
// ------------
24+
// find deleted nodes
25+
// ------------
26+
MATCH (n:Node)-[e:IS_PART_OF]->(:Root)
27+
WHERE e.status = "deleted" OR e.to IS NOT NULL
28+
WITH DISTINCT n, e.branch AS delete_branch, e.branch_level AS delete_branch_level, CASE
29+
WHEN e.status = "deleted" THEN e.from
30+
ELSE e.to
31+
END AS delete_time
32+
// ------------
33+
// find the edges added to the deleted node after the delete time
34+
// ------------
35+
MATCH (n)-[added_e]-(peer)
36+
WHERE added_e.from > delete_time
37+
AND type(added_e) <> "IS_PART_OF"
38+
// if the node was deleted on a branch (delete_branch_level > 1), and then updated on main/global (added_e.branch_level = 1), we can ignore it
39+
AND added_e.branch_level >= delete_branch_level
40+
AND (added_e.branch = delete_branch OR delete_branch_level = 1)
41+
WITH DISTINCT n, delete_branch, delete_time, added_e, peer
42+
// ------------
43+
// get the branched_from for the branch on which the node was deleted
44+
// ------------
45+
CALL {
46+
WITH added_e
47+
MATCH (b:Branch {name: added_e.branch})
48+
RETURN b.branched_from AS added_e_branched_from
49+
}
50+
// ------------
51+
// account for the following situations, given that the edge update time is after the node delete time
52+
// - deleted on main/global, updated on branch
53+
// - illegal if the delete is before branch.branched_from
54+
// - deleted on branch, updated on branch
55+
// - illegal
56+
// ------------
57+
WITH n, delete_branch, delete_time, added_e, peer
58+
WHERE delete_branch = added_e.branch
59+
OR delete_time < added_e_branched_from
60+
DELETE added_e
61+
// --------------
62+
// the peer _should_ only be an Attribute, but I want to make sure we don't
63+
// inadvertently delete Root or an AttributeValue or a Boolean
64+
// --------------
65+
WITH peer
66+
WHERE "Attribute" IN labels(peer)
67+
DETACH DELETE peer
68+
"""
69+
self.add_to_query(query)
70+
71+
72+
class Migration030(GraphMigration):
73+
"""
74+
Edges could have been added to Nodes after the Node was deleted, so we need to hard-delete those illegal edges
75+
"""
76+
77+
name: str = "030_delete_illegal_edges"
78+
minimum_version: int = 29
79+
queries: Sequence[type[Query]] = [DeletePosthumousEdges]
80+
81+
async def validate_migration(self, db: InfrahubDatabase) -> MigrationResult: # noqa: ARG002
82+
result = MigrationResult()
83+
return result

‎backend/infrahub/core/migrations/query/attribute_add.py‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -63,28 +63,32 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: dict[str, Any]) -> No
6363
MATCH p = (n:%(node_kind)s)
6464
CALL {
6565
WITH n
66-
MATCH (root:Root)<-[r1:IS_PART_OF]-(n)
67-
OPTIONAL MATCH (n)-[r2:HAS_ATTRIBUTE]-(:Attribute { name: $attr_name })
68-
WHERE all(r in [r1, r2] WHERE (%(branch_filter)s))
69-
RETURN n as n1, r1 as r11, r2 as r12
70-
ORDER BY r2.branch_level DESC, r2.from ASC, r1.branch_level DESC, r1.from ASC
66+
MATCH (:Root)<-[r:IS_PART_OF]-(n)
67+
WHERE %(branch_filter)s
68+
WITH n, r AS is_part_of_e
69+
OPTIONAL MATCH (n)-[r:HAS_ATTRIBUTE]-(:Attribute { name: $attr_name })
70+
WHERE %(branch_filter)s
71+
WITH is_part_of_e, r AS has_attr_e
72+
RETURN is_part_of_e, has_attr_e
73+
ORDER BY has_attr_e.branch_level DESC, has_attr_e.from ASC, is_part_of_e.branch_level DESC, is_part_of_e.from ASC
7174
LIMIT 1
7275
}
73-
WITH n1 as n, r11 as r1, r12 as r2, av, is_protected_value, is_visible_value
74-
WHERE r1.status = "active" AND (r2 IS NULL OR r2.status = "deleted")
76+
WITH n, is_part_of_e, has_attr_e, av, is_protected_value, is_visible_value
77+
WHERE is_part_of_e.status = "active" AND (has_attr_e IS NULL OR has_attr_e.status = "deleted")
7578
CREATE (a:Attribute { name: $attr_name, branch_support: $branch_support })
7679
CREATE (n)-[:HAS_ATTRIBUTE $rel_props ]->(a)
7780
CREATE (a)-[:HAS_VALUE $rel_props ]->(av)
7881
CREATE (a)-[:IS_PROTECTED $rel_props]->(is_protected_value)
7982
CREATE (a)-[:IS_VISIBLE $rel_props]->(is_visible_value)
8083
%(uuid_generation)s
81-
FOREACH (i in CASE WHEN r2.status = "deleted" THEN [1] ELSE [] END |
82-
SET r2.to = $current_time
84+
FOREACH (i in CASE WHEN has_attr_e.status = "deleted" THEN [1] ELSE [] END |
85+
SET has_attr_e.to = $current_time
8386
)
8487
""" % {
8588
"branch_filter": branch_filter,
8689
"node_kind": self.node_kind,
8790
"uuid_generation": db.render_uuid_generation(node_label="a", node_attr="uuid"),
8891
}
92+
8993
self.add_to_query(query)
9094
self.return_labels = ["n.uuid", "a.uuid"]

‎backend/infrahub/database/validation.py‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,3 +36,66 @@ async def verify_no_duplicate_relationships(db: InfrahubDatabase) -> None:
3636
f"{num_duplicates} duplicate relationships ({branch=},{direction=}) between nodes '{node_id1}' and '{node_id2}'"
3737
f" with relationship name '{rel_name}'"
3838
)
39+
40+
41+
async def verify_no_edges_added_after_node_delete(db: InfrahubDatabase) -> None:
42+
"""
43+
Verify that no edges are added to a Node after it is deleted on a given branch
44+
"""
45+
query = """
46+
// ------------
47+
// find deleted nodes
48+
// ------------
49+
MATCH (n:Node)-[e:IS_PART_OF]->(:Root)
50+
WHERE e.status = "deleted" OR e.to IS NOT NULL
51+
WITH DISTINCT n, e.branch AS delete_branch, e.branch_level AS delete_branch_level, CASE
52+
WHEN e.status = "deleted" THEN e.from
53+
ELSE e.to
54+
END AS delete_time
55+
// ------------
56+
// find the edges added to the deleted node after the delete time
57+
// ------------
58+
MATCH (n)-[added_e]-(peer)
59+
WHERE added_e.from > delete_time
60+
AND type(added_e) <> "IS_PART_OF"
61+
// if the node was deleted on a branch (delete_branch_level > 1), and then updated on main/global (added_e.branch_level = 1), we can ignore it
62+
AND added_e.branch_level >= delete_branch_level
63+
AND (added_e.branch = delete_branch OR delete_branch_level = 1)
64+
WITH DISTINCT n, delete_branch, delete_time, added_e, peer AS added_peer
65+
// ------------
66+
// get the branched_from for the branch on which the node was deleted
67+
// ------------
68+
CALL {
69+
WITH added_e
70+
MATCH (b:Branch {name: added_e.branch})
71+
RETURN b.branched_from AS added_e_branched_from
72+
}
73+
// ------------
74+
// account for the following situations, given that the edge update time is after the node delete time
75+
// - deleted on main/global, updated on branch
76+
// - illegal if the delete is before branch.branched_from
77+
// - deleted on branch, updated on branch
78+
// - illegal
79+
// ------------
80+
WITH n, delete_branch, delete_time, added_e, added_peer
81+
WHERE delete_branch = added_e.branch
82+
OR delete_time < added_e_branched_from
83+
RETURN n.uuid AS n_uuid, delete_branch, delete_time, added_e, added_peer
84+
"""
85+
results = await db.execute_query(query=query)
86+
error_messages = []
87+
for result in results:
88+
n_uuid = result.get("n_uuid")
89+
delete_branch = result.get("delete_branch")
90+
delete_time = result.get("delete_time")
91+
added_e = result.get("added_e")
92+
added_e_branch = added_e.get("branch")
93+
added_e_from = added_e.get("from")
94+
added_peer = result.get("added_peer")
95+
message = (
96+
f"Node {n_uuid} was deleted on {delete_branch} at {delete_time} but has an {added_e.type} edge added on"
97+
f" branch {added_e_branch} at {added_e_from} to {added_peer.element_id}"
98+
)
99+
error_messages.append(message)
100+
if error_messages:
101+
raise ValueError(error_messages)

‎backend/tests/integration/schema_lifecycle/test_generic_migrations.py‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from infrahub.core.relationship.model import RelationshipManager
1313
from infrahub.core.schema.node_schema import NodeSchema
1414
from infrahub.database import InfrahubDatabase
15+
from infrahub.database.validation import verify_no_duplicate_relationships, verify_no_edges_added_after_node_delete
1516

1617
from ..shared import load_schema
1718
from .shared import TestSchemaLifecycleBase
@@ -137,14 +138,35 @@ async def initial_objects(
137138
await specific_one.new(db=db, generic_attr_text="Alpha", generic_attr_num=1, favorite_thing=thing_one)
138139
await specific_one.save(db=db)
139140

141+
deleted_specific_one = await Node.init(schema=SPECIFIC_ONE_KIND, db=db)
142+
await deleted_specific_one.new(
143+
db=db, generic_attr_text="Deleted-Alpha", generic_attr_num=-1, favorite_thing=thing_one
144+
)
145+
await deleted_specific_one.save(db=db)
146+
await deleted_specific_one.delete(db=db)
147+
140148
specific_two = await Node.init(schema=SPECIFIC_TWO_KIND, db=db)
141149
await specific_two.new(db=db, generic_attr_text="Bravo", generic_attr_num=2, favorite_thing=thing_two)
142150
await specific_two.save(db=db)
143151

152+
deleted_specific_two = await Node.init(schema=SPECIFIC_TWO_KIND, db=db)
153+
await deleted_specific_two.new(
154+
db=db, generic_attr_text="Deleted-Bravo", generic_attr_num=-2, favorite_thing=thing_two
155+
)
156+
await deleted_specific_two.save(db=db)
157+
await deleted_specific_two.delete(db=db)
158+
144159
specific_three = await Node.init(schema=SPECIFIC_THREE_KIND, db=db)
145160
await specific_three.new(db=db, generic_attr_text="Charlie", generic_attr_num=3, favorite_thing=thing_three)
146161
await specific_three.save(db=db)
147162

163+
deleted_specific_three = await Node.init(schema=SPECIFIC_THREE_KIND, db=db)
164+
await deleted_specific_three.new(
165+
db=db, generic_attr_text="Deleted-Charlie", generic_attr_num=-3, favorite_thing=thing_three
166+
)
167+
await deleted_specific_three.save(db=db)
168+
await deleted_specific_three.delete(db=db)
169+
148170
objs = {
149171
"thing_one": thing_one,
150172
"thing_two": thing_two,
@@ -1164,6 +1186,10 @@ async def test_step06_load_schema_with_override_deletes(
11641186
)
11651187
assert not errors
11661188

1189+
async def test_final_validate(self, db: InfrahubDatabase):
1190+
await verify_no_duplicate_relationships(db=db)
1191+
await verify_no_edges_added_after_node_delete(db=db)
1192+
11671193

11681194
class TestSchemaLifecycleGenericUpdates(SchemaLifecycleGenericBase):
11691195
async def validate_database(

‎backend/tests/integration/schema_lifecycle/test_migration_attribute_branch.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
)
1313
from infrahub.core.node import Node
1414
from infrahub.database import InfrahubDatabase
15+
from infrahub.database.validation import verify_no_duplicate_relationships, verify_no_edges_added_after_node_delete
1516
from infrahub.exceptions import InitializationError
1617

1718
from ..shared import load_schema
@@ -56,6 +57,11 @@ async def initial_dataset(self, db: InfrahubDatabase, initialize_registry, schem
5657
await john.new(db=db, name="John", height=175, description="The famous Joe Doe")
5758
await john.save(db=db)
5859

60+
deleted_bob = await Node.init(schema=PERSON_KIND, db=db)
61+
await deleted_bob.new(db=db, name="Deleted Bob", height=175, description="He's not here")
62+
await deleted_bob.save(db=db)
63+
await deleted_bob.delete(db=db)
64+
5965
renault = await Node.init(schema=MANUFACTURER_KIND_01, db=db)
6066
await renault.new(
6167
db=db, name="renault", description="Groupe Renault is a French multinational automobile manufacturer"
@@ -87,6 +93,11 @@ async def initial_dataset(self, db: InfrahubDatabase, initialize_registry, schem
8793
await richard.new(db=db, name="Richard", height=180, description="The less famous Richard Doe")
8894
await richard.save(db=db)
8995

96+
deleted_chuck = await Node.init(schema=PERSON_KIND, db=db, branch=branch1)
97+
await deleted_chuck.new(db=db, name="Deleted Chuck", height=175, description="He's not here")
98+
await deleted_chuck.save(db=db)
99+
await deleted_chuck.delete(db=db)
100+
90101
mercedes = await Node.init(schema=MANUFACTURER_KIND_01, db=db, branch=branch1)
91102
await mercedes.new(
92103
db=db, name="mercedes", description="Mercedes-Benz, commonly referred to as Mercedes and sometimes as Benz"
@@ -128,8 +139,10 @@ async def initial_dataset(self, db: InfrahubDatabase, initialize_registry, schem
128139

129140
objs = {
130141
"john": john.id,
142+
"deleted_bob": deleted_bob.id,
131143
"jane": jane.id,
132144
"richard": richard.id,
145+
"deleted_chuck": deleted_chuck.id,
133146
"honda": honda.id,
134147
"renault": renault.id,
135148
"mercedes": mercedes.id,
@@ -325,6 +338,8 @@ async def test_rebase(self, db: InfrahubDatabase, client: InfrahubClient, initia
325338
assert {"name": "firstname", "value": "Jane"} in janes_event["attributes"]
326339
assert {"name": "description", "value": "The famous Jane Doe"} in janes_event["attributes"]
327340

341+
await verify_no_edges_added_after_node_delete(db=db)
342+
328343
async def test_merge(self, db: InfrahubDatabase, client: InfrahubClient, initial_dataset):
329344
branch = await client.branch.merge(branch_name=self.branch1.name)
330345
assert branch
@@ -346,6 +361,10 @@ async def test_merge(self, db: InfrahubDatabase, client: InfrahubClient, initial
346361
assert not hasattr(jane, "height")
347362
assert not hasattr(jane, "name")
348363

364+
async def test_final_validate(self, db: InfrahubDatabase):
365+
await verify_no_duplicate_relationships(db=db)
366+
await verify_no_edges_added_after_node_delete(db=db)
367+
349368

350369
QUERY_EVENT = """
351370
query(

‎backend/tests/integration/schema_lifecycle/test_migration_generic_renaming.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
from infrahub.core.schema.generic_schema import GenericSchema
99
from infrahub.core.schema.node_schema import NodeSchema
1010
from infrahub.database import InfrahubDatabase
11+
from infrahub.database.validation import verify_no_duplicate_relationships, verify_no_edges_added_after_node_delete
1112
from tests.helpers.schema import load_schema
1213
from tests.integration.schema_lifecycle.shared import TestSchemaLifecycleBase
1314

@@ -51,6 +52,11 @@ async def initial_dataset(
5152
await second_device.new(db=db, name="Test Device 02", role="Provider Edge")
5253
await second_device.save(db=db)
5354

55+
deleted_device = await Node.init(schema=DEVICE_KIND, db=db)
56+
await deleted_device.new(db=db, name="Test Device Deleted", role="Provider Edge")
57+
await deleted_device.save(db=db)
58+
await deleted_device.delete(db=db)
59+
5460
objs = {"first_device": first_device.id, "second_device": second_device.id}
5561

5662
return objs
@@ -82,3 +88,7 @@ async def test_step03_get_devices(self, db: InfrahubDatabase) -> None:
8288
async def test_step04_get_devices_via_graphql(self, client: InfrahubClient) -> None:
8389
devices = await client.all(kind=DEVICE_KIND)
8490
assert len(devices) == 2
91+
92+
async def test_final_validate(self, db: InfrahubDatabase):
93+
await verify_no_duplicate_relationships(db=db)
94+
await verify_no_edges_added_after_node_delete(db=db)

‎backend/tests/integration/schema_lifecycle/test_migration_hierarchy_change.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from infrahub.core.schema.generic_schema import GenericSchema
1111
from infrahub.core.schema.node_schema import NodeSchema
1212
from infrahub.database import InfrahubDatabase
13+
from infrahub.database.validation import verify_no_duplicate_relationships, verify_no_edges_added_after_node_delete
1314
from tests.helpers.test_app import TestInfrahubApp
1415

1516
PERSON_KIND = "TestingPerson"
@@ -185,3 +186,7 @@ async def test_load_schema_02(
185186
site_schema = db.schema.get(name="LocationSite", branch=branch_1, duplicate=False)
186187
assert site_schema.parent == "LocationMetro"
187188
assert site_schema.children == "" # noqa: PLC1901
189+
190+
async def test_final_validate(self, db: InfrahubDatabase):
191+
await verify_no_duplicate_relationships(db=db)
192+
await verify_no_edges_added_after_node_delete(db=db)

0 commit comments

Comments
 (0)