feat: use browser download manager for archive downloads #24121
Conversation
|
Label error. Requires exactly 1 of: changelog:.*. Found: 🖥️web, 🗄️server. A maintainer will add the required label. |
|
Are you aware of #22385 ? |
|
Other direction @danieldietzler ;) |
|
@danieldietzler I recall having seen it before, but I think this PR is unrelated as it is for downloads instead of uploads (which makes things much simpler). Also, the main goal for now is to avoid buffering on the browser, the resumable functionality is just a consequence. |
diogotcorreia
left a comment
diogotcorreia
left a comment
There was a problem hiding this comment.
This is my first significant PR to immich, so I'm sure I got some stuff wrong, especially to where code should go and naming things. There's still a few changes to be made in this PR (see todo list on PR description), but feel free to request changes meanwhile
| @Post('request') | ||
| @Authenticated({ permission: Permission.AssetDownload, sharedLink: true }) | ||
| @Endpoint({ | ||
| summary: 'Prepare download archive', | ||
| description: | ||
| 'Create a download request for the specified assets or album. The response includes one or more tokens that can be used to download groups of assets.', | ||
| history: new HistoryBuilder().added('v2').stable('v2'), | ||
| }) | ||
| prepareDownload(@Auth() auth: AuthDto, @Body() dto: DownloadInfoDto): Promise<PrepareDownloadResponseDto> { | ||
| return this.service.prepareDownload(auth, dto); | ||
| } |
There was a problem hiding this comment.
I'm not fond of the endpoint URL nor the descriptions, so if anyone has suggestions I'd appreciate them
|
|
||
| @Get('archive/:id') | ||
| @Authenticated({ permission: Permission.AssetDownload, sharedLink: true }) | ||
| @FileResponse() | ||
| @HttpCode(HttpStatus.OK) | ||
| @Endpoint({ | ||
| summary: 'Download asset archive from download request', | ||
| description: | ||
| 'Download a ZIP archive corresponding to the given download request. The download request needs to be created first.', | ||
| history: new HistoryBuilder().added('v1').beta('v1').stable('v2'), | ||
| }) | ||
| downloadRequestArchive(@Auth() auth: AuthDto, @Param() { id }: UUIDParamDto): Promise<StreamableFile> { | ||
| return this.service.downloadRequestArchive(auth, id).then(asStreamableFile); | ||
| } |
There was a problem hiding this comment.
Should I merge this into download.repository.ts instead? They have quite different purposes
Perhaps download-request is not the best name either?
| }); | ||
|
|
||
| downloadBlob(data, archiveName); | ||
| toastManager.success($t('downloading')); |
There was a problem hiding this comment.
Note to self: this message needs to be changed to be more descriptive
|
Oh woops, yeah. I was indeed confused by the direction, we don't have anything for downloads in the pipeline right now 😅 |
| } | ||
|
|
||
| async prepareDownload(auth: AuthDto, dto: DownloadInfoDto): Promise<PrepareDownloadResponseDto> { | ||
| const info = await this.getDownloadInfo(auth, dto); |
There was a problem hiding this comment.
This just immediately creates the archive while the user is still waiting for the request to complete. That could take a long time; it should probably happen in a background job instead.
There was a problem hiding this comment.
In fact, if creating it immediately on request was feasible then this whole prepare-download flow wouldn't be necessary at all - it could just be created and then immediately returned.
There was a problem hiding this comment.
Does it? This is equivalent to the /download/info endpoint which doesn't "create" anything, it just resolves which assets should be downloaded and how to group them (given a maximum archive size and optionally an album id).
The only change being made by this PR is that you do not need to make a POST request to download the archive, but instead a GET request (which wasn't possible because there is a limit on the length of the URL).
It might take a while, I haven't tested with many/large assets, but I don't see how it is different from the current implementation
There was a problem hiding this comment.
Ah sorry, I misread what getDownloadInfo does exactly.
3 participants
TODO
Note: very WIP
Potentially on a separate PR
Description
Currently, when downloading more than one asset at a time (i.e., an archive), the contents of the archive are buffered in memory. This does not scale to big archives, causing browsers to fail.
In this PR, we are introducing new API endpoints that instead create a "download request" (essentially a mapping between a key and a list of assets) that allow requests to be made via a GET requests.
This would pave the way for resumable downloads and accurate download progress in the browser itself (see #14725 (comment)).
Fixes #14725
How Has This Been Tested?
For now, just manual testing has been done by creating an album and downloading its entire contents.
E2E Tests WIP
API Changes
Two new endpoints:
POST /download/requestGET /download/archive/:idSecurity Concerns
This would allow any user having access to a shared link to increase the database size in a potentially unbounded way by just creating "download requests" in rapid succession. Not sure if this is something we should worry about.
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
No LLM has or will be used for this PR, just me and my trusty neovim :)