Skip to content

Commit

Permalink
diff calculation bug where owner is also the peer on a related node (#…
Browse files Browse the repository at this point in the history
…4128)

* update diff calculation query for unhandled path pattern

* fix bug preventing adding SOURCE/OWNER without updating peer ID
  • Loading branch information
ajtmccarty authored Aug 18, 2024
1 parent 17cc8ac commit 65a6c1d
Show file tree
Hide file tree
Showing 4 changed files with 156 additions and 14 deletions.
8 changes: 6 additions & 2 deletions backend/infrahub/core/diff/query_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,11 @@ class DiffRelationshipIntermediate:
_single_relationship_list: list[DiffSingleRelationshipIntermediate] = field(default_factory=list)

def add_path(self, database_path: DatabasePath) -> None:
if database_path.property_type == DatabaseEdgeType.IS_RELATED:
if database_path.property_type in [
DatabaseEdgeType.IS_RELATED,
DatabaseEdgeType.HAS_OWNER,
DatabaseEdgeType.HAS_SOURCE,
]:
value = database_path.peer_id
else:
value = database_path.property_value
Expand Down Expand Up @@ -553,7 +557,7 @@ def _remove_empty_base_diff_root(self) -> None:
for relationship_diff in node_diff.relationships_by_name.values():
for diff_relationship_property_list in relationship_diff.properties_by_db_id.values():
for diff_relationship_property in diff_relationship_property_list:
if diff_relationship_property.changed_at <= self.from_time:
if diff_relationship_property.changed_at >= self.from_time:
return
del self._diff_root_by_branch[self.base_branch_name]

Expand Down
8 changes: 4 additions & 4 deletions backend/infrahub/core/query/diff.py
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ async def query_init(self, db: InfrahubDatabase, **kwargs):
MATCH (p)-[diff_rel]-(q)
WHERE any(l in labels(p) WHERE l in ["Node", "Attribute", "Relationship"])
AND %(diff_rel_filter)s
AND p.branch_support IN $branch_support
AND (p.branch_support IN $branch_support OR q.branch_support IN $branch_support)
AND %(p_node_where)s
// subqueries to get full paths associated with the above update edges
WITH p, diff_rel, q
Expand Down Expand Up @@ -604,7 +604,7 @@ async def query_init(self, db: InfrahubDatabase, **kwargs):
WHERE r.from <= $to_time AND (r.to IS NULL or r.to >= $to_time)
AND r.branch IN $branch_names
)
AND ID(inner_p) <> ID(prop)
AND [ID(inner_p), type(inner_diff_rel)] <> [ID(prop), type(r_prop)]
WITH path, prop, r_prop, r_root
ORDER BY
ID(prop),
Expand Down Expand Up @@ -635,7 +635,7 @@ async def query_init(self, db: InfrahubDatabase, **kwargs):
WHERE r.from <= $to_time AND (r.to IS NULL or r.to >= $to_time)
AND r.branch IN $branch_names
)
AND ID(inner_p) <> ID(prop)
AND [ID(inner_p), type(r_node)] <> [ID(prop), type(r_prop)]
WITH path, node, prop, r_prop, r_node
ORDER BY
ID(node),
Expand All @@ -644,7 +644,7 @@ async def query_init(self, db: InfrahubDatabase, **kwargs):
r_node.branch = diff_rel.branch DESC,
r_prop.from DESC,
r_node.from DESC
WITH node, prop, head(collect(path)) AS latest_path
WITH node, prop, type(r_prop) AS r_prop_type, type(r_node) AS r_node_type, head(collect(path)) AS latest_path
RETURN latest_path
}
WITH p, q, diff_rel, full_diff_paths, collect(latest_path) AS latest_paths
Expand Down
23 changes: 15 additions & 8 deletions backend/infrahub/core/relationship/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,21 @@ def get_peer_schema(self, db: InfrahubDatabase) -> MainSchemaTypes:
def compare_properties_with_data(self, data: RelationshipPeerData) -> list[str]:
different_properties = []

for prop_name, prop in data.properties.items():
if hasattr(self, "_flag_properties") and prop_name in self._flag_properties:
if prop.value != getattr(self, prop_name):
different_properties.append(prop_name)

elif hasattr(self, "_node_properties") and prop_name in self._node_properties:
if prop.value != getattr(self, f"{prop_name}_id"):
different_properties.append(prop_name)
if hasattr(self, "_flag_properties"):
for property_name in self._flag_properties:
memory_value = getattr(self, property_name, None)
database_prop = data.properties.get(property_name)
database_value = database_prop.value if database_prop else None
if memory_value != database_value:
different_properties.append(property_name)

if hasattr(self, "_node_properties"):
for property_name in self._node_properties:
memory_value = getattr(self, f"{property_name}_id", None)
database_prop = data.properties.get(property_name)
database_value = database_prop.value if database_prop else None
if memory_value != database_value:
different_properties.append(property_name)

return different_properties

Expand Down
131 changes: 131 additions & 0 deletions backend/tests/unit/core/diff/test_diff_query_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -590,3 +590,134 @@ async def test_add_node_branch(
(DatabaseEdgeType.IS_PROTECTED, DiffAction.ADDED, False, None),
(DatabaseEdgeType.IS_RELATED, DiffAction.ADDED, person_jane_main.id, None),
}


async def test_agnostic_source_relationship_update(
db: InfrahubDatabase,
default_branch: Branch,
car_person_schema_global,
):
person_1 = await Node.init(db=db, schema="TestPerson", branch=default_branch)
await person_1.new(db=db, name="Herb", height=165)
await person_1.save(db=db)
new_car = await Node.init(db=db, branch=default_branch, schema="TestCar")
await new_car.new(db=db, name="Batmobile", color="#000000", nbr_seats=1, is_electric=False, owner=person_1)
await new_car.save(db=db)
branch = await create_branch(db=db, branch_name="branch")
from_time = Timestamp(branch.created_at)
branch_car = await NodeManager.get_one(db=db, branch=branch, id=new_car.id)
await branch_car.owner.update(db=db, data={"id": person_1.id, "_relation__source": person_1.id})
await branch_car.save(db=db)

diff_query = await DiffAllPathsQuery.init(
db=db,
branch=branch,
base_branch=default_branch,
)
await diff_query.execute(db=db)

diff_parser = DiffQueryParser(
diff_query=diff_query,
base_branch_name=default_branch.name,
diff_branch_name=branch.name,
schema_manager=registry.schema,
from_time=from_time,
)
diff_parser.parse()

assert diff_parser.get_branches() == {branch.name}
root_path = diff_parser.get_diff_root_for_branch(branch=branch.name)
assert root_path.branch == branch.name
assert len(root_path.nodes) == 1
diff_node = root_path.nodes.pop()
assert diff_node.uuid == new_car.get_id()
assert diff_node.action is DiffAction.UPDATED
assert diff_node.attributes == []
assert len(diff_node.relationships) == 1
diff_relationship = diff_node.relationships.pop()
assert diff_relationship.name == "owner"
assert diff_relationship.action is DiffAction.UPDATED
assert len(diff_relationship.relationships) == 1
diff_element = diff_relationship.relationships.pop()
assert diff_element.peer_id == person_1.get_id()
assert diff_element.action is DiffAction.UPDATED
diff_props_by_type = {p.property_type: p for p in diff_element.properties}
assert set(diff_props_by_type.keys()) == {DatabaseEdgeType.IS_RELATED, DatabaseEdgeType.HAS_SOURCE}
diff_prop_is_related = diff_props_by_type[DatabaseEdgeType.IS_RELATED]
assert diff_prop_is_related.previous_value == person_1.get_id()
assert diff_prop_is_related.new_value == person_1.get_id()
assert diff_prop_is_related.action is DiffAction.UNCHANGED
diff_prop_has_source = diff_props_by_type[DatabaseEdgeType.HAS_SOURCE]
assert diff_prop_has_source.previous_value is None
assert diff_prop_has_source.new_value == person_1.get_id()
assert diff_prop_has_source.action is DiffAction.ADDED


async def test_agnostic_owner_relationship_added(
db: InfrahubDatabase,
default_branch: Branch,
car_person_schema_global,
):
branch = await create_branch(db=db, branch_name="branch")
from_time = Timestamp(branch.created_at)
person_1 = await Node.init(db=db, schema="TestPerson", branch=branch)
await person_1.new(db=db, name="Herb", height=165)
await person_1.save(db=db)
new_car = await Node.init(db=db, branch=branch, schema="TestCar")
await new_car.new(db=db, name="Batmobile", color="#000000", nbr_seats=1, is_electric=False, owner=person_1)
await new_car.owner.update(db=db, data={"id": person_1.id, "_relation__owner": person_1.id})
await new_car.save(db=db)

diff_query = await DiffAllPathsQuery.init(
db=db,
branch=branch,
base_branch=default_branch,
)
await diff_query.execute(db=db)

diff_parser = DiffQueryParser(
diff_query=diff_query,
base_branch_name=default_branch.name,
diff_branch_name=branch.name,
schema_manager=registry.schema,
from_time=from_time,
)
diff_parser.parse()

assert diff_parser.get_branches() == {branch.name}
root_path = diff_parser.get_diff_root_for_branch(branch=branch.name)
assert root_path.branch == branch.name
diff_nodes_by_id = {n.uuid: n for n in root_path.nodes}
assert set(diff_nodes_by_id.keys()) == {new_car.get_id()}
diff_node_car = diff_nodes_by_id[new_car.get_id()]
assert diff_node_car.action is DiffAction.ADDED
assert {(attr.name, attr.action) for attr in diff_node_car.attributes} == {
("name", DiffAction.ADDED),
("color", DiffAction.ADDED),
("is_electric", DiffAction.ADDED),
}
assert len(diff_node_car.relationships) == 1
diff_relationship = diff_node_car.relationships.pop()
assert diff_relationship.name == "owner"
assert diff_relationship.action is DiffAction.ADDED
assert len(diff_relationship.relationships) == 1
diff_element = diff_relationship.relationships.pop()
assert diff_element.peer_id == person_1.get_id()
assert diff_element.action is DiffAction.ADDED
diff_props_by_type = {p.property_type: p for p in diff_element.properties}
assert set(diff_props_by_type.keys()) == {
DatabaseEdgeType.IS_RELATED,
DatabaseEdgeType.HAS_OWNER,
DatabaseEdgeType.IS_PROTECTED,
DatabaseEdgeType.IS_VISIBLE,
}
diff_prop_tuples = {
(diff_prop.property_type, diff_prop.action, diff_prop.previous_value, diff_prop.new_value)
for diff_prop in diff_props_by_type.values()
}
assert diff_prop_tuples == {
(DatabaseEdgeType.IS_RELATED, DiffAction.ADDED, None, person_1.get_id()),
(DatabaseEdgeType.HAS_OWNER, DiffAction.ADDED, None, person_1.get_id()),
(DatabaseEdgeType.IS_PROTECTED, DiffAction.ADDED, None, False),
(DatabaseEdgeType.IS_VISIBLE, DiffAction.ADDED, None, True),
}

0 comments on commit 65a6c1d

Please sign in to comment.