immich icon indicating copy to clipboard operation
immich copied to clipboard

refactor(server): use kysely

Open mertalev opened this issue 1 year ago • 2 comments

Description

This PR transitions the asset and search 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
    • I noticed that the includeNull parameter wasn't being respected for the search suggestion queries, so I changed their signatures to handle it
  • 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. A related limitation is that there's no reference to the columns at runtime, so the server introspects the database after migrations run to get this information (TypeORM provides this as well, but I didn't want to rely on it).
  • 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. The one issue I'm currently seeing is that scrubbing the timeline doesn't work: I can't move it, even though normal scrolling works.

If testing this PR, be sure to make the indices mentioned in the TODO comments in the asset repository. I haven't made a migration to apply them yet.

mertalev avatar Sep 23 '24 02:09 mertalev

Will this fix #11868?

danieldietzler avatar Sep 23 '24 19:09 danieldietzler

Will this fix #11868?

Yup.

mertalev avatar Sep 23 '24 20:09 mertalev

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

mmomjian avatar Dec 19 '24 18:12 mmomjian

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

mertalev avatar Dec 19 '24 18:12 mertalev

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 avatar Dec 20 '24 22:12 alextran1502

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

alextran1502 avatar Dec 23 '24 20:12 alextran1502

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

I haven't been able to reproduce this.

mertalev avatar Jan 02 '25 23:01 mertalev

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

image

alextran1502 avatar Jan 03 '25 06:01 alextran1502

Is it possible that this change broke the DuplicatService?

See my Q&A: https://github.com/immich-app/immich/discussions/15722

SuperSnackman avatar Jan 28 '25 08:01 SuperSnackman