Allow empty string to clear URL-type DB fields. (#4780)

* Allow empty string to clear URL-type DB fields.

- To address difficulties with clearing URL-type fields like
  avatars, banners, site icons, this PR turns the URL type form
  fields into strings.
- This allows an empty string to be used as a "clear data", as
  in the case with the regular text form fields.
- Also includes various cleanups.
- Fixes #4777
- Context: #2287

* Fixing comment.

* Use Option<&str> and deref.

---------

Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com>

api_tests/src/user.spec.ts

@@ -21,6 +21,7 @@ import {
   fetchFunction,
   alphaImage,
   unfollows,
+  saveUserSettingsBio,
 } from "./shared";
 import { LemmyHttp, SaveUserSettings, UploadImage } from "lemmy-js-client";
 import { GetPosts } from "lemmy-js-client/dist/types/GetPosts";
@@ -198,4 +199,14 @@ test("Set a new avatar, old avatar is deleted", async () => {
   // make sure only the new avatar is kept
   const listMediaRes3 = await alphaImage.listMedia();
   expect(listMediaRes3.images.length).toBe(1);
+
+  // Now try to save a user settings, with the icon missing,
+  // and make sure it doesn't clear the data, or delete the image
+  await saveUserSettingsBio(alpha);
+  let site = await getSite(alpha);
+  expect(site.my_user?.local_user_view.person.avatar).toBe(upload2.url);
+
+  // make sure only the new avatar is kept
+  const listMediaRes4 = await alphaImage.listMedia();
+  expect(listMediaRes4.images.length).toBe(1);
 });

crates/api/src/community/ban.rs

@@ -43,7 +43,10 @@ pub async fn ban_from_community(
     &mut context.pool(),
   )
   .await?;
-  is_valid_body_field(&data.reason, false)?;
+
+  if let Some(reason) = &data.reason {
+    is_valid_body_field(reason, false)?;
+  }
 
   let community_user_ban_form = CommunityPersonBanForm {
     community_id: data.community_id,

crates/api/src/local_user/ban_person.rs

@@ -31,7 +31,9 @@ pub async fn ban_from_site(
   // Make sure user is an admin
   is_admin(&local_user_view)?;
 
-  is_valid_body_field(&data.reason, false)?;
+  if let Some(reason) = &data.reason {
+    is_valid_body_field(reason, false)?;
+  }
 
   let expires = check_expire_time(data.expires)?;
 

crates/api/src/local_user/save_settings.rs

@@ -21,7 +21,7 @@ use lemmy_db_schema::{
     person::{Person, PersonUpdateForm},
   },
   traits::Crud,
-  utils::diesel_option_overwrite,
+  utils::{diesel_string_update, diesel_url_update},
 };
 use lemmy_db_views::structs::{LocalUserView, SiteView};
 use lemmy_utils::{
@@ -42,18 +42,24 @@ pub async fn save_user_settings(
 
   let slur_regex = local_site_to_slur_regex(&site_view.local_site);
   let url_blocklist = get_url_blocklist(&context).await?;
-  let bio = diesel_option_overwrite(
+  let bio = diesel_string_update(
-    process_markdown_opt(&data.bio, &slur_regex, &url_blocklist, &context).await?,
+    process_markdown_opt(&data.bio, &slur_regex, &url_blocklist, &context)
+      .await?
+      .as_deref(),
   );
-  replace_image(&data.avatar, &local_user_view.person.avatar, &context).await?;
-  replace_image(&data.banner, &local_user_view.person.banner, &context).await?;
 
-  let avatar = proxy_image_link_opt_api(&data.avatar, &context).await?;
+  let avatar = diesel_url_update(data.avatar.as_deref())?;
-  let banner = proxy_image_link_opt_api(&data.banner, &context).await?;
+  replace_image(&avatar, &local_user_view.person.avatar, &context).await?;
-  let display_name = diesel_option_overwrite(data.display_name.clone());
+  let avatar = proxy_image_link_opt_api(avatar, &context).await?;
-  let matrix_user_id = diesel_option_overwrite(data.matrix_user_id.clone());
+
+  let banner = diesel_url_update(data.banner.as_deref())?;
+  replace_image(&banner, &local_user_view.person.banner, &context).await?;
+  let banner = proxy_image_link_opt_api(banner, &context).await?;
+
+  let display_name = diesel_string_update(data.display_name.as_deref());
+  let matrix_user_id = diesel_string_update(data.matrix_user_id.as_deref());
   let email_deref = data.email.as_deref().map(str::to_lowercase);
-  let email = diesel_option_overwrite(email_deref.clone());
+  let email = diesel_string_update(email_deref.as_deref());
 
   if let Some(Some(email)) = &email {
     let previous_email = local_user_view.local_user.email.clone().unwrap_or_default();

crates/api/src/post/get_link_metadata.rs

@@ -4,14 +4,19 @@ use lemmy_api_common::{
   post::{GetSiteMetadata, GetSiteMetadataResponse},
   request::fetch_link_metadata,
 };
-use lemmy_utils::error::LemmyResult;
+use lemmy_utils::{
+  error::{LemmyErrorExt, LemmyResult},
+  LemmyErrorType,
+};
+use url::Url;
 
 #[tracing::instrument(skip(context))]
 pub async fn get_link_metadata(
   data: Query<GetSiteMetadata>,
   context: Data<LemmyContext>,
 ) -> LemmyResult<Json<GetSiteMetadataResponse>> {
-  let metadata = fetch_link_metadata(&data.url, &context).await?;
+  let url = Url::parse(&data.url).with_lemmy_type(LemmyErrorType::InvalidUrl)?;
+  let metadata = fetch_link_metadata(&url, &context).await?;
 
   Ok(Json(GetSiteMetadataResponse { metadata }))
 }

crates/api/src/site/registration_applications/approve.rs

@@ -10,7 +10,7 @@ use lemmy_db_schema::{
     registration_application::{RegistrationApplication, RegistrationApplicationUpdateForm},
   },
   traits::Crud,
-  utils::diesel_option_overwrite,
+  utils::diesel_string_update,
 };
 use lemmy_db_views::structs::{LocalUserView, RegistrationApplicationView};
 use lemmy_utils::{error::LemmyResult, LemmyErrorType};
@@ -26,7 +26,7 @@ pub async fn approve_registration_application(
   is_admin(&local_user_view)?;
 
   // Update the registration with reason, admin_id
-  let deny_reason = diesel_option_overwrite(data.deny_reason.clone());
+  let deny_reason = diesel_string_update(data.deny_reason.as_deref());
   let app_form = RegistrationApplicationUpdateForm {
     admin_id: Some(Some(local_user_view.person.id)),
     deny_reason,

crates/api_common/src/post.rs

@@ -10,7 +10,6 @@ use serde::{Deserialize, Serialize};
 use serde_with::skip_serializing_none;
 #[cfg(feature = "full")]
 use ts_rs::TS;
-use url::Url;
 
 #[skip_serializing_none]
 #[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq, Hash)]
@@ -20,8 +19,7 @@ use url::Url;
 pub struct CreatePost {
   pub name: String,
   pub community_id: CommunityId,
-  #[cfg_attr(feature = "full", ts(type = "string"))]
+  pub url: Option<String>,
-  pub url: Option<Url>,
   /// An optional body for the post in markdown.
   pub body: Option<String>,
   /// An optional alt_text, usable for image posts.
@@ -30,9 +28,8 @@ pub struct CreatePost {
   pub honeypot: Option<String>,
   pub nsfw: Option<bool>,
   pub language_id: Option<LanguageId>,
-  #[cfg_attr(feature = "full", ts(type = "string"))]
   /// Instead of fetching a thumbnail, use a custom one.
-  pub custom_thumbnail: Option<Url>,
+  pub custom_thumbnail: Option<String>,
 }
 
 #[derive(Debug, Serialize, Deserialize, Clone)]
@@ -114,17 +111,15 @@ pub struct CreatePostLike {
 pub struct EditPost {
   pub post_id: PostId,
   pub name: Option<String>,
-  #[cfg_attr(feature = "full", ts(type = "string"))]
+  pub url: Option<String>,
-  pub url: Option<Url>,
   /// An optional body for the post in markdown.
   pub body: Option<String>,
   /// An optional alt_text, usable for image posts.
   pub alt_text: Option<String>,
   pub nsfw: Option<bool>,
   pub language_id: Option<LanguageId>,
-  #[cfg_attr(feature = "full", ts(type = "string"))]
   /// Instead of fetching a thumbnail, use a custom one.
-  pub custom_thumbnail: Option<Url>,
+  pub custom_thumbnail: Option<String>,
 }
 
 #[derive(Debug, Serialize, Deserialize, Clone, Copy, Default, PartialEq, Eq, Hash)]
@@ -249,8 +244,7 @@ pub struct ListPostReportsResponse {
 #[cfg_attr(feature = "full", ts(export))]
 /// Get metadata for a given site.
 pub struct GetSiteMetadata {
-  #[cfg_attr(feature = "full", ts(type = "string"))]
+  pub url: String,
-  pub url: Url,
 }
 
 #[derive(Debug, Serialize, Deserialize, Clone)]

crates/api_common/src/request.rs

@@ -340,15 +340,15 @@ async fn is_image_content_type(client: &ClientWithMiddleware, url: &Url) -> Lemm
 
 /// When adding a new avatar, banner or similar image, delete the old one.
 pub async fn replace_image(
-  new_image: &Option<String>,
+  new_image: &Option<Option<DbUrl>>,
   old_image: &Option<DbUrl>,
   context: &Data<LemmyContext>,
 ) -> LemmyResult<()> {
-  if let (Some(new_image), Some(old_image)) = (new_image, old_image) {
+  if let (Some(Some(new_image)), Some(old_image)) = (new_image, old_image) {
     // Note: Oftentimes front ends will include the current image in the form.
     // In this case, deleting `old_image` would also be deletion of `new_image`,
     // so the deletion must be skipped for the image to be kept.
-    if new_image != old_image.as_str() {
+    if new_image != old_image {
       // Ignore errors because image may be stored externally.
       let image = LocalImage::delete_by_url(&mut context.pool(), old_image)
         .await

crates/api_common/src/utils.rs

@@ -1004,26 +1004,25 @@ pub(crate) async fn proxy_image_link(link: Url, context: &LemmyContext) -> Lemmy
 }
 
 pub async fn proxy_image_link_opt_api(
-  link: &Option<String>,
+  link: Option<Option<DbUrl>>,
   context: &LemmyContext,
 ) -> LemmyResult<Option<Option<DbUrl>>> {
-  proxy_image_link_api(link, context).await.map(Some)
+  if let Some(Some(link)) = link {
+    proxy_image_link(link.into(), context)
+      .await
+      .map(Some)
+      .map(Some)
+  } else {
+    Ok(link)
+  }
 }
 
 pub async fn proxy_image_link_api(
-  link: &Option<String>,
+  link: Option<DbUrl>,
   context: &LemmyContext,
 ) -> LemmyResult<Option<DbUrl>> {
-  let link: Option<DbUrl> = match link.as_ref().map(String::as_str) {
+  if let Some(link) = link {
-    // An empty string is an erase
+    proxy_image_link(link.into(), context).await.map(Some)
-    Some("") => None,
-    Some(str_url) => Url::parse(str_url)
-      .map(|u| Some(u.into()))
-      .with_lemmy_type(LemmyErrorType::InvalidUrl)?,
-    None => None,
-  };
-  if let Some(l) = link {
-    proxy_image_link(l.into(), context).await.map(Some)
   } else {
     Ok(link)
   }
@@ -1130,29 +1129,4 @@ mod tests {
         .is_ok()
     );
   }
-
-  #[tokio::test]
-  #[serial]
-  async fn test_diesel_option_overwrite_to_url() {
-    let context = LemmyContext::init_test_context().await;
-
-    assert!(matches!(
-      proxy_image_link_api(&None, &context).await,
-      Ok(None)
-    ));
-    assert!(matches!(
-      proxy_image_link_opt_api(&Some(String::new()), &context).await,
-      Ok(Some(None))
-    ));
-    assert!(
-      proxy_image_link_opt_api(&Some("invalid_url".to_string()), &context)
-        .await
-        .is_err()
-    );
-    let example_url = "https://lemmy-alpha/image.png";
-    assert!(matches!(
-      proxy_image_link_opt_api(&Some(example_url.to_string()), &context).await,
-      Ok(Some(Some(url))) if url == Url::parse(example_url).unwrap().into()
-    ));
-  }
 }

crates/api_crud/src/comment/create.rs

@@ -47,7 +47,7 @@ pub async fn create_comment(
   let slur_regex = local_site_to_slur_regex(&local_site);
   let url_blocklist = get_url_blocklist(&context).await?;
   let content = process_markdown(&data.content, &slur_regex, &url_blocklist, &context).await?;
-  is_valid_body_field(&Some(content.clone()), false)?;
+  is_valid_body_field(&content, false)?;
 
   // Check for a community ban
   let post_id = data.post_id;

crates/api_crud/src/comment/update.rs

@@ -63,7 +63,9 @@ pub async fn update_comment(
   let slur_regex = local_site_to_slur_regex(&local_site);
   let url_blocklist = get_url_blocklist(&context).await?;
   let content = process_markdown_opt(&data.content, &slur_regex, &url_blocklist, &context).await?;
-  is_valid_body_field(&content, false)?;
+  if let Some(content) = &content {
+    is_valid_body_field(content, false)?;
+  }
 
   let comment_id = data.comment_id;
   let form = CommentUpdateForm {

crates/api_crud/src/community/create.rs

@@ -30,6 +30,7 @@ use lemmy_db_schema::{
     },
   },
   traits::{ApubActor, Crud, Followable, Joinable},
+  utils::diesel_url_create,
 };
 use lemmy_db_views::structs::{LocalUserView, SiteView};
 use lemmy_utils::{
@@ -61,11 +62,18 @@ pub async fn create_community(
   check_slurs(&data.title, &slur_regex)?;
   let description =
     process_markdown_opt(&data.description, &slur_regex, &url_blocklist, &context).await?;
-  let icon = proxy_image_link_api(&data.icon, &context).await?;
+
-  let banner = proxy_image_link_api(&data.banner, &context).await?;
+  let icon = diesel_url_create(data.icon.as_deref())?;
+  let icon = proxy_image_link_api(icon, &context).await?;
+
+  let banner = diesel_url_create(data.banner.as_deref())?;
+  let banner = proxy_image_link_api(banner, &context).await?;
 
   is_valid_actor_name(&data.name, local_site.actor_name_max_length as usize)?;
-  is_valid_body_field(&data.description, false)?;
+
+  if let Some(desc) = &data.description {
+    is_valid_body_field(desc, false)?;
+  }
 
   // Double check for duplicate community actor_ids
   let community_actor_id = generate_local_apub_endpoint(

crates/api_crud/src/community/update.rs

@@ -21,7 +21,7 @@ use lemmy_db_schema::{
     local_site::LocalSite,
   },
   traits::Crud,
-  utils::{diesel_option_overwrite, naive_now},
+  utils::{diesel_string_update, diesel_url_update, naive_now},
 };
 use lemmy_db_views::structs::LocalUserView;
 use lemmy_utils::{
@@ -40,18 +40,28 @@ pub async fn update_community(
   let slur_regex = local_site_to_slur_regex(&local_site);
   let url_blocklist = get_url_blocklist(&context).await?;
   check_slurs_opt(&data.title, &slur_regex)?;
-  let description =
+
-    process_markdown_opt(&data.description, &slur_regex, &url_blocklist, &context).await?;
+  let description = diesel_string_update(
-  is_valid_body_field(&data.description, false)?;
+    process_markdown_opt(&data.description, &slur_regex, &url_blocklist, &context)
+      .await?
+      .as_deref(),
+  );
+
+  if let Some(Some(desc)) = &description {
+    is_valid_body_field(desc, false)?;
+  }
+
   let old_community = Community::read(&mut context.pool(), data.community_id)
     .await?
     .ok_or(LemmyErrorType::CouldntFindCommunity)?;
-  replace_image(&data.icon, &old_community.icon, &context).await?;
-  replace_image(&data.banner, &old_community.banner, &context).await?;
 
-  let description = diesel_option_overwrite(description);
+  let icon = diesel_url_update(data.icon.as_deref())?;
-  let icon = proxy_image_link_opt_api(&data.icon, &context).await?;
+  replace_image(&icon, &old_community.icon, &context).await?;
-  let banner = proxy_image_link_opt_api(&data.banner, &context).await?;
+  let icon = proxy_image_link_opt_api(icon, &context).await?;
+
+  let banner = diesel_url_update(data.banner.as_deref())?;
+  replace_image(&banner, &old_community.banner, &context).await?;
+  let banner = proxy_image_link_opt_api(banner, &context).await?;
 
   // Verify its a mod (only mods can edit it)
   check_community_mod_action(

crates/api_crud/src/post/create.rs

@@ -26,6 +26,7 @@ use lemmy_db_schema::{
     post::{Post, PostInsertForm, PostLike, PostLikeForm, PostUpdateForm},
   },
   traits::{Crud, Likeable},
+  utils::diesel_url_create,
   CommunityVisibility,
 };
 use lemmy_db_views::structs::LocalUserView;
@@ -37,7 +38,6 @@ use lemmy_utils::{
     slurs::check_slurs,
     validation::{
       check_url_scheme,
-      clean_url_params,
       is_url_blocked,
       is_valid_alt_text_field,
       is_valid_body_field,
@@ -64,16 +64,27 @@ pub async fn create_post(
   let url_blocklist = get_url_blocklist(&context).await?;
 
   let body = process_markdown_opt(&data.body, &slur_regex, &url_blocklist, &context).await?;
-  let data_url = data.url.as_ref();
+  let url = diesel_url_create(data.url.as_deref())?;
-  let url = data_url.map(clean_url_params); // TODO no good way to handle a "clear"
+  let custom_thumbnail = diesel_url_create(data.custom_thumbnail.as_deref())?;
-  let custom_thumbnail = data.custom_thumbnail.as_ref().map(clean_url_params);
 
   is_valid_post_title(&data.name)?;
-  is_valid_body_field(&body, true)?;
+
-  is_valid_alt_text_field(&data.alt_text)?;
+  if let Some(url) = &url {
-  is_url_blocked(&url, &url_blocklist)?;
+    is_url_blocked(url, &url_blocklist)?;
-  check_url_scheme(&url)?;
+    check_url_scheme(url)?;
-  check_url_scheme(&custom_thumbnail)?;
+  }
+
+  if let Some(custom_thumbnail) = &custom_thumbnail {
+    check_url_scheme(custom_thumbnail)?;
+  }
+
+  if let Some(alt_text) = &data.alt_text {
+    is_valid_alt_text_field(alt_text)?;
+  }
+
+  if let Some(body) = &body {
+    is_valid_body_field(body, true)?;
+  }
 
   check_community_user_action(
     &local_user_view.person,
@@ -156,7 +167,7 @@ pub async fn create_post(
 
   generate_post_link_metadata(
     updated_post.clone(),
-    custom_thumbnail,
+    custom_thumbnail.map(Into::into),
     |post| Some(SendActivityData::CreatePost(post)),
     Some(local_site),
     context.reset_request_count(),

crates/api_crud/src/post/update.rs

@@ -20,16 +20,15 @@ use lemmy_db_schema::{
     post::{Post, PostUpdateForm},
   },
   traits::Crud,
-  utils::{diesel_option_overwrite, naive_now},
+  utils::{diesel_string_update, diesel_url_update, naive_now},
 };
 use lemmy_db_views::structs::LocalUserView;
 use lemmy_utils::{
   error::{LemmyErrorExt, LemmyErrorType, LemmyResult},
   utils::{
-    slurs::check_slurs_opt,
+    slurs::check_slurs,
     validation::{
       check_url_scheme,
-      clean_url_params,
       is_url_blocked,
       is_valid_alt_text_field,
       is_valid_body_field,
@@ -47,26 +46,43 @@ pub async fn update_post(
 ) -> LemmyResult<Json<PostResponse>> {
   let local_site = LocalSite::read(&mut context.pool()).await?;
 
-  // TODO No good way to handle a clear.
+  let url = diesel_url_update(data.url.as_deref())?;
-  // Issue link: https://github.com/LemmyNet/lemmy/issues/2287
+
-  let url = data.url.as_ref().map(clean_url_params);
+  let custom_thumbnail = diesel_url_update(data.custom_thumbnail.as_deref())?;
-  let custom_thumbnail = data.custom_thumbnail.as_ref().map(clean_url_params);
 
   let url_blocklist = get_url_blocklist(&context).await?;
 
   let slur_regex = local_site_to_slur_regex(&local_site);
-  check_slurs_opt(&data.name, &slur_regex)?;
+
-  let body = process_markdown_opt(&data.body, &slur_regex, &url_blocklist, &context).await?;
+  let body = diesel_string_update(
+    process_markdown_opt(&data.body, &slur_regex, &url_blocklist, &context)
+      .await?
+      .as_deref(),
+  );
+
+  let alt_text = diesel_string_update(data.alt_text.as_deref());
 
   if let Some(name) = &data.name {
     is_valid_post_title(name)?;
+    check_slurs(name, &slur_regex)?;
   }
 
-  is_valid_body_field(&body, true)?;
+  if let Some(Some(body)) = &body {
-  is_valid_alt_text_field(&data.alt_text)?;
+    is_valid_body_field(body, true)?;
-  is_url_blocked(&url, &url_blocklist)?;
+  }
-  check_url_scheme(&url)?;
+
-  check_url_scheme(&custom_thumbnail)?;
+  if let Some(Some(alt_text)) = &alt_text {
+    is_valid_alt_text_field(alt_text)?;
+  }
+
+  if let Some(Some(url)) = &url {
+    is_url_blocked(url, &url_blocklist)?;
+    check_url_scheme(url)?;
+  }
+
+  if let Some(Some(custom_thumbnail)) = &custom_thumbnail {
+    check_url_scheme(custom_thumbnail)?;
+  }
 
   let post_id = data.post_id;
   let orig_post = Post::read(&mut context.pool(), post_id)
@@ -95,9 +111,9 @@ pub async fn update_post(
 
   let post_form = PostUpdateForm {
     name: data.name.clone(),
-    url: Some(url.map(Into::into)),
+    url,
-    body: diesel_option_overwrite(body),
+    body,
-    alt_text: diesel_option_overwrite(data.alt_text.clone()),
+    alt_text,
     nsfw: data.nsfw,
     language_id: data.language_id,
     updated: Some(Some(naive_now())),
@@ -111,7 +127,7 @@ pub async fn update_post(
 
   generate_post_link_metadata(
     updated_post.clone(),
-    custom_thumbnail,
+    custom_thumbnail.flatten().map(Into::into),
     |post| Some(SendActivityData::UpdatePost(post)),
     Some(local_site),
     context.reset_request_count(),

crates/api_crud/src/private_message/create.rs

@@ -39,7 +39,7 @@ pub async fn create_private_message(
   let slur_regex = local_site_to_slur_regex(&local_site);
   let url_blocklist = get_url_blocklist(&context).await?;
   let content = process_markdown(&data.content, &slur_regex, &url_blocklist, &context).await?;
-  is_valid_body_field(&Some(content.clone()), false)?;
+  is_valid_body_field(&content, false)?;
 
   check_person_block(
     local_user_view.person.id,

crates/api_crud/src/private_message/update.rs

@@ -41,7 +41,7 @@ pub async fn update_private_message(
   let slur_regex = local_site_to_slur_regex(&local_site);
   let url_blocklist = get_url_blocklist(&context).await?;
   let content = process_markdown(&data.content, &slur_regex, &url_blocklist, &context).await?;
-  is_valid_body_field(&Some(content.clone()), false)?;
+  is_valid_body_field(&content, false)?;
 
   let private_message_id = data.private_message_id;
   PrivateMessage::update(

crates/api_crud/src/site/create.rs

@@ -11,7 +11,7 @@ use lemmy_api_common::{
     local_site_rate_limit_to_rate_limit_config,
     local_site_to_slur_regex,
     process_markdown_opt,
-    proxy_image_link_opt_api,
+    proxy_image_link_api,
   },
 };
 use lemmy_db_schema::{
@@ -23,7 +23,7 @@ use lemmy_db_schema::{
     tagline::Tagline,
   },
   traits::Crud,
-  utils::{diesel_option_overwrite, naive_now},
+  utils::{diesel_string_update, diesel_url_create, naive_now},
 };
 use lemmy_db_views::structs::{LocalUserView, SiteView};
 use lemmy_utils::{
@@ -61,21 +61,25 @@ pub async fn create_site(
   let slur_regex = local_site_to_slur_regex(&local_site);
   let url_blocklist = get_url_blocklist(&context).await?;
   let sidebar = process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context).await?;
-  let icon = proxy_image_link_opt_api(&data.icon, &context).await?;
+
-  let banner = proxy_image_link_opt_api(&data.banner, &context).await?;
+  let icon = diesel_url_create(data.icon.as_deref())?;
+  let icon = proxy_image_link_api(icon, &context).await?;
+
+  let banner = diesel_url_create(data.banner.as_deref())?;
+  let banner = proxy_image_link_api(banner, &context).await?;
 
   let site_form = SiteUpdateForm {
     name: Some(data.name.clone()),
-    sidebar: diesel_option_overwrite(sidebar),
+    sidebar: diesel_string_update(sidebar.as_deref()),
-    description: diesel_option_overwrite(data.description.clone()),
+    description: diesel_string_update(data.description.as_deref()),
-    icon,
+    icon: Some(icon),
-    banner,
+    banner: Some(banner),
     actor_id: Some(actor_id),
     last_refreshed_at: Some(naive_now()),
     inbox_url,
     private_key: Some(Some(keypair.private_key)),
     public_key: Some(keypair.public_key),
-    content_warning: diesel_option_overwrite(data.content_warning.clone()),
+    content_warning: diesel_string_update(data.content_warning.as_deref()),
     ..Default::default()
   };
 
@@ -91,16 +95,16 @@ pub async fn create_site(
     enable_nsfw: data.enable_nsfw,
     community_creation_admin_only: data.community_creation_admin_only,
     require_email_verification: data.require_email_verification,
-    application_question: diesel_option_overwrite(data.application_question.clone()),
+    application_question: diesel_string_update(data.application_question.as_deref()),
     private_instance: data.private_instance,
     default_theme: data.default_theme.clone(),
     default_post_listing_type: data.default_post_listing_type,
     default_sort_type: data.default_sort_type,
-    legal_information: diesel_option_overwrite(data.legal_information.clone()),
+    legal_information: diesel_string_update(data.legal_information.as_deref()),
     application_email_admins: data.application_email_admins,
     hide_modlog_mod_names: data.hide_modlog_mod_names,
     updated: Some(Some(naive_now())),
-    slur_filter_regex: diesel_option_overwrite(data.slur_filter_regex.clone()),
+    slur_filter_regex: diesel_string_update(data.slur_filter_regex.as_deref()),
     actor_name_max_length: data.actor_name_max_length,
     federation_enabled: data.federation_enabled,
     captcha_enabled: data.captcha_enabled,
@@ -179,7 +183,9 @@ fn validate_create_payload(local_site: &LocalSite, create_site: &CreateSite) ->
   )?;
 
   // Ensure that the sidebar has fewer than the max num characters...
-  is_valid_body_field(&create_site.sidebar, false)?;
+  if let Some(body) = &create_site.sidebar {
+    is_valid_body_field(body, false)?;
+  }
 
   application_question_check(
     &local_site.application_question,

crates/api_crud/src/site/update.rs

@@ -27,7 +27,7 @@ use lemmy_db_schema::{
     tagline::Tagline,
   },
   traits::Crud,
-  utils::{diesel_option_overwrite, naive_now},
+  utils::{diesel_string_update, diesel_url_update, naive_now},
   RegistrationMode,
 };
 use lemmy_db_views::structs::{LocalUserView, SiteView};
@@ -67,22 +67,29 @@ pub async fn update_site(
     SiteLanguage::update(&mut context.pool(), discussion_languages.clone(), &site).await?;
   }
 
-  replace_image(&data.icon, &site.icon, &context).await?;
-  replace_image(&data.banner, &site.banner, &context).await?;
-
   let slur_regex = local_site_to_slur_regex(&local_site);
   let url_blocklist = get_url_blocklist(&context).await?;
-  let sidebar = process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context).await?;
+  let sidebar = diesel_string_update(
-  let icon = proxy_image_link_opt_api(&data.icon, &context).await?;
+    process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context)
-  let banner = proxy_image_link_opt_api(&data.banner, &context).await?;
+      .await?
+      .as_deref(),
+  );
+
+  let icon = diesel_url_update(data.icon.as_deref())?;
+  replace_image(&icon, &site.icon, &context).await?;
+  let icon = proxy_image_link_opt_api(icon, &context).await?;
+
+  let banner = diesel_url_update(data.banner.as_deref())?;
+  replace_image(&banner, &site.banner, &context).await?;
+  let banner = proxy_image_link_opt_api(banner, &context).await?;
 
   let site_form = SiteUpdateForm {
     name: data.name.clone(),
-    sidebar: diesel_option_overwrite(sidebar),
+    sidebar,
-    description: diesel_option_overwrite(data.description.clone()),
+    description: diesel_string_update(data.description.as_deref()),
     icon,
     banner,
-    content_warning: diesel_option_overwrite(data.content_warning.clone()),
+    content_warning: diesel_string_update(data.content_warning.as_deref()),
     updated: Some(Some(naive_now())),
     ..Default::default()
   };
@@ -99,16 +106,16 @@ pub async fn update_site(
     enable_nsfw: data.enable_nsfw,
     community_creation_admin_only: data.community_creation_admin_only,
     require_email_verification: data.require_email_verification,
-    application_question: diesel_option_overwrite(data.application_question.clone()),
+    application_question: diesel_string_update(data.application_question.as_deref()),
     private_instance: data.private_instance,
     default_theme: data.default_theme.clone(),
     default_post_listing_type: data.default_post_listing_type,
     default_sort_type: data.default_sort_type,
-    legal_information: diesel_option_overwrite(data.legal_information.clone()),
+    legal_information: diesel_string_update(data.legal_information.as_deref()),
     application_email_admins: data.application_email_admins,
     hide_modlog_mod_names: data.hide_modlog_mod_names,
     updated: Some(Some(naive_now())),
-    slur_filter_regex: diesel_option_overwrite(data.slur_filter_regex.clone()),
+    slur_filter_regex: diesel_string_update(data.slur_filter_regex.as_deref()),
     actor_name_max_length: data.actor_name_max_length,
     federation_enabled: data.federation_enabled,
     captcha_enabled: data.captcha_enabled,
@@ -229,7 +236,9 @@ fn validate_update_payload(local_site: &LocalSite, edit_site: &EditSite) -> Lemm
   )?;
 
   // Ensure that the sidebar has fewer than the max num characters...
-  is_valid_body_field(&edit_site.sidebar, false)?;
+  if let Some(body) = &edit_site.sidebar {
+    is_valid_body_field(body, false)?;
+  }
 
   application_question_check(
     &local_site.application_question,

crates/apub/src/objects/post.rs

@@ -219,7 +219,10 @@ impl Object for ApubPost {
     } else {
       None
     };
-    check_url_scheme(&url)?;
+
+    if let Some(url) = &url {
+      check_url_scheme(url)?;
+    }
 
     let alt_text = first_attachment.cloned().and_then(Attachment::alt_text);
 

crates/db_schema/src/utils.rs

@@ -29,6 +29,7 @@ use i_love_jesus::CursorKey;
 use lemmy_utils::{
   error::{LemmyErrorExt, LemmyErrorType, LemmyResult},
   settings::SETTINGS,
+  utils::validation::clean_url_params,
 };
 use once_cell::sync::Lazy;
 use regex::Regex;
@@ -287,37 +288,35 @@ pub fn is_email_regex(test: &str) -> bool {
   EMAIL_REGEX.is_match(test)
 }
 
-pub fn diesel_option_overwrite(opt: Option<String>) -> Option<Option<String>> {
+/// Takes an API text input, and converts it to an optional diesel DB update.
+pub fn diesel_string_update(opt: Option<&str>) -> Option<Option<String>> {
   match opt {
     // An empty string is an erase
-    Some(unwrapped) => {
+    Some("") => Some(None),
-      if !unwrapped.eq("") {
+    Some(str) => Some(Some(str.into())),
-        Some(Some(unwrapped))
-      } else {
-        Some(None)
-      }
-    }
     None => None,
   }
 }
 
-pub fn diesel_option_overwrite_to_url(opt: &Option<String>) -> LemmyResult<Option<Option<DbUrl>>> {
+/// Takes an optional API URL-type input, and converts it to an optional diesel DB update.
-  match opt.as_ref().map(String::as_str) {
+/// Also cleans the url params.
+pub fn diesel_url_update(opt: Option<&str>) -> LemmyResult<Option<Option<DbUrl>>> {
+  match opt {
     // An empty string is an erase
     Some("") => Ok(Some(None)),
     Some(str_url) => Url::parse(str_url)
-      .map(|u| Some(Some(u.into())))
+      .map(|u| Some(Some(clean_url_params(&u).into())))
       .with_lemmy_type(LemmyErrorType::InvalidUrl),
     None => Ok(None),
   }
 }
 
-pub fn diesel_option_overwrite_to_url_create(opt: &Option<String>) -> LemmyResult<Option<DbUrl>> {
+/// Takes an optional API URL-type input, and converts it to an optional diesel DB create.
-  match opt.as_ref().map(String::as_str) {
+/// Also cleans the url params.
-    // An empty string is nothing
+pub fn diesel_url_create(opt: Option<&str>) -> LemmyResult<Option<DbUrl>> {
-    Some("") => Ok(None),
+  match opt {
     Some(str_url) => Url::parse(str_url)
-      .map(|u| Some(u.into()))
+      .map(|u| Some(clean_url_params(&u).into()))
       .with_lemmy_type(LemmyErrorType::InvalidUrl),
     None => Ok(None),
   }
@@ -569,7 +568,6 @@ impl<RF, LF> Queries<RF, LF> {
 }
 
 #[cfg(test)]
-#[allow(clippy::unwrap_used)]
 #[allow(clippy::indexing_slicing)]
 mod tests {
 
@@ -593,26 +591,24 @@ mod tests {
 
   #[test]
   fn test_diesel_option_overwrite() {
-    assert_eq!(diesel_option_overwrite(None), None);
+    assert_eq!(diesel_string_update(None), None);
-    assert_eq!(diesel_option_overwrite(Some(String::new())), Some(None));
+    assert_eq!(diesel_string_update(Some("")), Some(None));
     assert_eq!(
-      diesel_option_overwrite(Some("test".to_string())),
+      diesel_string_update(Some("test")),
       Some(Some("test".to_string()))
     );
   }
 
   #[test]
-  fn test_diesel_option_overwrite_to_url() {
+  fn test_diesel_option_overwrite_to_url() -> LemmyResult<()> {
-    assert!(matches!(diesel_option_overwrite_to_url(&None), Ok(None)));
+    assert!(matches!(diesel_url_update(None), Ok(None)));
-    assert!(matches!(
+    assert!(matches!(diesel_url_update(Some("")), Ok(Some(None))));
-      diesel_option_overwrite_to_url(&Some(String::new())),
+    assert!(diesel_url_update(Some("invalid_url")).is_err());
-      Ok(Some(None))
-    ));
-    assert!(diesel_option_overwrite_to_url(&Some("invalid_url".to_string())).is_err());
     let example_url = "https://example.com";
     assert!(matches!(
-      diesel_option_overwrite_to_url(&Some(example_url.to_string())),
+      diesel_url_update(Some(example_url)),
-      Ok(Some(Some(url))) if url == Url::parse(example_url).unwrap().into()
+      Ok(Some(Some(url))) if url == Url::parse(example_url)?.into()
     ));
+    Ok(())
   }
 }

crates/utils/src/utils/validation.rs

@@ -158,14 +158,12 @@ pub fn is_valid_post_title(title: &str) -> LemmyResult<()> {
 }
 
 /// This could be post bodies, comments, or any description field
-pub fn is_valid_body_field(body: &Option<String>, post: bool) -> LemmyResult<()> {
+pub fn is_valid_body_field(body: &str, post: bool) -> LemmyResult<()> {
-  if let Some(body) = body {
+  if post {
-    if post {
+    max_length_check(body, POST_BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?;
-      max_length_check(body, POST_BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?;
+  } else {
-    } else {
+    max_length_check(body, BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?;
-      max_length_check(body, BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?;
+  };
-    };
-  }
   Ok(())
 }
 
@@ -173,16 +171,14 @@ pub fn is_valid_bio_field(bio: &str) -> LemmyResult<()> {
   max_length_check(bio, BIO_MAX_LENGTH, LemmyErrorType::BioLengthOverflow)
 }
 
-pub fn is_valid_alt_text_field(alt_text: &Option<String>) -> LemmyResult<()> {
+pub fn is_valid_alt_text_field(alt_text: &str) -> LemmyResult<()> {
-  if let Some(alt_text) = alt_text {
+  max_length_check(
-    max_length_check(
+    alt_text,
-      alt_text,
+    ALT_TEXT_MAX_LENGTH,
-      ALT_TEXT_MAX_LENGTH,
+    LemmyErrorType::AltTextLengthOverflow,
-      LemmyErrorType::AltTextLengthOverflow,
+  )?;
-    )
+
-  } else {
+  Ok(())
-    Ok(())
-  }
 }
 
 /// Checks the site name length, the limit as defined in the DB.
@@ -287,23 +283,17 @@ pub fn check_site_visibility_valid(
   }
 }
 
-pub fn check_url_scheme(url: &Option<Url>) -> LemmyResult<()> {
+pub fn check_url_scheme(url: &Url) -> LemmyResult<()> {
-  if let Some(url) = url {
+  if !ALLOWED_POST_URL_SCHEMES.contains(&url.scheme()) {
-    if !ALLOWED_POST_URL_SCHEMES.contains(&url.scheme()) {
+    Err(LemmyErrorType::InvalidUrlScheme)?
-      Err(LemmyErrorType::InvalidUrlScheme.into())
-    } else {
-      Ok(())
-    }
-  } else {
-    Ok(())
   }
+
+  Ok(())
 }
 
-pub fn is_url_blocked(url: &Option<Url>, blocklist: &RegexSet) -> LemmyResult<()> {
+pub fn is_url_blocked(url: &Url, blocklist: &RegexSet) -> LemmyResult<()> {
-  if let Some(url) = url {
+  if blocklist.is_match(url.as_str()) {
-    if blocklist.is_match(url.as_str()) {
+    Err(LemmyErrorType::BlockedUrl)?
-      Err(LemmyErrorType::BlockedUrl)?
-    }
   }
 
   Ok(())
@@ -350,12 +340,11 @@ pub fn build_url_str_without_scheme(url_str: &str) -> LemmyResult<String> {
 }
 
 #[cfg(test)]
-#[allow(clippy::unwrap_used)]
 #[allow(clippy::indexing_slicing)]
 mod tests {
 
   use crate::{
-    error::LemmyErrorType,
+    error::{LemmyErrorType, LemmyResult},
     utils::validation::{
       build_and_check_regex,
       check_site_visibility_valid,
@@ -379,15 +368,17 @@ mod tests {
   use url::Url;
 
   #[test]
-  fn test_clean_url_params() {
+  fn test_clean_url_params() -> LemmyResult<()> {
-    let url = Url::parse("https://example.com/path/123?utm_content=buffercf3b2&utm_medium=social&username=randomuser&id=123").unwrap();
+    let url = Url::parse("https://example.com/path/123?utm_content=buffercf3b2&utm_medium=social&username=randomuser&id=123")?;
     let cleaned = clean_url_params(&url);
-    let expected = Url::parse("https://example.com/path/123?username=randomuser&id=123").unwrap();
+    let expected = Url::parse("https://example.com/path/123?username=randomuser&id=123")?;
     assert_eq!(expected.to_string(), cleaned.to_string());
 
-    let url = Url::parse("https://example.com/path/123").unwrap();
+    let url = Url::parse("https://example.com/path/123")?;
     let cleaned = clean_url_params(&url);
     assert_eq!(url.to_string(), cleaned.to_string());
+
+    Ok(())
   }
 
   #[test]
@@ -465,7 +456,7 @@ mod tests {
   }
 
   #[test]
-  fn test_valid_site_name() {
+  fn test_valid_site_name() -> LemmyResult<()> {
     let valid_names = [
       (0..SITE_NAME_MAX_LENGTH).map(|_| 'A').collect::<String>(),
       String::from("A"),
@@ -496,12 +487,13 @@ mod tests {
 
         assert!(result.is_err());
         assert!(
-          result.unwrap_err().error_type.eq(&expected_err.clone()),
+          result.is_err_and(|e| e.error_type.eq(&expected_err.clone())),
           "Testing {}, expected error {}",
           invalid_name,
           expected_err
         );
       });
+    Ok(())
   }
 
   #[test]
@@ -513,10 +505,7 @@ mod tests {
 
     assert!(
       invalid_result.is_err()
-        && invalid_result
+        && invalid_result.is_err_and(|e| e.error_type.eq(&LemmyErrorType::BioLengthOverflow))
-          .unwrap_err()
-          .error_type
-          .eq(&LemmyErrorType::BioLengthOverflow)
     );
   }
 
@@ -537,10 +526,9 @@ mod tests {
 
     assert!(
       invalid_result.is_err()
-        && invalid_result
+        && invalid_result.is_err_and(|e| e
-          .unwrap_err()
           .error_type
-          .eq(&LemmyErrorType::SiteDescriptionLengthOverflow)
+          .eq(&LemmyErrorType::SiteDescriptionLengthOverflow))
     );
   }
 
@@ -570,7 +558,7 @@ mod tests {
 
         assert!(result.is_err());
         assert!(
-          result.unwrap_err().error_type.eq(&expected_err.clone()),
+          result.is_err_and(|e| e.error_type.eq(&expected_err.clone())),
           "Testing regex {:?}, expected error {}",
           regex_str,
           expected_err
@@ -591,38 +579,38 @@ mod tests {
   }
 
   #[test]
-  fn test_check_url_scheme() {
+  fn test_check_url_scheme() -> LemmyResult<()> {
-    assert!(check_url_scheme(&None).is_ok());
+    assert!(check_url_scheme(&Url::parse("http://example.com")?).is_ok());
-    assert!(check_url_scheme(&Some(Url::parse("http://example.com").unwrap())).is_ok());
+    assert!(check_url_scheme(&Url::parse("https://example.com")?).is_ok());
-    assert!(check_url_scheme(&Some(Url::parse("https://example.com").unwrap())).is_ok());
+    assert!(check_url_scheme(&Url::parse("https://example.com")?).is_ok());
-    assert!(check_url_scheme(&Some(Url::parse("https://example.com").unwrap())).is_ok());
+    assert!(check_url_scheme(&Url::parse("ftp://example.com")?).is_err());
-    assert!(check_url_scheme(&Some(Url::parse("ftp://example.com").unwrap())).is_err());
+    assert!(check_url_scheme(&Url::parse("javascript:void")?).is_err());
-    assert!(check_url_scheme(&Some(Url::parse("javascript:void").unwrap())).is_err());
 
     let magnet_link="magnet:?xt=urn:btih:4b390af3891e323778959d5abfff4b726510f14c&dn=Ravel%20Complete%20Piano%20Sheet%20Music%20-%20Public%20Domain&tr=udp%3A%2F%2Fopen.tracker.cl%3A1337%2Fannounce";
-    assert!(check_url_scheme(&Some(Url::parse(magnet_link).unwrap())).is_ok());
+    assert!(check_url_scheme(&Url::parse(magnet_link)?).is_ok());
+
+    Ok(())
   }
 
   #[test]
-  fn test_url_block() {
+  fn test_url_block() -> LemmyResult<()> {
     let set = regex::RegexSet::new(vec![
       r"(https://)?example\.org/page/to/article",
       r"(https://)?example\.net/?",
       r"(https://)?example\.com/?",
-    ])
+    ])?;
-    .unwrap();
 
-    assert!(is_url_blocked(&Some(Url::parse("https://example.blog").unwrap()), &set).is_ok());
+    assert!(is_url_blocked(&Url::parse("https://example.blog")?, &set).is_ok());
 
-    assert!(is_url_blocked(&Some(Url::parse("https://example.org").unwrap()), &set).is_ok());
+    assert!(is_url_blocked(&Url::parse("https://example.org")?, &set).is_ok());
 
-    assert!(is_url_blocked(&None, &set).is_ok());
+    assert!(is_url_blocked(&Url::parse("https://example.com")?, &set).is_err());
 
-    assert!(is_url_blocked(&Some(Url::parse("https://example.com").unwrap()), &set).is_err());
+    Ok(())
   }
 
   #[test]
-  fn test_url_parsed() {
+  fn test_url_parsed() -> LemmyResult<()> {
     // Make sure the scheme is removed, and uniques also
     assert_eq!(
       &check_urls_are_valid(&vec![
@@ -630,8 +618,7 @@ mod tests {
         "http://example.com".to_string(),
         "https://example.com".to_string(),
         "https://example.com/test?q=test2&q2=test3#test4".to_string(),
-      ])
+      ])?,
-      .unwrap(),
       &vec![
         "example.com".to_string(),
         "example.com/test?q=test2&q2=test3#test4".to_string()
@@ -639,5 +626,6 @@ mod tests {
     );
 
     assert!(check_urls_are_valid(&vec!["https://example .com".to_string()]).is_err());
+    Ok(())
   }
 }