incorporate feedback

Author: ogenstad

dev/specs/ihs-138-store-merge/decisions.md

@@ -270,7 +270,7 @@ its results.
 ## Summary table
 
 | ID | Decision | Outcome |
-|----|----------|---------|
+| -- | -------- | ------- |
 | D1 | Returned vs stored object | Return per-query object; store holds the merged canonical copy |
 | D2 | In-place mutation model | Accept; one always-current merged object per node; protect local edits |
 | D3 | `store.set()` default | **Merge by default, uniformly** (override); replace is opt-in via `merge=False` |

infrahub_sdk/node/attribute.py

@@ -163,6 +163,10 @@ def _merge(self, incoming: Attribute) -> None:
         fetched = incoming._fetched_fields
         if "value" in fetched or incoming.value_has_been_mutated:
             self._value = incoming._value
+            # The pool-allocation intent travels with the value: a fetched copy carries
+            # None, clearing any stale allocation request that would otherwise take
+            # precedence over the value in the next mutation payload.
+            self._from_pool = incoming._from_pool
             if incoming.value_has_been_mutated:
                 self.value_has_been_mutated = True
         for field_name in (

infrahub_sdk/node/node.py

@@ -42,6 +42,7 @@
 
     from ..client import InfrahubClient, InfrahubClientSync
     from ..context import RequestContext
+    from ..protocols_base import CoreNodeBase
     from ..schema import MainSchemaTypesAPI
     from ..types import Order
 
@@ -276,7 +277,7 @@ def get_node_metadata(self) -> NodeMetadata | None:
         return self._metadata
 
     @staticmethod
-    def _field_was_fetched(data: Any, name: str) -> bool:
+    def _field_was_fetched(data: object, name: str) -> bool:
         """Return whether a field was present in the response a node was built from.
 
         Key-presence is the only unambiguous "was fetched" signal: an absent field and
@@ -316,7 +317,7 @@ def _get_request_context(self, request_context: RequestContext | None = None) ->
     def _init_relationships(self, data: dict | None = None) -> None:
         pass
 
-    def _merge(self, node: InfrahubNodeBase) -> None:
+    def _merge(self, node: InfrahubNodeBase | CoreNodeBase) -> None:
         """Merge a fresher copy of the same node into this one, field by field.
 
         Used by the client store when a node with this UUID is fetched again: fields the
@@ -348,8 +349,8 @@ def _merge(self, node: InfrahubNodeBase) -> None:
                 self._merge_raw_field(name=name, stored_data=stored_data, incoming_data=incoming_data)
 
         for container_field in RELATIONSHIP_CONTAINER_FIELDS:
-            incoming_bucket: dict[str, Any] = getattr(node, container_field, {})
-            stored_bucket: dict[str, Any] = getattr(self, container_field, {})
+            incoming_bucket: dict[str, RelatedNodeBase | RelationshipManagerBase] = getattr(node, container_field, {})
+            stored_bucket: dict[str, RelatedNodeBase | RelationshipManagerBase] = getattr(self, container_field, {})
             for name, incoming_rel in incoming_bucket.items():
                 if self._merge_relationship(stored_bucket, name, incoming_rel):
                     self._merge_raw_field(name=name, stored_data=stored_data, incoming_data=incoming_data)
@@ -385,29 +386,32 @@ def _merge_attribute(self, name: str, incoming_attr: Attribute) -> bool:
         return True
 
     @staticmethod
-    def _merge_relationship(stored_bucket: dict[str, Any], name: str, incoming_rel: Any) -> bool:
+    def _merge_relationship(
+        stored_bucket: dict[str, RelatedNodeBase | RelationshipManagerBase],
+        name: str,
+        incoming_rel: RelatedNodeBase | RelationshipManagerBase,
+    ) -> bool:
         """Merge one relationship entry into ``stored_bucket``; return whether it was taken."""
+        stored_rel = stored_bucket.get(name)
         if isinstance(incoming_rel, RelatedNodeBase):
             if not incoming_rel.is_fetched and not incoming_rel._peer_has_been_mutated:
                 return False
-            stored_rel = stored_bucket.get(name)
-            if stored_rel is None:
+            if not isinstance(stored_rel, RelatedNodeBase):
                 stored_bucket[name] = incoming_rel
             elif stored_rel._peer_has_been_mutated:
                 return False
             else:
                 stored_rel._merge(incoming_rel)
             return True
 
-        if not isinstance(incoming_rel, RelationshipManagerBase) or not incoming_rel.is_fetched:
+        if not incoming_rel.is_fetched:
             return False
-        stored_manager = stored_bucket.get(name)
-        if stored_manager is None:
+        if not isinstance(stored_rel, RelationshipManagerBase):
             stored_bucket[name] = incoming_rel
-        elif stored_manager.has_update:
+        elif stored_rel.has_update:
             return False
         else:
-            stored_manager._merge(incoming_rel)
+            stored_rel._merge(incoming_rel)
         return True
 
     def _reset_mutation_tracking(self) -> None:
@@ -421,7 +425,8 @@ def _reset_mutation_tracking(self) -> None:
         for attr in self._attribute_data.values():
             attr.value_has_been_mutated = False
         for container_field in RELATIONSHIP_CONTAINER_FIELDS:
-            for rel in getattr(self, container_field, {}).values():
+            container: dict[str, RelatedNodeBase | RelationshipManagerBase] = getattr(self, container_field, {})
+            for rel in container.values():
                 if isinstance(rel, RelatedNodeBase):
                     rel._peer_has_been_mutated = False
                 elif isinstance(rel, RelationshipManagerBase):

infrahub_sdk/node/related_node.py

@@ -44,6 +44,14 @@ class RelatedNodeBase:
 
     """
 
+    # Relationship-edge properties, assigned dynamically in _init_properties from
+    # PROPERTIES_FLAG / PROPERTIES_OBJECT; declared here so they are part of the
+    # visible class surface.
+    is_protected: bool | None
+    updated_at: str | None
+    source: str | None
+    owner: str | None
+
     def __init__(
         self,
         branch: str,
@@ -283,21 +291,32 @@ def _merge(self, incoming: RelatedNodeBase) -> None:
         """Merge a fresher copy of this relationship into this one.
 
         Which peer the relationship points at always refreshes (even to no peer, so a
-        cleared relationship or a move-to-root is reflected). When the peer stays the
-        same, its descriptive identity fields (hfid, display label, typename, kind) and
-        the edge properties are only overwritten when the incoming fetch actually
-        carried them, so a narrow payload does not null out previously fetched data.
-        An incoming unsaved edit keeps its pending-mutation marker. Callers are
-        responsible for the higher-level gates (``is_fetched`` on the incoming
-        relationship, ``_peer_has_been_mutated`` on this one).
+        cleared relationship or a move-to-root is reflected). A changed peer means a
+        different relationship edge, so the incoming edge is taken wholesale,
+        properties included. When the peer stays the same, its descriptive identity
+        fields (hfid, display label, typename, kind) and the edge properties are only
+        overwritten when the incoming fetch actually carried them, so a narrow payload
+        does not null out previously fetched data. An incoming unsaved edit keeps its
+        pending-mutation marker. Callers are responsible for the higher-level gates
+        (``is_fetched`` on the incoming relationship, ``_peer_has_been_mutated`` on
+        this one).
         """
-        if incoming._id != self._id:
+        # An hfid-only relationship (no id on either side) changes peer when the hfid does.
+        peer_changed = incoming._id != self._id or (incoming._id is None and incoming._hfid != self._hfid)
+        if peer_changed:
             self._peer = incoming._peer
             self._id = incoming._id
             self._hfid = incoming._hfid
             self._display_label = incoming._display_label
             self._typename = incoming._typename
             self._kind = incoming._kind
+            # The stored properties and metadata described the edge to the old peer;
+            # they must not survive onto the new edge.
+            for prop in self._properties:
+                setattr(self, prop, getattr(incoming, prop))
+            self._source_typename = incoming._source_typename
+            self._relationship_metadata = incoming._relationship_metadata
+            self._fetched_properties = incoming._fetched_properties
         else:
             if incoming._peer is not None:
                 self._peer = incoming._peer
@@ -309,14 +328,14 @@ def _merge(self, incoming: RelatedNodeBase) -> None:
                 self._typename = incoming._typename
             if incoming._kind is not None:
                 self._kind = incoming._kind
-        for prop in self._properties:
-            if prop in incoming._fetched_properties:
-                setattr(self, prop, getattr(incoming, prop))
-                if prop == "source":
-                    self._source_typename = incoming._source_typename
-        if incoming._relationship_metadata is not None:
-            self._relationship_metadata = incoming._relationship_metadata
-        self._fetched_properties = intern_frozenset(self._fetched_properties | incoming._fetched_properties)
+            for prop in self._properties:
+                if prop in incoming._fetched_properties:
+                    setattr(self, prop, getattr(incoming, prop))
+                    if prop == "source":
+                        self._source_typename = incoming._source_typename
+            if incoming._relationship_metadata is not None:
+                self._relationship_metadata = incoming._relationship_metadata
+            self._fetched_properties = intern_frozenset(self._fetched_properties | incoming._fetched_properties)
         self.is_fetched = True
         self._peer_has_been_mutated = incoming._peer_has_been_mutated
 

infrahub_sdk/protocols_base.py

@@ -6,6 +6,8 @@
     import ipaddress
 
     from .context import RequestContext
+    from .node import InfrahubNodeBase
+    from .node.attribute import Attribute as NodeAttribute
     from .node.metadata import NodeMetadata
     from .schema import MainSchemaTypes
 
@@ -175,6 +177,10 @@ class AnyAttributeOptional(Attribute):
 class CoreNodeBase:
     _schema: MainSchemaTypes
     _internal_id: str
+    _data: dict | None
+    _existing: bool
+    _metadata: NodeMetadata | None
+    _attribute_data: dict[str, NodeAttribute]
     id: str  # NOTE this is incorrect, should be str | None
     display_label: str | None
     typename: str | None
@@ -211,7 +217,8 @@ def get_raw_graphql_data(self) -> dict | None: ...
 
     def get_node_metadata(self) -> NodeMetadata | None: ...
 
-    def _merge(self, node: Any) -> None: ...
+    def _merge(self, node: CoreNodeBase | InfrahubNodeBase) -> None:
+        raise NotImplementedError
 
 
 class CoreNode(CoreNodeBase):

infrahub_sdk/store.py

@@ -2,7 +2,7 @@
 
 import inspect
 import warnings
-from typing import TYPE_CHECKING, Any, Literal, cast, overload
+from typing import TYPE_CHECKING, Literal, overload
 
 from infrahub_sdk.protocols_base import CoreNodeBase
 
@@ -68,7 +68,7 @@ def set(
                 if merge and existing.get_kind() == node.get_kind():
                     # Merge into the existing object and keep its internal id so every
                     # reference already handed out by the store stays current.
-                    existing._merge(cast("Any", node))
+                    existing._merge(node)
                     node = existing
                 else:
                     # Replace wholesale: explicit merge=False, or the node was converted

tests/unit/sdk/test_store_merge.py

@@ -392,6 +392,86 @@ def test_merge_same_peer_keeps_identity_details_not_carried(
     assert stored.primary_tag.typename == "BuiltinTag"
 
 
+@pytest.mark.parametrize("client_type", client_types)
+def test_merge_clears_pool_allocation_intent_on_refetch(
+    client_type: str, clients: BothClients, location_schema: NodeSchemaAPI
+) -> None:
+    """A fetched value clears a stale from_pool request.
+
+    A surviving from_pool would take precedence over the value in the next mutation
+    payload and trigger a re-allocation.
+    """
+    client, store, node_class = setup_store(client_type, clients)
+
+    pool_data = deep_location_data()
+    pool_data["node"]["description"] = {"from_pool": {"id": "pppppppp-pppp-pppp-pppp-pppppppppppp"}}
+    stored_node = node_class(client=client, schema=location_schema, data=pool_data)
+    assert stored_node._attribute_data["description"]._from_pool is not None
+    store.set(node=stored_node)
+
+    refetch = shallow_location_data()
+    refetch["node"]["description"] = {"value": "allocated-value"}
+    store.set(node=node_class(client=client, schema=location_schema, data=refetch))
+
+    assert get_location(store) is stored_node
+    assert stored_node._attribute_data["description"].value == "allocated-value"
+    assert stored_node._attribute_data["description"]._from_pool is None
+
+
+@pytest.mark.parametrize("client_type", client_types)
+def test_merge_peer_change_drops_old_edge_properties(
+    client_type: str, clients: BothClients, location_schema: NodeSchemaAPI
+) -> None:
+    """A changed peer is a different edge: the old edge's properties must not survive."""
+    client, store, node_class = setup_store(client_type, clients)
+
+    with_properties = deep_location_data()
+    with_properties["node"]["primary_tag"] = {
+        "properties": {
+            "is_protected": True,
+            "source": {
+                "id": "ssssssss-ssss-ssss-ssss-ssssssssssss",
+                "display_label": "crm",
+                "__typename": "CoreAccount",
+            },
+        },
+        "node": {"id": TAG_RED_ID, "display_label": "red", "__typename": "BuiltinTag"},
+    }
+    store.set(node=node_class(client=client, schema=location_schema, data=with_properties))
+
+    refetch = shallow_location_data()
+    refetch["node"]["primary_tag"] = {
+        "node": {"id": TAG_GREEN_ID, "display_label": "green", "__typename": "BuiltinTag"}
+    }
+    store.set(node=node_class(client=client, schema=location_schema, data=refetch))
+
+    stored = get_location(store)
+    assert stored.primary_tag.id == TAG_GREEN_ID
+    assert stored.primary_tag.is_protected is None
+    assert stored.primary_tag.source is None
+
+
+@pytest.mark.parametrize("client_type", client_types)
+def test_merge_clears_hfid_only_relationship(
+    client_type: str, clients: BothClients, location_schema: NodeSchemaAPI
+) -> None:
+    """A cleared relationship tracked only by hfid (no id on either side) is reflected."""
+    client, store, node_class = setup_store(client_type, clients)
+
+    hfid_only = deep_location_data()
+    hfid_only["node"]["primary_tag"] = {"node": {"hfid": ["red"], "__typename": "BuiltinTag"}}
+    store.set(node=node_class(client=client, schema=location_schema, data=hfid_only))
+    assert get_location(store).primary_tag.hfid == ["red"]
+
+    refetch = shallow_location_data()
+    refetch["node"]["primary_tag"] = {"node": None}
+    store.set(node=node_class(client=client, schema=location_schema, data=refetch))
+
+    stored = get_location(store)
+    assert stored.primary_tag.hfid is None
+    assert stored.primary_tag.initialized is False
+
+
 @pytest.mark.parametrize("client_type", client_types)
 def test_merge_propagates_unsaved_edits(client_type: str, clients: BothClients, location_schema: NodeSchemaAPI) -> None:
     """Unsaved edits merged into the store keep their pending-mutation markers.