#8480 closed defect (fixed)
dojo.data.util.sorter.BasicComparator does not handle nulls properly
| Reported by: | criecke | Owned by: | Jared Jurkiewicz |
|---|---|---|---|
| Priority: | high | Milestone: | 1.3 |
| Component: | Data | Version: | 1.2.3 |
| Keywords: | Cc: | ||
| Blocked By: | Blocking: |
Description (last modified by )
According to JavaScript spec, the expression a > b is always true if either a or b is null. BasicComparator is spec'ed to return 1 if a===null && b!==null and -1 if a!==null && b===null. However, that's not second case does not hold because a>b is true, thus it returns 1. That messes up sorts with null values because it effectively says null>999 and 999>null.
Attachments (2)
Change History (12)
comment:1 Changed 11 years ago by
| Component: | General → Data |
|---|---|
| Description: | modified (diff) |
| Owner: | changed from anonymous to Jared Jurkiewicz |
comment:2 Changed 11 years ago by
| Milestone: | tbd → future |
|---|
comment:3 Changed 11 years ago by
Are you sure your claim is correct? I've tested this now on three browsers:
Firefox 2
IE 7
Safari 3.1
All of them return:
null>999: false 999>null: true 999>undefined: false undefined>999: false
Which does not support the statement that a > b if either a or b is null.
I'llattached my exact testcase showing this.
comment:4 Changed 11 years ago by
Are you sure your claim is correct? I've tested this now on three browsers:
Firefox 2
IE 7
Safari 3.1
All of them return:
null>999: false 999>null: true 999>undefined: false undefined>999: false
Which does not support the statement that a > b is true if either a or b is null.
I'll attach my exact testcase showing this.
Changed 11 years ago by
| Attachment: | test_compare.html added |
|---|
comment:5 Changed 11 years ago by
Can you provide code illustrating where the current compare fails?
comment:6 Changed 11 years ago by
| Resolution: | → invalid |
|---|---|
| Status: | new → closed |
Marking invalid till testcase provided showing where the compare currently fails.
comment:7 Changed 11 years ago by
| Resolution: | invalid |
|---|---|
| Status: | closed → reopened |
Reopening. Doing a bit more investigation, but I think I understand your concern/issue.
Changed 11 years ago by
| Attachment: | sorter.patch added |
|---|
Patch for this. Code reduction (I believe) and should nor properly push null/undefined down in a sort.
comment:8 Changed 11 years ago by
comment:9 Changed 11 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | reopened → closed |
comment:10 Changed 11 years ago by
| Milestone: | future → 1.3 |
|---|
batch move of tickets marked 'future' in the 1.3 timeframe
Agreed this is an issue. Not sure of a small fix for it (which is critical for its position in core.) So ... I need to look at this for a bit and determine the best reduction.