Problem with change in Aliases list sorting (Redmine #14015)

jrey · Mar 17, 2023, 1:08 AM

Fix default sorting of Aliases in lists (Redmine #14015)

This patch seems to have an undesired result.

In that on the Alias list if I click the pencil to edit the list it shows the wrong list.
(ie the list is opened for editing is not the one shown in the name, value or description field for that row)
If I revert the patch the edit function works and loads the correct record as expected again.

I've installed tested and reverted a couple of times just to verify

Edit: browser inspector (looking at the links for the edit pencil)
the ?id= parameter on the link is different

without the patch installed, (I have 7 different alias entries in the IP part of the table (Firewall/Alias/IP) and 10 in total (Firewall/Alias/All)

under the IP section without the patch from top to bottom, no sort applied
3, 4, 5, 6, 7, 9, 8
with the patch applied the id's are
0, 1, 2, 3, 4, 5, 6
looks like the id's are getting assigned based on ALL the available Aliases, not the subset of IP

for the ALL section, without patch
3, 4, 5, 6, 7, 9, 8, 1, 2, 0
with patch
0, 1, 2, 3, 4, 5, 6, 7, 8 ,9

In each case the lists are displayed in the same order by name, and no additional sort has been applied to the table.

On a 2100 Device
23.01-RELEASE (arm64)
built on Fri Feb 10 20:06:48 UTC 2023
FreeBSD 14.0-CURRENT

SteveITS · Mar 17, 2023, 1:24 AM

@jrey I am not seeing a problem, unless I'm misunderstanding? No matter how I sort the list I edit the alias I'm trying to edit. It does seem to skip numbers if one creates aliases on other tabs but that may not be important if it's tracking them internally, correctly.

jrey · Mar 17, 2023, 1:59 AM

@steveits

I'm just loading the page (not applying any additional sort by clicking on a heading)

without the patch it is loading the correct record

🔒 Log in to view

so if I click the pencil on the last one if loads ?id=8
everything about this record is correct Name, Description and Values etc
The edit link is correct

Apply the patch and reload the page, only the last 2 in the table have switch location, but all the associated id's are wrong..
🔒 Log in to view

so now when I click on that same (uch) record, I get ?id=5
so it is loading the wrong record 5 <> 8

if modify the parameter in the browser changing the 5 to 8 and hit enter it will load the correct record that matches the name, values, description, and would be the record I expect it to load.

without the patch
If I use the first entry on the list the id=3 that Alias is called Buddy and that is id = 3

with the patch installed
Buddy (still at the top of the list) but now has a link id=0 and when I click on that it actually loads the record for pfB_PRI1_v4 (which is not even visible on the IP list)

again here if in the browser change the 0 to a 3 and reload I get the proper Buddy record.

The links generated with the patch applied do not line up to the existing records.

jimp · Mar 17, 2023, 2:01 AM

I suspect once you save those aliases and they're stored in the sorted order in the config it will straighten itself out. But there might be an issue as you say if they're not in the right order in the config. I have an idea how to fix it, I'll try to reproduce it in the morning.

jrey · Mar 17, 2023, 12:18 PM

@jimp

I suspected the issue was having the msort as a parameter to the foreach statement.

Correct assumption it seems.
I applied the patch again, confirmed it was assigning the wrong id values.
it was
then changed the code as follows to take the msort out of the foreach:

    msort($a_aliases, 'name');
    foreach ($a_aliases as $i => $alias):

Works as expected again, well at least is assigning the correct id and loading the correct record when editing

System PHP version: 8.1.11
looks like with this version having the msort as a parameter to the foreach is problematic.

Verified by changing it back, to patch original code, and reverting patch completely -

Edits correct records
without patch = yes (original asort)
with patch = no (msort as parameter to foreach)
with patch applied but modified as above = yes (msort before foreach)

Cheers
JR

Edit: just as a footnote to this - you can delete the msort line completely and the table still loads in the same order (not really sorted by name) The displayed table does match the order if the records in the config. So the msort is not actually changing the initial order of the table that is displayed. The id= are still correct after removing the msort. Then clicking on the headers "Name" for example sort Asc or Desc. The ids remain the same.

jimp · Mar 17, 2023, 12:18 PM

@jrey said in System Patches package version 2.2:

@jimp
I suspected the issue was having the msort as a parameter to the foreach statement.

Yes but the msort is the point of that patch so the list shows sorted. If you view the contents of the patch it's just a short diff changing the old sort method to msort. On my systems the aliases are all aligned correctly when I check them, though. They should be getting sorted any time you make a change to the aliases, it's not clear how yours are out of order. But once I figure out how to reproduce it locally I have ideas on how to fix it and maintain the sorting.

jimp · Mar 17, 2023, 12:32 PM

@jrey said in System Patches package version 2.2:

Edit: just as a footnote to this - you can delete the msort line completely and the table still loads in the same order (not really sorted by name) The displayed table does match the order if the records in the config. So the msort is not actually changing the initial order of the table that is displayed. The id= are still correct after removing the msort. Then clicking on the headers "Name" for example sort Asc or Desc. The ids remain the same.

msort returns a sorted array, it doesn't modify the array, hence it being used in the foreach statement and not before. But that's getting way off the topic here, I may split this part of the discussion off into a separate thread.

jimp · Mar 17, 2023, 12:34 PM

OK I split this off and moved it to the 23.05 category since that change was put in after 23.01.

jrey · Mar 17, 2023, 1:38 PM

@jimp

Sorry I added a footnote to my previous post. Apparently we were both adding comments at the same time and you moved the thread.

so that said: with or without the msort the initial display of the table always matches the order of the records in the config.
I can then click on any of the headers, and the sort changes, with the id's remaining correct as the records move change on the display

So if your expectation is that the msort is changing the initial data order of the data displayed, that is not happening here.
with the msort as part of the foreach construct the ids are wrong (and the list is not in name order originally anyway)
with the msort above the foreach the ids are correct but the list still isn't in 'name' order
remove the msort line completely and the ids are also correct.

Not the end of the world, the list (in my case is small enough) and I can sort it by clicking on the headers if I want and that works. So as long as the id's are correct in the first place it will open the correct record. It is all good.

Let me know if there is something specific you want me to try.

now I want to go back to the original asort, as I can't remember the order of the initial list on load, I believe it was order of the config as well. Thinking about it, the records in the config are in the same order as originally created. The order in the config has never changed.

Thanks

jimp · Mar 17, 2023, 12:50 PM

And on each of mine the IDs always match the entries no matter what I've done so far.

I just started a fresh system and made three aliases: aaaa, cccc, then bbbb.

The order in the list is aaaa, bbbb, cccc, and the IDs to edit are 0, 2, 1.

So it's still not clear to me why yours don't match when any test I'm running comes out OK.

jrey · Mar 17, 2023, 1:06 PM

@jimp

Both the asort and msort give no indication that the list has been sorted.

and clearly in my case msort not giving the results as expected anyway, the initial load of the list is not sorted.

Might it be better to just load the list, (and the ids are correct) then generate a event required to order the list by name (as if clicking on the header) ??

on initial load the table looks like this (in all cases, asort, msort)

🔒 Log in to view

After clicking on the header (the appropriate header is changed as)

🔒 Log in to view

I could see if there are people with much larger lists that sorting would be important. But then at the same time if (for example) I've sorted that long list on Description, then made a change and the screen refreshes and the default sort goes back to name, that might be annoying too.

In my case not worth the effort to care as long as the ids match what is being loaded, I don't care about the sort order, I can seem them all right there.

So I think for right now, I am going to just revert the existing patch back to the original asort, so as not mess something up if a revised patch "fixes" it at some point down the road.

Thanks

jrey · Mar 17, 2023, 1:38 PM

@jimp

@jrey said in Problem with change in Aliases list sorting (Redmine #14015):

now I want to go back to the original asort, as I can't remember the order of the initial list on load,

so I've gone back to the original (asort code)
and the ids still match what I had originally posted as the order of id's 3, 4, 5, 6, 7, 9, 8, 1, 2, 0 and everything work
->> and now that I'm looking at the complete list of 10, they are in alpha order by name (which is why the id's are out of order on the display).

So....in short for me, if it ain't broke don't fix it ... no patch.

Again let me know if there is something else you want me to specifically look at.

JR

jimp · Mar 17, 2023, 2:34 PM

If you think it's correct you must not have any mixed case alias named (some start with uppercase, some lowercase). asort is case sensitive and will put all uppercase before all lowercase instead of putting them in actual alphabetical order.

asort:

🔒 Log in to view

msort:

🔒 Log in to view

Unfortunately, asort doesn't have a flag to do a case-insensitive sort (that actually works for this -- note there is SORT_FLAG_CASE but it does work as expected on its own or when used with the natural or string sorts).

jimp · Mar 17, 2023, 3:09 PM

This seems to do the trick:

uasort($aliases, function ($a, $b) { return strnatcasecmp($a['name'], $b['name']); });

🔒 Log in to view

jimp · Mar 17, 2023, 4:09 PM

I pushed a fix for #14015 (again) on 23.05 that fixes it there in a better way, but I also added a new patch for the system patches package on 23.01 that is a minimalist application of the sorting change alone.

I had to do it that way because the code for aliases in 23.05 diverged too much after 23.01 to apply the 23.05 fix directly.

Updating the system patches package to 2.2.1 when it's ready will also get the fix, you can just apply the new patch on top of the old patch and it will do the right thing.

jrey · Mar 17, 2023, 5:03 PM

@jimp

Okay so I reapplied the first patch to setup for "P2" version and installed that.

All good,

Much better now that editing loads the record you expect, and bonus (I guess) they are in order.

Thanks for looking into this so promptly,

(the only rules with lower case letters at the start are pfB aliases, and 1 alias that I had created for testing myTest

JR

SteveITS · Mar 20, 2023, 10:19 PM

@steveits said in Problem with change in Aliases list sorting (Redmine #14015):

I am not seeing a problem

Just for the record I did eventually notice this on a (different) router, in our data center. Fixed by the patch part 2.

jrey · Mar 20, 2023, 10:32 PM

@steveits said in Problem with change in Aliases list sorting (Redmine #14015):

Just for the record I did eventually notice this on a (different) router, in our data center. Fixed by the patch part 2.

See I'm only half crazy - lol - which I think with your confirmation now makes you the other half.
Thanks for the confirmation - good to know I'm not the only one.

and I for one am really glad it is "sorted" out.