Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
a89b8a2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bc8ba092.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}
Contributor
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 |
Contributor
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>
Contributor
Author
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.
Contributor
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 86.28% 86.34% +0.06%
====================================================
Files 149 149
Lines 14541 14586 +45
Branches 1994 2023 +29
====================================================
+ Hits 12547 12595 +48
+ Misses 1431 1425 -6
- Partials 563 566 +3
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
Contributor
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)) |
Contributor
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>
Contributor
Author
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.)
Contributor
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.
Contributor
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 | | ||
| | -- | -------- | ------- | |
Contributor
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>
ogenstad
marked this pull request as ready for review
July 5, 2026 17:30
gmazoyer
reviewed
Jul 6, 2026
Comment on lines
+372
to
+373
| else: | ||
| stored_data[name] = incoming_value |
Contributor
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 |
Contributor
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.
Comment on lines
+406
to
+424
| ## 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 |
Contributor
There was a problem hiding this comment.
Seems like section 9 was removed but things did not get renumbered
Comment on lines
+499
to
+512
| ### 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). |
Contributor
There was a problem hiding this comment.
The bot definitely has troubles with numbers :D
ajtmccarty
reviewed
Jul 6, 2026
ajtmccarty
left a comment
ajtmccarty
left a comment
Contributor
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
…y override Rebasing onto infrahub-develop picked up two upstream changes this branch predates. ty 0.0.74 now rejects the dual-flavour test pattern, where the client, the store and the node class are correlated unions the type system cannot tie together. Scope invalid-argument-type to test_store_merge.py, matching how test_store.py and the other dual-client test files are already handled. 1.23.0 through 1.23.2 have since shipped without the store merge behaviour, so the guide, the changelog and the store_merge description now name 1.24.0 instead. Reference docs regenerated.
ogenstad
marked this pull request as draft
September 18, 2026 11:11
ogenstad
force-pushed
the
pog-store-merge-ihs-138
branch
from
September 18, 2026 11:11
4fa86f6 to
5035b71
Compare
Contributor
There was a problem hiding this comment.
15 issues found across 21 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="docs/docs/python-sdk/guides/store.mdx">
<violation number="1" location="docs/docs/python-sdk/guides/store.mdx:69">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The guide says the store remembers “the union of everything it has seen,” but fetched fields overwrite old values and cardinality-many relationship lists replace prior members. Describe this as the merged view of fetched fields instead of a union, so users do not expect removed peers or overwritten values to remain cached.</violation>
<violation number="2" location="docs/docs/python-sdk/guides/store.mdx:69">
P3: The query result is not always a per-query snapshot: the first population returns the same object that `store.get()` stores. Qualify this statement for re-fetches only.</violation>
</file>
<file name="tests/unit/sdk/test_store_merge.py">
<violation number="1" location="tests/unit/sdk/test_store_merge.py:32">
P3: Module-wide `can_send_already_matched_responses=True` makes every already-consumed mock response reusable, so unexpected extra or reordered requests silently resolve to the wrong payload instead of raising. Several tests (e.g. `test_query_merges_into_store_and_returns_per_query_object`, `test_consistent_at_queries_populate_and_merge`, the two timestamp tests) register two distinct responses matched only by the same `X-Infrahub-Tracker` header and rely on exact call order and count; with this flag, a `get()` issuing one query too many would silently reuse the last response and the test could pass or fail on the wrong data. Only the prefetch test needs reuse, and it marks its response `is_reusable=True` explicitly. Recommend scoping the reuse opt-in per response instead of setting it module-wide.</violation>
</file>
<file name="docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/relationship.mdx">
<violation number="1" location="docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/relationship.mdx:32">
P2: The reference documents `is_fetched` as callable, but `RelationshipManagerBase.is_fetched` is a property. Users following this signature will call `manager.is_fetched()` and get a `TypeError`; document it as the boolean attribute `is_fetched` under **Attributes**, like `Attribute` and `RelatedNode`.</violation>
</file>
<file name="docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdx">
<violation number="1" location="docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdx:25">
P3: When callers construct `Attribute` directly, `is_fetched` defaults to `True` even though no response was involved, so this description gives the flag a stronger provenance guarantee than the implementation provides. Document the manual-construction default and that omitted response fields are `False` so callers do not use this as an authoritative response-origin signal.</violation>
</file>
<file name="infrahub_sdk/client.py">
<violation number="1" location="infrahub_sdk/client.py:751">
P2: Existing positional calls to `get`, `all`, or `filters` now bind their old `fragment`/`offset` argument to `merge`, silently changing query and store behavior. Append `merge` after the existing parameters, or make it keyword-only, across every overload and implementation.</violation>
</file>
<file name="infrahub_sdk/store.py">
<violation number="1" location="infrahub_sdk/store.py:336">
P2: When a node is added through `store.set()` before a historical query, the branch is not marked as live, so `_reserve_at_context()` later accepts the historical timestamp and mixes data from different contexts. Ensure direct store population is context-aware or reject it when the branch already holds a different timestamp context.</violation>
</file>
<file name="infrahub_sdk/node/relationship.py">
<violation number="1" location="infrahub_sdk/node/relationship.py:138">
P2: When the same cardinality-many peers are re-fetched without edge properties, this assignment discards properties and relationship metadata previously loaded for those peers. Merge matching peers through `RelatedNodeBase._merge()` instead of replacing their wrappers, while still using the incoming list to remove peers deleted on the server. According to linked Jira issue IHS-138, previously fetched relationship information must remain accessible after a later query.</violation>
</file>
<file name="dev/specs/ihs-138-store-merge/decisions.md">
<violation number="1" location="dev/specs/ihs-138-store-merge/decisions.md:10">
P3: The "Already settled" version line still names SDK 1.23.0, but this change now ships in 1.24.0: 1.23.0 through 1.23.2 have already been released without the store merge behavior (per the retarget commit in this PR and CHANGELOG.md). Update the settled version to 1.24.0, and align the release-gate references at lines 291 and 310 that still speak of the "1.23.0 pre-release", so the spec does not contradict the shipped plan.</violation>
</file>
<file name="infrahub_sdk/node/related_node.py">
<violation number="1" location="infrahub_sdk/node/related_node.py:157">
P2: When a relationship response contains `is_protected=False`, this truthiness check falls through and stores `None` despite marking the property as fetched. Test for a non-`None` value (or key presence) so merge preserves the server's `False` value.</violation>
<violation number="2" location="infrahub_sdk/node/related_node.py:306">
P2: When a stored relationship is identified only by HFID and the re-fetch carries the peer's UUID (or the reverse), `incoming._id != self._id` compares `None` against a UUID and reports a peer change even when both sides name the same peer (matching hfids). The wholesale branch then copies the incoming hold-all wholesale — and because the incoming fetch typically has an empty `_fetched_properties`, all previously fetched edge properties (`source`, `owner`, `is_protected`, `updated_at`) and `relationship_metadata` are nulled out of the store. That is exactly the kind of data loss this merge is meant to prevent (IHS-138). Treat ids as comparable only when both sides have one, and fall back to comparing the hfids (a unique peer identifier) when either side lacks an id; additionally adopt `incoming._id` in the same-peer branch when the stored side had none.</violation>
</file>
<file name="infrahub_sdk/node/node.py">
<violation number="1" location="infrahub_sdk/node/node.py:1772">
P2: After a successful update, the next update still resends saved fields because `_reset_mutation_tracking()` clears mutation flags without advancing `_data` to the persisted state. Refresh the raw baseline from the successful mutation response or otherwise make the diff baseline reflect the saved values before clearing tracking.</violation>
</file>
<file name="dev/specs/ihs-138-store-merge/plan.md">
<violation number="1" location="dev/specs/ihs-138-store-merge/plan.md:6">
P3: The spec targets SDK 1.23.0 and calls the old behaviour "pre-1.23.0", but this feature ships in 1.24.0. The PR description states "Target SDK version: 1.24.0", and the repo's own docs all describe the change as landing in 1.24.0 (store.mdx "Behaviour change in version 1.24.0" / "pre-1.24.0", config.mdx "the pre-1.24.0 behaviour", and the changelog fragment +store-merge-behaviour.changed.md "pre-1.24.0 versions did"). CHANGELOG.md shows 1.23.0 (2026-08-19) and 1.23.1 (2026-08-28) were already released without this feature. Update the version references across plan.md and decisions.md so the migration note, changelog callout, and the pending 1.23.0 pre-release gate name the release that actually carries the change.</violation>
<violation number="2" location="dev/specs/ihs-138-store-merge/plan.md:258">
P3: Stage 1a and Stage 3 name the new attribute presence flag `initialized`, but D5 (recorded in section 11 and decisions.md) settled on `is_fetched` as a uniform accessor across `Attribute`, `RelatedNode`, and `RelationshipManager`, and the shipped code uses `is_fetched` (`Attribute.__init__`, `RelatedNodeBase.__init__`, and the `is_fetched` alias in RelationshipManager). Since the plan claims status "implemented" with all decisions honoured, the stage text documents a name the code never used. Update Stage 1a/Stage 3 to `is_fetched` so a future reader does not look for `Attribute.initialized`.</violation>
<violation number="3" location="dev/specs/ihs-138-store-merge/plan.md:293">
P3: Stage 3 prescribes a whole-object `Attribute` swap, contradicting the finalized design in the same document: section 3 (grill 4) and section 12 item 4 require merging field-by-field into the existing `Attribute` so a value-only re-fetch does not null previously fetched `source`/`owner`/`is_protected` metadata, and section 13 lists the shipped `Attribute._merge` (which merges field-by-field, gated on `value`/`value_has_been_mutated`). A reader treating this "implemented" plan as the record of what shipped would get the merge semantics wrong. Rewrite the bullet to match the field-by-field rule.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| ### Returned objects compared to stored objects | ||
|
|
||
| Queries hand you what you asked for; the store remembers the union of everything it has seen. The object returned by `get`, `filters` or `all` reflects only that query, while `store.get()` returns the merged view, so the two can differ. They compare equal (`==`, equality is based on the node id) but they are not the same Python object (`is`). |
Contributor
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The guide says the store remembers “the union of everything it has seen,” but fetched fields overwrite old values and cardinality-many relationship lists replace prior members. Describe this as the merged view of fetched fields instead of a union, so users do not expect removed peers or overwritten values to remain cached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/python-sdk/guides/store.mdx, line 69:
<comment>The guide says the store remembers “the union of everything it has seen,” but fetched fields overwrite old values and cardinality-many relationship lists replace prior members. Describe this as the merged view of fetched fields instead of a union, so users do not expect removed peers or overwritten values to remain cached.</comment>
<file context>
@@ -12,6 +12,72 @@ The store is mainly used for the internal working of the SDK. It is used to crea
+
+### Returned objects compared to stored objects
+
+Queries hand you what you asked for; the store remembers the union of everything it has seen. The object returned by `get`, `filters` or `all` reflects only that query, while `store.get()` returns the merged view, so the two can differ. They compare equal (`==`, equality is based on the node id) but they are not the same Python object (`is`).
+
+### The store hands out living objects
</file context>
Suggested change
| Queries hand you what you asked for; the store remembers the union of everything it has seen. The object returned by `get`, `filters` or `all` reflects only that query, while `store.get()` returns the merged view, so the two can differ. They compare equal (`==`, equality is based on the node id) but they are not the same Python object (`is`). | |
| Queries hand you what you asked for; the store remembers the merged view of everything it has fetched. The object returned by `get`, `filters` or `all` reflects only that query, while `store.get()` returns the merged view, so the two can differ. They compare equal (`==`, equality is based on the node id) but they are not the same Python object (`is`). |
| #### `is_fetched` | ||
|
|
||
| ```python | ||
| is_fetched(self) -> bool |
Contributor
There was a problem hiding this comment.
P2: The reference documents is_fetched as callable, but RelationshipManagerBase.is_fetched is a property. Users following this signature will call manager.is_fetched() and get a TypeError; document it as the boolean attribute is_fetched under Attributes, like Attribute and RelatedNode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/relationship.mdx, line 32:
<comment>The reference documents `is_fetched` as callable, but `RelationshipManagerBase.is_fetched` is a property. Users following this signature will call `manager.is_fetched()` and get a `TypeError`; document it as the boolean attribute `is_fetched` under **Attributes**, like `Attribute` and `RelatedNode`.</comment>
<file context>
@@ -26,6 +26,22 @@ members are not loaded and editing is not allowed.
+#### `is_fetched`
+
+```python
+is_fetched(self) -> bool
+```
+
</file context>
| include: list[str] | None = None, | ||
| exclude: list[str] | None = None, | ||
| populate_store: bool = True, | ||
| merge: bool | None = None, |
Contributor
There was a problem hiding this comment.
P2: Existing positional calls to get, all, or filters now bind their old fragment/offset argument to merge, silently changing query and store behavior. Append merge after the existing parameters, or make it keyword-only, across every overload and implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/client.py, line 751:
<comment>Existing positional calls to `get`, `all`, or `filters` now bind their old `fragment`/`offset` argument to `merge`, silently changing query and store behavior. Append `merge` after the existing parameters, or make it keyword-only, across every overload and implementation.</comment>
<file context>
@@ -742,6 +748,7 @@ async def get(
include: list[str] | None = None,
exclude: list[str] | None = None,
populate_store: bool = True,
+ merge: bool | None = None,
fragment: bool = False,
prefetch_relationships: bool = False,
</file context>
| self._branches[branch] = NodeStoreBranch(name=branch) | ||
|
|
||
| self._branches[branch].set(node=node, key=key) | ||
| self._branches[branch].set(node=node, key=key, merge=self._default_merge if merge is None else merge) |
Contributor
There was a problem hiding this comment.
P2: When a node is added through store.set() before a historical query, the branch is not marked as live, so _reserve_at_context() later accepts the historical timestamp and mixes data from different contexts. Ensure direct store population is context-aware or reject it when the branch already holds a different timestamp context.
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 336:
<comment>When a node is added through `store.set()` before a historical query, the branch is not marked as live, so `_reserve_at_context()` later accepts the historical timestamp and mixes data from different contexts. Ensure direct store population is context-aware or reject it when the branch already holds a different timestamp context.</comment>
<file context>
@@ -217,22 +281,59 @@ def __init__(self, default_branch: str | None = None) -> None:
self._branches[branch] = NodeStoreBranch(name=branch)
- self._branches[branch].set(node=node, key=key)
+ self._branches[branch].set(node=node, key=key, merge=self._default_merge if merge is None else merge)
def _get( # type: ignore[no-untyped-def]
</file context>
| keeps its pending-update marker. Callers are responsible for the higher-level | ||
| gates (``is_fetched`` on the incoming manager, ``has_update`` on this one). | ||
| """ | ||
| self.peers = list(incoming.peers) |
Contributor
There was a problem hiding this comment.
P2: When the same cardinality-many peers are re-fetched without edge properties, this assignment discards properties and relationship metadata previously loaded for those peers. Merge matching peers through RelatedNodeBase._merge() instead of replacing their wrappers, while still using the incoming list to remove peers deleted on the server. According to linked Jira issue IHS-138, previously fetched relationship information must remain accessible after a later query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/node/relationship.py, line 138:
<comment>When the same cardinality-many peers are re-fetched without edge properties, this assignment discards properties and relationship metadata previously loaded for those peers. Merge matching peers through `RelatedNodeBase._merge()` instead of replacing their wrappers, while still using the incoming list to remove peers deleted on the server. According to linked Jira issue IHS-138, previously fetched relationship information must remain accessible after a later query.</comment>
<file context>
@@ -113,6 +127,18 @@ def is_from_profile(self) -> bool:
+ keeps its pending-update marker. Callers are responsible for the higher-level
+ gates (``is_fetched`` on the incoming manager, ``has_update`` on this one).
+ """
+ self.peers = list(incoming.peers)
+ self.initialized = True
+ self._has_update = incoming._has_update
</file context>
Suggested change
| self.peers = list(incoming.peers) | |
| existing_by_id = {peer.id: peer for peer in self.peers if peer.id} | |
| existing_by_hfid = {tuple(peer.hfid): peer for peer in self.peers if not peer.id and peer.hfid} | |
| merged_peers = [] | |
| for peer in incoming.peers: | |
| existing = existing_by_id.get(peer.id) if peer.id else existing_by_hfid.get(tuple(peer.hfid or ())) | |
| if existing is not None: | |
| existing._merge(peer) | |
| merged_peers.append(existing) | |
| else: | |
| merged_peers.append(peer) | |
| self.peers = merged_peers |
|
|
||
| - Consume `Attribute.initialized` in the merge: take the incoming attribute when | ||
| present and not locally mutated; otherwise keep the stored one. | ||
| - Swap the whole `Attribute` object on take so metadata refreshes with the value. |
Contributor
There was a problem hiding this comment.
P3: Stage 3 prescribes a whole-object Attribute swap, contradicting the finalized design in the same document: section 3 (grill 4) and section 12 item 4 require merging field-by-field into the existing Attribute so a value-only re-fetch does not null previously fetched source/owner/is_protected metadata, and section 13 lists the shipped Attribute._merge (which merges field-by-field, gated on value/value_has_been_mutated). A reader treating this "implemented" plan as the record of what shipped would get the merge semantics wrong. Rewrite the bullet to match the field-by-field rule.
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/plan.md, line 293:
<comment>Stage 3 prescribes a whole-object `Attribute` swap, contradicting the finalized design in the same document: section 3 (grill 4) and section 12 item 4 require merging field-by-field into the existing `Attribute` so a value-only re-fetch does not null previously fetched `source`/`owner`/`is_protected` metadata, and section 13 lists the shipped `Attribute._merge` (which merges field-by-field, gated on `value`/`value_has_been_mutated`). A reader treating this "implemented" plan as the record of what shipped would get the merge semantics wrong. Rewrite the bullet to match the field-by-field rule.</comment>
<file context>
@@ -0,0 +1,676 @@
+
+- Consume `Attribute.initialized` in the merge: take the incoming attribute when
+ present and not locally mutated; otherwise keep the stored one.
+- Swap the whole `Attribute` object on take so metadata refreshes with the value.
+
+### Stage 4 - Explicit replace escape hatch
</file context>
Suggested change
| - Swap the whole `Attribute` object on take so metadata refreshes with the value. | |
| - Merge field-by-field into the existing `Attribute` (value, then each property/metadata sub-field only when the re-fetch carried it), so a value-only re-fetch never nulls cached `source`/`owner`/`is_protected` etc. |
| - **Ticket:** [IHS-138](https://opsmill.atlassian.net/browse/IHS-138) (GitHub [#413](https://github.com/opsmill/infrahub-sdk-python/issues/413)) | ||
| - **Priority:** High | ||
| - **Affected versions:** observed on SDK 1.12.1 | ||
| - **Target version:** SDK 1.23.0 (ships alongside Infrahub 1.11.0) |
Contributor
There was a problem hiding this comment.
P3: The spec targets SDK 1.23.0 and calls the old behaviour "pre-1.23.0", but this feature ships in 1.24.0. The PR description states "Target SDK version: 1.24.0", and the repo's own docs all describe the change as landing in 1.24.0 (store.mdx "Behaviour change in version 1.24.0" / "pre-1.24.0", config.mdx "the pre-1.24.0 behaviour", and the changelog fragment +store-merge-behaviour.changed.md "pre-1.24.0 versions did"). CHANGELOG.md shows 1.23.0 (2026-08-19) and 1.23.1 (2026-08-28) were already released without this feature. Update the version references across plan.md and decisions.md so the migration note, changelog callout, and the pending 1.23.0 pre-release gate name the release that actually carries the change.
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/plan.md, line 6:
<comment>The spec targets SDK 1.23.0 and calls the old behaviour "pre-1.23.0", but this feature ships in 1.24.0. The PR description states "Target SDK version: 1.24.0", and the repo's own docs all describe the change as landing in 1.24.0 (store.mdx "Behaviour change in version 1.24.0" / "pre-1.24.0", config.mdx "the pre-1.24.0 behaviour", and the changelog fragment +store-merge-behaviour.changed.md "pre-1.24.0 versions did"). CHANGELOG.md shows 1.23.0 (2026-08-19) and 1.23.1 (2026-08-28) were already released without this feature. Update the version references across plan.md and decisions.md so the migration note, changelog callout, and the pending 1.23.0 pre-release gate name the release that actually carries the change.</comment>
<file context>
@@ -0,0 +1,676 @@
+- **Ticket:** [IHS-138](https://opsmill.atlassian.net/browse/IHS-138) (GitHub [#413](https://github.com/opsmill/infrahub-sdk-python/issues/413))
+- **Priority:** High
+- **Affected versions:** observed on SDK 1.12.1
+- **Target version:** SDK 1.23.0 (ships alongside Infrahub 1.11.0)
+- **Status:** implemented (2026-07-02); hardened after code review (2026-07-04, section 13)
+
</file context>
|
|
||
| ### Returned objects compared to stored objects | ||
|
|
||
| Queries hand you what you asked for; the store remembers the union of everything it has seen. The object returned by `get`, `filters` or `all` reflects only that query, while `store.get()` returns the merged view, so the two can differ. They compare equal (`==`, equality is based on the node id) but they are not the same Python object (`is`). |
Contributor
There was a problem hiding this comment.
P3: The query result is not always a per-query snapshot: the first population returns the same object that store.get() stores. Qualify this statement for re-fetches only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/python-sdk/guides/store.mdx, line 69:
<comment>The query result is not always a per-query snapshot: the first population returns the same object that `store.get()` stores. Qualify this statement for re-fetches only.</comment>
<file context>
@@ -12,6 +12,72 @@ The store is mainly used for the internal working of the SDK. It is used to crea
+
+### Returned objects compared to stored objects
+
+Queries hand you what you asked for; the store remembers the union of everything it has seen. The object returned by `get`, `filters` or `all` reflects only that query, while `store.get()` returns the merged view, so the two can differ. They compare equal (`==`, equality is based on the node id) but they are not the same Python object (`is`).
+
+### The store hands out living objects
</file context>
Suggested change
| Queries hand you what you asked for; the store remembers the union of everything it has seen. The object returned by `get`, `filters` or `all` reflects only that query, while `store.get()` returns the merged view, so the two can differ. They compare equal (`==`, equality is based on the node id) but they are not the same Python object (`is`). | |
| Queries hand you what you asked for; the store remembers the union of everything it has seen. On the first population, the query result is also the canonical store object; on a later re-fetch, the returned object reflects only that query while `store.get()` returns the merged view. They compare equal (`==`, equality is based on the node id), but re-fetch results and the existing store object are not the same Python object (`is`). |
Four confirmed correctness bugs, plus the actionable review feedback on the pull request. - A locally cleared cardinality-one relationship reached the server but not the store. Mutation tracking now carries two lifetimes: the payload markers stay set so a later save still re-asserts an explicit clear or peer set, while a new _has_unsaved_change drives the store merge and is cleared once the value is persisted. - The timestamp-coherence guard covered query population only, so a save left the branch unclaimed and a later historical query merged onto live data. NodeStoreBranch now owns the invariant and every write reserves through it. - Cardinality-many peers were replaced wholesale, dropping edge properties already loaded for members still in the set. Peers present on both sides are now merged. - Peer identity is compared through the public accessors, so an edge hydrated from a peer object, or one naming the peer by hfid against an id-carrying re-fetch, is no longer misread as a peer change. - _data merges nested payload dicts at every level and copies containers rather than adopting them, so the update() baseline neither drops edge properties the live object keeps nor aliases the per-query snapshot. The merge option moves to the end of the client query signatures: every option added to get/all/filters so far has been appended, and inserting one mid-signature would silently rebind existing positional callers. Making these keyword-only is tracked in #1372.
Contributor
There was a problem hiding this comment.
4 existing issues remain and no new issues found across 21 files
Requires human review: Auto-approval blocked because this review re-detected 4 unresolved issues already reported by Cubic.
Re-trigger cubic
Addresses the review feedback on the previous commit. _merge_raw_value only copied the top level when the stored side had no counterpart, so nested containers were still adopted from the per-query snapshot. It now recurses on that branch too, making the no-aliasing guarantee in its docstring actually hold. The aliasing test could not fail: two payloads always start with distinct dicts, so asserting identity proved nothing. It now mutates the snapshot after the merge and asserts the stored baseline is unchanged, which does discriminate against the previous behaviour. The timestamp-coherence tests exercised store.set() directly and so could not catch a broken claim in the mutation path. Two tests now go through save(): one asserting a save is refused against a historical store, one asserting a save claims the live context so a later historical query is refused. Documentation corrections: a directly assigned peer is not always detached (assigning the result of store.get() keeps the live object), a skipped query does not make .peer universally unavailable (already cached or already stored peers still resolve), and the returned-object identity guarantee also holds whenever the entry is replaced rather than merged.
Contributor
There was a problem hiding this comment.
2 existing issues remain and 1 new issue found across 21 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="infrahub_sdk/node/node.py">
<violation number="1" location="infrahub_sdk/node/node.py:424">
P1: This merge call drops mutation markers that `_reset_mutation_tracking()` deliberately preserves. Preserve the existing relationship markers, or combine them with the incoming markers, so a later save still reasserts persisted relationship edits.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
| elif stored_rel._has_unsaved_change: | ||
| return False | ||
| else: | ||
| stored_rel._merge(incoming_rel) |
Contributor
There was a problem hiding this comment.
P1: This merge call drops mutation markers that _reset_mutation_tracking() deliberately preserves. Preserve the existing relationship markers, or combine them with the incoming markers, so a later save still reasserts persisted relationship edits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At infrahub_sdk/node/node.py, line 424:
<comment>This merge call drops mutation markers that `_reset_mutation_tracking()` deliberately preserves. Preserve the existing relationship markers, or combine them with the incoming markers, so a later save still reasserts persisted relationship edits.</comment>
<file context>
@@ -297,6 +320,150 @@ def _get_request_context(self, request_context: RequestContext | None = None) ->
+ elif stored_rel._has_unsaved_change:
+ return False
+ else:
+ stored_rel._merge(incoming_rel)
+ return True
+
</file context>
The sticky payload markers were only sticky until the next fetch. _reset_mutation_tracking() deliberately leaves value_has_been_mutated, _peer_has_been_mutated and has_update set so a later save re-asserts the edit, but both relationship _merge methods then assigned those markers straight from the incoming copy. Any ordinary refetch of a node living in the store cleared them, and the save after that omitted the peer set or the explicit clear - exactly the failure the markers exist to prevent. They are now combined rather than replaced, so a refetch can add to them but never clear them. Attribute._merge was already correct. A cardinality-many relationship the response carried as null also read as never-fetched, because the manager inferred presence from the data instead of from key presence the way attributes and cardinality-one relationships do. The store therefore kept members the server had emptied - a regression, since replacing the node used to drop them. The manager now takes the same is_fetched signal, plumbed from all four construction sites (cardinality-many and hierarchical, async and sync). Also corrects the D3 decision record: per-call merge=False on get/all/filters is a third way to force replace, so "the sole way" was inaccurate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.24 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.24 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- 83 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).Rebase onto
infrahub-developThe branch was rebased onto
infrahub-developafter it had moved 475 commits ahead. Three files conflicted:infrahub_sdk/config.py(both sides inserted into the same field block, resolved by keepingstore_mergealongside the rewrittentimeoutand the newconnect_timeout) and two generated docs files, resolved by regenerating. The source delta is byte-for-byte identical to the pre-rebase branch, so nothing was lost or silently altered in the merge.Two upstream changes landed in the window and needed a response, both in the final commit:
ty0.0.74 now rejects the dual-flavour test pattern, where the client, the store and the node class are correlated unions the type system cannot tie together.tests/unit/sdk/test_store_merge.pygets a[[tool.ty.overrides]]entry forinvalid-argument-type, matching howtest_store.pyand the other dual-client test files are already handled. This is the one new suppression in the PR.store_mergedescription now name 1.24.0.Worth a look while reviewing:
_reset_mutation_tracking()(D7) now sits in_process_mutation_resultnext to the per-requestpriorityforwarding that upstream added to the same method. The two are independent, but they read together.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, andRelationshipManagerBase.is_fetched(a real signal, not an alias ofinitialized) tell fetched-empty from not-requested; cardinality-many relationships read it from key presence too, so anullresponse empties the stored members instead of looking never-fetched.store.get()returns the same live object that updates in place; query methods return per-query snapshots; merged payloads are deep-copied at every nesting level so the stored baseline never aliases a snapshot; local unsaved edits win; successful saves reset mutation tracking, with payload markers kept separate from_has_unsaved_change; relationship merges combine those sticky markers rather than replacing them, so a refetch never clears a pending re-assert.atcontext; every write (queries and saves) claims the context, and mismatched timestamps skip the store with a warning instead of blending data from different points in time.mergeonget/all/filtersandstore.set()(appended to signature ends so positional callers aren't rebound); globalConfig.store_merge(INFRAHUB_STORE_MERGE); reverse indexes keepset()O(1).Migration
merge=Falseper call or setConfig.store_merge=Falseto restore full replace semantics.Written for commit a89b8a2. Summary will update on new commits.