Opened 10 years ago

Closed 8 years ago

#15047 closed defect (patchwelcome)

JsonRest should nor use a plus sign when sorting ascending.

Reported by: alaska Owned by: alaska
Priority: low Milestone: 1.10
Component: Data Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

I am using a DataGrid linked to a JsonRest store pointing at a Sinatra backend. I set "sortParam" to "sort" when I initialize the store, which should be sending urls like the following when sorting:


The problem is that Sinatra (like a lot of other frameworks, I imagine) is helpfully replacing the "+" with a space, given that that is what it is supposed to signify in URLs, so in the backend I'm getting params like

{"sort" =>" field"} and {"sort" =>"-field"}

This behavior should be changed or at least user-customizable. I recommend something like "ASC:field" "DSC:field".

Change History (13)

comment:1 Changed 10 years ago by bill

Component: GeneralData
Description: modified (diff)
Owner: set to Kris Zyp

comment:2 Changed 10 years ago by alaska

'DESC:' would probably be better than 'DSC:', actually, as you can just parse it out and drop it directly into the SQL (provided you're smart enough to use a regex like the following that won't allow just anything in):



--- dojo/store/JsonRest.js.uncompressed.js      2012-02-16 15:01:29.000000000 -0
+++ new/store/JsonRest.js.uncompressed.js       2012-03-20 21:40:00.985753363 -0
@@ -128,13 +128,13 @@
                if(query && typeof query == "object"){
                        query = xhr.objectToQuery(query);
                        query = query ? "?" + query: "";
-               }
-               if(options && options.sort){
+                }
+                if(options && options.sort){
                        var sortParam = this.sortParam;
                        query += (query ? "&" : "?") + (sortParam ? sortParam + 
'=' : "sort(");
                        for(var i = 0; i<options.sort.length; i++){
                                var sort = options.sort[i];
-                               query += (i > 0 ? "," : "") + (sort.descending ?
 '-' : '+') + encodeURIComponent(sort.attribute);
+                               query += (i > 0 ? "," : "") + (sort.descending ?
 'ASC:' : 'DESC:') + encodeURIComponent(sort.attribute);
                                query += ")";

I suppose you could also do something like

query += (i > 0 ? "," : "") + encodeURIComponent((sort.descending ? '-' : '+') + sort.attribute);


comment:3 Changed 9 years ago by b0dh1

I believe it's the same problem as in #14537 which is closed now. Actually according to RFC (see 3.4. Query Component):

Within a query component, the characters ";", "/", "?", ":", "@", "&", "=", "+", ",", and "$" are reserved.

Which means that using an unencoded plus in a query is a violation of this specification (as well as a suggestion to treat space as a plus on a server-side). Therefore if you are using "+" in a query it should be encoded. Perhaps there is a way in Dojo to make it configurable which strings JsonRest uses as DESC and ASC query attributes?

comment:4 Changed 9 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

In [30873]:

Allow for alternate ascending and descending prefixes, fixes #15047 !strict

comment:5 Changed 9 years ago by bill

Milestone: tbd1.9

comment:6 Changed 9 years ago by cmketchu

Allowing for alternate prefixes is nice, but leaving the default sort indicators as + and - will never work for the same reasons the bug was opened. I feel like the correct fix here is to encode the prefixes so that we are not in violation the RFC spec and can continue to use the defaults.

comment:7 Changed 9 years ago by bill

Resolution: fixed
Status: closedreopened

Yeah, sounds like you need the encodeURIComponent() call to process the prefixes too, like:

query += (i > 0 ? "," : "") +
encodeURIComponent((sort.descending ? '-' : '+') + sort.attribute)

or if you still want the customization from [30873] then:

query += (i > 0 ? "," : "") + encodeURIComponent(
(sort.descending ? this.descendingPrefix : this.ascendingPrefix) +

Reopening so this gets considered.

comment:8 Changed 9 years ago by freddefisk

Related ticket: #16876

comment:9 Changed 9 years ago by bill

Milestone: 1.91.10

Bumping this ticket since we are past the deadline for the 1.9RC. The fix can be put into 1.9.1 too, if desired.

comment:10 Changed 8 years ago by Kris Zyp

But this isn't a backwards compatible fix is it? Also, I am probably reading this wrong, but I am curious how this violates the RFC spec. According to the grammar in RFC 2986, it looks like the query string is supposed to consist of pchar's which include sub-delims, which include '+'.

comment:11 Changed 8 years ago by Kris Zyp

Priority: undecidedlow

comment:12 Changed 8 years ago by dylan

Owner: changed from Kris Zyp to alaska
Status: reopenedpending

comment:13 Changed 8 years ago by dylan

Resolution: patchwelcome
Status: pendingclosed
Note: See TracTickets for help on using tickets.