Skip to content

fix(node): include user keys on fetch to avoid spurious changed status#334

Merged
BeArchiTek merged 2 commits into
developfrom
fix-diff-include-relationships
Jun 11, 2026
Merged

fix(node): include user keys on fetch to avoid spurious changed status#334
BeArchiTek merged 2 commits into
developfrom
fix-diff-include-relationships

Conversation

@BeArchiTek

@BeArchiTek BeArchiTek commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

The node module can report changed: true on idempotent re-runs of a task whose data includes a relationship. Re-applying the exact same payload should be a no-op but currently flips status to changed.

Root cause

_get_object calls fetch_single_node without an include list, so cardinality-MANY non-attribute/non-parent relationships are not requested in the GraphQL query and stay uninitialized on the returned node. When _update_object builds the diff:

  • serialized_before = node._generate_input_data() — skips uninitialized relationships (SDK: if not rel.initialized: continue).
  • setattr(node, rel_name, …) initializes the relationship.
  • serialized_after = node._generate_input_data() — now includes the relationship.

dict_hash(serialized_before) != dict_hash(serialized_after) even when the underlying value matches → false changed: true.

This regressed when the original include-on-fetch fix (commit 6f1315e) was removed during the diff rework in 0142ad6.

Fix

In _get_object, derive include from the user-provided data keys (excluding id / hfid which are not schema fields) and pass it to fetch_single_node. Empty list collapses to None so the state: absent path and identifier-only lookups keep their default fetch behavior.

include = [key for key in data if key not in ("id", "hfid")] or None
node = self.client.fetch_single_node(
    kind=kind, id=node_id, hfid=node_hfid, include=include, raise_when_missing=False
)

Test plan

  • Unit tests pass: pytest tests/unit/plugins/module_utils/test_node.py → 23/23
  • Integration cycle playbook against sandbox.infrahub.app:
    • Create tag1 → changed
    • Update tag1 → changed
    • Existing tag1ok (not changed) ← the idempotency assertion this PR restores
    • Delete tag1 → changed
    • Delete tag1 (already absent) → ok
  • Re-run the cycle playbook a second time to confirm deterministic idempotency on relationship-bearing kinds.

Summary by cubic

Fixes false changed status in the node module by fetching nodes with the user’s fields included. Adds regression tests to prevent this idempotency issue from returning.

  • Bug Fixes
    • Derive include from user data keys (excluding id and hfid) and pass to fetch_single_node.
    • Ensure relationships are initialized in the "before" snapshot to avoid spurious diffs.
    • Fall back to default fetch when only identifiers are provided, preserving state: absent and lookup behavior.
    • Add unit tests asserting include contains user keys and defaults to None for identifier-only data.

Written for commit 0f977e1. Summary will update on new commits.

Review in cubic

Pass `include=list(user_data_keys)` to `fetch_single_node` in
`_get_object` so every attribute and relationship the user intends
to update is initialized on the fetched node.

Without this, the SDK skips uninitialized relationships in
`_generate_input_data()` (`if not rel.initialized: continue`), so
the "before" snapshot used for diff comparison misses keys that the
"after" snapshot has after `setattr`. The resulting `dict_hash`
mismatch reports `changed: true` for a no-op idempotent re-run.

The `or None` fallback preserves default fetch behavior when the
user provides only identifier keys (e.g. `state: absent`).
@BeArchiTek BeArchiTek requested a review from a team as a code owner May 28, 2026 13:46
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented May 28, 2026

Copy link
Copy Markdown

Deploying infrahub-ansible with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0f977e1
Status: ✅  Deploy successful!
Preview URL: https://30d08f5c.infrahub-ansible.pages.dev
Branch Preview URL: https://fix-diff-include-relationshi.infrahub-ansible.pages.dev

View logs

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Re-trigger cubic

Regression guard for the spurious changed-status fix: the original
include-on-fetch fix (6f1315e) was silently dropped during the diff
rework (0142ad6) because nothing asserted the fetch_single_node call
arguments. These tests fail if the include derivation is removed again.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@BeArchiTek BeArchiTek merged commit 17bc1ad into develop Jun 11, 2026
23 checks passed
@BeArchiTek BeArchiTek deleted the fix-diff-include-relationships branch June 11, 2026 14:42
@BeArchiTek BeArchiTek added the changes/patch Maintenance tasks label Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes/patch Maintenance tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants