From c29842e6971ed8a6d82460cae96b401257cdef96 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 23 Mar 2022 17:26:31 +0100 Subject: [PATCH 1/2] Add sorting to GraphAPI users and groups The GraphAPI endpoints for users and groups support ordering now. User can be ordered by displayName, onPremisesSamAccountName and mail. Groups can be ordered by displayName. Example: https://localhost:9200/graph/v1.0/groups?$orderby=displayName asc Closes #3360 --- changelog/unreleased/graph-user-sort.md | 10 +++++ graph/pkg/service/v0/groups.go | 40 ++++++++++++++++++ graph/pkg/service/v0/ordering.go | 56 +++++++++++++++++++++++++ graph/pkg/service/v0/users.go | 44 +++++++++++++++++++ 4 files changed, 150 insertions(+) create mode 100644 changelog/unreleased/graph-user-sort.md diff --git a/changelog/unreleased/graph-user-sort.md b/changelog/unreleased/graph-user-sort.md new file mode 100644 index 000000000..e7f8a2885 --- /dev/null +++ b/changelog/unreleased/graph-user-sort.md @@ -0,0 +1,10 @@ +Enhancement: Add sorting to GraphAPI users and groups + +The GraphAPI endpoints for users and groups support ordering now. +User can be ordered by displayName, onPremisesSamAccountName and mail. +Groups can be ordered by displayName. + +Example: +https://localhost:9200/graph/v1.0/groups?$orderby=displayName asc + +https://github.com/owncloud/ocis/issues/3360 diff --git a/graph/pkg/service/v0/groups.go b/graph/pkg/service/v0/groups.go index 76c74f212..8c76c5c18 100644 --- a/graph/pkg/service/v0/groups.go +++ b/graph/pkg/service/v0/groups.go @@ -6,8 +6,10 @@ import ( "fmt" "net/http" "net/url" + "sort" "strings" + "github.com/CiscoM31/godata" libregraph "github.com/owncloud/libre-graph-api-go" "github.com/owncloud/ocis/graph/pkg/service/v0/errorcode" @@ -19,6 +21,14 @@ const memberRefsLimit = 20 // GetGroups implements the Service interface. func (g Graph) GetGroups(w http.ResponseWriter, r *http.Request) { + sanitizedPath := strings.TrimPrefix(r.URL.Path, "/graph/v1.0/") + odataReq, err := godata.ParseRequest(r.Context(), sanitizedPath, r.URL.Query()) + if err != nil { + g.logger.Err(err).Interface("query", r.URL.Query()).Msg("query error") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) + return + } + groups, err := g.identityBackend.GetGroups(r.Context(), r.URL.Query()) if err != nil { @@ -29,6 +39,17 @@ func (g Graph) GetGroups(w http.ResponseWriter, r *http.Request) { errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) } } + + groups, err = sortGroups(odataReq, groups) + if err != nil { + var errcode errorcode.Error + if errors.As(err, &errcode) { + errcode.Render(w, r) + } else { + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) + } + return + } render.Status(r, http.StatusOK) render.JSON(w, r, &listResponse{Value: groups}) } @@ -318,3 +339,22 @@ func (g Graph) parseMemberRef(ref string) (string, string, error) { memberType := segments[len(segments)-2] return memberType, id, nil } + +func sortGroups(req *godata.GoDataRequest, groups []*libregraph.Group) ([]*libregraph.Group, error) { + var sorter sort.Interface + if req.Query.OrderBy == nil || len(req.Query.OrderBy.OrderByItems) != 1 { + return groups, nil + } + switch req.Query.OrderBy.OrderByItems[0].Field.Value { + case "displayName": + sorter = groupsByDisplayName{groups} + default: + return nil, fmt.Errorf("we do not support <%s> as a order parameter", req.Query.OrderBy.OrderByItems[0].Field.Value) + } + + if req.Query.OrderBy.OrderByItems[0].Order == "asc" { + sorter = sort.Reverse(sorter) + } + sort.Sort(sorter) + return groups, nil +} diff --git a/graph/pkg/service/v0/ordering.go b/graph/pkg/service/v0/ordering.go index 4dd0f4a54..0655af011 100644 --- a/graph/pkg/service/v0/ordering.go +++ b/graph/pkg/service/v0/ordering.go @@ -45,3 +45,59 @@ func (s spacesByLastModifiedDateTime) Less(i, j int) bool { // fallback to name if no dateTime is set on both items return strings.ToLower(*s.spacesSlice[i].Name) > strings.ToLower(*s.spacesSlice[j].Name) } + +type userSlice []*libregraph.User + +// Len is the number of elements in the collection. +func (d userSlice) Len() int { return len(d) } + +// Swap swaps the elements with indexes i and j. +func (d userSlice) Swap(i, j int) { d[i], d[j] = d[j], d[i] } + +type usersByDisplayName struct { + userSlice +} + +type usersByMail struct { + userSlice +} + +type usersByOnPremisesSamAccountName struct { + userSlice +} + +// Less reports whether the element with index i +// must sort before the element with index j. +func (u usersByDisplayName) Less(i, j int) bool { + return strings.ToLower(u.userSlice[i].GetDisplayName()) > strings.ToLower(u.userSlice[j].GetDisplayName()) +} + +// Less reports whether the element with index i +// must sort before the element with index j. +func (u usersByMail) Less(i, j int) bool { + return strings.ToLower(u.userSlice[i].GetMail()) > strings.ToLower(u.userSlice[j].GetMail()) +} + +// Less reports whether the element with index i +// must sort before the element with index j. +func (u usersByOnPremisesSamAccountName) Less(i, j int) bool { + return strings.ToLower(u.userSlice[i].GetOnPremisesSamAccountName()) > strings.ToLower(u.userSlice[j].GetOnPremisesSamAccountName()) +} + +type groupSlice []*libregraph.Group + +// Len is the number of elements in the collection. +func (d groupSlice) Len() int { return len(d) } + +// Swap swaps the elements with indexes i and j. +func (d groupSlice) Swap(i, j int) { d[i], d[j] = d[j], d[i] } + +type groupsByDisplayName struct { + groupSlice +} + +// Less reports whether the element with index i +// must sort before the element with index j. +func (g groupsByDisplayName) Less(i, j int) bool { + return strings.ToLower(g.groupSlice[i].GetDisplayName()) > strings.ToLower(g.groupSlice[j].GetDisplayName()) +} diff --git a/graph/pkg/service/v0/users.go b/graph/pkg/service/v0/users.go index 182fc5cd9..7495a2f54 100644 --- a/graph/pkg/service/v0/users.go +++ b/graph/pkg/service/v0/users.go @@ -7,7 +7,10 @@ import ( "net/http" "net/url" "regexp" + "sort" + "strings" + "github.com/CiscoM31/godata" revactx "github.com/cs3org/reva/v2/pkg/ctx" "github.com/go-chi/chi/v5" "github.com/go-chi/render" @@ -38,6 +41,13 @@ func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) { // GetUsers implements the Service interface. func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) { + sanitizedPath := strings.TrimPrefix(r.URL.Path, "/graph/v1.0/") + odataReq, err := godata.ParseRequest(r.Context(), sanitizedPath, r.URL.Query()) + if err != nil { + g.logger.Err(err).Interface("query", r.URL.Query()).Msg("query error") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) + return + } users, err := g.identityBackend.GetUsers(r.Context(), r.URL.Query()) if err != nil { var errcode errorcode.Error @@ -48,6 +58,17 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) { } return } + + users, err = sortUsers(odataReq, users) + if err != nil { + var errcode errorcode.Error + if errors.As(err, &errcode) { + errcode.Render(w, r) + } else { + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) + } + return + } render.Status(r, http.StatusOK) render.JSON(w, r, &listResponse{Value: users}) } @@ -234,3 +255,26 @@ func isValidEmail(e string) bool { } return emailRegex.MatchString(e) } + +func sortUsers(req *godata.GoDataRequest, users []*libregraph.User) ([]*libregraph.User, error) { + var sorter sort.Interface + if req.Query.OrderBy == nil || len(req.Query.OrderBy.OrderByItems) != 1 { + return users, nil + } + switch req.Query.OrderBy.OrderByItems[0].Field.Value { + case "displayName": + sorter = usersByDisplayName{users} + case "mail": + sorter = usersByMail{users} + case "onPremisesSamAccountName": + sorter = usersByOnPremisesSamAccountName{users} + default: + return nil, fmt.Errorf("we do not support <%s> as a order parameter", req.Query.OrderBy.OrderByItems[0].Field.Value) + } + + if req.Query.OrderBy.OrderByItems[0].Order == "asc" { + sorter = sort.Reverse(sorter) + } + sort.Sort(sorter) + return users, nil +} From 24897f8b2d843abc7a1feabf3e0583b52c711b25 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 29 Mar 2022 11:11:03 +0200 Subject: [PATCH 2/2] ordering: fix the semantics of Less() This tries to address a semantic glitch in Less() to actually do a "less than" operation on strings and timestamps. It does not change the actual behaviour of the endpoints that support "orderby". The sorted results will be the same as before. --- graph/pkg/service/v0/drives.go | 2 +- graph/pkg/service/v0/groups.go | 2 +- graph/pkg/service/v0/ordering.go | 24 ++++++++++++------------ graph/pkg/service/v0/users.go | 2 +- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/graph/pkg/service/v0/drives.go b/graph/pkg/service/v0/drives.go index c2abdc436..0c1fda57c 100644 --- a/graph/pkg/service/v0/drives.go +++ b/graph/pkg/service/v0/drives.go @@ -791,7 +791,7 @@ func sortSpaces(req *godata.GoDataRequest, spaces []*libregraph.Drive) ([]*libre return nil, fmt.Errorf("we do not support <%s> as a order parameter", req.Query.OrderBy.OrderByItems[0].Field.Value) } - if req.Query.OrderBy.OrderByItems[0].Order == "asc" { + if req.Query.OrderBy.OrderByItems[0].Order == "desc" { sorter = sort.Reverse(sorter) } sort.Sort(sorter) diff --git a/graph/pkg/service/v0/groups.go b/graph/pkg/service/v0/groups.go index 8c76c5c18..0baa90759 100644 --- a/graph/pkg/service/v0/groups.go +++ b/graph/pkg/service/v0/groups.go @@ -352,7 +352,7 @@ func sortGroups(req *godata.GoDataRequest, groups []*libregraph.Group) ([]*libre return nil, fmt.Errorf("we do not support <%s> as a order parameter", req.Query.OrderBy.OrderByItems[0].Field.Value) } - if req.Query.OrderBy.OrderByItems[0].Order == "asc" { + if req.Query.OrderBy.OrderByItems[0].Order == "desc" { sorter = sort.Reverse(sorter) } sort.Sort(sorter) diff --git a/graph/pkg/service/v0/ordering.go b/graph/pkg/service/v0/ordering.go index 0655af011..117356a4f 100644 --- a/graph/pkg/service/v0/ordering.go +++ b/graph/pkg/service/v0/ordering.go @@ -24,7 +24,7 @@ type spacesByLastModifiedDateTime struct { // Less reports whether the element with index i // must sort before the element with index j. func (s spacesByName) Less(i, j int) bool { - return strings.ToLower(*s.spacesSlice[i].Name) > strings.ToLower(*s.spacesSlice[j].Name) + return strings.ToLower(*s.spacesSlice[i].Name) < strings.ToLower(*s.spacesSlice[j].Name) } // Less reports whether the element with index i @@ -32,18 +32,18 @@ func (s spacesByName) Less(i, j int) bool { func (s spacesByLastModifiedDateTime) Less(i, j int) bool { // compare the items when both dates are set if s.spacesSlice[i].LastModifiedDateTime != nil && s.spacesSlice[j].LastModifiedDateTime != nil { - return s.spacesSlice[i].LastModifiedDateTime.After(*s.spacesSlice[j].LastModifiedDateTime) + return s.spacesSlice[i].LastModifiedDateTime.Before(*s.spacesSlice[j].LastModifiedDateTime) } - // move left item down if it has no value + // an item without a timestamp is considered "less than" an item with a timestamp if s.spacesSlice[i].LastModifiedDateTime == nil && s.spacesSlice[j].LastModifiedDateTime != nil { - return false - } - // move right item down if it has no value - if s.spacesSlice[i].LastModifiedDateTime != nil && s.spacesSlice[j].LastModifiedDateTime == nil { return true } + // an item without a timestamp is considered "less than" an item with a timestamp + if s.spacesSlice[i].LastModifiedDateTime != nil && s.spacesSlice[j].LastModifiedDateTime == nil { + return false + } // fallback to name if no dateTime is set on both items - return strings.ToLower(*s.spacesSlice[i].Name) > strings.ToLower(*s.spacesSlice[j].Name) + return strings.ToLower(*s.spacesSlice[i].Name) < strings.ToLower(*s.spacesSlice[j].Name) } type userSlice []*libregraph.User @@ -69,19 +69,19 @@ type usersByOnPremisesSamAccountName struct { // Less reports whether the element with index i // must sort before the element with index j. func (u usersByDisplayName) Less(i, j int) bool { - return strings.ToLower(u.userSlice[i].GetDisplayName()) > strings.ToLower(u.userSlice[j].GetDisplayName()) + return strings.ToLower(u.userSlice[i].GetDisplayName()) < strings.ToLower(u.userSlice[j].GetDisplayName()) } // Less reports whether the element with index i // must sort before the element with index j. func (u usersByMail) Less(i, j int) bool { - return strings.ToLower(u.userSlice[i].GetMail()) > strings.ToLower(u.userSlice[j].GetMail()) + return strings.ToLower(u.userSlice[i].GetMail()) < strings.ToLower(u.userSlice[j].GetMail()) } // Less reports whether the element with index i // must sort before the element with index j. func (u usersByOnPremisesSamAccountName) Less(i, j int) bool { - return strings.ToLower(u.userSlice[i].GetOnPremisesSamAccountName()) > strings.ToLower(u.userSlice[j].GetOnPremisesSamAccountName()) + return strings.ToLower(u.userSlice[i].GetOnPremisesSamAccountName()) < strings.ToLower(u.userSlice[j].GetOnPremisesSamAccountName()) } type groupSlice []*libregraph.Group @@ -99,5 +99,5 @@ type groupsByDisplayName struct { // Less reports whether the element with index i // must sort before the element with index j. func (g groupsByDisplayName) Less(i, j int) bool { - return strings.ToLower(g.groupSlice[i].GetDisplayName()) > strings.ToLower(g.groupSlice[j].GetDisplayName()) + return strings.ToLower(g.groupSlice[i].GetDisplayName()) < strings.ToLower(g.groupSlice[j].GetDisplayName()) } diff --git a/graph/pkg/service/v0/users.go b/graph/pkg/service/v0/users.go index 7495a2f54..0cc29954f 100644 --- a/graph/pkg/service/v0/users.go +++ b/graph/pkg/service/v0/users.go @@ -272,7 +272,7 @@ func sortUsers(req *godata.GoDataRequest, users []*libregraph.User) ([]*libregra return nil, fmt.Errorf("we do not support <%s> as a order parameter", req.Query.OrderBy.OrderByItems[0].Field.Value) } - if req.Query.OrderBy.OrderByItems[0].Order == "asc" { + if req.Query.OrderBy.OrderByItems[0].Order == "desc" { sorter = sort.Reverse(sorter) } sort.Sort(sorter)