fix(sdk): merge re-fetched nodes into the client store instead of overwriting#1120
fix(sdk): merge re-fetched nodes into the client store instead of overwriting#1120ogenstad wants to merge 4 commits into
Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
4fa86f6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0e5a3abf.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-store-merge-ihs-138.infrahub-sdk-python.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ihs-138-store-merge/decisions.md">
<violation number="1" location="dev/specs/ihs-138-store-merge/decisions.md:133">
P3: Stale line reference in D5: `related_node.py:189` points to `display_label`, not `initialized`. The `initialized` property is at line 182.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| --- | ||
|
|
||
| ## D5 - Internal presence-flag naming |
There was a problem hiding this comment.
P3: Stale line reference in D5: related_node.py:189 points to display_label, not initialized. The initialized property is at line 182.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ihs-138-store-merge/decisions.md, line 133:
<comment>Stale line reference in D5: `related_node.py:189` points to `display_label`, not `initialized`. The `initialized` property is at line 182.</comment>
<file context>
@@ -0,0 +1,187 @@
+
+---
+
+## D5 - Internal presence-flag naming
+
+**Question.** Name for the new "present in this response" flag on `Attribute` and
</file context>
There was a problem hiding this comment.
Invalid - the reference was accurate when the decision was recorded: at that commit, related_node.py:189 was the return bool(self.id) or bool(self.hfid) body of initialized. The implementation landed after the decision sheet was written, shifting line numbers. decisions.md is a point-in-time record of the sign-off, so its line references are intentionally left as they were at decision time; the current behaviour is documented in the code itself and in plan.md section 13.
There was a problem hiding this comment.
You're right — the reference was accurate at decision time, and decisions.md is intentionally a point-in-time sign-off record. The parent comment was too broad for this PR; the current behavior is documented in the code and in plan.md section 13.
github-actions
Bot
added
the
type/documentation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## infrahub-develop #1120 +/- ##
====================================================
+ Coverage 82.16% 82.54% +0.38%
====================================================
Files 138 138
Lines 11897 12126 +229
Branches 1784 1837 +53
====================================================
+ Hits 9775 10010 +235
+ Misses 1572 1567 -5
+ Partials 550 549 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
ogenstad
changed the title
There was a problem hiding this comment.
4 issues found across 20 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ihs-138-store-merge/decisions.md">
<violation number="1" location="dev/specs/ihs-138-store-merge/decisions.md:133">
P3: Stale line reference in D5: `related_node.py:189` points to `display_label`, not `initialized`. The `initialized` property is at line 182.</violation>
</file>
<file name="infrahub_sdk/store.py">
<violation number="1" location="infrahub_sdk/store.py:71">
P1: **CoreNode/CoreNodeSync merge path silently loses data or crashes.** The store's `set()` accepts `CoreNode | CoreNodeSync` objects and attempts merge logic when a UUID is already registered. However, `CoreNodeBase._merge()` (protocols_base.py:214) has a no-op body (`...`), and `CoreNodeBase.get_kind()` (protocols_base.py:199) raises `NotImplementedError`. Neither `CoreNode` nor `CoreNodeSync` override these methods. This means:
- If a `CoreNode`/`CoreNodeSync` is stored and a re-fetch tries to merge: the `get_kind()` call on `existing` raises `NotImplementedError`, crashing the store population.
- If `get_kind()` were somehow satisfied: the `_merge()` no-op would silently discard all re-fetched field data while keeping the stale stored object.
Since `_query_nodes` currently produces `InfrahubNode`/`InfrahubNodeSync` objects, this path may not be triggered today — but the type annotations declare it as supported, so it's a latent correctness bug that will bite as soon as any caller stores a `CoreNode`/`CoreNodeSync`.
**Recommendation**: Either (a) implement `_merge` and `get_kind` on `CoreNode`/`CoreNodeSync` — or (b) tighten `NodeStoreBranch.set()` to only merge for concrete merge-supporting types, evicting CoreNode objects instead of calling `_merge`/`get_kind` on them.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| 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)) |
There was a problem hiding this comment.
P1: CoreNode/CoreNodeSync merge path silently loses data or crashes. The store's set() accepts CoreNode | CoreNodeSync objects and attempts merge logic when a UUID is already registered. However, CoreNodeBase._merge() (protocols_base.py:214) has a no-op body (...), and CoreNodeBase.get_kind() (protocols_base.py:199) raises NotImplementedError. Neither CoreNode nor CoreNodeSync override these methods. This means:
- If a
CoreNode/CoreNodeSyncis stored and a re-fetch tries to merge: theget_kind()call onexistingraisesNotImplementedError, crashing the store population. - If
get_kind()were somehow satisfied: the_merge()no-op would silently discard all re-fetched field data while keeping the stale stored object.
Since _query_nodes currently produces InfrahubNode/InfrahubNodeSync objects, this path may not be triggered today — but the type annotations declare it as supported, so it's a latent correctness bug that will bite as soon as any caller stores a CoreNode/CoreNodeSync.
Recommendation: Either (a) implement _merge and get_kind on CoreNode/CoreNodeSync — or (b) tighten NodeStoreBranch.set() to only merge for concrete merge-supporting types, evicting CoreNode objects instead of calling _merge/get_kind on them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/store.py, line 71:
<comment>**CoreNode/CoreNodeSync merge path silently loses data or crashes.** The store's `set()` accepts `CoreNode | CoreNodeSync` objects and attempts merge logic when a UUID is already registered. However, `CoreNodeBase._merge()` (protocols_base.py:214) has a no-op body (`...`), and `CoreNodeBase.get_kind()` (protocols_base.py:199) raises `NotImplementedError`. Neither `CoreNode` nor `CoreNodeSync` override these methods. This means:
- If a `CoreNode`/`CoreNodeSync` is stored and a re-fetch tries to merge: the `get_kind()` call on `existing` raises `NotImplementedError`, crashing the store population.
- If `get_kind()` were somehow satisfied: the `_merge()` no-op would silently discard all re-fetched field data while keeping the stale stored object.
Since `_query_nodes` currently produces `InfrahubNode`/`InfrahubNodeSync` objects, this path may not be triggered today — but the type annotations declare it as supported, so it's a latent correctness bug that will bite as soon as any caller stores a `CoreNode`/`CoreNodeSync`.
**Recommendation**: Either (a) implement `_merge` and `get_kind` on `CoreNode`/`CoreNodeSync` — or (b) tighten `NodeStoreBranch.set()` to only merge for concrete merge-supporting types, evicting CoreNode objects instead of calling `_merge`/`get_kind` on them.</comment>
<file context>
@@ -40,24 +40,88 @@ def __init__(self, name: str) -> None:
+ 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))
+ node = existing
+ else:
</file context>
There was a problem hiding this comment.
Invalid in practice, but it prompted a hardening (604375d). CoreNode/CoreNodeSync are typing facades that describe runtime InfrahubNode/InfrahubNodeSync objects for the kind= overloads - they are never instantiated anywhere in the codebase (verified), and a hypothetical raw instance would already fail at node._internal_id on the first line of set(), before any merge logic runs; that is pre-existing and by design. The store's union type includes them because typed SchemaType results flow through it, not because facade instances are supported. That said, the comment exposed a real inconsistency: get_kind() raises NotImplementedError while the _merge stub was a silent ... - the silent-data-loss arm of the scenario. The stub now raises NotImplementedError too, so the failure mode is loud even in theory. (The cast("Any", node) shown in the file context is also gone - the merge seam is now fully typed via CoreNodeBase declarations.)
There was a problem hiding this comment.
The facade-instantiation path was too broad for this PR; those types aren’t actually instantiated, and raw instances would fail earlier. The real issue was the inconsistent stubs, and that part is now hardened because _merge also raises NotImplementedError.
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ihs-138-store-merge/decisions.md">
<violation number="1" location="dev/specs/ihs-138-store-merge/decisions.md:273">
P2: Table separator row uses `--` (two dashes) per column, which is not valid GFM table syntax — the table will not render as a table. Each column separator needs at least 3 dashes (e.g., `|---|`).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ## Summary table | ||
|
|
||
| | ID | Decision | Outcome | | ||
| | -- | -------- | ------- | |
There was a problem hiding this comment.
P2: Table separator row uses -- (two dashes) per column, which is not valid GFM table syntax — the table will not render as a table. Each column separator needs at least 3 dashes (e.g., |---|).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ihs-138-store-merge/decisions.md, line 273:
<comment>Table separator row uses `--` (two dashes) per column, which is not valid GFM table syntax — the table will not render as a table. Each column separator needs at least 3 dashes (e.g., `|---|`).</comment>
<file context>
@@ -270,7 +270,7 @@ its results.
| 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 |
</file context>
| else: | ||
| stored_data[name] = incoming_value |
There was a problem hiding this comment.
The else branch assigns the incoming dict by reference, so the store's _data can alias the snapshot's _data. Should dict(incoming_value) be used instead?
| Field-presence sets are attached to every attribute and relationship the SDK | ||
| builds, and all objects produced by the same query carry identical sets. Sharing | ||
| one instance per distinct set keeps the per-object overhead at pointer size | ||
| instead of a full frozenset (~700 bytes) each. The cache is unbounded but only |
There was a problem hiding this comment.
I don't know where the number comes from, maybe it is not necessary to have it in the docstring.
| ## 8. Rollout / PR strategy | ||
|
|
||
| Recommended: **one PR, layered commits** (stages 1-6), so reviewers see how the | ||
| presence flag feeds the merge and the bug fix lands atomically. The presence flag | ||
| is inert on its own, which is why splitting along the flag/merge seam is a poor | ||
| idea. | ||
|
|
||
| Acceptable alternative if the ticket fix must ship sooner: split along the | ||
| relationship/attribute seam. | ||
|
|
||
| - PR 1: stage 2 (relationship merge) - fixes IHS-138 literally, uses existing | ||
| `initialized`, lowest risk. | ||
| - PR 2: stages 1 + 3 + 4 (attribute presence flag, attribute merge, escape hatch) | ||
| - the generalization plus the debatable policy calls. | ||
|
|
||
| Do not split along the flag/merge seam (flag PR then merge PR): the flag PR would | ||
| be unexplainable dead code. | ||
|
|
||
| ## 10. Failure scenarios, lurking bugs, and gaps |
There was a problem hiding this comment.
Seems like section 9 was removed but things did not get renumbered
| ### C8 - `RelatedNode.initialized` means "has a peer," not "was fetched" | ||
|
|
||
| `RelationshipManager.initialized` is `data is not None` (a true fetched signal), | ||
| but `RelatedNode.initialized` is `bool(self.id) or bool(self.hfid)` | ||
| (`related_node.py:189`) - "has a peer." A fetched-but-empty cardinality-one | ||
| relationship (move to root, optional relationship cleared) reports | ||
| `initialized == False`, indistinguishable from "not fetched." Gating the merge on | ||
| it would keep a stale `parent` after a move-to-root - the opposite of expected. | ||
| Fix: add a presence flag to `RelatedNode` (Stage 1) and gate the merge on it. This | ||
| is the single most likely "looks correct, ships a bug" mistake in this work. | ||
|
|
||
| ### C7 - Minor | ||
|
|
||
| - Attribute metadata staleness on wholesale `Attribute` swap (see section 3 caveat). |
There was a problem hiding this comment.
The bot definitely has troubles with numbers :D
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
this certainly sounds and looks like a big improvement, but it hesitate to approve it until someone takes full responsibility for it
3 participants
Why
Querying the same node more than once silently dropped data from the client store. The store keyed objects by a random per-object id, so the latest query fully replaced the stored node: a shallow re-fetch (for example a node returned as a related node of another query) lost attributes and relationships loaded by an earlier, deeper query, and left duplicate entries behind (IHS-138).
Closes #413
What changed
Behavioral changes (all called out in the changelog and the store guide):
None), fields it did not request keep their stored value, and cardinality-many member lists are replaced, never unioned. Fixes the reported bug and the symmetric attribute case.client.get(id) is client.store.get(id)is no longer true (they still compare equal), andstore.get()hands out one living object per node that later fetches update in place.atinstant. Same-timestamp queries get full store functionality; a mismatching query skips the store with a warning instead of blending data from different points in time (pre-1.23 it silently overwrote).save()/create()/update()resets in-memory mutation tracking, so saved fields refresh from later fetches instead of being protected as pending local edits forever. Unsaved local edits still always win over a re-fetch and keep their pending markers through a merge.merge=Falseonget/all/filtersandstore.set(), globally via the newConfig.store_merge(INFRAHUB_STORE_MERGE), which restores the pre-1.23 behaviour.Implementation notes:
is_fetchedpresence signal (key-presence in the response, so fetched-but-empty is distinguishable from not-queried) and owns its own_merge:Attribute,RelatedNodeBase,RelationshipManagerBase.ConvertObjectType) replaces the store entry wholesale; merging across two schemas is incoherent.set()/_evict()stay O(1) per call, and the presence sets are interned (objects from the same query shape share one frozenset).dev/specs/ihs-138-store-merge/(plan section 13 and decisions D1-D9 record what changed during implementation and why).What stayed the same: save payloads are unaffected by the presence flags (serialization keys on mutation, never on presence);
merge=Falsescope is the node entry only; the store remains partitioned by branch.Suggested review order
infrahub_sdk/node/attribute.py,related_node.py,relationship.py- presence flags and per-type_mergeinfrahub_sdk/node/node.py- node-level_merge,_reset_mutation_trackinginfrahub_sdk/store.py- merge/replace/kind-change paths, reverse indexes,atcontextinfrahub_sdk/client.py,config.py-merge=threading andstore_mergetests/unit/sdk/test_store_merge.py- 54 tests, one per decided behaviourdocs/docs/python-sdk/guides/store.mdx, changelog entriesHow to review
update()sends), the timestamp-coherence warning on mixedat/live workflows (D9 - those "worked", subtly wrong, before), and the same-peer identity gating inRelatedNodeBase._merge(D8).docs/docs/python-sdk/reference/config.mdxanddocs/docs/python-sdk/sdk_ref/**are regenerated; theclient.pychurn is mostly themerge=parameter threaded through all overloads (async and sync).How to test
The original reproduction from #413 (deep fetch of an interface, then
client.allover circuit endpoints, thenstore.get(interface.id).device) now returns the device instead of raisingAttributeError.Impact & rollout
store_merge=Falserestores replace semantics). Four observable behaviour changes are enumerated inchangelog/+store-merge-behaviour.changed.md; no API removals,populate_storekeeps itsbool = Truesignature.Config.store_merge/INFRAHUB_STORE_MERGE(defaultTrue).infrahubctlintegration suites against the pre-release.Checklist
changelog/413.fixed.md,changelog/+store-merge-behaviour.changed.md)Summary by cubic
Fixes IHS-138: the SDK store now merges re-fetched nodes by UUID instead of overwriting, preserving previously fetched fields and preventing duplicates in
client.store. Adds per-callmergecontrols and a globalConfig.store_mergeopt‑out; includes timestamp-coherent caching, docs, and expanded tests.Bug Fixes
Attribute.is_fetched,RelatedNode.is_fetched,Relationship.is_fetched(alias ofinitialized) to tell fetched-empty from not-requested; each field type owns its merge.store.get()returns the same live object that updates in place; query methods return per-query snapshots; local unsaved edits win; successfulsave()/create()/update()resets mutation tracking.atcontext; mismatched timestamps skip the store with a warning to avoid blending data from different points in time.mergeonget/all/filtersandstore.set(); globalConfig.store_merge(INFRAHUB_STORE_MERGE); reverse indexes and interned presence sets keepset()O(1) and memory stable.Migration
merge=Falseper call or setConfig.store_merge=Falseto restore full replace semantics.Written for commit 4fa86f6. Summary will update on new commits.