Second attempt at fixing #13722 and refactoring alphanumeric range code #13730

pv2b · 2023-09-09T13:00:53Z

Fixes: #13722

This pull request replaces my first train-wreck of an attempt in #13728.

The first two commits are preparatory refactoring work to improve the testability of current code.

First, I identified an issue where the tested behavious wasn't the actual behaviour. That's because expand_alphanumeric_pattern was only called from ExpandableNameField.to_python, which did its own regex matching on the input string before patting it over to expand_alphanumeric_pattern. It makes a lot more sense for that regex matching to only happen in expand_alphanumeric_pattern, so that was moved to that function, which, it turned out, was already doing it implicity by string splitting on the same regex.

Doing that change in turn made it possible to further simplify expand_alphanumeric_pattern by removing some unneccessary extra checks.

However, this caused some test failures. This is due to the fact that expand_alphanumeric_pattern's behaviour was changed. Before, it would throw an exception if there are no patterns in it, now it instead returns the string unmodified, which is exactly what the appication as a whole was doing, but now it's all contained in that function. That made it neccessary to adjust the unit tests to reflect this change of responsibility.

Once the prep-work was done, I replaced parse_alphanumeric_range with an almost entirely new implementation which I believe is cleaner and easier to read. This is what actually fixes the bug. It also adds some better error messages for the user when something goes wrong.

During the work in refactoring, I discovered another unrelated bug that also somehow made it into the test suite as a feature. For some reason, entering a string like r[a-9]a would make expand_alphanumeric_pattern return an empty list. This behaviour made no sense to me, so again, I changed the tests. This shouldn't have changed the external behaviour of the application, since when an empty list was received by to_python, the "name" field just ended up be empty, which in turn caused the form validation to fail with an incorrect message that the field was required. This way, there's a clearer error message as to what's going on.

There's still some improvements that could be done here, for example, I don't like how a pure utility function throws forms.ValidationError, but I'm not going to change it because it's not really causing any issues at the moment, and it's easy enough to change in the future if it ever becomes a concern.

The new behaviour of expand_alphanumeric_pattern is to return the input unmodified if there are no expansion patterns in it. Unit tests changed to reflect this new behaviour. This is fine because the only place that calls expand_alphanumeric_pattern was doing this check before anyway (and that place has been changed in this commit), this aligns the unit tests better with actual application behaviour, without changing external behaviour at all.

The regex search of the pattern is unneccessary. Instead, we rely on the new behaviour that terminates early if there are no patterns.

Input errors should throw exceptions, not return an empty list. Should not significantly change external behaviour, in the old versions a "Field is required" message was an end result of these cases.

github-actions · 2023-12-09T04:01:23Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

github-actions · 2024-01-08T04:01:36Z

This PR has been automatically closed due to lack of activity.

DanSheps · 2024-01-23T13:59:18Z

Re-opening at the request of Peter.

DanSheps

Honestly, I think you are trying to fix too much at once. Some of this would be better suited as a FR honestly.

We should stick to the bug report at hand and try to resolve that and only that first. Fundamentally, it is treating "," as a single range (568, 568) and that is failing a check on line 64 in utilities\forms\utils.py.

This should easily be fixable with a simple check to catch that.

DanSheps · 2024-01-23T14:06:11Z

netbox/utilities/tests/test_forms.py

Please comment each test with what it should contain and what it should output.

Example:

"""
Input expansion string: r[8-9]a
Resultant list: [r8a,r9a]
"""

I know this wasn't documented previously, but we should document it

In my opinion, it's unneccessary and distracting to write a comment that simply re-iterates the plain logic of the function in question. It also introduces repetition into the code which gives an opportunity for things to diverge.

Ideally a unit test should be simple enough to understand without comments, but it might make sense to add some comments to explain why a certain pattern is illegal.

That said, this isn't my call to make, and if your preferences are for such docstrings to be added to each tests, I'm happy to add them as per your example, if you confirm you want them in.

DanSheps · 2024-01-23T14:07:34Z

netbox/utilities/tests/test_forms.py

+        input = 'r[8--9]a'
+        output = sorted([input])
+        self.assertEqual(sorted(expand_alphanumeric_pattern(input)), output)


This is an invalid pattern and should still raise an exception

Please see my comment at d40a49e#r1463708607 for elaboration.

The current (pre-patch) Netbox does not raise an exception for r[8--9]a, this patch doesn't touch that behaviour.

DanSheps · 2024-01-23T14:12:25Z

netbox/utilities/tests/test_forms.py

+        input = 'r[-8]a'
+        output = sorted([input])
+        self.assertEqual(sorted(expand_alphanumeric_pattern(input)), output)


If you are planning on making changes, this should expand to r8a or raise a ValueError.

That said, we shouldn't need to make changes right now. We should focus on the single issue we are trying to fix.

Please see my comment at d40a49e#r1463708607 discussing a similar change.

DanSheps · 2024-01-23T14:12:42Z

netbox/utilities/tests/test_forms.py

+        input = 'r[8-]a'
+        output = sorted([input])
+        self.assertEqual(sorted(expand_alphanumeric_pattern(input)), output)


If you are planning on making changes, this should expand to r8a or raise a ValueError.

That said, we shouldn't need to make changes right now. We should focus on the single issue we are trying to fix.

This test doesn't reflect any change in external behaviour. Current Netbox already handles the input r[8-]a by just generating an interface named r[8-]a. The problem is that the current test is broken, and this fixes the test to reflect current reality about the behaviour before and after the change.

The problem is that current test is testing the expand_alphanumeric_pattern function directly, but this is subtly different to testing what happens if the users enters said string. That's because the old code that calls expand_alphanumeric_pattern in ExpandableNameField.to_python first checks if the string matches ALPHANUMERIC_EXPANSION_PATTERN, which the input string r[8-]a does not.

Therefore, if the user types in r[8-]a in the old code, it never even reaches expand_alphanumeric_pattern. But in the proposed code, it does.

So that leaves me with a choice:

Change the unit test to reflect the actual behaviour of the old code, thus preserving the current behaviour, considering the current behaviour to be a feature.

Change the code so that r[8-]a in fact does throw an error. This is a behaviour change or a bug fix whatever you want to call it.

In the interest of minimizing the amount of changes to behaviour, I decided to just keep the existing behaviour and make it into a feature. Although, if you will confirm to me that the intended behaviour is for r[8-]a to throw an error, I can of course patch the code and the test to reflect that.

Please let me know what you'd prefer.

Sorry for the spam here, but as a TL;DR summary:

The current unit tests don't reflect how Netbox actually works user-facing, because the entire logic is not under test. In the interest of "not doing too much" in one commit, I've preserved the existing "bugs" and updated the unit tests to reflect those bugs as features, because it's not my place to do otherwise.

DanSheps · 2024-01-23T14:52:28Z

netbox/utilities/forms/utils.py

+    try:
+        lead, pattern, remnant = re.split(ALPHANUMERIC_EXPANSION_PATTERN, string, maxsplit=1)
+    except ValueError:
+        yield string
+        return


ALPHANUMERIC_EXPANSION_PATTERN should be adjusted as needed to account for the cases below where it should either return a ValueError or a string.

Perhaps we may need to just check for the presence of an expansion string first (brackets) and if none, return a string, otherwise we can parse or return a ValueError

If I understand you correctly you're advocating changing ALPHANUMERIC_EXPANSION_PATTERN from the current r'\[((?:[a-zA-Z0-9]+[?:,-])+[a-zA-Z0-9]+)\]' to something like r'\[[^]*\]', which basically matches anything in brackets, even if it's never going to be parsable.

This would be possible as a simplification, but it would be a potentially breaking change, since it'll change how ranges are parsed, so I think it's best to leave this one alone.

DanSheps · 2024-01-23T14:53:28Z

netbox/utilities/tests/test_forms.py

+        input = 'r[a]a'
+        output = sorted([input])
+        self.assertEqual(sorted(expand_alphanumeric_pattern(input)), output)


If you are planning on making changes, this should expand to raa or raise a ValueError.

That said, we shouldn't need to make changes right now. We should focus on the single issue we are trying to fix.

Again, this is not a change in external behaviour. See: d40a49e#r1463708607

Current Netbox will create an interface literally named r[a]a if given that input, and I don't have a bug assigned to me to change that behaviour.

DanSheps · 2024-01-23T14:53:55Z

netbox/utilities/tests/test_forms.py

+        input = 'r[a,]a'
+        output = sorted([input])
+        self.assertEqual(sorted(expand_alphanumeric_pattern(input)), output)


If you are planning on making changes, this should expand to raa or raise a ValueError.

That said, we shouldn't need to make changes right now. We should focus on the single issue we are trying to fix.

Again, this is not a change in external behaviour, just an update to the unit test to reflect current reality.

See: d40a49e#r1463708607

DanSheps · 2024-01-23T14:54:07Z

netbox/utilities/tests/test_forms.py

+        input = 'r[,a]a'
+        output = sorted([input])
+        self.assertEqual(sorted(expand_alphanumeric_pattern(input)), output)


If you are planning on making changes, this should expand to raa or raise a ValueError.

That said, we shouldn't need to make changes right now. We should focus on the single issue we are trying to fix.

Again, this is not a change in external behaviour, just an update to the unit test to reflect current reality.

See: d40a49e#r1463708607

DanSheps · 2024-01-23T14:54:11Z

netbox/utilities/tests/test_forms.py

+        input = 'r[a,,b]a'
+        output = sorted([input])
+        self.assertEqual(sorted(expand_alphanumeric_pattern(input)), output)


If you are planning on making changes, this should expand to raa or raise a ValueError.

That said, we shouldn't need to make changes right now. We should focus on the single issue we are trying to fix.

Again, this is not a change in external behaviour, just an update to the unit test to reflect current reality.

See: d40a49e#r1463708607

pv2b · 2024-01-23T17:55:39Z

Honestly, I think you are trying to fix too much at once.

On the contrary, the changes to the unit tests are in an effort to not fix current bugs, but instead preserve current behaviour.

Fixing all the "bugs" would make the patch a lot cleaner, but it would also mean changing a lot of behaviour in one patch, which, ironically, is what you're trying to avoid :-)

DanSheps · 2024-01-23T18:31:28Z

Honestly, I think you are trying to fix too much at once.

On the contrary, the changes to the unit tests are in an effort to not fix current bugs, but instead preserve current behaviour.

Fixing all the "bugs" would make the patch a lot cleaner, but it would also mean changing a lot of behaviour in one patch, which, ironically, is what you're trying to avoid :-)

As I mentioned, which you seemingly ignored, all you need to "fix" this specific bug is:

if begin == end:
    values.append(begin)

after if begin.isdigit() and end.isdigit()

We could also simplify this to:

if begin == end:
   ...
elif begin.isdigit() and end.isdigit():
   ...
else:
   ...

and remove the begin==end logic from the else statement.

pv2b · 2024-01-23T18:59:49Z

Honestly, I think you are trying to fix too much at once.

On the contrary, the changes to the unit tests are in an effort to not fix current bugs, but instead preserve current behaviour.
Fixing all the "bugs" would make the patch a lot cleaner, but it would also mean changing a lot of behaviour in one patch, which, ironically, is what you're trying to avoid :-)

As I mentioned, which you seemingly ignored, all you need to "fix" this specific bug is:
if begin == end:
    values.append(begin)
after if begin.isdigit() and end.isdigit()

We could also simplify this to:
if begin == end:
   ...
elif begin.isdigit() and end.isdigit():
   ...
else:
   ...
and remove the begin==end logic from the else statement.

Okay, let's back up a bit here. There's no reason for us to get confrontational over this. I didn't intend to ignore anything. I probably just forgot to actually write that I agreed with it in my effort to explain what I believed was a misunderstanding in your review.

Anyway, a simpler fix probably would have sufficed, but at the time I wrote this code a few months ago, I took the opportunity to clean things up, and in doing so, I uncovered a few more bugs, and now here we are. I think my patch as written makes Netbox better, but if you want to reject it as too big of a change, I understand.

So, if you'd prefer to abandon this refactor and just introduce a short fix along the lines you outlined, that's fine. Let me know and I'll make a new branch like that, or if you'd rather do it yourself, that's fine with me too.

But if you'd for me to finish the PR in the state it is now, I'm happy to make sure it merges. If you want me to revert the unit tests to how they used to be and instead change the code's behaviour, so change how things like r[9-]a are handled to make them throw errors, to match the "intent" of the unit tests, I'll be happy to do that too. Ultimately there's no right and wrong here, just a decision to be made.

So just let me know how you want me to proceed (or not) and we'll take it from there.

jeremystretch · 2024-03-11T14:42:44Z

Thank you for your effort, however the root bug is quite simple and has been resolved in PR #15301.

pv2b added 7 commits September 9, 2023 14:16

Simplify expand_alphanumeric_pattern

834d34a

The regex search of the pattern is unneccessary. Instead, we rely on the new behaviour that terminates early if there are no patterns.

Add test for netbox-community#13722

a8c6f10

Rewrite parse_alphanumeric_range to fix netbox-community#13722

e485ed3

Fix bogus test_invalid_range_alphanumeric test

9f3c152

Input errors should throw exceptions, not return an empty list. Should not significantly change external behaviour, in the old versions a "Field is required" message was an end result of these cases.

Improve error message

8bd37d3

Removed extra newline

218ac24

pv2b marked this pull request as ready for review September 9, 2023 13:02

pv2b mentioned this pull request Sep 28, 2023

Bug in the parser part that declares Interface, etc. together. #13924

Closed

markh0338 mentioned this pull request Nov 30, 2023

Range "568" is invalid when creating interface with names vlan [568,570] #13722

Closed

github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Dec 9, 2023

github-actions bot closed this Jan 8, 2024

DanSheps reopened this Jan 23, 2024

DanSheps self-requested a review January 23, 2024 14:17

DanSheps requested changes Jan 23, 2024

View reviewed changes

DanSheps mentioned this pull request Feb 28, 2024

Fixes: #13722 - Correct range expansion code when a numeric set is used #15301

Merged

jeremystretch closed this Mar 11, 2024

jeremystretch removed the pending closure Requires immediate attention to avoid being closed for inactivity label Mar 11, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2024

Second attempt at fixing #13722 and refactoring alphanumeric range code #13730

Second attempt at fixing #13722 and refactoring alphanumeric range code #13730

Uh oh!

Conversation

pv2b commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes: #13722

Uh oh!

github-actions bot commented Dec 9, 2023

Uh oh!

github-actions bot commented Jan 8, 2024

Uh oh!

DanSheps commented Jan 23, 2024

Uh oh!

DanSheps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanSheps Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanSheps Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanSheps Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanSheps Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanSheps Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanSheps Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pv2b commented Jan 23, 2024

Uh oh!

DanSheps commented Jan 23, 2024

Uh oh!

pv2b commented Jan 23, 2024

Uh oh!

jeremystretch commented Mar 11, 2024

Uh oh!

Uh oh!

pv2b commented Sep 9, 2023 •

edited

Loading

DanSheps Jan 23, 2024 •

edited

Loading

DanSheps Jan 23, 2024 •

edited

Loading

DanSheps Jan 23, 2024 •

edited

Loading

DanSheps Jan 23, 2024 •

edited

Loading

DanSheps Jan 23, 2024 •

edited

Loading

DanSheps Jan 23, 2024 •

edited

Loading