From 53dc2edbb1c1028e578ae47bd819cff005ea77e9 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 5 Jun 2024 11:30:24 +0200 Subject: [PATCH] graph/users: small ooptimization for filter by role queries 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. --- services/graph/mocks/role_service.go | 74 +++++++++++++++++++ services/graph/pkg/service/v0/users.go | 2 +- services/graph/pkg/service/v0/users_filter.go | 26 ++++++- 3 files changed, 97 insertions(+), 5 deletions(-) diff --git a/services/graph/mocks/role_service.go b/services/graph/mocks/role_service.go index 4538a943030..d9e19c16e7e 100644 --- a/services/graph/mocks/role_service.go +++ b/services/graph/mocks/role_service.go @@ -175,6 +175,80 @@ func (_c *RoleService_ListRoleAssignments_Call) RunAndReturn(run func(context.Co return _c } +// ListRoleAssignmentsFiltered provides a mock function with given fields: ctx, in, opts +func (_m *RoleService) ListRoleAssignmentsFiltered(ctx context.Context, in *v0.ListRoleAssignmentsFilteredRequest, opts ...client.CallOption) (*v0.ListRoleAssignmentsResponse, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, in) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for ListRoleAssignmentsFiltered") + } + + var r0 *v0.ListRoleAssignmentsResponse + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *v0.ListRoleAssignmentsFilteredRequest, ...client.CallOption) (*v0.ListRoleAssignmentsResponse, error)); ok { + return rf(ctx, in, opts...) + } + if rf, ok := ret.Get(0).(func(context.Context, *v0.ListRoleAssignmentsFilteredRequest, ...client.CallOption) *v0.ListRoleAssignmentsResponse); ok { + r0 = rf(ctx, in, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.ListRoleAssignmentsResponse) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *v0.ListRoleAssignmentsFilteredRequest, ...client.CallOption) error); ok { + r1 = rf(ctx, in, opts...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// RoleService_ListRoleAssignmentsFiltered_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ListRoleAssignmentsFiltered' +type RoleService_ListRoleAssignmentsFiltered_Call struct { + *mock.Call +} + +// ListRoleAssignmentsFiltered is a helper method to define mock.On call +// - ctx context.Context +// - in *v0.ListRoleAssignmentsFilteredRequest +// - opts ...client.CallOption +func (_e *RoleService_Expecter) ListRoleAssignmentsFiltered(ctx interface{}, in interface{}, opts ...interface{}) *RoleService_ListRoleAssignmentsFiltered_Call { + return &RoleService_ListRoleAssignmentsFiltered_Call{Call: _e.mock.On("ListRoleAssignmentsFiltered", + append([]interface{}{ctx, in}, opts...)...)} +} + +func (_c *RoleService_ListRoleAssignmentsFiltered_Call) Run(run func(ctx context.Context, in *v0.ListRoleAssignmentsFilteredRequest, opts ...client.CallOption)) *RoleService_ListRoleAssignmentsFiltered_Call { + _c.Call.Run(func(args mock.Arguments) { + variadicArgs := make([]client.CallOption, len(args)-2) + for i, a := range args[2:] { + if a != nil { + variadicArgs[i] = a.(client.CallOption) + } + } + run(args[0].(context.Context), args[1].(*v0.ListRoleAssignmentsFilteredRequest), variadicArgs...) + }) + return _c +} + +func (_c *RoleService_ListRoleAssignmentsFiltered_Call) Return(_a0 *v0.ListRoleAssignmentsResponse, _a1 error) *RoleService_ListRoleAssignmentsFiltered_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *RoleService_ListRoleAssignmentsFiltered_Call) RunAndReturn(run func(context.Context, *v0.ListRoleAssignmentsFilteredRequest, ...client.CallOption) (*v0.ListRoleAssignmentsResponse, error)) *RoleService_ListRoleAssignmentsFiltered_Call { + _c.Call.Return(run) + return _c +} + // ListRoles provides a mock function with given fields: ctx, in, opts func (_m *RoleService) ListRoles(ctx context.Context, in *v0.ListBundlesRequest, opts ...client.CallOption) (*v0.ListBundlesResponse, error) { _va := make([]interface{}, len(opts)) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 258dc847d0f..9a15803662d 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -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 diff --git a/services/graph/pkg/service/v0/users_filter.go b/services/graph/pkg/service/v0/users_filter.go index fd3768b27b9..bf0d025b02f 100644 --- a/services/graph/pkg/service/v0/users_filter.go +++ b/services/graph/pkg/service/v0/users_filter.go @@ -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 @@ -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 @@ -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 {