From 9be709b6d643de5fbc3bd128591b2f5e3693da59 Mon Sep 17 00:00:00 2001 From: Ville Laitila Date: Thu, 5 Feb 2026 02:37:10 +0200 Subject: [PATCH] optimize create_unique_element_association() --- src/sgraph/modelapi.py | 9 +++---- src/sgraph/selement.py | 5 +++- src/sgraph/selementassociation.py | 29 +++++++++----------- tests/modelapi_test.py | 44 +++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/sgraph/modelapi.py b/src/sgraph/modelapi.py index de2b1dd..c42975e 100644 --- a/src/sgraph/modelapi.py +++ b/src/sgraph/modelapi.py @@ -229,12 +229,11 @@ def create_assoc(x: SElement, other: SElement, is_outgoing: bool, ea: SElementAs new_or_existing_referred_elem, this_is_new = sub_graph.create_or_get_element(x) if is_outgoing: - SElementAssociation(other, new_or_existing_referred_elem, ea.deptype, - ea.attrs).initElems() + SElementAssociation.create_unique_element_association( + other, new_or_existing_referred_elem, ea.deptype, ea.attrs) else: - - SElementAssociation(new_or_existing_referred_elem, other, ea.deptype, - ea.attrs).initElems() + SElementAssociation.create_unique_element_association( + new_or_existing_referred_elem, other, ea.deptype, ea.attrs) return new_or_existing_referred_elem, this_is_new diff --git a/src/sgraph/selement.py b/src/sgraph/selement.py index ef51763..bb28cf1 100644 --- a/src/sgraph/selement.py +++ b/src/sgraph/selement.py @@ -11,7 +11,7 @@ class SElement: __slots__ = 'name', 'parent', 'children', 'childrenDict', 'outgoing', 'incoming', 'attrs', \ - 'human_readable_name' + 'human_readable_name', '_incoming_index' name: str parent: Optional["SElement"] @@ -21,6 +21,8 @@ class SElement: incoming: list["SElementAssociation"] attrs: dict[str, str | int | list[str]] human_readable_name: str + # Index for O(1) duplicate association lookup: (id(fromElement), deptype) -> assoc + _incoming_index: dict[tuple[int, str], "SElementAssociation"] def __init__(self, parent: Optional['SElement'], name: str): """ @@ -69,6 +71,7 @@ def __init__(self, parent: Optional['SElement'], name: str): self.childrenDict = {} self.outgoing = [] self.incoming = [] + self._incoming_index = {} self.attrs = {} # self.num = '0' diff --git a/src/sgraph/selementassociation.py b/src/sgraph/selementassociation.py index 4f134c3..a2dcbfe 100644 --- a/src/sgraph/selementassociation.py +++ b/src/sgraph/selementassociation.py @@ -36,23 +36,14 @@ def create_unique_element_association( :returns: Return tuple of the existing or new element and a boolean indicating if a new element was created (true if new was created, false otherwise)""" - def filter_existing(incoming: "SElementAssociation"): - from_element_matches = incoming.fromElement == from_element - if dependency_type: - return dependency_type == incoming.deptype and \ - from_element_matches - else: - return from_element_matches - - filtered_associations = filter(filter_existing, to_element.incoming) - existing_associations = list(filtered_associations) - - # Do not create association if the same association already exists - if len(existing_associations) > 0: + # O(1) lookup using the index + key = (id(from_element), dependency_type) + existing = to_element._incoming_index.get(key) + + if existing is not None: # Combine attributes to the existing association - existing_associations[0].attrs.update(dependency_attributes) - # return {'existingOrNewAssociation': existing_associations[0], 'isNew': False} - return existing_associations[0], False + existing.attrs.update(dependency_attributes) + return existing, False new_association = SElementAssociation(from_element, to_element, dependency_type, dependency_attributes) @@ -114,10 +105,16 @@ def getAttributes(self): def initElems(self): self.fromElement.outgoing.append(self) self.toElement.incoming.append(self) + # Maintain index for O(1) duplicate lookup + key = (id(self.fromElement), self.deptype) + self.toElement._incoming_index[key] = self def remove(self): self.fromElement.outgoing.remove(self) self.toElement.incoming.remove(self) + # Maintain index for O(1) duplicate lookup + key = (id(self.fromElement), self.deptype) + self.toElement._incoming_index.pop(key, None) def addAttribute(self, attr_name: str, attr_val: str | int | list[str]): self.attrs[attr_name] = attr_val diff --git a/tests/modelapi_test.py b/tests/modelapi_test.py index 5284f3b..86289fa 100644 --- a/tests/modelapi_test.py +++ b/tests/modelapi_test.py @@ -190,3 +190,47 @@ def test_filter_model_with_attributes_ref(): assert (original_model.createOrGetElementFromPath(e).attrs == m2.createOrGetElementFromPath(e).attrs) print(m2.to_deps(None)) + +def test_filter_model_no_duplicate_associations(): + """Test that filter_model does not create duplicate associations. + + This test verifies the fix for issue #37 - using create_unique_element_association + to prevent duplicate associations when the same dependency is encountered multiple + times during traversal. + """ + from sgraph import SGraph, SElement, SElementAssociation + + # Create a model where the same association could be encountered multiple times + model = SGraph(SElement(None, '')) + a = model.createOrGetElementFromPath('/root/a') + b = model.createOrGetElementFromPath('/root/b') + c = model.createOrGetElementFromPath('/root/c') + external = model.createOrGetElementFromPath('/external/lib') + + # Both a and b depend on external + SElementAssociation(a, external, 'use').initElems() + SElementAssociation(b, external, 'use').initElems() + # a also depends on b + SElementAssociation(a, b, 'use').initElems() + # c depends on a + SElementAssociation(c, a, 'use').initElems() + + model_api = ModelApi(model=model) + root_elem = model.createOrGetElementFromPath('/root') + + # Filter with DirectAndIndirect to traverse through multiple paths + subgraph = model_api.filter_model( + root_elem, model, + filter_outgoing=FilterAssocations.DirectAndIndirect, + filter_incoming=FilterAssocations.Ignore) + + # Find the external element in the subgraph + external_in_subgraph = subgraph.findElementFromPath('/external/lib') + assert external_in_subgraph is not None + + # Verify no duplicate incoming associations to external + # Each unique (from, to, type) combination should only appear once + incoming_signatures = [(ea.fromElement.getPath(), ea.deptype) for ea in external_in_subgraph.incoming] + assert len(incoming_signatures) == len(set(incoming_signatures)), \ + f"Duplicate associations found: {incoming_signatures}" +