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

dessalines · 2024-06-05T20:28:11Z

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 [Bug]: Site logo disappearing #4777
Context: Editing a post body field will not save an empty string #2287

- 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

dessalines · 2024-06-05T20:32:22Z

crates/utils/src/utils/validation.rs

-      max_length_check(body, BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?;
-    };
-  }
+pub fn is_valid_body_field(body: &str, post: bool) -> LemmyResult<()> {


We already have a lot of if let Some(... in the API code, so I've removed a few of these internal if let wrappers, as they're pretty pointless. Makes sense to just work with the data directly in a lot of cases.

dessalines · 2024-06-05T20:33:09Z

crates/utils/src/utils/validation.rs

@@ -350,12 +340,11 @@ pub fn build_url_str_without_scheme(url_str: &str) -> LemmyResult<String> {
 }

 #[cfg(test)]
-#[allow(clippy::unwrap_used)]


Nothing really important below, just removing the unwraps in favor of using LemmyResult in the tests.

dessalines · 2024-06-05T20:35:01Z

crates/api_crud/src/site/update.rs

+    &process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context).await?,
+  );
+
+  let icon = diesel_url_update(&data.icon)?;


The heart of it is here. We transform these url-type ones into proper diesel forms for an update to an optional url column (IE Option<Option<DbUrl>> ), before doing later actions to it.

dessalines · 2024-06-05T20:36:34Z

api_tests/src/user.spec.ts

@@ -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,


Added a test here to make sure that it isn't clearing the avatar, when that field is missing.

SleeplessOne1917 · 2024-06-06T00:19:17Z

crates/api_common/src/request.rs

  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) {


I think the following should work:

Suggested change

if let (Some(Some(new_image)), Some(old_image)) = (new_image, old_image) {

if let Some((new_image, old_image)) = new_image.flatten().zip(old_image) {

Might work, but seems less clear to me.

Fair. Personally, I find this a bit easier than needing to parse nested Somes.

SleeplessOne1917 · 2024-06-06T00:20:57Z

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>>,


Is there anything preventing us from using a single layer of Option here?

These ones I left as helper functions, because of the internal async call, and not being able to use async map. The one right below this uses the single option version.

SleeplessOne1917 · 2024-06-06T00:21:57Z

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>>,


Same question as here.

SleeplessOne1917 · 2024-06-06T00:22:56Z

crates/api_common/src/utils.rs

  context: &LemmyContext,
 ) -> LemmyResult<Option<Option<DbUrl>>> {
-  proxy_image_link_api(link, context).await.map(Some)
+  if let Some(Some(link)) = link {


Can call flatten on link if you're stuck using 2 levels of Option.

I tried, but at least for this one, it would've forced me to use a clone and deref with it, in this case its cleaner to just do Some(Some .

SleeplessOne1917 · 2024-06-06T00:26:46Z

crates/api_crud/src/post/update.rs

+    check_slurs(name, &slur_regex)?;
+  }
+
+  if let Some(Some(body)) = &body {


Can call flatten on body.

I tried, but in all these cases it forces me to do a .clone(), in order to not move the value of body, and .flatten() doesn't work on refs of options for some reason.

SleeplessOne1917 · 2024-06-06T00:27:08Z

crates/api_crud/src/post/update.rs

+  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)?;
+  }


SleeplessOne1917 · 2024-06-06T00:33:34Z

crates/db_schema/src/utils.rs

-pub fn diesel_option_overwrite(opt: Option<String>) -> Option<Option<String>> {
-  match opt {
+/// Takes an API text input, and converts it to an optional diesel DB update.
+pub fn diesel_string_update(opt: &Option<String>) -> Option<Option<String>> {


Should probably be taking an Option<&String>, maybe even Option<&str>. See here.

I tried that at first, but it forces you to add a lot of deref calls. This one uses the least amount of cloning and mutating.

Does calling as_ref on the param at the site of the function call work for you? It should let you turn, e.g., an &Option<String> to Option<&String>. Docs for reference.

Probably won't make too big a difference, but I've now updated it. Does add a lot of deref calls unfortunately.

Should at least prevent us from needing to clone and pass owned objects around as much, which I think is a win.

SleeplessOne1917 · 2024-06-06T00:41:52Z

I know I've asked the question before, but what is with all of the Option<Option<T>>s? I'm not sure what that type expresses that just Option<T> doesn't already express. I'm inclined to even call it a code smell.

dullbananas · 2024-06-06T01:52:05Z

If I understand correctly, None keeps the existing value, and Some(None) removes the existing value

SleeplessOne1917 · 2024-06-06T06:25:29Z

crates/db_schema/src/utils.rs

-  match opt.as_ref().map(String::as_str) {
+/// Takes an optional API URL-type input, and converts it to an optional diesel DB update.
+/// Also cleans the url params.
+pub fn diesel_url_update(opt: &Option<String>) -> LemmyResult<Option<Option<DbUrl>>> {


Same as here.

SleeplessOne1917 · 2024-06-06T06:26:52Z

If I understand correctly, None keeps the existing value, and Some(None) removes the existing value

I suppose that makes sense. Is that how diesel usually handles this sort of thing, or is that idiosyncratic to our codebase?

dessalines · 2024-06-06T12:21:13Z

If I understand correctly, None keeps the existing value, and Some(None) removes the existing value

That's correct. I made a table for this somewhere but I can't find it, for update type (create or update), column type (nullable vs not null), and default type (has default or no default value). But the main outlier case, is that if its a nullable DB column update, then you need to use Option<Option< on update changesets, so that Some(None can be a set null.

…LemmyNet/lemmy into dont_db_delete_empty_url_fields

dessalines requested review from Nutomic, phiresky, dullbananas and SleeplessOne1917 as code owners June 5, 2024 20:28

Fixing comment.

9eb5d34

dessalines commented Jun 5, 2024

View reviewed changes

dullbananas approved these changes Jun 5, 2024

View reviewed changes

SleeplessOne1917 reviewed Jun 6, 2024

View reviewed changes

Merge branch 'main' into dont_db_delete_empty_url_fields

c6579d8

SleeplessOne1917 reviewed Jun 6, 2024

View reviewed changes

SleeplessOne1917 and others added 3 commits June 6, 2024 08:57

Merge branch 'main' into dont_db_delete_empty_url_fields

9b4b253

Use Option<&str> and deref.

8721985

Merge branch 'dont_db_delete_empty_url_fields' of https://github.com/…

e6bc8f6

…LemmyNet/lemmy into dont_db_delete_empty_url_fields

SleeplessOne1917 approved these changes Jun 6, 2024

View reviewed changes

dessalines merged commit 16a8286 into main Jun 6, 2024
2 checks passed

SleeplessOne1917 deleted the dont_db_delete_empty_url_fields branch June 6, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dessalines commented Jun 5, 2024

dessalines Jun 5, 2024

dessalines Jun 5, 2024

dessalines Jun 5, 2024

dessalines Jun 5, 2024

SleeplessOne1917 Jun 6, 2024

dessalines Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

dessalines Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

dessalines Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

dessalines Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

dessalines Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

dessalines Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

SleeplessOne1917 commented Jun 6, 2024

dullbananas commented Jun 6, 2024

SleeplessOne1917 Jun 6, 2024

SleeplessOne1917 commented Jun 6, 2024

dessalines commented Jun 6, 2024

	if let (Some(Some(new_image)), Some(old_image)) = (new_image, old_image) {
	if let Some((new_image, old_image)) = new_image.flatten().zip(old_image) {

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

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

Conversation

dessalines commented Jun 5, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SleeplessOne1917 commented Jun 6, 2024

dullbananas commented Jun 6, 2024

Choose a reason for hiding this comment

SleeplessOne1917 commented Jun 6, 2024

dessalines commented Jun 6, 2024