refactor(server): use kysely #12857

mertalev · 2024-09-23T02:57:43Z

Description

This PR transitions the asset, search and view repositories from TypeORM to Kysely. The goals of this change include type safety—including for complex queries—improved performance, more transparent behavior and better code composability. Other repositories as well as dev tooling will be migrated in later PRs to limit the scope of this one.

Notes:

Almost every query is faster than before, some significantly so. However, there is a possible regression for queries using both file and tag relations, in the order of single-digit milliseconds for 100 assets. This seems to be because the plan for the subquery approach is more reliant on random access through indices. This is only measuring the queries themselves — the difference may or may not disappear after accounting for the additional postprocessing that TypeORM needs to do.
Most repository methods have the same signatures as before to ease the transition. There are some exceptions to this:
- Some upsert/update methods have Kysely types as inputs since this was an easy change and made things simpler
- I made changes to some queries that necessitated a different signature
Kysely considers ORM-like upsert helpers to be out of scope. Since this is a common need, I made a few type-safe helpers to make these queries more ergonomic.
There can be differences in JSON-ified and normal DB responses. bytea types are returned as hex-encoded strings in the former and as Buffer in the latter. Timestamps likewise end with +00:00 in JSON and .000Z without.
- +00:00 is more accurate if anything, but the inconsistency is a bit annoying.
- I'm not sure if it's worth parsing the hex strings into buffers and back into base64 strings. The payload is smaller this way, but the server spends more time to do this.

Testing

Over the course of working on this branch, I've tested almost everything the web app can do at one point or another. But there may be regressions since testing certain functionality or subtle issues that I haven't noticed, and I haven't tested it with the mobile app.

danieldietzler · 2024-09-23T19:38:10Z

Will this fix #11868?

mertalev · 2024-09-23T20:38:52Z

Will this fix #11868?

Yup.

server/src/entities/asset.entity.ts

server/src/interfaces/asset.interface.ts

server/src/interfaces/search.interface.ts

server/src/repositories/database.repository.ts

jrasm91 · 2024-09-25T13:02:50Z

server/src/services/duplicate.service.ts

+    const duplicates = await this.assetRepository.getDuplicates(auth.user.id);
+    return duplicates.map(({ duplicateId, assets }) => ({
+      duplicateId,
+      assets: assets.map((asset) => mapAsset(asset, { auth })),


why did you remove stacking?

I don't think it makes sense for how duplicate detection works right now. It won't detect the whole stack as being a duplicate, just particular members of that stack if they're similar enough. The primary asset could be a duplicate, but others might not be and vice versa. This could be addressed in a separate PR, though.

I'm not against removing it, but I think it does make sense for those types of decisions to be transparently made, which most likely means happening in a separate PR.

server/src/utils/events.ts

mmomjian · 2024-12-19T18:31:50Z

Nice! Any changes here that would affect people using external Postgres? I don’t see any new extensions etc.

mertalev · 2024-12-19T18:32:27Z

Nice! Any changes here that would affect people using external Postgres? I don’t see any new extensions etc.

I don't think so. It's an internal change

alextran1502

Some questions about the migration. What are the steps we need to do after this PR get merged? Would you like to lay out the TODO list for the team?

alextran1502 · 2024-12-19T20:32:42Z

server/src/dtos/asset-response.dto.ts

@@ -97,10 +97,19 @@ const mapStack = (entity: AssetEntity) => {
  return {
    id: entity.stack.id,
    primaryAssetId: entity.stack.primaryAssetId,
-    assetCount: entity.stack.assetCount ?? entity.stack.assets.length,
+    assetCount: entity.stack.assetCount ?? entity.stack.assets.length + 1,


Why does it need to append 1 here?

This is from a while ago. I think it was because entity.stack.assetCount includes the primary asset, but entity.stack.assets.length does not.

Is this related to the kysely change?

server/src/dtos/duplicate.dto.ts

alextran1502 · 2024-12-19T22:29:30Z

server/src/repositories/config.repository.ts

+    types: {
+      date: {
+        to: 1184,
+        from: [1082, 1114, 1184],


What do these numbers mean?

They're Postgres data type IDs.

alextran1502 · 2024-12-19T22:30:22Z

server/src/db.d.ts

How does the content in this file get generated? Can we provide some developer documentation for this?

It uses kysely-codegen. I could have sworn I added an npm run command for this, but maybe it got lost in a rebase at some point. I can add docs for it.

danieldietzler

Not done yet, so just my state of today

danieldietzler · 2024-12-19T18:59:02Z

server/src/dtos/asset-response.dto.ts

@@ -97,10 +97,19 @@ const mapStack = (entity: AssetEntity) => {
  return {
    id: entity.stack.id,
    primaryAssetId: entity.stack.primaryAssetId,
-    assetCount: entity.stack.assetCount ?? entity.stack.assets.length,
+    assetCount: entity.stack.assetCount ?? entity.stack.assets.length + 1,


Was that just a random bug you found along the way?

server/src/repositories/asset.repository.ts

jrasm91 · 2024-12-20T16:19:36Z

server/tsconfig.json

@@ -19,7 +19,8 @@
    "preserveWatchOutput": true,
    "baseUrl": "./",
    "jsx": "react",
-    "types": ["vitest/globals"]
+    "types": ["vitest/globals"],
+    "noErrorTruncation": true


What is this for?

The Kysely types can be pretty complex, so this tells VS Code to show more of the type when you hover over something.

alextran1502 · 2024-12-20T21:31:11Z

server/src/utils/database.ts

-        builder.andWhere(`exifInfo.${key} = :${key}`, { [key]: value });
-      }
+export const UPSERT_COLUMNS = {} as { [T in keyof DB]: { [K in keyof DB[T]]: RawBuilder<unknown> } };
+/** Any repository that upserts to a table using `mapUpsertColumns` should call this method in its constructor with that table. */


Can you help me understand the purpose of this mechanism?

When an upsert is done, every field that's going to be updated has to be part of the query as excluded.{field}. We precompute the excluded.* for a table so mapUpsertColumns can reference this instead of recomputing it on each upsert.

Can you help giving an example to demonstrate the idea?

If you have a table with id, path, type and perform an update on path and type, mapUpsertColumns will return an object { path: sql'excluded.${sql.ref('path')}', type: sql'excluded.${sql.ref('type')}' }. It will exclude id from that object because it's in the conflictKeys. If you updated only path, then type would not be in the object either.

alextran1502 · 2024-12-20T22:07:17Z

I just performed a functional testing run.

The Tag page doesn't render the assets correctly
The sync endpoint throws the error below

immich_server            | [Nest] 28  - 12/20/2024, 4:06:42 PM   ERROR [Api:ErrorInterceptor~2thpecne] Unknown error: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
immich_server            | TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
immich_server            |     at extname (node:path:1463:5)
immich_server            |     at Object.lookup (/usr/src/app/src/utils/mime-types.ts:90:51)
immich_server            |     at mapAsset (/usr/src/app/src/dtos/asset-response.dto.ts:140:33)
immich_server            |     at <anonymous> (/usr/src/app/src/services/sync.service.ts:24:38)
immich_server            |     at Array.map (<anonymous>)
immich_server            |     at SyncService.getFullSync (/usr/src/app/src/services/sync.service.ts:24:19)
immich_server            |     at process.processTicksAndRejections (node:internal/process/task_queues:105:5)

alextran1502 · 2024-12-23T20:16:00Z

Hi Mert, the latest update causes the web timeline to be unable to load

mertalev · 2025-01-02T23:29:37Z

Hi Mert, the latest update causes the web timeline to be unable to load

I haven't been able to reproduce this.

alextran1502 · 2025-01-03T06:22:56Z

Hi Mert, the latest update causes the web timeline to be unable to load

I haven't been able to reproduce this.

I am running the latest update with make dev-update and this is what I see, same behavior for album, and in incognito

jrasm91

LGTM

github-actions bot added the 🗄️server label Sep 23, 2024

mertalev added the changelog:enhancement label Sep 23, 2024

jrasm91 reviewed Sep 25, 2024

View reviewed changes

mertalev force-pushed the feat/kysely branch from 83799aa to 6639a22 Compare November 12, 2024 23:51

mertalev mentioned this pull request Nov 27, 2024

feat: extend smart search, match exif-description too #14380

Draft

mertalev force-pushed the feat/kysely branch 4 times, most recently from 92b0e58 to 2df635f Compare December 10, 2024 21:41

mertalev force-pushed the feat/kysely branch 8 times, most recently from c6ee910 to 030cab8 Compare December 19, 2024 18:16

mertalev marked this pull request as ready for review December 19, 2024 18:28

mertalev requested a review from danieldietzler as a code owner December 19, 2024 18:28

alextran1502 reviewed Dec 19, 2024

View reviewed changes

danieldietzler reviewed Dec 19, 2024

View reviewed changes

jrasm91 reviewed Dec 20, 2024

View reviewed changes

alextran1502 reviewed Dec 20, 2024

View reviewed changes

wip

38a82d3

mertalev added 5 commits January 2, 2025 14:52

use min year for memory time series, sql generation fixes

4731f27

closure begone

aa7f486

unnecessary parentheses

7ed62d4

fixes for sync queries

c91a139

add count option to withStack

88bf061

mertalev force-pushed the feat/kysely branch from 45cd21a to 88bf061 Compare January 2, 2025 23:29

update openapi

951f573

mertalev and others added 5 commits January 3, 2025 15:30

introspection at startup

41aa4ef

ensure utc in getTimeBuckets

f1a6c8b

update mock

39e456c

remove unused import

51eab5b

Merge branch 'main' into feat/kysely

7847653

jrasm91 approved these changes Jan 8, 2025

View reviewed changes

alextran1502 approved these changes Jan 9, 2025

View reviewed changes

mertalev merged commit 2e12c46 into main Jan 9, 2025
36 checks passed

mertalev deleted the feat/kysely branch January 9, 2025 16:15

aminesebastian pushed a commit to suburban-digital/immich-sd that referenced this pull request Jan 12, 2025

refactor(server): use kysely (immich-app#12857)

455c1ae

aminesebastian mentioned this pull request Jan 12, 2025

Stacked photos may be rendering in duplicate with the new Kysely migration #15280

Open

3 tasks

yosit pushed a commit to yosit/immich that referenced this pull request Jan 13, 2025

refactor(server): use kysely (immich-app#12857)

bce4938

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(server): use kysely #12857

refactor(server): use kysely #12857

mertalev commented Sep 23, 2024 •

edited

Loading

danieldietzler commented Sep 23, 2024

mertalev commented Sep 23, 2024

jrasm91 Sep 25, 2024

mertalev Sep 25, 2024

jrasm91 Sep 25, 2024

mmomjian commented Dec 19, 2024

mertalev commented Dec 19, 2024

alextran1502 left a comment

alextran1502 Dec 19, 2024

mertalev Dec 19, 2024 •

edited

Loading

jrasm91 Dec 20, 2024

alextran1502 Dec 19, 2024

mertalev Dec 19, 2024

alextran1502 Dec 19, 2024

mertalev Dec 19, 2024

danieldietzler left a comment

danieldietzler Dec 19, 2024

jrasm91 Dec 20, 2024

mertalev Dec 20, 2024

alextran1502 Dec 20, 2024

mertalev Dec 20, 2024

alextran1502 Dec 23, 2024

mertalev Dec 23, 2024 •

edited

Loading

alextran1502 commented Dec 20, 2024

alextran1502 commented Dec 23, 2024

mertalev commented Jan 2, 2025

alextran1502 commented Jan 3, 2025

jrasm91 left a comment

refactor(server): use kysely #12857

refactor(server): use kysely #12857

Conversation

mertalev commented Sep 23, 2024 • edited Loading

Description

Testing

danieldietzler commented Sep 23, 2024

mertalev commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmomjian commented Dec 19, 2024

mertalev commented Dec 19, 2024

alextran1502 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertalev Dec 19, 2024 • edited Loading

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

danieldietzler 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

mertalev Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

alextran1502 commented Dec 20, 2024

alextran1502 commented Dec 23, 2024

mertalev commented Jan 2, 2025

alextran1502 commented Jan 3, 2025

jrasm91 left a comment

Choose a reason for hiding this comment

mertalev commented Sep 23, 2024 •

edited

Loading

mertalev Dec 19, 2024 •

edited

Loading

mertalev Dec 23, 2024 •

edited

Loading