Adding admin image deletion endpoint. #5588
Conversation
dessalines
commented
Apr 2, 2025
dessalines
commented
- Also moving the list media endpoints, since these can go under a common heading.
- Fixes Add admin API endpoint to delete or purge images from pict-rs #5563
- Also moving the list media endpoints, since these can go under a common heading. - Fixes #5563
dessalines
requested review from
Nutomic,
phiresky,
dullbananas and
SleeplessOne1917
as code owners
src/api_routes_v4.rs
Outdated
| .service( | ||
| scope("/media") | ||
| .route("/list", get().to(list_media)) | ||
| .route("", delete().to(delete_image)), |
There was a problem hiding this comment.
The user delete image was incorrectly under the admin scope. I moved it here.
src/api_routes_v4.rs
Outdated
| ) | ||
| .route("/proxy", get().to(image_proxy)) | ||
| .route("/health", get().to(pictrs_health)) | ||
| .route("/{filename}", get().to(get_image)), | ||
| .route("/{filename}", get().to(get_image)) | ||
| .route("/list_all", get().to(list_all_media)), |
There was a problem hiding this comment.
Maybe just list would be fine?
There was a problem hiding this comment.
True the all part is unnecessary, and we may want to add more filters in the future.
|
Needs adjustments to api client. |
crates/routes/src/images/delete.rs
Outdated
|
|
||
| LocalImage::delete_by_alias(&mut context.pool(), &data.filename).await?; | ||
|
|
||
| delete_image_from_pictrs(&data.filename, &context).await?; |
There was a problem hiding this comment.
for the admin deletion endpoint, this should branch here depending on whether a purge is desired
There was a problem hiding this comment.
Any time its deleted from the DB, it should also be removed from pictrs. Since this one is an explicit call, it should do both, like the one above it.
There was a problem hiding this comment.
I'm talking about the difference between purge and delete.
This only deletes the current alias, but depending on the content, admins may want to purge the image, ensuring that it is fully deleted from storage, even if there are other aliases besides the one in the request.
There was a problem hiding this comment.
We don't need a flag for this, it should just do one or the other. I think purge is better here, since this is an admin / mod action. I'll fix it for this one.
| LocalImage::delete_by_alias(&mut context.pool(), &data.filename).await?; | ||
|
|
||
| // Use purge, since it should remove any other aliases. | ||
| purge_image_from_pictrs(&data.filename, &context).await?; |
There was a problem hiding this comment.
Agree with using purge here, but then we should also delete other aliases for the same image from LocalImage table. Pict-rs does return an array aliases from the purge endpoint.
There was a problem hiding this comment.
I think that's impossible, as we don't store anything else. I don't even think pictrs exposes real ids.
There was a problem hiding this comment.
POST /internal/purge?alias={alias}Purge a file by it's alias. This removes all aliases and files associated with the query.Available source arguments are
`?alias={alias}` Purge a file by it's alias `?proxy={url}` Purge a proxied file by its URLThis endpoint returns the following JSON
{ "msg": "ok", "aliases": ["asdf.png"] }
The list of aliases can just be matched against the pictrs_alias column in local_image.
