diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index a280e0adc..a68160fa3 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/server/src/nest/collections/collections.service.ts @@ -133,11 +133,8 @@ export class CollectionsService { @InjectRepository(CollectionMembers) private readonly members: CollectionMembersRepository, @InjectRepository(CollectionLabels) private readonly labels: CollectionLabelsRepository, @InjectRepository(Categories) private readonly categories: CategoriesRepository, - // Plan 3h Task 2 (part B) — savePlace onward. `collectionPlaces`/ - // `collectionPlaceRatings` own this service's own two remaining tables; - // `trips`/`tripMembers`/`tripPlaces`/`tripPlaceRatings`/`tags`/`users` - // are 3b/3c's, injected directly (AP1-AP6 each become a - // `TripsRepository.findAccessible` call, replacing `this.db.canAccessTrip`). + // The saved places and their ratings are this service's own; the trip-side + // repositories below are injected directly for the copy-to-trip paths. @InjectRepository(CollectionPlaces) private readonly collectionPlaces: CollectionPlacesRepository, @InjectRepository(CollectionPlaceRatings) private readonly collectionPlaceRatings: CollectionPlaceRatingsRepository, @InjectRepository(Trips) private readonly trips: TripsRepository, @@ -594,8 +591,9 @@ export class CollectionsService { } async createCollection(userId: number, body: CollectionCreateRequest): Promise { - const max = await this.collectionsRepo.maxSortOrder(userId); - const id = await this.collectionsRepo.insertCollection({ + // The MAX read and the insert in one transaction: two lists created at once + // cannot take the same position. + const id = await this.uow.transactional(async () => this.collectionsRepo.insertCollection({ owner_id: userId, name: body.name, description: body.description ?? null, @@ -603,8 +601,8 @@ export class CollectionsService { icon: body.icon ?? 'Bookmark', cover_image: body.cover_image ?? null, links: serializeLinks(body.links), - sort_order: max + 1, - }); + sort_order: (await this.collectionsRepo.maxSortOrder(userId)) + 1, + })); const col = await this.getCollectionRow(id); return { ...col, is_owner: true }; } @@ -1286,13 +1284,15 @@ export class CollectionsService { await this.assertCanEdit(userId, collectionId); const trimmed = name.trim(); if (!trimmed) httpError(400, 'Label name is required'); - const count = await this.labels.countByCollection(collectionId); - if (count >= MAX_LABELS_PER_COLLECTION) httpError(400, `A list can have at most ${MAX_LABELS_PER_COLLECTION} labels`); - if (await this.labels.nameExists(collectionId, trimmed)) { - httpError(409, 'A label with this name already exists'); - } - const nextSort = (await this.labels.maxSortOrder(collectionId)) + 1; - const newId = await this.labels.insertLabel({ collection_id: collectionId, name: trimmed, color: color ?? '#6366f1', sort_order: nextSort }); + // The limit, the name check, the position and the insert in one transaction, + // so two labels added at once can neither both pass the checks nor share a position. + const newId = await this.uow.transactional(async () => { + const count = await this.labels.countByCollection(collectionId); + if (count >= MAX_LABELS_PER_COLLECTION) httpError(400, `A list can have at most ${MAX_LABELS_PER_COLLECTION} labels`); + if (await this.labels.nameExists(collectionId, trimmed)) httpError(409, 'A label with this name already exists'); + const nextSort = (await this.labels.maxSortOrder(collectionId)) + 1; + return await this.labels.insertLabel({ collection_id: collectionId, name: trimmed, color: color ?? '#6366f1', sort_order: nextSort }); + }); await this.notifyCollectionUsers(collectionId, socketId, 'collections:updated'); return this.getLabelById(newId); } diff --git a/server/src/nest/days/days.service.ts b/server/src/nest/days/days.service.ts index 372df8ee6..3a4b4769c 100644 --- a/server/src/nest/days/days.service.ts +++ b/server/src/nest/days/days.service.ts @@ -304,18 +304,12 @@ export class DaysService { // of defect `PlacesService.create` was fixed for (A-M1). const tripIdNum = toRowId(tripId); if (tripIdNum === null) throw new HttpException({ error: 'Trip not found' }, 404); - // DY5: `DaysRepository.maxDayNumber` already folds the no-rows case to 0 - // (`?? 0`); the `|| 0` here is the legacy expression kept verbatim so a - // stored 0 still becomes 1 — the same outcome either fold produces, but - // the ruling is to keep the literal formula in the service. - const maxDayNumber = await this.daysRepo.maxDayNumber(tripIdNum); - const dayNumber = (maxDayNumber || 0) + 1; - - const day = await this.daysRepo.createDay({ - trip_id: tripIdNum, - day_number: dayNumber, - date: date || null, - notes: notes || null, + // DY5: the next day number and the row that takes it in one transaction, so + // two days added at once cannot both read the same MAX. `|| 0` keeps a stored + // 0 becoming 1, as the legacy formula did. + const day = await this.uow.transactional(async () => { + const dayNumber = ((await this.daysRepo.maxDayNumber(tripIdNum)) || 0) + 1; + return await this.daysRepo.createDay({ trip_id: tripIdNum, day_number: dayNumber, date: date || null, notes: notes || null }); }); return { ...day, assignments: [] }; } diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 2d4a47969..8b62d047a 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -277,12 +277,13 @@ export class PackingService { ownerId?: number, ) { if (data.bag_id != null && !(await this.bagInTrip(tripId, data.bag_id))) return { invalidBag: true } as const; - const maxOrder = await this.itemsRepo.maxSortOrder(tripId); - const sortOrder = (maxOrder !== null ? maxOrder : -1) + 1; const qty = Math.max(1, Math.min(999, Number(data.quantity) || 1)); const isPrivate = this.visibilityToPrivate(data.visibility, data.is_private); + // The MAX read and the insert share the transaction, so two items added at + // once cannot take the same position. const itemId = await this.uow.transactional(async () => { + const sortOrder = ((await this.itemsRepo.maxSortOrder(tripId)) ?? -1) + 1; const id = await this.itemsRepo.insertItem({ trip_id: tripId, name: data.name, @@ -490,12 +491,11 @@ export class PackingService { // ── Bulk Import ──────────────────────────────────────────────────────────── async bulkImport(tripId: string | number, items: ImportItem[], ownerId?: number) { - const maxOrder = await this.itemsRepo.maxSortOrder(tripId); - let sortOrder = (maxOrder !== null ? maxOrder : -1) + 1; - const created: any[] = []; await this.uow.transactional(async () => { + // Read inside the transaction, like createItem. + let sortOrder = ((await this.itemsRepo.maxSortOrder(tripId)) ?? -1) + 1; for (const item of items) { if (!item.name?.trim()) continue; const checked = item.checked ? 1 : 0; @@ -615,13 +615,15 @@ export class PackingService { } async createBag(tripId: string | number, data: { name: string; color?: string; weight_limit_grams?: number | null }) { - const maxOrder = await this.bagsRepo.maxSortOrder(tripId); - const newId = await this.bagsRepo.insertBag({ - trip_id: tripId, - name: data.name.trim(), - color: data.color || '#6366f1', - sort_order: (maxOrder ?? -1) + 1, - weight_limit_grams: data.weight_limit_grams ?? null, + const newId = await this.uow.transactional(async () => { + const maxOrder = await this.bagsRepo.maxSortOrder(tripId); + return await this.bagsRepo.insertBag({ + trip_id: tripId, + name: data.name.trim(), + color: data.color || '#6366f1', + sort_order: (maxOrder ?? -1) + 1, + weight_limit_grams: data.weight_limit_grams ?? null, + }); }); return await this.bagsRepo.findById(newId); } @@ -682,13 +684,13 @@ export class PackingService { if (templateItems.length === 0) return null; - const maxOrder = await this.itemsRepo.maxSortOrder(tripId); - let sortOrder = (maxOrder !== null ? maxOrder : -1) + 1; const isPrivate = ownerId != null ? this.visibilityToPrivate(visibility) : 0; const owner = isPrivate ? ownerId! : null; const added: any[] = []; await this.uow.transactional(async () => { + // Read inside the transaction, like createItem. + let sortOrder = ((await this.itemsRepo.maxSortOrder(tripId)) ?? -1) + 1; for (const ti of templateItems) { // Weight, count and bag ride along since #1131; a bag the trip lacks is // created, the way the import does it. @@ -837,8 +839,10 @@ export class PackingService { if (!name?.trim()) return { error: 'Category name is required', status: 400 }; const template = await this.templatesRepo.findById(templateId); if (!template) return { error: 'Template not found', status: 404 }; - const maxOrder = await this.templateCategoriesRepo.maxSortOrder(templateId); - const newId = await this.templateCategoriesRepo.insertCategory(templateId, name.trim(), (maxOrder ?? -1) + 1); + const newId = await this.uow.transactional(async () => { + const maxOrder = await this.templateCategoriesRepo.maxSortOrder(templateId); + return await this.templateCategoriesRepo.insertCategory(templateId, name.trim(), (maxOrder ?? -1) + 1); + }); return { category: await this.templateCategoriesRepo.findById(newId) }; } @@ -863,8 +867,10 @@ export class PackingService { if (!name?.trim()) return { error: 'Item name is required', status: 400 }; const cat = await this.templateCategoriesRepo.findInTemplate(catId, templateId); if (!cat) return { error: 'Category not found', status: 404 }; - const maxOrder = await this.templateItemsRepo.maxSortOrder(catId); - const newId = await this.templateItemsRepo.insertTemplateItem({ category_id: catId, name: name.trim(), sort_order: (maxOrder ?? -1) + 1 }); + const newId = await this.uow.transactional(async () => { + const maxOrder = await this.templateItemsRepo.maxSortOrder(catId); + return await this.templateItemsRepo.insertTemplateItem({ category_id: catId, name: name.trim(), sort_order: (maxOrder ?? -1) + 1 }); + }); return { item: await this.templateItemsRepo.findById(newId) }; } diff --git a/server/src/nest/roadtrip/roadtrip.service.ts b/server/src/nest/roadtrip/roadtrip.service.ts index 7911da412..116820306 100644 --- a/server/src/nest/roadtrip/roadtrip.service.ts +++ b/server/src/nest/roadtrip/roadtrip.service.ts @@ -107,15 +107,17 @@ export class RoadtripService { async create(dayId: string | number, input: { after_order_index: number; lat: number; lng: number; sequence?: number }): Promise { const dayIdNum = this.requireDayId(dayId); // Appended after whatever already follows that stop, unless the caller says where. - // RT4 (`nextSequence`) then RT5 (`insertVia`), un-transacted (R7 — pin, don't fix): - // two concurrent adds on one leg can read the same next sequence. - const sequence = input.sequence ?? (await this.viasRepo.nextSequence(dayIdNum, input.after_order_index)); - const id = await this.viasRepo.insertVia({ - day_id: dayIdNum, - after_order_index: input.after_order_index, - sequence, - lat: input.lat, - lng: input.lng, + // RT4 (`nextSequence`) and RT5 (`insertVia`) in one transaction, so two adds on + // one leg at once cannot read the same next sequence. + const id = await this.uow.transactional(async () => { + const sequence = input.sequence ?? (await this.viasRepo.nextSequence(dayIdNum, input.after_order_index)); + return await this.viasRepo.insertVia({ + day_id: dayIdNum, + after_order_index: input.after_order_index, + sequence, + lat: input.lat, + lng: input.lng, + }); }); return (await this.viasRepo.findById(id))!; } diff --git a/server/src/nest/todo/todo.service.ts b/server/src/nest/todo/todo.service.ts index 527e29f42..da07ba2ba 100644 --- a/server/src/nest/todo/todo.service.ts +++ b/server/src/nest/todo/todo.service.ts @@ -64,13 +64,15 @@ export class TodoService { async createItem(tripId: string | number, data: { name: string; category?: string | null; due_date?: string | null; description?: string | null; assigned_user_id?: number | null; priority?: number; }) { - const maxOrder = await this.todoItemsRepo.maxSortOrder(tripId); - const sortOrder = (maxOrder !== null ? maxOrder : -1) + 1; - - const id = await this.todoItemsRepo.insertItem({ - trip_id: tripId, name: data.name, category: data.category || null, sort_order: sortOrder, - due_date: data.due_date || null, description: data.description || null, - assigned_user_id: data.assigned_user_id || null, priority: data.priority || 0, + // The next position and the row that takes it in one transaction, so two items + // added at once cannot both read the same MAX. + const id = await this.uow.transactional(async () => { + const maxOrder = await this.todoItemsRepo.maxSortOrder(tripId); + return await this.todoItemsRepo.insertItem({ + trip_id: tripId, name: data.name, category: data.category || null, sort_order: (maxOrder ?? -1) + 1, + due_date: data.due_date || null, description: data.description || null, + assigned_user_id: data.assigned_user_id || null, priority: data.priority || 0, + }); }); return this.todoItemsRepo.findById(id); diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index a548619e0..5260f080f 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -146,6 +146,23 @@ describe('collections CRUD + visibility', () => { expect((await svc.listCollections(b.id)).collections).toHaveLength(0); }); + it('COLLECTIONS-SVC-001b: lists and labels created at the same time take one position each', async () => { + const a = createUser(testDb).user; + // Without the transaction every one read the same MAX and they shared a position. + const lists = await Promise.all(['A', 'B', 'C'].map((name) => svc.createCollection(a.id, { name }))); + expect(lists.map((l) => l.sort_order).sort((x, y) => x - y)).toEqual([0, 1, 2]); + const labels = await Promise.all(['Food', 'Sights', 'Bars'].map((name) => svc.createLabel(a.id, lists[0].id, name))); + expect(labels.map((l) => l.sort_order).sort((x, y) => x - y)).toEqual([0, 1, 2]); + }); + + it('COLLECTIONS-SVC-001c: two labels of one name created at the same time: one lands, the other is a 409', async () => { + const a = createUser(testDb).user; + const list = await svc.createCollection(a.id, { name: 'Tokyo' }); + const results = await Promise.allSettled([svc.createLabel(a.id, list.id, 'Food'), svc.createLabel(a.id, list.id, 'Food')]); + expect(results.filter((r) => r.status === 'fulfilled')).toHaveLength(1); + expect(results.find((r) => r.status === 'rejected')).toMatchObject({ reason: { status: 409 } }); + }); + it('COLLECTIONS-SVC-002: getCollection 404 for a non-member', async () => { const a = createUser(testDb).user; const b = createUser(testDb).user; diff --git a/server/tests/unit/nest/days.service.test.ts b/server/tests/unit/nest/days.service.test.ts index d0dc4933a..ceeb918b7 100644 --- a/server/tests/unit/nest/days.service.test.ts +++ b/server/tests/unit/nest/days.service.test.ts @@ -241,6 +241,16 @@ describe('create (service)', () => { expect(d2.day_number).toBe(2); }); + it('DAY-SVC-009b: days added at the same time take one day number each', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + // Without the transaction both read the same MAX and both became day 1. + await Promise.all([svc.create(trip.id), svc.create(trip.id), svc.create(trip.id)]); + const numbers = (testDb.prepare('SELECT day_number FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { day_number: number }[]) + .map((d) => d.day_number); + expect(numbers).toEqual([1, 2, 3]); + }); + it('DAY-SVC-010 — returns day with empty assignments array', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index e81adc1d3..adc1e816f 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -224,6 +224,22 @@ function seedTemplate(userId: number, itemNames: string[]): number { return templateId; } +describe('template positions', () => { + it('PACK-SVC-002b: template categories and items added at the same time take one position each', async () => { + const { user } = createUser(testDb); + const templateId = seedTemplate(user.id, ['Tent']); + // seedTemplate's own category holds position 0 and its item position 0. + const categories = await Promise.all(['Kitchen', 'Sleep', 'Clothes'].map((name) => svc.createTemplateCategory(String(templateId), name))); + const catIds = categories.map((c) => (c as { category: { id: number } }).category.id); + expect((testDb.prepare('SELECT sort_order FROM packing_template_categories WHERE template_id = ? ORDER BY sort_order').all(templateId) as { sort_order: number }[]) + .map((r) => r.sort_order)).toEqual([0, 1, 2, 3]); + + await Promise.all(['Stove', 'Pot', 'Lighter'].map((name) => svc.createTemplateItem(String(templateId), String(catIds[0]), name))); + expect((testDb.prepare('SELECT sort_order FROM packing_template_items WHERE category_id = ? ORDER BY sort_order').all(catIds[0]) as { sort_order: number }[]) + .map((r) => r.sort_order)).toEqual([0, 1, 2]); + }); +}); + describe('applyTemplate', () => { it('PACK-SVC-003: adds template items to a trip packing list', async () => { const { user } = createUser(testDb); @@ -332,6 +348,18 @@ describe('createBag / deleteBag', () => { expect(bag.name).toBe('Carry-On'); }); + it('PACK-SVC-005b: bags and items created at the same time take one position each', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + // Without the transaction every one of them read the same MAX and landed on 0. + await Promise.all(['Carry-On', 'Checked', 'Daypack'].map((name) => svc.createBag(trip.id, { name }))); + await Promise.all(['Tent', 'Stove', 'Mat'].map((name) => svc.createItem(trip.id, { name }, user.id))); + const orders = (table: string) => (testDb.prepare(`SELECT sort_order FROM ${table} WHERE trip_id = ? ORDER BY sort_order`).all(trip.id) as { sort_order: number }[]) + .map((r) => r.sort_order); + expect(orders('packing_bags')).toEqual([0, 1, 2]); + expect(orders('packing_items')).toEqual([0, 1, 2]); + }); + it('PACK-SVC-006: deleteBag removes the bag and returns true', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/roadtrip.service.test.ts b/server/tests/unit/nest/roadtrip.service.test.ts index 530e170d0..d27994e16 100644 --- a/server/tests/unit/nest/roadtrip.service.test.ts +++ b/server/tests/unit/nest/roadtrip.service.test.ts @@ -326,15 +326,11 @@ describe('RoadtripService', () => { expect(await service.tracksForTrip(f.tripA.id)).toEqual([]); }); - // R7 (Plan 3d Task 7 whole-plan review, item 12 — flag + pin, not fix, the - // MEMBERS-SVC-018 shape): `create`'s `nextSequence` (MAX(sequence)+1) READ - // and its `insertVia` WRITE are two separate statements with no lock - // between them. Measured under real concurrency (`Promise.allSettled`, no - // mocks): both calls succeed — there is no UNIQUE constraint to lose a - // race on, unlike `trip_members` — but both land on the SAME `sequence` - // (`0`), a genuine duplicate the un-serialized read-then-write leaves - // behind. Today's actual outcome, pinned; not a fix. - it('R7: two concurrent creates on the same leg both succeed but can land on the SAME sequence (unserialized nextSequence read)', async () => { + // R7: `create`'s `nextSequence` (MAX(sequence)+1) read and its `insertVia` + // write run in one transaction, which holds the connection, so two concurrent + // creates (`Promise.allSettled`, no mocks) take one sequence each. Before, + // both landed on sequence 0. + it('R7: two concurrent creates on the same leg both succeed and take consecutive sequences', async () => { const results = await Promise.allSettled([ service.create(f.dayA1.id, { after_order_index: 0, lat: 1, lng: 1 }), service.create(f.dayA1.id, { after_order_index: 0, lat: 2, lng: 2 }), @@ -342,8 +338,7 @@ describe('RoadtripService', () => { expect(results.every((r) => r.status === 'fulfilled')).toBe(true); const rows = await service.listForDay(f.dayA1.id); expect(rows).toHaveLength(2); - // Both landed on the same leg, at the same sequence — the duplicate R7 flags. expect(rows.map((r) => r.after_order_index)).toEqual([0, 0]); - expect(rows.map((r) => r.sequence)).toEqual([0, 0]); + expect(rows.map((r) => r.sequence).sort((a, b) => a - b)).toEqual([0, 1]); }); }); diff --git a/server/tests/unit/nest/todo.service.test.ts b/server/tests/unit/nest/todo.service.test.ts index fb1abb8e3..ff8a56ff8 100644 --- a/server/tests/unit/nest/todo.service.test.ts +++ b/server/tests/unit/nest/todo.service.test.ts @@ -128,6 +128,16 @@ describe('listItems and createItem', () => { expect(b.sort_order).toBe(a.sort_order + 1); }); + it('TODO-SVC-006b: items created at the same time still take one position each', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + // Without the transaction both read the same MAX and both landed on 0. + await Promise.all(['A', 'B', 'C'].map((name) => svc.createItem(trip.id, { name }))); + const orders = (testDb.prepare('SELECT sort_order FROM todo_items WHERE trip_id = ? ORDER BY sort_order').all(trip.id) as { sort_order: number }[]) + .map((r) => r.sort_order); + expect(orders).toEqual([0, 1, 2]); + }); + it('TODO-SVC-007: createItem stores optional fields', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id);