feat(server): search unknown place #10866

jforseth210 · 2024-07-04T20:51:40Z

This is a rough draft for allowing users to search for photos that specifically don't have a location (or at least a country) as requested in #8362.

It updates the server to accept null as a value for country, which tells the database to search for assets with no country exifInfo.

The frontend is updated to automatically include an "Unknown" option in the country combo box, which is passed to the server as {"country":null}. The filter list in the results is also updated to display "Unknown" if the value of a filter is null.

I don't have a ton of experience contributing to FOSS, so this probably is a ways from merge ready. Let me know what I need to do differently.

I suspect there's something I need to update in /open-api but I'm not sure what.
Removing the validators in search.dto.ts makes me a bit nervous. Is there anything that's expecting a string that I missed?
I'd love some advice to make selectedOption easier to read
I haven't touched the mobile app at all

andWhere({country:null}) produces `"exifInfo"."country" = NULL`. We want `"exifInfo"."country" IS NULL`, so we have to treat NULL as a special case

…into search-unknown-place

jforseth210 · 2024-07-05T19:39:59Z

I'm not really sure how to correctly update the types for @immich/sdk. Tried reading through the docs but still not getting it... Where's the best place to get a hand? Discord?

danieldietzler · 2024-07-05T19:53:59Z

I'm not really sure how to correctly update the types for @immich/sdk. Tried reading through the docs but still not getting it... Where's the best place to get a hand? Discord?

make open-api. But yeah, generally joining our discord (https://discord.immich.app) is a good idea

server/src/dtos/search.dto.ts

jforseth210 · 2024-07-10T16:19:29Z

I think that last commit should fix those api issues. Thanks @danieldietzler and @jrasm91 for the help! What's next?

web/src/lib/components/shared-components/search-bar/search-location-section.svelte

jrasm91

Thanks for the PR! I made a few changes, but I think it is good to go now:

Moved the logic to return "null" options to the server. This means they are only shown in the UI if there are actually valid results (given other criteria) with null values.
Updated the api to accept empty string or null. (The mobile app can't actually send null, and empty string makes the combobox types a lot simpler)
Added some e2e tests for searching with empty/null
Expanded the scope to include camera make, camera model, and camera lens.

danieldietzler

LGTM

mertalev

Thanks for working on this! I think some of the backend behavior is strange and surprising for the sake of letting the frontend send '' instead of null. I'd much prefer if you could make the frontend send null instead.

mertalev · 2024-07-11T16:40:00Z

server/src/repositories/metadata.repository.ts

      .createQueryBuilder('exif')
      .leftJoin('exif.asset', 'asset')
      .where('asset.ownerId = :userId', { userId })
-      .andWhere('exif.country IS NOT NULL')
-      .select('exif.country')
+      .select('exif.country', 'exif_country')


Is there a particular reason you set an alias here and make it a raw query? You can remove the alias and use getMany like the original, just removing the filter at the end.

Right, when I did that it only returned one result (United States) instead of two (United States, null).

Oh, maybe because you don't select the id field. That's fine, it can be a raw query. Maybe just make the alias camel case (or remove it).

mertalev · 2024-07-11T16:48:05Z

server/src/utils/database.ts

+      if (value === null || value === '') {
+        builder.andWhere(`exifInfo.${key} IS NULL`);


An empty string is not the same as null and shouldn't have this behavior.

I could map it in the service instead, if you want, but at the API level it is going to mean the same thing.

web/src/lib/components/shared-components/search-bar/search-filter-box.svelte

web/src/routes/(user)/search/[[photos=photos]]/[[assetId=id]]/+page.svelte

mertalev · 2024-07-11T16:53:30Z

web/src/lib/components/shared-components/search-bar/search-camera-section.svelte

+    if (filters.make && !makes.includes(filters.make)) {
+      filters.make = undefined;
+    }


Why is this necessary?

This just matches what was in the location filter file. If the current set filter isn't in the list of valid options it is discarded.

Sure, but when can that actually happen?

I think only by directly navigating with those params. Just seemed good to make them consistent, but maybe it isn't necessary

mertalev · 2024-07-11T16:56:30Z

server/src/dtos/search.dto.ts


  @IsString()
-  @IsNotEmpty()


I think the request should explicitly be null and not an empty string, so this doesn't need to be removed.

I could do that, at least for the web. But the mobile app can't actually send null values through the generated client. So either the mobile app doesn't get this functionality or we need to use an empty string in the api to indicate querying for null.

mertalev · 2024-07-11T17:04:19Z

The mobile app can't actually send null

How can it not send null? That's so weird. In that case I'd rather handle the '' => null conversion as a transformer without polluting other code.

jforseth210 and others added 9 commits July 3, 2024 16:30

Allow submission of null country

760e647

Update searchAssetBuilder to handle nulls

54a0fc4

andWhere({country:null}) produces `"exifInfo"."country" = NULL`. We want `"exifInfo"."country" IS NULL`, so we have to treat NULL as a special case

Allow null country in frontend

c70c926

Merge branch 'immich-app:main' into search-unknown-place

37bd949

Make the query code a bit more straightforward

e4c4fe5

Merge branch 'search-unknown-place' of github.com:jforseth210/immich …

3b5a7fc

…into search-unknown-place

Remove unused brackets import

690652c

Remove log message

a363cf3

Don't change whitespace for no reason

505ed2e

jforseth210 requested a review from danieldietzler as a code owner July 4, 2024 20:51

github-actions bot added 🖥️web 🗄️server labels Jul 4, 2024

jforseth210 changed the title ~~Search unknown place~~ [feat] Search unknown place Jul 4, 2024

Fix prettier style issue

4485601

alextran1502 changed the title ~~[feat] Search unknown place~~ feat(server): search unknown place Jul 5, 2024

jrasm91 reviewed Jul 8, 2024

View reviewed changes

server/src/dtos/search.dto.ts Outdated Show resolved Hide resolved

jforseth210 added 2 commits July 8, 2024 09:57

Update search.dto.ts validators per @jrasm91's recommendation

b9d95cd

Update api types

3dbc993

jrasm91 reviewed Jul 10, 2024

View reviewed changes

web/src/lib/components/shared-components/search-bar/search-location-section.svelte Outdated Show resolved Hide resolved

Combine null country and state into one guard clause

ef54ed1

danieldietzler reviewed Jul 11, 2024

View reviewed changes

web/src/lib/components/shared-components/search-bar/search-location-section.svelte Outdated Show resolved Hide resolved

jrasm91 force-pushed the search-unknown-place branch from dd39540 to 99b54f2 Compare July 11, 2024 13:51

danieldietzler approved these changes Jul 11, 2024

View reviewed changes

jrasm91 added 2 commits July 11, 2024 10:20

chore: clean up

38be7cd

chore: add e2e for null/empty city, state, country search

6ee68b8

jrasm91 force-pushed the search-unknown-place branch from 99b54f2 to 6ee68b8 Compare July 11, 2024 14:28

refactor: server returns suggestion for null values

4932314

jrasm91 approved these changes Jul 11, 2024

View reviewed changes

danieldietzler approved these changes Jul 11, 2024

View reviewed changes

jrasm91 requested a review from mertalev July 11, 2024 15:10

mertalev reviewed Jul 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): search unknown place #10866

feat(server): search unknown place #10866

jforseth210 commented Jul 4, 2024

jforseth210 commented Jul 5, 2024

danieldietzler commented Jul 5, 2024

jforseth210 commented Jul 10, 2024

jrasm91 left a comment •

edited

Loading

danieldietzler left a comment

mertalev left a comment

mertalev Jul 11, 2024

jrasm91 Jul 11, 2024

mertalev Jul 11, 2024

mertalev Jul 11, 2024

jrasm91 Jul 11, 2024

mertalev Jul 11, 2024

jrasm91 Jul 11, 2024

mertalev Jul 11, 2024

jrasm91 Jul 11, 2024

mertalev Jul 11, 2024

jrasm91 Jul 11, 2024

mertalev commented Jul 11, 2024

		if (value === null \|\| value === '') {
		builder.andWhere(`exifInfo.${key} IS NULL`);

feat(server): search unknown place #10866

Are you sure you want to change the base?

feat(server): search unknown place #10866

Conversation

jforseth210 commented Jul 4, 2024

jforseth210 commented Jul 5, 2024

danieldietzler commented Jul 5, 2024

jforseth210 commented Jul 10, 2024

jrasm91 left a comment • edited Loading

Choose a reason for hiding this comment

danieldietzler left a comment

Choose a reason for hiding this comment

mertalev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertalev commented Jul 11, 2024

jrasm91 left a comment •

edited

Loading