Change Record.serialize() to handle tags in NetBox 2.9+ format

[markkuleinio]

With this PR Record.serialize() only tries to deduplicate tags (or tagged_vlans) if all the list members are either strings or integers, so NetBox 2.9+ style dicts will not be causing errors with OrderedDict.fromkeys() anymore. So this fixes #289.

It has to be noted that deduplicating dict-style tags (when dict is input by the user) is not straightforward due to various input formats allowed (id, name, slug). (Or the user can supply just the tag id.) My take is that the pynetbox user should do the deduplication checks herself if needed. And AFAIK NetBox does the deduplication anyway. Maybe that would call an extra note in the release notes that pynetbox does not enforce user-supplied tag deduplication anymore with new-style tags.

PoC with NetBox 2.9.11:

>>> netbox.extras.tags.get(name="Test1").id
5
>>> netbox.extras.tags.get(name="Test2").id
6
>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[]
>>> # Add tag by id
>>> d.tags.append(5)
>>> d.save()
True
>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[Test1]
>>> # Add tag by name in a dict
>>> d.tags.append({"name":"Test2"})
>>> d.save()
True
>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[Test1, Test2]
>>> # Remove the last item from the list
>>> d.tags.pop()
Test2
>>> d.tags
[Test1]
>>> d.save()
True
>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[Test1]
>>>

Note: I didn't specifically test this with NetBox 2.8 or older, but it should work as the tags list then contains all strings, so the "all-isinstance" statement returns True and ensures original behaviour.

[markkuleinio]

About adding a duplicate tag (with NetBox 2.9.11):

>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[Test1]
>>> d.tags[0].id
5
>>> d.tags.append(5)     # Adding duplicate tag by id
>>> d.save()
False
>>> ### So it failed to save it due to duplicate tag = no change in data
>>>
>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[Test1]
>>> d.tags.append({"name":"Test1"})   # Adding duplicate tag by dict
>>> d.save()
True
>>> ### = saves the duplicate tag if given as dict
>>>
>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[Test1]
>>>

Again, it is possible to add code in pynetbox so that it deduplicates the tag list regardless of the format given by the user, but I'm not sure if we want to do that. Do note that user can supply a new duplicate tag in various formats:

>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[Test1]
>>> d.tags.append(5)    # Test1
>>> d.tags.append({"name":"Test1"})   # The same Test1
>>> d.tags
[Test1, 5, {'name': 'Test1'}]
>>> d.save()
True
>>> d = netbox.dcim.devices.get(name="Test")
>>> d.tags
[Test1]
>>>

[markkuleinio]

About tag handling in general in pynetbox-based apps in NetBox 2.9+ world: Previously I was able to use simple "My Tag" in device.tags to check tag presence, but now I have to do something like this:

for tag in device.tags:
    if tag.name == "My Tag":
        # do my things
        break

or more pythonic

if "My Tag" in [tag.name for tag in device.tags]:
    # do my things

(note that the above does not work with unsaved user-supplied tags!)

Removing a tag with NetBox 2.8 or earlier:

device.tags.remove("My Tag")

and with NetBox 2.9+:

device.tags = [tag for tag in device.tags if tag.name != "My Tag"]

So handling tags in NetBox 2.9+ apps requires significant user mindset shift and application changes anyway.

[zachmoody]

for tag in device.tags:
    if tag.name == "My Tag":
        # do my things
        break

or more pythonic

if "My Tag" in [tag.name for tag in device.tags]:
    # do my things

and with NetBox 2.9+:

device.tags = [tag for tag in device.tags if tag.name != "My Tag"]

So handling tags in NetBox 2.9+ apps requires significant user mindset shift and application changes anyway.

😬 Figure this'll end up as boiler-plate in a lot of people's code. Take this with a grain of salt since I've never actually played with the 2.9+ behavior myself. But do client's really have to think about it differently? Could we implement an object with set-like behavior that provided an .add() (and .remove()) method that took a list of ints, strs, or dicts and handled them all, more-or-less, the same? Or is there some intrinsic behavior on the new tags I'm missing?

[markkuleinio]

But do client's really have to think about it differently? Could we implement an object with set-like behavior that provided an .add() (and .remove()) method that took a list of ints, strs, or dicts and handled them all, more-or-less, the same? Or is there some intrinsic behavior on the new tags I'm missing?

I'd say it's up to you if you want to expand the functionality of pynetbox by creating abstraction for tags. I thought you were out of time 😉 Seriously though, I guess it is doable. We have to agree for example how to deal with add() calls where creating tag is successful but saving the tagged object is not: should we then delete the just-created tag or leave it?

[markkuleinio]

And obviously the NetBox version handling needs to be present first and only after that we can add the version-independent tag handling code.

[markkuleinio]

After reading your message again, adding add_tag() and remove_tag() methods is one option. That way the existing tags handling code works as-is, but users can benefit from those convenience methods. If full deduplication is desired, then add_tag() needs to read the tags from NetBox to resolve the user-input tag IDs or dicts to actual tags.

[markkuleinio]

One more comment: adding a new tag in NetBox (2.9+) is non-trivial: slug must be given, and generating slug sensibly is surprisingly hard for us non-English language users. For example, if I create a tag with name "Leiniö" (my last name), can I have the slug as "leiniö" or will it cause problems for other users that want to manage tags? Or, if I create a mapping function that sets the slug as "leinio", then I run into risk that "Leinió" would have the same slug and that needs to be handled (same goes if the "strange" character is just removed, like NetBox GUI does). So, I would like to have slug decided by the application programmer, not by pynetbox, thus pynetbox should not create new tags in NetBox by itself (or at least when the programmer did not supply the slug).

I'd like other views for this as well.

Edit: to make it extra clear: in NetBox 2.9+ tags must be created separately (if not exist) before they can be assigned to objects.

[zachmoody]

One more comment: adding a new tag in NetBox (2.9+) is non-trivial: slug must be given, and generating slug sensibly is surprisingly hard for us non-English language users. For example, if I create a tag with name "Leiniö" (my last name), can I have the slug as "leiniö" or will it cause problems for other users that want to manage tags? Or, if I create a mapping function that sets the slug as "leinio", then I run into risk that "Leinió" would have the same slug and that needs to be handled (same goes if the "strange" character is just removed, like NetBox GUI does). So, I would like to have slug decided by the application programmer, not by pynetbox, thus pynetbox should not create new tags in NetBox by itself (or at least when the programmer did not supply the slug).

I'd like other views for this as well.

Oh, yeah that taps the brakes on that approach then. Generating a slug is definitely something I don't want to get mixed up in.
Was assuming hoping NetBox would do it for us.

Edit: to make it extra clear: in NetBox 2.9+ tags must be created separately (if not exist) before they can be assigned to objects.

Ah ok, didn't know that either.

[zachmoody]

Reading back over the earlier comments, I think I might've misrepresented the idea I was going for, in part, by explaining it as a bad solution 😆. Really what I was going for was that a list of strings vs objects with a name field may not be so different that we have to put the burden handling new types in client code. Also, I wasn't suggesting we create new tags if the user supplied one that didn't already exist. I think it's sensible to assume 2.9+ users know they have to create the tag (and its slug) first.

Question is, do we continue treating tags as a list of strings and fix it up as necessary when dealing with NetBox or do we represent them differently because the NetBox API changed 🤔? They both make pretty good claims to following the principle of least astonishment.

[markkuleinio]

What I was suggesting in the PR is that we would treat the tags data as-is, whatever it is, and also treat the updated (user-entered) data as-is. Then it's up to the user to modify tags as seems fit. In NetBox 2.8 or earlier the user must use strings. In NetBox 2.9+ the user can use either

• id (int) of the added tag, or
• a dict containing enough data to make the tag uniquely identifiable by NetBox to do the add operation. The dict can just contain name, id or slug field, or a combination of those. (Note that in future NetBox versions some new unique fields might be introduced.)

If you are looking for normalizing the tags list somehow in the NetBox 2.9+ dict input case then you would need to separately read the user-given tag(s) from NetBox in order to retrieve the full tag data and present it normalized. (I don't mean this would be unreasonable or wrong, just stating the situation. If you are after implementing some tag handling layer (add_tag() or some object or whatever) then that lookup would be expected to happen I guess.)

The current behaviour with this PR is that after save() the tags list stays as modified by the user, so it is not (necessarily) easily reusable in a for loop for example by the user because some list items may be full dicts (as retrieved earlier from NetBox), some ints, some maybe minimal dicts. That's what we have without the normalization. And the user obviously knows how she had changed the list so she can handle the case if needed.

Currently, as a pynetbox user, I have never assumed that whenever I have modified an object and saved it, I would have full object data available in the modified object (because it does not have the full resolved data at that point, I would need to retrieve the data again from NetBox to get the full resolved data, if I'm correct).

So, my answer to your question is that we don't need to "fix up" anything, with this PR it is up to the user to use tags as suitable with her NetBox version. If you want to add some tag handling layer, then more specifications are needed. (Edit: feel free to express your ideas or concerns further, I may have missed some aspect of course😀)

[zachmoody]

If you are looking for normalizing the tags list somehow in the NetBox 2.9+ dict input case then you would need to separately read the user-given tag(s) from NetBox in order to retrieve the full tag data and present it normalized. (I don't mean this would be unreasonable or wrong, just stating the situation. If you are after implementing some tag handling layer (add_tag() or some object or whatever) then that lookup would be expected to happen I guess.)

I'm floating the idea that we normalize to a list of strings in all cases. On reads, if we're dealing with <2.9, the response from the server is unmodified. if >=2.9, we pull the name field's value from the object and add it to the list of strings that ends up in .tags. Likewise, on writes, if a user wishes to add or remove a tag then they append or pop a string on the .tags list and we handle which format to send in .serialize().

The current behaviour with this PR is that after save() the tags list stays as modified by the user, so it is not (necessarily) easily reusable in a for loop for example by the user because some list items may be full dicts (as retrieved earlier from NetBox), some ints, some maybe minimal dicts. That's what we have without the normalization. And the user obviously knows how she had changed the list so she can handle the case if needed.

Currently, as a pynetbox user, I have never assumed that whenever I have modified an object and saved it, I would have full object data available in the modified object (because it does not have the full resolved data at that point, I would need to retrieve the data again from NetBox to get the full resolved data, if I'm correct).

Right, unless you called .full_details() after .save(). Which has always been the case for related fields. If anything, normalizing to a list of strings would help here since the client wouldn't need to worry about how they modified the tag list earlier in their code.

So, my answer to your question is that we don't need to "fix up" anything, with this PR it is up to the user to use tags as suitable with her NetBox version.

Kinda leaning that way too. Just wanted to throw another option out there that might be simpler, from the client's perspective.

If you want to add some tag handling layer, then more specifications are needed. (Edit: feel free to express your ideas or concerns further, I may have missed some aspect of course)

I'm not too keen on adding helper methods for tags, users can do that in their code if they want. More looking to nail down how we treat the field by default.

[zachmoody]

Will go ahead and merge this. If we decide normalization to a string is the way to go we'd have to cut do it on a major release anyways.

[markkuleinio]

If we decide normalization to a string is the way to go we'd have to cut do it on a major release anyways.

Feel free to open an issue (or a PR) if you want further discussion about that later. I guess it takes some time for people to settle with 2.9+ habits.