Roundup Tracker - Issues

Issue 2551207

classification
Empty order attribute produces traceback in HTMLDatabase.list
Type: crash Severity: normal
Components: Web interface Versions: devel
process
Status: closed fixed
:
: schlatterbeck : rouilj, schlatterbeck
Priority: normal : python3

Created on 2022-05-30 15:57 by schlatterbeck, last changed 2022-05-31 21:32 by rouilj.

Messages
msg7538 Author: [hidden] (schlatterbeck) Date: 2022-05-30 15:57
When moving an existing tracker to Python3 I'm getting the traceback below from a HTML form.
Turns out the HTMLDatabase.list function tries to sort items (ids) of a class.
If the id is numeric *and* the class has an order attribute that allows NULL values (happens when the class has a explicit order attribute) and we encounter such NULL values (None in python) we compare a string to None which is disallowed in Python3. So the solution is to check for None in the sort-key function.

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/client.py", line 1927, in renderContext
    result = pt.render(self, None, None, **args)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/engine_zopetal.py", line 94, in render
    TALInterpreter.TALInterpreter(self._v_program, self.macros,
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 192, in __call__
    self.interpret(self.program)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 236, in interpret
    handlers[opcode](self, args)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 680, in do_useMacro
    self.interpret(macro)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 236, in interpret
    handlers[opcode](self, args)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 413, in do_optTag_tal
    self.do_optTag(stuff)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 398, in do_optTag
    return self.no_tag(start, program)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 392, in no_tag
    self.interpret(program)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 236, in interpret
    handlers[opcode](self, args)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 644, in do_condition
    self.interpret(block)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 236, in interpret
    handlers[opcode](self, args)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 413, in do_optTag_tal
    self.do_optTag(stuff)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 398, in do_optTag
    return self.no_tag(start, program)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 392, in no_tag
    self.interpret(program)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 236, in interpret
    handlers[opcode](self, args)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 705, in do_defineSlot
    self.interpret(slot)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 236, in interpret
    handlers[opcode](self, args)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/TAL/TALInterpreter.py", line 467, in do_setLocal_tal
    self.engine.setLocal(name, self.engine.evaluateValue(expr))
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/PageTemplates/TALES.py", line 225, in evaluate
    return expression(self)
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/PageTemplates/PythonExpr.py", line 91, in __call__
    return f()
  File "<string>", line 2, in f
  File "/usr/local/lib/python3.9/dist-packages/roundup/cgi/templating.py", line 791, in list
    l.sort(key=keyfunc)
TypeError: '<' not supported between instances of 'NoneType' and 'str'
msg7539 Author: [hidden] (schlatterbeck) Date: 2022-05-30 16:05
Added fix and a test for the HTMLDatabase.list method.

Fixed in 994893cf3e1a
msg7540 Author: [hidden] (rouilj) Date: 2022-05-30 16:21
I think there was a similar issue before, iirc there
was a mechanism to sort null first or last in the
sequence. that might help find the earlier ticket
and fix.
msg7541 Author: [hidden] (schlatterbeck) Date: 2022-05-30 21:01
On Mon, May 30, 2022 at 04:21:24PM +0000, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> I think there was a similar issue before, iirc there
> was a mechanism to sort null first or last in the
> sequence. that might help find the earlier ticket
> and fix.

Wasn't that in the database code when retrieving parameters of a query?
The code that I've just fixed seems to be used only in the web-interface
presumably when displaying menus or something like it. It's still not
clear to me why the code doesn't retrieve things sorted via the database
interface and does the sorting in python.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com
msg7542 Author: [hidden] (rouilj) Date: 2022-05-30 21:27
Ralf, is this a case I missed in issue2551120?
msg7544 Author: [hidden] (rouilj) Date: 2022-05-30 21:35
Also this is documented in customizing.txt:

   Note that if an "order" property is defined on a Class that is used for
   sorting, all items of that Class *must* have a value against the "order"
   property, or sorting will result in random ordering.

Is this still valid for items without a valid order property?
msg7545 Author: [hidden] (rouilj) Date: 2022-05-31 02:07
> The code that I've just fixed seems to be used only in the web-interface
> presumably when displaying menus or something like it. It's still not
> clear to me why the code doesn't retrieve things sorted via the database
> interface and does the sorting in python.

The change I did was for the sorted() method in templating for a MultilinkHTMLProperty.

It appears that this level of sorting is done in the hyperdb (see sort_repr methods).

My guess is it's a holdover from the anydbm key/value store not having a
native sort method. Also since id's are stored as strings, but sorted as integers, we
can't use rdbms native sorting on that field.
msg7546 Author: [hidden] (schlatterbeck) Date: 2022-05-31 07:17
On Mon, May 30, 2022 at 09:35:15PM +0000, John Rouillard wrote:
> 
> Also this is documented in customizing.txt:
> 
>    Note that if an "order" property is defined on a Class that is used for
>    sorting, all items of that Class *must* have a value against the "order"
>    property, or sorting will result in random ordering.
> 
> Is this still valid for items without a valid order property?

Hmm, I don't think this has ever been valid but looking through the
tests I'm not so sure. We *do* test for Link values being NULL on
sorting and generate complicated SQL for postgresql to match the default
sort order of NULL values of mysql (it sorts NULL values *last*).
But I'm not sure we do the same for non-Link properties. So maybe we
create an issue on this to investigate. It may well be that the
different SQL backends behave differently:

The python2 postgresql backend sorts NULL values in order attributes
*first* (in the index view of a class) and python3 behaviour hasn't
changed. So it might well be that postgres sorts NULL first while mysql
sorts NULL last and the above statement comes from that observation.  I
still find "NULL first" the more intuitive behaviour.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com
msg7547 Author: [hidden] (schlatterbeck) Date: 2022-05-31 07:24
On Mon, May 30, 2022 at 09:27:02PM +0000, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> Ralf, is this a case I missed in issue2551120?

The Traceback is the same but the code-path is different. The relevant
code is in the 'list' method of HTMLClass in cgi/templating.py:

    def list(self, sort_on=None):
        """ List all items in this class.
        """
        # get the list and sort it nicely
        l = self._klass.list()
        keyfunc = make_key_function(self._db, self._classname, sort_on)
        l.sort(key=keyfunc)

The make_key_function:

def make_key_function(db, classname, sort_on=None):
[...]
    linkcl = db.getclass(classname)
    if sort_on is None:
        sort_on = linkcl.orderprop()
    def keyfunc(a):
        if num_re.match(a):
            a = linkcl.get(a, sort_on)
            # In Python3 we may not compare strings and None
            if a is None :
                return ''
        return a

The 'if a is None' part is new. [Oops the spacing is wrong.]

What it does is look up the order attribute (or whatever was given as a
sort_on parameter) of the numeric ID (the parameter 'a').

So this is a different code-path with the same bug (one could argue that
the bug is in python3 refusing to compare different types).

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com
msg7548 Author: [hidden] (schlatterbeck) Date: 2022-05-31 07:29
On Tue, May 31, 2022 at 02:07:20AM +0000, John Rouillard wrote:
> My guess is it's a holdover from the anydbm key/value store not having
> a native sort method. Also since id's are stored as strings, but
> sorted as integers, we can't use rdbms native sorting on that field.

Umm, I think the original code used IDs as strings. But these days at
least all the sql backends use a numeric field for the id.

And I think that the anydbm code also can sort IDs numerically these
days?!

So I think we *can* use the native order for sorting. See
testFilteringSortId in test/db_test_base.py

But as outlined in another message in this discussion the code I changed
does *not* compare IDs, instead it retrieves the order property (or if
explicitly given, the 'sort_on' parameter) and sorts on that.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com
msg7549 Author: [hidden] (rouilj) Date: 2022-05-31 21:32
Hi Ralf:

In message <20220531071724.njnskvgjs4l3ev5b@runtux.com>,
Ralf Schlatterbeck writes:
>On Mon, May 30, 2022 at 09:35:15PM +0000, John Rouillard wrote:
>> 
>> Also this is documented in customizing.txt:
>> 
>>    Note that if an "order" property is defined on a Class that is used for
>>    sorting, all items of that Class *must* have a value against the "order"
>>    property, or sorting will result in random ordering.
>> 
>> Is this still valid for items without a valid order property?
>
>Hmm, I don't think this has ever been valid but looking through the
>tests I'm not so sure. We *do* test for Link values being NULL on
>sorting and generate complicated SQL for postgresql to match the default
>sort order of NULL values of mysql (it sorts NULL values *last*).
>But I'm not sure we do the same for non-Link properties. So maybe we
>create an issue on this to investigate. It may well be that the
>different SQL backends behave differently:
>
>The python2 postgresql backend sorts NULL values in order attributes
>*first* (in the index view of a class) and python3 behaviour hasn't
>changed. So it might well be that postgres sorts NULL first while mysql
>sorts NULL last and the above statement comes from that observation.  I
>still find "NULL first" the more intuitive behaviour.

Different back ends sorting NULL values in different order can be
considered "random" ordering of the items without an explicit value.
If I have items A and B with a NULL order prop, does A sort before B
or B sort before A? It may be better to replace "random" with
"undefined" but I think the statement still stands.  If you think we
should open a ticket to investigate further I'll defer to you.

Interesting about ID's being integers in rdbms backends. I didn't
realize that but sqlite shows it to be true.  So we could use native
sorting. Are orderprops constrained to be integers? I assume not since
orderprop will fall back to labelprop which is a string not an int.

In your code the sort order could be on any field so we still need to
handle NULL/None correctly.
History
Date User Action Args
2022-05-31 21:32:38rouiljsetmessages: + msg7549
2022-05-31 07:29:58schlatterbecksetmessages: + msg7548
2022-05-31 07:24:56schlatterbecksetmessages: + msg7547
2022-05-31 07:17:27schlatterbecksetmessages: + msg7546
2022-05-31 02:07:20rouiljsetmessages: + msg7545
2022-05-30 21:35:15rouiljsetmessages: + msg7544
2022-05-30 21:27:02rouiljsetmessages: + msg7542
2022-05-30 21:01:50schlatterbecksetmessages: + msg7541
2022-05-30 16:21:24rouiljsetmessages: + msg7540
2022-05-30 16:05:09schlatterbecksetstatus: new -> closed
resolution: fixed
messages: + msg7539
2022-05-30 15:57:25schlatterbeckcreate