Add convert_object_type method #554
Conversation
WalkthroughAdds object-type conversion support: a new GraphQL mutation constant (CONVERT_OBJECT_MUTATION), Pydantic models (ConversionFieldValue, ConversionFieldInput) for field-mapping validation, and new client APIs on both async and sync InfrahubClient to invoke the mutation with node_id, target_kind, optional branch, and fields_mapping. Adds integration tests and test constants for async/sync clients, plus a changelog entry file documenting the addition. No existing public API signatures were changed except for the newly added methods and models. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Deploying infrahub-sdk-python with
|
| Latest commit: |
ff3f4aa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fc121783.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://lgu-convert-obj-type.infrahub-sdk-python.pages.dev |
| """ | ||
|
|
||
|
|
||
| class ConversionFieldValue(BaseModel): # Only one of these fields can be not None |
There was a problem hiding this comment.
I'll make a follow up PR infrahub side to use these classes from the SDK instead of having them duplicated
LucasG0
force-pushed
the
lgu-convert-obj-type
branch
from
aea909a to
a6fc80f
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #554 +/- ##
===========================================
+ Coverage 75.64% 75.67% +0.03%
===========================================
Files 100 101 +1
Lines 8919 8959 +40
Branches 1755 1765 +10
===========================================
+ Hits 6747 6780 +33
- Misses 1689 1692 +3
- Partials 483 487 +4
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrahub_sdk/client.py (1)
3020-3050: Sync method: mirror the async fixesSame two changes as above.
Apply this diff:
if fields_mapping is None: mapping_dict = {} else: - mapping_dict = {field_name: model.model_dump(mode="json") for field_name, model in fields_mapping.items()} + mapping_dict = { + field_name: model.model_dump(mode="json", exclude_none=True) + for field_name, model in fields_mapping.items() + } @@ - response = self.execute_graphql( + response = self.execute_graphql( query=CONVERT_OBJECT_MUTATION, variables={ "node_id": node_id, "fields_mapping": mapping_dict, "target_kind": target_kind, }, branch_name=branch_name, ) - return InfrahubNodeSync.from_graphql(client=self, branch=branch_name, data=response["ConvertObjectType"]) + result = response["ConvertObjectType"] + if not result.get("ok") or not result.get("node"): + raise Error("ConvertObjectType failed: ok is False or node is null") + return InfrahubNodeSync.from_graphql(client=self, branch=branch_name, data=result)
♻️ Duplicate comments (1)
infrahub_sdk/client.py (1)
1674-1686: API parity between async and sync is maintainedSignatures and behavior are aligned, matching our dual‑client principle.
Also applies to: 3021-3032
🧹 Nitpick comments (6)
changelog/+convert-object-type.added.md (1)
1-1: Prefer “node/kind” wording and mention both clientsAlign with SDK terminology and note async+sync availability.
Apply this diff:
-Add `convert_object_type` method to allow converting an object to another type. +Add `convert_object_type` method (async and sync) to convert a node to another kind.tests/constants.py (1)
1-3: Make constants clearer and immutable
- Consider using a tuple for CLIENT_TYPES.
- “standard” is vague; “async” is clearer if it won’t break consumers.
Apply this diff:
-CLIENT_TYPE_ASYNC = "standard" +CLIENT_TYPE_ASYNC = "async" CLIENT_TYPE_SYNC = "sync" -CLIENT_TYPES = [CLIENT_TYPE_ASYNC, CLIENT_TYPE_SYNC] +CLIENT_TYPES = (CLIENT_TYPE_ASYNC, CLIENT_TYPE_SYNC)infrahub_sdk/convert_object_type.py (3)
7-18: Name the GraphQL operationAdding an operation name improves tracing and tooling.
Apply this diff:
-CONVERT_OBJECT_MUTATION = """ - mutation($node_id: String!, $target_kind: String!, $fields_mapping: GenericScalar!) { +CONVERT_OBJECT_MUTATION = """ + mutation ConvertObjectType($node_id: String!, $target_kind: String!, $fields_mapping: GenericScalar!) { ConvertObjectType(data: { node_id: $node_id, target_kind: $target_kind, fields_mapping: $fields_mapping }) { ok node } } """
21-33: Inconsistent naming: peer_ids vs peers_idsDocstring uses peer_ids but the field is peers_ids. Keep the current wire key but accept both spellings and fix docs.
Apply this diff:
-from pydantic import BaseModel, model_validator +from pydantic import BaseModel, model_validator, Field, AliasChoices @@ - Use `peer_ids` to specify new peers of a cardinality many relationship. + Use `peers_ids` to specify new peers of a cardinality many relationship. @@ - peers_ids: list[str] | None = None + peers_ids: list[str] | None = Field( + default=None, + # Accept both 'peers_ids' (current) and 'peer_ids' (common spelling) on input + validation_alias=AliasChoices("peers_ids", "peer_ids"), + )
34-41: Optional: tighten validationConsider rejecting empty lists for peers_ids.
Apply this diff:
- fields = [self.attribute_value, self.peer_id, self.peers_ids] + fields = [self.attribute_value, self.peer_id, self.peers_ids] set_fields = [f for f in fields if f is not None] if len(set_fields) != 1: raise ValueError("Exactly one of attribute_value, peer_id, or peers_ids must be set") + if self.peers_ids is not None and len(self.peers_ids) == 0: + raise ValueError("peers_ids must contain at least one id") return selftests/integration/test_convert_object_type.py (1)
91-96: Use the sync client’s default branch in the sync pathMinor consistency/readability tweak.
Apply this diff:
- person_2 = client_sync.convert_object_type( + person_2 = client_sync.convert_object_type( node_id=person_1.id, target_kind="TestconvPerson2", - branch=client.default_branch, + branch=client_sync.default_branch, fields_mapping=fields_mapping, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
changelog/+convert-object-type.added.md(1 hunks)infrahub_sdk/client.py(3 hunks)infrahub_sdk/convert_object_type.py(1 hunks)tests/constants.py(1 hunks)tests/integration/test_convert_object_type.py(1 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_convert_object_type.pytests/constants.pyinfrahub_sdk/convert_object_type.pyinfrahub_sdk/client.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_convert_object_type.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_convert_object_type.pytests/constants.py
infrahub_sdk/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/client.py: Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Support authentication via API tokens or JWT with automatic refresh
Files:
infrahub_sdk/client.py
🧠 Learnings (1)
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
PR: opsmill/infrahub-sdk-python#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Maintain a dual client pattern: InfrahubClient (async) and InfrahubClientSync (sync) must provide identical interfaces
Applied to files:
infrahub_sdk/convert_object_type.py
🧬 Code graph analysis (2)
tests/integration/test_convert_object_type.py (3)
infrahub_sdk/client.py (2)
convert_object_type(1674-1703)convert_object_type(3020-3049)infrahub_sdk/convert_object_type.py (2)
ConversionFieldInput(43-60)ConversionFieldValue(21-40)infrahub_sdk/testing/docker.py (1)
TestInfrahubDockerClient(36-47)
infrahub_sdk/client.py (2)
infrahub_sdk/convert_object_type.py (1)
ConversionFieldInput(43-60)infrahub_sdk/node/node.py (4)
InfrahubNode(458-1086)from_graphql(486-500)from_graphql(1117-1131)InfrahubNodeSync(1089-1712)
🔇 Additional comments (1)
infrahub_sdk/client.py (1)
36-36: Import looks goodPublic API depends only on the input type; no circularity concerns.
|
|
||
| attribute_value: Any | None = None | ||
| peer_id: str | None = None | ||
| peers_ids: list[str] | None = None |
There was a problem hiding this comment.
the docstring says peer_ids, but the code says peers_ids
I think peer_ids is better, but, either way, they should match
There was a problem hiding this comment.
Going for peers_ids as it what the server expects
| fields_mapping = { | ||
| "name": ConversionFieldInput(source_field="name"), | ||
| "age": ConversionFieldInput(data=ConversionFieldValue(attribute_value=new_age)), | ||
| "worst_car": ConversionFieldInput(data=ConversionFieldValue(peer_id=car_1.id)), |
There was a problem hiding this comment.
is this in here to test that it is ignored?
There was a problem hiding this comment.
No, I updated schema to replace my_car by worst_car
|
|
||
| assert person_2.get_kind() == "TestconvPerson2" | ||
| assert person_2.name.value == person_1.name.value | ||
| assert person_2.age.value == new_age |
There was a problem hiding this comment.
maybe worth testing that fastest_cars is correctly updated too
2 participants
Summary by CodeRabbit