Interface.objects.order_naturally() enhancements

[tarkatronic]

Issue type

[X] Feature request
[X] Bug report
[ ] Documentation

Environment

• Python version:
• NetBox version:

Description

I have discovered a few issues/limitations in the order_naturally() method which I have had to address in a local project, and would be happy to submit a PR for if desired.

The first is a bug in the actual ordering. Given interfaces with the following names:

• Ethernet1/3/1
• Ethernet1/4
• Ethernet1/5/1

I would expect order_naturally() to return them in that same order. However, it is instead returning them in the order:

• Ethernet1/3/1
• Ethernet1/5/1
• Ethernet1/4

This is a side effect of the regular expressions used in the method being anchored to the end of the string.

Second, I have added support for a third slash in the name (Ethernet1/2/3/4).

And finally, because it is highly recommended you do not use the .extra() method, I have updated the method to use .annotate() and RawSQL() instead.

Again, I can have a pull request ready for this ASAP if desired, along with appropriate tests.

[jeremystretch]

@tarkatronic great work, thanks for the contribution! Unfortunately it still needs some work. With this approach, lo0 will appear between xe-0/9/9 and xe-1/0/0. I'm fiddling with it now. I merged the PR anyway because we definitely want the conversion from extra() to annotate() and the subposition field.

[jeremystretch]

Ok, I think I figured out the trick. The solution, as always, is to add more regex. Previously, we were treating the zero in e.g. lo0 as its "slot," which isn't quite appropriate for the reason in my previous comment. What we can do is catch the zero as a separate ID with the regex ^(?:[^0-9]+)([0-9]+)$, and tweak the slot regex to match on a trailing slash. Then, each interface will have either a slot or an ID, and we simply sort IDs after slots (because IMO it's more common to have lo0/fxp0-type interfaces show up after the physical interfaces).

I seem to have this working perfectly locally. I just need to clean it up a bit and extend your test.

[jeremystretch]

I'm happy with how this is working right now. This issue can be re-opened if any notices a particular problem with the new logic (please be sure to include instructions detailing the steps needed to reproduce).

[tarkatronic]

@jeremystretch
So I'm doing some more looking and testing of this updated logic, and I'm not sure it's quite right.

Now, this data is purely theoretical; I'm not sure this could happen in a real scenario, but let's just suppose it could. You have the following:

Ethernet1/1
Ethernet1
Ethernet1/3
Ethernet1/4
Ethernet1/1/1
Ethernet1/2

Now, my expected/preferred ordering for this would be

Ethernet1
Ethernet1/1
Ethernet1/1/1
Ethernet1/2
Ethernet1/3
Ethernet1/4

However currently I am getting

Ethernet1/1/1
Ethernet1/1
Ethernet1/2
Ethernet1/3
Ethernet1/4
Ethernet1

This is due to a combination of two things. The first is the removal of the COALESCE calls, which turns the unmatched fields into NULL, which PostgreSQL will, by default, sort to the bottom of the stack.

The second part is due to the addition of the \/ in the SLOT_RE. For interfaces with no subslot, that match is failing.

So I've arrived at the following:

        type_re = r"SUBSTRING({} FROM '^([^0-9]+)')"
        id_re = r"CAST(SUBSTRING({} FROM '^(?:[^0-9]+)([0-9]+)$') AS integer)"
        slot_re = r"CAST(SUBSTRING({} FROM '^(?:[^0-9]+)([0-9]+)\/?') AS integer)"
        subslot_re = r"COALESCE(CAST(SUBSTRING({} FROM '^(?:[^0-9]+)(?:[0-9]+\/)([0-9]+)') AS integer), 0)"
        position_re = r"COALESCE(CAST(SUBSTRING({} FROM '^(?:[^0-9]+)(?:[0-9]+\/){{2}}([0-9]+)') AS integer), 0)"
        subposition_re = r"COALESCE(CAST(SUBSTRING({} FROM '^(?:[^0-9]+)(?:[0-9]+\/){{3}}([0-9]+)') AS integer), 0)"
        channel_re = r"COALESCE(CAST(SUBSTRING({} FROM ':([0-9]+)(\.[0-9]+)?$') AS integer), 0)"
        vc_re = r"COALESCE(CAST(SUBSTRING({} FROM '\.([0-9]+)$') AS integer), 0)"

Thoughts?

[jeremystretch]

I've made some tweaks, and interfaces are now ordered as:

Eth1/1
Eth1/1/1
Eth1/1/1/1
Eth1/1/2
Eth1/2
Eth1

I want to keep interfaces without a slash in their name at the bottom of the list, because a device typically has either slotted or non-slotted physical interfaces. In the cases where both exist, the non-slotted interfaces tend to be "special" interfaces like em0. This also avoids problems like mixing physical and virtual interfaces in the list.

These changes will appear in v2.2. I'm going to close out this issue for now, but as is tradition it can be re-opened if further adjustments are needed.

[tarkatronic]

Hey @jeremystretch, interested in another small enhancement? In my local project, I've created a SubstrFrom db function (which I'm also attempting to get added to Django core). With that, I was able to eliminate RawSQL entirely! So I now have this method working with straight up ORM.

        fields = {
            '_type': SubstrFrom(field, self.TYPE_RE),
            '_id': Cast(SubstrFrom(field, self.ID_RE), IntegerField()),
            '_slot': Cast(SubstrFrom(field, self.SLOT_RE), IntegerField()),
            '_subslot': Coalesce(Cast(SubstrFrom(field, self.SUBSLOT_RE), IntegerField()), Value(0)),
            '_position': Coalesce(Cast(SubstrFrom(field, self.POSITION_RE), IntegerField()), Value(0)),
            '_subposition': Coalesce(Cast(SubstrFrom(field, self.SUBPOSITION_RE), IntegerField()), Value(0)),
            '_channel': Coalesce(Cast(SubstrFrom(field, self.CHANNEL_RE), IntegerField()), Value(0)),
            '_vc': Coalesce(Cast(SubstrFrom(field, self.VC_RE), IntegerField()), Value(0)),
        }