Fix object output for users in permissions

[markkuleinio]

The problem:

>>> permission = list(netbox.users.permissions.all())[0]
>>> from pprint import pprint as ppr
>>> ppr(dict(permission))
{'actions': ['view'],
 'constraints': None,
 'description': '',
 'display': 'ViewSites',
 'enabled': True,
 'groups': [],
 'id': 1,
 'name': 'ViewSites',
 'object_types': ['dcim.site'],
 'url': 'http://netbox-test.lein.io/api/users/permissions/1/',
 'users': [{'display': 'test1',
            'id': 2,
            'url': 'http://netbox-test.lein.io/api/users/users/2/',
            'username': 'test1'}]}
>>> permission.users
[]
>>>

Meaning that the username "test1" is shown as an empty string. That is because Record.__str__() only tries to get fields name and label, and returns empty string if neither of those is found.

This PR addresses this issue by modifying Record.__str__() to also try username field when returning the string representation of an object.

After the change:

>>> permission.users
[test1]
>>>

[zachmoody]

Is it possible to go for a custom model here instead of changing the default behavior?

[markkuleinio]

Oh, that sounds like a good idea, but I have currently no idea how that works 🤔😂

There is a Users(Record) class in models/users.py, how should we use it in this case?

[zachmoody]

Ah ok, it's already there and doing the right thing.

class Users(Record):
    def __str__(self):
        return self.username

Looks like we need a custom model for permissions letting it know that field is a list of User objects. It being a list might present some problems though, not sure we've had to do that before 🤔

[markkuleinio]

What's the meaning of this in Record._parse_values():

pynetbox/pynetbox/core/response.py

Lines 314 to 315 in 4efcab0

	if isinstance(v, dict):
	lookup = getattr(self.__class__, k, None)

Does it sometimes return something else than None? In my small test lookup was just None all the time.

Anyway, would a lookup table be enough for a solution?

from pynetbox.models.users import Users # and other relevant custom models
_field_model_lookup = {
    "users": Users,
}
...
if isinstance(v, dict):
    field_model = _field_model_lookup.get(k, Record)
# and the same in list_parser()

... except that is does not work due to circular imports: response.py would be importing users.py, which in turn imports response.py...

I guess the current model structure is something that is based on Django ideology so I don't know what's the right+clean solution off the bat. Collecting all the models in the same source files and rewriting the model lookup logic in app.py and endpoint.py? Or moving Record in a separate file and moving the _parse_values() logic in a standalone function in response.py? (Update: Not working because the circular dependency is still there)

I'll create an issue for this (created: #389) so that we can link also the other possible PRs to that.

[zachmoody]

Does it sometimes return something else than None? In my small test lookup was just None all the time.

Yeah, if memory serves, it shouldn't be none when a model has an attribute with an explicit reference to another model. E.g.

pynetbox/pynetbox/models/dcim.py

Lines 88 to 91 in ace1061

	device_type = DeviceTypes
	primary_ip = IpAddresses
	primary_ip4 = IpAddresses
	primary_ip6 = IpAddresses

Like if this wasn't a list, I think we could build out a Permissions class under Users and bring it in something like this:

class Permissions(Record):
    users = Users
...

Haven't had a chance to think about it too much, but I think you're on the right track in #390 with fixing up list_parser. It might be as simple as just doing a similar lookup there.

[markkuleinio]

@zachmoody Do you have have a good idea how to tackle this forward? New data models are being introduced and they have the same issue:

In [14]: list(netbox31beta.ipam.asns.all())
Out[14]: []

In [15]: for asn in netbox31beta.ipam.asns.all():
    ...:     pprint(dict(asn))
    ...:
{'asn': 64800,
 'created': '2021-11-21',
 'custom_fields': {},
 'description': '',
 'display': 'AS64800',
 'id': 1,
 'last_updated': '2021-11-21T11:36:46.344524+02:00',
 'rir': 1,
 'site_count': 0,
 'tags': [],
 'tenant': None,
 'url': 'http://netbox31beta.lein.io/api/ipam/asns/1/'}

In [28]: list(netbox31beta.users.tokens.all())
Out[28]: []

In [29]: for token in netbox31beta.users.tokens.all():
    ...:     pprint(dict(token))
    ...:
{'created': '2021-11-21T11:34:47.288607+02:00',
 'description': '',
 'display': '35cfe9 (admin)',
 'expires': None,
 'id': 1,
 'key': 'b4ca131411f66867401c30209a8ff139f335cfe9',
 'url': 'http://netbox31beta.lein.io/api/users/tokens/1/',
 'user': {'display': 'admin',
          'id': 1,
          'url': 'http://netbox31beta.lein.io/api/users/users/1/',
          'username': 'admin'},
 'write_enabled': True}

Not a major problem in my production applications though, only in the development stage when executing the commands interactively, still causes some raised eyebrowns 😃

How is it with the display field in the models, could that be used straight away instead of name (and model-specific fields)? (Oops that's only with NetBox 2.11+: netbox-community/netbox#5891 but could still be used in the getattr() 'chain')

[markkuleinio]

@zachmoody Please see PR #390, I've attempted to add support for custom model lists:

class Permissions(Record):
    users = [Users]

I will now modify this PR to use the display field as a last resort to support any future NetBox models that don't yet have a custom model in pynetbox.

[markkuleinio]

One more comment on this PR. test_users.py might need some improvements now, so maybe it's best to have #390 merged in first (if accepted), and I could then iron out the tests in this PR or in a new PR if better suited.

[zachmoody]

Hey @markkuleinio, apologies, haven't been able to keep up with the project as of late. Should have time to review all these next week though.

[markkuleinio]

Replaced by #419