Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: API and UI to replace asset data without changing record id #8480

Closed
wants to merge 47 commits into from

Conversation

midzelis
Copy link
Contributor

@midzelis midzelis commented Apr 3, 2024

A new API to update an existing asset:

PUT /api/assets/:id/upload

The asset will continue to have the same id and any album memberships. Only the asset data will be updated. The existing asset data will be copied into a new asset (with a new id) and immediately put into the trash. Reused the same parameters as POST /api/asset that pertain to the asset binary itself.

This is to support the Lightroom workflow using the Immich Publisher plugin: (https://github.com/midzelis/mi.Immich.Publisher) Lightroom is a non-destructive editor. If you make edits to an image that has been published to immich, lightroom will attempt to send the new updates to the destination. Immich didn't support updating existing pictures until now.

Since the asset thumbnails are served with a very long cache, updating the asset in this way needs a way to tell the client to ignore the cache. Easiest way to do this is with a cache-buster query param. This optional parameter is set to the asset checksum, which changes when the asset it updated.

Algorithm:
Retrieve the existing asset
Retrieve the existing livePhotoAsset (if present)
Update or Create the existing livePhotoAsset with new livePhotoAsset (if present)
Update the existing asset with uploaded file path, and new livePhotoAsset (if present)
"Clone" the existing asset (copy all properties to a new asset, but with a newid), run the standard jobs, except don't inform client of a new successful upload, and then immediately trash it.

@midzelis midzelis marked this pull request as draft April 3, 2024 03:39
@midzelis midzelis marked this pull request as ready for review April 18, 2024 18:47
@bo0tzz bo0tzz requested a review from jrasm91 April 18, 2024 19:05
@midzelis
Copy link
Contributor Author

midzelis commented Apr 18, 2024

This adds a new api:

PUT /assets/:id/upload

It accepts the following properties, declared in a new UpdateAssetDataDto which differs slightly from CreateAssetDto in that it does not allow you to specify the albumId, isFavorite, isArchived, isVisible, isOffline, isReadonly parameters, since these properties are taken from the existing record.

assetData is always required. livePhotoData is optional, and sidecarData is optional.

If an existing asset has livePhotoData or sidecarData, and the request does not contain a new livePhotoData or sidecarData - these fields are removed from the existing asset record. The thought process behind this is that the livePhotoData is derived from the asset, and if you are replacing the main data, it doesn't make sense to keep the livePhotoData.

Note

Nothing is deleted in any case. A new asset is created as a backup using the original photo data, livePhotoData, and sidecar, if present.

Related artifacts such as: stack information, faces, smart search data, favorite status, etc are not modified by the replace action. This information is also NOT copied to the backup asset. This means you can not perfectly undo this action if you "restore" the backup copy. The backup is really just a safety measure. You'd be better off downloading the backup copy, and replacing the original again.

Job behavior changes:

A new enum value was added to the source property of IEntityJob. In addition to upload and sidecar-write - there is a clone value, which represents that an asset record was copied into a backup record.

The Jobs execution pattern for a new asset is as follows: An update of the existing record will perform exactly the same steps, so that the existing record has new thumbnails, faces, indexes, etc.

METADATA_EXTRACTION
LINK_LIVE_PHOTOS
STORAGE_TEMPLATE_MIGRATION_SINGLE
GENERATE_PREVIEW
FACE_DETECTION
GENERATE_THUMBNAIL (send UPLOAD_SUCCESS to client)
GENERATE_THUMBHASH
SMART_SEARCH
VIDEO_CONVERSION
FACE_DETECTION

For the backup (the clone), since its being trashed, we can skip some of these steps, so for the cloned asset, it looks like this:

METADATA_EXTRACTION
LINK_LIVE_PHOTOS
STORAGE_TEMPLATE_MIGRATION_SINGLE
GENERATE_PREVIEW
GENERATE_THUMBNAIL 
GENERATE_THUMBHASH

UI changes:

A new option "Replace photo" was added to menu on asset-viewer-nav-bar

The file uploader was modified to accept a new parameter, assetId that will cause it to use the updateFile method instead of uploadFile. Fancy typescript was added to ensure that assetId OR albumId can be specified, but not both. The bare-params were turned into a param-object.

A cache-buster was added to all thumbnail, video-thumbnail urls, which uses the checksum of the image as query param. When the client receives and event, it will regen the url with a new query param - this causes the browser to treat this like a new image, and bypass the cache.

In photo-viewer and video-viewer, it doesn't use <img src> to load the images. So a websocket listener was added in asset-viewer, and it will update the displayed asset if any after the upload is done. The photo-viewer and video-viewer were modified to use a svelte effect to listen to changes to the asset, and update the url or data for the asset. video required adding a .load() on the <video> tag after updating the src.

Questions

I have noticed the key param used in several places, but not clear on what it is. Is this similar to a cache buster ? If so, we could maybe use that instead of the checksum/cachebuster.

@midzelis midzelis changed the title Add api to update existing asset feat(server, web): API and UI to replace asset data without changing record id Apr 18, 2024
@midzelis
Copy link
Contributor Author

The cachebuster might fix #6955

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 19, 2024

The key query param is used for shared link authentication. So routes that require authentication even when you are not logged in.

@jrasm91 jrasm91 self-assigned this Apr 29, 2024
server/src/dtos/asset-v1.dto.ts Outdated Show resolved Hide resolved
server/src/interfaces/job.interface.ts Outdated Show resolved Hide resolved
server/src/services/asset-v1.service.ts Outdated Show resolved Hide resolved
server/src/services/asset-v1.service.ts Outdated Show resolved Hide resolved
server/src/services/asset-v1.service.ts Outdated Show resolved Hide resolved
server/src/services/asset-v1.service.ts Outdated Show resolved Hide resolved
server/src/services/asset-v1.service.spec.ts Outdated Show resolved Hide resolved
server/src/services/asset-v1.service.spec.ts Outdated Show resolved Hide resolved
Comment on lines 80 to 109
const _getExistingAsset = {
..._getAsset_1(),
duration: null,
type: AssetType.IMAGE,
checksum: Buffer.from('_getExistingAsset', 'utf8'),
libraryId: 'libraryId',
};
const _getExistingAssetWithSideCar = {
..._getExistingAsset,
sidecarPath: 'sidecar-path',
checksum: Buffer.from('_getExistingAssetWithSideCar', 'utf8'),
};
const _getClonedAsset = {
id: 'cloned-copy',
originalPath: 'cloned-path',
};
const _getExistingLivePhotoMotionAsset = {
...assetStub.livePhotoMotionAsset,
checksum: Buffer.from('_getExistingLivePhotoMotionAsset', 'utf8'),
libraryId: 'libraryId',
};
const _getExistingLivePhotoStillAsset = {
..._getExistingAsset,
livePhotoVideoId: _getExistingLivePhotoMotionAsset.id,
};
const _getClonedLivePhotoMotionAsset = {
id: 'cloned-livephotomotion',
originalPath: 'cloned-livephotomotion-path',
checksum: Buffer.from('cloned-livephotomotion', 'utf8'),
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since those aren't methods I find _get misleading here. IMO it's just the existingAsset for instance

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename these to just cloneLivePhotoMotionAsset, etc.?

server/src/services/asset-v1.service.spec.ts Outdated Show resolved Hide resolved
web/src/lib/components/asset-viewer/photo-viewer.svelte Outdated Show resolved Hide resolved
}
interface FileUploadWithAlbum extends FileUploadParams {
albumId?: string;
assetId?: never;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that type never?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes it possible to access item.assetId when it is this type of above without needing to cast it.

@midzelis midzelis marked this pull request as draft May 7, 2024 03:36
@alextran1502 alextran1502 self-assigned this May 20, 2024
@alextran1502
Copy link
Contributor

Hello, thank you for the PR. I wonder if we need the replace photos option on the UI at all. This work mainly supports the Lightroom workflow, I don't see normal usage behavior to use this mechanism. Let me know what you think!

@midzelis midzelis changed the title feat(server, web): API and UI to replace asset data without changing record id feat: API and UI to replace asset data without changing record id May 21, 2024
@midzelis
Copy link
Contributor Author

Hello, thank you for the PR. I wonder if we need the replace photos option on the UI at all. This work mainly supports the Lightroom workflow, I don't see normal usage behavior to use this mechanism. Let me know what you think!

I agree - I think it would be relatively rare to want to do this from the UI, but there may be times where a user would want to do it manually - manually syncing/replacing an asset with an edit, for example. Most users won't be using API, so they won't even know what they would be missing.

Anyways, for now, I've hidden behind a query param, because if nothing else, it is useful for testing purposes. To show the menu option, append ?showDevUI=true to the URL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

5 participants