Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
ab6cdb4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://38539bf1.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.33% +0.04%
====================================================
Files 149 149
Lines 14541 14582 +41
Branches 1994 2023 +29
====================================================
+ Hits 12547 12589 +42
+ Misses 1431 1426 -5
- Partials 563 567 +4
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.
20 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>
<violation number="3" location="docs/docs/python-sdk/guides/store.mdx:73">
P2: When the same UUID arrives under a different node kind, the store replaces the entry wholesale, so earlier references do not receive later updates. Document this cross-kind replacement exception alongside the living-object guarantee.</violation>
<violation number="4" location="docs/docs/python-sdk/guides/store.mdx:75">
P3: `fetch()` does not always cache a detached peer: an initially fetched peer can be the canonical store object and receive later merges. Qualify this exception to existing store entries.</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>
<violation number="2" location="tests/unit/sdk/test_store_merge.py:530">
P3: The simulated riding-along peer is built from `location_schema` + `deep_location_data()`, so it is a `BuiltinLocation` node whose id is `LOCATION_ID` — the same UUID as the host location being stored. Attaching it to `primary_tag` (a `BuiltinTag` relationship) makes the stored graph self-referential: the location's tag relationship points at a location-typed object with the host's own id. A real prefetched peer would be a BuiltinTag node with a distinct id, so this fixture never exercises authentic peer identity/content and could mask bugs in the same-peer merge path (D8).</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="infrahub_sdk/node/node.py">
<violation number="1" location="infrahub_sdk/node/node.py:374">
P2: When a relationship refetch carries only some edge properties, this shallow merge drops the other properties from `_data` while `RelatedNodeBase._merge()` keeps them in memory. Recursively merge nested raw dictionaries so the update baseline matches the canonical relationship state.</violation>
<violation number="2" 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/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="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>
<file name="changelog/+store-merge-behaviour.changed.md">
<violation number="1" location="changelog/+store-merge-behaviour.changed.md:1">
P3: The changelog says every query result is a distinct snapshot, but initial population still returns the same object as the store entry. Limit this behavior change to re-fetches.</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>
| @@ -0,0 +1 @@ | |||
| The client store merge fix comes with four observable behaviour changes. First, the object returned by `get`, `filters` or `all` is a per-query snapshot and is no longer the same Python object as the store entry: `client.get(id) is client.store.get(id)` was previously true and now is false, although the two still compare equal (`==`). Second, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first population stamps the branch cache as live or as one `at` point in time, queries at the same timestamp use the store normally (a fully historical script gets complete store functionality), and a query at a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.24.0 versions did). Fourth, a successful `save()`, `create()` or `update()` now resets the node's in-memory mutation tracking: the persisted values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever. | |||
Contributor
There was a problem hiding this comment.
P3: The changelog says every query result is a distinct snapshot, but initial population still returns the same object as the store entry. Limit this behavior change to re-fetches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At changelog/+store-merge-behaviour.changed.md, line 1:
<comment>The changelog says every query result is a distinct snapshot, but initial population still returns the same object as the store entry. Limit this behavior change to re-fetches.</comment>
<file context>
@@ -0,0 +1 @@
+The client store merge fix comes with four observable behaviour changes. First, the object returned by `get`, `filters` or `all` is a per-query snapshot and is no longer the same Python object as the store entry: `client.get(id) is client.store.get(id)` was previously true and now is false, although the two still compare equal (`==`). Second, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first population stamps the branch cache as live or as one `at` point in time, queries at the same timestamp use the store normally (a fully historical script gets complete store functionality), and a query at a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.24.0 versions did). Fourth, a successful `save()`, `create()` or `update()` now resets the node's in-memory mutation tracking: the persisted values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever.
</file context>
Suggested change
| The client store merge fix comes with four observable behaviour changes. First, the object returned by `get`, `filters` or `all` is a per-query snapshot and is no longer the same Python object as the store entry: `client.get(id) is client.store.get(id)` was previously true and now is false, although the two still compare equal (`==`). Second, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first population stamps the branch cache as live or as one `at` point in time, queries at the same timestamp use the store normally (a fully historical script gets complete store functionality), and a query at a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.24.0 versions did). Fourth, a successful `save()`, `create()` or `update()` now resets the node's in-memory mutation tracking: the persisted values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever. | |
| The client store merge fix comes with four observable behaviour changes. First, on a re-fetch, the object returned by `get`, `filters` or `all` is a per-query snapshot rather than the existing store entry; the first population still returns the canonical object, and the two objects compare equal (`==`). Second, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first population stamps the branch cache as live or as one `at` point in time, queries at the same timestamp use the store normally (a fully historical script gets complete store functionality), and a query at a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.24.0 versions did). Fourth, a successful `save()`, `create()` or `update()` now resets the node's in-memory mutation tracking: the persisted values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever. |
|
|
||
| ### 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`). |
|
|
||
| The store keeps one object per node and mutates it in place when new data arrives. Every `store.get()` call for the same node returns the same object, and a later query that re-fetches the node refreshes the object that earlier references already point at. If you need a snapshot that does not change under you, keep the object returned by the query instead of the store copy. | ||
|
|
||
| One exception: a relationship peer resolved with `fetch()` or assigned directly is cached on the relationship itself and does not pick up later store merges. |
Contributor
There was a problem hiding this comment.
P3: fetch() does not always cache a detached peer: an initially fetched peer can be the canonical store object and receive later merges. Qualify this exception to existing store entries.
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 75:
<comment>`fetch()` does not always cache a detached peer: an initially fetched peer can be the canonical store object and receive later merges. Qualify this exception to existing store entries.</comment>
<file context>
@@ -12,6 +12,72 @@ The store is mainly used for the internal working of the SDK. It is used to crea
+
+The store keeps one object per node and mutates it in place when new data arrives. Every `store.get()` call for the same node returns the same object, and a later query that re-fetches the node refreshes the object that earlier references already point at. If you need a snapshot that does not change under you, keep the object returned by the query instead of the store copy.
+
+One exception: a relationship peer resolved with `fetch()` or assigned directly is cached on the relationship itself and does not pick up later store merges.
+
+### Staleness caveat
</file context>
Suggested change
| One exception: a relationship peer resolved with `fetch()` or assigned directly is cached on the relationship itself and does not pick up later store merges. | |
| One caveat: a relationship peer resolved with `fetch()` or assigned directly may be cached on the relationship itself and not pick up later store merges when a canonical store entry already existed; if `fetch()` creates the first entry, it points at the canonical store object. |
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 5 new 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="changelog/+store-merge-behaviour.changed.md">
<violation number="1" location="changelog/+store-merge-behaviour.changed.md:1">
P3: With `store_merge=False` or per-call `merge=False`, a re-fetch replaces the entry; kind changes also replace it even in merge mode. Qualify the identity guarantee to same-kind nodes using the default merge behavior, or document these exceptions.</violation>
</file>
<file name="docs/docs/python-sdk/guides/store.mdx">
<violation number="1" location="docs/docs/python-sdk/guides/store.mdx:81">
P3: Directly assigning a peer does not always create a detached copy: assigning the object returned by `store.get()` keeps that exact live store object. Distinguish `fetch()` results from direct assignment so users do not choose a snapshot strategy based on an incorrect guarantee.</violation>
<violation number="2" location="docs/docs/python-sdk/guides/store.mdx:295">
P3: A mismatching query does not make `.peer` universally unavailable: an already-cached peer can still be returned from the relationship or existing store. Say that newly fetched peers may be unavailable unless the peer was already cached, then recommend the identity fields for the unavailable case.</violation>
</file>
<file name="tests/unit/sdk/test_store_merge.py">
<violation number="1" location="tests/unit/sdk/test_store_merge.py:558">
P2: This assertion cannot detect aliasing because both payloads create separate `name` dictionaries before merging. Mutate `incoming._data["name"]` after `store.set()` and assert the stored baseline is unchanged.</violation>
<violation number="2" location="tests/unit/sdk/test_store_merge.py:980">
P2: This test never invokes `save()` or `update()`, so it cannot catch a broken timestamp claim in the mutation path. Perform a successful mutation, then verify a historical store write is refused.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 4 unresolved issues already reported by Cubic.
Re-trigger cubic
| store.set(node=node_class(client=client, schema=location_schema, data=deep_location_data())) | ||
|
|
||
| with pytest.warns(UserWarning, match="Not populating the store"): | ||
| assert store._reserve_at_context(at="2020-01-01T00:00:00Z", branch="main") is False |
Contributor
There was a problem hiding this comment.
P2: This test never invokes save() or update(), so it cannot catch a broken timestamp claim in the mutation path. Perform a successful mutation, then verify a historical store write is refused.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/sdk/test_store_merge.py, line 980:
<comment>This test never invokes `save()` or `update()`, so it cannot catch a broken timestamp claim in the mutation path. Perform a successful mutation, then verify a historical store write is refused.</comment>
<file context>
@@ -0,0 +1,1411 @@
+ store.set(node=node_class(client=client, schema=location_schema, data=deep_location_data()))
+
+ with pytest.warns(UserWarning, match="Not populating the store"):
+ assert store._reserve_at_context(at="2020-01-01T00:00:00Z", branch="main") is False
+
+
</file context>
|
|
||
| assert isinstance(stored_node._data, dict) | ||
| assert isinstance(incoming._data, dict) | ||
| assert stored_node._data["name"] is not incoming._data["name"] |
Contributor
There was a problem hiding this comment.
P2: This assertion cannot detect aliasing because both payloads create separate name dictionaries before merging. Mutate incoming._data["name"] after store.set() and assert the stored baseline is unchanged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/sdk/test_store_merge.py, line 558:
<comment>This assertion cannot detect aliasing because both payloads create separate `name` dictionaries before merging. Mutate `incoming._data["name"]` after `store.set()` and assert the stored baseline is unchanged.</comment>
<file context>
@@ -0,0 +1,1411 @@
+
+ assert isinstance(stored_node._data, dict)
+ assert isinstance(incoming._data, dict)
+ assert stored_node._data["name"] is not incoming._data["name"]
+
+
</file context>
| @@ -0,0 +1 @@ | |||
| The client store merge fix comes with four observable behaviour changes. First, the object returned by `get`, `filters` or `all` is a per-query snapshot and is no longer the same Python object as the store entry once that node has been fetched more than once: `client.get(id) is client.store.get(id)` was previously true and is now false after a re-fetch, although the two still compare equal (`==`). The first query to store a node has nothing to merge into, so it still returns the stored object. Second, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first write stamps the branch cache as live or as one `at` point in time, work at the same timestamp uses the store normally (a fully historical script gets complete store functionality), and anything carrying a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.24.0 versions did). This covers every write, `node.save()` included: a save carries live data, so it claims an unclaimed branch and is refused against a branch holding point-in-time data. Fourth, a successful `save()`, `create()` or `update()` now marks the node's in-memory state as persisted: the saved values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever. This does not change what a subsequent mutation sends - an explicitly cleared relationship or an edited peer set is still re-asserted on the next `save()`, exactly as before. | |||
Contributor
There was a problem hiding this comment.
P3: With store_merge=False or per-call merge=False, a re-fetch replaces the entry; kind changes also replace it even in merge mode. Qualify the identity guarantee to same-kind nodes using the default merge behavior, or document these exceptions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At changelog/+store-merge-behaviour.changed.md, line 1:
<comment>With `store_merge=False` or per-call `merge=False`, a re-fetch replaces the entry; kind changes also replace it even in merge mode. Qualify the identity guarantee to same-kind nodes using the default merge behavior, or document these exceptions.</comment>
<file context>
@@ -0,0 +1 @@
+The client store merge fix comes with four observable behaviour changes. First, the object returned by `get`, `filters` or `all` is a per-query snapshot and is no longer the same Python object as the store entry once that node has been fetched more than once: `client.get(id) is client.store.get(id)` was previously true and is now false after a re-fetch, although the two still compare equal (`==`). The first query to store a node has nothing to merge into, so it still returns the stored object. Second, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first write stamps the branch cache as live or as one `at` point in time, work at the same timestamp uses the store normally (a fully historical script gets complete store functionality), and anything carrying a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.24.0 versions did). This covers every write, `node.save()` included: a save carries live data, so it claims an unclaimed branch and is refused against a branch holding point-in-time data. Fourth, a successful `save()`, `create()` or `update()` now marks the node's in-memory state as persisted: the saved values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever. This does not change what a subsequent mutation sends - an explicitly cleared relationship or an edited peer set is still re-asserted on the next `save()`, exactly as before.
</file context>
Suggested change
| The client store merge fix comes with four observable behaviour changes. First, the object returned by `get`, `filters` or `all` is a per-query snapshot and is no longer the same Python object as the store entry once that node has been fetched more than once: `client.get(id) is client.store.get(id)` was previously true and is now false after a re-fetch, although the two still compare equal (`==`). The first query to store a node has nothing to merge into, so it still returns the stored object. Second, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first write stamps the branch cache as live or as one `at` point in time, work at the same timestamp uses the store normally (a fully historical script gets complete store functionality), and anything carrying a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.24.0 versions did). This covers every write, `node.save()` included: a save carries live data, so it claims an unclaimed branch and is refused against a branch holding point-in-time data. Fourth, a successful `save()`, `create()` or `update()` now marks the node's in-memory state as persisted: the saved values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever. This does not change what a subsequent mutation sends - an explicitly cleared relationship or an edited peer set is still re-asserted on the next `save()`, exactly as before. | |
| The client store merge fix comes with four observable behaviour changes. First, the object returned by `get`, `filters` or `all` is a per-query snapshot and is no longer the same Python object as the store entry once that node has been fetched more than once: `client.get(id) is client.store.get(id)` was previously true and is now false after a re-fetch, although the two still compare equal (`==`). The first query to store a node has nothing to merge into, so it still returns the stored object. Second, with the default merge behavior and an unchanged node kind, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first write stamps the branch cache as live or as one `at` point in time, work at the same timestamp uses the store normally (a fully historical script gets complete store functionality), and anything carrying a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.24.0 versions did). This covers every write, `node.save()` included: a save carries live data, so it claims an unclaimed branch and is refused against a branch holding point-in-time data. Fourth, a successful `save()`, `create()` or `update()` now marks the node's in-memory state as persisted: the saved values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever. This does not change what a subsequent mutation sends - an explicitly cleared relationship or an edited peer set is still re-asserted on the next `save()`, exactly as before. |
|
|
||
| :::warning Behaviour change in version 1.24.0 | ||
|
|
||
| Before SDK 1.24.0, queries using `at` overwrote store entries regardless of what the store held, so historical data could silently replace live data. Since 1.24.0 the store is timestamp-coherent: mismatching writes skip the store with a warning instead. Note that on a node whose query skipped the store, relationship peers cannot be resolved through `.peer` (the peers were never stored) - read the peer identity directly from the relationship (`.id`, `.display_label`, `.typename`) or use a dedicated client for that timestamp. |
Contributor
There was a problem hiding this comment.
P3: A mismatching query does not make .peer universally unavailable: an already-cached peer can still be returned from the relationship or existing store. Say that newly fetched peers may be unavailable unless the peer was already cached, then recommend the identity fields for the unavailable case.
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 295:
<comment>A mismatching query does not make `.peer` universally unavailable: an already-cached peer can still be returned from the relationship or existing store. Say that newly fetched peers may be unavailable unless the peer was already cached, then recommend the identity fields for the unavailable case.</comment>
<file context>
@@ -166,6 +241,63 @@ You can store objects in the store manually using the `set` method. This has the
+
+:::warning Behaviour change in version 1.24.0
+
+Before SDK 1.24.0, queries using `at` overwrote store entries regardless of what the store held, so historical data could silently replace live data. Since 1.24.0 the store is timestamp-coherent: mismatching writes skip the store with a warning instead. Note that on a node whose query skipped the store, relationship peers cannot be resolved through `.peer` (the peers were never stored) - read the peer identity directly from the relationship (`.id`, `.display_label`, `.typename`) or use a dedicated client for that timestamp.
+
+One caveat: compute the `at` timestamp once and reuse it. A script that rebuilds a relative timestamp for every call creates a slightly different instant each time and will trip the mismatch warning on every query after the first.
</file context>
Suggested change
| Before SDK 1.24.0, queries using `at` overwrote store entries regardless of what the store held, so historical data could silently replace live data. Since 1.24.0 the store is timestamp-coherent: mismatching writes skip the store with a warning instead. Note that on a node whose query skipped the store, relationship peers cannot be resolved through `.peer` (the peers were never stored) - read the peer identity directly from the relationship (`.id`, `.display_label`, `.typename`) or use a dedicated client for that timestamp. | |
| + Before SDK 1.24.0, queries using `at` overwrote store entries regardless of what the store held, so historical data could silently replace live data. Since 1.24.0 the store is timestamp-coherent: mismatching writes skip the store with a warning instead. A newly fetched relationship peer from a skipped query may be unavailable through `.peer` unless it was already cached; use the relationship identity (`.id`, `.display_label`, `.typename`) or a dedicated client for that timestamp when the peer is unavailable. |
|
|
||
| Two exceptions: | ||
|
|
||
| - A relationship peer resolved with `fetch()` or assigned directly is cached on the relationship itself. When the store already held that node, the cached peer is a detached copy and does not pick up later store merges. When `fetch()` was the first thing to store the node, the cached peer *is* the store object and keeps updating. |
Contributor
There was a problem hiding this comment.
P3: Directly assigning a peer does not always create a detached copy: assigning the object returned by store.get() keeps that exact live store object. Distinguish fetch() results from direct assignment so users do not choose a snapshot strategy based on an incorrect guarantee.
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 81:
<comment>Directly assigning a peer does not always create a detached copy: assigning the object returned by `store.get()` keeps that exact live store object. Distinguish `fetch()` results from direct assignment so users do not choose a snapshot strategy based on an incorrect guarantee.</comment>
<file context>
@@ -12,6 +12,79 @@ The store is mainly used for the internal working of the SDK. It is used to crea
+
+Two exceptions:
+
+- A relationship peer resolved with `fetch()` or assigned directly is cached on the relationship itself. When the store already held that node, the cached peer is a detached copy and does not pick up later store merges. When `fetch()` was the first thing to store the node, the cached peer *is* the store object and keeps updating.
+- When the same UUID arrives under a different node kind, the store replaces the entry wholesale instead of merging. References handed out before the kind change keep pointing at the old object and stop receiving updates.
+
</file context>
Suggested change
| - A relationship peer resolved with `fetch()` or assigned directly is cached on the relationship itself. When the store already held that node, the cached peer is a detached copy and does not pick up later store merges. When `fetch()` was the first thing to store the node, the cached peer *is* the store object and keeps updating. | |
| - A peer resolved with `fetch()` is cached on the relationship itself: if the store already held that node, the fetched query result is a detached copy; if not, it becomes the store object. Direct assignment caches the exact object assigned, so it follows store updates only when that object is the store entry. |
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(alias ofinitialized) 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; successful saves reset mutation tracking, with payload markers kept separate from_has_unsaved_changeso a cleared relationship is still re-asserted on a later save.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 ab6cdb4. Summary will update on new commits.