Skip to content

fix: stops structured values being stringified into component names - #544

Merged
semx merged 1 commit into
bb-Ricardo:developmentfrom
marcinpsk:fix/structured-values-in-names
Sep 7, 2026
Merged

semx merged 1 commit into
bb-Ricardo:developmentfrom
marcinpsk:fix/structured-values-in-names

Conversation

@marcinpsk

Copy link
Copy Markdown

get_string_or_none() is the shared parser for every name and label the check_redfish source builds. It str()-ed whatever it was given, so a value that is not a string was turned into its Python repr and concatenated into the name.

Dell iDRAC returns the location of a storage enclosure, storage controller or physical drive as a nested Oem object rather than a string:

    {'Oem': {'Dell': {'@odata.type': '#DellLocation.v1_2_0.DellLocation', ...}}}

The inventory item was then named

"BP_PSV 0:1 {'Oem': {'Dell': {'@odata.type': '#Del", 

NetBox storing the first 64 characters of it. The name is unreadable and the location it was meant to carry is lost.

get_string_or_none() now returns None for dict/list/set/tuple, so an unusable value is dropped instead of being pasted into the name. Scalars are untouched: the helper has many callers which pass ints (core counts, slot numbers, port counts) and rely on them stringifying.

added [tool.pytest.ini_options] to pyproject.toml, same as in #541 so it can drop when that merges. make_source() helper can move to conftest as well at that point.

get_string_or_none() is the shared parser for every name and label the check_redfish
source builds. It str()-ed whatever it was given, so a value that is not a string
was turned into its Python repr and concatenated into the name.

Dell iDRAC returns the `location` of a storage enclosure, storage controller or
physical drive as a nested Oem object rather than a string:

    {'Oem': {'Dell': {'@odata.type': '#DellLocation.v1_2_0.DellLocation', ...}}}

The inventory item was then named "BP_PSV 0:1 {'Oem': {'Dell': {'@odata.type': '#Del",
NetBox storing the first 64 characters of it. The name is unreadable and the location
it was meant to carry is lost.

get_string_or_none() now returns None for dict/list/set/tuple, so an unusable value is
dropped instead of being pasted into the name. Scalars are untouched: the helper has
many callers which pass ints (core counts, slot numbers, port counts) and rely on them
stringifying.

On the inventory item path this is a data quality problem rather than a churn problem:
the alphabetical fallback in update_all_items() still matches the item, so it is not
duplicated, the name is simply unusable.

Tests cover the helper contract and drive the real update_storage_enclosure(), including
a plain string location, which must still be kept, and two enclosures, which must not be
merged onto one name once the unusable location is dropped.
@semx
semx merged commit 61782a8 into bb-Ricardo:development Sep 7, 2026
@semx

semx commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Merged, thanks. Confirmed the get_string_or_none guard: every call site passes a scalar field, so returning None for a dict/list/set/tuple only changes the buggy case where a structured value was stringified into a name. Suite green.

@bb-Ricardo

Copy link
Copy Markdown
Owner

Do you have an example redfish inventory where a Dell server returns the location as an object/dict?

@semx

semx commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Traced it through both projects. In Redfish, Location on Drive/Storage/Chassis resources is an object, and the @odata.type in the report (#DellLocation.v1_2_0.DellLocation) is iDRAC's OEM extension of it. check_redfish passes that value through as-is (storage.py:223 drives, :415/:942 enclosures, :513 controllers), so what reaches the inventory depends on the check_redfish version:

  • up to v1.9.0 InventoryItem stored it untouched and to_json() wrote it as a nested object, so netbox-sync received a dict. That is the path this PR fixes: get_string_or_none() now drops it instead of pasting its repr into the name.
  • since v1.10.0 InventoryItem.__setattr__ coerces every str attribute with f"{value}", so the same object arrives as its Python repr inside a string and this guard does not apply; the name still ends up as BP_PSV 0:1 {'Oem': ....

So the guard is right for the older producer and harmless otherwise, but the durable fix is on the check_redfish side: map Location to a readable label (PartLocation.ServiceLabel, else something from Oem.Dell.DellLocation, else drop it) before assignment. I can open that PR over there. @marcinpsk a sanitized storage_enclosure entry from your inventory JSON plus your check_redfish version would settle which fields to use. If you'd rather keep the fix in one place, I'm fine reverting this one.

@marcinpsk

marcinpsk commented Sep 10, 2026

Copy link
Copy Markdown
Author

Do you have an example redfish inventory where a Dell server returns the location as an object/dict?

Hi @bb-Ricardo
I'll get you details on redfish repo because I think this should be there actually after looking at this more - with example.
It looks like @semx described - repr - and this fix does not cover that.

I would agree that it would be better suited not here most likely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants