Re-implement casting of HFIDs to a list if the rel peer schema has hf… #746
Conversation
WalkthroughAdds HFID normalization utilities ( 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
infrahub_sdk/spec/object.pytests/fixtures/schema_01.jsontests/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:
infrahub_sdk/spec/object.pytests/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
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
🪛 GitHub Actions: CI
infrahub_sdk/spec/object.py
[error] 427-427: Ruff: PLR0915 Too many statements (51 > 50) in async def create_node.
🔇 Additional comments (11)
infrahub_sdk/spec/object.py (5)
10-10: LGTM!Import of
is_valid_uuidis appropriate for the UUID detection logic in the new normalization functions.
37-54: LGTM!The normalization logic is clear and well-documented. The function correctly handles all three cases: lists (unchanged), UUIDs (unchanged), and non-UUID strings (wrapped in list for single-component HFID).
57-64: LGTM!Simple and correct mapping of the reference normalization over a list of values.
85-85: LGTM!The
peer_has_hfidfield with a conservativeFalsedefault ensures normalization is only applied when explicitly determined.
153-154: LGTM!The
peer_has_hfidis correctly set after fetching the peer schema. Usingbool()properly handlesNone, empty lists, and non-empty lists.tests/fixtures/schema_01.json (1)
245-248: LGTM!Adding
human_friendly_idto the Tag node enables the HFID normalization tests. TheBuiltinLocationschema has relationships toBuiltinTag(bothtagsandprimary_tag), so this change properly supports testing both cardinality-one and cardinality-many HFID normalization scenarios.Note: The AI summary mentions
human_friendly_idwas added toGraphQLQuery, but the actual code shows onlyTagreceived this field. This is not an issue—the test coverage targets theBuiltinLocation→BuiltinTagrelationships.tests/unit/sdk/spec/test_object.py (5)
3-20: LGTM!Imports are appropriate for the new test infrastructure. Good use of
TYPE_CHECKINGfor theInfrahubNodetype hint.
277-336: LGTM!Comprehensive test cases covering all HFID normalization scenarios: cardinality-one (string, list, UUID) and cardinality-many (strings, lists, mixed, UUIDs). The dataclass provides clear structure for parameterized testing.
339-376: Well-structured integration test for HFID normalization.The test correctly:
- Mocks
InfrahubNode.saveto prevent HTTP calls- Captures the data passed to
client.createfor assertion- Validates both cardinality-one and cardinality-many normalization
One minor suggestion: Consider using
patch.object(client_with_schema_01, 'create', ...)for consistency with thepatchusage forsave, though the direct assignment works fine for tests.
379-469: LGTM!Excellent unit test coverage for
normalize_hfid_reference. The test cases clearly demonstrate the expected behavior difference whenpeer_has_hfidis True vs False, and the assertions verify both the type and value of the output.
472-529: LGTM!Good test coverage for verifying the actual GraphQL payload structure produced by
RelatedNode. While_generate_input_data()is a private method, testing it directly here is appropriate to verify the critical{"id": ...}vs{"hfid": [...]}payload distinction that the HFID normalization feature relies upon.
Deploying infrahub-sdk-python with
|
| Latest commit: |
e96c799
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1529be28.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://may-202601-hfid-object-loadi.infrahub-sdk-python.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## stable #746 +/- ##
=======================================
Coverage 80.30% 80.30%
=======================================
Files 115 115
Lines 9874 9885 +11
Branches 1518 1520 +2
=======================================
+ Hits 7929 7938 +9
- Misses 1417 1418 +1
- Partials 528 529 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/sdk/spec/test_object.py (1)
357-377: Consider usingpatch.objectfor cleaner fixture handling.Directly assigning
client_with_schema_01.create = mock_createmutates the fixture without restoration. While function-scoped fixtures typically get recreated, usingpatch.objectensures consistent cleanup and is more idiomatic.🔧 Suggested refactor using patch.object
- client_with_schema_01.create = mock_create - - with patch("infrahub_sdk.node.InfrahubNode.save", new_callable=AsyncMock): + with ( + patch.object(client_with_schema_01, "create", side_effect=mock_create), + patch("infrahub_sdk.node.InfrahubNode.save", new_callable=AsyncMock), + ): await obj.process(client=client_with_schema_01)
📜 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
🧬 Code graph analysis (1)
tests/unit/sdk/spec/test_object.py (1)
infrahub_sdk/spec/object.py (9)
spec(676-682)ObjectFile(672-700)RelationshipDataFormat(67-75)get_relationship_info(136-199)normalize_hfid_reference(37-54)validate_format(212-231)validate_format(693-697)process(233-246)process(699-700)
⏰ 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.12)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.14)
- GitHub Check: unit-tests (3.13)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
tests/unit/sdk/spec/test_object.py (6)
1-21: LGTM!Imports are well-organized with appropriate use of
TYPE_CHECKINGfor type-hint-only imports. The new imports align with the test requirements for mocking and HFID normalization testing.
277-336: LGTM!The
HfidLoadTestCasedataclass is well-structured with proper type hints. The test cases provide comprehensive coverage including string-to-list normalization, UUID passthrough, and cardinality-many scenarios with mixed formats.
379-436: LGTM!The
GraphQLPayloadTestCasedataclass is well-documented with a clear explanation of howRelatedNodeinterprets data formats. The test cases provide good coverage of the normalization matrix for both HFID-enabled and non-HFID peers.
439-469: LGTM!The synchronous test is appropriate for the sync
normalize_hfid_referencefunction. The conditional normalization logic (based onpeer_has_hfid) correctly mirrors the expected runtime behavior, and assertions include helpful error messages.
472-500: LGTM!The
RelatedNodePayloadTestCasestructure is clean with appropriate test cases covering the key payload scenarios: UUID-to-id and HFID-to-hfid transformations.
503-527: No changes needed. The test appropriately verifies the GraphQL payload structure.
_generate_input_data()is the standard internal protocol for payload generation across the entire codebase and is the only way to access the combined payload structure. This method is defined inprotocols_base.pyas part of the contract and is used consistently in integration tests, unit tests, and core implementations. Testing this internal contract is appropriate for unit tests, and there is no public alternative API available.
Reimplements #732 but only casting if the peer schema has an HFID defined.
Relevant CI runs in Infrahub: opsmill/infrahub#8096
Summary by CodeRabbit
New Features
Tests
Schema
✏️ Tip: You can customize this high-level summary in your review settings.