Fix optional relationships preventing object template defaults from a… #710
Conversation
…pplying (#634) * Initial plan * Fix optional attributes not being set when using object templates - Modified _generate_input_data() to skip adding None for optional relationships on new nodes - For existing nodes, still include None to allow clearing relationships (preserves PR #515 behavior) - Updated tests to reflect new behavior - Added test to verify existing nodes still work correctly Co-authored-by: minitriga <26367336+minitriga@users.noreply.github.com> * Add type annotations to test parameters and use BothClients fixture - Added BothClients import to TYPE_CHECKING block - Updated test_update_input_data_existing_node_with_optional_relationship to use clients fixture with proper type annotations - Changed from client fixture to clients fixture (clients.standard and clients.sync) - Added type annotation for client_type parameter Co-authored-by: minitriga <26367336+minitriga@users.noreply.github.com> * Use __future__ annotations and add changelog for issue 630 - Added `from __future__ import annotations` to enable postponed evaluation - Changed `clients: "BothClients"` to `clients: BothClients` without string quotes - Moved GenericSchema, NodeSchemaAPI, and HTTPXMock imports to TYPE_CHECKING block - Added changelog/630.fixed.md documenting the fix Co-authored-by: minitriga <26367336+minitriga@users.noreply.github.com> * Fix linting: correct import path in TYPE_CHECKING block Changed `from tests.unit.sdk.conftest import BothClients` to `from .conftest import BothClients` to fix ruff import sorting error after merge from stable branch. Co-authored-by: minitriga <26367336+minitriga@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: minitriga <26367336+minitriga@users.noreply.github.com> Co-authored-by: Alex Gittings <agitting96@gmail.com>
WalkthroughThe pull request includes a changelog entry documenting a SDK fix for handling uninitialized optional relationships. The Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #710 +/- ##
==========================================
+ Coverage 75.48% 76.03% +0.55%
==========================================
Files 113 113
Lines 9512 9744 +232
Branches 1893 1491 -402
==========================================
+ Hits 7180 7409 +229
- Misses 1832 1840 +8
+ Partials 500 495 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 26 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/sdk/test_node.py (1)
22-28: Duplicate import:HTTPXMockis already imported at runtime.
HTTPXMockis imported at line 8 (from pytest_httpx import HTTPXMock) and again inside theTYPE_CHECKINGblock at line 23. Since it's needed at runtime for the test function signatures, the TYPE_CHECKING import is redundant.🔎 Suggested fix
if TYPE_CHECKING: - from pytest_httpx import HTTPXMock - from infrahub_sdk.client import InfrahubClient, InfrahubClientSync from infrahub_sdk.schema import GenericSchema, NodeSchemaAPI from .conftest import BothClients
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelog/630.fixed.mdinfrahub_sdk/node/node.pytests/unit/sdk/test_node.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/test_node.pyinfrahub_sdk/node/node.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/test_node.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/test_node.py
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{md,mdx}: Usetextlanguage for directory structure code blocks in markdown documentation
Add blank lines before and after lists in markdown documentation
Always specify language in fenced code blocks in markdown documentation
Files:
changelog/630.fixed.md
infrahub_sdk/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Follow async/sync dual pattern for new features in the Python SDK
Files:
infrahub_sdk/node/node.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: wvandeun
Repo: opsmill/infrahub-sdk-python PR: 637
File: infrahub_sdk/operation.py:74-76
Timestamp: 2025-11-25T13:23:15.190Z
Learning: In infrahub_sdk/operation.py, the recursive=True parameter in _process_relationships is a temporary workaround to access Proposed Changes data. This will be replaced with proper object-level metadata implementation in version 1.7.
📚 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/test_node.py
📚 Learning: 2025-11-25T13:23:15.190Z
Learnt from: wvandeun
Repo: opsmill/infrahub-sdk-python PR: 637
File: infrahub_sdk/operation.py:74-76
Timestamp: 2025-11-25T13:23:15.190Z
Learning: In infrahub_sdk/operation.py, the recursive=True parameter in _process_relationships is a temporary workaround to access Proposed Changes data. This will be replaced with proper object-level metadata implementation in version 1.7.
Applied to files:
infrahub_sdk/node/node.py
🧬 Code graph analysis (2)
tests/unit/sdk/test_node.py (1)
infrahub_sdk/node/node.py (1)
_generate_input_data(195-284)
infrahub_sdk/node/node.py (6)
infrahub_sdk/node/attribute.py (1)
_generate_input_data(74-105)infrahub_sdk/node/property.py (1)
_generate_input_data(23-24)infrahub_sdk/node/related_node.py (1)
_generate_input_data(128-145)infrahub_sdk/node/relationship.py (1)
_generate_input_data(59-60)infrahub_sdk/protocols_base.py (1)
_generate_input_data(35-35)infrahub_sdk/yaml.py (1)
data(147-150)
⏰ 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). (4)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.14)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
infrahub_sdk/node/node.py (1)
195-242: LGTM! Clean implementation of the optional relationship fix.The conditional logic correctly distinguishes between new and existing nodes:
- For existing nodes (
self._existing is True):Noneis included to allow clearing relationships- For new nodes: the field is omitted to allow object template defaults to apply
The added comment clearly documents the intent of this behavior.
tests/unit/sdk/test_node.py (3)
1405-1435: Well-structured test for the existing node behavior.The test correctly validates that:
- Existing nodes (those with an
id) includeNonefor uninitialized optional relationships- Both sync and async clients are covered
The docstring clearly explains the test's purpose, and the use of the
BothClientsfixture follows the pattern established in other tests.
1680-1696: Test expectations correctly updated.The expected output for new nodes now omits uninitialized optional relationships, aligning with the implementation change that allows object template defaults to apply.
1361-1375: Test correctly reflects new behavior for new nodes.The expected output no longer includes
primary_tag: Nonefor new nodes, which aligns with the implementation change that omits optional uninitialized relationships to allow object template defaults to apply.changelog/630.fixed.md (1)
1-1: Clear and accurate changelog entry.The entry concisely explains the issue (explicit
nullvalues for uninitialized optional relationships) and its impact (preventing backend template defaults from applying).
…pplying (#634)
Initial plan
Fix optional attributes not being set when using object templates
from __future__ import annotationsto enable postponed evaluationclients: "BothClients"toclients: BothClientswithout string quotesChanged
from tests.unit.sdk.conftest import BothClientstofrom .conftest import BothClientsto fix ruff import sorting error after merge from stable branch.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.