Skip to content

Commit

Permalink
graph/users: small ooptimization for filter by role queries
Browse files Browse the repository at this point in the history
When filtering by role assignment we now populate the
'appRoleAssignments' property of the returned users when the incoming
request contained and '$expand=appRoleAssignments'. This avoids having
to query the role assignments for all users in the result set a second
time.
The effect of this optimization depends a lot on the actual setup. In
single binary all-in-one installations it is rather small (this it saves
quite a few CPU cycles). In distributed setup it should be larger as it
reduces the number of network round trips to the settings service
significantly.
  • Loading branch information
rhafer committed Jun 5, 2024
1 parent 69ab738 commit 53dc2ed
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 5 deletions.
74 changes: 74 additions & 0 deletions services/graph/mocks/role_service.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion services/graph/pkg/service/v0/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) {
expandAppRoleAssignments := slices.Contains(exp, "appRoleAssignments")
expandMemberOf := slices.Contains(exp, "memberOf")
for _, u := range users {
if expandAppRoleAssignments {
if expandAppRoleAssignments && u.AppRoleAssignments == nil {
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
Expand Down
26 changes: 22 additions & 4 deletions services/graph/pkg/service/v0/users_filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func (g Graph) applyFilterLogicalAnd(ctx context.Context, req *godata.GoDataRequ
return []*libregraph.User{}, err
}
logger.Debug().Str("property", property).Str("value", value).Msg("applying approleAssignments filter on result set")
return g.filterUsersByAppRoleID(ctx, value, res2)
return g.filterUsersByAppRoleID(ctx, req, value, res2)
}

// 1st part is no appRoleAssignmentFilter, run the filter query
Expand All @@ -172,7 +172,7 @@ func (g Graph) applyFilterLogicalAnd(ctx context.Context, req *godata.GoDataRequ
return users, unsupportedFilterError()
}
logger.Debug().Str("property", property).Str("value", value).Msg("applying approleAssignments filter on result set")
return g.filterUsersByAppRoleID(ctx, value, res1)
return g.filterUsersByAppRoleID(ctx, req, value, res1)
}

// 2nd part is no appRoleAssignmentFilter either
Expand Down Expand Up @@ -329,22 +329,40 @@ func (g Graph) applyAppRoleAssignmentEq(ctx context.Context, req *godata.GoDataR
return users, err
}

return g.filterUsersByAppRoleID(ctx, filterValue, users)
return g.filterUsersByAppRoleID(ctx, req, filterValue, users)
}
return users, unsupportedFilterError()
}

func (g Graph) filterUsersByAppRoleID(ctx context.Context, id string, users []*libregraph.User) ([]*libregraph.User, error) {
func (g Graph) filterUsersByAppRoleID(ctx context.Context, req *godata.GoDataRequest, id string, users []*libregraph.User) ([]*libregraph.User, error) {
// We're using a map for the results here, in order to avoid returning
// a user twice. The settings API, still has an issue that causes it to
// duplicate some assignments on restart:
// https://github.com/owncloud/ocis/issues/3432

var expand bool
exp := req.Query.GetExpand()
if exp != nil {
for _, item := range exp.ExpandItems {
if item.Path[0].Value == "appRoleAssignments" {
expand = true
break
}
}
}

resultUsersMap := make(map[string]*libregraph.User, len(users))
for _, user := range users {
assignments, err := g.fetchAppRoleAssignments(ctx, user.GetId())
if err != nil {
return users, err
}
// To avoid re-fetching all assignments in the upper layer handler when
// the $expand query parameter is set, we're adding the assignments to the
// resulting users here already.
if expand {
user.AppRoleAssignments = assignments
}
for _, assignment := range assignments {
if assignment.GetAppRoleId() == id {
if _, ok := resultUsersMap[user.GetId()]; !ok {
Expand Down

0 comments on commit 53dc2ed

Please sign in to comment.