Opened 8 years ago
Closed 6 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 )
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:
/baseURL/?sort=+field /baseURL/?sort=-field
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 8 years ago by
Component: | General → Data |
---|---|
Description: | modified (diff) |
Owner: | set to Kris Zyp |
comment:2 Changed 8 years ago by
comment:3 Changed 7 years ago by
I believe it's the same problem as in #14537 which is closed now. Actually according to RFC http://www.ietf.org/rfc/rfc2396.txt (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:5 Changed 7 years ago by
Milestone: | tbd → 1.9 |
---|
comment:6 Changed 7 years ago by
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 7 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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) + sort.attribute);
Reopening so this gets considered.
comment:9 Changed 7 years ago by
Milestone: | 1.9 → 1.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 6 years ago by
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 6 years ago by
Priority: | undecided → low |
---|
comment:12 Changed 6 years ago by
Owner: | changed from Kris Zyp to alaska |
---|---|
Status: | reopened → pending |
comment:13 Changed 6 years ago by
Resolution: | → patchwelcome |
---|---|
Status: | pending → closed |
'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):
Patch:
I suppose you could also do something like
}}}