Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#15515 closed defect (fixed)

SimpleQueryEngine doesn't correctly sort data with null [PATCH, CLA]

Reported by: Stefan Bird Owned by: Kris Zyp
Priority: undecided Milestone: 1.8
Component: Data Version: 1.7.2
Keywords: dohfail Cc:
Blocked By: Blocking:

Description

The sorting code in SimpleQueryEngine? assumes that all of the values it encounters are directly comparable, however strings and nulls are not: null < "hello"
null > "hello" == false. This means that sorting on an attribute that contains both strings and nulls will result in the rows being in an undefined order.

Note there are other situations where this can occur but the patch doesn't deal with them for performance reasons. The situation most likely to occur is sorting numbers with NaN values, eg [{a: 5}, {a: NaN}]. Maybe a flag could be added to the sort parameter to say that NaN values may be encountered (eg {attribute: "MyNumber?", mayHaveNaN: true}).

The code in the patch considers null to be larger than any string value; this is consistent with Array.sort()'s behaviour ([null, "hello"].sort() == ["hello", null])

Attachments (1)

SimpleQueryEngine.patch (1.8 KB) - added by Stefan Bird 7 years ago.

Download all attachments as: .zip

Change History (7)

Changed 7 years ago by Stefan Bird

Attachment: SimpleQueryEngine.patch added

comment:1 Changed 7 years ago by dante

fwiw, I have an override in dojo/data/util/sorter that exposes the "direction" of the sort for this reason. Currently, default sort treats nulls as "higher than any value", so they always appear at the top (or bottom if sorted descending...). I needed a behavior where nulls were always treated as "incomplete", and _always_ at the bottom regardless of sort direction. (when ascending sort, treat null as "lower than any value", descending, treat as "higher") ... slightly related to this ticket is all. Would be nice to expose the direction of the sort to the sorting function so that it could more easily handle custom considerations like this.

comment:2 Changed 7 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

In [29085]:

Properly handle nulls in sorting and allow sorting functions, fixes #15534, fixes #15515 !strict

comment:3 Changed 7 years ago by bill

Milestone: tbd1.8

comment:4 Changed 7 years ago by bill

Keywords: dohfail added
Resolution: fixed
Status: closedreopened

[29085] breaks util/doh/runner.html?testModule=dojo.tests.store.Memory on IE8:

    _AssertFailure: assertEqual() failed: 	expected
		four
	but got
		two

     ERROR IN: 		 function testQueryWithSort(t){
				t.is(store.query({prime: true}, {sort:[{attribute:"name"}]}).length, 3);
				t.is(store.query({even: true}, {sort:[{attribute:"name"}]})[1].name, "two");
				t.is(store.query({even: true}, {sort:function(a, b){
						return a > b ? -1 : 1;
					}})[1].name, "two");
				t.is(store.query(null, {sort:[{attribute:"mappedTo"}]})[4].name, "four");
			} FAILED test: testQueryWithSort 0 ms

comment:5 Changed 7 years ago by Kris Zyp

Resolution: fixed
Status: reopenedclosed

In [29243]:

Fix custom sorting function test to use an actual property value, fixes #15515 !strict

comment:6 Changed 7 years ago by Kris Zyp

#16065 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.