add orderby name and lastModifiedDateTime (#3201)

* add orderby name and lastModifiedDateTime

* add unit test for sort

* fix orderby name

* add more tests

* exclude test files from duplication analyzer

* remove flaky test

* order by name if no date is set

* refactor

* add test for wrong parameter name

changelog/unreleased/add-ordering-list-drives.md

@@ -0,0 +1,9 @@
+Enhancement: Add sorting to list Spaces
+
+We added the OData query param "orderBy" for listing spaces. We can now order by Space Name and LastModifiedDateTime.
+
+Example 1: https://localhost:9200/graph/v1.0/me/drives/?$orderby=lastModifiedDateTime desc
+Example 2: https://localhost:9200/graph/v1.0/me/drives/?$orderby=name asc
+
+https://github.com/owncloud/ocis/issues/3200
+https://github.com/owncloud/ocis/pull/3201

graph/pkg/service/v0/drives.go

@@ -7,6 +7,7 @@ import (
 	"math"
 	"net/http"
 	"net/url"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -72,15 +73,22 @@ func (g Graph) GetDrives(w http.ResponseWriter, r *http.Request) {
 		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
 		return
 	}
-	files, err := g.formatDrives(ctx, wdu, res.StorageSpaces)
+	spaces, err := g.formatDrives(ctx, wdu, res.StorageSpaces)
 	if err != nil {
 		g.logger.Error().Err(err).Msg("error encoding response as json")
 		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
 		return
 	}
 
+	spaces, err = sortSpaces(odataReq, spaces)
+	if err != nil {
+		g.logger.Error().Err(err).Msg("error sorting the spaces list")
+		errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error())
+		return
+	}
+
 	render.Status(r, http.StatusOK)
-	render.JSON(w, r, &listResponse{Value: files})
+	render.JSON(w, r, &listResponse{Value: spaces})
 }
 
 // GetSingleDrive does a lookup of a single space by spaceId
@@ -710,3 +718,24 @@ func (g Graph) DeleteDrive(w http.ResponseWriter, r *http.Request) {
 
 	w.WriteHeader(http.StatusNoContent)
 }
+
+func sortSpaces(req *godata.GoDataRequest, spaces []*libregraph.Drive) ([]*libregraph.Drive, error) {
+	var sorter sort.Interface
+	if req.Query.OrderBy == nil || len(req.Query.OrderBy.OrderByItems) != 1 {
+		return spaces, nil
+	}
+	switch req.Query.OrderBy.OrderByItems[0].Field.Value {
+	case "name":
+		sorter = spacesByName{spaces}
+	case "lastModifiedDateTime":
+		sorter = spacesByLastModifiedDateTime{spaces}
+	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 spaces, nil
+}

graph/pkg/service/v0/drives_test.go

@@ -0,0 +1,123 @@
+package svc
+
+import (
+	"testing"
+	"time"
+
+	"github.com/CiscoM31/godata"
+	libregraph "github.com/owncloud/libre-graph-api-go"
+	"github.com/stretchr/testify/assert"
+)
+
+type sortTest struct {
+	Drives       []*libregraph.Drive
+	Query        godata.GoDataRequest
+	DrivesSorted []*libregraph.Drive
+}
+
+var time1 = time.Date(2022, 02, 02, 15, 00, 00, 00, time.UTC)
+var time2 = time.Date(2022, 02, 03, 15, 00, 00, 00, time.UTC)
+var time3, time5, time6 *time.Time
+var time4 = time.Date(2022, 02, 05, 15, 00, 00, 00, time.UTC)
+var drives = []*libregraph.Drive{
+	drive("3", "project", "Admin", time3),
+	drive("1", "project", "Einstein", &time1),
+	drive("2", "project", "Marie", &time2),
+	drive("4", "project", "Richard", &time4),
+}
+var drivesLong = append(drives, []*libregraph.Drive{
+	drive("5", "project", "Bob", time5),
+	drive("6", "project", "Alice", time6),
+}...)
+
+var sortTests = []sortTest{
+	{
+		Drives: drives,
+		Query: godata.GoDataRequest{
+			Query: &godata.GoDataQuery{
+				OrderBy: &godata.GoDataOrderByQuery{
+					OrderByItems: []*godata.OrderByItem{
+						{Field: &godata.Token{Value: "name"}, Order: "asc"},
+					},
+				},
+			},
+		},
+		DrivesSorted: []*libregraph.Drive{
+			drive("3", "project", "Admin", time3),
+			drive("1", "project", "Einstein", &time1),
+			drive("2", "project", "Marie", &time2),
+			drive("4", "project", "Richard", &time4),
+		},
+	},
+	{
+		Drives: drives,
+		Query: godata.GoDataRequest{
+			Query: &godata.GoDataQuery{
+				OrderBy: &godata.GoDataOrderByQuery{
+					OrderByItems: []*godata.OrderByItem{
+						{Field: &godata.Token{Value: "name"}, Order: "desc"},
+					},
+				},
+			},
+		},
+		DrivesSorted: []*libregraph.Drive{
+			drive("4", "project", "Richard", &time4),
+			drive("2", "project", "Marie", &time2),
+			drive("1", "project", "Einstein", &time1),
+			drive("3", "project", "Admin", time3),
+		},
+	},
+	{
+		Drives: drivesLong,
+		Query: godata.GoDataRequest{
+			Query: &godata.GoDataQuery{
+				OrderBy: &godata.GoDataOrderByQuery{
+					OrderByItems: []*godata.OrderByItem{
+						{Field: &godata.Token{Value: "lastModifiedDateTime"}, Order: "asc"},
+					},
+				},
+			},
+		},
+		DrivesSorted: []*libregraph.Drive{
+			drive("3", "project", "Admin", time3),
+			drive("6", "project", "Alice", time6),
+			drive("5", "project", "Bob", time5),
+			drive("1", "project", "Einstein", &time1),
+			drive("2", "project", "Marie", &time2),
+			drive("4", "project", "Richard", &time4),
+		},
+	},
+	{
+		Drives: drivesLong,
+		Query: godata.GoDataRequest{
+			Query: &godata.GoDataQuery{
+				OrderBy: &godata.GoDataOrderByQuery{
+					OrderByItems: []*godata.OrderByItem{
+						{Field: &godata.Token{Value: "lastModifiedDateTime"}, Order: "desc"},
+					},
+				},
+			},
+		},
+		DrivesSorted: []*libregraph.Drive{
+			drive("4", "project", "Richard", &time4),
+			drive("2", "project", "Marie", &time2),
+			drive("1", "project", "Einstein", &time1),
+			drive("5", "project", "Bob", time5),
+			drive("6", "project", "Alice", time6),
+			drive("3", "project", "Admin", time3),
+		},
+	},
+}
+
+func drive(ID string, dType string, name string, lastModified *time.Time) *libregraph.Drive {
+	return &libregraph.Drive{Id: libregraph.PtrString(ID), DriveType: libregraph.PtrString(dType), Name: libregraph.PtrString(name), LastModifiedDateTime: lastModified}
+}
+
+// TestSort tests the available orderby queries
+func TestSort(t *testing.T) {
+	for _, test := range sortTests {
+		sorted, err := sortSpaces(&test.Query, test.Drives)
+		assert.NoError(t, err)
+		assert.Equal(t, test.DrivesSorted, sorted)
+	}
+}

graph/pkg/service/v0/graph_test.go

@@ -2,6 +2,7 @@ package svc_test
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"io"
 	"net/http"
@@ -12,9 +13,11 @@ import (
 	"github.com/cs3org/reva/pkg/rgrpc/status"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	libregraph "github.com/owncloud/libre-graph-api-go"
 	"github.com/owncloud/ocis/graph/mocks"
 	"github.com/owncloud/ocis/graph/pkg/config"
 	service "github.com/owncloud/ocis/graph/pkg/service/v0"
+	"github.com/owncloud/ocis/graph/pkg/service/v0/errorcode"
 	"github.com/stretchr/testify/mock"
 )
 
@@ -101,5 +104,114 @@ var _ = Describe("Graph", func() {
 			}
 			`))
 		})
+		It("can list a spaces with sort", func() {
+			gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{
+				Status: status.NewOK(ctx),
+				StorageSpaces: []*provider.StorageSpace{
+					{
+						Id:        &provider.StorageSpaceId{OpaqueId: "bspaceid"},
+						SpaceType: "bspacetype",
+						Root: &provider.ResourceId{
+							StorageId: "bspaceid",
+							OpaqueId:  "bopaqueid",
+						},
+						Name: "bspacename",
+					},
+					{
+						Id:        &provider.StorageSpaceId{OpaqueId: "aspaceid"},
+						SpaceType: "aspacetype",
+						Root: &provider.ResourceId{
+							StorageId: "aspaceid",
+							OpaqueId:  "anopaqueid",
+						},
+						Name: "aspacename",
+					},
+				},
+			}, nil)
+			gatewayClient.On("InitiateFileDownload", mock.Anything, mock.Anything).Return(&gateway.InitiateFileDownloadResponse{
+				Status: status.NewNotFound(ctx, "not found"),
+			}, nil)
+			gatewayClient.On("GetQuota", mock.Anything, mock.Anything).Return(&provider.GetQuotaResponse{
+				Status: status.NewUnimplemented(ctx, fmt.Errorf("not supported"), "not supported"),
+			}, nil)
+
+			r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives?$orderby=name%20asc", nil)
+			rr := httptest.NewRecorder()
+			svc.GetDrives(rr, r)
+
+			Expect(rr.Code).To(Equal(http.StatusOK))
+
+			body, _ := io.ReadAll(rr.Body)
+			Expect(body).To(MatchJSON(`
+			{
+				"value":[
+					{
+						"driveType":"aspacetype",
+						"id":"aspaceid",
+						"name":"aspacename",
+						"root":{
+							"id":"aspaceid!anopaqueid",
+							"webDavUrl":"https://localhost:9200/dav/spaces/aspaceid"
+						}
+					},
+					{
+						"driveType":"bspacetype",
+						"id":"bspaceid",
+						"name":"bspacename",
+						"root":{
+							"id":"bspaceid!bopaqueid",
+							"webDavUrl":"https://localhost:9200/dav/spaces/bspaceid"
+						}
+					}
+				]
+			}
+			`))
+		})
+		It("can not list spaces with wrong sort parameter", func() {
+			gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{
+				Status:        status.NewOK(ctx),
+				StorageSpaces: []*provider.StorageSpace{}}, nil)
+			gatewayClient.On("InitiateFileDownload", mock.Anything, mock.Anything).Return(&gateway.InitiateFileDownloadResponse{
+				Status: status.NewNotFound(ctx, "not found"),
+			}, nil)
+			gatewayClient.On("GetQuota", mock.Anything, mock.Anything).Return(&provider.GetQuotaResponse{
+				Status: status.NewUnimplemented(ctx, fmt.Errorf("not supported"), "not supported"),
+			}, nil)
+
+			r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives?$orderby=owner%20asc", nil)
+			rr := httptest.NewRecorder()
+			svc.GetDrives(rr, r)
+			Expect(rr.Code).To(Equal(http.StatusBadRequest))
+
+			body, _ := io.ReadAll(rr.Body)
+			var libreError libregraph.OdataError
+			err := json.Unmarshal(body, &libreError)
+			Expect(err).To(Not(HaveOccurred()))
+			Expect(libreError.Error.Message).To(Equal("we do not support <owner> as a order parameter"))
+			Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String()))
+		})
+		It("can list a spaces with invalid query parameter", func() {
+			gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{
+				Status:        status.NewOK(ctx),
+				StorageSpaces: []*provider.StorageSpace{}}, nil)
+			gatewayClient.On("InitiateFileDownload", mock.Anything, mock.Anything).Return(&gateway.InitiateFileDownloadResponse{
+				Status: status.NewNotFound(ctx, "not found"),
+			}, nil)
+			gatewayClient.On("GetQuota", mock.Anything, mock.Anything).Return(&provider.GetQuotaResponse{
+				Status: status.NewUnimplemented(ctx, fmt.Errorf("not supported"), "not supported"),
+			}, nil)
+
+			r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives?§orderby=owner%20asc", nil)
+			rr := httptest.NewRecorder()
+			svc.GetDrives(rr, r)
+			Expect(rr.Code).To(Equal(http.StatusBadRequest))
+
+			body, _ := io.ReadAll(rr.Body)
+			var libreError libregraph.OdataError
+			err := json.Unmarshal(body, &libreError)
+			Expect(err).To(Not(HaveOccurred()))
+			Expect(libreError.Error.Message).To(Equal("Query parameter '§orderby' is not supported. Cause: Query parameter '§orderby' is not supported"))
+			Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String()))
+		})
 	})
 })

graph/pkg/service/v0/ordering.go

@@ -0,0 +1,45 @@
+package svc
+
+import (
+	libregraph "github.com/owncloud/libre-graph-api-go"
+)
+
+type spacesSlice []*libregraph.Drive
+
+// Len is the number of elements in the collection.
+func (d spacesSlice) Len() int { return len(d) }
+
+// Swap swaps the elements with indexes i and j.
+func (d spacesSlice) Swap(i, j int) { d[i], d[j] = d[j], d[i] }
+
+type spacesByName struct {
+	spacesSlice
+}
+type spacesByLastModifiedDateTime struct {
+	spacesSlice
+}
+
+// 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 *s.spacesSlice[i].Name > *s.spacesSlice[j].Name
+}
+
+// Less reports whether the element with index i
+// must sort before the element with index j.
+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)
+	}
+	// move left item down if it has no value
+	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
+	}
+	// fallback to name if no dateTime is set on both items
+	return *s.spacesSlice[i].Name > *s.spacesSlice[j].Name
+}

sonar-project.properties

@@ -38,3 +38,4 @@ sonar.go.golangci-lint.reportPaths=cache/checkstyle/accounts_checkstyle.xml,cach
 # Exclude files
 sonar.exclusions=**/third_party,docs/**,changelog/**,*/pkg/assets/embed.go,idp/assets/identifier/**,**/package.json,**/rollup.config.js,CHANGELOG.md,**/pkg/proto/**/*.pb.*,deployments/**,tests/**,vendor-bin/**,README.md,**/mocks/
 sonar.coverage.exclusions=**/*_test.go
+sonar.cpd.exclusions=**/*_test.go