go-jmap: Fix an issue with `isAscending` v1 PROPOSED

Hi there!

I spent a while trying to figure out why JavaScript and Go clients were
returning different results for the same query, turns out this was the
root cause.

Relevant snippet from the JMAP Core spec (https://jmap.io/spec-
core.html):

"""
isAscending: Boolean (optional; default: true) If true, sort in
ascending order. If false, reverse the comparator’s results to sort in
descending order.
"""

Brandon Sprague (2):
  Fix an issue with `isAscending`
  Fix tests

 mail/email/sort.go           | 2 +-
 mail/emailsubmission/sort.go | 2 +-
 mail/mailbox/query_test.go   | 2 +-
 mail/mailbox/sort.go         | 2 +-
 mail/mailbox/sort_test.go    | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.38.5

Thanks!

One thing that didn't occur to me until after is that this
unfortunately isn't a backwards compatible change. Since the library
is pre-1.0.0, it's probably fine, but worth noting. Anywhere that
'IsAscending' is not specified will now sort in the opposite direction
(assuming a compliant JMAP server).

The other alternative would be to make 'isAscending' a *bool, but
that's just backwards-incompatible in a different way, causing
compilation errors for anyone currently specifying 'IsAscending'. That
approach has the benefit of failing loudly rather than quietly though.

Tim Culverhouse <tim@timculverhouse.com>

1 year, 1 month ago

I noticed this too, and decided that yes - pre-1.0.0 is fine to make this
change! Thanks for pointing it out.

Brandon

[PATCH go-jmap 1/2] Fix an issue with `isAscending` Export this patch

From: Brandon Sprague <brandon@sprague.mx>

According to the spec, it defaults to true. But since `omitempty` was set, the absence == the zero value == false, so it ends up true in the API.
---

Tim Culverhouse <tim@timculverhouse.com>

1 year, 1 month ago

Hey Brandon -

Thanks for the patch! Just so I am clear on where this is coming up...if you are
a client and set sortAscending to false, it will be empty in JSON thus
defaulting to true...effectively making it impossible to sort descending. Do I
have that right?

 mail/email/sort.go           | 2 +-
 mail/emailsubmission/sort.go | 2 +-
 mail/mailbox/sort.go         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mail/email/sort.go b/mail/email/sort.go
index f28e396..ddfa791 100644
--- a/mail/email/sort.go
+++ b/mail/email/sort.go
@@ -9,7 +9,7 @@ type SortComparator struct {
 
 	Keyword string `json:"keyword,omitempty"`
 
-	IsAscending bool `json:"isAscending,omitempty"`
+	IsAscending bool `json:"isAscending"`
 
 	Collation jmap.CollationAlgo `json:"collation,omitempty"`
 }
diff --git a/mail/emailsubmission/sort.go b/mail/emailsubmission/sort.go
index 04d6174..38083a9 100644
--- a/mail/emailsubmission/sort.go
+++ b/mail/emailsubmission/sort.go
@@ -5,7 +5,7 @@ import "git.sr.ht/~rockorager/go-jmap"
 type SortComparator struct {
 	Property string `json:"property,omitempty"`
 
-	IsAscending bool `json:"isAscending,omitempty"`
+	IsAscending bool `json:"isAscending"`
 
 	Collation jmap.CollationAlgo `json:"collation,omitempty"`
 }
diff --git a/mail/mailbox/sort.go b/mail/mailbox/sort.go
index fb07b6d..560eea8 100644
--- a/mail/mailbox/sort.go
+++ b/mail/mailbox/sort.go
@@ -7,7 +7,7 @@ type SortComparator struct {
 	Property string `json:"property,omitempty"`
 
 	// If true, sort in ascending order.
-	IsAscending bool `json:"isAscending,omitempty"`
+	IsAscending bool `json:"isAscending"`
 
 	// The identifier, as registered in the collation registry defined in
 	// RFC4790, for the algorithm to use when comparing the order of
-- 
2.38.5

[PATCH go-jmap 2/2] Fix tests Export this patch

From: Brandon Sprague <brandon@sprague.mx>

---
 mail/mailbox/query_test.go | 2 +-
 mail/mailbox/sort_test.go  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mail/mailbox/query_test.go b/mail/mailbox/query_test.go
index 05d29ab..fb96497 100644
--- a/mail/mailbox/query_test.go
+++ b/mail/mailbox/query_test.go
@@ -23,6 +23,6 @@ func TestQuery(t *testing.T) {
 	}
 	data, err := json.Marshal(query)
 	assert.NoError(err)
-	expected := `{"accountId":"xyz","filter":{"name":"Inbox"},"sort":[{"property":"name"}],"limit":10}`
+	expected := `{"accountId":"xyz","filter":{"name":"Inbox"},"sort":[{"property":"name","isAscending":false}],"limit":10}`
 	assert.Equal(expected, string(data))
 }
diff --git a/mail/mailbox/sort_test.go b/mail/mailbox/sort_test.go
index ca4771c..f170ff0 100644
--- a/mail/mailbox/sort_test.go
+++ b/mail/mailbox/sort_test.go
@@ -18,6 +18,6 @@ func TestSort(t *testing.T) {
 	}
 	data, err := json.Marshal(query)
 	assert.NoError(err)
-	expected := `{"sort":[{"property":"name"}]}`
+	expected := `{"sort":[{"property":"name","isAscending":false}]}`
 	assert.Equal(expected, string(data))
 }
-- 
2.38.5