From fe91b44ab9f5b10db57a55df9bd24df943dc6f09 Mon Sep 17 00:00:00 2001 From: Zack Pollard Date: Thu, 19 Jun 2025 17:05:03 +0100 Subject: [PATCH] fix: people ordering incorrect (#19298) --- e2e/src/api/specs/person.e2e-spec.ts | 83 +++++++++++++++----- server/src/queries/person.repository.sql | 31 ++++++++ server/src/repositories/person.repository.ts | 3 +- 3 files changed, 98 insertions(+), 19 deletions(-) diff --git a/e2e/src/api/specs/person.e2e-spec.ts b/e2e/src/api/specs/person.e2e-spec.ts index a56118a68b..bc88dbdb03 100644 --- a/e2e/src/api/specs/person.e2e-spec.ts +++ b/e2e/src/api/specs/person.e2e-spec.ts @@ -14,7 +14,11 @@ describe('/people', () => { let nameAlicePerson: PersonResponseDto; let nameBobPerson: PersonResponseDto; let nameCharliePerson: PersonResponseDto; - let nameNullPerson: PersonResponseDto; + let nameNullPerson4Assets: PersonResponseDto; + let nameNullPerson3Assets: PersonResponseDto; + let nameNullPerson1Asset: PersonResponseDto; + let nameBillPersonFavourite: PersonResponseDto; + let nameFreddyPersonFavourite: PersonResponseDto; beforeAll(async () => { await utils.resetDatabase(); @@ -27,7 +31,11 @@ describe('/people', () => { nameCharliePerson, nameBobPerson, nameAlicePerson, - nameNullPerson, + nameNullPerson4Assets, + nameNullPerson3Assets, + nameNullPerson1Asset, + nameBillPersonFavourite, + nameFreddyPersonFavourite, ] = await Promise.all([ utils.createPerson(admin.accessToken, { name: 'visible_person', @@ -52,11 +60,26 @@ describe('/people', () => { utils.createPerson(admin.accessToken, { name: '', }), + utils.createPerson(admin.accessToken, { + name: '', + }), + utils.createPerson(admin.accessToken, { + name: '', + }), + utils.createPerson(admin.accessToken, { + name: 'Bill', + isFavorite: true, + }), + utils.createPerson(admin.accessToken, { + name: 'Freddy', + isFavorite: true, + }), ]); const asset1 = await utils.createAsset(admin.accessToken); const asset2 = await utils.createAsset(admin.accessToken); const asset3 = await utils.createAsset(admin.accessToken); + const asset4 = await utils.createAsset(admin.accessToken); await Promise.all([ utils.createFace({ assetId: asset1.id, personId: visiblePerson.id }), @@ -64,15 +87,27 @@ describe('/people', () => { utils.createFace({ assetId: asset1.id, personId: multipleAssetsPerson.id }), utils.createFace({ assetId: asset1.id, personId: multipleAssetsPerson.id }), utils.createFace({ assetId: asset2.id, personId: multipleAssetsPerson.id }), - utils.createFace({ assetId: asset3.id, personId: multipleAssetsPerson.id }), + utils.createFace({ assetId: asset3.id, personId: multipleAssetsPerson.id }), // 4 assets // Named persons utils.createFace({ assetId: asset1.id, personId: nameCharliePerson.id }), // 1 asset utils.createFace({ assetId: asset1.id, personId: nameBobPerson.id }), utils.createFace({ assetId: asset2.id, personId: nameBobPerson.id }), // 2 assets utils.createFace({ assetId: asset1.id, personId: nameAlicePerson.id }), // 1 asset - // Null-named person - utils.createFace({ assetId: asset1.id, personId: nameNullPerson.id }), - utils.createFace({ assetId: asset2.id, personId: nameNullPerson.id }), // 2 assets + // Null-named person 4 assets + utils.createFace({ assetId: asset1.id, personId: nameNullPerson4Assets.id }), + utils.createFace({ assetId: asset2.id, personId: nameNullPerson4Assets.id }), + utils.createFace({ assetId: asset3.id, personId: nameNullPerson4Assets.id }), + utils.createFace({ assetId: asset4.id, personId: nameNullPerson4Assets.id }), // 4 assets + // Null-named person 3 assets + utils.createFace({ assetId: asset1.id, personId: nameNullPerson3Assets.id }), + utils.createFace({ assetId: asset2.id, personId: nameNullPerson3Assets.id }), + utils.createFace({ assetId: asset3.id, personId: nameNullPerson3Assets.id }), // 3 assets + // Null-named person 1 asset + utils.createFace({ assetId: asset3.id, personId: nameNullPerson1Asset.id }), + // Favourite People + utils.createFace({ assetId: asset1.id, personId: nameFreddyPersonFavourite.id }), + utils.createFace({ assetId: asset2.id, personId: nameFreddyPersonFavourite.id }), + utils.createFace({ assetId: asset1.id, personId: nameBillPersonFavourite.id }), ]); }); @@ -87,15 +122,19 @@ describe('/people', () => { expect(status).toBe(200); expect(body).toEqual({ hasNextPage: false, - total: 7, + total: 11, hidden: 1, people: [ - expect.objectContaining({ name: 'multiple_assets_person' }), - expect.objectContaining({ name: 'Bob' }), + expect.objectContaining({ name: 'Bill' }), + expect.objectContaining({ name: 'Freddy' }), expect.objectContaining({ name: 'Alice' }), + expect.objectContaining({ name: 'Bob' }), expect.objectContaining({ name: 'Charlie' }), + expect.objectContaining({ name: 'multiple_assets_person' }), expect.objectContaining({ name: 'visible_person' }), - expect.objectContaining({ name: 'hidden_person' }), + expect.objectContaining({ id: nameNullPerson4Assets.id, name: '' }), + expect.objectContaining({ id: nameNullPerson3Assets.id, name: '' }), + expect.objectContaining({ name: 'hidden_person' }), // Should really be before the null names ], }); }); @@ -105,17 +144,21 @@ describe('/people', () => { expect(status).toBe(200); expect(body.hasNextPage).toBe(false); - expect(body.total).toBe(7); // All persons + expect(body.total).toBe(11); // All persons expect(body.hidden).toBe(1); // 'hidden_person' const people = body.people as PersonResponseDto[]; expect(people.map((p) => p.id)).toEqual([ - multipleAssetsPerson.id, // name: 'multiple_assets_person', count: 3 - nameBobPerson.id, // name: 'Bob', count: 2 + nameBillPersonFavourite.id, // name: 'Bill', count: 2 + nameFreddyPersonFavourite.id, // name: 'Freddy', count: 2 nameAlicePerson.id, // name: 'Alice', count: 1 + nameBobPerson.id, // name: 'Bob', count: 2 nameCharliePerson.id, // name: 'Charlie', count: 1 + multipleAssetsPerson.id, // name: 'multiple_assets_person', count: 3 visiblePerson.id, // name: 'visible_person', count: 1 + nameNullPerson4Assets.id, // name: '', count: 4 + nameNullPerson3Assets.id, // name: '', count: 3 ]); expect(people.some((p) => p.id === hiddenPerson.id)).toBe(false); @@ -127,14 +170,18 @@ describe('/people', () => { expect(status).toBe(200); expect(body).toEqual({ hasNextPage: false, - total: 7, + total: 11, hidden: 1, people: [ - expect.objectContaining({ name: 'multiple_assets_person' }), - expect.objectContaining({ name: 'Bob' }), + expect.objectContaining({ name: 'Bill' }), + expect.objectContaining({ name: 'Freddy' }), expect.objectContaining({ name: 'Alice' }), + expect.objectContaining({ name: 'Bob' }), expect.objectContaining({ name: 'Charlie' }), + expect.objectContaining({ name: 'multiple_assets_person' }), expect.objectContaining({ name: 'visible_person' }), + expect.objectContaining({ id: nameNullPerson4Assets.id, name: '' }), + expect.objectContaining({ id: nameNullPerson3Assets.id, name: '' }), ], }); }); @@ -148,9 +195,9 @@ describe('/people', () => { expect(status).toBe(200); expect(body).toEqual({ hasNextPage: true, - total: 7, + total: 11, hidden: 1, - people: [expect.objectContaining({ name: 'visible_person' })], + people: [expect.objectContaining({ name: 'Charlie' })], }); }); }); diff --git a/server/src/queries/person.repository.sql b/server/src/queries/person.repository.sql index b8da3b5ae3..a69038e741 100644 --- a/server/src/queries/person.repository.sql +++ b/server/src/queries/person.repository.sql @@ -12,6 +12,37 @@ delete from "person" where "person"."id" in ($1) +-- PersonRepository.getAllForUser +select + "person".* +from + "person" + inner join "asset_faces" on "asset_faces"."personId" = "person"."id" + inner join "assets" on "asset_faces"."assetId" = "assets"."id" + and "assets"."visibility" = 'timeline' + and "assets"."deletedAt" is null +where + "person"."ownerId" = $1 + and "asset_faces"."deletedAt" is null + and "person"."isHidden" = $2 +group by + "person"."id" +having + ( + "person"."name" != $3 + or count("asset_faces"."assetId") >= $4 + ) +order by + "person"."isHidden" asc, + "person"."isFavorite" desc, + NULLIF(person.name, '') asc nulls last, + count("asset_faces"."assetId") desc, + "person"."createdAt" +limit + $5 +offset + $6 + -- PersonRepository.getAllWithoutFaces select "person".* diff --git a/server/src/repositories/person.repository.ts b/server/src/repositories/person.repository.ts index 6633fb68ac..931eb6234b 100644 --- a/server/src/repositories/person.repository.ts +++ b/server/src/repositories/person.repository.ts @@ -138,6 +138,7 @@ export class PersonRepository { .stream(); } + @GenerateSql({ params: [{ take: 1, skip: 0 }, DummyValue.UUID] }) async getAllForUser(pagination: PaginationOptions, userId: string, options?: PersonSearchOptions) { const items = await this.db .selectFrom('person') @@ -179,8 +180,8 @@ export class PersonRepository { ) .$if(!options?.closestFaceAssetId, (qb) => qb + .orderBy(sql`NULLIF(person.name, '')`, (om) => om.asc().nullsLast()) .orderBy((eb) => eb.fn.count('asset_faces.assetId'), 'desc') - .orderBy(sql`NULLIF(person.name, '') asc nulls last`) .orderBy('person.createdAt'), ) .$if(!options?.withHidden, (qb) => qb.where('person.isHidden', '=', false))