Rewrite of parse_alphanumeric_range and expand_alphanumeric_pattern

[pv2b]

Fixes: #13722

The code for expand_alphanumeric_pattern and parse_alphanumeric_range which is used to expand range expressions such as Gi1/0/[1-24] had got a bit unruly and at least one bug had snuck in where a case like vlan[123,456] could not be parsed correctly. (Issue #13722).

The goal here was to add a unit test for that broken behaviour and then fix it. Fixing it turned into a complete rewrite/refactor of these two functions, because they had reached a point where they were no longer maintainable as is.

This refactor surfaced two other unexpected behaviours:

1. When passing a pattern like r[a-9]a, which is invalid because it makes no sense to try to make a range from a letter to a number, the existing code returned an empty list. Surprisingly, the unit tests also seem to expect this very strange behaviour. Since the behaviour makes no sense, I didn't see any harm in changing the unit tests in order to reflect the new behaviour which is to throw an exception in thie case.

2. There are some warts as to how exception handling works. The old code ended up throwing exceptions implicitly when failing to do number conversion, etc. Instead, I have opted to do explicit checking and exception thowing in all cases I've thought of. However, this surfaced another wart. It seems that depending on the exact nature of the error, sometimes the unit tests expect a forms.ValidationError and sometimes a ValueError. In my opinion the correct exception type is ValueError, the functions are generic utility functions and should not be coupled to django forms in any way. However, in an effort to remain bug-for-bug compatible with existing unit tests, I have added some code to expand_alphanumeric_pattern to catch and rethrow the exceptions using the "correct" exception type, thereby containing the wart to one place in the code. Fixing it would have to involve changing the unit tests and auditing all calling code, and that didn't really seem to be in the scope of this issue so I've left it be. Instead I've just added some comments for any future housekeeping.

Anyway, original bug seems to be squashed, so... mission accomplished?

[pv2b]

Oh, one more thing, if anyone's tempted to clean up ALPHANUMERIC_EXPANSION_PATTERN to be a little more precise in what patterns it matches... well... you could, but then you'd break about half the unit tests, because of how the unit tests expect some wonky stuff to be passed through and some to have exceptions thrown. It was out of scope so I left it alone.

[pv2b]

I slept on it and came up on the idea of extracting the "warty" behaviour into its own function, and also ended up with a cleaner recursive implementation that's a lot easier to read than the itertools.product monstrosity I wrote yesterday. I also decided that expand_alphanumeric_pattern throwing forms.ValidationError was bogus and made it throw ValueError, and have the caller do that translation itself. This change in interface ended up requiring (justified) changes to the unit tests to expect a different exception type.

However, after this cleanup, I've discovered something else lurking under the surface that I think I'd need some clarity on, so I'm going to leave this PR non-ready for now until I can get a gauge on the correct behaviour. I'll come back and clarify what I mean by that, I'll extract that out as a seperate bug report.

[pv2b]

Actually, on second thought, I think the right context to put it is within this PR.

Anyway, the problem I've found (that was pre-existing before I even started working on this) is that the unit tests are testing compeltely dead code. Let me explain.

expand_alphanumeric_pattern is only ever called by one function like this:

netbox/netbox/utilities/forms/fields/expandable.py

Lines 32 to 33 in a8a36c0

	if re.search(ALPHANUMERIC_EXPANSION_PATTERN, value):
	return list(expand_alphanumeric_pattern(value))

As I have discovered in this refactoring, that means that, if there's nothing that looks like a range expression in the code, it'll just end up using the interface name with no changes.

However, that also means that some unit tests are flawed, such as these ones and others:

netbox/netbox/utilities/tests/test_forms.py

Lines 275 to 286 in a8a36c0

	def test_invalid_set(self):
	with self.assertRaises(ValueError):
	sorted(expand_alphanumeric_pattern('r[a]a'))
	with self.assertRaises(ValueError):
	sorted(expand_alphanumeric_pattern('r[a,]a'))
	with self.assertRaises(ValueError):
	sorted(expand_alphanumeric_pattern('r[,a]a'))
	with self.assertRaises(ValueError):
	sorted(expand_alphanumeric_pattern('r[a,,b]a'))

If we for example look at the test case of r[a]a, if you just read the unit tests you'll expect that to throw an exception and therefore cause an error to the user. But if you actually try this in NetBox, you'll find it'll happilly create an interface name r[a]a. That's because to_python won't even let expand_alphanumeric_pattern to ever run if it doesn't find anything looking like a pattern in it, and [a] doesn't look like a pattern. So it'll never even try expanding the ranges, leaving no opportunity for the exception to be thrown.

In effect, these tests (and others like it) end up testing dead code paths, and do not test the actual code path taken when the user puts in a value.

Now, in my opinion, the correct way to handle this is to remove the test in to_python so that testing expand_alphanumeric_pattern more accurately tests what the user actually will experience, but that leaves us at a crossroads, because doing that means doing one of two things.

Either we modify the tests to reflect the fact that a test case like r[a]a will just return back r[a]a because it doesn't recognise it as a pattern. I.e. no exception is thrown. This would reflect the current behaviour of NetBox pre-change.

Or, we consider that the case of entering something r[a]a ending up with the behaviour where NetBox just puts it in verbatim is wrong, and that NetBox shouldn't allow such input, and should instead throw an error. This would leave the tests unchanged, but would change the behaviour of NetBox.

In my opinion, neither behaviour is inherently more correct or wrong, so I think it makes sense for NetBox from an external standpoint to keep doing what it's always done, and to update the tests to test its actual behaviour instead of testing the dead code. So that's why I'm going ahead and changing these unit tests to reflect actual reality, even though the behaviour hasn't changed.

But there's a lot going on here, so I'm going to take a step back here and go back to the develop branch and re-factor the calling code and the unit tests seperately, and then merge them in here once I'm convinced the unit tests are valid both before and after.

[pv2b]

Actually, I'm just going ahead to make a new PR, this one is just way too verbose and difficult for anyone other than myself to follow. I'll make a seperate PR that's more organized and easier to review. Sorry for any confusion so far.