diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 41fbf5d..a88050d 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -10,6 +10,9 @@ jobs: runs-on: ubuntu-latest container: image: golang:1.27-bookworm + volumes: + - go-mod-cache:/go/pkg/mod + - go-build-cache:/root/.cache/go-build steps: - name: Install system dependencies run: | @@ -20,9 +23,6 @@ jobs: - name: Checkout uses: actions/checkout@v4 - - name: Lint - run: make lint - - name: Check formatting run: | unformatted=$(gofmt -l .) @@ -32,6 +32,12 @@ jobs: exit 1 fi + - name: Download modules + run: go mod download + + - name: Lint + run: make lint + - name: Test run: make test diff --git a/internal/api/handler/list.go b/internal/api/handler/list.go index c103db6..af697ee 100644 --- a/internal/api/handler/list.go +++ b/internal/api/handler/list.go @@ -1,6 +1,7 @@ package handler import ( + "errors" "log/slog" "net/http" @@ -232,6 +233,52 @@ func (h *ListHandler) RemoveUserFromList(w http.ResponseWriter, r *http.Request) response.Status(w, http.StatusNoContent) } +// OrderLists handles re-ordering a users lists +// +// @Summary Order lists of a user +// @Description Re-assigns the display order of all users lists +// @Tags List +// @Accept json +// @Produce json +// @Param payload body request.OrderListsPayload true "Order lists payload" +// @Success 204 {object} nil +// @Error 400 {object} response.ErrorResponse "failed to decode request body" +// @Error 401 {object} response.ErrorResponse "not authorized" +// @Error 500 {object} response.ErrorResponse "failed to order lists" +// @Router /lists/order [post] +func (h *ListHandler) OrderLists(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + + payload, err := request.DecodeJSON[request.OrderListsPayload](r) + if err != nil { + slog.ErrorContext(ctx, "failed to decode order lists payload", slog.Any("error", err)) + response.Error(ctx, w, http.StatusBadRequest, "failed to decode request body") + return + } + + authContext, err := domain.GetAuthContext(ctx) + if err != nil { + slog.ErrorContext(ctx, "failed to get auth context", slog.Any("error", err)) + response.Error(ctx, w, http.StatusUnauthorized, "not authorized") + return + } + + err = h.ListService.OrderLists(ctx, authContext.UserID, payload.ListIDs) + if err != nil { + if errors.Is(err, domain.ErrStaleListIDs) { + response.Error(ctx, w, http.StatusBadRequest, "stale list IDs") + } else { + response.Error(ctx, w, http.StatusInternalServerError, "failed to order list items") + } + + slog.ErrorContext(ctx, "failed to order lists", slog.Any("error", err)) + return + } + + slog.InfoContext(ctx, "lists re-ordered successfully", slog.String("user_id", authContext.UserID)) + response.Status(w, http.StatusNoContent) +} + // CreateListItem handles creation of a new list item on a given list // // @Summary Create list item diff --git a/internal/api/request/list.go b/internal/api/request/list.go index fcb5093..700934d 100644 --- a/internal/api/request/list.go +++ b/internal/api/request/list.go @@ -14,6 +14,10 @@ type RemoveUserFromListPayload struct { Email string `json:"email"` } +type OrderListsPayload struct { + ListIDs []string `json:"list_ids"` +} + type CreateListItemPayload struct { ListID string `json:"list_id"` Title string `json:"title"` diff --git a/internal/api/router/router.go b/internal/api/router/router.go index 31832be..967ca26 100644 --- a/internal/api/router/router.go +++ b/internal/api/router/router.go @@ -65,6 +65,7 @@ func NewMux(cfg Config) http.Handler { apiMux.Handle("GET /lists", protected(listHandler.GetLists)) apiMux.Handle("POST /lists/user", protected(listHandler.AddUserToList)) apiMux.Handle("DELETE /lists/user", protected(listHandler.RemoveUserFromList)) + apiMux.Handle("POST /lists/order", protected(listHandler.OrderLists)) apiMux.Handle("POST /lists/items", protected(listHandler.CreateListItem)) apiMux.Handle("DELETE /lists/items/{id}", protected(listHandler.DeleteListItem)) apiMux.Handle("PUT /lists/items", protected(listHandler.UpdateListItem)) diff --git a/internal/database/migrations/000006_add_list_position.down.sql b/internal/database/migrations/000006_add_list_position.down.sql new file mode 100644 index 0000000..9d63b03 --- /dev/null +++ b/internal/database/migrations/000006_add_list_position.down.sql @@ -0,0 +1,8 @@ +BEGIN; + +DROP INDEX IF EXISTS idx_list_users_position; + +ALTER TABLE IF EXISTS list_users + DROP COLUMN IF EXISTS position; + +COMMIT; diff --git a/internal/database/migrations/000006_add_list_position.up.sql b/internal/database/migrations/000006_add_list_position.up.sql new file mode 100644 index 0000000..f927ead --- /dev/null +++ b/internal/database/migrations/000006_add_list_position.up.sql @@ -0,0 +1,20 @@ +BEGIN; + +ALTER TABLE IF EXISTS list_users + ADD COLUMN IF NOT EXISTS position NOT NULL DEFAULT 0; + +CREATE INDEX IF NOT EXISTS idx_list_users_user_id_position ON list_users (user_id, position); + +UPDATE list_users lu + SET position = r.rn - 1 +FROM (SELECT list_id, + user_id, + row_number() OVER ( + PARTITION BY user_id + ORDER BY created_at, list_id + ) AS rn + FROM list_users) r +WHERE lu.list_id = r.list_id + AND lu.user_id = r.user_id; + +COMMIT; diff --git a/internal/domain/list.go b/internal/domain/list.go index 68bed6d..8e96094 100644 --- a/internal/domain/list.go +++ b/internal/domain/list.go @@ -4,6 +4,7 @@ import ( "context" "errors" "log/slog" + "strings" "time" ) @@ -13,6 +14,7 @@ var ( ErrListItemIDMissing = errors.New("list item id is required") ErrListItemTitleMissing = errors.New("list item title is required") ErrUserNotInList = errors.New("user not in list") + ErrStaleListIDs = errors.New("list ids out of date") ) type List struct { @@ -22,6 +24,7 @@ type List struct { ModifiedAt time.Time `json:"modified_at"` TotalItems int `json:"total_items"` CompletedItems int `json:"completed_items"` + Position int `json:"position"` } type ListItem struct { @@ -39,6 +42,8 @@ type ListRepository interface { GetLists(ctx context.Context, userID string) ([]List, error) AddUserToList(ctx context.Context, listID string, userID string) error RemoveUserFromList(ctx context.Context, listID string, userID string) error + OrderLists(ctx context.Context, userID string, listIDs []string) error + LockUsersLists(ctx context.Context, userID string) ([]string, error) IsUserInList(ctx context.Context, listID string, userID string) (bool, error) IsUserInListByItemID(ctx context.Context, listItemID string, userID string) (bool, error) CreateListItem(ctx context.Context, listID string, title string) (*ListItem, error) @@ -131,6 +136,36 @@ func (s *ListService) RemoveUserFromList(ctx context.Context, authUserID string, return s.repo.RemoveUserFromList(ctx, listID, userID) } +func (s *ListService) OrderLists(ctx context.Context, authUserID string, listIDs []string) error { + if authUserID == "" { + return ErrUserIDMissing + } + if len(listIDs) == 0 { + return ErrListIDMissing + } + + return s.tx.WithinTx(ctx, func(ctx context.Context) error { + serverIDs, err := s.repo.LockUsersLists(ctx, authUserID) + if err != nil { + return err + } + + if !isPermutation(listIDs, serverIDs) { + slog.ErrorContext(ctx, "no permutation", + slog.Any("client_list_ids", listIDs), + slog.Any("server_list_ids", serverIDs)) + return ErrStaleListIDs + } + + err = s.repo.OrderLists(ctx, authUserID, listIDs) + if err != nil { + return err + } + + return nil + }) +} + func (s *ListService) CreateListItem(ctx context.Context, authUserID string, listID string, title string) (*ListItem, error) { if listID == "" { return nil, ErrListIDMissing @@ -257,3 +292,28 @@ func (s *ListService) userAllowedToAccessListItem(ctx context.Context, authUserI return nil } + +func isPermutation(a []string, b []string) bool { + if len(a) != len(b) { + return false + } + + aMap := make(map[string]struct{}, len(a)) + for _, v := range a { + aMap[strings.ToLower(v)] = struct{}{} + } + + seen := make(map[string]struct{}, len(a)) + for _, v := range b { + id := strings.ToLower(v) + if _, ok := aMap[id]; !ok { + return false + } + if _, dup := seen[id]; dup { + return false + } + seen[id] = struct{}{} + } + + return true +} diff --git a/internal/domain/list_test.go b/internal/domain/list_test.go index eeee2cd..f232973 100644 --- a/internal/domain/list_test.go +++ b/internal/domain/list_test.go @@ -11,6 +11,12 @@ import ( "github.com/stretchr/testify/require" ) +var ( + _ Transactor = (*fakeTransactor)(nil) + _ ListRepository = (*mockListRepository)(nil) +) + +type txCtxKey struct{} type fakeTransactor struct { calls int err error @@ -21,9 +27,14 @@ func (f *fakeTransactor) WithinTx(ctx context.Context, fn func(ctx context.Conte if f.err != nil { return f.err } - return fn(ctx) + return fn(context.WithValue(ctx, txCtxKey{}, true)) } +var inTx = mock.MatchedBy(func(ctx context.Context) bool { + v, _ := ctx.Value(txCtxKey{}).(bool) + return v +}) + type mockListRepository struct { mock.Mock } @@ -55,6 +66,17 @@ func (m *mockListRepository) RemoveUserFromList(ctx context.Context, listID stri return args.Error(0) } +func (m *mockListRepository) OrderLists(ctx context.Context, userID string, listIDs []string) error { + args := m.Called(ctx, userID, listIDs) + return args.Error(0) +} + +func (m *mockListRepository) LockUsersLists(ctx context.Context, userID string) ([]string, error) { + args := m.Called(ctx, userID) + ids, _ := args.Get(0).([]string) + return ids, args.Error(1) +} + func (m *mockListRepository) IsUserInList(ctx context.Context, listID string, userID string) (bool, error) { args := m.Called(ctx, listID, userID) return args.Bool(0), args.Error(1) @@ -101,6 +123,7 @@ func newListService(t *testing.T) (*ListService, *mockListRepository, *fakeTrans t.Helper() repo := &mockListRepository{} + repo.Test(t) t.Cleanup(func() { repo.AssertExpectations(t) }) tx := &fakeTransactor{} @@ -108,224 +131,630 @@ func newListService(t *testing.T) (*ListService, *mockListRepository, *fakeTrans return NewListService(tx, repo), repo, tx } +func assertCallOrder(t *testing.T, repo *mockListRepository, want ...string) { + t.Helper() + + var got []string + for _, c := range repo.Calls { + got = append(got, c.Method) + } + assert.Equal(t, want, got) +} + func TestListService_CreateList(t *testing.T) { - t.Run("creates the list and adds the owner", func(t *testing.T) { + ctx := context.Background() + + t.Run("creates the list and adds the creator in one transaction", func(t *testing.T) { svc, repo, tx := newListService(t) created := &List{ID: "list-1", Name: "Groceries", CreatedAt: time.Now()} - repo.On("CreateList", mock.Anything, "Groceries").Return(created, nil) - repo.On("AddUserToList", mock.Anything, "list-1", "user-1").Return(nil) + repo.On("CreateList", inTx, "Groceries").Return(created, nil) + repo.On("AddUserToList", inTx, "list-1", "user-1").Return(nil) - list, err := svc.CreateList(context.Background(), "user-1", "Groceries") + list, err := svc.CreateList(ctx, "user-1", "Groceries") require.NoError(t, err) assert.Equal(t, created, list) assert.Equal(t, 1, tx.calls) - }) - - t.Run("empty name is rejected before any repo call", func(t *testing.T) { - svc, _, tx := newListService(t) - - list, err := svc.CreateList(context.Background(), "user-1", "") - - assert.Nil(t, list) - assert.ErrorIs(t, err, ErrListNameMissing) - assert.Zero(t, tx.calls) + assertCallOrder(t, repo, "CreateList", "AddUserToList") }) t.Run("insert error aborts before adding the user", func(t *testing.T) { svc, repo, _ := newListService(t) repoErr := errors.New("insert failed") - repo.On("CreateList", mock.Anything, "Groceries").Return(nil, repoErr) + repo.On("CreateList", inTx, "Groceries").Return(nil, repoErr) - list, err := svc.CreateList(context.Background(), "user-1", "Groceries") + list, err := svc.CreateList(ctx, "user-1", "Groceries") assert.Nil(t, list) assert.ErrorIs(t, err, repoErr) - repo.AssertNotCalled(t, "AddUserToList", mock.Anything, mock.Anything, mock.Anything) + assertCallOrder(t, repo, "CreateList") }) - t.Run("transaction error is returned", func(t *testing.T) { - svc, _, tx := newListService(t) + t.Run("membership insert error fails the whole operation", func(t *testing.T) { + svc, repo, _ := newListService(t) + + repoErr := errors.New("foreign key violation") + repo.On("CreateList", inTx, "Groceries").Return(&List{ID: "list-1", Name: "Groceries"}, nil) + repo.On("AddUserToList", inTx, "list-1", "user-1").Return(repoErr) + + list, err := svc.CreateList(ctx, "user-1", "Groceries") + + assert.Nil(t, list, "no half-created list may be returned") + assert.ErrorIs(t, err, repoErr) + }) + + t.Run("transaction begin error is returned", func(t *testing.T) { + svc, repo, tx := newListService(t) tx.err = errors.New("could not begin transaction") - list, err := svc.CreateList(context.Background(), "user-1", "Groceries") + list, err := svc.CreateList(ctx, "user-1", "Groceries") assert.Nil(t, list) assert.ErrorIs(t, err, tx.err) + assertCallOrder(t, repo) }) } func TestListService_DeleteList(t *testing.T) { - t.Run("deletes when the user is a member", func(t *testing.T) { - svc, repo, _ := newListService(t) + svc, repo, _ := newListService(t) - repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(true, nil) - repo.On("DeleteList", mock.Anything, "list-1").Return(nil) + repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(true, nil) + repo.On("DeleteList", mock.Anything, "list-1").Return(nil) - assert.NoError(t, svc.DeleteList(context.Background(), "user-1", "list-1")) - }) + err := svc.DeleteList(context.Background(), "user-1", "list-1") - t.Run("refuses when the user is not a member", func(t *testing.T) { - svc, repo, _ := newListService(t) - - repo.On("IsUserInList", mock.Anything, "list-1", "intruder").Return(false, nil) - - err := svc.DeleteList(context.Background(), "intruder", "list-1") - - assert.ErrorIs(t, err, ErrUserNotInList) - repo.AssertNotCalled(t, "DeleteList", mock.Anything, mock.Anything) - }) - - t.Run("membership check error is propagated", func(t *testing.T) { - svc, repo, _ := newListService(t) - - repoErr := errors.New("connection reset") - repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(false, repoErr) - - err := svc.DeleteList(context.Background(), "user-1", "list-1") - - assert.ErrorIs(t, err, repoErr) - assert.NotErrorIs(t, err, ErrUserNotInList) - repo.AssertNotCalled(t, "DeleteList", mock.Anything, mock.Anything) - }) + require.NoError(t, err) + assertCallOrder(t, repo, "IsUserInList", "DeleteList") } func TestListService_GetLists(t *testing.T) { + ctx := context.Background() + + t.Run("returns the user's lists", func(t *testing.T) { + svc, repo, _ := newListService(t) + + want := []List{{ID: "list-1", Name: "Groceries"}, {ID: "list-2", Name: "Reading"}} + repo.On("GetLists", mock.Anything, "user-1").Return(want, nil) + + lists, err := svc.GetLists(ctx, "user-1") + + require.NoError(t, err) + assert.Equal(t, want, lists) + }) + + t.Run("user without lists gets an empty result", func(t *testing.T) { + svc, repo, _ := newListService(t) + + repo.On("GetLists", mock.Anything, "user-1").Return([]List{}, nil) + + lists, err := svc.GetLists(ctx, "user-1") + + require.NoError(t, err) + assert.Empty(t, lists) + }) + + t.Run("repository error is propagated", func(t *testing.T) { + svc, repo, _ := newListService(t) + + repoErr := errors.New("connection reset") + repo.On("GetLists", mock.Anything, "user-1").Return(nil, repoErr) + + lists, err := svc.GetLists(ctx, "user-1") + + assert.Nil(t, lists) + assert.ErrorIs(t, err, repoErr) + }) +} + +func TestListService_AddUserToList(t *testing.T) { svc, repo, _ := newListService(t) - want := []List{{ID: "list-1", Name: "Groceries"}, {ID: "list-2", Name: "Reading"}} - repo.On("GetLists", mock.Anything, "user-1").Return(want, nil) + repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(true, nil) + repo.On("AddUserToList", mock.Anything, "list-1", "user-2").Return(nil) - lists, err := svc.GetLists(context.Background(), "user-1") + err := svc.AddUserToList(context.Background(), "user-1", "list-1", "user-2") require.NoError(t, err) - assert.Equal(t, want, lists) + assertCallOrder(t, repo, "IsUserInList", "AddUserToList") +} + +func TestListService_RemoveUserFromList(t *testing.T) { + ctx := context.Background() + + t.Run("member removes another member", func(t *testing.T) { + svc, repo, _ := newListService(t) + + repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(true, nil) + repo.On("RemoveUserFromList", mock.Anything, "list-1", "user-2").Return(nil) + + err := svc.RemoveUserFromList(ctx, "user-1", "list-1", "user-2") + + require.NoError(t, err) + assertCallOrder(t, repo, "IsUserInList", "RemoveUserFromList") + }) + + t.Run("member leaves the list", func(t *testing.T) { + svc, repo, _ := newListService(t) + + repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(true, nil) + repo.On("RemoveUserFromList", mock.Anything, "list-1", "user-1").Return(nil) + + err := svc.RemoveUserFromList(ctx, "user-1", "list-1", "user-1") + + require.NoError(t, err) + }) +} + +func TestListService_OrderLists(t *testing.T) { + ctx := context.Background() + + t.Run("stores the client's order inside one transaction", func(t *testing.T) { + svc, repo, tx := newListService(t) + + repo.On("LockUsersLists", inTx, "user-1").Return([]string{"list-a", "list-b", "list-c"}, nil) + repo.On("OrderLists", inTx, "user-1", []string{"list-c", "list-a", "list-b"}).Return(nil) + + err := svc.OrderLists(ctx, "user-1", []string{"list-c", "list-a", "list-b"}) + + require.NoError(t, err) + assert.Equal(t, 1, tx.calls) + assertCallOrder(t, repo, "LockUsersLists", "OrderLists") + }) + + t.Run("ids are compared case-insensitively", func(t *testing.T) { + svc, repo, _ := newListService(t) + + server := []string{ + "3f2a8c1e-7d4b-4e21-9a6f-2b1c0d9e8f7a", + "9b1d4e6f-0a2c-4b3d-8e5f-6a7b8c9d0e1f", + } + client := []string{ + "9B1D4E6F-0A2C-4B3D-8E5F-6A7B8C9D0E1F", + "3F2A8C1E-7D4B-4E21-9A6F-2B1C0D9E8F7A", + } + repo.On("LockUsersLists", inTx, "user-1").Return(server, nil) + // The client's spelling is passed on unchanged; Postgres' uuid cast + // doesn't care about case. + repo.On("OrderLists", inTx, "user-1", client).Return(nil) + + err := svc.OrderLists(ctx, "user-1", client) + + require.NoError(t, err) + }) + + t.Run("stale ids are rejected without writing", func(t *testing.T) { + tests := []struct { + name string + server []string + client []string + }{ + {name: "client is missing a list", server: []string{"a", "b", "c"}, client: []string{"a", "b"}}, + {name: "client has an extra list", server: []string{"a", "b"}, client: []string{"a", "b", "c"}}, + {name: "client has an unknown list", server: []string{"a", "b"}, client: []string{"a", "x"}}, + {name: "client repeats a list", server: []string{"a", "b"}, client: []string{"a", "a"}}, + {name: "user has no lists anymore", server: []string{}, client: []string{"a"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, repo, _ := newListService(t) + + repo.On("LockUsersLists", inTx, "user-1").Return(tt.server, nil) + + err := svc.OrderLists(ctx, "user-1", tt.client) + + assert.ErrorIs(t, err, ErrStaleListIDs) + assertCallOrder(t, repo, "LockUsersLists") + }) + } + }) + + t.Run("lock error is propagated", func(t *testing.T) { + svc, repo, _ := newListService(t) + + lockErr := errors.New("lock timeout") + repo.On("LockUsersLists", inTx, "user-1").Return(nil, lockErr) + + err := svc.OrderLists(ctx, "user-1", []string{"list-a"}) + + assert.ErrorIs(t, err, lockErr) + assert.NotErrorIs(t, err, ErrStaleListIDs) + assertCallOrder(t, repo, "LockUsersLists") + }) + + t.Run("update error is propagated", func(t *testing.T) { + svc, repo, _ := newListService(t) + + updateErr := errors.New("update failed") + repo.On("LockUsersLists", inTx, "user-1").Return([]string{"list-a"}, nil) + repo.On("OrderLists", inTx, "user-1", []string{"list-a"}).Return(updateErr) + + err := svc.OrderLists(ctx, "user-1", []string{"list-a"}) + + assert.ErrorIs(t, err, updateErr) + }) + + t.Run("transaction begin error is returned", func(t *testing.T) { + svc, repo, tx := newListService(t) + + tx.err = errors.New("could not begin transaction") + + err := svc.OrderLists(ctx, "user-1", []string{"list-a"}) + + assert.ErrorIs(t, err, tx.err) + assertCallOrder(t, repo) + }) } func TestListService_CreateListItem(t *testing.T) { - t.Run("creates the item for a member", func(t *testing.T) { - svc, repo, _ := newListService(t) + svc, repo, _ := newListService(t) - created := &ListItem{ID: "item-1", ListID: "list-1", Title: "Milk"} - repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(true, nil) - repo.On("CreateListItem", mock.Anything, "list-1", "Milk").Return(created, nil) + created := &ListItem{ID: "item-1", ListID: "list-1", Title: "Milk", CreatedAt: time.Now()} + repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(true, nil) + repo.On("CreateListItem", mock.Anything, "list-1", "Milk").Return(created, nil) - item, err := svc.CreateListItem(context.Background(), "user-1", "list-1", "Milk") + item, err := svc.CreateListItem(context.Background(), "user-1", "list-1", "Milk") - require.NoError(t, err) - assert.Equal(t, created, item) - }) + require.NoError(t, err) + assert.Equal(t, created, item) + assertCallOrder(t, repo, "IsUserInList", "CreateListItem") +} - t.Run("refuses for a non-member", func(t *testing.T) { - svc, repo, _ := newListService(t) +func TestListService_DeleteListItem(t *testing.T) { + svc, repo, _ := newListService(t) - repo.On("IsUserInList", mock.Anything, "list-1", "intruder").Return(false, nil) + repo.On("IsUserInListByItemID", mock.Anything, "item-1", "user-1").Return(true, nil) + repo.On("DeleteListItem", mock.Anything, "item-1").Return(nil) - item, err := svc.CreateListItem(context.Background(), "intruder", "list-1", "Milk") + err := svc.DeleteListItem(context.Background(), "user-1", "item-1") - assert.Nil(t, item) - assert.ErrorIs(t, err, ErrUserNotInList) - repo.AssertNotCalled(t, "CreateListItem", mock.Anything, mock.Anything, mock.Anything) - }) + require.NoError(t, err) + assertCallOrder(t, repo, "IsUserInListByItemID", "DeleteListItem") +} + +func TestListService_UpdateListItem(t *testing.T) { + svc, repo, _ := newListService(t) + + repo.On("IsUserInListByItemID", mock.Anything, "item-1", "user-1").Return(true, nil) + repo.On("UpdateListItem", mock.Anything, "item-1", "Oat milk", true).Return(nil) + + err := svc.UpdateListItem(context.Background(), "user-1", "item-1", "Oat milk", true) + + require.NoError(t, err) + assertCallOrder(t, repo, "IsUserInListByItemID", "UpdateListItem") +} + +func TestListService_SetListItemTitle(t *testing.T) { + svc, repo, _ := newListService(t) + + repo.On("IsUserInListByItemID", mock.Anything, "item-1", "user-1").Return(true, nil) + repo.On("SetListItemTitle", mock.Anything, "item-1", "Oat milk").Return(nil) + + err := svc.SetListItemTitle(context.Background(), "user-1", "item-1", "Oat milk") + + require.NoError(t, err) + assertCallOrder(t, repo, "IsUserInListByItemID", "SetListItemTitle") } func TestListService_SetListItemCompleted(t *testing.T) { - t.Run("checks membership through the item id", func(t *testing.T) { - svc, repo, _ := newListService(t) + tests := []struct { + name string + completed bool + }{ + {name: "marks the item as completed", completed: true}, + {name: "marks the item as open again", completed: false}, + } - repo.On("IsUserInListByItemID", mock.Anything, "item-1", "user-1").Return(true, nil) - repo.On("SetListItemCompleted", mock.Anything, "item-1", true).Return(nil) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, repo, _ := newListService(t) - assert.NoError(t, svc.SetListItemCompleted(context.Background(), "user-1", "item-1", true)) - }) + repo.On("IsUserInListByItemID", mock.Anything, "item-1", "user-1").Return(true, nil) + repo.On("SetListItemCompleted", mock.Anything, "item-1", tt.completed).Return(nil) - t.Run("refuses for a non-member", func(t *testing.T) { - svc, repo, _ := newListService(t) + err := svc.SetListItemCompleted(context.Background(), "user-1", "item-1", tt.completed) - repo.On("IsUserInListByItemID", mock.Anything, "item-1", "intruder").Return(false, nil) + require.NoError(t, err) + assertCallOrder(t, repo, "IsUserInListByItemID", "SetListItemCompleted") + }) + } +} - err := svc.SetListItemCompleted(context.Background(), "intruder", "item-1", false) +func TestListService_GetListItems(t *testing.T) { + svc, repo, _ := newListService(t) - assert.ErrorIs(t, err, ErrUserNotInList) - repo.AssertNotCalled(t, "SetListItemCompleted", mock.Anything, mock.Anything, mock.Anything) - }) + want := []ListItem{ + {ID: "item-1", ListID: "list-1", Title: "Milk"}, + {ID: "item-2", ListID: "list-1", Title: "Bread", IsCompleted: true}, + } + repo.On("IsUserInList", mock.Anything, "list-1", "user-1").Return(true, nil) + repo.On("GetListItems", mock.Anything, "list-1").Return(want, nil) + + items, err := svc.GetListItems(context.Background(), "user-1", "list-1") + + require.NoError(t, err) + assert.Equal(t, want, items) + assertCallOrder(t, repo, "IsUserInList", "GetListItems") +} + +type guardedOp struct { + name string + // byItem is true when membership is resolved through a list item id + // (IsUserInListByItemID) instead of a list id (IsUserInList). + byItem bool + // expectRepo registers the delegated repository call, returning err. + expectRepo func(repo *mockListRepository, err error) + // call invokes the service method on behalf of userID. + call func(svc *ListService, userID string) error +} + +func (op guardedOp) guardMethod() string { + if op.byItem { + return "IsUserInListByItemID" + } + return "IsUserInList" +} + +func (op guardedOp) expectGuard(repo *mockListRepository, userID string, inList bool, err error) { + if op.byItem { + repo.On("IsUserInListByItemID", mock.Anything, "item-1", userID).Return(inList, err) + return + } + repo.On("IsUserInList", mock.Anything, "list-1", userID).Return(inList, err) +} + +func guardedOps() []guardedOp { + ctx := context.Background() + + return []guardedOp{ + { + name: "DeleteList", + expectRepo: func(repo *mockListRepository, err error) { + repo.On("DeleteList", mock.Anything, "list-1").Return(err) + }, + call: func(svc *ListService, userID string) error { + return svc.DeleteList(ctx, userID, "list-1") + }, + }, + { + name: "AddUserToList", + expectRepo: func(repo *mockListRepository, err error) { + repo.On("AddUserToList", mock.Anything, "list-1", "user-2").Return(err) + }, + call: func(svc *ListService, userID string) error { + return svc.AddUserToList(ctx, userID, "list-1", "user-2") + }, + }, + { + name: "RemoveUserFromList", + expectRepo: func(repo *mockListRepository, err error) { + repo.On("RemoveUserFromList", mock.Anything, "list-1", "user-2").Return(err) + }, + call: func(svc *ListService, userID string) error { + return svc.RemoveUserFromList(ctx, userID, "list-1", "user-2") + }, + }, + { + name: "CreateListItem", + expectRepo: func(repo *mockListRepository, err error) { + repo.On("CreateListItem", mock.Anything, "list-1", "Milk").Return(nil, err) + }, + call: func(svc *ListService, userID string) error { + item, err := svc.CreateListItem(ctx, userID, "list-1", "Milk") + if item != nil { + return errors.New("expected no item on failure") + } + return err + }, + }, + { + name: "GetListItems", + expectRepo: func(repo *mockListRepository, err error) { + repo.On("GetListItems", mock.Anything, "list-1").Return(nil, err) + }, + call: func(svc *ListService, userID string) error { + items, err := svc.GetListItems(ctx, userID, "list-1") + if items != nil { + return errors.New("expected no items on failure") + } + return err + }, + }, + { + name: "DeleteListItem", + byItem: true, + expectRepo: func(repo *mockListRepository, err error) { + repo.On("DeleteListItem", mock.Anything, "item-1").Return(err) + }, + call: func(svc *ListService, userID string) error { + return svc.DeleteListItem(ctx, userID, "item-1") + }, + }, + { + name: "UpdateListItem", + byItem: true, + expectRepo: func(repo *mockListRepository, err error) { + repo.On("UpdateListItem", mock.Anything, "item-1", "Oat milk", true).Return(err) + }, + call: func(svc *ListService, userID string) error { + return svc.UpdateListItem(ctx, userID, "item-1", "Oat milk", true) + }, + }, + { + name: "SetListItemTitle", + byItem: true, + expectRepo: func(repo *mockListRepository, err error) { + repo.On("SetListItemTitle", mock.Anything, "item-1", "Oat milk").Return(err) + }, + call: func(svc *ListService, userID string) error { + return svc.SetListItemTitle(ctx, userID, "item-1", "Oat milk") + }, + }, + { + name: "SetListItemCompleted", + byItem: true, + expectRepo: func(repo *mockListRepository, err error) { + repo.On("SetListItemCompleted", mock.Anything, "item-1", true).Return(err) + }, + call: func(svc *ListService, userID string) error { + return svc.SetListItemCompleted(ctx, userID, "item-1", true) + }, + }, + } +} + +func TestListService_AccessControl(t *testing.T) { + for _, op := range guardedOps() { + t.Run(op.name, func(t *testing.T) { + t.Run("non-member is refused before any write", func(t *testing.T) { + svc, repo, _ := newListService(t) + + op.expectGuard(repo, "intruder", false, nil) + + err := op.call(svc, "intruder") + + assert.ErrorIs(t, err, ErrUserNotInList) + assertCallOrder(t, repo, op.guardMethod()) + }) + + t.Run("membership check error is propagated, not masked", func(t *testing.T) { + svc, repo, _ := newListService(t) + + repoErr := errors.New("connection reset") + op.expectGuard(repo, "user-1", false, repoErr) + + err := op.call(svc, "user-1") + + assert.ErrorIs(t, err, repoErr) + assert.NotErrorIs(t, err, ErrUserNotInList) + assertCallOrder(t, repo, op.guardMethod()) + }) + + t.Run("repository error is propagated", func(t *testing.T) { + svc, repo, _ := newListService(t) + + repoErr := errors.New("write failed") + op.expectGuard(repo, "user-1", true, nil) + op.expectRepo(repo, repoErr) + + err := op.call(svc, "user-1") + + assert.ErrorIs(t, err, repoErr) + assertCallOrder(t, repo, op.guardMethod(), op.name) + }) + }) + } } func TestListService_ValidationErrors(t *testing.T) { + ctx := context.Background() + tests := []struct { name string call func(svc *ListService) error wantErr error }{ + { + name: "CreateList without name", + call: func(svc *ListService) error { + _, err := svc.CreateList(ctx, "user-1", "") + return err + }, + wantErr: ErrListNameMissing, + }, { name: "DeleteList without list id", - call: func(svc *ListService) error { return svc.DeleteList(context.Background(), "user-1", "") }, + call: func(svc *ListService) error { return svc.DeleteList(ctx, "user-1", "") }, wantErr: ErrListIDMissing, }, { - name: "AddUserToList without list id", - call: func(svc *ListService) error { - return svc.AddUserToList(context.Background(), "user-1", "", "user-2") - }, + name: "AddUserToList without list id", + call: func(svc *ListService) error { return svc.AddUserToList(ctx, "user-1", "", "user-2") }, wantErr: ErrListIDMissing, }, { - name: "AddUserToList without user id", - call: func(svc *ListService) error { - return svc.AddUserToList(context.Background(), "user-1", "list-1", "") - }, + name: "AddUserToList without user id", + call: func(svc *ListService) error { return svc.AddUserToList(ctx, "user-1", "list-1", "") }, wantErr: ErrUserIDMissing, }, { - name: "RemoveUserFromList without user id", - call: func(svc *ListService) error { - return svc.RemoveUserFromList(context.Background(), "user-1", "list-1", "") - }, + name: "RemoveUserFromList without list id", + call: func(svc *ListService) error { return svc.RemoveUserFromList(ctx, "user-1", "", "user-2") }, + wantErr: ErrListIDMissing, + }, + { + name: "RemoveUserFromList without user id", + call: func(svc *ListService) error { return svc.RemoveUserFromList(ctx, "user-1", "list-1", "") }, wantErr: ErrUserIDMissing, }, + { + name: "OrderLists without user id", + call: func(svc *ListService) error { return svc.OrderLists(ctx, "", []string{"list-1"}) }, + wantErr: ErrUserIDMissing, + }, + { + name: "OrderLists with nil list ids", + call: func(svc *ListService) error { return svc.OrderLists(ctx, "user-1", nil) }, + wantErr: ErrListIDMissing, + }, + { + name: "OrderLists with empty list ids", + call: func(svc *ListService) error { return svc.OrderLists(ctx, "user-1", []string{}) }, + wantErr: ErrListIDMissing, + }, + { + name: "CreateListItem without list id", + call: func(svc *ListService) error { + _, err := svc.CreateListItem(ctx, "user-1", "", "Milk") + return err + }, + wantErr: ErrListIDMissing, + }, { name: "CreateListItem without title", call: func(svc *ListService) error { - _, err := svc.CreateListItem(context.Background(), "user-1", "list-1", "") + _, err := svc.CreateListItem(ctx, "user-1", "list-1", "") return err }, wantErr: ErrListItemTitleMissing, }, { - name: "DeleteListItem without item id", - call: func(svc *ListService) error { - return svc.DeleteListItem(context.Background(), "user-1", "") - }, + name: "DeleteListItem without item id", + call: func(svc *ListService) error { return svc.DeleteListItem(ctx, "user-1", "") }, wantErr: ErrListItemIDMissing, }, { - name: "UpdateListItem without title", - call: func(svc *ListService) error { - return svc.UpdateListItem(context.Background(), "user-1", "item-1", "", false) - }, + name: "UpdateListItem without item id", + call: func(svc *ListService) error { return svc.UpdateListItem(ctx, "user-1", "", "Milk", false) }, + wantErr: ErrListItemIDMissing, + }, + { + name: "UpdateListItem without title", + call: func(svc *ListService) error { return svc.UpdateListItem(ctx, "user-1", "item-1", "", false) }, wantErr: ErrListItemTitleMissing, }, { - name: "SetListItemTitle without item id", - call: func(svc *ListService) error { - return svc.SetListItemTitle(context.Background(), "user-1", "", "Milk") - }, + name: "SetListItemTitle without item id", + call: func(svc *ListService) error { return svc.SetListItemTitle(ctx, "user-1", "", "Milk") }, wantErr: ErrListItemIDMissing, }, { - name: "SetListItemCompleted without item id", - call: func(svc *ListService) error { - return svc.SetListItemCompleted(context.Background(), "user-1", "", true) - }, + name: "SetListItemTitle without title", + call: func(svc *ListService) error { return svc.SetListItemTitle(ctx, "user-1", "item-1", "") }, + wantErr: ErrListItemTitleMissing, + }, + { + name: "SetListItemCompleted without item id", + call: func(svc *ListService) error { return svc.SetListItemCompleted(ctx, "user-1", "", true) }, wantErr: ErrListItemIDMissing, }, { name: "GetListItems without list id", call: func(svc *ListService) error { - _, err := svc.GetListItems(context.Background(), "user-1", "") + _, err := svc.GetListItems(ctx, "user-1", "") return err }, wantErr: ErrListIDMissing, @@ -334,9 +763,41 @@ func TestListService_ValidationErrors(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - svc, _, _ := newListService(t) + svc, repo, tx := newListService(t) - assert.ErrorIs(t, tt.call(svc), tt.wantErr) + err := tt.call(svc) + + assert.ErrorIs(t, err, tt.wantErr) + assert.Zero(t, tx.calls, "no transaction may be started") + assertCallOrder(t, repo) + }) + } +} + +func TestIsPermutation(t *testing.T) { + tests := []struct { + name string + a []string + b []string + want bool + }{ + {name: "both empty", a: nil, b: []string{}, want: true}, + {name: "same order", a: []string{"a", "b", "c"}, b: []string{"a", "b", "c"}, want: true}, + {name: "reordered", a: []string{"a", "b", "c"}, b: []string{"c", "a", "b"}, want: true}, + {name: "different case", a: []string{"ABC", "def"}, b: []string{"DEF", "abc"}, want: true}, + {name: "first is shorter", a: []string{"a", "b"}, b: []string{"a", "b", "c"}, want: false}, + {name: "second is shorter", a: []string{"a", "b", "c"}, b: []string{"a", "b"}, want: false}, + {name: "same length, different element", a: []string{"a", "b"}, b: []string{"a", "c"}, want: false}, + {name: "duplicate in first", a: []string{"a", "a"}, b: []string{"a", "b"}, want: false}, + {name: "duplicate in second", a: []string{"a", "b"}, b: []string{"a", "a"}, want: false}, + {name: "same duplicate on both sides", a: []string{"a", "a"}, b: []string{"a", "a"}, want: false}, + {name: "duplicate differing only in case", a: []string{"a", "b"}, b: []string{"a", "A"}, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isPermutation(tt.a, tt.b)) + assert.Equal(t, tt.want, isPermutation(tt.b, tt.a), "must be symmetric") }) } } diff --git a/internal/repository/list.go b/internal/repository/list.go index 6e92921..12a303f 100644 --- a/internal/repository/list.go +++ b/internal/repository/list.go @@ -38,7 +38,7 @@ func (r *ListRepo) DeleteList(ctx context.Context, listID string) error { func (r *ListRepo) GetLists(ctx context.Context, userID string) ([]domain.List, error) { rows, err := r.conn(ctx).QueryContext(ctx, - "SELECT l.id, l.name, l.created_at, l.modified_at, (SELECT COUNT(*) FROM list_items WHERE list_id=l.id), (SELECT COUNT(*) FROM list_items WHERE list_id=l.id AND is_completed=true) FROM lists AS l INNER JOIN list_users ON l.id=list_users.list_id WHERE list_users.user_id = $1", + "SELECT l.id, l.name, l.created_at, l.modified_at, (SELECT COUNT(*) FROM list_items WHERE list_id=l.id), (SELECT COUNT(*) FROM list_items WHERE list_id=l.id AND is_completed=true), lu.position FROM lists AS l INNER JOIN list_users AS lu ON l.id=lu.list_id WHERE lu.user_id = $1 ORDER BY lu.position", userID, ) if err != nil { @@ -52,7 +52,7 @@ func (r *ListRepo) GetLists(ctx context.Context, userID string) ([]domain.List, lists := make([]domain.List, 0, 16) for rows.Next() { var l domain.List - err = rows.Scan(&l.ID, &l.Name, &l.CreatedAt, &l.ModifiedAt, &l.TotalItems, &l.CompletedItems) + err = rows.Scan(&l.ID, &l.Name, &l.CreatedAt, &l.ModifiedAt, &l.TotalItems, &l.CompletedItems, &l.Position) if err != nil { return nil, err } @@ -87,6 +87,42 @@ func (r *ListRepo) RemoveUserFromList(ctx context.Context, listID string, userID return nil } +func (r *ListRepo) OrderLists(ctx context.Context, userID string, listIDs []string) error { + _, err := r.conn(ctx).ExecContext(ctx, + "UPDATE list_users AS lu SET position = o.idx - 1 FROM unnest($2::uuid[]) WITH ORDINALITY AS o(list_id, idx) WHERE lu.list_id = o.list_id AND lu.user_id=$1", + userID, listIDs, + ) + if err != nil { + return fmt.Errorf("failed to order lists: %w", err) + } + + return nil +} + +func (r *ListRepo) LockUsersLists(ctx context.Context, userID string) ([]string, error) { + rows, err := r.conn(ctx).QueryContext(ctx, + "SELECT list_id FROM list_users WHERE user_id = $1 FOR UPDATE", + userID, + ) + if err != nil { + return nil, fmt.Errorf("failed to lock users lists: %w", err) + } + defer rows.Close() + + ids := make([]string, 0, 16) + for rows.Next() { + var listID string + err = rows.Scan(&listID) + if err != nil { + return nil, fmt.Errorf("failed to read list id: %w", err) + } + + ids = append(ids, listID) + } + + return ids, nil +} + func (r *ListRepo) IsUserInList(ctx context.Context, listID string, userID string) (bool, error) { var cnt int