feat: add SHA-1 idempotency primitives for CoreFileObject

[minitriga]

Summary

Adds SHA-1 compare-and-skip primitives to InfrahubNode / InfrahubNodeSync so downstream libraries stop maintaining parallel copies of this logic. Four public additions:

• sha1_of_source(source: bytes | Path | BinaryIO) -> str in infrahub_sdk.file_handler — streaming SHA-1 helper (64 KiB chunks; rewinds BinaryIO to original position). Canonical import path: from infrahub_sdk.file_handler import sha1_of_source.
• matches_local_checksum(source) -> bool — atomic compare against the node's server-stored checksum attribute. No network, no mutation.
• upload_if_changed(source, name=None) -> UploadResult — composes compare + upload_from_* + save(). Returns a frozen UploadResult(was_uploaded, checksum) dataclass. Skips the transfer when the local SHA-1 matches.
• download_file(..., skip_if_unchanged=True) — short-circuits the download when dest exists on disk with a matching SHA-1. Returns 0 bytes written on skip.

Both async and sync twins. Full async↔sync symmetry.

Motivation

Both opsmill/nornir-infrahub#71 and opsmill/infrahub-ansible#317 shipped near-identical hashlib.sha1(...).hexdigest() + compare logic. Centralising it in the SDK gives one source of truth and — because nornir-infrahub already extended idempotency to the download side — brings that capability to the Ansible collection too.

Usage — nornir-infrahub example

Concretely, the nornir-infrahub tasks in nornir_infrahub/plugins/tasks/file_object.py collapse from ~30 lines of hand-rolled idempotency into a single method call per operation.

Upload

Before (today, on nornir-infrahub main):

import hashlib

def _sha1(data: bytes) -> str:
    return hashlib.sha1(data, usedforsecurity=False).hexdigest()

# ... inside upload_file_object ...
existing_obj = _lookup_existing_object(...)
if existing_obj:
    if _sha1(payload) == existing_obj.checksum.value:
        return Result(changed=False, result="already up to date (checksum match)")
    for attr_name, attr_value in data.items():
        if attr_name in existing_obj._schema.attribute_names:
            setattr(existing_obj, attr_name, attr_value)
    if path is not None:
        existing_obj.upload_from_path(path)
    else:
        existing_obj.upload_from_bytes(content=payload, name=upload_name)
    existing_obj.save()
    return Result(changed=True, result="updated (checksum changed)")

After (with this PR):

from infrahub_sdk.node import UploadResult

# ... inside upload_file_object ...
existing_obj = _lookup_existing_object(...)
if existing_obj:
    for attr_name, attr_value in data.items():
        if attr_name in existing_obj._schema.attribute_names:
            setattr(existing_obj, attr_name, attr_value)
    outcome: UploadResult = existing_obj.upload_if_changed(
        source=path if path is not None else payload,
        name=upload_name,
    )
    return Result(
        changed=outcome.was_uploaded,
        result=(
            "updated (checksum changed)" if outcome.was_uploaded
            else "already up to date (checksum match)"
        ),
    )

No _sha1 helper, no hashlib import, no two-branch upload_from_path / upload_from_bytes dispatch — the SDK handles the compare, the staging, and the save in one call. The returned UploadResult.was_uploaded maps directly to Nornir's Result(changed=...).

Download

Before (today):

server_checksum = obj.checksum.value
changed = False
if resolved_save_to is not None and resolved_save_to.exists() and resolved_save_to.is_file():
    local_checksum = _sha1(resolved_save_to.read_bytes())
    if local_checksum == server_checksum:
        content: bytes = resolved_save_to.read_bytes()
    else:
        content = obj.download_file()  # type: ignore[assignment]
        resolved_save_to.parent.mkdir(parents=True, exist_ok=True)
        resolved_save_to.write_bytes(content)
        changed = True
else:
    content = obj.download_file()  # type: ignore[assignment]
    if resolved_save_to is not None:
        resolved_save_to.parent.mkdir(parents=True, exist_ok=True)
        resolved_save_to.write_bytes(content)
        changed = True

After:

if resolved_save_to is not None:
    resolved_save_to.parent.mkdir(parents=True, exist_ok=True)
    bytes_written = obj.download_file(dest=resolved_save_to, skip_if_unchanged=True)
    changed = bytes_written > 0  # 0 means the local file already matched
    content = resolved_save_to.read_bytes()
else:
    content = obj.download_file()
    changed = False

Two branches collapse into one, the manual SHA-1 compare and file-exists handling move into the SDK, and the changed signal falls out of the return value — no more tracking it imperatively across branches.

Notes

• upload_if_changed computes sha1_of_source(source) once up front and reuses it for both the compare and the returned UploadResult.checksum. This is the semantically correct value (the server stores sha1(received_bytes), which equals the local digest), and it avoids double-hashing or reading a BinaryIO twice.
• download_file(skip_if_unchanged=True) validates the saved-node precondition before the skip-check, so an unsaved node with a coincidentally-matching checksum.value still raises ValueError rather than silently returning 0.
• Three new *_FEATURE_NOT_SUPPORTED_MESSAGE constants were added alongside the existing FILE_DOWNLOAD_FEATURE_NOT_SUPPORTED_MESSAGE, following the same convention.

Follow-ups (not in this PR)

Once this lands and a release ships with the new lower bound:

• [opsmill/nornir-infrahub] bump infrahub-sdk floor and replace local SHA-1 code in nornir_infrahub/plugins/tasks/file_object.py with the new methods (per the usage example above).
• [opsmill/infrahub-ansible] bump infrahub-sdk floor and replace local SHA-1 code in plugins/module_utils/node.py (get_file_object_local_checksum, _update_object_with_file) with the new methods. Also gains download-side idempotency for free by wiring skip_if_unchanged=True into the object_file_fetch action.

Test plan

• Unit tests pass for both InfrahubNode and InfrahubNodeSync (84 file-related tests across test_file_handler.py and test_file_object.py, up from ~41 baseline)
• ruff check clean
• mypy clean
• Towncrier Added fragment validates
• Integration test against a live Infrahub instance (separate branch)

Changelog fragments

• changelog/+idempotent-file-ops.added.md — describes the four new primitives

[cloudflare-workers-and-pages]

Deploying infrahub-sdk-python with    Cloudflare Pages

Latest commit:	7a21d36
Status:	✅  Deploy successful!
Preview URL:	https://0c6ea863.infrahub-sdk-python.pages.dev
Branch Preview URL:	https://feat-idempotent-file-ops.infrahub-sdk-python.pages.dev

View logs

[codecov]

Codecov Report

❌ Patch coverage is 93.27731% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
infrahub_sdk/node/node.py	91.66%	4 Missing and 4 partials ⚠️

@@            Coverage Diff             @@
##           stable     #963      +/-   ##
==========================================
+ Coverage   80.77%   81.57%   +0.80%     
==========================================
  Files         132      134       +2     
  Lines       10999    11444     +445     
  Branches     1681     1730      +49     
==========================================
+ Hits         8884     9335     +451     
+ Misses       1566     1564       -2     
+ Partials      549      545       -4     

Flag	Coverage Δ
integration-tests	41.48% <35.29%> (-1.29%)	⬇️
python-3.10	54.59% <67.22%> (+1.02%)	⬆️
python-3.11	54.59% <67.22%> (+1.04%)	⬆️
python-3.12	54.57% <67.22%> (+1.01%)	⬆️
python-3.13	54.59% <67.22%> (+1.04%)	⬆️
python-3.14	54.57% <67.22%> (-0.58%)	⬇️
python-filler-3.12	22.71% <15.96%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
infrahub_sdk/file_handler.py	83.95% <100.00%> (+2.26%)	⬆️
infrahub_sdk/node/__init__.py	100.00% <100.00%> (ø)
infrahub_sdk/node/constants.py	100.00% <100.00%> (ø)
infrahub_sdk/node/node.py	86.91% <91.66%> (+0.70%)	⬆️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ajtmccarty]

I think this looks good, so approving. comments are little suggestions and a higher-level question that is probably more a reflection of my SDK-ignorance than any issues here

[ajtmccarty]

is there a way to write this so that is covers how the bug would affect the user?

[minitriga]

Rewritten in user-impact terms in 18e1572 — the new wording foregrounds the "success-looking 0 masks an unsaved node" user-visible failure mode instead of describing the internal reorder.

[ajtmccarty]

was_uploaded makes it more clear that this is Boolean reflecting whether something happened or not

[minitriga]

Renamed to was_uploaded across the dataclass, both async+sync upload_if_changed return sites, docstrings, tests, and the PR description in 4e2d5a9.

[ajtmccarty]

I don't know if this is really necessary

[minitriga]

Agreed — dropped test_is_frozen in 93fe705. You were right that it was asserting @dataclass(frozen=True)'s contract rather than anything this PR adds.

[ajtmccarty]

good use of a dataclass instead of a tuple 👍

[minitriga]

Thanks!

[ajtmccarty]

should this one include the httpx_mock and check if the upload was sent?

[minitriga]

Added a positive-path assertion in 9b8ebc1 — test_uploads_when_checksum_differs now verifies via httpx_mock.get_requests() that the update mutation POST actually fired, instead of just trusting was_uploaded is True.

[ajtmccarty]

should this be checked to verify the upload was sent?

[minitriga]

Covered by the same change in 9b8ebc1 — the test now asserts the update mutation actually fires via httpx_mock.get_requests(), not just the return value.

[ajtmccarty]

it still doesn't look like this test asserts anything about the sent requests

[minitriga]

You're right — I added the HTTP assertion to test_uploads_when_checksum_differs last round, but line 481 actually points at test_uploads_when_node_unsaved, which was still missing the check. Added the equivalent mock_node_create_with_file.get_requests() assertion to that test in 2f44bff.

[minitriga]

Replying directly to this follow-up (the earlier reply may not have threaded clearly under it).

Line 481 at the time of your comment was pointing at test_uploads_when_node_unsaved — which was still missing the request assertion. Fixed in 2f44bff — the new assertion block at test_file_object.py:497-500 now reads:

# Positive-path HTTP verification: the create mutation must have been dispatched.
requests = mock_node_create_with_file.get_requests()
assert len(requests) > 0
assert any(r.method == "POST" for r in requests)

So the test now verifies via httpx_mock that the create mutation was actually dispatched, not just that the return value says was_uploaded is True. Let me know if you had a specific assertion shape in mind — e.g. checking that it was exactly one request, or inspecting the URL path for the GraphQL endpoint — happy to tighten it.

[minitriga]

Tightened the assertions in 033246d to mirror the existing test_node_{create,update}_with_file_uses_multipart pattern:

requests = mock_node_create_with_file.get_requests()
assert len(requests) == 1
assert requests[0].headers.get("x-infrahub-tracker") == "mutation-networkcircuitcontract-create"
assert requests[0].headers.get("content-type").startswith("multipart/form-data;")
assert b'filename="' + FILE_NAME.encode() + b'"' in requests[0].content

Same shape for the differs (update) test. Each positive-path upload test now verifies exactly-one request, the specific mutation tracker, multipart encoding, and the uploaded filename.

[minitriga]

Tightened the assertions on both upload tests in 033246d. Each one now verifies four specific properties of the sent request (mirroring test_node_{create,update}_with_file_uses_multipart):

requests = mock_node_create_with_file.get_requests()
assert len(requests) == 1
assert requests[0].headers.get("x-infrahub-tracker") == "mutation-networkcircuitcontract-create"
assert requests[0].headers.get("content-type").startswith("multipart/form-data;")
assert f"filename=\"{FILE_NAME}\"".encode() in requests[0].content

Same shape for the differs (update) variant, with the -update tracker. That covers: exactly-one request, the correct GraphQL mutation tracker header, multipart encoding, and the uploaded filename in the body.

[ajtmccarty]

should the name be asserted somehow? truly not sure

[minitriga]

Skipping at this layer — the existing test_node_create_with_file_uses_multipart / test_node_update_with_file_uses_multipart tests already assert the filename in the multipart body, and upload_if_changed delegates to upload_from_path unchanged, so a name assertion here would duplicate coverage. Happy to add one explicitly at the upload_if_changed layer if you'd rather have it visible in this test class.

[ajtmccarty]

I'm not an SDK expert, but I am not sure how you know if a remote file has changed without downloading it

[minitriga]

Good catch — the docstrings didn't make this explicit. Clarified in 5269fb6 across UploadResult, matches_local_checksum, and both download_file(skip_if_unchanged=...) docstrings: the comparison uses the checksum attribute as loaded when the node was fetched via client.get(...). If the server replaces the file after the fetch, callers need to re-fetch the node to refresh checksum before the skip-check can see the change.

[ajtmccarty]

same question about whether this should be verified in the test

[minitriga]

Also covered in 9b8ebc1 — test_downloads_when_local_differs now checks httpx_mock.get_requests() for exactly one GET to /api/storage/files/.

[ajtmccarty]

correct me if I'm wrong, but skip_if_unchanged did not exist before this PR. I still don't really understand what this is saying, but I don't think it can be "fixed" and reference a new kwarg

[minitriga]

You're right — since skip_if_unchanged is new in this PR, the bug never reached users and a .fixed.md fragment is self-contradictory. Dropped the fragment entirely in fda914c, also updated the PR description and docstring cross-references that mentioned it. Only the .added.md fragment ships in the changelog now.

[ajtmccarty]

it still doesn't look like this test asserts anything about the sent requests

[ajtmccarty]

I don't think the reference to another test in the comment is necessary

[minitriga]

Trimmed in 7a21d36 — comment now reads # Positive-path HTTP verification: the update mutation must have been dispatched as a multipart request. with no cross-test reference.

[ajtmccarty]

samesies

[minitriga]

Same trim applied to this one in 7a21d36 — now reads # Positive-path HTTP verification: the create mutation must have been dispatched as a multipart request..