Fix __str__ on Record to always return a string

[Frazew]

Summary

On certain edge cases, it is possible for the __str__ method of a Record object to return something that is not of type str, which raises an Exception: TypeError: __str__ returned non-string (type Record)

Steps to reproduce

import pynetbox
nb = pynetbox.api("http://localhost:8080", token="0123456789abcdef0123456789abcdef01234567")

# create test platform
platform = nb.dcim.platforms.create(
    name="test platform",
    slug="test-platform",
    napalm_args={
        "anything": {
            "name": {
                "attr1": "value1",
                "attr2": "value2",
            }
        },
    }
)

This is one of the ways this bug will be triggered. It can happen on any object can hold arbitrary JSON data in a field that is not explicitly mapped to a JsonField class in pynetbox (for example a config_context on dcim.devices will not trigger the bug)

Description

When a generic Record is generated from data returned by the API, each field is dynamically mapped deeply to find candidate Records with other well-known objects types. However, some netbox objects can hold arbitrary JSON, and if that JSON (dictionary) is not explicitly mapped to a JsonField, it will be interpreted as a Record object. Printing the Record will hence look for the keys that should represent it (name, label, display) and otherwise return an empty string. Given that these fields can be set to arbitrary values, the data can be of any type, which breaks the implicit assumption that it should be a str.

Fix

The PR simply add a call to str on the result that should be returned.

[Frazew]

You're absolutely right about the type check, I naively thought that __str__ would not accept returning str subclasses but it absolutely does, a isinstance check would've worked better.

Anyway your suggestion is a lot better while providing the same functionality, I updated the PR accordingly. I added the last or "" case: it might happen that getattr(self, "display", "") returns None, which would lead to returning the string "None" instead of an empty string as is currently the case.

Thank you!

[Frazew]

Updated with None since indeed it's the same thing. The PR is down to a super simple addition now.

---

Note: the initial reason for type-checking str before returning was for cases like this:

platform = nb.dcim.platforms.create(
    name="test platform",
    slug="test-platform",
    napalm_args={
        "name": {
            "display": ["test"]
        },
    }
)
print(platform.napalm_args)

which outputs the string "['test']".

This is also obviously true with other types (e.g. "name": 42 or "name": 4.3). I think it boils down to personal preference: returning a string representation for a number is not weird per se, but that means also returning a string representation for lists, which feels weird given that the expectation for str on a Record object is to have a user-friendly name. What do you think?

[abhi1693]

I think a better way would be to map the field with a type rather than forcing the function to always return string. This use case is very limited to the change at hand and I'd discourage it until it can be tested more thoroughly for all or most other use cases.

Moreover, if this is still an issue that requires to be solved, please open up an issue and discuss over there.