Image api rework #5260

Nutomic · 2024-12-13T10:54:02Z

Move all image related endpoints under /api/v4/image/
Image parameters and responses are now in crates/api_common/src/image.rs, no more exposing of pictrs data structures
Reorganize code in crates/routes/images
Only use adapt_request() for image upload, not for simple things like delete or health check
New endpoints POST /api/v4/account/avatar which directly takes image upload, instead of setting avatar url via /api/v4/account/settings/save
Also POST /api/v4/account/banner, POST /api/v4/community/icon, POST /api/v4/community/banner, POST /api/v4/site/icon and POST /api/v4/site/banner

Nutomic · 2024-12-17T10:47:08Z

src/api_routes_v3.rs

 use lemmy_utils::rate_limit::RateLimitCell;

 // Deprecated, use api v4 instead.
 // When removing api v3, we also need to rewrite all links in database with
-// `/api/v3/image_proxy` to use `/api/v4/image_proxy` instead.
+// `/api/v3/image_proxy` to use `/api/v4/image/proxy` instead.
 pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) {


Indentation was changed in this file, use "hide whitespace" in github to review it.

Nutomic · 2024-12-18T10:59:42Z

Ready to review.

dessalines

Nice job, adding these as API actions is going to be much better.

We still need a way to clear images. I'd add some POST or DELETE endpoints, that just delete the images and return that item. IE POST /site/icon/delete, /community/banner/delete...
- Speaking of which, I see that this PR is the first one that adds HTTP DELETE. Maybe we should use that instead of POST for all our delete API actions.
Banners can't be square. So whether that means adding both a max height and max width, or just unfortunately not being able to resize them.
We're gonna need a DB migration to fix the history of all proxied images. So unfortunately that means doing a DB replace for comment.content, and post.body, where local = true. I believe the regular image endpoints stayed the same, so no need to correct those.

dessalines · 2024-12-19T20:45:36Z

config/defaults.hjson

+    max_banner_size: 512
+    # Prevent users from uploading images for posts or embedding in markdown. Avatars, icons and
+    # banners can still be uploaded.
+    image_upload_disabled: false


Need to run scripts/./update_config_defaults.sh

dessalines · 2024-12-19T20:52:55Z

crates/routes/src/images/upload.rs

+  delete_old_image(&local_user_view.person.banner, &context).await?;
+
+  let form = PersonUpdateForm {
+    banner: Some(Some(image.image_url.into())),


So how are we clearing these now?

You removed the code that sent an empty string to be able to set Some(None) for these fields, so postgres can clear / unset an image. You need to have some way to be able to do this now.

dessalines · 2024-12-19T20:53:28Z

crates/api/src/local_user/save_settings.rs

@@ -46,14 +44,6 @@ pub async fn save_user_settings(
      .as_deref(),
  );

-  let avatar = diesel_url_update(data.avatar.as_deref())?;


By removing this, we don't have a way to clear / unset images, see comment above.

dessalines · 2024-12-19T20:58:48Z

crates/routes/src/images/utils.rs

+
+// Pictrs cannot use proxy
+#[allow(clippy::expect_used)]
+pub(super) static PICTRS_CLIENT: LazyLock<ClientWithMiddleware> = LazyLock::new(|| {


This scares me, using a global static for a client. We should be trying to avoid these as much as possible, and instantiating a single client in the context and passing that around explicitly.

dessalines · 2024-12-19T21:01:40Z

crates/utils/src/settings/structs.rs

+  ///       Otherwise we have to use crop, or use max_width/max_height which throws error
+  ///       if image is larger.
+  /// TODO: whats a sensible default here?
+  #[default(512)]


Banners should never be square, so we probably should do max width + max height.

I found some banner sizes for current websites here.

dessalines · 2024-12-19T21:06:46Z

crates/routes/src/images/upload.rs

+
+pub async fn upload_community_icon(
+  req: HttpRequest,
+  query: Query<CommunityIdQuery>,


dessalines · 2024-12-19T21:15:38Z

crates/utils/src/utils/markdown/image_links.rs

@@ -18,7 +18,7 @@ pub fn markdown_rewrite_image_links(mut src: String) -> (String, Vec<Url>) {
        // If link points to remote domain, replace with proxied link
        if parsed.domain() != Some(&SETTINGS.hostname) {
          let mut proxied = format!(
-            "{}/api/v4/image_proxy?url={}",
+            "{}/api/v4/image/proxy?url={}",


This will be the right endpoint for new ones, but what about for all the past post bodies and comments? This is probably going to need a DB migration that fixes all the past ones, at least for our local images.

dessalines · 2024-12-19T21:18:08Z

src/api_routes_v4.rs

+          .route("/proxy", get().to(image_proxy))
+          .route("/{filename}", get().to(get_image))


I guess the filename one is the same, but the /proxy is new, correct? That means we're going to need to do a DB migration to correct all the past ones, or we're going to have broken proxied images.

Nothing4You · 2024-12-19T23:59:19Z

is there a reason to expose the pict-rs delete token to the user?

i guess this was historically exposed to the user during upload before it got used in the media listing, but there isn't really any need to keep that, now that we have the media listing.
instead, the deletion api should probably just accept the image url itself and fetch the delete token from the user upload table, which can be combined with verifying that either the user is an admin or the user is the uploader.
for older uploads, in the unlikely case that someone recorded deletion tokens prior to lemmy recording which user the upload is associated with, a compatibility api could exist that allows deletion when the token is provided and the upload does not exist in the table.

dullbananas

For handling old image proxy links, I would prefer keeping both the old and new image proxy api endpoints permanently. Replacing old links in markdown content is slow, complicated, error prone, and might break a few image links from outside of Lemmy.

dullbananas · 2024-12-20T00:20:34Z

crates/routes/src/images/download.rs

+  let local_site = LocalSite::read(&mut context.pool()).await?;
+  if local_site.private_instance && local_user_view.is_none() {
+    return Ok(HttpResponse::Unauthorized().finish());
+  }


optimization

Suggested change

let local_site = LocalSite::read(&mut context.pool()).await?;

if local_site.private_instance && local_user_view.is_none() {

return Ok(HttpResponse::Unauthorized().finish());

}

if local_user_view.is_none() {

let local_site = LocalSite::read(&mut context.pool()).await?;

if local_site.private_instance {

return Ok(HttpResponse::Unauthorized().finish());

}

}

dullbananas · 2024-12-20T01:12:50Z

crates/routes/src/images/utils.rs

+pub(super) fn file_type(file_type: Option<String>, name: &str) -> String {
+  file_type
+    .clone()
+    .unwrap_or_else(|| name.split('.').last().unwrap_or("jpg").to_string())
+}


I'm not totally sure yet, but the calls to clone and unwrap_or_else probably shouldn't be in this function's body. The meaning of file_type.clone.unwrap_or_else(|| file_type(name)) could be more intuitive than file_type(file_type, name)

Nutomic added 3 commits December 12, 2024 15:59

Split image endpoints into API v3 and v4

56ab67f

Move into subfolders

05843fb

Upload avatar endpoint and other changes

cfa866a

Nutomic mentioned this pull request Dec 13, 2024

Rework image api LemmyNet/lemmy-js-client#425

Draft

Various other changes

d252be2

fixes #1772 fixes #4001

Nutomic force-pushed the image-api-rework branch from b38a9ee to d252be2 Compare December 13, 2024 14:06

Nutomic added 2 commits December 13, 2024 15:13

clippy

e815778

config options

60ba7af

Nutomic force-pushed the image-api-rework branch from 09b06f0 to 60ba7af Compare December 13, 2024 14:26

fix ts bindings

7c771d2

Nutomic force-pushed the image-api-rework branch from f66e392 to 7c771d2 Compare December 13, 2024 14:32

fix api tests

8ae4b40

Nutomic commented Dec 17, 2024

View reviewed changes

Nutomic added 8 commits December 17, 2024 16:10

Add option to disable image upload (fixes #1118)

b0d4bdb

split files into upload, download

55b8ace

move sitemap to top level, not in api

460ccc8

simplify code

5f06195

add upload user banner

5f49b2a

community icon/banner

a6f7e76

site icon/banner

ada8f1b

update js client

4951aa0

Nutomic marked this pull request as ready for review December 18, 2024 10:58

Nutomic requested review from dessalines, phiresky, dullbananas and SleeplessOne1917 as code owners December 18, 2024 10:58

dessalines reviewed Dec 19, 2024

View reviewed changes

Nothing4You self-requested a review December 19, 2024 22:31

dullbananas requested changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image api rework #5260

Image api rework #5260

Nutomic commented Dec 13, 2024 •

edited

Loading

Nutomic Dec 17, 2024

Nutomic commented Dec 18, 2024

dessalines left a comment •

edited

Loading

dessalines Dec 19, 2024

dessalines Dec 19, 2024

dessalines Dec 19, 2024

dessalines Dec 19, 2024

dessalines Dec 19, 2024

dessalines Dec 19, 2024

dessalines Dec 19, 2024

dessalines Dec 19, 2024

Nothing4You commented Dec 19, 2024

dullbananas left a comment

dullbananas Dec 20, 2024

dullbananas Dec 20, 2024

		.route("/proxy", get().to(image_proxy))
		.route("/{filename}", get().to(get_image))

Image api rework #5260

Are you sure you want to change the base?

Image api rework #5260

Conversation

Nutomic commented Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Nutomic commented Dec 18, 2024

dessalines left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nothing4You commented Dec 19, 2024

dullbananas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Dec 13, 2024 •

edited

Loading

dessalines left a comment •

edited

Loading