SCCP rules sorting based on ID

[faizann]

Currently SCCP rules are sorted based on calledDigits and callingDigits patterns. This gets confusing to know which rule is checked first while rule matching. The GUI displays rules sorted based on ID. It is better to sort the rules internally for matching based on RuleID as well. This will make it cleaner. Also there is a bug in callingDigits rule sorting in case where there are 2 rules with same calledDigits but 1 of them has callingDigits and there are 1 or more rules with only calledDigits. The comparison between calledDigits rules can in some cases put the callingDigits rule below in the order and hence doesn't get matched. A fix is this but still it is better that we just simplify and use RuleIDs for sorting.

// if rule1 has a calling party and rule2 does not then put rule 1 above and do check vice versa
        if (o1.getPatternCallingAddress()!=null && o2.getPatternCallingAddress() == null)
            return -1;
        if ( o1.getPatternCallingAddress() == null && o2.getPatternCallingAddress() != null )
            return 1;

[vetss]

Hello @faizann

1) I see your point for manually sorting of rules (depending on ID or something like it). Now we have sorting depending on digits (liek more automated). We need some significant reason to change the behavior.

2) This is a current code. I have not found significant changing as compared with your version. Can you please comment.

        // Check if digits are exactly same. In that case we sort based on the callingDigits
        if ( digits1.equals( digits2 )) {
            // if rule1 has calling party and rule2 doesn't then we put rule1 first
            if ( o1.getPatternCallingAddress() != null && o2.getPatternCallingAddress() == null ) {
                return -1;
            } else if ( o1.getPatternCallingAddress() == null && o2.getPatternCallingAddress() != null ) {
                return 1;
            } else if ( o1.getPatternCallingAddress() != null && o2.getPatternCallingAddress() != null ) {
                // both have calling party addresses. lets compare these 2
                digits1 = o1.getPatternCallingAddress().getGlobalTitle().getDigits();
                digits2 = o2.getPatternCallingAddress().getGlobalTitle().getDigits();

                // Normalize rule. Remove all separator
                digits1 = digits1.replaceAll(SECTION_SEPARTOR, "");
                digits2 = digits2.replaceAll(SECTION_SEPARTOR, "");

                return compareDigits( digits1, digits2 );
            }
        }
        return compareDigits( digits1, digits2 );

[faizann]

Hi @vetss

The change is significant in functionality but is not a lot of code lines. I didn't delete the code but just commented out the older rules. You can see at this line I am only comparing IDs and returning that as the result of comparator. I also commented out all the tests. https://github.com/RestComm/jss7/pull/250/commits/8a400322893c510a0f6d11e4ba6fdb80032d21b0#diff-bdd35461f53c8db4e2c28c93103d8f16R72

[faizann]

My reasoning for change is that there is already a bug in my previous PR where we sort based on callingParty if calledParty addresses are same. That was a flawed logic as that still doesn't give proper sorting when comparing a rule which has callingParty but a different calledParty. Hence that randomness is highly dangerous and I realised that it gets complicated to keep track of sorting. ID based rules sorting gives a very clear view of what gets matched first and then has a natural fall through process.

[vetss]

Hello @faizann

The change is significant in functionality but is not a lot of code lines. I didn't delete the code but just commented out the older rules. You can see at this line I am only comparing IDs and returning that as the result of comparator. I also commented out all the tests.

I was asking for the case "Also there is a bug in callingDigits rule sorting in case where there are 2 rules with same calledDigits but 1 of them has callingDigits and there are 1 or more rules with only calledDigits.". I do not possibly still understand what is a bug in current implementation. If you see it please clarify more, better in soce update suggestion.

that randomness is highly dangerous and I realised that it gets complicated to keep track of sorting.

Please describe more what you see dangerous (may be examples). I am asking because just changing of behavior will affect current customers and there must be a big reason to do it. We need to care in this case of backward compatibility when old configs will be auto converted into a new style (possibly with proper ID changing) + manual update announsing.

[faizann]

Hi @vetss

The problem with current sorting algorithm with both calling/called party is this. Rule 1 calledParty 1234 Rule 2 calledParty 5678 Rule 3 calledParty 5678 callingParty 8888

The sorting goes like this -- Rule 3 is put above Rule 2 because Rule 3 has callingParty -- Rule 1 is put above Rule 3 as they both have same digit length and digits are different. -- Rule 2 is put above Rule 1 as they both have same digit length and digits are different. So, the order endsup being Rule 2, Rule 1, Rule 3 which will be incorrect as Rule 3 is never matched. This happened to me and I realised that these sorting rules are too complicated to remember while creating rules. A simple ID based rule ordering is more logical to understand.

Of course we cannot just move to RuleID based sorting. I think we can allow the type of ordering based on a specific flag in SCCP general settings where users can pick legacy or new RuleID based ordering.

I can submit that code if you agree. For me RuleID based sorting was very helpful and solved a lot of headaches.

[vetss]

Hello @faizann

thanks for a detailed problem description.

1) for a problem of sorting - can we add sorting depending on digits in compareDigits() ?

like replace:

    private int compareDigits( String digits1, String digits2 ) {
        // ............................
        return 1;
    }

with

    private int compareDigits( String digits1, String digits2 ) {
        // ............................
        return digits1.compareTo(digits2);
    }

?

2. Befor switching to "sorting based on ID" lets think of all options / problems. I see your point for it, but as I described switching has also disadvantages.

[vetss]

Hello @faizann

after internal discussion I think that it is better "to introduce a Factory to load Rule algorithms, by default it would be shipped with the current RuleAlgorithm and config but you could add a new RuleAlgorithm to be able to make a better version of rule ordering and matching thus keeping backward compatibility but allowing new customers to move to the new rule ordering, once it is stable we can declare the previous one deprecated and remove it 1 or 2 major versions later."

So we agree for a possibility of both algos implementation with old algo by default. @faizann please let me know if it is good for you. Can you please also comment my comment for brinning to a liveness of the old algo ("1)" in my previous message).

[faizann]

Hi @vetss

This is a very good idea indeed. I will work on a PR to get rule sorting based on your comments and test it. I will also submit later the rule algorithms being loaded by a factory.

Thanks