fix(server): update ocr & faces after asset edit - #29303
Conversation
|
📱 Android release APK (universal) — Download: https://github.com/immich-app/immich/actions/runs/28094818223/artifacts/7848129884 QR codeInstalls as a separate app (applicationId |
shenlong-tanwen
left a comment
shenlong-tanwen
left a comment
There was a problem hiding this comment.
@YarosMallorca The OCR table does not update off of visibility so the proper way to do it is to sync and handle both the entities based off of their visibility
8802f64 to
87f858a
Compare
YarosMallorca
changed the title
|
Please test thoroughly before approving/merging. I wasn't able to test much due to my half-broken environment. |
|
was about to work on a fix. rotating an image deleted the ocr text completely without rerunning the job. looking forward to the merge! |
| @UpdateDateColumn() | ||
| updatedAt!: Generated<Timestamp>; |
There was a problem hiding this comment.
You don't even need this, do you? It's simply only been missed to add the @UpdatedAtTrigger
There was a problem hiding this comment.
The updatedAt column is required here.
@UpdatedAtTrigger binds to the shared updated_at() function, which sets both new."updatedAt" and new."updateId".
Without the column, every UPDATE on asset_ocr would throw record "new" has no field "updatedAt".
All 25 tables using @UpdatedAtTrigger (incl. asset_face) pair it with an updatedAt column, so should match the existing pattern.
danieldietzler
left a comment
danieldietzler
left a comment
There was a problem hiding this comment.
@shenlong-tanwen feel free to merge if you're happy with it now, too
|
@YarosMallorca We might need to reset the sync checkpoint for the OCR table so the older rows that were not pushed on visibility update can be synced back to the clients |
|
We actually have no way of knowing which we missed, so that we can only backfil those, right? We have to reset the checkpoint |
Yep, Maaaaybe we can handle this client side by calculating the visibility off of the edit table with the same logic as to what the server does, but resetting the sync seems simpler and cleaner for what is a one time migration |
4 participants
Description
Fixes #29217 - an issue where the OCR & people wouldn't update properly after an edit on mobile.
How Has This Been Tested?
Please describe to which degree, if any, an LLM was used in creating this pull request.
Initially analyzed and implemented using Claude Opus, manually reviewed and edited by me.