Skip to content

fix: resolve from_pool string names to pool IDs in object spec #862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
FragmentedPacket wants to merge 1 commit into from
Closed

fix: resolve from_pool string names to pool IDs in object spec #862

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion infrahub_sdk/spec/object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
from ..node import InfrahubNode
from ..schema import MainSchemaTypesAPI, RelationshipSchema

_ATTRIBUTE_KIND_TO_POOL_KIND: dict[str, str] = {
"Number": "CoreNumberPool",
}


def validate_list_of_scalars(value: list[Any]) -> bool:
return all(isinstance(item, (str, int, float, bool)) for item in value)
Expand Down Expand Up @@ -426,6 +430,30 @@ async def validate_related_nodes(
def enrich_node(cls, data: dict, context: dict) -> dict: # noqa: ARG003
return data

@classmethod
async def _resolve_attribute_pool(
cls,
client: InfrahubClient,
schema: MainSchemaTypesAPI,
name: str,
value: Any,
branch: str | None = None,
) -> Any:
"""If an attribute value has from_pool as a plain string name, resolve it to {id: uuid}."""
if not isinstance(value, dict) or "from_pool" not in value:
return value
from_pool = value["from_pool"]
if not isinstance(from_pool, str):
return value
attr_schema = schema.get_attribute_or_none(name=name)
if attr_schema is None:
return value
pool_kind = _ATTRIBUTE_KIND_TO_POOL_KIND.get(attr_schema.kind)
if pool_kind is None:
return value
pool = await client.get(kind=pool_kind, hfid=[from_pool], branch=branch, raise_when_missing=True)
return {**value, "from_pool": {"id": pool.id}}
Comment on lines +433 to +455
Copy link

@coderabbitai coderabbitai bot Mar 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Handle UUID from_pool strings before doing an HFID lookup.

Right now every plain string goes through hfid=[from_pool] on Line 454. If callers pass a pool UUID string instead of a name, this path misses the direct-id case even though the mutation payload wants {"id": "<uuid>"}.

💡 Suggested fix 
     async def _resolve_attribute_pool(
         cls,
         client: InfrahubClient,
         schema: MainSchemaTypesAPI,
         name: str,
         value: Any,
         branch: str | None = None,
     ) -> Any:
         """If an attribute value has from_pool as a plain string name, resolve it to {id: uuid}."""
         if not isinstance(value, dict) or "from_pool" not in value:
             return value
         from_pool = value["from_pool"]
         if not isinstance(from_pool, str):
             return value
+        if is_valid_uuid(from_pool):
+            return {**value, "from_pool": {"id": from_pool}}
         attr_schema = schema.get_attribute_or_none(name=name)
         if attr_schema is None:
             return value
         pool_kind = _ATTRIBUTE_KIND_TO_POOL_KIND.get(attr_schema.kind)
         if pool_kind is None:
             return value
         pool = await client.get(kind=pool_kind, hfid=[from_pool], branch=branch, raise_when_missing=True)
         return {**value, "from_pool": {"id": pool.id}}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@infrahub_sdk/spec/object.py` around lines 433 - 455, The
_resolve_attribute_pool flow currently always treats a string from_pool as an
HFID lookup; update InfrahubObject._resolve_attribute_pool to first detect if
from_pool is a UUID string and, if so, return the value with from_pool set to
{"id": from_pool} immediately (before calling client.get). Use the existing
local variable from_pool and the attr_schema/_ATTRIBUTE_KIND_TO_POOL_KIND checks
as before; only call client.get(kind=pool_kind, hfid=[from_pool], ...) when
from_pool is not a UUID.

Copy link
Contributor

@gmazoyer gmazoyer Mar 6, 2026

Choose a reason for hiding this comment

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

I think the bot highlights a valid point here, since we have is_valid_uuid, we could check if the string is a valid UUID prior to making a HFID-based query. That would spare us a possible useless query.

Also can you add a test to validate how the fix should behave and prevent possible regressions?

Copy link

@coderabbitai coderabbitai bot Mar 6, 2026

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find test files related to spec/object or InfrahubObjectFileData
fd -e py . tests/ | xargs grep -l "object\|InfrahubObjectFileData\|create_node\|resolve_attribute" 2>/dev/null | head -20

# Also look at the test directory structure
tree tests/ -L 3 2>/dev/null || find tests/ -type f -name "*.py" | head -30

Length of output: 5195

🏁 Script executed:

#!/bin/bash
cat tests/unit/sdk/spec/test_object.py
echo "---SEPARATOR---"
cat tests/unit/sdk/pool/test_attribute_from_pool.py
echo "---SEPARATOR---"
cat tests/unit/sdk/pool/conftest.py

Length of output: 26453

🏁 Script executed:

#!/bin/bash
# Check if there's a conftest in the spec directory
cat tests/unit/sdk/spec/conftest.py 2>/dev/null || echo "NO SPEC CONFTEST"

# Also check the unit/sdk conftest.py for client_with_schema_01 and schema setup
cat tests/unit/sdk/conftest.py

echo "---SEPARATOR---"
# Look at the pool conftest for vlan_schema
cat tests/unit/sdk/pool/conftest.py | grep -A 60 "vlan_schema"

Length of output: 88643

@gmazoyer, agreed on both points — the UUID fast-path is a valid improvement that avoids a redundant network round-trip. Here's the plan:

  • Apply UUID fast-path in _resolve_attribute_pool before the HFID query
  • Add unit tests for _resolve_attribute_pool directly, covering all four cases

💡 Use @coderabbitai plan to create a plan for code changes


@classmethod
async def create_node(
cls,
Expand Down Expand Up @@ -463,7 +491,9 @@ async def create_node(

for key, value in data.items():
if key in schema.attribute_names:
clean_data[key] = value
clean_data[key] = await cls._resolve_attribute_pool(
client=client, schema=schema, name=key, value=value, branch=branch
)
continue

if key in schema.relationship_names:
Expand Down