-
Notifications
You must be signed in to change notification settings - Fork 7
Fix ConversionFieldValue validator to accept explicit null attribute values #668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: stable
Are you sure you want to change the base?
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
…alue handling Co-authored-by: BeArchiTek <1334310+BeArchiTek@users.noreply.github.com>
Deploying infrahub-sdk-python with
|
| Latest commit: |
f63cd2b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://45ee4db0.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://copilot-fix-convert-object-t.infrahub-sdk-python.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## stable #668 +/- ##
==========================================
+ Coverage 75.48% 75.52% +0.03%
==========================================
Files 113 113
Lines 9512 9510 -2
Branches 1893 1892 -1
==========================================
+ Hits 7180 7182 +2
+ Misses 1832 1830 -2
+ Partials 500 498 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@BeArchiTek, does this fix the entire problem or just the assignment? I haven't looked too closely at the code within the SDK for this but I'm wondering what gets sent to the API. async def convert_object_type(
self,
node_id: str,
target_kind: str,
branch: str | None = None,
fields_mapping: dict[str, ConversionFieldInput] | None = None,
) -> InfrahubNode:
"""
Convert a given node to another kind on a given branch. `fields_mapping` keys are target fields names
and its values indicate how to fill in these fields. Any mandatory field not having an equivalent field
in the source kind should be specified in this mapping. See https://docs.infrahub.app/guides/object-convert-type
for more information.
"""
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()}I.e. if we'd always send this which seems incorrect: >>> value.model_dump(mode="json")
{'attribute_value': None, 'peer_id': None, 'peers_ids': None}Do we perhaps need to override the model_dump for that model to only send the field identified as |
|
@ogenstad so far this doesn't seems to fix the issue. |
Ok, thanks for checking. Do you think my suggestion would work? |
The
ConvertObjectTypemutation fails when the UI sendsattribute_value: nullin the fields mapping. The validator checked for non-None values instead of explicitly set fields, causing valid null attribute assignments to fail validation.Changes
ConversionFieldValue.check_only_one_fieldvalidator to usemodel_fields_setinstead of checking for non-None valuesFix
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
mock/home/REDACTED/.cache/pypoetry/virtualenvs/infrahub-sdk-aoa6suxx-py3.12/bin/pytest /home/REDACTED/.cache/pypoetry/virtualenvs/infrahub-sdk-aoa6suxx-py3.12/bin/pytest tests/unit/ -v --no-cov(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
This section details on the original issue you should resolve
<issue_title>bug: ConvertObjectType mutation fails with validation error when UI sends null attribute_value</issue_title>
<issue_description>### Component
Infrahub version
release-1.6
Current Behavior
The
ConvertObjectTypemutation fails with a Pydantic validation error when the UI sendsattribute_value: nullin the fields mapping. The validator inConversionFieldValuetreats explicitly setnullvalues as a field being set, causing it to fail the "exactly one field must be set" constraint.UI Error:
Server Stack Trace:
Expected Behavior
The mutation should handle
nullvalues gracefully, treating them as unset fields rather than explicitly set fields. The conversion should complete successfully.Steps to Reproduce
Full mutation example:
Variables:
{ "nodeId": "187c27f6-7eef-186a-3665-c51903599ca0", "targetKind": "LocationSite", "fieldsMapping": { "city": { "data": { "attribute_value": null } }, "name": { "source_field": "name" }, "address": { "data": { "attribute_value": null } }, "description": { "source_field": "description" }, "contact": { "data": { "attribute_value": null } }, "tags": { "use_default_value": true }, "parent": { "data": { "peer_id": { "id": "187c22c9-80c3-d620-3663-c510a9105307", "display_label": "Mexico", "__typename": "LocationCountry" } } }, "member_of_groups": { "source_field": "member_of_groups" } } }Additional Information
Root Cause:
The issue is in
python_sdk/infrahub_sdk/convert_object_type.pyin theConversionFieldValueclass. The Pydantic validator checks if fields are notNone, but when the UI sends{"attribute_value": null}, Pydantic sees this as the field being explicitly set (even though it'sNone), which fails the validation.Suggested Fix:
The validator should check which fields were explicitly provided in the input, not just which are non-None. This can be done using Pydantic's
model_fields_setattribute:Alternatively, the UI could be updated to not send fields with
nullvalues, ...✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.