refactor(server): use includeNull in query for search suggestions (#14626)

* use `includeNull` * push down `includeNull` into query, inner joins * remove filter * update sql * fix tests * maybe fix e2e * more e2e tests * handle no exif row * whoops * update sql
2025-01-15 08:16:48 +01:00 · 2024-12-10 16:22:37 -05:00 · 2024-12-10 16:22:37 -05:00 · 25ca3b1124
commit 25ca3b1124
parent 60c783bbe9
6 changed files with 259 additions and 52 deletions
--- a/e2e/src/api/specs/search.e2e-spec.ts
+++ b/e2e/src/api/specs/search.e2e-spec.ts
@ -98,6 +98,7 @@ describe('/search', () => {
      { latitude: 31.634_16, longitude: -7.999_94 }, // marrakesh
      { latitude: 38.523_735_4, longitude: -78.488_619_4 }, // tanners ridge
      { latitude: 59.938_63, longitude: 30.314_13 }, // st. petersburg
+      { latitude: 0, longitude: 0 }, // null island
    ];

    const updates = coordinates.map((dto, i) =>
@ -532,7 +533,7 @@ describe('/search', () => {
      expect(body).toEqual(errorDto.unauthorized);
    });

-    it('should get suggestions for country', async () => {
+    it('should get suggestions for country (including null)', async () => {
      const { status, body } = await request(app)
        .get('/search/suggestions?type=country&includeNull=true')
        .set('Authorization', `Bearer ${admin.accessToken}`);
@ -555,7 +556,29 @@ describe('/search', () => {
      expect(status).toBe(200);
    });

-    it('should get suggestions for state', async () => {
+    it('should get suggestions for country', async () => {
+      const { status, body } = await request(app)
+        .get('/search/suggestions?type=country')
+        .set('Authorization', `Bearer ${admin.accessToken}`);
+      expect(body).toEqual([
+        'Cuba',
+        'France',
+        'Georgia',
+        'Germany',
+        'Ghana',
+        'Japan',
+        'Morocco',
+        "People's Republic of China",
+        'Russian Federation',
+        'Singapore',
+        'Spain',
+        'Switzerland',
+        'United States of America',
+      ]);
+      expect(status).toBe(200);
+    });
+
+    it('should get suggestions for state (including null)', async () => {
      const { status, body } = await request(app)
        .get('/search/suggestions?type=state&includeNull=true')
        .set('Authorization', `Bearer ${admin.accessToken}`);
@ -579,7 +602,30 @@ describe('/search', () => {
      expect(status).toBe(200);
    });

-    it('should get suggestions for city', async () => {
+    it('should get suggestions for state', async () => {
+      const { status, body } = await request(app)
+        .get('/search/suggestions?type=state')
+        .set('Authorization', `Bearer ${admin.accessToken}`);
+      expect(body).toEqual([
+        'Andalusia',
+        'Berlin',
+        'Glarus',
+        'Greater Accra',
+        'Havana',
+        'Île-de-France',
+        'Marrakesh-Safi',
+        'Mississippi',
+        'New York',
+        'Shanghai',
+        'St.-Petersburg',
+        'Tbilisi',
+        'Tokyo',
+        'Virginia',
+      ]);
+      expect(status).toBe(200);
+    });
+
+    it('should get suggestions for city (including null)', async () => {
      const { status, body } = await request(app)
        .get('/search/suggestions?type=city&includeNull=true')
        .set('Authorization', `Bearer ${admin.accessToken}`);
@ -604,7 +650,31 @@ describe('/search', () => {
      expect(status).toBe(200);
    });

-    it('should get suggestions for camera make', async () => {
+    it('should get suggestions for city', async () => {
+      const { status, body } = await request(app)
+        .get('/search/suggestions?type=city')
+        .set('Authorization', `Bearer ${admin.accessToken}`);
+      expect(body).toEqual([
+        'Accra',
+        'Berlin',
+        'Glarus',
+        'Havana',
+        'Marrakesh',
+        'Montalbán de Córdoba',
+        'New York City',
+        'Novena',
+        'Paris',
+        'Philadelphia',
+        'Saint Petersburg',
+        'Shanghai',
+        'Stanley',
+        'Tbilisi',
+        'Tokyo',
+      ]);
+      expect(status).toBe(200);
+    });
+
+    it('should get suggestions for camera make (including null)', async () => {
      const { status, body } = await request(app)
        .get('/search/suggestions?type=camera-make&includeNull=true')
        .set('Authorization', `Bearer ${admin.accessToken}`);
@ -621,7 +691,23 @@ describe('/search', () => {
      expect(status).toBe(200);
    });

-    it('should get suggestions for camera model', async () => {
+    it('should get suggestions for camera make', async () => {
+      const { status, body } = await request(app)
+        .get('/search/suggestions?type=camera-make')
+        .set('Authorization', `Bearer ${admin.accessToken}`);
+      expect(body).toEqual([
+        'Apple',
+        'Canon',
+        'FUJIFILM',
+        'NIKON CORPORATION',
+        'PENTAX Corporation',
+        'samsung',
+        'SONY',
+      ]);
+      expect(status).toBe(200);
+    });
+
+    it('should get suggestions for camera model (including null)', async () => {
      const { status, body } = await request(app)
        .get('/search/suggestions?type=camera-model&includeNull=true')
        .set('Authorization', `Bearer ${admin.accessToken}`);
@ -642,5 +728,26 @@ describe('/search', () => {
      ]);
      expect(status).toBe(200);
    });
+
+    it('should get suggestions for camera model', async () => {
+      const { status, body } = await request(app)
+        .get('/search/suggestions?type=camera-model')
+        .set('Authorization', `Bearer ${admin.accessToken}`);
+      expect(body).toEqual([
+        'Canon EOS 7D',
+        'Canon EOS R5',
+        'DSLR-A550',
+        'FinePix S3Pro',
+        'iPhone 7',
+        'NIKON D700',
+        'NIKON D750',
+        'NIKON D80',
+        'PENTAX K10D',
+        'SM-F711N',
+        'SM-S906U',
+        'SM-T970',
+      ]);
+      expect(status).toBe(200);
+    });
  });
 });
--- a/server/src/interfaces/search.interface.ts
+++ b/server/src/interfaces/search.interface.ts
@ -170,6 +170,22 @@ export interface AssetDuplicateResult {
  distance: number;
 }

+export interface GetStatesOptions {
+  country?: string;
+}
+
+export interface GetCitiesOptions extends GetStatesOptions {
+  state?: string;
+}
+
+export interface GetCameraModelsOptions {
+  make?: string;
+}
+
+export interface GetCameraMakesOptions {
+  model?: string;
+}
+
 export interface ISearchRepository {
  searchMetadata(pagination: SearchPaginationOptions, options: AssetSearchOptions): Paginated<AssetEntity>;
  searchSmart(pagination: SearchPaginationOptions, options: SmartSearchOptions): Paginated<AssetEntity>;
@ -183,8 +199,8 @@ export interface ISearchRepository {
  getDimensionSize(): Promise<number>;
  setDimensionSize(dimSize: number): Promise<void>;
  getCountries(userIds: string[]): Promise<Array<string | null>>;
-  getStates(userIds: string[], country?: string): Promise<Array<string | null>>;
-  getCities(userIds: string[], country?: string, state?: string): Promise<Array<string | null>>;
-  getCameraMakes(userIds: string[], model?: string): Promise<Array<string | null>>;
-  getCameraModels(userIds: string[], make?: string): Promise<Array<string | null>>;
+  getStates(userIds: string[], options: GetStatesOptions): Promise<Array<string | null>>;
+  getCities(userIds: string[], options: GetCitiesOptions): Promise<Array<string | null>>;
+  getCameraMakes(userIds: string[], options: GetCameraMakesOptions): Promise<Array<string | null>>;
+  getCameraModels(userIds: string[], options: GetCameraModelsOptions): Promise<Array<string | null>>;
 }
--- a/server/src/queries/search.repository.sql
+++ b/server/src/queries/search.repository.sql
@ -585,52 +585,57 @@ SELECT DISTINCT
  ON ("exif"."country") "exif"."country" AS "country"
 FROM
  "exif" "exif"
-  LEFT JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
+  INNER JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
  AND ("asset"."deletedAt" IS NULL)
 WHERE
  "asset"."ownerId" IN ($1)
+  AND "exif"."country" != ''
+  AND "exif"."country" IS NOT NULL

 -- SearchRepository.getStates
 SELECT DISTINCT
  ON ("exif"."state") "exif"."state" AS "state"
 FROM
  "exif" "exif"
-  LEFT JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
+  INNER JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
  AND ("asset"."deletedAt" IS NULL)
 WHERE
  "asset"."ownerId" IN ($1)
-  AND "exif"."country" = $2
+  AND "exif"."state" != ''
+  AND "exif"."state" IS NOT NULL

 -- SearchRepository.getCities
 SELECT DISTINCT
  ON ("exif"."city") "exif"."city" AS "city"
 FROM
  "exif" "exif"
-  LEFT JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
+  INNER JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
  AND ("asset"."deletedAt" IS NULL)
 WHERE
  "asset"."ownerId" IN ($1)
-  AND "exif"."country" = $2
-  AND "exif"."state" = $3
+  AND "exif"."city" != ''
+  AND "exif"."city" IS NOT NULL

 -- SearchRepository.getCameraMakes
 SELECT DISTINCT
  ON ("exif"."make") "exif"."make" AS "make"
 FROM
  "exif" "exif"
-  LEFT JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
+  INNER JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
  AND ("asset"."deletedAt" IS NULL)
 WHERE
  "asset"."ownerId" IN ($1)
-  AND "exif"."model" = $2
+  AND "exif"."make" != ''
+  AND "exif"."make" IS NOT NULL

 -- SearchRepository.getCameraModels
 SELECT DISTINCT
  ON ("exif"."model") "exif"."model" AS "model"
 FROM
  "exif" "exif"
-  LEFT JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
+  INNER JOIN "assets" "asset" ON "asset"."id" = "exif"."assetId"
  AND ("asset"."deletedAt" IS NULL)
 WHERE
  "asset"."ownerId" IN ($1)
-  AND "exif"."make" = $2
+  AND "exif"."model" != ''
+  AND "exif"."model" IS NOT NULL
--- a/server/src/repositories/search.repository.ts
+++ b/server/src/repositories/search.repository.ts
@ -17,6 +17,10 @@ import {
  AssetSearchOptions,
  FaceEmbeddingSearch,
  FaceSearchResult,
+  GetCameraMakesOptions,
+  GetCameraModelsOptions,
+  GetCitiesOptions,
+  GetStatesOptions,
  ISearchRepository,
  SearchPaginationOptions,
  SmartSearchOptions,
@ -342,23 +346,27 @@ export class SearchRepository implements ISearchRepository {

  @GenerateSql({ params: [[DummyValue.UUID]] })
  async getCountries(userIds: string[]): Promise<string[]> {
-    const results = await this.exifRepository
+    const query = this.exifRepository
      .createQueryBuilder('exif')
-      .leftJoin('exif.asset', 'asset')
+      .innerJoin('exif.asset', 'asset')
      .where('asset.ownerId IN (:...userIds )', { userIds })
+      .andWhere(`exif.country != ''`)
+      .andWhere('exif.country IS NOT NULL')
      .select('exif.country', 'country')
-      .distinctOn(['exif.country'])
-      .getRawMany<{ country: string }>();
+      .distinctOn(['exif.country']);

-    return results.map(({ country }) => country).filter((item) => item !== '');
+    const results = await query.getRawMany<{ country: string }>();
+    return results.map(({ country }) => country);
  }

  @GenerateSql({ params: [[DummyValue.UUID], DummyValue.STRING] })
-  async getStates(userIds: string[], country: string | undefined): Promise<string[]> {
+  async getStates(userIds: string[], { country }: GetStatesOptions): Promise<string[]> {
    const query = this.exifRepository
      .createQueryBuilder('exif')
-      .leftJoin('exif.asset', 'asset')
+      .innerJoin('exif.asset', 'asset')
      .where('asset.ownerId IN (:...userIds )', { userIds })
+      .andWhere(`exif.state != ''`)
+      .andWhere('exif.state IS NOT NULL')
      .select('exif.state', 'state')
      .distinctOn(['exif.state']);

@ -367,16 +375,17 @@ export class SearchRepository implements ISearchRepository {
    }

    const result = await query.getRawMany<{ state: string }>();
-
-    return result.map(({ state }) => state).filter((item) => item !== '');
+    return result.map(({ state }) => state);
  }

  @GenerateSql({ params: [[DummyValue.UUID], DummyValue.STRING, DummyValue.STRING] })
-  async getCities(userIds: string[], country: string | undefined, state: string | undefined): Promise<string[]> {
+  async getCities(userIds: string[], { country, state }: GetCitiesOptions): Promise<string[]> {
    const query = this.exifRepository
      .createQueryBuilder('exif')
-      .leftJoin('exif.asset', 'asset')
+      .innerJoin('exif.asset', 'asset')
      .where('asset.ownerId IN (:...userIds )', { userIds })
+      .andWhere(`exif.city != ''`)
+      .andWhere('exif.city IS NOT NULL')
      .select('exif.city', 'city')
      .distinctOn(['exif.city']);

@ -389,16 +398,17 @@ export class SearchRepository implements ISearchRepository {
    }

    const results = await query.getRawMany<{ city: string }>();
-
-    return results.map(({ city }) => city).filter((item) => item !== '');
+    return results.map(({ city }) => city);
  }

  @GenerateSql({ params: [[DummyValue.UUID], DummyValue.STRING] })
-  async getCameraMakes(userIds: string[], model: string | undefined): Promise<string[]> {
+  async getCameraMakes(userIds: string[], { model }: GetCameraMakesOptions): Promise<string[]> {
    const query = this.exifRepository
      .createQueryBuilder('exif')
-      .leftJoin('exif.asset', 'asset')
+      .innerJoin('exif.asset', 'asset')
      .where('asset.ownerId IN (:...userIds )', { userIds })
+      .andWhere(`exif.make != ''`)
+      .andWhere('exif.make IS NOT NULL')
      .select('exif.make', 'make')
      .distinctOn(['exif.make']);

@ -407,15 +417,17 @@ export class SearchRepository implements ISearchRepository {
    }

    const results = await query.getRawMany<{ make: string }>();
-    return results.map(({ make }) => make).filter((item) => item !== '');
+    return results.map(({ make }) => make);
  }

  @GenerateSql({ params: [[DummyValue.UUID], DummyValue.STRING] })
-  async getCameraModels(userIds: string[], make: string | undefined): Promise<string[]> {
+  async getCameraModels(userIds: string[], { make }: GetCameraModelsOptions): Promise<string[]> {
    const query = this.exifRepository
      .createQueryBuilder('exif')
-      .leftJoin('exif.asset', 'asset')
+      .innerJoin('exif.asset', 'asset')
      .where('asset.ownerId IN (:...userIds )', { userIds })
+      .andWhere(`exif.model != ''`)
+      .andWhere('exif.model IS NOT NULL')
      .select('exif.model', 'model')
      .distinctOn(['exif.model']);

@ -424,7 +436,7 @@ export class SearchRepository implements ISearchRepository {
    }

    const results = await query.getRawMany<{ model: string }>();
-    return results.map(({ model }) => model).filter((item) => item !== '');
+    return results.map(({ model }) => model);
  }

  private getRuntimeConfig(numResults?: number): string | undefined {
--- a/server/src/services/search.service.spec.ts
+++ b/server/src/services/search.service.spec.ts
@ -59,20 +59,84 @@ describe(SearchService.name, () => {
  });

  describe('getSearchSuggestions', () => {
-    it('should return search suggestions (including null)', async () => {
-      searchMock.getCountries.mockResolvedValue(['USA', null]);
+    it('should return search suggestions for country', async () => {
+      searchMock.getCountries.mockResolvedValue(['USA']);
+      await expect(
+        sut.getSearchSuggestions(authStub.user1, { includeNull: false, type: SearchSuggestionType.COUNTRY }),
+      ).resolves.toEqual(['USA']);
+      expect(searchMock.getCountries).toHaveBeenCalledWith([authStub.user1.user.id]);
+    });
+
+    it('should return search suggestions for country (including null)', async () => {
+      searchMock.getCountries.mockResolvedValue(['USA']);
      await expect(
        sut.getSearchSuggestions(authStub.user1, { includeNull: true, type: SearchSuggestionType.COUNTRY }),
      ).resolves.toEqual(['USA', null]);
      expect(searchMock.getCountries).toHaveBeenCalledWith([authStub.user1.user.id]);
    });

-    it('should return search suggestions (without null)', async () => {
-      searchMock.getCountries.mockResolvedValue(['USA', null]);
+    it('should return search suggestions for state', async () => {
+      searchMock.getStates.mockResolvedValue(['California']);
      await expect(
-        sut.getSearchSuggestions(authStub.user1, { includeNull: false, type: SearchSuggestionType.COUNTRY }),
-      ).resolves.toEqual(['USA']);
-      expect(searchMock.getCountries).toHaveBeenCalledWith([authStub.user1.user.id]);
+        sut.getSearchSuggestions(authStub.user1, { includeNull: false, type: SearchSuggestionType.STATE }),
+      ).resolves.toEqual(['California']);
+      expect(searchMock.getStates).toHaveBeenCalledWith([authStub.user1.user.id], expect.anything());
+    });
+
+    it('should return search suggestions for state (including null)', async () => {
+      searchMock.getStates.mockResolvedValue(['California']);
+      await expect(
+        sut.getSearchSuggestions(authStub.user1, { includeNull: true, type: SearchSuggestionType.STATE }),
+      ).resolves.toEqual(['California', null]);
+      expect(searchMock.getStates).toHaveBeenCalledWith([authStub.user1.user.id], expect.anything());
+    });
+
+    it('should return search suggestions for city', async () => {
+      searchMock.getCities.mockResolvedValue(['Denver']);
+      await expect(
+        sut.getSearchSuggestions(authStub.user1, { includeNull: false, type: SearchSuggestionType.CITY }),
+      ).resolves.toEqual(['Denver']);
+      expect(searchMock.getCities).toHaveBeenCalledWith([authStub.user1.user.id], expect.anything());
+    });
+
+    it('should return search suggestions for city (including null)', async () => {
+      searchMock.getCities.mockResolvedValue(['Denver']);
+      await expect(
+        sut.getSearchSuggestions(authStub.user1, { includeNull: true, type: SearchSuggestionType.CITY }),
+      ).resolves.toEqual(['Denver', null]);
+      expect(searchMock.getCities).toHaveBeenCalledWith([authStub.user1.user.id], expect.anything());
+    });
+
+    it('should return search suggestions for camera make', async () => {
+      searchMock.getCameraMakes.mockResolvedValue(['Nikon']);
+      await expect(
+        sut.getSearchSuggestions(authStub.user1, { includeNull: false, type: SearchSuggestionType.CAMERA_MAKE }),
+      ).resolves.toEqual(['Nikon']);
+      expect(searchMock.getCameraMakes).toHaveBeenCalledWith([authStub.user1.user.id], expect.anything());
+    });
+
+    it('should return search suggestions for camera make (including null)', async () => {
+      searchMock.getCameraMakes.mockResolvedValue(['Nikon']);
+      await expect(
+        sut.getSearchSuggestions(authStub.user1, { includeNull: true, type: SearchSuggestionType.CAMERA_MAKE }),
+      ).resolves.toEqual(['Nikon', null]);
+      expect(searchMock.getCameraMakes).toHaveBeenCalledWith([authStub.user1.user.id], expect.anything());
+    });
+
+    it('should return search suggestions for camera model', async () => {
+      searchMock.getCameraModels.mockResolvedValue(['Fujifilm X100VI']);
+      await expect(
+        sut.getSearchSuggestions(authStub.user1, { includeNull: false, type: SearchSuggestionType.CAMERA_MODEL }),
+      ).resolves.toEqual(['Fujifilm X100VI']);
+      expect(searchMock.getCameraModels).toHaveBeenCalledWith([authStub.user1.user.id], expect.anything());
+    });
+
+    it('should return search suggestions for camera model (including null)', async () => {
+      searchMock.getCameraModels.mockResolvedValue(['Fujifilm X100VI']);
+      await expect(
+        sut.getSearchSuggestions(authStub.user1, { includeNull: true, type: SearchSuggestionType.CAMERA_MODEL }),
+      ).resolves.toEqual(['Fujifilm X100VI', null]);
+      expect(searchMock.getCameraModels).toHaveBeenCalledWith([authStub.user1.user.id], expect.anything());
    });
  });
 });
--- a/server/src/services/search.service.ts
+++ b/server/src/services/search.service.ts
@ -108,8 +108,11 @@ export class SearchService extends BaseService {

  async getSearchSuggestions(auth: AuthDto, dto: SearchSuggestionRequestDto) {
    const userIds = await this.getUserIdsToSearch(auth);
-    const results = await this.getSuggestions(userIds, dto);
-    return results.filter((result) => (dto.includeNull ? true : result !== null));
+    const suggestions = await this.getSuggestions(userIds, dto);
+    if (dto.includeNull) {
+      suggestions.push(null);
+    }
+    return suggestions;
  }

  private getSuggestions(userIds: string[], dto: SearchSuggestionRequestDto) {
@ -118,19 +121,19 @@ export class SearchService extends BaseService {
        return this.searchRepository.getCountries(userIds);
      }
      case SearchSuggestionType.STATE: {
-        return this.searchRepository.getStates(userIds, dto.country);
+        return this.searchRepository.getStates(userIds, dto);
      }
      case SearchSuggestionType.CITY: {
-        return this.searchRepository.getCities(userIds, dto.country, dto.state);
+        return this.searchRepository.getCities(userIds, dto);
      }
      case SearchSuggestionType.CAMERA_MAKE: {
-        return this.searchRepository.getCameraMakes(userIds, dto.model);
+        return this.searchRepository.getCameraMakes(userIds, dto);
      }
      case SearchSuggestionType.CAMERA_MODEL: {
-        return this.searchRepository.getCameraModels(userIds, dto.make);
+        return this.searchRepository.getCameraModels(userIds, dto);
      }
      default: {
-        return [];
+        return [] as (string | null)[];
      }
    }
  }