Local AI bug fixing pipeline test #10 normalize IPHost/IPNetwork/MAC address values at input time (closes #8896) by polmichel · Pull Request #8904 · opsmill/infrahub

polmichel · 2026-04-14T09:24:57Z

Why

IPHost attribute values are inconsistent across Infrahub's interfaces when no netmask is provided at creation time. The HFID returned by GraphQL cannot be used for subsequent lookups, display labels disagree with stored values, and the Python SDK returns a different representation than the UI.

This PR is about unifying this value display across the different layers of the application.

This was only impacting after attribute creation. Once the attribute is reloaded, bug should have disappeared.

Goals

Ensure IPHost/IPNetwork values are normalized to their canonical with_prefixlen form immediately on input, so all downstream consumers (HFID, display labels, GraphQL, SDK) see a consistent representation.

Out-of-scope

MAC address attributes had the same issue, and this PR also fixes it.

Closes #8896

What changed

Behavioral change: IPHost and IPNetwork attr.value is now always in canonical with_prefixlen form (e.g., "192.0.2.1" becomes "192.0.2.1/32") from the moment the attribute is initialized. This makes HFIDs, display labels, GraphQL responses, and SDK values all consistent.
Implementation: Added _canonicalize_value() hook to BaseAttribute (no-op by default), overridden in IPHost and IPNetwork and MACAddress. Called in BaseAttribute.__init__() after validation. For these classes, serialize_value has been removed.

How to test

uv run pytest backend/tests/component/core/test_manager_node.py::test_hfid_round_trip_with_iphost_attribute -x -v

Impact & rollout

Backward compatibility: IPHost values now always include the prefix in attr.value. Code that previously relied on the raw un-prefixed form (e.g., "192.0.2.1") from attr.value will now see "192.0.2.1/32". Display labels using {{ ip_address__value }} will include the mask; users wanting mask-free display can use {{ ip_address__ip }}.
Deployment notes: Safe to deploy. Existing nodes with stale HFIDs (stored without mask) will be corrected when their HFID is next recomputed (e.g., on update). A separate data migration may be desirable for bulk correction.

Checklist

Tests added/updated
Changelog entry added (uv run towncrier create ...)
External docs updated (if user-facing or ops-facing change)
Internal .md docs updated (internal knowledge and AI code tools knowledge)

codspeed-hq · 2026-04-14T09:46:34Z

Merging this PR will not alter performance

✅ 12 untouched benchmarks

_{Comparing ai-bug-pipeline-8896-iphost-hfid-consistency (87a0799) with stable (575e145)}

ajtmccarty

does this need a migration for existing non-normalized IP networks/hosts and MAC addresses?

ajtmccarty · 2026-04-28T17:08:57Z


        if self.value is not None:
            self.validate(value=self.value, name=self.name, schema=self.schema)
+            self.value = self._canonicalize_value(self.value)


is canonicalize a networking term? normalize seems more common

I have heard it in mathematics, but normalize seems more common, thanks

ajtmccarty · 2026-04-28T17:11:43Z

-
-        return ipaddress.ip_network(self.value).with_prefixlen
+        """Serialize the value before storing it in the database."""
+        return self._canonicalize_value(self.value)


it looks like this should be handled by the update to BaseAttribute.__init__, so maybe serialize_value can be removed from IPNetwork and IPHost and MacAddress, but maybe I don't understand the order of operations

You are right, initially I spotted that serialize_value was containing the same logic but the refactor was not good. This is logic should not be in database serialization step but value normalization step.

ajtmccarty · 2026-04-28T17:13:49Z

-    assert test_mac.value == mac_address
+    # MacAddress canonicalizes the input to the standard EUI-48 form so the
+    # in-memory value matches the DB-stored representation.
+    assert test_mac.value == test_mac._canonicalize_value(mac_address)


this isn't really testing that the MAC address is in the expected format. it's just testing that the mac address is in the format returned by _canonicalize_value

You are right, I have left the comme and remove the call to _canonicalize_value. Value is now hardcoded

polmichel · 2026-04-28T21:51:04Z

does this need a migration for existing non-normalized IP networks/hosts and MAC addresses?

I do not think so. Because the value was serialized before being saved into the database, so the real value in the database should be correct. However, the initial value displayed in display_label or hfid fields was using the Attribute.value before refetching it from the database. This value was not serialized (yet).

I have updated the PR description with an additional element about this precision.

ajtmccarty · 2026-04-29T19:11:17Z

+| Kind | Normalized form |
+|------|-----------------|
+| `IPHost` | `ipaddress.ip_interface(value).with_prefixlen` (e.g. `192.0.2.1` → `192.0.2.1/32`) |
+| `IPNetwork` | `ipaddress.ip_network(value).with_prefixlen` |


no example for this one?

Good catch, thank you

ajtmccarty · 2026-04-29T19:13:43Z

+    reloaded = await NodeManager.get_one(db=db, id=node.id, branch=default_branch)
+    assert reloaded is not None
+    assert reloaded.mac.value == "AA-BB-CC-DD-EE-FF"
+


should there be a test for retrieving a Node that was saved before this change was made? i.e. an object with an IP address of 192.168.0.0 in the database is retrieved as 192.168.0.0/32. I believe it would require a cypher query to set the data to be incorrect

The attribute value was already serialized before reaching the database. The HFID and display labels were using the value before the serialization, at least until the node is recomputed. So the test would be:

Creating a fake value through a Cypher query to set up a fake value on a HFID attribute

Update the node

Verify the HFID is correctly recomputed

I think this is something that is already tested somewhere and would not bring value to the test suite.
This means that node containing wrong HFID value will be updated to correct HFID value when the node is updated. Would this be a correct way of handling this in your opinion?

right. I forgot this is only for HFIDs. I know I already asked about a migration for the existing IP Network/Host data and I agree that we don't need that. But now I'm wondering if there should be a migration to updates HFIDs using IPNetwork/Host or MAC address attributes to make sure they all have the same format.

I guess I'm not certain if this fix addresses this particular part of the bug report

GraphQL: Queries that look up an instance by its hfid require the mask to be included in the hfid input, but the hfid returned by the GraphQL query response does not include the mask — making the returned hfid unusable as a direct input for subsequent lookups.

If the node update is not enough as a synchronization tool, let's do a migration for this. In that case, I will add another changelog and do the migration about @BaptisteGi comment in a chained PR targeting this branch.

Regarding the second part, I think this PR resolves the issue as the graphQL query will return the value of the hfid through
infrahub.core.attribute.BaseAttribute.to_graphql which get the attribute directly through get_attr() if I am not wrong.
I will add an integration test in order to verify that there is no additional behavior that would remove the normalization when going through the graphQL layer. However, I do not think there would be one.

for the component test reproducing the HFID lookup query see 4da9aa7

BaptisteGi · 2026-04-30T08:56:57Z

    type = str
    value: str

+    def _normalize_value(self, value: Any) -> str:


My 2cents here: recently opened #9015, from a user perspective the : notation is more natural. Doesn't mean we need to change the way it's stored in the database but perhaps only the way it's returned in the API so the value returns MAC address in : format.

In my opinion, a specific update to this value using a database migration would be a better long-term solution, as it would avoid to add another step of data transformation between the database and the UI. This kind of layer is the root cause of this bug.
In that case, I would handle the value format update in another PR.

polmichel · 2026-05-04T17:33:58Z

@@ -0,0 +1,89 @@
+from infrahub.core import registry


GraphQL query-level test about the look up HFID query

polmichel · 2026-05-04T17:37:13Z

@@ -0,0 +1,310 @@
+from __future__ import annotations


This is reusing the logic from m044 and m047

polmichel · 2026-05-08T15:01:28Z

EDIT: looks good

I have to check one last thing on execute_on_branch method regarding branch awareness type

…8 form (closes #9015) Adds an input-time `_normalize_value()` hook on `BaseAttribute` (no-op default), overridden in `MacAddress` to emit colon-uppercase form (`AA:BB:CC:DD:EE:FF`). Adds m070 migration that rewrites stored MAC values + dependent HFIDs / display labels in the DB. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

…display labels (closes #8896) IPHost and IPNetwork now expose `_normalize_value()` so `attr.value` matches the canonical with_prefixlen form before HFID compute runs. Adds m071 migration that rewrites HFIDs and display labels for nodes whose schema references IPHost, IPNetwork, or MacAddress attributes (MAC values are already in colon form by this point because m070 ran first). Factors shared display-label / path-details helpers out of m044/m046/m047/m059. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

m070 already rewrites every MacAddress value plus its dependent HFID and display_label to colon form. m071 only needs to recompute HFIDs/display labels for IPHost / IPNetwork attributes, so removing MacAddress from NORMALIZED_KINDS avoids redundant work and clarifies the migration's scope. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Drop the local copies of `_format_hfid`, `_make_display_label_formatter`, `_RowFormatter`, `_read_path_values`, and `_paginate_recompute` that duplicated the helpers extracted in m070's simplification. Picks up the per-kind Jinja template parsing optimization for free. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

…ct_plans Match m070's structure: branch filter is applied inside `_collect_plans`, and both entry points delegate to a shared `_run` method to drop the duplicated boilerplate (schema lookup, error wrapping, attribute schema resolution). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Drop the multi-paragraph class docstring and the redundant `_RecomputePlan` docstring. Field names self-document; the why-#8896 link is enough context. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

m071 was scoped down to IPHost/IPNetwork only when MAC handling moved to m070; the file, test file, and migration `name` still said "ip_mac". Rename so the artifacts match the actual scope. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Mirror the m070 review fix: an attribute can be branch-aware on a branch-agnostic/local schema, so filtering on schema.branch misses real cases. The gate now checks each affected IPHost/IPNetwork path's attribute_schema branch support against the branch filter; the path list is still returned in full so HFID/display_label recomputation reads every contributor. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Replace the three separate test methods with one test that mirrors the m070 review pattern: seed default + user branch with raw HFID/display_label, run on default, rebase + run on user branch, then re-run both for idempotency. Drops the per-kind parametrize and the schema-root factory in favor of an inline SCHEMA_ROOT and a single helper for value reads/writes. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Migration tests verify the database; routing reads through the GraphQL layer adds noise without catching anything the DB-level checks miss. Replace the `_query_node` calls and the pre-migration GraphQL sanity check with direct attribute reads. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Detect any duplicated or orphaned graph state introduced by the migration once the consolidated scenario is complete. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

… run Mirror the m070 default-branch fix: an IPHost/IPNetwork attribute can be branch-agnostic or branch-local on a branch-aware schema, so HFID/display_label recomputation on the default branch must cover all three branch support types, not just AWARE. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

github-actions Bot added group/backend Issue related to the backend (API Server, Git Agent) group/ci Issue related to the CI pipeline labels Apr 14, 2026

polmichel mentioned this pull request Apr 14, 2026

bug: IPHost attribute values vary in when they require/return their netmask from API and UI #8896

Closed

polmichel changed the title ~~test: failing test for #8896 -- IPHost HFID round-trip inconsistency~~ fix: canonicalize IPHost/IPNetwork values at input time (closes #8896) Apr 14, 2026

polmichel changed the title ~~fix: canonicalize IPHost/IPNetwork values at input time (closes #8896)~~ Local AI bug fixing pipeline test #10 canonicalize IPHost/IPNetwork values at input time (closes #8896) Apr 14, 2026

This comment was marked as outdated.

Sign in to view

polmichel force-pushed the ai-bug-pipeline-8896-iphost-hfid-consistency branch from 4dc7717 to d0fcb28 Compare April 28, 2026 11:50

github-actions Bot added type/documentation Improvements or additions to documentation and removed group/ci Issue related to the CI pipeline labels Apr 28, 2026

polmichel marked this pull request as ready for review April 28, 2026 13:24

polmichel requested review from a team as code owners April 28, 2026 13:24

polmichel changed the title ~~Local AI bug fixing pipeline test #10 canonicalize IPHost/IPNetwork values at input time (closes #8896)~~ Local AI bug fixing pipeline test #10 canonicalize IPHost/IPNetwork/MAC address values at input time (closes #8896) Apr 28, 2026

gmazoyer reviewed Apr 28, 2026

View reviewed changes

Comment thread backend/tests/component/core/test_manager_node.py Outdated

ajtmccarty reviewed Apr 28, 2026

View reviewed changes

polmichel changed the title ~~Local AI bug fixing pipeline test #10 canonicalize IPHost/IPNetwork/MAC address values at input time (closes #8896)~~ Local AI bug fixing pipeline test #10 normalize IPHost/IPNetwork/MAC address values at input time (closes #8896) Apr 28, 2026

polmichel requested review from ajtmccarty and gmazoyer April 29, 2026 07:05

ajtmccarty approved these changes Apr 29, 2026

View reviewed changes

BaptisteGi reviewed Apr 30, 2026

View reviewed changes

polmichel requested a review from ajtmccarty April 30, 2026 13:12

polmichel marked this pull request as draft May 4, 2026 08:50

polmichel force-pushed the ai-bug-pipeline-8896-iphost-hfid-consistency branch from 1731e70 to 0d533b0 Compare May 4, 2026 12:48

polmichel marked this pull request as ready for review May 4, 2026 16:19

polmichel force-pushed the ai-bug-pipeline-8896-iphost-hfid-consistency branch from e41f529 to 2f4870f Compare May 4, 2026 17:18

polmichel commented May 4, 2026

View reviewed changes

Base automatically changed from pmi-20260504-mac-address-values to stable May 8, 2026 16:54

polmichel marked this pull request as draft May 9, 2026 10:03

polmichel self-assigned this May 9, 2026

polmichel marked this pull request as ready for review May 9, 2026 10:03

polmichel force-pushed the ai-bug-pipeline-8896-iphost-hfid-consistency branch from 2cd9edc to 9217460 Compare May 9, 2026 10:09

github-actions Bot added the type/documentation Improvements or additions to documentation label May 9, 2026

pol-opsmill and others added 17 commits May 9, 2026 17:47

refactor(m071): trim verbose docstrings to match m070

622dcbc

Drop the multi-paragraph class docstring and the redundant `_RecomputePlan` docstring. Field names self-document; the why-#8896 link is enough context. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

renamed migration

89feea0

not required since git blame exists

48c9f39

test(m071): call verify_graph at end of test

b8e0a0a

Detect any duplicated or orphaned graph state introduced by the migration once the consolidated scenario is complete. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

format

b80bf23

useless mention to work item since git blame exist

a0be627

remove docs modification

87a0799

polmichel force-pushed the ai-bug-pipeline-8896-iphost-hfid-consistency branch from 9217460 to 87a0799 Compare May 9, 2026 15:49

github-actions Bot removed the type/documentation Improvements or additions to documentation label May 9, 2026

opsmill-bug-pipeline Bot mentioned this pull request May 9, 2026

[aw] No-Op Runs #9201

Open

ajtmccarty approved these changes May 11, 2026

View reviewed changes

polmichel merged commit 0c292c8 into stable May 12, 2026
56 checks passed

polmichel deleted the ai-bug-pipeline-8896-iphost-hfid-consistency branch May 12, 2026 06:39

Uh oh!

Conversation

polmichel commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Goals

Out-of-scope

What changed

How to test

Impact & rollout

Checklist

Uh oh!

codspeed-hq Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

This comment was marked as outdated.

Uh oh!

ajtmccarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmichel Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmichel commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmichel May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmichel May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmichel commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

polmichel commented Apr 14, 2026 •

edited

Loading

codspeed-hq Bot commented Apr 14, 2026 •

edited

Loading

polmichel Apr 28, 2026 •

edited

Loading

polmichel commented Apr 28, 2026 •

edited

Loading

polmichel May 4, 2026 •

edited

Loading

polmichel May 4, 2026 •

edited

Loading

polmichel commented May 8, 2026 •

edited

Loading