diff --git a/server/src/nest/collections/collections.controller.ts b/server/src/nest/collections/collections.controller.ts index d9505468..450e26bd 100644 --- a/server/src/nest/collections/collections.controller.ts +++ b/server/src/nest/collections/collections.controller.ts @@ -33,6 +33,8 @@ import { isDemoEmail } from '../../services/demo'; import { CollectionCreateDto, CollectionUpdateDto, + CollectionReorderDto, + CollectionDeleteManyDto, CollectionSavePlaceDto, CollectionSaveFromTripDto, CollectionSaveFromTripManyDto, @@ -97,11 +99,8 @@ export class CollectionsController { @Post('reorder') @HttpCode(200) - reorder(@CurrentUser() user: User, @Body('orderedIds') orderedIds: unknown) { - if (!Array.isArray(orderedIds) || !orderedIds.every((v) => Number.isFinite(Number(v)))) { - throw new HttpException({ error: 'orderedIds must be an array of numbers' }, 400); - } - this.collections.reorderCollections(user.id, orderedIds.map(Number)); + reorder(@CurrentUser() user: User, @Body() body: CollectionReorderDto) { + this.collections.reorderCollections(user.id, body.orderedIds); return { success: true }; } @@ -126,11 +125,8 @@ export class CollectionsController { @Post('places/delete-many') @HttpCode(200) - deleteMany(@CurrentUser() user: User, @Body('ids') ids: unknown, @Headers('x-socket-id') socketId?: string) { - if (!Array.isArray(ids) || !ids.every((v) => Number.isFinite(Number(v)))) { - throw new HttpException({ error: 'ids must be an array of numbers' }, 400); - } - return { deleted: this.collections.deletePlacesMany(user.id, ids.map(Number), socketId) }; + deleteMany(@CurrentUser() user: User, @Body() body: CollectionDeleteManyDto, @Headers('x-socket-id') socketId?: string) { + return { deleted: this.collections.deletePlacesMany(user.id, body.ids, socketId) }; } @Patch('places/:pid') diff --git a/server/src/nest/collections/collections.dto.ts b/server/src/nest/collections/collections.dto.ts index 4b4bf2d4..15e966d7 100644 --- a/server/src/nest/collections/collections.dto.ts +++ b/server/src/nest/collections/collections.dto.ts @@ -2,6 +2,8 @@ import { createZodDto } from 'nestjs-zod'; import { collectionCreateRequestSchema, collectionUpdateRequestSchema, + collectionReorderRequestSchema, + collectionDeleteManyRequestSchema, collectionSavePlaceRequestSchema, collectionSaveFromTripRequestSchema, collectionSaveFromTripManyRequestSchema, @@ -27,6 +29,8 @@ import { export class CollectionCreateDto extends createZodDto(collectionCreateRequestSchema) {} export class CollectionUpdateDto extends createZodDto(collectionUpdateRequestSchema) {} +export class CollectionReorderDto extends createZodDto(collectionReorderRequestSchema) {} +export class CollectionDeleteManyDto extends createZodDto(collectionDeleteManyRequestSchema) {} export class CollectionSavePlaceDto extends createZodDto(collectionSavePlaceRequestSchema) {} export class CollectionSaveFromTripDto extends createZodDto(collectionSaveFromTripRequestSchema) {} export class CollectionSaveFromTripManyDto extends createZodDto(collectionSaveFromTripManyRequestSchema) {} diff --git a/server/src/nest/common/body-contract-allow-list.ts b/server/src/nest/common/body-contract-allow-list.ts index b870cc80..03db7f94 100644 --- a/server/src/nest/common/body-contract-allow-list.ts +++ b/server/src/nest/common/body-contract-allow-list.ts @@ -51,8 +51,6 @@ export const BODY_CONTRACT_ALLOW_LIST: string[] = [ 'BookingImportController.previewAsync', 'CategoriesController.create', 'CategoriesController.update', - 'CollectionsController.deleteMany', - 'CollectionsController.reorder', 'ImmichMemoriesController.putSettings', 'ImmichMemoriesController.search', 'ImmichMemoriesController.test', diff --git a/server/tests/unit/nest/collections.controller.test.ts b/server/tests/unit/nest/collections.controller.test.ts index 745e6f4a..fce83eca 100644 --- a/server/tests/unit/nest/collections.controller.test.ts +++ b/server/tests/unit/nest/collections.controller.test.ts @@ -68,15 +68,12 @@ describe('CollectionsController', () => { }); describe('reorder', () => { - it('400 when orderedIds is not an array of numbers', () => { - expect(thrown(() => new CollectionsController(makeService()).reorder(user, 'nope' as never))) - .toEqual({ status: 400, body: { error: 'orderedIds must be an array of numbers' } }); - expect(thrown(() => new CollectionsController(makeService()).reorder(user, [1, 'x'] as never))) - .toEqual({ status: 400, body: { error: 'orderedIds must be an array of numbers' } }); - }); + // The legacy 'orderedIds must be an array of numbers' 400 is gone: + // collectionReorderRequestSchema types the array, so the pipe rejects a + // bad payload before the handler runs. it('reorders a valid array', () => { const svc = makeService(); - expect(new CollectionsController(svc).reorder(user, [3, 1, 2] as never)).toEqual({ success: true }); + expect(new CollectionsController(svc).reorder(user, { orderedIds: [3, 1, 2] } as never)).toEqual({ success: true }); expect(svc.reorderCollections).toHaveBeenCalledWith(1, [3, 1, 2]); }); }); @@ -98,11 +95,12 @@ describe('CollectionsController', () => { expect(c.copyToTrip(user, { trip_id: 5, place_ids: [9] } as never)).toEqual({ copied: 1, skipped: [] }); }); - it('deleteMany 400 on a bad payload, deletes a valid one', () => { - expect(thrown(() => new CollectionsController(makeService()).deleteMany(user, { nope: 1 } as never))) - .toEqual({ status: 400, body: { error: 'ids must be an array of numbers' } }); + // The legacy 'ids must be an array of numbers' 400 is gone: + // collectionDeleteManyRequestSchema types the array, so the pipe rejects a + // bad payload before the handler runs. + it('deleteMany deletes a valid list', () => { const svc = makeService(); - expect(new CollectionsController(svc).deleteMany(user, [1, 2] as never, 'sid')).toEqual({ deleted: [1, 2] }); + expect(new CollectionsController(svc).deleteMany(user, { ids: [1, 2] } as never, 'sid')).toEqual({ deleted: [1, 2] }); expect(svc.deletePlacesMany).toHaveBeenCalledWith(1, [1, 2], 'sid'); }); }); diff --git a/shared/src/collection/collection.schema.spec.ts b/shared/src/collection/collection.schema.spec.ts index 68935951..fb2f0703 100644 --- a/shared/src/collection/collection.schema.spec.ts +++ b/shared/src/collection/collection.schema.spec.ts @@ -1,4 +1,8 @@ -import { collectionPlaceUpdateRequestSchema } from './collection.schema'; +import { + collectionDeleteManyRequestSchema, + collectionPlaceUpdateRequestSchema, + collectionReorderRequestSchema, +} from './collection.schema'; import { describe, expect, it } from 'vitest'; @@ -20,3 +24,20 @@ describe('collectionPlaceUpdateRequestSchema', () => { expect(collectionPlaceUpdateRequestSchema.parse({ status: 'bogus' as never }).status).toBe('idea'); }); }); + +// The two ratchet schemas replace hand-rolled "array of numbers" checks — they +// must stay exactly that permissive (empty arrays included, no int/positive). +describe('collectionReorderRequestSchema / collectionDeleteManyRequestSchema', () => { + it('accepts any array of numbers, empty included', () => { + expect(collectionReorderRequestSchema.parse({ orderedIds: [] }).orderedIds).toEqual([]); + expect(collectionReorderRequestSchema.parse({ orderedIds: [3, 1, 2] }).orderedIds).toEqual([3, 1, 2]); + expect(collectionDeleteManyRequestSchema.parse({ ids: [] }).ids).toEqual([]); + expect(collectionDeleteManyRequestSchema.parse({ ids: [7] }).ids).toEqual([7]); + }); + + it('rejects non-arrays and non-numeric members', () => { + expect(() => collectionReorderRequestSchema.parse({ orderedIds: 'nope' })).toThrow(); + expect(() => collectionReorderRequestSchema.parse({})).toThrow(); + expect(() => collectionDeleteManyRequestSchema.parse({ ids: ['a'] })).toThrow(); + }); +}); diff --git a/shared/src/collection/collection.schema.ts b/shared/src/collection/collection.schema.ts index 3845fcb8..a8e9623e 100644 --- a/shared/src/collection/collection.schema.ts +++ b/shared/src/collection/collection.schema.ts @@ -128,6 +128,14 @@ export const collectionUpdateRequestSchema = collectionCreateRequestSchema.parti }); export type CollectionUpdateRequest = z.infer; +/** Reorder the caller's lists — every visible collection id in the desired order. + * Plain z.number() (no .min/.int) mirrors the legacy hand-rolled check the DTO + * ratchet replaced: any array of numbers, empty included. */ +export const collectionReorderRequestSchema = z.object({ + orderedIds: z.array(z.number()), +}); +export type CollectionReorderRequest = z.infer; + /** Save a place into a list from a raw maps/manual payload (or carrying provenance). */ export const collectionSavePlaceRequestSchema = z.object({ collection_id: z.number(), @@ -198,6 +206,13 @@ export type CollectionPlaceUpdateRequest = z.infer; +/** Bulk-delete saved places. Plain z.number() (no .min/.int) mirrors the legacy + * hand-rolled check the DTO ratchet replaced (same shape as placeBulkDeleteRequestSchema). */ +export const collectionDeleteManyRequestSchema = z.object({ + ids: z.array(z.number()), +}); +export type CollectionDeleteManyRequest = z.infer; + /** Copy one or many saved places INTO a trip (dedup precheck on server). */ export const collectionCopyToTripRequestSchema = z.object({ trip_id: z.number(),