From 51825cfffe84d20df60abf84436bd6dcdabffb09 Mon Sep 17 00:00:00 2001 From: Will Assis <35489495+gassiss@users.noreply.github.com> Date: Thu, 27 Mar 2025 17:01:07 -0300 Subject: [PATCH] unified-storage: Reduce calls to the user service (#102934) * Create ListByIdOrUID in user service * create UnstructuredToLegacyFolderList * update GetFolders to use list parser * update GetDescendants to use list parser * update UnstructuredToLegacyFolder to also make a single call to the user service --------- Co-authored-by: Stephanie Hingtgen --- pkg/services/folder/folderimpl/conversions.go | 150 +++++++-- .../folder/folderimpl/conversions_test.go | 300 +++++++++++++++++- .../folder/folderimpl/unifiedstore.go | 27 +- .../folder/folderimpl/unifiedstore_test.go | 7 +- pkg/services/user/user.go | 1 + pkg/services/user/userimpl/store.go | 20 ++ pkg/services/user/userimpl/store_test.go | 108 ++++++- pkg/services/user/userimpl/user.go | 13 + pkg/services/user/userimpl/user_test.go | 5 + pkg/services/user/usertest/fake.go | 31 +- pkg/services/user/usertest/mock.go | 29 ++ pkg/util/xorm/session_cond.go | 8 +- pkg/util/xorm/statement.go | 7 + 13 files changed, 643 insertions(+), 63 deletions(-) diff --git a/pkg/services/folder/folderimpl/conversions.go b/pkg/services/folder/folderimpl/conversions.go index 404a78b9e03..f00afcf855d 100644 --- a/pkg/services/folder/folderimpl/conversions.go +++ b/pkg/services/folder/folderimpl/conversions.go @@ -2,7 +2,7 @@ package folderimpl import ( "context" - "errors" + "fmt" "strconv" "strings" @@ -13,13 +13,12 @@ import ( "github.com/grafana/grafana/pkg/infra/slugify" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/folder" - "github.com/grafana/grafana/pkg/services/user" ) -func (ss *FolderUnifiedStoreImpl) UnstructuredToLegacyFolder(ctx context.Context, item *unstructured.Unstructured) (*folder.Folder, error) { +func parseUnstructuredToLegacyFolder(item *unstructured.Unstructured) (*folder.Folder, string, string, error) { meta, err := utils.MetaAccessor(item) if err != nil { - return nil, err + return nil, "", "", err } info, _ := authlib.ParseNamespace(meta.GetNamespace()) @@ -46,16 +45,9 @@ func (ss *FolderUnifiedStoreImpl) UnstructuredToLegacyFolder(ctx context.Context updated = &tmp } - creator, err := ss.getUserFromMeta(ctx, meta.GetCreatedBy()) - if err != nil { - return nil, err - } - - updater, err := ss.getUserFromMeta(ctx, meta.GetUpdatedBy()) - if err != nil { - return nil, err - } - if updater.UID == "" { + creator := meta.GetCreatedBy() + updater := meta.GetUpdatedBy() + if updater == "" { updater = creator } @@ -75,34 +67,132 @@ func (ss *FolderUnifiedStoreImpl) UnstructuredToLegacyFolder(ctx context.Context Created: created, Updated: *updated, OrgID: info.OrgID, - CreatedBy: creator.ID, - UpdatedBy: updater.ID, - }, nil + }, creator, updater, nil } -func (ss *FolderUnifiedStoreImpl) getUserFromMeta(ctx context.Context, userMeta string) (*user.User, error) { - if userMeta == "" || toUID(userMeta) == "" { - return &user.User{}, nil +func (ss *FolderUnifiedStoreImpl) UnstructuredToLegacyFolder(ctx context.Context, item *unstructured.Unstructured) (*folder.Folder, error) { + folder, creatorRaw, updaterRaw, err := parseUnstructuredToLegacyFolder(item) + if err != nil { + return nil, err } - usr, err := ss.getUser(ctx, toUID(userMeta)) - if err != nil && errors.Is(err, user.ErrUserNotFound) { - return &user.User{}, nil + + userUIDtoIDmapping, err := ss.getUserUIDtoIDmappingFromIdentifiers(ctx, []string{creatorRaw, updaterRaw}) + if err != nil { + return nil, err } - return usr, err + + creatorId := getIdFromMapping(creatorRaw, userUIDtoIDmapping) + updaterId := getIdFromMapping(updaterRaw, userUIDtoIDmapping) + + if updaterId == 0 { + updaterId = creatorId + } + + folder.CreatedBy = creatorId + folder.UpdatedBy = updaterId + + return folder, nil } -func (ss *FolderUnifiedStoreImpl) getUser(ctx context.Context, uid string) (*user.User, error) { - userID, err := strconv.ParseInt(uid, 10, 64) - if err == nil { - return ss.userService.GetByID(ctx, &user.GetUserByIDQuery{ID: userID}) +func (ss *FolderUnifiedStoreImpl) UnstructuredToLegacyFolderList(ctx context.Context, unstructuredList *unstructured.UnstructuredList) ([]*folder.Folder, error) { + folders := make([]*folder.Folder, 0) + identifiers := make([]string, 0) + for _, item := range unstructuredList.Items { + meta, err := utils.MetaAccessor(&item) + if err != nil { + return nil, fmt.Errorf("unable to convert unstructured item to legacy folder %w", err) + } + + identifiers = append(identifiers, meta.GetCreatedBy(), meta.GetUpdatedBy()) } - return ss.userService.GetByUID(ctx, &user.GetUserByUIDQuery{UID: uid}) + + userUIDtoIDmapping, err := ss.getUserUIDtoIDmappingFromIdentifiers(ctx, identifiers) + if err != nil { + return nil, err + } + + for _, item := range unstructuredList.Items { + folder, creatorRaw, updaterRaw, err := parseUnstructuredToLegacyFolder(&item) + if err != nil { + return nil, err + } + + creatorId := getIdFromMapping(creatorRaw, userUIDtoIDmapping) + updaterId := getIdFromMapping(updaterRaw, userUIDtoIDmapping) + + if updaterId == 0 { + updaterId = creatorId + } + + folder.CreatedBy = creatorId + folder.UpdatedBy = updaterId + folders = append(folders, folder) + } + + return folders, nil } -func toUID(rawIdentifier string) string { +func (ss *FolderUnifiedStoreImpl) getUserUIDtoIDmappingFromIdentifiers(ctx context.Context, rawIdentifiers []string) (map[string]int64, error) { + userUIDs, userIds := parseIdentifiers(rawIdentifiers) + allUsers, err := ss.userService.ListByIdOrUID(ctx, userUIDs, userIds) + if err != nil { + return nil, err + } + + mapping := make(map[string]int64) + for _, user := range allUsers { + mapping[user.UID] = user.ID + } + + return mapping, nil +} + +func getIdentifier(rawIdentifier string) string { parts := strings.Split(rawIdentifier, ":") if len(parts) < 2 { return "" } + if parts[0] != "user" { + return "" + } return parts[1] } + +func parseIdentifiers(rawIdentifiers []string) ([]string, []int64) { + uids := make([]string, 0) + ids := make([]int64, 0) + for _, rawIdentifier := range rawIdentifiers { + identifier := getIdentifier(rawIdentifier) + if identifier == "" { + continue + } + + id, err := strconv.ParseInt(identifier, 10, 64) + if err == nil { + ids = append(ids, id) + } else if identifier != "" { + uids = append(uids, identifier) + } + } + + return uids, ids +} + +func getIdFromMapping(rawIdentifier string, mapping map[string]int64) int64 { + identifier := getIdentifier(rawIdentifier) + if identifier == "" { + return 0 + } + + id, err := strconv.ParseInt(identifier, 10, 64) + if err == nil { + return id + } + + uid, ok := mapping[identifier] + if ok { + return uid + } + + return 0 +} diff --git a/pkg/services/folder/folderimpl/conversions_test.go b/pkg/services/folder/folderimpl/conversions_test.go index 537193d5a15..9e18c6e0050 100644 --- a/pkg/services/folder/folderimpl/conversions_test.go +++ b/pkg/services/folder/folderimpl/conversions_test.go @@ -34,7 +34,7 @@ func TestFolderConversions(t *testing.T) { "grafana.app/updatedTimestamp": "2022-12-02T07:02:02Z", "grafana.app/repoName": "example-repo", "grafana.app/createdBy": "user:useruid", - "grafana.app/updatedBy": "user:useruid" + "grafana.app/updatedBy": "user:2" } }, "spec": { @@ -49,12 +49,23 @@ func TestFolderConversions(t *testing.T) { require.NoError(t, err) fake := usertest.NewUserServiceFake() - fake.ExpectedUser = &user.User{ID: 10, UID: "useruid"} + fake.ExpectedListUsersByIdOrUid = []*user.User{ + { + ID: 1, + UID: "useruid", + }, + { + ID: 2, + UID: "useruid2", + }, + } fs := ProvideUnifiedStore(nil, fake) converted, err := fs.UnstructuredToLegacyFolder(context.Background(), input) require.NoError(t, err) + require.Equal(t, 1, len(fake.ListUsersByIdOrUidCalls)) // only one call to the user service + require.Equal(t, usertest.ListUsersByIdOrUidCall{Uids: []string{"useruid"}, Ids: []int64{2}}, fake.ListUsersByIdOrUidCalls[0]) require.Equal(t, folder.Folder{ ID: 234, OrgID: 1, @@ -67,7 +78,288 @@ func TestFolderConversions(t *testing.T) { ManagedBy: utils.ManagerKindRepo, Created: created, Updated: created.Add(time.Hour * 5), - CreatedBy: 10, - UpdatedBy: 10, + CreatedBy: 1, + UpdatedBy: 2, }, *converted) } + +func TestFolderListConversions(t *testing.T) { + input := &unstructured.UnstructuredList{} + err := input.UnmarshalJSON([]byte(`{ + "apiVersion": "folder.grafana.app/v0alpha1", + "items": [ + { + "apiVersion": "folder.grafana.app/v0alpha1", + "kind": "Folder", + "metadata": { + "annotations": { + "grafana.app/createdBy": "access-policy:service" + }, + "creationTimestamp": "2022-12-02T02:02:02Z", + "generation": 1, + "labels": { + "grafana.app/deprecatedInternalID": "4", + "grafana.app/fullpath": "somefullpath", + "grafana.app/fullpathUIDs": "somefullpathuids" + }, + "name": "foldername1", + "namespace": "default", + "resourceVersion": "1741881243564020", + "uid": "dd669a77-7872-4c96-8fc7-899a5d8fdb6c" + }, + "spec": { + "title": "gdev dashboards", + "description": "description gdev" + } + }, + { + "apiVersion": "folder.grafana.app/v0alpha1", + "kind": "Folder", + "metadata": { + "annotations": { + "grafana.app/createdBy": "user:uuuuuuuuuuuuuu" + }, + "creationTimestamp": "2022-12-02T02:02:02Z", + "generation": 1, + "labels": { + "grafana.app/deprecatedInternalID": "149" + }, + "name": "foldername2", + "namespace": "default", + "resourceVersion": "1742998826046994", + "uid": "941bcef6-1579-48b0-9b25-7cc227910aae" + }, + "spec": { + "title": "yeye", + "description": "description yeye" + } + }, + { + "apiVersion": "folder.grafana.app/v0alpha1", + "kind": "Folder", + "metadata": { + "annotations": { + "grafana.app/createdBy": "user:iiiiiiiiiiiiii", + "grafana.app/updatedBy": "user:jjjjjjjjjjjjjj" + }, + "creationTimestamp": "2022-12-02T02:02:02Z", + "generation": 1, + "labels": { + "grafana.app/deprecatedInternalID": "145" + }, + "name": "foldername3", + "namespace": "default", + "resourceVersion": "1743003591477996", + "uid": "96083f1d-8501-425a-bbeb-c1c4ef1e9985" + }, + "spec": { + "title": "yoyo", + "description": "description yoyo" + } + }, + { + "apiVersion": "folder.grafana.app/v0alpha1", + "kind": "Folder", + "metadata": { + "annotations": { + "grafana.app/createdBy": "user:1" + }, + "creationTimestamp": "2022-12-02T02:02:02Z", + "generation": 1, + "labels": { + "grafana.app/deprecatedInternalID": "146" + }, + "name": "foldername4", + "namespace": "default", + "resourceVersion": "1742998754624006", + "uid": "f42e5c38-8ad3-43ee-813e-575592ac88b2" + }, + "spec": { + "title": "yaya", + "description": "description yaya" + } + }, + { + "apiVersion": "folder.grafana.app/v0alpha1", + "kind": "Folder", + "metadata": { + "annotations": { + "grafana.app/createdBy": "user:2", + "grafana.app/updatedBy": "user:3" + }, + "creationTimestamp": "2022-12-02T02:02:02Z", + "generation": 1, + "labels": { + "grafana.app/deprecatedInternalID": "147" + }, + "name": "foldername5", + "namespace": "default", + "resourceVersion": "1742317244002993", + "uid": "b52ef257-2fed-4edc-a559-a1f68c46e75c" + }, + "spec": { + "title": "yiyi", + "description": "description yiyi" + } + }, + { + "apiVersion": "folder.grafana.app/v0alpha1", + "kind": "Folder", + "metadata": { + "annotations": {}, + "creationTimestamp": "2022-12-02T02:02:02Z", + "generation": 1, + "labels": { + "grafana.app/deprecatedInternalID": "148" + }, + "name": "foldername6", + "namespace": "default", + "resourceVersion": "1742998544679983", + "uid": "771747f4-d93c-4b4d-b5be-db287e559c64" + }, + "spec": { + "title": "yuyu", + "description": "description yuyu" + } + } + ], + "kind": "FolderList", + "metadata": { + "resourceVersion": "1743003591477997" + } + }`)) + + require.NoError(t, err) + + created, err := time.Parse(time.RFC3339, "2022-12-02T02:02:02Z") + created = created.UTC() + require.NoError(t, err) + + fake := usertest.NewUserServiceFake() + fake.ExpectedListUsersByIdOrUid = []*user.User{ + { + ID: 1, + UID: "aaaaaaaaaaaaaa", + }, + { + ID: 2, + UID: "oooooooooooooo", + }, + { + ID: 3, + UID: "eeeeeeeeeeeeee", + }, + { + ID: 4, + UID: "uuuuuuuuuuuuuu", + }, + { + ID: 5, + UID: "iiiiiiiiiiiiii", + }, + { + ID: 6, + UID: "jjjjjjjjjjjjjj", + }, + } + + fs := ProvideUnifiedStore(nil, fake) + + converted, err := fs.UnstructuredToLegacyFolderList(context.Background(), input) + require.NoError(t, err) + require.Equal(t, 1, len(fake.ListUsersByIdOrUidCalls)) // only one call to the user service + require.Equal(t, usertest.ListUsersByIdOrUidCall{Uids: []string{"uuuuuuuuuuuuuu", "iiiiiiiiiiiiii", "jjjjjjjjjjjjjj"}, Ids: []int64{1, 2, 3}}, fake.ListUsersByIdOrUidCalls[0]) + require.Equal(t, 6, len(converted)) + require.Equal(t, []*folder.Folder{ + { + ID: 4, + OrgID: 1, + Version: 1, + UID: "foldername1", + ParentUID: "", + Title: "gdev dashboards", + Description: "description gdev", + URL: "/dashboards/f/foldername1/gdev-dashboards", + ManagedBy: "", + Created: created, + Updated: created, + CreatedBy: 0, // service account + UpdatedBy: 0, // service account, + }, + { + ID: 149, + OrgID: 1, + Version: 1, + UID: "foldername2", + ParentUID: "", + Title: "yeye", + Description: "description yeye", + URL: "/dashboards/f/foldername2/yeye", + ManagedBy: "", + Created: created, + Updated: created, + CreatedBy: 4, + UpdatedBy: 4, + }, + { + ID: 145, + OrgID: 1, + Version: 1, + UID: "foldername3", + ParentUID: "", + Title: "yoyo", + Description: "description yoyo", + URL: "/dashboards/f/foldername3/yoyo", + ManagedBy: "", + Created: created, + Updated: created, + CreatedBy: 5, + UpdatedBy: 6, + }, + { + ID: 146, + OrgID: 1, + Version: 1, + UID: "foldername4", + ParentUID: "", + Title: "yaya", + Description: "description yaya", + URL: "/dashboards/f/foldername4/yaya", + ManagedBy: "", + Created: created, + Updated: created, + CreatedBy: 1, + UpdatedBy: 1, + }, + { + ID: 147, + OrgID: 1, + Version: 1, + UID: "foldername5", + ParentUID: "", + Title: "yiyi", + Description: "description yiyi", + URL: "/dashboards/f/foldername5/yiyi", + ManagedBy: "", + Created: created, + Updated: created, + CreatedBy: 2, + UpdatedBy: 3, + }, + { + ID: 148, + OrgID: 1, + Version: 1, + UID: "foldername6", + ParentUID: "", + Title: "yuyu", + Description: "description yuyu", + URL: "/dashboards/f/foldername6/yuyu", + ManagedBy: "", + Created: created, + Updated: created, + CreatedBy: 0, // no createdby + UpdatedBy: 0, + }, + }, converted) +} diff --git a/pkg/services/folder/folderimpl/unifiedstore.go b/pkg/services/folder/folderimpl/unifiedstore.go index 1e80c2afb57..4118e586b14 100644 --- a/pkg/services/folder/folderimpl/unifiedstore.go +++ b/pkg/services/folder/folderimpl/unifiedstore.go @@ -319,15 +319,14 @@ func (ss *FolderUnifiedStoreImpl) GetFolders(ctx context.Context, q folder.GetFo if err != nil { return nil, err } + // convert item to legacy folder format + folders, err := ss.UnstructuredToLegacyFolderList(ctx, out) + if err != nil { + return nil, err + } m := map[string]*folder.Folder{} - for _, item := range out.Items { - // convert item to legacy folder format - f, err := ss.UnstructuredToLegacyFolder(ctx, &item) - if f == nil { - return nil, fmt.Errorf("unable to convert unstructured item to legacy folder %w", err) - } - + for _, f := range folders { if (q.WithFullpath || q.WithFullpathUIDs) && f.Fullpath == "" { parents, err := ss.GetParents(ctx, folder.GetParentsQuery{UID: f.UID, OrgID: q.OrgID}) if err != nil { @@ -375,14 +374,14 @@ func (ss *FolderUnifiedStoreImpl) GetDescendants(ctx context.Context, orgID int6 return nil, err } - nodes := map[string]*folder.Folder{} - for _, item := range out.Items { - // convert item to legacy folder format - f, err := ss.UnstructuredToLegacyFolder(ctx, &item) - if f == nil { - return nil, fmt.Errorf("unable to convert unstructured item to legacy folder %w", err) - } + // convert item to legacy folder format + folders, err := ss.UnstructuredToLegacyFolderList(ctx, out) + if err != nil { + return nil, err + } + nodes := map[string]*folder.Folder{} + for _, f := range folders { nodes[f.UID] = f } diff --git a/pkg/services/folder/folderimpl/unifiedstore_test.go b/pkg/services/folder/folderimpl/unifiedstore_test.go index 6c51829f671..0641586d7ca 100644 --- a/pkg/services/folder/folderimpl/unifiedstore_test.go +++ b/pkg/services/folder/folderimpl/unifiedstore_test.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/services/apiserver/client" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/folder" + "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/storage/unified/resource" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -93,7 +94,8 @@ func TestComputeFullPath(t *testing.T) { func TestGetParents(t *testing.T) { mockCli := new(client.MockK8sHandler) store := FolderUnifiedStoreImpl{ - k8sclient: mockCli, + k8sclient: mockCli, + userService: usertest.NewUserServiceFake(), } ctx := context.Background() @@ -187,7 +189,8 @@ func TestGetParents(t *testing.T) { func TestGetChildren(t *testing.T) { mockCli := new(client.MockK8sHandler) store := FolderUnifiedStoreImpl{ - k8sclient: mockCli, + k8sclient: mockCli, + userService: usertest.NewUserServiceFake(), } ctx := context.Background() diff --git a/pkg/services/user/user.go b/pkg/services/user/user.go index 2b7712a52f1..3a10ac0f349 100644 --- a/pkg/services/user/user.go +++ b/pkg/services/user/user.go @@ -16,6 +16,7 @@ type Service interface { GetByID(context.Context, *GetUserByIDQuery) (*User, error) // GetByUID returns a user by UID. This also includes service accounts (identity use only) GetByUID(context.Context, *GetUserByUIDQuery) (*User, error) + ListByIdOrUID(context.Context, []string, []int64) ([]*User, error) GetByLogin(context.Context, *GetUserByLoginQuery) (*User, error) GetByEmail(context.Context, *GetUserByEmailQuery) (*User, error) Update(context.Context, *UpdateUserCommand) error diff --git a/pkg/services/user/userimpl/store.go b/pkg/services/user/userimpl/store.go index eb6b5a68227..a5216b04e7a 100644 --- a/pkg/services/user/userimpl/store.go +++ b/pkg/services/user/userimpl/store.go @@ -25,6 +25,7 @@ type store interface { Insert(context.Context, *user.User) (int64, error) GetByID(context.Context, int64) (*user.User, error) GetByUID(ctx context.Context, uid string) (*user.User, error) + ListByIdOrUID(ctx context.Context, uids []string, ids []int64) ([]*user.User, error) GetByLogin(context.Context, *user.GetUserByLoginQuery) (*user.User, error) GetByEmail(context.Context, *user.GetUserByEmailQuery) (*user.User, error) Delete(context.Context, int64) error @@ -127,6 +128,25 @@ func (ss *sqlStore) GetByUID(ctx context.Context, uid string) (*user.User, error return &usr, err } +func (ss *sqlStore) ListByIdOrUID(ctx context.Context, uids []string, ids []int64) ([]*user.User, error) { + users := make([]*user.User, 0) + + err := ss.db.WithDbSession(ctx, func(sess *db.Session) error { + err := sess.Table("user").In("uid", uids).OrIn("id", ids).Find(&users) + if err != nil { + return err + } + + return nil + }) + + if err != nil { + return nil, err + } + + return users, err +} + func (ss *sqlStore) notServiceAccountFilter() string { return fmt.Sprintf("%s.is_service_account = %s", ss.dialect.Quote("user"), diff --git a/pkg/services/user/userimpl/store_test.go b/pkg/services/user/userimpl/store_test.go index bca7ac13a49..ade6366a895 100644 --- a/pkg/services/user/userimpl/store_test.go +++ b/pkg/services/user/userimpl/store_test.go @@ -3,9 +3,12 @@ package userimpl import ( "context" "fmt" + "sort" "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -848,6 +851,105 @@ func TestIntegrationUserDataAccess(t *testing.T) { assert.Equal(t, queryResult.OrgName, "user1@test.com") assert.Equal(t, queryResult.IsGrafanaAdmin, false) }) + + t.Run("Can get users by UID list", func(t *testing.T) { + users := createFiveTestUsers(t, usrSvc, func(i int) *user.CreateUserCommand { + return &user.CreateUserCommand{ + Email: fmt.Sprint("USERLISTUIDTEST", i, "@test.com"), + Name: fmt.Sprint("USERLISTUIDTEST", i), + Login: fmt.Sprint("loginUSERLISTUIDTEST", i), + IsDisabled: false, + } + }) + + sort.Slice(users, func(i, j int) bool { + return users[i].ID < users[j].ID + }) + + alluids := make([]string, 0, 5) + for _, user := range users { + alluids = append(alluids, user.UID) + } + + resultOnlyUIDs, err := userStore.ListByIdOrUID(context.Background(), alluids, []int64{}) + require.NoError(t, err) + + sort.Slice(resultOnlyUIDs, func(i, j int) bool { + return resultOnlyUIDs[i].ID < resultOnlyUIDs[j].ID + }) + require.Equal(t, len(resultOnlyUIDs), len(users)) + ignoreTimeFields := cmpopts.IgnoreFields(user.User{}, "Created", "Updated", "LastSeenAt") + if diff := cmp.Diff(users, resultOnlyUIDs, ignoreTimeFields); diff != "" { + t.Errorf("structs don't match (-want +got):\n%s", diff) + } + }) + + t.Run("Can get users by ID list", func(t *testing.T) { + users := createFiveTestUsers(t, usrSvc, func(i int) *user.CreateUserCommand { + return &user.CreateUserCommand{ + Email: fmt.Sprint("USERLISTIDTEST", i, "@test.com"), + Name: fmt.Sprint("USERLISTIDTEST", i), + Login: fmt.Sprint("loginUSERLISTIDTEST", i), + IsDisabled: false, + } + }) + + sort.Slice(users, func(i, j int) bool { + return users[i].ID < users[j].ID + }) + + allids := make([]int64, 0, 5) + for _, user := range users { + allids = append(allids, user.ID) + } + + resultOnlyIDs, err := userStore.ListByIdOrUID(context.Background(), []string{}, allids) + require.NoError(t, err) + + sort.Slice(resultOnlyIDs, func(i, j int) bool { + return resultOnlyIDs[i].ID < resultOnlyIDs[j].ID + }) + ignoreTimeFields := cmpopts.IgnoreFields(user.User{}, "Created", "Updated", "LastSeenAt") + if diff := cmp.Diff(users, resultOnlyIDs, ignoreTimeFields); diff != "" { + t.Errorf("structs don't match (-want +got):\n%s", diff) + } + }) + + t.Run("Can get users by UID and ID list", func(t *testing.T) { + users := createFiveTestUsers(t, usrSvc, func(i int) *user.CreateUserCommand { + return &user.CreateUserCommand{ + Email: fmt.Sprint("USERLISTUIDANDIDTEST", i, "@test.com"), + Name: fmt.Sprint("USERLISTUIDANDIDTEST", i), + Login: fmt.Sprint("loginUSERLISTUIDANDIDTEST", i), + IsDisabled: false, + } + }) + + sort.Slice(users, func(i, j int) bool { + return users[i].ID < users[j].ID + }) + + ids := make([]int64, 0, 2) + uids := make([]string, 0, 3) + for i, user := range users { + if i < 2 { + ids = append(ids, user.ID) + } else { + uids = append(uids, user.UID) + } + } + + resultOnlyIDs, err := userStore.ListByIdOrUID(context.Background(), uids, ids) + require.NoError(t, err) + + sort.Slice(resultOnlyIDs, func(i, j int) bool { + return resultOnlyIDs[i].ID < resultOnlyIDs[j].ID + }) + ignoreTimeFields := cmpopts.IgnoreFields(user.User{}, "Created", "Updated", "LastSeenAt") + if diff := cmp.Diff(users, resultOnlyIDs, ignoreTimeFields); diff != "" { + t.Errorf("structs don't match (-want +got):\n%s", diff) + } + }) } func TestIntegrationUserUpdate(t *testing.T) { @@ -905,15 +1007,15 @@ func TestIntegrationUserUpdate(t *testing.T) { }) } -func createFiveTestUsers(t *testing.T, svc user.Service, fn func(i int) *user.CreateUserCommand) []user.User { +func createFiveTestUsers(t *testing.T, svc user.Service, fn func(i int) *user.CreateUserCommand) []*user.User { t.Helper() - users := make([]user.User, 5) + users := make([]*user.User, 5) for i := 0; i < 5; i++ { cmd := fn(i) user, err := svc.Create(context.Background(), cmd) require.Nil(t, err) - users[i] = *user + users[i] = user } return users diff --git a/pkg/services/user/userimpl/user.go b/pkg/services/user/userimpl/user.go index 55b3b809d23..9a19eca19e3 100644 --- a/pkg/services/user/userimpl/user.go +++ b/pkg/services/user/userimpl/user.go @@ -235,6 +235,19 @@ func (s *Service) GetByUID(ctx context.Context, query *user.GetUserByUIDQuery) ( return s.store.GetByUID(ctx, query.UID) } +func (s *Service) ListByIdOrUID(ctx context.Context, uids []string, ids []int64) ([]*user.User, error) { + if len(uids) == 0 && len(ids) == 0 { + return []*user.User{}, nil + } + ctx, span := s.tracer.Start(ctx, "user.ListByIdOrUID", trace.WithAttributes( + attribute.StringSlice("userUIDs", uids), + attribute.Int64Slice("userIDs", ids), + )) + defer span.End() + + return s.store.ListByIdOrUID(ctx, uids, ids) +} + func (s *Service) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) { ctx, span := s.tracer.Start(ctx, "user.GetByLogin") defer span.End() diff --git a/pkg/services/user/userimpl/user_test.go b/pkg/services/user/userimpl/user_test.go index 9e24f9c2ffd..c71431718de 100644 --- a/pkg/services/user/userimpl/user_test.go +++ b/pkg/services/user/userimpl/user_test.go @@ -274,6 +274,7 @@ type FakeUserStore struct { ExpectedError error ExpectedDeleteUserError error ExpectedCountUserAccountsWithEmptyRoles int64 + ExpectedListUsersByIdOrUid []*user.User } func newUserStoreFake() *FakeUserStore { @@ -296,6 +297,10 @@ func (f *FakeUserStore) GetByUID(context.Context, string) (*user.User, error) { return f.ExpectedUser, f.ExpectedError } +func (f *FakeUserStore) ListByIdOrUID(context.Context, []string, []int64) ([]*user.User, error) { + return f.ExpectedListUsersByIdOrUid, f.ExpectedError +} + func (f *FakeUserStore) LoginConflict(context.Context, string, string) error { return f.ExpectedError } diff --git a/pkg/services/user/usertest/fake.go b/pkg/services/user/usertest/fake.go index d0552d16d0f..3687ead9f8f 100644 --- a/pkg/services/user/usertest/fake.go +++ b/pkg/services/user/usertest/fake.go @@ -6,16 +6,22 @@ import ( "github.com/grafana/grafana/pkg/services/user" ) +type ListUsersByIdOrUidCall struct { + Uids []string + Ids []int64 +} + type FakeUserService struct { - ExpectedUser *user.User - ExpectedSignedInUser *user.SignedInUser - ExpectedError error - ExpectedSetUsingOrgError error - ExpectedSearchUsers user.SearchUserQueryResult - ExpectedListUsers user.ListUserResult - ExpectedUserProfileDTO *user.UserProfileDTO - ExpectedUserProfileDTOs []*user.UserProfileDTO - ExpectedUsageStats map[string]any + ExpectedUser *user.User + ExpectedSignedInUser *user.SignedInUser + ExpectedError error + ExpectedSetUsingOrgError error + ExpectedSearchUsers user.SearchUserQueryResult + ExpectedListUsers user.ListUserResult + ExpectedListUsersByIdOrUid []*user.User + ExpectedUserProfileDTO *user.UserProfileDTO + ExpectedUserProfileDTOs []*user.UserProfileDTO + ExpectedUsageStats map[string]any UpdateFn func(ctx context.Context, cmd *user.UpdateUserCommand) error GetSignedInUserFn func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error) @@ -25,6 +31,8 @@ type FakeUserService struct { GetByEmailFn func(ctx context.Context, query *user.GetUserByEmailQuery) (*user.User, error) counter int + + ListUsersByIdOrUidCalls []ListUsersByIdOrUidCall } func NewUserServiceFake() *FakeUserService { @@ -59,6 +67,11 @@ func (f *FakeUserService) GetByUID(ctx context.Context, query *user.GetUserByUID return f.ExpectedUser, f.ExpectedError } +func (f *FakeUserService) ListByIdOrUID(ctx context.Context, uids []string, ids []int64) ([]*user.User, error) { + f.ListUsersByIdOrUidCalls = append(f.ListUsersByIdOrUidCalls, ListUsersByIdOrUidCall{Uids: uids, Ids: ids}) + return f.ExpectedListUsersByIdOrUid, f.ExpectedError +} + func (f *FakeUserService) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) { if f.GetByLoginFn != nil { return f.GetByLoginFn(ctx, query) diff --git a/pkg/services/user/usertest/mock.go b/pkg/services/user/usertest/mock.go index 348d1040997..943b1520fb1 100644 --- a/pkg/services/user/usertest/mock.go +++ b/pkg/services/user/usertest/mock.go @@ -230,6 +230,35 @@ func (_m *MockService) GetByUID(_a0 context.Context, _a1 *user.GetUserByUIDQuery return r0, r1 } +func (_m *MockService) ListByIdOrUID(_a0 context.Context, _a1 []string, _a2 []int64) ([]*user.User, error) { + ret := _m.Called(_a0, _a1, _a2) + + if len(ret) == 0 { + panic("no return value specified for ListByIdOrUID") + } + + var r0 []*user.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, []string, []int64) ([]*user.User, error)); ok { + return rf(_a0, _a1, _a2) + } + if rf, ok := ret.Get(0).(func(context.Context, []string, []int64) []*user.User); ok { + r0 = rf(_a0, _a1, _a2) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*user.User) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, []string, []int64) error); ok { + r1 = rf(_a0, _a1, _a2) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GetProfile provides a mock function with given fields: _a0, _a1 func (_m *MockService) GetProfile(_a0 context.Context, _a1 *user.GetUserProfileQuery) (*user.UserProfileDTO, error) { ret := _m.Called(_a0, _a1) diff --git a/pkg/util/xorm/session_cond.go b/pkg/util/xorm/session_cond.go index a0510436ef9..d3d14b3dce5 100644 --- a/pkg/util/xorm/session_cond.go +++ b/pkg/util/xorm/session_cond.go @@ -52,12 +52,18 @@ func (session *Session) ID(id any) *Session { return session } -// In provides a query string like "id in (1, 2, 3)" +// In provides a query string like "id in (1, 2, 3)" using the AND conditional func (session *Session) In(column string, args ...any) *Session { session.statement.In(column, args...) return session } +// OrIn provides a query string like "id in (1, 2, 3)" using the OR conditional +func (session *Session) OrIn(column string, args ...any) *Session { + session.statement.OrIn(column, args...) + return session +} + // NotIn provides a query string like "id in (1, 2, 3)" func (session *Session) NotIn(column string, args ...any) *Session { session.statement.NotIn(column, args...) diff --git a/pkg/util/xorm/statement.go b/pkg/util/xorm/statement.go index a2ec2fd7aea..b1cefb4a0dd 100644 --- a/pkg/util/xorm/statement.go +++ b/pkg/util/xorm/statement.go @@ -201,6 +201,13 @@ func (statement *Statement) In(column string, args ...any) *Statement { return statement } +// OrIn generate "Where column IN (?) " statement +func (statement *Statement) OrIn(column string, args ...any) *Statement { + in := builder.In(statement.Engine.Quote(column), args...) + statement.cond = statement.cond.Or(in) + return statement +} + // NotIn generate "Where column NOT IN (?) " statement func (statement *Statement) NotIn(column string, args ...any) *Statement { notIn := builder.NotIn(statement.Engine.Quote(column), args...)