From b6a4627fb02930e0b66f2b36b99bd25306ffd93e Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 5 Feb 2026 10:48:35 +0100 Subject: [PATCH] docs: add wip plans --- plans/fix-s3-unsigned-payload.md | 240 ++++++ plans/fix-s3-upload-error-handling.md | 121 +++ plans/impl-task-index-counter-table.md | 1008 ++++++++++++++++++++++++ plans/investigation-migration-bugs.md | 255 ++++++ plans/task-index-increment.md | 176 +++++ 5 files changed, 1800 insertions(+) create mode 100644 plans/fix-s3-unsigned-payload.md create mode 100644 plans/fix-s3-upload-error-handling.md create mode 100644 plans/impl-task-index-counter-table.md create mode 100644 plans/investigation-migration-bugs.md create mode 100644 plans/task-index-increment.md diff --git a/plans/fix-s3-unsigned-payload.md b/plans/fix-s3-unsigned-payload.md new file mode 100644 index 000000000..68adb2011 --- /dev/null +++ b/plans/fix-s3-unsigned-payload.md @@ -0,0 +1,240 @@ +# Fix S3 XAmzContentSHA256Mismatch for S3-Compatible Stores + +**Goal:** Add a configuration option to disable S3 payload signing (use `UNSIGNED-PAYLOAD`), fixing uploads to S3-compatible stores like Cellar/Ceph that fail with `XAmzContentSHA256Mismatch`. + +**Architecture:** The AWS SDK Go v2 computes a SHA256 hash of the request body and sends it in the `X-Amz-Content-Sha256` header for SigV4 signing. Some S3-compatible stores (Ceph RadosGW, Cellar, etc.) don't verify this correctly, causing `XAmzContentSHA256Mismatch` errors. The SDK provides `v4.SwapComputePayloadSHA256ForUnsignedPayloadMiddleware` to replace the hash with `UNSIGNED-PAYLOAD`, which is safe over HTTPS. We add a `files.s3.disablesigning` config option that applies this middleware when creating the S3 client. + +**Tech Stack:** Go, AWS SDK Go v2 (`aws/signer/v4`), viper config + +--- + +### Task 1: Add the config key + +**Files:** +- Modify: `pkg/config/config.go:170` (add key constant) +- Modify: `pkg/config/config.go:448` (add default) + +**Step 1: Add the config key constant** + +Add after `FilesS3UsePathStyle` (line 170) in `pkg/config/config.go`: + +```go +FilesS3UsePathStyle Key = `files.s3.usepathstyle` +FilesS3DisableSigning Key = `files.s3.disablesigning` +FilesS3TempDir Key = `files.s3.tempdir` +``` + +**Step 2: Add the default value** + +Add after `FilesS3UsePathStyle.setDefault(false)` (line 448) in `pkg/config/config.go`: + +```go +FilesS3UsePathStyle.setDefault(false) +FilesS3DisableSigning.setDefault(false) +FilesS3TempDir.setDefault("") +``` + +**Step 3: Commit** + +```bash +git add pkg/config/config.go +git commit -m "feat: add files.s3.disablesigning config key" +``` + +--- + +### Task 2: Add config to `config-raw.json` + +This file generates `config.yml.sample` and documents the option for users. + +**Files:** +- Modify: `config-raw.json` (add entry after `usepathstyle` in the `s3` children array, around line 531) + +**Step 1: Add the config entry** + +Insert after the `usepathstyle` entry (line 527-531) in `config-raw.json`: + +```json +{ + "key": "disablesigning", + "default_value": "false", + "comment": "When enabled, the S3 client will send UNSIGNED-PAYLOAD instead of computing a SHA256 hash for request signing. Some S3-compatible providers (such as Ceph RadosGW, Clever Cloud Cellar) do not correctly verify payload signatures and return XAmzContentSHA256Mismatch errors. Enabling this option works around the issue. Only applies over HTTPS." +} +``` + +**Step 2: Commit** + +```bash +git add config-raw.json +git commit -m "docs: add files.s3.disablesigning to config template" +``` + +--- + +### Task 3: Write the failing test + +**Files:** +- Modify: `pkg/files/s3_test.go` (add test at the end) + +**Step 1: Write the test** + +Add to the end of `pkg/files/s3_test.go`: + +```go +func TestInitS3FileHandler_DisableSigningOption(t *testing.T) { + // Save original config + originalType := config.FilesType.GetString() + originalEndpoint := config.FilesS3Endpoint.GetString() + originalBucket := config.FilesS3Bucket.GetString() + originalAccessKey := config.FilesS3AccessKey.GetString() + originalSecretKey := config.FilesS3SecretKey.GetString() + originalDisableSigning := config.FilesS3DisableSigning.GetBool() + originalClient := s3Client + + defer func() { + config.FilesType.Set(originalType) + config.FilesS3Endpoint.Set(originalEndpoint) + config.FilesS3Bucket.Set(originalBucket) + config.FilesS3AccessKey.Set(originalAccessKey) + config.FilesS3SecretKey.Set(originalSecretKey) + config.FilesS3DisableSigning.Set(originalDisableSigning) + s3Client = originalClient + }() + + config.FilesS3Endpoint.Set("https://fake-endpoint.example.com") + config.FilesS3Bucket.Set("test-bucket") + config.FilesS3AccessKey.Set("test-key") + config.FilesS3SecretKey.Set("test-secret") + config.FilesS3DisableSigning.Set(true) + + // initS3FileHandler should succeed (validation will fail because the + // endpoint is fake, but that happens later in InitFileHandler). + // We call initS3FileHandler directly to test the client is created. + err := initS3FileHandler() + // The error will be from ValidateFileStorage or nil -- either way, + // initS3FileHandler itself should not error for the signing option. + // We can't easily verify the middleware was applied without a real + // request, so we verify no panic or init error. + assert.NoError(t, err) + assert.NotNil(t, s3Client) +} +``` + +**Step 2: Run the test to verify it fails** + +Run: `mage test:filter TestInitS3FileHandler_DisableSigningOption` +Expected: FAIL -- `config.FilesS3DisableSigning` does not exist yet if Task 1 isn't done, or passes if Task 1 is already done (in which case the test serves as a regression guard). + +**Step 3: Commit** + +```bash +git add pkg/files/s3_test.go +git commit -m "test: add test for S3 disable signing config option" +``` + +--- + +### Task 4: Apply the middleware in `initS3FileHandler` + +**Files:** +- Modify: `pkg/files/filehandling.go:96-100` (add middleware option) +- Modify: `pkg/files/filehandling.go` imports (add `v4` signer import) + +**Step 1: Add the v4 signer import** + +Add to the imports in `pkg/files/filehandling.go`: + +```go +v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" +``` + +**Step 2: Apply the middleware when config is enabled** + +Replace the S3 client creation block (lines 97-100) in `pkg/files/filehandling.go`: + +```go +// Before: +client := s3.NewFromConfig(cfg, func(o *s3.Options) { + o.BaseEndpoint = aws.String(endpoint) + o.UsePathStyle = config.FilesS3UsePathStyle.GetBool() +}) + +// After: +client := s3.NewFromConfig(cfg, func(o *s3.Options) { + o.BaseEndpoint = aws.String(endpoint) + o.UsePathStyle = config.FilesS3UsePathStyle.GetBool() + if config.FilesS3DisableSigning.GetBool() { + o.APIOptions = append(o.APIOptions, v4.SwapComputePayloadSHA256ForUnsignedPayloadMiddleware) + } +}) +``` + +This replaces the `ComputePayloadSHA256` middleware with `UnsignedPayload` for all requests made by this client. Instead of computing the body's SHA256 and setting it in `X-Amz-Content-Sha256`, the client sends `UNSIGNED-PAYLOAD`. This is safe over HTTPS since TLS provides integrity guarantees. + +**Step 3: Run the test** + +Run: `mage test:filter TestInitS3FileHandler_DisableSigningOption` +Expected: PASS + +**Step 4: Run the full file test suite** + +Run: `mage test:filter TestFileSave_S3` +Expected: PASS (existing tests unaffected since the config defaults to false) + +**Step 5: Run lint** + +Run: `mage lint` +Expected: PASS + +**Step 6: Commit** + +```bash +git add pkg/files/filehandling.go +git commit -m "feat: support UNSIGNED-PAYLOAD for S3-compatible stores + +Add files.s3.disablesigning config option that replaces the SHA256 payload +hash with UNSIGNED-PAYLOAD in S3 requests. This fixes XAmzContentSHA256Mismatch +errors with S3-compatible providers like Ceph RadosGW and Clever Cloud Cellar." +``` + +--- + +### Task 5: Run the config init tests + +**Files:** (no changes, verification only) + +**Step 1: Run all S3 config tests** + +Run: `mage test:filter TestInitFileHandler_S3` +Expected: PASS -- existing config validation tests should still pass since `disablesigning` is only read, not validated as required. + +**Step 2: Run full lint** + +Run: `mage lint` +Expected: PASS + +--- + +### Summary of changes + +| File | Change | +|------|--------| +| `pkg/config/config.go` | Add `FilesS3DisableSigning` key constant and `false` default | +| `config-raw.json` | Add `disablesigning` entry with documentation comment | +| `pkg/files/filehandling.go` | Import `v4` signer, apply `SwapComputePayloadSHA256ForUnsignedPayloadMiddleware` when config is true | +| `pkg/files/s3_test.go` | Add `TestInitS3FileHandler_DisableSigningOption` test | + +### User-facing documentation + +Users who encounter `XAmzContentSHA256Mismatch` errors should add to their `config.yml`: + +```yaml +files: + type: s3 + s3: + endpoint: "https://..." + bucket: "..." + accesskey: "..." + secretkey: "..." + disablesigning: true +``` diff --git a/plans/fix-s3-upload-error-handling.md b/plans/fix-s3-upload-error-handling.md new file mode 100644 index 000000000..b15b32607 --- /dev/null +++ b/plans/fix-s3-upload-error-handling.md @@ -0,0 +1,121 @@ +# Fix Frontend S3 Upload Error Handling Implementation Plan + +**Goal:** Show meaningful error notifications when file attachment uploads fail, instead of silently swallowing errors. + +**Architecture:** The backend already returns `{errors: [...], success: [...]}` in the response body (HTTP 200) for batch upload semantics. The frontend helper `uploadFiles()` detects errors and throws, but the component caller never awaits or catches. Fix the caller to handle errors, and fix the error formatting to produce readable messages. + +**Tech Stack:** Vue 3 + TypeScript, vue-i18n + +--- + +### Task 1: Fix error message formatting in `uploadFiles()` + +The current code `throw Error(response.errors)` passes an array of `{code, message}` objects to `Error()`, which coerces them to `"[object Object]"`. Instead, extract the message strings and join them. + +**Files:** +- Modify: `frontend/src/helpers/attachments.ts:32-34` + +**Step 1: Fix the error throw to produce a readable message** + +Replace lines 32-34 in `frontend/src/helpers/attachments.ts`: + +```ts +// Before: +if (response.errors !== null) { + throw Error(response.errors) +} + +// After: +if (response.errors !== null) { + const messages = response.errors.map((e: {message: string}) => e.message) + throw new Error(messages.join('\n')) +} +``` + +**Step 2: Run frontend lint to verify** + +Run: `cd frontend && pnpm lint` +Expected: PASS (no new lint errors) + +**Step 3: Commit** + +```bash +git add frontend/src/helpers/attachments.ts +git commit -m "fix: format attachment upload error messages as readable strings" +``` + +--- + +### Task 2: Add error handling in `Attachments.vue` upload caller + +The function `uploadFilesToTask()` at line 336-338 calls `uploadFiles()` without `await` or error handling. The promise rejection goes unhandled. Add `try/catch` with the existing `error()` notification function (already imported at line 192). + +**Files:** +- Modify: `frontend/src/components/tasks/partials/Attachments.vue:336-338` + +**Step 1: Add try/catch to `uploadFilesToTask`** + +Replace lines 336-338: + +```ts +// Before: +function uploadFilesToTask(files: File[] | FileList) { + uploadFiles(attachmentService, props.task.id, files) +} + +// After: +async function uploadFilesToTask(files: File[] | FileList) { + try { + await uploadFiles(attachmentService, props.task.id, files) + } catch (e) { + error(e) + } +} +``` + +This mirrors the existing pattern used in `deleteAttachment()` at line 346-358 in the same file. The `error` import from `@/message` is already present at line 192. + +**Step 2: Run frontend lint to verify** + +Run: `cd frontend && pnpm lint` +Expected: PASS + +**Step 3: Run frontend type check** + +Run: `cd frontend && pnpm typecheck` +Expected: PASS + +**Step 4: Commit** + +```bash +git add frontend/src/components/tasks/partials/Attachments.vue +git commit -m "fix: handle attachment upload errors with user-visible notifications" +``` + +--- + +### Task 3: Manual verification + +There are no existing unit tests for `uploadFilesToTask` or `uploadFiles`. Verify manually: + +**Step 1: Verify error display with backend returning errors** + +1. Start the dev server: `cd frontend && pnpm dev` +2. Upload a file to a task +3. If using S3 with a misconfigured endpoint, the error should now show as a toast notification at the bottom left +4. If using local storage, simulate by temporarily making the files directory read-only, then uploading + +**Step 2: Verify partial success still works** + +1. Upload multiple files where one exceeds the size limit +2. The successful files should still appear as attachments +3. The failed file(s) should show an error toast with the message from the backend + +--- + +### Summary of changes + +| File | Change | +|------|--------| +| `frontend/src/helpers/attachments.ts:32-34` | Format error array into readable message string | +| `frontend/src/components/tasks/partials/Attachments.vue:336-338` | Add `async`, `await`, `try/catch` with `error()` notification | diff --git a/plans/impl-task-index-counter-table.md b/plans/impl-task-index-counter-table.md new file mode 100644 index 000000000..e24e91f04 --- /dev/null +++ b/plans/impl-task-index-counter-table.md @@ -0,0 +1,1008 @@ +# Task Index Counter Table Implementation Plan + +**Goal:** Eliminate concurrent duplicate task indexes by using a PostgreSQL counter table with row-level locking to assign project-scoped task indexes atomically. + +**Architecture:** On PostgreSQL, a new `project_task_counters` table and trigger atomically assign task indexes on INSERT. On MySQL/SQLite, nothing changes — the existing application-level `MAX(index)+1` logic remains as-is with no new tables or schema changes. The frontend stops pre-calculating indexes during bulk insert — the backend always owns index assignment. + +**Tech Stack:** Go (XORM ORM), PostgreSQL triggers/plpgsql, Vue 3 + TypeScript + +--- + +### Task 1: Create the database migration file and update initSchema + +**Files:** +- Create: `pkg/migration/20260130120000.go` +- Modify: `pkg/migration/migration.go:265-273` + +**Why two files:** The migration handles existing installs that are upgrading. But on brand-new installations, `initSchema` runs instead (via xormigrate's `InitSchema` callback), creates all tables from `GetTables()`, and marks every migration as already applied — so the migration never executes. Since `ProjectTaskCounter` is deliberately not in `GetTables()`, we must also set up the PostgreSQL counter table and trigger inside `initSchema`. + +**Step 1: Generate migration skeleton** + +Run: `mage dev:make-migration ProjectTaskCounter` + +This creates a timestamped file in `pkg/migration/`. The actual timestamp will differ — use whatever `mage` generates. The instructions below reference the generated filename. + +**Step 2: Implement the migration** + +Replace the generated migration content with: + +```go +package migration + +import ( + "fmt" + + "code.vikunja.io/api/pkg/db" + + "src.techknowlogick.com/xormigrate" + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +type ProjectTaskCounter20260130120000 struct { + ProjectID int64 `xorm:"bigint not null pk"` + LastIndex int64 `xorm:"bigint not null default 0"` +} + +func (ProjectTaskCounter20260130120000) TableName() string { + return "project_task_counters" +} + +func init() { + migrations = append(migrations, &xormigrate.Migration{ + ID: "20260130120000", + Description: "Add project task counter table and PostgreSQL trigger for atomic index assignment", + Migrate: func(tx *xorm.Engine) error { + // Only PostgreSQL gets the counter table and trigger. + // MySQL/SQLite continue using the existing application-level logic unchanged. + if db.Type() != schemas.POSTGRES { + return nil + } + return setupPostgresTaskIndexCounter(tx) + }, + Rollback: func(tx *xorm.Engine) error { + if db.Type() != schemas.POSTGRES { + return nil + } + if _, err := tx.Exec(`DROP TRIGGER IF EXISTS task_set_index ON tasks`); err != nil { + return err + } + if _, err := tx.Exec(`DROP FUNCTION IF EXISTS set_task_index()`); err != nil { + return err + } + return tx.DropTables(ProjectTaskCounter20260130120000{}) + }, + }) +} + +// setupPostgresTaskIndexCounter creates the counter table, trigger function, +// and trigger for atomic task index assignment on PostgreSQL. +// Called from both the migration (existing installs) and initSchema (fresh installs). +func setupPostgresTaskIndexCounter(tx *xorm.Engine) error { + // 1. Create the counter table + if err := tx.Sync(ProjectTaskCounter20260130120000{}); err != nil { + return fmt.Errorf("create project_task_counters table: %w", err) + } + + // 2. Seed the counter table from existing task data (no-op on fresh installs) + _, err := tx.Exec(` + INSERT INTO project_task_counters (project_id, last_index) + SELECT project_id, COALESCE(MAX("index"), 0) + FROM tasks + GROUP BY project_id + ON CONFLICT (project_id) DO UPDATE + SET last_index = EXCLUDED.last_index + `) + if err != nil { + return fmt.Errorf("seed project_task_counters: %w", err) + } + + // 3. Create the trigger function + _, err = tx.Exec(` + CREATE OR REPLACE FUNCTION set_task_index() + RETURNS TRIGGER AS $$ + BEGIN + INSERT INTO project_task_counters (project_id, last_index) + VALUES (NEW.project_id, 1) + ON CONFLICT (project_id) DO UPDATE + SET last_index = project_task_counters.last_index + 1 + RETURNING last_index INTO NEW."index"; + RETURN NEW; + END; + $$ LANGUAGE plpgsql + `) + if err != nil { + return fmt.Errorf("create set_task_index function: %w", err) + } + + // 4. Create the trigger on the tasks table + _, err = tx.Exec(` + CREATE TRIGGER task_set_index + BEFORE INSERT ON tasks + FOR EACH ROW EXECUTE FUNCTION set_task_index() + `) + if err != nil { + return fmt.Errorf("create task_set_index trigger: %w", err) + } + + return nil +} +``` + +Note: The function is named `setupPostgresTaskIndexCounter` (exported-style within the package) so `initSchema` can call it too. + +**Step 3: Update `initSchema` to call the PostgreSQL setup on fresh installs** + +In `pkg/migration/migration.go`, modify the `initSchema` function (around line 265): + +```go +func initSchema(tx *xorm.Engine) error { + schemeBeans := []interface{}{} + schemeBeans = append(schemeBeans, models.GetTables()...) + schemeBeans = append(schemeBeans, files.GetTables()...) + schemeBeans = append(schemeBeans, migration.GetTables()...) + schemeBeans = append(schemeBeans, user.GetTables()...) + schemeBeans = append(schemeBeans, notifications.GetTables()...) + if err := tx.Sync2(schemeBeans...); err != nil { + return err + } + + // Set up PostgreSQL-specific counter table and trigger for atomic task index + // assignment. This table is intentionally not in GetTables() to avoid creating + // it on MySQL/SQLite. + if db.Type() == schemas.POSTGRES { + if err := setupPostgresTaskIndexCounter(tx); err != nil { + return err + } + } + + return nil +} +``` + +You'll need to add `"xorm.io/xorm/schemas"` to the imports of `migration.go` if not already present. + +**Step 4: Verify it compiles** + +Run: `mage build` + +Expected: Successful build with no errors. + +**Step 5: Commit** + +```bash +git add pkg/migration/.go pkg/migration/migration.go +git commit -m "feat(db): add migration and initSchema setup for postgres task index counter" +``` + +--- + +### Task 2: Create the counter model struct (PostgreSQL only) + +**Files:** +- Create: `pkg/models/project_task_counter.go` + +The model struct is needed so Go code can reference the table (for deletion cleanup, counter reads, etc.), but it must **not** be registered in `GetTables()`. Registering it there would cause XORM's `Sync2` to auto-create the table on all databases, including MySQL/SQLite where we don't want it. + +The table is created exclusively by the migration (Task 1) on PostgreSQL only. + +**Step 1: Create the model struct** + +Create `pkg/models/project_task_counter.go`: + +```go +package models + +// ProjectTaskCounter tracks the last assigned task index per project. +// This table only exists on PostgreSQL, where it is created by a migration +// and maintained by a database trigger. It is NOT registered in GetTables() +// to avoid XORM auto-syncing it on MySQL/SQLite. +type ProjectTaskCounter struct { + ProjectID int64 `xorm:"bigint not null pk" json:"-"` + LastIndex int64 `xorm:"bigint not null default 0" json:"-"` +} + +func (ProjectTaskCounter) TableName() string { + return "project_task_counters" +} +``` + +**Step 2: Verify it compiles** + +Run: `mage build` + +**Step 3: Commit** + +```bash +git add pkg/models/project_task_counter.go +git commit -m "feat(models): add ProjectTaskCounter struct for postgres counter table" +``` + +--- + +### Task 3: Write failing tests for the new index logic + +**Files:** +- Modify: `pkg/models/tasks_test.go` + +We need tests that verify: +1. On PostgreSQL, the trigger assigns the index (application code skips assignment) +2. On non-PostgreSQL, the existing logic still works +3. Creating multiple tasks in the same project yields sequential indexes +4. Moving a task between projects still gets a correct index + +**Step 1: Write the failing test for concurrent-safe index assignment** + +Add to `pkg/models/tasks_test.go`, inside or after the existing `TestTask_Create` function: + +```go +func TestTask_Create_IndexAssignment(t *testing.T) { + usr := &user.User{ + ID: 1, + Username: "user1", + Email: "user1@example.com", + } + + t.Run("sequential index assignment", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Project 1 has tasks with indexes up to 17 in fixtures. + // Creating a new task should get index 18. + task1 := &Task{ + Title: "First new task", + ProjectID: 1, + } + err := task1.Create(s, usr) + require.NoError(t, err) + assert.Equal(t, int64(18), task1.Index) + + // Creating another task in the same session should get 19. + task2 := &Task{ + Title: "Second new task", + ProjectID: 1, + } + err = task2.Create(s, usr) + require.NoError(t, err) + assert.Equal(t, int64(19), task2.Index) + + err = s.Commit() + require.NoError(t, err) + }) + + t.Run("index assignment in empty project", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Project 25 has no tasks. First task should get index 1. + task := &Task{ + Title: "First task in empty project", + ProjectID: 25, + } + err := task.Create(s, usr) + require.NoError(t, err) + assert.Equal(t, int64(1), task.Index) + + err = s.Commit() + require.NoError(t, err) + }) + + t.Run("provided index is ignored on postgres", func(t *testing.T) { + if db.Type() != schemas.POSTGRES { + t.Skip("trigger-based index assignment only on PostgreSQL") + } + + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Even if the client provides an index, the trigger should override it. + task := &Task{ + Title: "Task with provided index", + ProjectID: 1, + Index: 999, + } + err := task.Create(s, usr) + require.NoError(t, err) + // The trigger always assigns the next sequential index. + assert.Equal(t, int64(18), task.Index) + + err = s.Commit() + require.NoError(t, err) + }) +} +``` + +Note: You'll also need to add `"xorm.io/xorm/schemas"` to the import block of this test file (if not already present) for the `schemas.POSTGRES` reference. + +**Step 2: Run the tests to verify they fail (or pass on the existing logic)** + +Run: `mage test:filter TestTask_Create_IndexAssignment` + +Expected: The first two tests should pass (they verify existing behavior). The third test (`provided index is ignored on postgres`) will only run on PostgreSQL and will fail until we modify the application code, because the current code respects provided indexes. + +**Step 3: Commit** + +```bash +git add pkg/models/tasks_test.go +git commit -m "test(tasks): add tests for sequential and trigger-based index assignment" +``` + +--- + +### Task 4: Modify backend index logic to be database-aware + +**Files:** +- Modify: `pkg/models/tasks.go:835-869` (the `calculateNextTaskIndex` and `setNewTaskIndex` functions) + +**Step 1: Modify `setNewTaskIndex` to skip on PostgreSQL** + +On PostgreSQL, the trigger handles index assignment. The application code should set `Index = 0` so XORM doesn't include it in the INSERT column list and the trigger's `NEW.index` assignment takes effect. + +However — there's a subtlety. XORM's `Insert` will include `index` in the INSERT statement if the struct field is non-zero. On PostgreSQL, we need XORM to either: +- Not include `index` in the INSERT (so the trigger sets it), or +- Include it but the trigger overwrites it regardless. + +Looking at the trigger: it **always** runs `RETURNING last_index INTO NEW."index"` which overwrites whatever value was in `NEW.index`. So the trigger always wins, regardless of what XORM sends. This means the application code change is simpler: on PostgreSQL, we can skip the `setNewTaskIndex` call entirely, but we still need to read back the trigger-assigned index after INSERT. + +Replace the functions at `pkg/models/tasks.go:835-869`: + +```go +func calculateNextTaskIndex(s *xorm.Session, projectID int64) (nextIndex int64, err error) { + latestTask := &Task{} + _, err = s. + Where("project_id = ?", projectID). + OrderBy("`index` desc"). + Get(latestTask) + if err != nil { + return 0, err + } + + return latestTask.Index + 1, nil +} + +func setNewTaskIndex(s *xorm.Session, t *Task) (err error) { + // On PostgreSQL, the database trigger handles index assignment atomically. + // We set index to 0 so that after INSERT we can read back the trigger-assigned value. + if db.Type() == schemas.POSTGRES { + t.Index = 0 + return nil + } + + // For MySQL/SQLite: keep the existing application-level logic. + if t.Index == 0 { + t.Index, err = calculateNextTaskIndex(s, t.ProjectID) + return + } + + exists, err := s.Where("project_id = ? AND `index` = ?", t.ProjectID, t.Index).Exist(&Task{}) + if err != nil { + return err + } + if exists { + t.Index, err = calculateNextTaskIndex(s, t.ProjectID) + if err != nil { + return err + } + } + + return +} +``` + +You also need to add the imports to `tasks.go` if not already present: + +```go +"code.vikunja.io/api/pkg/db" +"xorm.io/xorm/schemas" +``` + +**Step 2: Read back the trigger-assigned index after INSERT (PostgreSQL only)** + +In the `createTask` function at `pkg/models/tasks.go:889-991`, after `s.Insert(t)` (line 922), add a read-back for PostgreSQL: + +Find this block (around lines 922-925): + +```go + _, err = s.Insert(t) + if err != nil { + return err + } +``` + +Replace with: + +```go + _, err = s.Insert(t) + if err != nil { + return err + } + + // On PostgreSQL, the trigger assigns the index. Read it back. + if db.Type() == schemas.POSTGRES { + has, err := s.ID(t.ID).Cols("index").Get(t) + if err != nil { + return err + } + if !has { + return fmt.Errorf("task %d not found after insert", t.ID) + } + } +``` + +Note: Make sure `"fmt"` is in the imports. + +**Step 3: Handle the project-move case** + +In `updateSingleTask` around line 1188-1196, the code calls `calculateNextTaskIndex` when moving between projects. On PostgreSQL, the trigger only fires on INSERT, not UPDATE. So the application code still needs to calculate the next index for project moves. But we should use the counter table on PostgreSQL for this too. + +Add a new helper function after `calculateNextTaskIndex`: + +```go +// calculateNextTaskIndexFromCounter reads the counter table and returns the next index, +// atomically incrementing it. Used for project moves on PostgreSQL. +func calculateNextTaskIndexFromCounter(s *xorm.Session, projectID int64) (nextIndex int64, err error) { + var lastIndex int64 + _, err = s.SQL(` + INSERT INTO project_task_counters (project_id, last_index) + VALUES (?, 1) + ON CONFLICT (project_id) DO UPDATE + SET last_index = project_task_counters.last_index + 1 + RETURNING last_index + `, projectID).Get(&lastIndex) + if err != nil { + return 0, err + } + return lastIndex, nil +} +``` + +Then modify the project-move block in `updateSingleTask` (around line 1188-1196): + +```go + // If the task is being moved between projects, make sure to move the bucket + index as well + if t.ProjectID != 0 && ot.ProjectID != t.ProjectID { + if db.Type() == schemas.POSTGRES { + t.Index, err = calculateNextTaskIndexFromCounter(s, t.ProjectID) + } else { + t.Index, err = calculateNextTaskIndex(s, t.ProjectID) + } + if err != nil { + return err + } + t.BucketID = 0 + colsToUpdate = append(colsToUpdate, "index") + } +``` + +**Step 4: Verify it compiles** + +Run: `mage build` + +**Step 5: Run all task tests** + +Run: `mage test:filter TestTask_Create` + +Expected: All tests pass. + +Also run: `mage test:filter TestTask_Create_IndexAssignment` + +Expected: All tests pass (including the PostgreSQL-specific test if running against PostgreSQL). + +**Step 6: Commit** + +```bash +git add pkg/models/tasks.go +git commit -m "feat(tasks): use postgres trigger for atomic index assignment, keep app logic for mysql/sqlite" +``` + +--- + +### Task 5: Clean up counter rows on project deletion (PostgreSQL only) + +**Files:** +- Modify: `pkg/models/project.go:1164-1277` + +Since the counter table only exists on PostgreSQL, the cleanup must be guarded by a `db.Type()` check. + +**Step 1: Write a failing test** + +Add to the existing project deletion tests (find where `TestProject_Delete` or similar exists, or add to `pkg/models/project_test.go`): + +```go +t.Run("deleting a project should clean up its task counter", func(t *testing.T) { + if db.Type() != schemas.POSTGRES { + t.Skip("Counter table only exists on PostgreSQL") + } + + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // First create a task to ensure a counter row exists + task := &Task{ + Title: "Counter test task", + ProjectID: 36, // Use a project the test user can delete + } + err := task.Create(s, usr) + require.NoError(t, err) + + // Verify counter row exists + counter := &ProjectTaskCounter{ProjectID: 36} + has, err := s.Get(counter) + require.NoError(t, err) + assert.True(t, has) + + // Delete the project + p := &Project{ID: 36} + err = p.Delete(s, usr) + require.NoError(t, err) + + // Counter row should be gone + has, err = s.Get(&ProjectTaskCounter{ProjectID: 36}) + require.NoError(t, err) + assert.False(t, has) + + err = s.Commit() + require.NoError(t, err) +}) +``` + +Note: You'll need to pick a suitable project ID from the fixtures that the test user owns and can delete. Adjust the project ID based on the available fixtures. + +**Step 2: Run the test to verify it fails** + +Run: `mage test:filter "deleting a project should clean up its task counter"` + +Expected: FAIL — counter row is not cleaned up yet. + +**Step 3: Add cleanup to project Delete** + +In `pkg/models/project.go`, in the `Delete` method, add the counter cleanup. Find the section that deletes related project entities (around line 1234) and add before the project itself is deleted. Guard it with a PostgreSQL check: + +```go + if db.Type() == schemas.POSTGRES { + _, err = s.Where("project_id = ?", p.ID).Delete(&ProjectTaskCounter{}) + if err != nil { + return + } + } +``` + +Add it just before the line `// Delete the project` (around line 1249). + +You'll need to add imports for `"code.vikunja.io/api/pkg/db"` and `"xorm.io/xorm/schemas"` if not already present in `project.go`. + +**Step 4: Run the test to verify it passes** + +Run: `mage test:filter "deleting a project should clean up its task counter"` + +Expected: PASS. + +**Step 5: Commit** + +```bash +git add pkg/models/project.go pkg/models/project_test.go +git commit -m "fix(projects): clean up project_task_counters row on project deletion on postgres" +``` + +--- + +### Task 6: Remove frontend bulk index pre-calculation + +**Files:** +- Modify: `frontend/src/components/tasks/AddTask.vue:136-196` + +**Step 1: Remove the index pre-calculation logic** + +The frontend currently queries the newest task per project and pre-calculates indexes for bulk task creation. This is no longer needed — the backend always assigns the correct index (atomically on PostgreSQL, sequentially on other DBs). + +In `frontend/src/components/tasks/AddTask.vue`, simplify the `addTask()` function. + +Remove the `projectIndices` Map and the loop that fetches the newest task per project (lines ~144-169): + +```typescript +// DELETE THIS BLOCK: +const taskCollectionService = new TaskService() +const projectIndices = new Map() + +let currentProjectId = authStore.settings.defaultProjectId +if (typeof router.currentRoute.value.params.projectId !== 'undefined') { + currentProjectId = Number(router.currentRoute.value.params.projectId) +} + +// Create a map of project indices before creating tasks +if (tasksToCreate.length > 1) { + for (const {project} of tasksToCreate) { + const projectId = project !== null + ? await taskStore.findProjectId({project, projectId: 0}) + : currentProjectId + + if (!projectIndices.has(projectId)) { + const newestTask = await taskCollectionService.getAll(new TaskModel({}), { + sort_by: ['id'], + order_by: ['desc'], + per_page: 1, + filter: `project_id = ${projectId}`, + }) + projectIndices.set(projectId, newestTask[0]?.index || 0) + } + } +} +``` + +Keep the `currentProjectId` variable (it's still used below) — just move it up above the deleted block. Also remove the `taskCollectionService` usage here (but check if it's used elsewhere in the function — if not, remove its instantiation). + +Then simplify the task creation map (lines ~171-196). Remove the index calculation: + +```typescript +// REPLACE the newTasks map with this simplified version: +const newTasks = tasksToCreate.map(async ({title, project}, index) => { + if (title === '') { + return + } + + const projectId = project !== null + ? await taskStore.findProjectId({project, projectId: 0}) + : currentProjectId + + const task = await taskStore.createNewTask({ + title, + projectId: projectId || authStore.settings.defaultProjectId, + position: props.defaultPosition, + }) + createdTasks[title] = task + return task +}) +``` + +The key change: we no longer pass `index: taskIndex` to `createNewTask`. The backend assigns the index. + +Also remove the `TaskService` import at the top of the `