Roundup Tracker - Issues

Issue 2551120

classification
The sorted method of MultilinkHTMLProperty crashes, if the given property is unset for an element of the list
Type: crash Severity: major
Components: Web interface Versions: 2.0.0
process
Status: fixed fixed
:
: rouilj : ngaba, rouilj, schlatterbeck
Priority: high : Blocker, patch

Created on 2021-03-25 01:06 by ngaba, last changed 2022-05-30 21:27 by rouilj.

Files
File name Uploaded Description Edit Remove
sort_within_multilink_support_NoneType.patch rouilj, 2021-03-28 14:53 Patch for sorting issues
Messages
msg7139 Author: [hidden] (ngaba) Date: 2021-03-25 01:06
Everything is said in the title, and it is discussed in the ML:
https://sourceforge.net/p/roundup/mailman/message/37247243/

The obtained error is:
<class 'TypeError'>: '<' not supported between instances of 'str' and 'NoneType'
msg7143 Author: [hidden] (rouilj) Date: 2021-03-28 02:36
Hi Nagy:

Is this the error you get when you are ordering a multilink by
an integer property?

-- rouilj
msg7144 Author: [hidden] (rouilj) Date: 2021-03-28 03:39
I think the way to handle this is to return tuples to
the sort routine. E.G.:

 sorted(propname, reverse=False, NoneFirst=False):

     NoneCode = (2,0)[NoneFirst] # use 2 if NoneFirst is False to sort 
to end

     def keyfunc(a):
	  if v:
	     return (1, v)
	  elif v is None
	     return (NoneCode, v)

     value.sort(key=keyfunc, reverse=reverse)

Returning a tuple from the keyfunc will allow anything that is not
None to sort first by 1 and then by the value of the item.  Items that
are None will sort before or after the items starting with 1 (0 or 2)
followed by comparing None to None which is supported.

E.G. sorting:

         id:   1  2,      3, 4,    5
   prop val:   5, 20, None, 16, None

should result in (with NoneFirst = False): 1,4,2,3,5. Sort is stable
so you should not be able to get 1,4,2,5,3. If this isn't true then I
don't see any way to add an order element last in the tuple to
preserve order.

Comments?

Also I may need to change the sort_repr methods to use the same
mechanism to sort None items as well.
msg7145 Author: [hidden] (rouilj) Date: 2021-03-28 14:53
I think this patch fixes the exception thrown when the sort
field includes None values. It may also fix the problem with
integers not sorting as integers.

Nagy, can you apply this (to roundup/cgi/templating.py)
and see if it fixes both this issue and issue2551122.

It also includes (commented out) code to address issue255112
by using the sort_repr function from the hyperdb base class
for the data type to sort by the base type of the data. I don't
think this is needed as using the _value from the HTMLProperty
object seems to make sorting correct.

Ralf it looks like the existing test code for test_templating is
not suitable for testing this. I think I need a real database
underneath it. Thoughts on how to test this.

-- rouilj
msg7152 Author: [hidden] (ngaba) Date: 2021-03-29 12:18
Dear John,

"Is this the error you get when you are ordering a multilink by
an integer property?"

When the property is itself and integer, for example on
context.documents.sorted('intprop')
where 'intprop' is a property defined as intprop = Integer() in the class 'documents' pointing to, then I get (before applying any of your patches):

<class 'TypeError'>: '<' not supported between instances of 'int' and 'NoneType'.

However, if I I call 
context.documents.sorted('classprop') 
where 'classprop' is a Class() with integer order property, then I still get 

<class 'TypeError'>: '<' not supported between instances of 'NoneType' and 'str'.

I hope this agrees with your observations.
msg7153 Author: [hidden] (ngaba) Date: 2021-03-29 12:53
Now I applied your sort_within_multilink_support_NoneType.patch.

This issue2551120 is fixed (in my usecase, at least :) for both 'intprop' and 'classprop' examples, NoneFirst also works as desired.

As I see, issue2551122 is only fixed for the 'intprop' case above.
In the 'classprop' case, the obtained list is still sorted as if the order property of 'classprop' were string.

[edited to fix issue2551122: rouilj 2021-04-12]
msg7190 Author: [hidden] (rouilj) Date: 2021-04-12 19:40
Hi Nagy:

You said:

> However, if I I call 
> context.documents.sorted('classprop') 
> where 'classprop' is a Class() with integer order property,
> then I still get 
>
> <class 'TypeError'>: '<' not supported between instances of 'NoneType' 
and 'str'.

by classprop do you mean classprop is a multilink to a Class?

From customizing.txt:

  Select a property of the class to be the order property. The order
  property is used whenever using a default sort order for the class,
  e.g., when grouping or sorting class A by a link to class B in the
  user interface, the order property of class B is used for sorting.

That is the only sorting that I see as mentioned in customizing.txt
that seems to sort of match what you stated.

I am having issues developing a test case for this code. I'll spend
another few hours of getting a test case put together. If I can't get 
something running, I'll commit the code.

Regarding issue2551122 (sorted method of MultilinkHTMLProperty does a 
string sort even if the property is an integer) still failing, I am not
sure. If my description of the 'classprop' case is correct, I'll update
and continue the discussion on that issue and close this issue.

I may need a more detailed test case for that issue as I expected
this patch would have (by accident) handled both cases.
msg7210 Author: [hidden] (ngaba) Date: 2021-04-22 01:00
> by classprop do you mean classprop is a multilink to a Class?

No. It is just a link.

It may be undocumented, but in this case 
context.documents.sorted('classprop')
sorts the documents list by the order property of the (linked) classprop property of the elements in this list. [*]

And in my opinion, this is the only sensible definition of the above command (in real life situations 'classprop' can be 'importance', for example: and instead of hard-coded numbers we use a Class() for representing the possible values of importance). Otherwise, this operation should be undefined. :)

And the same "bug" occurs in [*] as in issue2551122: The order property is always treated as string.
msg7233 Author: [hidden] (rouilj) Date: 2021-05-17 19:27
Committed patch, doc changes: changeset:   6414:3dbf1bc5e567

This is missing automated tests. I spent 3x more effort into getting
a test for this than I spent fixing the code. Finally gave up.
msg7543 Author: [hidden] (rouilj) Date: 2022-05-30 21:27
see also issue2551207.
History
Date User Action Args
2022-05-30 21:27:26rouiljsetmessages: + msg7543
2021-05-17 19:27:37rouiljsetstatus: open -> fixed
resolution: fixed
messages: + msg7233
2021-05-06 19:19:21rouiljsetkeywords: + Blocker
2021-04-22 01:00:32ngabasetmessages: + msg7210
2021-04-12 19:40:52rouiljsetmessages: + msg7190
2021-03-29 12:53:42ngabasetmessages: + msg7153
2021-03-29 12:18:37ngabasetmessages: + msg7152
2021-03-28 18:45:26rouiljsetpriority: high
assignee: rouilj
status: new -> open
2021-03-28 14:53:41rouiljsetfiles: + sort_within_multilink_support_NoneType.patch
keywords: + patch
messages: + msg7145
2021-03-28 03:39:02rouiljsetmessages: + msg7144
2021-03-28 02:36:54rouiljsetmessages: + msg7143
2021-03-27 03:41:44rouiljsetnosy: + rouilj
severity: minor -> major
2021-03-25 08:30:19schlatterbecksetnosy: + schlatterbeck
2021-03-25 01:06:33ngabacreate