IHS-152 Allow unsetting one-to-one relationship #515
Conversation
WalkthroughThe change introduces explicit None handling across GraphQL conversion and node input generation. In Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #515 +/- ##
==========================================
+ Coverage 75.74% 75.76% +0.02%
==========================================
Files 100 100
Lines 8846 8858 +12
Branches 1732 1737 +5
==========================================
+ Hits 6700 6711 +11
Misses 1670 1670
- Partials 476 477 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Deploying infrahub-sdk-python with
|
| Latest commit: |
b49f915
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://94fa5ca1.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://gma-20250827-ihs152.infrahub-sdk-python.pages.dev |
| "id": "llllllll-llll-llll-llll-llllllllllll", | ||
| "name": {"value": "DFW"}, | ||
| # "primary_tag": None, | ||
| "primary_tag": None, |
There was a problem hiding this comment.
I think having these should be alright. This does not seem to have a negative impact.
| if item in data and (data_item in ({}, []) or (data_item is None and original_data_item_is_none)): | ||
| data.pop(item) | ||
|
|
||
| def _strip_unmodified(self, data: dict, variables: dict) -> tuple[dict, dict]: |
There was a problem hiding this comment.
The whole "strip unmodified" code feels difficult to read. I think we probably want to take on the burden of trying to improve it to make it easier to maintain.
There was a problem hiding this comment.
I agree, we should probably look at the schema of the node as an initial starting point when refactoring and figure out what we want this to look like.
![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/graphql.py (1)
11-13: Type hint nit: Any makes the union redundantIncluding Any in the parameter type negates the benefit of the detailed union. Consider removing Any (or narrowing types) to improve static checking.
-def convert_to_graphql_as_string( # noqa: PLR0911 - value: str | bool | list | BaseModel | Enum | Any | None, convert_enum: bool = False -) -> str: +def convert_to_graphql_as_string( # noqa: PLR0911 + value: str | bool | int | float | list | BaseModel | Enum | None, convert_enum: bool = False +) -> str:infrahub_sdk/node/node.py (1)
313-314: Remove the informal aside in a TODOThe Thanos quote is funny but adds noise to already complex logic. Consider removing to keep focus.
- # TODO: I do not feel _great_ about this - # -> I don't even know who you are (but this is not great indeed) -- gmazoyer (quoting Thanos) + # TODO: This logic could be simplified/clarified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
infrahub_sdk/graphql.py(1 hunks)infrahub_sdk/node/node.py(3 hunks)tests/integration/test_infrahub_client.py(1 hunks)tests/unit/sdk/test_node.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
tests/integration/test_infrahub_client.pyinfrahub_sdk/graphql.pytests/unit/sdk/test_node.pyinfrahub_sdk/node/node.py
tests/integration/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write integration tests under tests/integration/ (tests against real Infrahub instances)
Files:
tests/integration/test_infrahub_client.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Files:
tests/integration/test_infrahub_client.pytests/unit/sdk/test_node.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write unit tests under tests/unit/ (isolated component tests)
Files:
tests/unit/sdk/test_node.py
🧬 Code graph analysis (3)
tests/integration/test_infrahub_client.py (3)
infrahub_sdk/testing/schemas/animal.py (1)
person_sophia(161-164)infrahub_sdk/node/node.py (2)
save(600-627)save(1226-1249)infrahub_sdk/client.py (14)
get(364-380)get(383-399)get(402-418)get(421-437)get(440-456)get(459-475)get(477-535)get(2040-2056)get(2059-2075)get(2078-2094)get(2097-2113)get(2116-2132)get(2135-2151)get(2153-2211)
infrahub_sdk/graphql.py (1)
infrahub_sdk/protocols_base.py (1)
Enum(111-112)
infrahub_sdk/node/node.py (3)
infrahub_sdk/schema/main.py (1)
RelationshipCardinality(17-19)infrahub_sdk/protocols_base.py (2)
initialized(27-27)RelatedNodeBase(13-40)infrahub_sdk/node/related_node.py (4)
initialized(107-108)get(216-226)get(263-273)RelatedNodeBase(17-179)
⏰ 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/graphql.py (1)
11-15: Correctly serializing None to GraphQL nullReturning "null" for Python None is correct and unblocks unsetting one-to-one relationships in mutations. Looks good.
tests/integration/test_infrahub_client.py (1)
173-178: Integration coverage for nullifying one-to-one relationshipGood addition verifying that setting favorite_animal = None removes the link on save and re-fetch.
infrahub_sdk/node/node.py (2)
237-240: Emit explicit null for optional one-to-one when uninitializedThis is the right place to surface None so the mutation payload can unset the relationship. Matches the PR objective.
328-330: Keep optional uninitialized one-to-one entries during stripThis ensures None survives stripping for optional RelatedNodeBase, enabling proper unsetting on update. Good change.
tests/unit/sdk/test_node.py (4)
1333-1338: Expect None for optional one-to-one on createUpdating expected input to include primary_tag: None aligns tests with new input generation. Good.
1585-1585: Include interface: None for IPHostMatches the new behavior for optional one-to-one relationships. Looks correct.
1599-1599: Include site: None for IPNetworkConsistent with emitting explicit nulls for optional one-to-one. Good.
1797-1797: Assert primary_tag: None when clearing relationshipThe expectations for empty-relationship updates now explicitly verify nulling the primary_tag. This is the right assertion for the new behavior.
Also applies to: 1806-1806
|
Might be good to fix it in stable to include it in 1.14.1 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
infrahub_sdk/node/node.py (1)
312-323: Tighten “original is none” detection and replace the snarky TODOConsider also treating {"id": None} as “original is none” and swap the tongue-in-cheek comment for a clear note.
Apply:
- # TODO: I do not feel _great_ about this - # -> I don't even know who you are (but this is not great indeed) -- gmazoyer (quoting Thanos) + # Treat original as "none" when it's None, when `node` is None, or when no `id` (or id is None) is present. original_data_item = original_data.get(item) original_data_item_is_none = original_data_item is None if isinstance(original_data_item, dict): if "node" in original_data_item: original_data_item_is_none = original_data_item["node"] is None - elif "id" not in original_data_item: + elif "id" not in original_data_item or original_data_item.get("id") is None: original_data_item_is_none = True(Agree with prior feedback: this “strip unmodified” block is hard to follow; factoring into small helpers would aid maintainability.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
changelog/479.fixed.md(1 hunks)infrahub_sdk/node/node.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- changelog/479.fixed.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/node/node.py
🧬 Code graph analysis (1)
infrahub_sdk/node/node.py (2)
infrahub_sdk/schema/main.py (1)
RelationshipCardinality(17-19)infrahub_sdk/node/related_node.py (4)
initialized(107-108)get(216-226)get(263-273)RelatedNodeBase(17-179)
⏰ 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). (5)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.12)
- GitHub Check: integration-tests-latest-infrahub
🔇 Additional comments (1)
infrahub_sdk/node/node.py (1)
331-333: All required attributes present on RelationshipManagerBase — resolvedQuick sanity check confirms that
RelationshipManagerBaseininfrahub_sdk/node/relationship.pydefines both members used by the pruning logic:
- In the constructor (around line 32), it sets
self.initialized: bool = False
and
self._has_update: bool = False.- Shortly afterward (around line 57), it exposes a
has_updateproperty:
@property def has_update(self) -> bool: return self._has_update.Since both
initializedandhas_updateexist onRelationshipManagerBase, there’s no risk of accidentalAttributeError.
gmazoyer
force-pushed
the
gma-20250827-ihs152
branch
from
86ffa79 to
4880a12
Compare
4 participants
This change fixes the use of
my_object.my_relationship = Noneby ensuring this is removing the link between an object and a related one tied together by an optional relationship of cardinality one.Summary by CodeRabbit