feat: enhance search #7127
feat: enhance search #7127
Conversation
Deploying with
|
| Latest commit: |
1f3ac7a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9df534b6.immich.pages.dev |
| Branch Preview URL: | https://feat-hybrid-search.immich.pages.dev |
There was a problem hiding this comment.
I think there might be some confusion here. What this PR adds on the server side already exists in the current smart search endpoint. A hybrid search endpoint would just also do full-text search on certain fields and isn't necessary for the current filters on the frontend.
| q: searchValue, | ||
| smart: smartSearch, | ||
| take: '100', | ||
| q: JSON.stringify(payload), |
There was a problem hiding this comment.
It seems strange to have a full JSON as the value of q. Wouldn't it be better for each filter to be a different parameter?
There was a problem hiding this comment.
This makes easier downstream to grab the whole string as JSON string and decode into javascript object
There was a problem hiding this comment.
You made the search endpoints POST, so what's the purpose of doing this instead of putting the JSON in the body?
| @@ -169,6 +169,12 @@ export class MetadataSearchDto extends BaseSearchDto { | |||
| @Optional() | |||
| @ApiProperty({ enumName: 'AssetOrder', enum: AssetOrder }) | |||
| order?: AssetOrder; | |||
|
|
|||
| @QueryBoolean({ optional: true }) | |||
| isNotInAlbum?: boolean; | |||
There was a problem hiding this comment.
I personally prefer not having negated variables. So I would like to have isInAlbum and if that's false, the asset should not be in an album.
There was a problem hiding this comment.
I thought about that as well, but to keep things easy to follow from the UI perspective specifically search for not in album. I think the name is justified to have not in it
| const hasExifQuery = Object.keys(exifInfo).length > 0; | ||
|
|
||
| if (options.withExif && !hasExifQuery) { | ||
| builder.leftJoinAndSelect(`${builder.alias}.exifInfo`, 'exifInfo'); | ||
| } | ||
|
|
||
| if (hasExifQuery) { | ||
| options.withExif | ||
| ? builder.leftJoinAndSelect(`${builder.alias}.exifInfo`, 'exifInfo') | ||
| : builder.leftJoin(`${builder.alias}.exifInfo`, 'exifInfo'); | ||
|
|
There was a problem hiding this comment.
Did you want to refactor this with what I had proposed on discord?
There was a problem hiding this comment.
There is no clean syntax for builder.select() i.e. builder.select('exifInfo.*') unfortunately
There was a problem hiding this comment.
I'm pretty sure there is either builder.select('exifInfo') or builder.select('exifInfo.*')
There was a problem hiding this comment.
Yeah I tried both and they don't work :/
web/src/lib/components/shared-components/gallery-viewer/gallery-viewer.svelte
Outdated
Show resolved
Hide resolved
web/src/lib/components/shared-components/search-bar/search-filter-box.svelte
Outdated
Show resolved
Hide resolved
web/src/lib/components/shared-components/search-bar/search-filter-box.svelte
Outdated
Show resolved
Hide resolved
web/src/lib/components/shared-components/search-bar/search-filter-box.svelte
Show resolved
Hide resolved
web/src/lib/components/shared-components/search-bar/search-filter-box.svelte
Show resolved
Hide resolved
| .leftJoin(`${builder.alias}.faces`, 'faces') | ||
| .andWhere('faces.personId IN (:...personIds)', { personIds: options.personIds }) | ||
| .addGroupBy(`${builder.alias}.id`) | ||
| .having('COUNT(faces.id) = :personCount', { personCount: options.personIds.length }); |
There was a problem hiding this comment.
I don't understand this. Why are we checking that the face count is equal to the person count?
There was a problem hiding this comment.
Let say you select people A and people B. Without that condition, the result will include people A only or people B only. We want the result includes both people A and people B altogether
This PR implements the rest of the search filter logic to search and render assets.