Populate expanded properties (#5421)

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

changelog/unreleased/populate-expanded-properties.md

@@ -0,0 +1,6 @@
+Bugfix: Populate expanded properties
+
+We now return an empty array when an expanded relation has no entries. This makes consuming the responses a little easier.
+
+https://github.com/owncloud/ocis/pull/5421
+https://github.com/owncloud/ocis/issues/5419

services/graph/pkg/identity/ldap.go

@@ -387,10 +387,7 @@ func (i *LDAP) GetUser(ctx context.Context, nameOrID string, queryParam url.Valu
 		if err != nil {
 			return nil, err
 		}
-		if len(userGroups) > 0 {
-			groups := i.groupsFromLDAPEntries(userGroups)
-			u.MemberOf = groups
-		}
+		u.MemberOf = i.groupsFromLDAPEntries(userGroups)
 	}
 	return u, nil
 }
@@ -454,14 +451,7 @@ func (i *LDAP) GetUsers(ctx context.Context, queryParam url.Values) ([]*libregra
 			if err != nil {
 				return nil, err
 			}
-			if len(userGroups) > 0 {
-				expand := ldap.EscapeFilter(queryParam.Get("expand"))
-				if expand == "" {
-					expand = "false"
-				}
-				groups := i.groupsFromLDAPEntries(userGroups)
-				u.MemberOf = groups
-			}
+			u.MemberOf = i.groupsFromLDAPEntries(userGroups)
 		}
 		users = append(users, u)
 	}

services/graph/pkg/identity/ldap_group.go

@@ -41,14 +41,13 @@ func (i *LDAP) GetGroup(ctx context.Context, nameOrID string, queryParam url.Val
 		if err != nil {
 			return nil, err
 		}
+		g.Members = make([]libregraph.User, 0, len(members))
 		if len(members) > 0 {
-			m := make([]libregraph.User, 0, len(members))
 			for _, ue := range members {
 				if u := i.createUserModelFromLDAP(ue); u != nil {
-					m = append(m, *u)
+					g.Members = append(g.Members, *u)
 				}
 			}
-			g.Members = m
 		}
 	}
 	return g, nil
@@ -120,14 +119,13 @@ func (i *LDAP) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregr
 			if err != nil {
 				return nil, err
 			}
+			g.Members = make([]libregraph.User, 0, len(members))
 			if len(members) > 0 {
-				m := make([]libregraph.User, 0, len(members))
 				for _, ue := range members {
 					if u := i.createUserModelFromLDAP(ue); u != nil {
-						m = append(m, *u)
+						g.Members = append(g.Members, *u)
 					}
 				}
-				g.Members = m
 			}
 		}
 		groups = append(groups, g)

services/graph/pkg/service/v0/drives.go

@@ -478,8 +478,10 @@ func (g Graph) formatDrives(ctx context.Context, baseURL *url.URL, storageSpaces
 		// can't access disabled space
 		if utils.ReadPlainFromOpaque(storageSpace.Opaque, "trashed") != "trashed" {
 			res.Special = g.getExtendedSpaceProperties(ctx, baseURL, storageSpace)
-			res.Quota, err = g.getDriveQuota(ctx, storageSpace)
+			quota, err := g.getDriveQuota(ctx, storageSpace)
+			res.Quota = &quota
 			if err != nil {
+				//logger.Debug().Err(err).Interface("id", sp.Id).Msg("error calling get quota on drive")
 				return nil, err
 			}
 		}
@@ -718,6 +720,8 @@ func (g Graph) cs3StorageSpaceToDrive(ctx context.Context, baseURL *url.URL, spa
 		lastModified := cs3TimestampToTime(space.Mtime)
 		drive.LastModifiedDateTime = &lastModified
 	}
+
+	drive.Quota = &libregraph.Quota{}
 	if space.Quota != nil {
 		var t int64
 		if space.Quota.QuotaMaxBytes > math.MaxInt64 {
@@ -725,15 +729,13 @@ func (g Graph) cs3StorageSpaceToDrive(ctx context.Context, baseURL *url.URL, spa
 		} else {
 			t = int64(space.Quota.QuotaMaxBytes)
 		}
-		drive.Quota = &libregraph.Quota{
-			Total: &t,
-		}
+		drive.Quota.Total = &t
 	}
 
 	return drive, nil
 }
 
-func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.StorageSpace) (*libregraph.Quota, error) {
+func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.StorageSpace) (libregraph.Quota, error) {
 	logger := g.logger.SubloggerWithRequestID(ctx)
 	client := g.GetGatewayClient()
 
@@ -747,13 +749,13 @@ func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.Storage
 	switch {
 	case err != nil:
 		logger.Error().Err(err).Interface("ref", req.Ref).Msg("could not call GetQuota: transport error")
-		return nil, nil
+		return libregraph.Quota{}, nil
 	case res.GetStatus().GetCode() == cs3rpc.Code_CODE_UNIMPLEMENTED:
 		logger.Debug().Msg("get quota is not implemented on the storage driver")
-		return nil, nil
+		return libregraph.Quota{}, nil
 	case res.GetStatus().GetCode() != cs3rpc.Code_CODE_OK:
 		logger.Debug().Str("grpc", res.GetStatus().GetMessage()).Msg("error sending get quota grpc request")
-		return nil, errors.New(res.GetStatus().GetMessage())
+		return libregraph.Quota{}, errors.New(res.GetStatus().GetMessage())
 	}
 
 	var remaining int64
@@ -783,7 +785,7 @@ func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.Storage
 	state := calculateQuotaState(t, used)
 	qta.State = &state
 
-	return &qta, nil
+	return qta, nil
 }
 
 func calculateQuotaState(total int64, used int64) (state string) {

services/graph/pkg/service/v0/drives_test.go

@@ -111,7 +111,7 @@ var sortTests = []sortTest{
 }
 
 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: name, LastModifiedDateTime: lastModified}
+	return &libregraph.Drive{Id: libregraph.PtrString(ID), DriveType: libregraph.PtrString(dType), Name: name, LastModifiedDateTime: lastModified, Quota: &libregraph.Quota{}}
 }
 
 // TestSort tests the available orderby queries

services/graph/pkg/service/v0/educationuser.go

@@ -257,7 +257,7 @@ func (g Graph) GetEducationUser(w http.ResponseWriter, r *http.Request) {
 			if err != nil {
 				logger.Debug().Err(err).Interface("id", sp.Id).Msg("error calling get quota on drive")
 			}
-			d.Quota = quota
+			d.Quota = &quota
 			if slices.Contains(sel, "drive") || slices.Contains(exp, "drive") {
 				if *d.DriveType == _spaceTypePersonal {
 					user.Drive = d

services/graph/pkg/service/v0/graph_test.go

@@ -143,6 +143,7 @@ var _ = Describe("Graph", func() {
 						"driveType":"aspacetype",
 						"id":"pro-1$sameID",
 						"name":"aspacename",
+						"quota": {},
 						"root":{
 							"id":"pro-1$sameID",
 							"webDavUrl":"https://localhost:9200/dav/spaces/pro-1$sameID"
@@ -213,6 +214,7 @@ var _ = Describe("Graph", func() {
 						"driveType":"aspacetype",
 						"id":"pro-1$asameID",
 						"name":"aspacename",
+						"quota": {},
 						"root":{
 							"eTag":"101112131415",
 							"id":"pro-1$asameID",
@@ -225,6 +227,7 @@ var _ = Describe("Graph", func() {
 						"driveType":"bspacetype",
 						"id":"pro-1$bsameID",
 						"name":"bspacename",
+						"quota": {},
 						"root":{
 							"eTag":"123456789",
 							"id":"pro-1$bsameID",

services/graph/pkg/service/v0/users.go

@@ -1,6 +1,7 @@
 package svc
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -57,13 +58,15 @@ func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) {
 			}
 			return
 		}
+		if me.MemberOf == nil {
+			me.MemberOf = []libregraph.Group{}
+		}
 	}
 
 	// expand appRoleAssignments if requested
 	if slices.Contains(exp, "appRoleAssignments") {
-		lrar, err := g.roleService.ListRoleAssignments(r.Context(), &settings.ListRoleAssignmentsRequest{
-			AccountUuid: me.GetId(),
-		})
+		var err error
+		me.AppRoleAssignments, err = g.fetchAppRoleAssignments(r.Context(), me.GetId())
 		if err != nil {
 			logger.Debug().Err(err).Str("userid", me.GetId()).Msg("could not get appRoleAssignments for self")
 			var errcode errorcode.Error
@@ -74,18 +77,27 @@ func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) {
 			}
 			return
 		}
-
-		values := make([]libregraph.AppRoleAssignment, 0, len(lrar.GetAssignments()))
-		for _, assignment := range lrar.GetAssignments() {
-			values = append(values, g.assignmentToAppRoleAssignment(assignment))
-		}
-		me.AppRoleAssignments = values
 	}
 
 	render.Status(r, http.StatusOK)
 	render.JSON(w, r, me)
 }
 
+func (g Graph) fetchAppRoleAssignments(ctx context.Context, accountuuid string) ([]libregraph.AppRoleAssignment, error) {
+	lrar, err := g.roleService.ListRoleAssignments(ctx, &settings.ListRoleAssignmentsRequest{
+		AccountUuid: accountuuid,
+	})
+	if err != nil {
+		return []libregraph.AppRoleAssignment{}, err
+	}
+
+	values := make([]libregraph.AppRoleAssignment, 0, len(lrar.Assignments))
+	for _, assignment := range lrar.GetAssignments() {
+		values = append(values, g.assignmentToAppRoleAssignment(assignment))
+	}
+	return values, nil
+}
+
 // GetUsers implements the Service interface.
 func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) {
 	logger := g.logger.SubloggerWithRequestID(r.Context())
@@ -113,21 +125,21 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) {
 
 	// expand appRoleAssignments if requested
 	exp := strings.Split(r.URL.Query().Get("$expand"), ",")
-	if slices.Contains(exp, "appRoleAssignments") {
-		for _, u := range users {
-			lrar, err := g.roleService.ListRoleAssignments(r.Context(), &settings.ListRoleAssignmentsRequest{
-				AccountUuid: u.GetId(),
-			})
+	expandAppRoleAssignments := slices.Contains(exp, "appRoleAssignments")
+	expandMemberOf := slices.Contains(exp, "memberOf")
+	for _, u := range users {
+		if expandAppRoleAssignments {
+			u.AppRoleAssignments, err = g.fetchAppRoleAssignments(r.Context(), u.GetId())
 			if err != nil {
+				// TODO I think we should not continue here, see http://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part1-protocol.html#sec_SystemQueryOptionexpand
+				// > The $expand system query option indicates the related entities and stream values that MUST be represented inline. The service MUST return the specified content, and MAY choose to return additional information.
 				logger.Debug().Err(err).Str("userid", u.GetId()).Msg("could not get appRoleAssignments when listing user")
-				continue
 			}
-
-			values := make([]libregraph.AppRoleAssignment, 0, len(lrar.GetAssignments()))
-			for _, assignment := range lrar.GetAssignments() {
-				values = append(values, g.assignmentToAppRoleAssignment(assignment))
+		}
+		if expandMemberOf {
+			if u.MemberOf == nil {
+				u.MemberOf = []libregraph.Group{}
 			}
-			u.AppRoleAssignments = values
 		}
 	}
 
@@ -307,7 +319,7 @@ func (g Graph) GetUser(w http.ResponseWriter, r *http.Request) {
 			if err != nil {
 				logger.Debug().Err(err).Interface("id", sp.Id).Msg("error calling get quota on drive")
 			}
-			d.Quota = quota
+			d.Quota = &quota
 			if slices.Contains(sel, "drive") || slices.Contains(exp, "drive") {
 				if *d.DriveType == "personal" {
 					user.Drive = d
@@ -320,10 +332,7 @@ func (g Graph) GetUser(w http.ResponseWriter, r *http.Request) {
 	}
 	// expand appRoleAssignments if requested
 	if slices.Contains(exp, "appRoleAssignments") {
-
-		lrar, err := g.roleService.ListRoleAssignments(r.Context(), &settings.ListRoleAssignmentsRequest{
-			AccountUuid: user.GetId(),
-		})
+		user.AppRoleAssignments, err = g.fetchAppRoleAssignments(r.Context(), user.GetId())
 		if err != nil {
 			logger.Debug().Err(err).Str("userid", user.GetId()).Msg("could not get appRoleAssignments for user")
 			var errcode errorcode.Error
@@ -334,12 +343,6 @@ func (g Graph) GetUser(w http.ResponseWriter, r *http.Request) {
 			}
 			return
 		}
-
-		values := make([]libregraph.AppRoleAssignment, 0, len(lrar.GetAssignments()))
-		for _, assignment := range lrar.GetAssignments() {
-			values = append(values, g.assignmentToAppRoleAssignment(assignment))
-		}
-		user.AppRoleAssignments = values
 	}
 
 	render.Status(r, http.StatusOK)