Skip to content

IFC-2271 IPAM find closes matched prefix for IP/prefix#8611

Merged
gmazoyer merged 13 commits intodevelopfrom
gma-431-ipam-closest-prefix
Mar 17, 2026
Merged

IFC-2271 IPAM find closes matched prefix for IP/prefix#8611
gmazoyer merged 13 commits intodevelopfrom
gma-431-ipam-closest-prefix

Conversation

@gmazoyer
Copy link
Contributor

@gmazoyer gmazoyer commented Mar 16, 2026

Why

This PR adds a parent prefix lookup: when a valid IP address or CIDR prefix is entered, all containing parent prefixes are displayed in a dedicated "Parent Prefixes" section, ordered from most specific to least specific, across all namespaces.

Non-goals: this PR does not modify any existing search behavior. The feature is purely additive, non-IP queries follow the exact same code path as before.

What changed

  • New IPParentPrefixLookupQuery Cypher query class that adapts the binary prefix matching algorithm from IPPrefixReconcileQuery, queries across all namespaces, returns results ordered by prefixlen DESC
  • New _try_parse_ip_or_prefix() helper that uses ipaddress module to detect valid IP/CIDR input
  • NodeEdges GraphQL type extended with nullable parent_prefixes: [NodeEdge!] field
  • search_resolver() extended: after IPv6 normalization, detects IP/CIDR, runs the parent prefix lookup, always runs existing text search regardless

Suggested review order

  1. backend/infrahub/core/query/ipam.py with the new IPParentPrefixLookupQuery and IPParentPrefixResult
  2. backend/infrahub/graphql/queries/search.py with _try_parse_ip_or_prefix() helper and resolver changes
  3. Tests: backend/tests/unit/graphql/queries/test_search.py and backend/tests/component/graphql/queries/test_search.py

How to review

  • The Cypher query is the core logic, compare it with IPPrefixReconcileQuery` for the algorithm it adapts
  • V verify the IP detection runs after IPv6 normalization and before text search, and that parent_prefixes is only populated when the field is requested

Risky areas:

  • The Cypher query's prefixlen <= prefix_and_length[1] condition (not =) is critical — multiple prefixes can share the same binary address with different prefix lengths

How to test

uv run pytest backend/tests/unit/graphql/queries/test_search.py -v
uv run pytest backend/tests/component/graphql/queries/test_search.py -v

Impact & rollout

  • Backward compatibility: No breaking changes. The parent_prefixes field is nullable and additive. Clients that don't request it see no difference.
  • Performance: Non-IP queries follow the exact same code path. IP queries add one additional Cypher query using the existing binary_address index.
  • Deployment notes: Safe to deploy. No migration needed.

Checklist

  • Tests added/updated
  • Changelog entry added (changelog/+ifc2271.added.md)
  • Internal .md docs updated (spec, plan, research, data-model, contracts in dev/specs/infp-431-ipam-closest-prefix/)
  • I have reviewed AI generated content

Summary by CodeRabbit

  • New Features

    • Global search now detects IP addresses/CIDRs and shows a "Parent Prefixes" section (IPv4 & IPv6), ordered most-to-least specific; exact-match prefixes remain in main results. Supports multi-namespace and branch-aware behavior.
  • Schema

    • GraphQL contract extended to include optional parent_prefixes on search results so clients can request parent-prefix data.
  • Tests

    • Added unit and component tests covering parsing, lookup ordering, branch isolation, and edge cases.
  • Documentation

    • Added changelog, specs, quickstart, plan, and research docs for the feature.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 16, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a parent-prefix lookup for IP/CIDR search queries. Introduces IPParentPrefixResult (frozen dataclass) and IPParentPrefixLookupQuery which builds candidate parent prefixes and queries the DB for active parents. Extends the GraphQL search flow with a _try_parse_ip_or_prefix helper and a nullable parent_prefixes field on NodeEdges; when q parses as an IP/CIDR and parent_prefixes is requested, the resolver runs the lookup and returns matching parent-prefix edges. Adds unit and component tests, schema entry, frontend GraphQL typings, changelog, and design/spec/docs.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'IFC-2271 IPAM find closes matched prefix for IP/prefix' clearly summarizes the main feature: parent prefix lookup for IP/CIDR queries in IPAM search. It is concise and directly related to the core change.
Description check ✅ Passed The PR description is comprehensive and well-structured. It includes Why (problem/goal/non-goals), What changed (with implementation details), How to review (with suggested order and risky areas), How to test (with specific commands), Impact & rollout (with compatibility/performance notes), and a completed checklist. All required sections are present and substantive.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Mar 16, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 16, 2026

Merging this PR will improve performance by 38.22%

⚡ 12 improved benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
test_nodemanager_querypeers 1.5 ms 1.3 ms +22.25%
test_base_schema_duplicate_CoreProposedChange 2.1 ms 1.7 ms +23.59%
test_query_one_model 464.3 ms 353.7 ms +31.26%
test_relationshipmanager_getpeer 163 µs 134.4 µs +21.24%
test_query_rel_one 662.5 ms 502 ms +31.96%
test_get_menu 245.4 ms 188.2 ms +30.44%
test_schemabranch_process 1,041.5 ms 798.2 ms +30.48%
test_schemabranch_duplicate 7.2 ms 5.5 ms +30.99%
test_get_schema 328.4 ms 252.7 ms +29.92%
test_graphql_generate_schema 515.9 ms 373.2 ms +38.22%
test_query_rel_many 692.8 ms 524.6 ms +32.08%
test_load_node_to_db_node_schema 67.1 ms 52.2 ms +28.58%

Comparing gma-431-ipam-closest-prefix (7d19a9d) with develop (2ff8ae9)

Open in CodSpeed

@github-actions github-actions bot added the group/frontend Issue related to the frontend (React) label Mar 16, 2026
return result.data["InfrahubSearchAnywhere"]


async def test_search_parent_prefix_ipv4(db: InfrahubDatabase, ip_dataset_01: dict[str, Any], branch: Branch) -> None:
Copy link
Contributor Author

@gmazoyer gmazoyer Mar 16, 2026

Choose a reason for hiding this comment

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

These tests are testing multiple things within the same test functions, but this helps in saving time during tests. I tried having more simple/individual tests but testing time increased by 25% for this file.

@gmazoyer gmazoyer marked this pull request as ready for review March 16, 2026 16:24
@gmazoyer gmazoyer requested review from a team as code owners March 16, 2026 16:24
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (1)
dev/specs/infp-431-ipam-closest-prefix/plan.md (1)

8-8: Tighten wording in the summary sentence.

“exact same code path” can be shortened to “same code path” for cleaner phrasing.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dev/specs/infp-431-ipam-closest-prefix/plan.md` at line 8, Update the summary
sentence in the plan to tighten wording by replacing “exact same code path” with
“same code path”; ensure the sentence still notes the feature is additive and
references the new IPParentPrefixLookupQuery (which adapts
IPPrefixReconcileQuery) and the “Parent Prefixes” section for Cmd+K search
results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/infrahub/core/query/ipam.py`:
- Around line 156-188: The query currently selects candidate AttributeIPNetwork
rows and walks back to parent_prefix without resolving branch-effective
relations, which can return stale ancestor values or detached prefixes; refactor
the block (the cypher built around ip_prefix_attribute_kind, ip_prefix_kind,
AttributeIPNetwork and parent_prefix) to follow the
IPPrefixReconcileQuery.get_new_parent_query() pattern: first shortlist
candidates using the index (binary_address IN $possible_prefix_list /
possible_prefix_and_length_list), then for each candidate resolve the latest
active HAS_VALUE relation and the latest active ip_namespace__ip_prefix /
IS_PART_OF (namespace link) relation using branch-level ordering and
head(collect(...)) logic to get branch-effective values, re-apply the prefixlen
matching test against those resolved active values, and only then RETURN
parent_prefix.uuid / kind / prefixlen so the returned parent_prefix is validated
as active on the current branch.

In `@backend/infrahub/graphql/queries/search.py`:
- Around line 142-143: The resolver currently uses the original input `q` for
text-search branches even when `_try_parse_ip_or_prefix(q)` returns a
canonicalized `parsed_ip` (e.g., turning host-bit CIDRs into network form), so
the exact prefix is lost from results; update the code in `search.py` to
replace/assign `q = parsed_ip` (or otherwise canonicalize the search string to
the parsed value) immediately after `_try_parse_ip_or_prefix(q)` succeeds,
before executing the `NodeGetListByAttributeValueQuery` / `NodeManager.query`
text-search branches, and add a regression test that calls the resolver with a
host-bit CIDR input (like `10.1.2.45/24`) asserting the normalized `10.1.2.0/24`
appears in `edges`.

In `@dev/specs/infp-431-ipam-closest-prefix/data-model.md`:
- Around line 43-55: The example IPParentPrefixResult.from_db accesses a nested
"parent_prefix" node but the actual query returns flat labels parent_prefix_uuid
and parent_prefix_kind; update the from_db implementation in
IPParentPrefixResult to read result.get("parent_prefix_uuid") and
result.get("parent_prefix_kind") (keeping the same field names prefix_id and
prefix_kind) so the dataclass aligns with the QueryResult produced by
IPParentPrefixLookupQuery.

In `@dev/specs/infp-431-ipam-closest-prefix/plan.md`:
- Around line 43-51: Update the structure section to use repository-relative
paths by changing the top entry from "specs/infp-431-ipam-closest-prefix/" to
"dev/specs/infp-431-ipam-closest-prefix/" and ensure any other listed entries
follow the same "dev/..." prefix convention; also make the Markdown guideline
reference consistent by keeping the exact quoted path
"dev/guidelines/markdown.md" in the comment so readers can follow the guideline
exactly.

In `@dev/specs/infp-431-ipam-closest-prefix/tasks.md`:
- Around line 3-5: Update the inconsistent root paths so all spec references
point to the repository's actual location under dev/specs; replace occurrences
of "specs/infp-431-ipam-closest-prefix/" with
"dev/specs/infp-431-ipam-closest-prefix/" in this file (including the header
Input line and the structure/prerequisites block listing plan.md, spec.md,
research.md, data-model.md, contracts/), and also fix the same pattern in the
other affected lines (mentions around lines 43-51) to ensure all Markdown links
and the referenced guideline rule remain valid.

---

Nitpick comments:
In `@dev/specs/infp-431-ipam-closest-prefix/plan.md`:
- Line 8: Update the summary sentence in the plan to tighten wording by
replacing “exact same code path” with “same code path”; ensure the sentence
still notes the feature is additive and references the new
IPParentPrefixLookupQuery (which adapts IPPrefixReconcileQuery) and the “Parent
Prefixes” section for Cmd+K search results.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ad547a7a-f8a0-4137-8d78-0d311bb99a61

📥 Commits

Reviewing files that changed from the base of the PR and between c9469aa and 633b167.

📒 Files selected for processing (16)
  • backend/infrahub/core/query/ipam.py
  • backend/infrahub/graphql/queries/search.py
  • backend/tests/component/graphql/queries/test_search.py
  • backend/tests/unit/graphql/queries/__init__.py
  • backend/tests/unit/graphql/queries/test_search.py
  • changelog/+ifc2271.added.md
  • dev/specs/infp-431-ipam-closest-prefix/contracts/graphql-schema.md
  • dev/specs/infp-431-ipam-closest-prefix/data-model.md
  • dev/specs/infp-431-ipam-closest-prefix/plan.md
  • dev/specs/infp-431-ipam-closest-prefix/quickstart.md
  • dev/specs/infp-431-ipam-closest-prefix/research.md
  • dev/specs/infp-431-ipam-closest-prefix/spec.md
  • dev/specs/infp-431-ipam-closest-prefix/tasks.md
  • frontend/app/src/shared/api/graphql/graphql-cache.d.ts
  • frontend/app/src/shared/api/graphql/graphql-env.d.ts
  • schema/schema.graphql

Also add a regression test cover this scenario.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
backend/infrahub/core/query/ipam.py (1)

177-211: ⚠️ Potential issue | 🟠 Major

Require a branch-effective namespace path before accepting maybe_parent.

This block revalidates the node and its latest prefix value, but it still never proves that maybe_parent is currently attached through an active ip_namespace__ip_prefix relation on this branch. A prefix detached from every namespace will still pass here and leak into parent_prefixes. Please mirror the parent_in_namespace step used by IPPrefixReconcileQuery.get_new_parent_query() before returning the candidate.

As per coding guidelines "Read dev/knowledge/backend/database-schema.md when writing or modifying Cypher queries to understand vertex/edge types, properties, and temporal branching rules".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/infrahub/core/query/ipam.py` around lines 177 - 211, The query must
require that maybe_parent has a branch-effective ip_namespace__ip_prefix
relation before accepting it; add a CALL block (mirroring
IPPrefixReconcileQuery.get_new_parent_query) that OPTIONAL MATCHes
(maybe_parent)-[r:ip_namespace__ip_prefix]->(ns:Namespace) with the same
%(branch_filter)s, computes r.status = "active" as is_active, ORDER BY
r.branch_level DESC, r.from DESC, r.status ASC and RETURN
head(collect(is_active)) = TRUE AS node_in_namespace, then filter WHERE
node_in_namespace = TRUE before continuing to the prefix-resolution CALL that
produces allowed_av so detached prefixes cannot be included in parent_prefixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@backend/infrahub/core/query/ipam.py`:
- Around line 177-211: The query must require that maybe_parent has a
branch-effective ip_namespace__ip_prefix relation before accepting it; add a
CALL block (mirroring IPPrefixReconcileQuery.get_new_parent_query) that OPTIONAL
MATCHes (maybe_parent)-[r:ip_namespace__ip_prefix]->(ns:Namespace) with the same
%(branch_filter)s, computes r.status = "active" as is_active, ORDER BY
r.branch_level DESC, r.from DESC, r.status ASC and RETURN
head(collect(is_active)) = TRUE AS node_in_namespace, then filter WHERE
node_in_namespace = TRUE before continuing to the prefix-resolution CALL that
produces allowed_av so detached prefixes cannot be included in parent_prefixes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9cc97b21-2ef7-45f5-aabd-09b6b2ea9c5a

📥 Commits

Reviewing files that changed from the base of the PR and between 633b167 and e63bb4f.

📒 Files selected for processing (2)
  • backend/infrahub/core/query/ipam.py
  • backend/tests/component/graphql/queries/test_search.py

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
backend/infrahub/core/query/ipam.py (1)

172-216: ⚠️ Potential issue | 🟠 Major

Re-check active namespace membership before returning maybe_parent.

After Line 175, maybe_parent is only validated against IS_PART_OF and its branch-effective prefix value. If a branch deletes the ip_namespace__ip_prefix link but leaves the node/value active, this prefix still passes and leaks into parent_prefixes. Please mirror the parent_in_namespace filtering pattern already used in IPPrefixReconcileQuery.get_new_parent_query() before the final RETURN.

As per coding guidelines "Read dev/knowledge/backend/database-schema.md when writing or modifying Cypher queries to understand vertex/edge types, properties, and temporal branching rules".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/infrahub/core/query/ipam.py` around lines 172 - 216, The query
returns maybe_parent without verifying branch-effective membership in the
ip_namespace__ip_prefix relationship, allowing prefixes deleted on a branch to
leak into parent_prefixes; mirror the parent_in_namespace filtering used in
IPPrefixReconcileQuery.get_new_parent_query(): add a CALL block (or OPTIONAL
MATCH) that finds (maybe_parent)-[r:ip_namespace__ip_prefix]->(:Namespace)
(using the same %(branch_filter)s), order by r.branch_level, r.from, r.status,
take head(collect(r)) and require that latest_r IS NOT NULL and latest_r.status
= "active" before allowing maybe_parent through; place this check after the
existing node_is_active and before the final RETURN so only branch-effective
namespace-linked prefixes are returned.
🧹 Nitpick comments (1)
backend/tests/component/graphql/queries/test_search.py (1)

514-572: Add the host-bits regression case alongside these IPv4 checks.

These cases cover plain addresses and canonical CIDRs, but not the split behavior that motivated the resolver change: a query like 10.10.1.1/24 should still match the stored interface-style value in edges while parent_prefixes are derived from the canonicalized network. Without that case, a future normalization of q in the text-search branch can regress silently.

Based on learnings "Normalizing q from the parsed IP would cause text search to miss objects stored with host bits set. Only the IPParentPrefixLookupQuery uses the parsed/normalized form."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/tests/component/graphql/queries/test_search.py` around lines 514 -
572, Add a test case in test_search_parent_prefix_ipv4 that submits the
host-bits query "10.10.1.1/24" via _search_with_parent_prefixes and asserts that
edges still include the stored interface-style object (e.g.
ip_dataset_01["address11"].id) while parent_prefixes are derived from the
canonical network (i.e. include broader parents like ip_dataset_01["net140"].id,
ip_dataset_01["net146"].id and/or ip_dataset_01["net240"].id) and explicitly
exclude the exact /24 prefix (ip_dataset_01["net142"].id); add this right after
the existing CIDR exact-match block so the test covers the host-bits regression
for IPParentPrefixLookupQuery and ensures resolver normalization doesn’t hide
host-bit-stored edges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@backend/infrahub/core/query/ipam.py`:
- Around line 172-216: The query returns maybe_parent without verifying
branch-effective membership in the ip_namespace__ip_prefix relationship,
allowing prefixes deleted on a branch to leak into parent_prefixes; mirror the
parent_in_namespace filtering used in
IPPrefixReconcileQuery.get_new_parent_query(): add a CALL block (or OPTIONAL
MATCH) that finds (maybe_parent)-[r:ip_namespace__ip_prefix]->(:Namespace)
(using the same %(branch_filter)s), order by r.branch_level, r.from, r.status,
take head(collect(r)) and require that latest_r IS NOT NULL and latest_r.status
= "active" before allowing maybe_parent through; place this check after the
existing node_is_active and before the final RETURN so only branch-effective
namespace-linked prefixes are returned.

---

Nitpick comments:
In `@backend/tests/component/graphql/queries/test_search.py`:
- Around line 514-572: Add a test case in test_search_parent_prefix_ipv4 that
submits the host-bits query "10.10.1.1/24" via _search_with_parent_prefixes and
asserts that edges still include the stored interface-style object (e.g.
ip_dataset_01["address11"].id) while parent_prefixes are derived from the
canonical network (i.e. include broader parents like ip_dataset_01["net140"].id,
ip_dataset_01["net146"].id and/or ip_dataset_01["net240"].id) and explicitly
exclude the exact /24 prefix (ip_dataset_01["net142"].id); add this right after
the existing CIDR exact-match block so the test covers the host-bits regression
for IPParentPrefixLookupQuery and ensures resolver normalization doesn’t hide
host-bit-stored edges.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a2cf5bb7-0420-41c8-9044-71ff70346b65

📥 Commits

Reviewing files that changed from the base of the PR and between e63bb4f and 05d248c.

📒 Files selected for processing (2)
  • backend/infrahub/core/query/ipam.py
  • backend/tests/component/graphql/queries/test_search.py

@gmazoyer gmazoyer force-pushed the gma-431-ipam-closest-prefix branch from 05d248c to 339b5d7 Compare March 17, 2026 11:39
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
backend/infrahub/core/query/ipam.py (1)

89-93: Consider adding from_db classmethod for consistency.

Other similar frozen dataclasses in this file (IPPrefixFreeData, IPv6PrefixFreeData, IPAddressFreeData, etc.) include a from_db(cls, result: QueryResult) classmethod. While get_data() directly constructs instances, adding from_db would maintain consistency and allow reuse if the result structure changes.

♻️ Suggested addition
 `@dataclass`(frozen=True)
 class IPParentPrefixResult:
     prefix_id: str
     prefix_kind: str
+
+    `@classmethod`
+    def from_db(cls, result: QueryResult) -> IPParentPrefixResult:
+        return cls(
+            prefix_id=result.get_as_type("parent_prefix_uuid", return_type=str),
+            prefix_kind=result.get_as_type("parent_prefix_kind", return_type=str),
+        )

Then in get_data():

     def get_data(self) -> list[IPParentPrefixResult]:
-        return [
-            IPParentPrefixResult(
-                prefix_id=result.get_as_type("parent_prefix_uuid", return_type=str),
-                prefix_kind=result.get_as_type("parent_prefix_kind", return_type=str),
-            )
-            for result in self.get_results()
-        ]
+        return [IPParentPrefixResult.from_db(result) for result in self.get_results()]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/infrahub/core/query/ipam.py` around lines 89 - 93, Add a from_db
classmethod to the frozen dataclass IPParentPrefixResult that accepts a
QueryResult (or similarly typed DB row) and returns an IPParentPrefixResult
built from the row fields; implement it analogously to other dataclasses in this
file (e.g., IPPrefixFreeData.from_db) mapping prefix_id and prefix_kind from the
result, and update the get_data() call sites to construct instances via
IPParentPrefixResult.from_db(result) to keep construction consistent and
reusable.
dev/specs/infp-431-ipam-closest-prefix/plan.md (1)

4-4: Use consistent repository-relative path.

Line 4 references /specs/infp-431-ipam-closest-prefix/spec.md, but this file is located at dev/specs/infp-431-ipam-closest-prefix/plan.md. For consistency with the actual file structure, update the path to dev/specs/infp-431-ipam-closest-prefix/spec.md.

📝 Proposed fix for path consistency
-**Input**: Feature specification from `/specs/infp-431-ipam-closest-prefix/spec.md`
+**Input**: Feature specification from `dev/specs/infp-431-ipam-closest-prefix/spec.md`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dev/specs/infp-431-ipam-closest-prefix/plan.md` at line 4, Update the
repository-relative path referenced in
dev/specs/infp-431-ipam-closest-prefix/plan.md by replacing the incorrect
"/specs/infp-431-ipam-closest-prefix/spec.md" string with the correct
"dev/specs/infp-431-ipam-closest-prefix/spec.md" so the link matches the actual
file location; ensure the updated path appears anywhere in plan.md where the old
path string is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@backend/infrahub/core/query/ipam.py`:
- Around line 89-93: Add a from_db classmethod to the frozen dataclass
IPParentPrefixResult that accepts a QueryResult (or similarly typed DB row) and
returns an IPParentPrefixResult built from the row fields; implement it
analogously to other dataclasses in this file (e.g., IPPrefixFreeData.from_db)
mapping prefix_id and prefix_kind from the result, and update the get_data()
call sites to construct instances via IPParentPrefixResult.from_db(result) to
keep construction consistent and reusable.

In `@dev/specs/infp-431-ipam-closest-prefix/plan.md`:
- Line 4: Update the repository-relative path referenced in
dev/specs/infp-431-ipam-closest-prefix/plan.md by replacing the incorrect
"/specs/infp-431-ipam-closest-prefix/spec.md" string with the correct
"dev/specs/infp-431-ipam-closest-prefix/spec.md" so the link matches the actual
file location; ensure the updated path appears anywhere in plan.md where the old
path string is used.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4819f071-6ff3-45b0-9b27-bc18e56f5ea8

📥 Commits

Reviewing files that changed from the base of the PR and between 05d248c and 339b5d7.

📒 Files selected for processing (16)
  • backend/infrahub/core/query/ipam.py
  • backend/infrahub/graphql/queries/search.py
  • backend/tests/component/graphql/queries/test_search.py
  • backend/tests/unit/graphql/queries/__init__.py
  • backend/tests/unit/graphql/queries/test_search.py
  • changelog/+ifc2271.added.md
  • dev/specs/infp-431-ipam-closest-prefix/contracts/graphql-schema.md
  • dev/specs/infp-431-ipam-closest-prefix/data-model.md
  • dev/specs/infp-431-ipam-closest-prefix/plan.md
  • dev/specs/infp-431-ipam-closest-prefix/quickstart.md
  • dev/specs/infp-431-ipam-closest-prefix/research.md
  • dev/specs/infp-431-ipam-closest-prefix/spec.md
  • dev/specs/infp-431-ipam-closest-prefix/tasks.md
  • frontend/app/src/shared/api/graphql/graphql-cache.d.ts
  • frontend/app/src/shared/api/graphql/graphql-env.d.ts
  • schema/schema.graphql
🚧 Files skipped from review as they are similar to previous changes (7)
  • dev/specs/infp-431-ipam-closest-prefix/spec.md
  • backend/tests/unit/graphql/queries/test_search.py
  • dev/specs/infp-431-ipam-closest-prefix/research.md
  • changelog/+ifc2271.added.md
  • dev/specs/infp-431-ipam-closest-prefix/data-model.md
  • dev/specs/infp-431-ipam-closest-prefix/contracts/graphql-schema.md
  • frontend/app/src/shared/api/graphql/graphql-cache.d.ts

@gmazoyer gmazoyer force-pushed the gma-431-ipam-closest-prefix branch from 339b5d7 to c8fd931 Compare March 17, 2026 16:21
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
backend/tests/component/graphql/queries/test_search.py (1)

514-573: Add a host-bit CIDR regression case.

These IPv4 assertions never exercise the 10.10.1.1/24 input that motivated keeping raw q for edges while only normalizing the parent-prefix lookup. Without that case, a future refactor can break the exact-match edge behavior without failing this suite.

Suggested addition
+    # Host-bit CIDR keeps the exact stored IP object in edges while parent lookup uses the normalized /24
+    data = await _search_with_parent_prefixes(gql_params=gql_params, query="10.10.1.1/24")
+    assert data["parent_prefixes"] is not None
+    edge_ids = {e["node"]["id"] for e in data["edges"]}
+    assert ip_dataset_01["address11"].id in edge_ids
+    parent_ids = {pp["node"]["id"] for pp in data["parent_prefixes"]}
+    assert ip_dataset_01["net143"].id not in parent_ids
+    assert ip_dataset_01["net140"].id in parent_ids

Based on learnings, "IP address objects can be stored with host bits set (e.g., 10.10.1.1/24 as a copy-pasted interface address). The search resolver intentionally keeps the original query string q for text-search branches ... Only the IPParentPrefixLookupQuery uses the parsed/normalized form."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/tests/component/graphql/queries/test_search.py` around lines 514 -
573, Add a regression case in test_search_parent_prefix_ipv4 that queries the
host-bit CIDR "10.10.1.1/24" (use the existing helper
_search_with_parent_prefixes) and assert that edges include the exact /24 prefix
(ip_dataset_01["net142"].id) while parent_prefixes excludes that exact match but
still contains the broader parents (e.g., ip_dataset_01["net140"].id and
ip_dataset_01["net146"].id); place the new assertions near the other
CIDR/exact-match checks in the same test so the behavior of edges (using raw q)
vs IPParentPrefixLookupQuery normalization is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@dev/specs/infp-431-ipam-closest-prefix/plan.md`:
- Around line 28-30: Update the table wording to avoid contradiction by
explicitly stating that there are no persistence/database schema changes: change
the Line referencing "No schema changes" to "No database/persistence schema
changes" and add a brief qualifier in the III row noting that the GraphQL API
contract is extended (new typed field parent_prefixes and IPParentPrefixResult)
but this does not alter the underlying DB schema; reference the existing symbols
BuiltinIPPrefix, AttributeIPNetwork, BuiltinIPNamespace, IPParentPrefixResult,
and parent_prefixes so reviewers can verify the distinction.

---

Nitpick comments:
In `@backend/tests/component/graphql/queries/test_search.py`:
- Around line 514-573: Add a regression case in test_search_parent_prefix_ipv4
that queries the host-bit CIDR "10.10.1.1/24" (use the existing helper
_search_with_parent_prefixes) and assert that edges include the exact /24 prefix
(ip_dataset_01["net142"].id) while parent_prefixes excludes that exact match but
still contains the broader parents (e.g., ip_dataset_01["net140"].id and
ip_dataset_01["net146"].id); place the new assertions near the other
CIDR/exact-match checks in the same test so the behavior of edges (using raw q)
vs IPParentPrefixLookupQuery normalization is validated.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c181da0e-ad1a-414c-ab94-84fb1719eece

📥 Commits

Reviewing files that changed from the base of the PR and between 339b5d7 and c8fd931.

📒 Files selected for processing (16)
  • backend/infrahub/core/query/ipam.py
  • backend/infrahub/graphql/queries/search.py
  • backend/tests/component/graphql/queries/test_search.py
  • backend/tests/unit/graphql/queries/__init__.py
  • backend/tests/unit/graphql/queries/test_search.py
  • changelog/+ifc2271.added.md
  • dev/specs/infp-431-ipam-closest-prefix/contracts/graphql-schema.md
  • dev/specs/infp-431-ipam-closest-prefix/data-model.md
  • dev/specs/infp-431-ipam-closest-prefix/plan.md
  • dev/specs/infp-431-ipam-closest-prefix/quickstart.md
  • dev/specs/infp-431-ipam-closest-prefix/research.md
  • dev/specs/infp-431-ipam-closest-prefix/spec.md
  • dev/specs/infp-431-ipam-closest-prefix/tasks.md
  • frontend/app/src/shared/api/graphql/graphql-cache.d.ts
  • frontend/app/src/shared/api/graphql/graphql-env.d.ts
  • schema/schema.graphql
✅ Files skipped from review due to trivial changes (1)
  • dev/specs/infp-431-ipam-closest-prefix/tasks.md
🚧 Files skipped from review as they are similar to previous changes (5)
  • dev/specs/infp-431-ipam-closest-prefix/spec.md
  • backend/tests/unit/graphql/queries/test_search.py
  • dev/specs/infp-431-ipam-closest-prefix/contracts/graphql-schema.md
  • dev/specs/infp-431-ipam-closest-prefix/research.md
  • changelog/+ifc2271.added.md

@gmazoyer gmazoyer force-pushed the gma-431-ipam-closest-prefix branch from c8fd931 to 7d19a9d Compare March 17, 2026 20:00
@github-actions github-actions bot added the type/spec A specification for an upcoming change to the project label Mar 17, 2026
@gmazoyer gmazoyer merged commit cb03370 into develop Mar 17, 2026
48 checks passed
@gmazoyer gmazoyer deleted the gma-431-ipam-closest-prefix branch March 17, 2026 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend Issue related to the backend (API Server, Git Agent) group/frontend Issue related to the frontend (React) type/spec A specification for an upcoming change to the project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants