Repository: owncloud/ocis
Pull request #5421 (2023-01-18 13:41:12)

Populate expanded properties
We now return an empty array when an expanded relation has no entries. This makes consuming the responses a little easier.

fixes https://github.com/owncloud/ocis/issues/5419

update-docs[bot]: Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a [changelog](https://github.com/owncloud/ocis/blob/master/changelog/README.md) item based on your changes.

sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=owncloud_ocis&pullRequest=5421)

[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=BUG)  
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=VULNERABILITY)  
[![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=owncloud_ocis&pullRequest=5421&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=owncloud_ocis&pullRequest=5421&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=owncloud_ocis&pullRequest=5421&resolved=false&types=SECURITY_HOTSPOT)  
[![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=5421&resolved=false&types=CODE_SMELL)

[![80.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60-16px.png '80.0%')](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=5421&metric=new_coverage&view=list) [80.0% Coverage](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=5421&metric=new_coverage&view=list)  
[![3.6%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/5-16px.png '3.6%')](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=5421&metric=new_duplicated_lines_density&view=list) [3.6% Duplication](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=5421&metric=new_duplicated_lines_density&view=list)



Patch:
diff --git a/changelog/unreleased/populate-expanded-properties.md b/changelog/unreleased/populate-expanded-properties.md
new file mode 100644
index 00000000000..b3ea06a4357
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go
index a7a34ec932d..ade4a9e2d04 100644
--- a/services/graph/pkg/identity/ldap.go
+++ b/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)
 	}
diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go
index 49a0d1e6399..70027ed1f6b 100644
--- a/services/graph/pkg/identity/ldap_group.go
+++ b/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)
diff --git a/services/graph/pkg/service/v0/drives.go b/services/graph/pkg/service/v0/drives.go
index c106d2ed781..29a4e27ad8b 100644
--- a/services/graph/pkg/service/v0/drives.go
+++ b/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) {
diff --git a/services/graph/pkg/service/v0/educationuser.go b/services/graph/pkg/service/v0/educationuser.go
index 8a5cdb3c724..1ce9dae2a24 100644
--- a/services/graph/pkg/service/v0/educationuser.go
+++ b/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
diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go
index 201d64af177..c0bddc77752 100644
--- a/services/graph/pkg/service/v0/users.go
+++ b/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)


Test patch:
diff --git a/services/graph/pkg/service/v0/drives_test.go b/services/graph/pkg/service/v0/drives_test.go
index 28bcb7cf56e..9abacf8403d 100644
--- a/services/graph/pkg/service/v0/drives_test.go
+++ b/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
diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go
index 82a896c282b..e7e6d86fe5e 100644
--- a/services/graph/pkg/service/v0/graph_test.go
+++ b/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",