From 0ced4f77277c3dbcd0f8a0b8fb33614e22e2e754 Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Wed, 11 Dec 2024 06:52:38 -0800 Subject: [PATCH] handle generics and generics peers correctly when deleting nodes (#5133) * partial fix and tests for generic delete constraints * fix unit test * add changelog --- .../infrahub/core/node/delete_validator.py | 51 ++++++--- backend/tests/conftest.py | 103 ++++++++++++++++++ .../unit/core/test_node_manager_delete.py | 40 +++++++ changelog/4332.fixed.md | 1 + 4 files changed, 178 insertions(+), 17 deletions(-) create mode 100644 changelog/4332.fixed.md diff --git a/backend/infrahub/core/node/delete_validator.py b/backend/infrahub/core/node/delete_validator.py index 2edd239886..b2bebe6d1f 100644 --- a/backend/infrahub/core/node/delete_validator.py +++ b/backend/infrahub/core/node/delete_validator.py @@ -11,7 +11,7 @@ RelationshipGetByIdentifierQuery, RelationshipPeersData, ) -from infrahub.core.schema import MainSchemaTypes +from infrahub.core.schema import MainSchemaTypes, NodeSchema, ProfileSchema from infrahub.core.timestamp import Timestamp from infrahub.database import InfrahubDatabase from infrahub.exceptions import ValidationError @@ -28,20 +28,29 @@ def __init__(self, all_schemas_map: dict[str, MainSchemaTypes]) -> None: # {node_schema: {DeleteRelationshipType: {relationship_identifier: peer_node_schema}}} self._dependency_graph: dict[str, dict[DeleteRelationshipType, dict[str, set[str]]]] = {} - def index(self, start_schemas: Iterable[MainSchemaTypes]) -> None: + def index(self, start_schemas: Iterable[NodeSchema | ProfileSchema]) -> None: self._index_cascading_deletes(start_schemas=start_schemas) self._index_dependent_schema(start_schemas=start_schemas) + def _get_schema_kinds(self, schema_kind: str) -> set[str]: + schema = self._all_schemas_map[schema_kind] + if schema.is_node_schema: + return {schema_kind} + used_by = getattr(schema, "used_by", None) + if schema.is_generic_schema and used_by: + return set(used_by) + return set() + def _add_to_dependency_graph( - self, kind: str, relationship_type: DeleteRelationshipType, relationship_identifier: str, peer_kind: str + self, kind: str, relationship_type: DeleteRelationshipType, relationship_identifier: str, peer_kinds: set[str] ) -> None: if kind not in self._dependency_graph: self._dependency_graph[kind] = {} if relationship_type not in self._dependency_graph[kind]: self._dependency_graph[kind][relationship_type] = defaultdict(set) - self._dependency_graph[kind][relationship_type][relationship_identifier].add(peer_kind) + self._dependency_graph[kind][relationship_type][relationship_identifier].update(peer_kinds) - def _index_cascading_deletes(self, start_schemas: Iterable[MainSchemaTypes]) -> None: + def _index_cascading_deletes(self, start_schemas: Iterable[NodeSchema | ProfileSchema]) -> None: kinds_to_check: set[str] = {schema.kind for schema in start_schemas} while True: try: @@ -52,27 +61,35 @@ def _index_cascading_deletes(self, start_schemas: Iterable[MainSchemaTypes]) -> for relationship_schema in node_schema.relationships: if relationship_schema.on_delete != RelationshipDeleteBehavior.CASCADE: continue + peer_kinds = self._get_schema_kinds(schema_kind=relationship_schema.peer) self._add_to_dependency_graph( kind=kind_to_check, relationship_type=DeleteRelationshipType.CASCADE_DELETE, relationship_identifier=relationship_schema.get_identifier(), - peer_kind=relationship_schema.peer, + peer_kinds=peer_kinds, ) - if relationship_schema.peer not in self._dependency_graph: - kinds_to_check.add(relationship_schema.peer) - - def _index_dependent_schema(self, start_schemas: Iterable[MainSchemaTypes]) -> None: - start_schema_kinds = {schema.kind for schema in start_schemas} + for peer_kind in peer_kinds: + if peer_kind not in self._dependency_graph: + kinds_to_check.add(peer_kind) + + def _index_dependent_schema(self, start_schemas: Iterable[NodeSchema | ProfileSchema]) -> None: + start_schema_kinds: set[str] = set() + for start_schema in start_schemas: + start_schema_kinds.add(start_schema.kind) + if start_schema.inherit_from: + start_schema_kinds.update(set(start_schema.inherit_from)) for node_schema in self._all_schemas_map.values(): for relationship_schema in node_schema.relationships: if relationship_schema.optional is True or relationship_schema.peer not in start_schema_kinds: continue - self._add_to_dependency_graph( - kind=relationship_schema.peer, - relationship_type=DeleteRelationshipType.DEPENDENT_NODE, - relationship_identifier=relationship_schema.get_identifier(), - peer_kind=node_schema.kind, - ) + + for peer_kind in self._get_schema_kinds(schema_kind=relationship_schema.peer): + self._add_to_dependency_graph( + kind=peer_kind, + relationship_type=DeleteRelationshipType.DEPENDENT_NODE, + relationship_identifier=relationship_schema.get_identifier(), + peer_kinds={node_schema.kind}, + ) def get_relationship_identifiers(self) -> list[FullRelationshipIdentifier]: full_relationship_identifiers = [] diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index b05d138e15..e24c37b6ff 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -670,6 +670,109 @@ async def animal_person_schema( return registry.schema.register_schema(schema=animal_person_schema_unregistered, branch=default_branch.name) +@pytest.fixture +async def dependent_generics_unregistered(db: InfrahubDatabase, node_group_schema, data_schema) -> SchemaRoot: + schema: dict[str, Any] = { + "generics": [ + { + "name": "Animal", + "namespace": "Test", + "human_friendly_id": ["owner__name__value", "name__value"], + "uniqueness_constraints": [ + ["owner", "name__value"], + ], + "branch": BranchSupportType.AWARE.value, + "attributes": [ + {"name": "name", "kind": "Text"}, + {"name": "weight", "kind": "Number", "optional": True}, + ], + "relationships": [ + { + "name": "owner", + "peer": "TestPerson", + "optional": False, + "identifier": "person__animal", + "cardinality": "one", + "direction": "outbound", + }, + ], + }, + { + "name": "Person", + "namespace": "Test", + "human_friendly_id": ["name__value"], + "uniqueness_constraints": [ + ["name__value"], + ], + "branch": BranchSupportType.AWARE.value, + "attributes": [ + {"name": "name", "kind": "Text"}, + {"name": "height", "kind": "Number", "optional": True}, + ], + "relationships": [ + { + "name": "animals", + "peer": "TestAnimal", + "identifier": "person__animal", + "cardinality": "many", + "direction": "inbound", + }, + ], + }, + ], + "nodes": [ + { + "name": "Dog", + "namespace": "Test", + "inherit_from": ["TestAnimal"], + "display_labels": ["name__value", "breed__value"], + "attributes": [ + {"name": "breed", "kind": "Text", "optional": False}, + {"name": "color", "kind": "Color", "default_value": "#444444", "optional": True}, + ], + }, + { + "name": "Cat", + "namespace": "Test", + "inherit_from": ["TestAnimal"], + "display_labels": ["name__value", "breed__value", "color__value"], + "attributes": [ + {"name": "breed", "kind": "Text", "optional": False}, + {"name": "color", "kind": "Color", "default_value": "#444444", "optional": True}, + ], + }, + { + "name": "Human", + "namespace": "Test", + "display_labels": ["name__value"], + "inherit_from": ["TestPerson"], + "default_filter": "name__value", + "human_friendly_id": ["name__value"], + }, + { + "name": "Cylon", + "namespace": "Test", + "display_labels": ["name__value"], + "inherit_from": ["TestPerson"], + "default_filter": "name__value", + "human_friendly_id": ["name__value"], + "attributes": [ + {"name": "model_number", "kind": "Number", "optional": False}, + ], + }, + ], + } + + return SchemaRoot(**schema) + + +@pytest.fixture +async def dependent_generics_schema( + db: InfrahubDatabase, default_branch: Branch, dependent_generics_unregistered +) -> SchemaBranch: + return registry.schema.register_schema(schema=dependent_generics_unregistered, branch=default_branch.name) + + @pytest.fixture async def node_group_schema(db: InfrahubDatabase, default_branch: Branch, data_schema) -> None: SCHEMA: dict[str, Any] = { diff --git a/backend/tests/unit/core/test_node_manager_delete.py b/backend/tests/unit/core/test_node_manager_delete.py index 16bf753715..88ed38b8b0 100644 --- a/backend/tests/unit/core/test_node_manager_delete.py +++ b/backend/tests/unit/core/test_node_manager_delete.py @@ -8,6 +8,7 @@ from infrahub.core.manager import NodeManager from infrahub.core.node import Node from infrahub.core.schema.relationship_schema import RelationshipSchema +from infrahub.core.schema.schema_branch import SchemaBranch from infrahub.database import InfrahubDatabase from infrahub.exceptions import ValidationError @@ -162,3 +163,42 @@ async def test_delete_with_cascade_both_directions_succeeds( assert {d.id for d in deleted} == {person_john_main.id, car_accord_main.id, car_prius_main.id} node_map = await NodeManager.get_many(db=db, ids=[person_john_main.id, car_accord_main.id, car_prius_main.id]) assert node_map == {} + + +async def test_delete_with_required_on_generic_prevented(db, default_branch, dependent_generics_schema: SchemaBranch): + human = await Node.init(db=db, schema="TestHuman", branch=default_branch) + await human.new(db=db, name="Jane", height=180) + await human.save(db=db) + dog = await Node.init(db=db, schema="TestDog", branch=default_branch) + await dog.new(db=db, name="Roofus", breed="whocares", weight=50, owner=human) + await dog.save(db=db) + + with pytest.raises(ValidationError) as exc: + await NodeManager.delete(db=db, branch=default_branch, nodes=[human]) + + assert f"Cannot delete TestHuman '{human.id}'" in str(exc.value) + assert f"It is linked to mandatory relationship owner on node TestDog '{dog.id}'" in str(exc.value) + + retrieved_human = await NodeManager.get_one(db=db, id=human.id) + assert retrieved_human.id == human.id + + +async def test_delete_with_cascade_on_generic_allowed(db, default_branch, dependent_generics_schema: SchemaBranch): + # set TestPerson.animals to be cascade delete + schema_branch = registry.schema.get_schema_branch(name=default_branch.name) + for schema_kind in ("TestPerson", "TestHuman", "TestCylon"): + schema = schema_branch.get(name=schema_kind, duplicate=False) + schema.get_relationship("animals").on_delete = RelationshipDeleteBehavior.CASCADE + + human = await Node.init(db=db, schema="TestHuman", branch=default_branch) + await human.new(db=db, name="Jane", height=180) + await human.save(db=db) + dog = await Node.init(db=db, schema="TestDog", branch=default_branch) + await dog.new(db=db, name="Roofus", breed="whocares", weight=50, owner=human) + await dog.save(db=db) + + deleted = await NodeManager.delete(db=db, branch=default_branch, nodes=[human]) + + assert {d.id for d in deleted} == {human.id, dog.id} + node_map = await NodeManager.get_many(db=db, ids=[human.id, dog.id]) + assert node_map == {} diff --git a/changelog/4332.fixed.md b/changelog/4332.fixed.md new file mode 100644 index 0000000000..9ecfda046d --- /dev/null +++ b/changelog/4332.fixed.md @@ -0,0 +1 @@ +Update delete constraints to correctly account for relationships on generics and relationships for which the peer kind is a generic. \ No newline at end of file