diff --git a/kv/user.go b/kv/user.go index 705a4ec5e08..f4c4fd63f31 100644 --- a/kv/user.go +++ b/kv/user.go @@ -354,7 +354,7 @@ func (s *Service) updateUser(ctx context.Context, tx Tx, id influxdb.ID, upd inf } if upd.Name != nil { - if err := s.removeUserFromIndex(ctx, tx, id, *upd.Name); err != nil { + if err := s.removeUserFromIndex(ctx, tx, id, u.Name); err != nil { return nil, err } diff --git a/tenant/error_user.go b/tenant/error_user.go index 79192f35d13..c8b4aa8e24b 100644 --- a/tenant/error_user.go +++ b/tenant/error_user.go @@ -8,6 +8,8 @@ import ( var ( // ErrUserNotFound is used when the user is not found. + // TODO(brett): Transition to using this error once the tenant service is + // universally available. ErrUserNotFound = &influxdb.Error{ Msg: "user not found", Code: influxdb.ENotFound, diff --git a/tenant/service_user.go b/tenant/service_user.go index eb3699736d5..e4c70aa43a5 100644 --- a/tenant/service_user.go +++ b/tenant/service_user.go @@ -31,7 +31,7 @@ func (s *Service) FindUserByID(ctx context.Context, id influxdb.ID) (*influxdb.U func (s *Service) FindUser(ctx context.Context, filter influxdb.UserFilter) (*influxdb.User, error) { // if im given no filters its not a valid find user request. (leaving it unchecked seems dangerous) if filter.ID == nil && filter.Name == nil { - return nil, ErrUserNotFound + return nil, kv.ErrUserNotFound } if filter.ID != nil { diff --git a/tenant/storage_user.go b/tenant/storage_user.go index 6ea11c89500..5738ec656e3 100644 --- a/tenant/storage_user.go +++ b/tenant/storage_user.go @@ -68,7 +68,7 @@ func (s *Store) GetUser(ctx context.Context, tx kv.Tx, id influxdb.ID) (*influxd v, err := b.Get(encodedID) if kv.IsNotFound(err) { - return nil, ErrUserNotFound + return nil, kv.ErrUserNotFound } if err != nil { @@ -86,7 +86,7 @@ func (s *Store) GetUserByName(ctx context.Context, tx kv.Tx, n string) (*influxd uid, err := b.Get([]byte(n)) if err == kv.ErrKeyNotFound { - return nil, ErrUserNotFound + return nil, kv.ErrUserNotFound } if err != nil { diff --git a/tenant/storage_user_test.go b/tenant/storage_user_test.go index 5e95616c2ae..b8468e65a00 100644 --- a/tenant/storage_user_test.go +++ b/tenant/storage_user_test.go @@ -90,11 +90,11 @@ func TestUser(t *testing.T) { t.Fatalf("expected identical user: \n%+v\n%+v", user, expected) } - if _, err := store.GetUser(context.Background(), tx, 500); err != tenant.ErrUserNotFound { + if _, err := store.GetUser(context.Background(), tx, 500); err != kv.ErrUserNotFound { t.Fatal("failed to get correct error when looking for invalid user by id") } - if _, err := store.GetUserByName(context.Background(), tx, "notauser"); err != tenant.ErrUserNotFound { + if _, err := store.GetUserByName(context.Background(), tx, "notauser"); err != kv.ErrUserNotFound { t.Fatal("failed to get correct error when looking for invalid user by name") } @@ -207,7 +207,7 @@ func TestUser(t *testing.T) { } err = store.DeleteUser(context.Background(), tx, 1) - if err != tenant.ErrUserNotFound { + if err != kv.ErrUserNotFound { t.Fatal("invalid error when deleting user that has already been deleted", err) } diff --git a/testing/user_service.go b/testing/user_service.go index 56e382cf147..fea892c9942 100644 --- a/testing/user_service.go +++ b/testing/user_service.go @@ -3,12 +3,14 @@ package testing import ( "bytes" "context" + "errors" "sort" "testing" "github.com/google/go-cmp/cmp" "github.com/influxdata/influxdb/v2" platform "github.com/influxdata/influxdb/v2" + "github.com/influxdata/influxdb/v2/kv" "github.com/influxdata/influxdb/v2/mock" ) @@ -79,6 +81,10 @@ func UserService( name: "UpdateUser", fn: UpdateUser, }, + { + name: "UpdateUser_IndexHygiene", + fn: UpdateUser_IndexHygiene, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -977,3 +983,51 @@ func UpdateUser( }) } } + +func UpdateUser_IndexHygiene( + init func(UserFields, *testing.T) (platform.UserService, string, func()), + t *testing.T, +) { + + oldUserName := "user1" + users := UserFields{ + Users: []*platform.User{ + { + ID: MustIDBase16(userOneID), + Name: oldUserName, + Status: "active", + }, + }, + } + s, _, done := init(users, t) + defer done() + + newUserName := "user1Updated" + upd := platform.UserUpdate{ + Name: &newUserName, + } + + ctx := context.Background() + _, err := s.UpdateUser(ctx, MustIDBase16(userOneID), upd) + if err != nil { + t.Error(err) + } + + // Ensure we can find the user with the new name. + _, nerr := s.FindUser(ctx, platform.UserFilter{ + Name: &newUserName, + }) + if nerr != nil { + t.Error("unexpected error when finding user by name", nerr) + } + + // Ensure we cannot find a user with the old name. The index used when + // searching by name should have been cleared out by the UpdateUser + // operation. + _, oerr := s.FindUser(ctx, platform.UserFilter{ + Name: &oldUserName, + }) + if !errors.Is(oerr, kv.ErrUserNotFound) { + t.Errorf("expected %q error, but got %v", kv.ErrUserNotFound, oerr) + } +}