Fix code for HFID casting of strings that aren't UUIDs #732
Conversation
…t have to provide HFID directly if single value HFID.
WalkthroughAdds HFID normalization utilities and applies them during object loading. Introduces import Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
infrahub_sdk/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)infrahub_sdk/spec/object.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (4)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
infrahub_sdk/spec/object.py (1)
37-48: Return type annotation is inconsistent with actual behavior.The function returns
strwhen the value is a UUID (line 47) butlist[str]otherwise. The current annotation-> list[str]is misleading to callers and requires atype: ignoreto silence the checker.🔎 Proposed fix
-def normalize_hfid_reference(value: str | list[str]) -> list[str]: +def normalize_hfid_reference(value: str | list[str]) -> str | list[str]: """Normalize a reference value to HFID format. If the value is a string and not a valid UUID, wrap it in a list to treat it as a single-component HFID. If the value is already a list, return it as-is. If the value is a UUID string, return it as-is (will be treated as an ID). """ if isinstance(value, list): return value if is_valid_uuid(value): - return value # type: ignore[return-value] + return value return [value]tests/unit/sdk/spec/test_object.py (1)
342-358: Consider refactoring duplicated mock pattern to a shared fixture.The
mock_createfunction andInfrahubNode.savepatch are duplicated across all 7 tests. Additionally, the first test uses a different pattern (original_create = client_with_schema_01.createon line 344) compared to others (client_with_schema_01.__class__.createon line 375).A shared fixture would reduce duplication and ensure consistency:
🔎 Suggested refactor
@pytest.fixture def mock_client_create(self, client_with_schema_01: InfrahubClient) -> tuple[InfrahubClient, list[dict]]: """Fixture to mock client.create and track calls.""" from unittest.mock import AsyncMock, patch create_calls: list[dict] = [] original_create = client_with_schema_01.create async def mock_create( kind: str, branch: str | None = None, data: dict | None = None, **kwargs: Any ) -> InfrahubNode: create_calls.append({"kind": kind, "data": data}) node = await original_create(kind=kind, branch=branch, data=data, **kwargs) return node client_with_schema_01.create = mock_create with patch("infrahub_sdk.node.InfrahubNode.save", new_callable=AsyncMock): yield client_with_schema_01, create_callsThen each test becomes simpler:
async def test_cardinality_one_string_hfid_normalized( self, mock_client_create: tuple[InfrahubClient, list[dict]], location_with_cardinality_one_string_hfid: dict ) -> None: client, create_calls = mock_client_create obj = ObjectFile(location="some/path", content=location_with_cardinality_one_string_hfid) await obj.validate_format(client=client) await obj.process(client=client) assert len(create_calls) == 1 assert create_calls[0]["data"]["primary_tag"] == [["Important"]]Also applies to: 371-383
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrahub_sdk/spec/object.pytests/unit/sdk/spec/test_object.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use type hints on all function signatures
Never mix async/sync inappropriately
Never bypass type checking without justification
Files:
tests/unit/sdk/spec/test_object.pyinfrahub_sdk/spec/object.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
tests/**/*.py: Usehttpx_mockfixture for HTTP mocking in tests instead of making real HTTP requests
Do not add@pytest.mark.asynciodecorator to async test functions (async auto-mode is globally enabled)
Files:
tests/unit/sdk/spec/test_object.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
Unit tests must be fast, mocked, and have no external dependencies
Files:
tests/unit/sdk/spec/test_object.py
infrahub_sdk/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Follow async/sync dual pattern for new features in the Python SDK
Files:
infrahub_sdk/spec/object.py
🧬 Code graph analysis (2)
tests/unit/sdk/spec/test_object.py (1)
infrahub_sdk/spec/object.py (5)
ObjectFile(663-691)validate_format(202-221)validate_format(684-688)process(223-236)process(690-691)
infrahub_sdk/spec/object.py (1)
infrahub_sdk/utils.py (1)
is_valid_uuid(85-91)
🪛 GitHub Actions: CI
tests/unit/sdk/spec/test_object.py
[error] 346-346: ANN202 Missing return type annotation for private function mock_create
[error] 346-346: ANN003 Missing type annotation for **kwargs
[warning] 346-347: PLC0415 import should be at the top-level of a file
[error] 373-373: ANN202 Missing return type annotation for private function mock_create
[error] 373-373: ANN003 Missing type annotation for **kwargs
[warning] 355-355: PLC0415 import should be at the top-level of a file
[warning] 380-380: PLC0415 import should be at the top-level of a file
[error] 397-397: ANN202 Missing return type annotation for private function mock_create
[error] 397-397: ANN003 Missing type annotation for **kwargs
[warning] 404-404: PLC0415 import should be at the top-level of a file
[error] 422-422: ANN202 Missing return type annotation for private function mock_create
[error] 422-422: ANN003 Missing type annotation for **kwargs
[warning] 429-429: PLC0415 import should be at the top-level of a file
[error] 446-446: ANN202 Missing return type annotation for private function mock_create
[error] 446-446: ANN003 Missing type annotation for **kwargs
[warning] 453-453: PLC0415 import should be at the top-level of a file
[error] 470-470: ANN202 Missing return type annotation for private function mock_create
[error] 470-470: ANN003 Missing type annotation for **kwargs
[warning] 477-477: PLC0415 import should be at the top-level of a file
[error] 495-495: ANN202 Missing return type annotation for private function mock_create
[error] 495-495: ANN003 Missing type annotation for **kwargs
[warning] 502-502: PLC0415 import should be at the top-level of a file
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
infrahub_sdk/spec/object.py (3)
10-10: LGTM!Import of
is_valid_uuidis correctly added to support UUID validation in the normalization functions.
51-56: LGTM!The function correctly applies normalization to each element in the list.
470-476: LGTM!The normalization logic is correctly integrated into the relationship handling. The inline comments clearly explain the behavior: string HFIDs are normalized to list format while UUIDs remain as strings.
tests/unit/sdk/spec/test_object.py (5)
268-278: LGTM!The test class and fixtures are well-structured with clear docstrings explaining each scenario.
279-333: Good test fixture coverage.The fixtures comprehensively cover cardinality-one and cardinality-many scenarios with strings, lists, UUIDs, and mixed inputs.
388-411: LGTM!The UUID test correctly verifies that UUIDs are passed through without being wrapped in a list, which aligns with the intended behavior of treating UUIDs as IDs rather than HFIDs.
461-484: LGTM!Good edge case coverage: the mixed HFID test validates that string HFIDs get wrapped while list HFIDs (multi-component) remain unchanged.
486-512: LGTM!The cardinality-many UUID test correctly validates that UUIDs remain as strings in the list, matching the normalization behavior.
Deploying infrahub-sdk-python with
|
| Latest commit: |
fc9b258
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://74623cf7.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://may-202512-object-load-many.infrahub-sdk-python.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## stable #732 +/- ##
==========================================
+ Coverage 76.84% 76.86% +0.02%
==========================================
Files 113 113
Lines 9767 9776 +9
Branches 1497 1499 +2
==========================================
+ Hits 7505 7514 +9
Misses 1757 1757
Partials 505 505
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/sdk/spec/test_object.py (2)
335-362: Refactor: Inconsistent mock pattern across tests.The first test (lines 335-362) uses:
original_create = client_with_schema_01.create # ... later ... return await original_create(kind=kind, branch=branch, data=data, **kwargs)While the remaining tests (lines 364-530) use:
original_create = client_with_schema_01.__class__.create # ... later ... return await original_create(client_with_schema_01, kind=kind, branch=branch, data=data, **kwargs)Both approaches work, but the inconsistency reduces code clarity. Consider standardizing on one pattern throughout the test class.
🔎 Recommended approach
Use the first pattern (instance method) consistently, as it's simpler and doesn't require passing
selfexplicitly:original_create = client_with_schema_01.create return await original_create(kind=kind, branch=branch, data=data, **kwargs)Also applies to: 364-530
335-530: Refactor: Extract common mock setup to reduce duplication.Each test method repeats nearly identical
mock_createsetup code. Consider extracting this into a helper fixture or method to improve maintainability.🔎 Suggested refactoring approach
Add a fixture or helper method to capture create calls:
@pytest.fixture def create_call_tracker(self, client_with_schema_01: InfrahubClient): """Fixture that tracks calls to client.create.""" create_calls: list[dict[str, Any]] = [] original_create = client_with_schema_01.create async def mock_create( kind: str, branch: str | None = None, data: dict | None = None, **kwargs: Any, # noqa: ANN401 ) -> InfrahubNode: create_calls.append({"kind": kind, "data": data}) return await original_create(kind=kind, branch=branch, data=data, **kwargs) client_with_schema_01.create = mock_create return create_callsThen simplify each test to:
async def test_cardinality_one_string_hfid_normalized( self, client_with_schema_01: InfrahubClient, location_with_cardinality_one_string_hfid: dict, create_call_tracker: list[dict[str, Any]] ) -> None: obj = ObjectFile(location="some/path", content=location_with_cardinality_one_string_hfid) await obj.validate_format(client=client_with_schema_01) with patch("infrahub_sdk.node.InfrahubNode.save", new_callable=AsyncMock): await obj.process(client=client_with_schema_01) assert len(create_call_tracker) == 1 assert create_call_tracker[0]["data"]["primary_tag"] == [["Important"]]This eliminates ~15 lines of duplicated code per test.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/sdk/spec/test_object.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use type hints on all function signatures
Never mix async/sync inappropriately
Never bypass type checking without justification
Files:
tests/unit/sdk/spec/test_object.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
tests/**/*.py: Usehttpx_mockfixture for HTTP mocking in tests instead of making real HTTP requests
Do not add@pytest.mark.asynciodecorator to async test functions (async auto-mode is globally enabled)
Files:
tests/unit/sdk/spec/test_object.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
Unit tests must be fast, mocked, and have no external dependencies
Files:
tests/unit/sdk/spec/test_object.py
🧠 Learnings (4)
📚 Learning: 2025-12-10T17:13:37.990Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: tests/AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:37.990Z
Learning: Configure pytest with async auto-mode enabled
Applied to files:
tests/unit/sdk/spec/test_object.py
📚 Learning: 2025-12-10T17:13:08.136Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:08.136Z
Learning: Applies to infrahub_sdk/client.py : Always provide both async and sync versions of client implementations in InfrahubClient
Applied to files:
tests/unit/sdk/spec/test_object.py
📚 Learning: 2025-12-10T17:13:21.977Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: infrahub_sdk/ctl/AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:21.977Z
Learning: Applies to infrahub_sdk/ctl/**/*.py : Do not instantiate InfrahubClient directly; always use initialize_client() or initialize_client_sync() helper functions
Applied to files:
tests/unit/sdk/spec/test_object.py
📚 Learning: 2025-12-10T17:13:29.593Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: infrahub_sdk/pytest_plugin/AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:29.593Z
Learning: Applies to infrahub_sdk/pytest_plugin/**/infrahub_sdk/pytest_plugin/items/*.py : Inherit from `InfrahubItem` base class when creating new test item classes in `infrahub_sdk/pytest_plugin/items/`
Applied to files:
tests/unit/sdk/spec/test_object.py
🧬 Code graph analysis (1)
tests/unit/sdk/spec/test_object.py (1)
infrahub_sdk/spec/object.py (5)
ObjectFile(663-691)validate_format(202-221)validate_format(684-688)process(223-236)process(690-691)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.14)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.12)
- GitHub Check: unit-tests (3.10)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
tests/unit/sdk/spec/test_object.py (2)
3-4: LGTM: Past review comments addressed.The imports have been correctly moved to the top level, and
InfrahubNodehas been added to theTYPE_CHECKINGblock. This resolves the previous linting warnings (PLC0415, ANN003, ANN202).Also applies to: 13-13
270-333: LGTM: Comprehensive fixture coverage.The test fixtures provide thorough coverage of HFID normalization scenarios across different cardinalities and input formats (strings, lists, UUIDs, mixed).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/sdk/spec/test_object.py (1)
335-532: Standardize the mock_create implementation pattern.The first test (lines 343-353) stores
original_createbefore replacingclient.create, then calls the stored reference. The remaining tests (lines 374-382, 402-410, etc.) access the original viaclient_with_schema_01.__class__.createwithout storing it first. While both approaches work, this inconsistency may confuse future maintainers.🔎 Suggested approach: Use the __class__.create pattern consistently
Update the first test to match the pattern used in other tests:
async def test_cardinality_one_string_hfid_normalized( self, client_with_schema_01: InfrahubClient, location_with_cardinality_one_string_hfid: dict ) -> None: """String HFID for cardinality-one should be wrapped in a list.""" obj = ObjectFile(location="some/path", content=location_with_cardinality_one_string_hfid) await obj.validate_format(client=client_with_schema_01) # Track calls to client.create create_calls: list[dict[str, Any]] = [] - original_create = client_with_schema_01.create async def mock_create( kind: str, branch: str | None = None, data: dict | None = None, **kwargs: Any, # noqa: ANN401 ) -> InfrahubNode: create_calls.append({"kind": kind, "data": data}) - return await original_create(kind=kind, branch=branch, data=data, **kwargs) + original_create = client_with_schema_01.__class__.create + return await original_create(client_with_schema_01, kind=kind, branch=branch, data=data, **kwargs) client_with_schema_01.create = mock_create
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrahub_sdk/spec/object.pytests/unit/sdk/spec/test_object.py
🚧 Files skipped from review as they are similar to previous changes (1)
- infrahub_sdk/spec/object.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use type hints on all function signatures
Never mix async/sync inappropriately
Never bypass type checking without justification
Files:
tests/unit/sdk/spec/test_object.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
tests/**/*.py: Usehttpx_mockfixture for HTTP mocking in tests instead of making real HTTP requests
Do not add@pytest.mark.asynciodecorator to async test functions (async auto-mode is globally enabled)
Files:
tests/unit/sdk/spec/test_object.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
Unit tests must be fast, mocked, and have no external dependencies
Files:
tests/unit/sdk/spec/test_object.py
🧠 Learnings (4)
📚 Learning: 2025-12-10T17:13:37.990Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: tests/AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:37.990Z
Learning: Configure pytest with async auto-mode enabled
Applied to files:
tests/unit/sdk/spec/test_object.py
📚 Learning: 2025-12-10T17:13:08.136Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:08.136Z
Learning: Applies to infrahub_sdk/client.py : Always provide both async and sync versions of client implementations in InfrahubClient
Applied to files:
tests/unit/sdk/spec/test_object.py
📚 Learning: 2025-12-10T17:13:21.977Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: infrahub_sdk/ctl/AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:21.977Z
Learning: Applies to infrahub_sdk/ctl/**/*.py : Do not instantiate InfrahubClient directly; always use initialize_client() or initialize_client_sync() helper functions
Applied to files:
tests/unit/sdk/spec/test_object.py
📚 Learning: 2025-12-10T17:13:29.593Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: infrahub_sdk/pytest_plugin/AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:29.593Z
Learning: Applies to infrahub_sdk/pytest_plugin/**/infrahub_sdk/pytest_plugin/items/*.py : Inherit from `InfrahubItem` base class when creating new test item classes in `infrahub_sdk/pytest_plugin/items/`
Applied to files:
tests/unit/sdk/spec/test_object.py
🧬 Code graph analysis (1)
tests/unit/sdk/spec/test_object.py (4)
infrahub_sdk/node/node.py (1)
InfrahubNode(483-1269)infrahub_sdk/client.py (1)
InfrahubClient(318-1744)infrahub_sdk/spec/object.py (5)
ObjectFile(665-693)validate_format(202-221)validate_format(686-690)process(223-236)process(692-693)infrahub_sdk/schema/main.py (1)
kind(279-280)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.14)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.12)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
tests/unit/sdk/spec/test_object.py (1)
3-4: LGTM! Imports properly organized.The imports have been moved to the top level and type annotations support added, addressing the previous CI failures.
Also applies to: 13-13
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrahub_sdk/spec/object.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use type hints on all function signatures
Never mix async/sync inappropriately
Never bypass type checking without justification
Files:
infrahub_sdk/spec/object.py
infrahub_sdk/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Follow async/sync dual pattern for new features in the Python SDK
Files:
infrahub_sdk/spec/object.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.14)
- GitHub Check: unit-tests (3.12)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.13)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
infrahub_sdk/spec/object.py (2)
51-56: LGTM with a note on type dependencies.The function correctly normalizes a list of references by applying
normalize_hfid_referenceto each item. The return typelist[str | list[str]]accurately reflects the polymorphic nature of the normalized values (strings for UUIDs, lists for HFIDs).Once the return type of
normalize_hfid_referenceis corrected tostr | list[str], this function will be fully type-safe without any suppressions.
470-479: Well-structured HFID normalization for relationship values.The updated logic correctly handles both cardinality cases:
- MANY_REF: Normalizes each reference in the list, converting non-UUID strings to HFID format
- ONE_REF: Normalizes the single reference value, preserving UUIDs as strings and wrapping non-UUID strings
The inline comments clearly explain how
RelatedNodeinterprets the different formats. This implementation addresses the bug described in issue #731 where plain strings weren't being cast into the expected HFID format.
This allows the user to pass in a single for single HFID values as per the documentation
Fixes #731
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.