Issue 2551207
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:38 | rouilj | set | messages: + msg7549 |
2022-05-31 07:29:58 | schlatterbeck | set | messages: + msg7548 |
2022-05-31 07:24:56 | schlatterbeck | set | messages: + msg7547 |
2022-05-31 07:17:27 | schlatterbeck | set | messages: + msg7546 |
2022-05-31 02:07:20 | rouilj | set | messages: + msg7545 |
2022-05-30 21:35:15 | rouilj | set | messages: + msg7544 |
2022-05-30 21:27:02 | rouilj | set | messages: + msg7542 |
2022-05-30 21:01:50 | schlatterbeck | set | messages: + msg7541 |
2022-05-30 16:21:24 | rouilj | set | messages: + msg7540 |
2022-05-30 16:05:09 | schlatterbeck | set | status: new -> closed resolution: fixed messages: + msg7539 |
2022-05-30 15:57:25 | schlatterbeck | create |