From 80c26d61f75ca739e6071cc25b594033face3aa1 Mon Sep 17 00:00:00 2001
From: tobi <31960611+tsmethurst@users.noreply.github.com>
Date: Sat, 4 Feb 2023 15:53:11 +0100
Subject: [PATCH] [bugfix] Allow instance thumbnail description to be set
 separately from image (#1417)

---
 internal/api/client/instance/instancepatch.go |  14 +-
 .../api/client/instance/instancepatch_test.go | 359 ++++++++++--------
 internal/processing/instance.go               |  10 +-
 3 files changed, 212 insertions(+), 171 deletions(-)

diff --git a/internal/api/client/instance/instancepatch.go b/internal/api/client/instance/instancepatch.go
index a71d670b9..c30b5850e 100644
--- a/internal/api/client/instance/instancepatch.go
+++ b/internal/api/client/instance/instancepatch.go
@@ -178,19 +178,17 @@ func validateInstanceUpdate(form *apimodel.InstanceSettingsUpdateRequest) error
 		return errors.New("empty form submitted")
 	}
 
-	maxImageSize := config.GetMediaImageMaxSize()
-	maxDescriptionChars := config.GetMediaDescriptionMaxChars()
-
-	// validate avatar if present
 	if form.Avatar != nil {
+		maxImageSize := config.GetMediaImageMaxSize()
 		if size := form.Avatar.Size; size > int64(maxImageSize) {
 			return fmt.Errorf("file size limit exceeded: limit is %d bytes but desired instance avatar was %d bytes", maxImageSize, size)
 		}
+	}
 
-		if form.AvatarDescription != nil {
-			if length := len([]rune(*form.AvatarDescription)); length > maxDescriptionChars {
-				return fmt.Errorf("avatar description length must be less than %d characters (inclusive), but provided avatar description was %d chars", maxDescriptionChars, length)
-			}
+	if form.AvatarDescription != nil {
+		maxDescriptionChars := config.GetMediaDescriptionMaxChars()
+		if length := len([]rune(*form.AvatarDescription)); length > maxDescriptionChars {
+			return fmt.Errorf("avatar description length must be less than %d characters (inclusive), but provided avatar description was %d chars", maxDescriptionChars, length)
 		}
 	}
 
diff --git a/internal/api/client/instance/instancepatch_test.go b/internal/api/client/instance/instancepatch_test.go
index 0b438b823..8833014f0 100644
--- a/internal/api/client/instance/instancepatch_test.go
+++ b/internal/api/client/instance/instancepatch_test.go
@@ -37,37 +37,44 @@ type InstancePatchTestSuite struct {
 	InstanceStandardTestSuite
 }
 
-func (suite *InstancePatchTestSuite) TestInstancePatch1() {
-	requestBody, w, err := testrig.CreateMultipartFormData(
-		"", "",
-		map[string]string{
-			"title":            "Example Instance",
-			"contact_username": "admin",
-			"contact_email":    "someone@example.org",
-		})
+func (suite *InstancePatchTestSuite) instancePatch(fieldName string, fileName string, extraFields map[string]string) (code int, body []byte) {
+	requestBody, w, err := testrig.CreateMultipartFormData(fieldName, fileName, extraFields)
 	if err != nil {
-		panic(err)
+		suite.FailNow(err.Error())
 	}
-	bodyBytes := requestBody.Bytes()
 
-	// set up the request
 	recorder := httptest.NewRecorder()
-	ctx := suite.newContext(recorder, http.MethodPatch, instance.InstanceInformationPathV1, bodyBytes, w.FormDataContentType(), true)
+	ctx := suite.newContext(recorder, http.MethodPatch, instance.InstanceInformationPathV1, requestBody.Bytes(), w.FormDataContentType(), true)
 
-	// call the handler
 	suite.instanceModule.InstanceUpdatePATCHHandler(ctx)
 
-	// we should have OK because our request was valid
-	suite.Equal(http.StatusOK, recorder.Code)
-
 	result := recorder.Result()
 	defer result.Body.Close()
 
 	b, err := io.ReadAll(result.Body)
-	suite.NoError(err)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	return recorder.Code, b
+}
+
+func (suite *InstancePatchTestSuite) TestInstancePatch1() {
+	code, b := suite.instancePatch("", "", map[string]string{
+		"title":            "Example Instance",
+		"contact_username": "admin",
+		"contact_email":    "someone@example.org",
+	})
+
+	if expectedCode := http.StatusOK; code != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code)
+	}
+
 	dst := new(bytes.Buffer)
-	err = json.Indent(dst, b, "", "  ")
-	suite.NoError(err)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
+
 	suite.Equal(`{
   "uri": "http://localhost:8080",
   "account_domain": "localhost:8080",
@@ -150,34 +157,19 @@ func (suite *InstancePatchTestSuite) TestInstancePatch1() {
 }
 
 func (suite *InstancePatchTestSuite) TestInstancePatch2() {
-	requestBody, w, err := testrig.CreateMultipartFormData(
-		"", "",
-		map[string]string{
-			"title": "<p>Geoff's Instance</p>",
-		})
-	if err != nil {
-		panic(err)
+	code, b := suite.instancePatch("", "", map[string]string{
+		"title": "<p>Geoff's Instance</p>",
+	})
+
+	if expectedCode := http.StatusOK; code != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code)
 	}
-	bodyBytes := requestBody.Bytes()
 
-	// set up the request
-	recorder := httptest.NewRecorder()
-	ctx := suite.newContext(recorder, http.MethodPatch, instance.InstanceInformationPathV1, bodyBytes, w.FormDataContentType(), true)
-
-	// call the handler
-	suite.instanceModule.InstanceUpdatePATCHHandler(ctx)
-
-	// we should have OK because our request was valid
-	suite.Equal(http.StatusOK, recorder.Code)
-
-	result := recorder.Result()
-	defer result.Body.Close()
-
-	b, err := io.ReadAll(result.Body)
-	suite.NoError(err)
 	dst := new(bytes.Buffer)
-	err = json.Indent(dst, b, "", "  ")
-	suite.NoError(err)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
+
 	suite.Equal(`{
   "uri": "http://localhost:8080",
   "account_domain": "localhost:8080",
@@ -260,34 +252,19 @@ func (suite *InstancePatchTestSuite) TestInstancePatch2() {
 }
 
 func (suite *InstancePatchTestSuite) TestInstancePatch3() {
-	requestBody, w, err := testrig.CreateMultipartFormData(
-		"", "",
-		map[string]string{
-			"short_description": "<p>This is some html, which is <em>allowed</em> in short descriptions.</p>",
-		})
-	if err != nil {
-		panic(err)
+	code, b := suite.instancePatch("", "", map[string]string{
+		"short_description": "<p>This is some html, which is <em>allowed</em> in short descriptions.</p>",
+	})
+
+	if expectedCode := http.StatusOK; code != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code)
 	}
-	bodyBytes := requestBody.Bytes()
 
-	// set up the request
-	recorder := httptest.NewRecorder()
-	ctx := suite.newContext(recorder, http.MethodPatch, instance.InstanceInformationPathV1, bodyBytes, w.FormDataContentType(), true)
-
-	// call the handler
-	suite.instanceModule.InstanceUpdatePATCHHandler(ctx)
-
-	// we should have OK because our request was valid
-	suite.Equal(http.StatusOK, recorder.Code)
-
-	result := recorder.Result()
-	defer result.Body.Close()
-
-	b, err := io.ReadAll(result.Body)
-	suite.NoError(err)
 	dst := new(bytes.Buffer)
-	err = json.Indent(dst, b, "", "  ")
-	suite.NoError(err)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
+
 	suite.Equal(`{
   "uri": "http://localhost:8080",
   "account_domain": "localhost:8080",
@@ -370,28 +347,18 @@ func (suite *InstancePatchTestSuite) TestInstancePatch3() {
 }
 
 func (suite *InstancePatchTestSuite) TestInstancePatch4() {
-	requestBody, w, err := testrig.CreateMultipartFormData(
-		"", "",
-		map[string]string{})
-	if err != nil {
-		panic(err)
+	code, b := suite.instancePatch("", "", map[string]string{
+		"": "",
+	})
+
+	if expectedCode := http.StatusBadRequest; code != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code)
 	}
-	bodyBytes := requestBody.Bytes()
 
-	// set up the request
-	recorder := httptest.NewRecorder()
-	ctx := suite.newContext(recorder, http.MethodPatch, instance.InstanceInformationPathV1, bodyBytes, w.FormDataContentType(), true)
-
-	// call the handler
-	suite.instanceModule.InstanceUpdatePATCHHandler(ctx)
-
-	suite.Equal(http.StatusBadRequest, recorder.Code)
-
-	result := recorder.Result()
-	defer result.Body.Close()
-
-	b, err := io.ReadAll(result.Body)
-	suite.NoError(err)
+	dst := new(bytes.Buffer)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
 
 	suite.Equal(`{"error":"Bad Request: empty form submitted"}`, string(b))
 }
@@ -431,34 +398,19 @@ func (suite *InstancePatchTestSuite) TestInstancePatch5() {
 }
 
 func (suite *InstancePatchTestSuite) TestInstancePatch6() {
-	requestBody, w, err := testrig.CreateMultipartFormData(
-		"", "",
-		map[string]string{
-			"contact_email": "",
-		})
-	if err != nil {
-		panic(err)
+	code, b := suite.instancePatch("", "", map[string]string{
+		"contact_email": "",
+	})
+
+	if expectedCode := http.StatusOK; code != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code)
 	}
-	bodyBytes := requestBody.Bytes()
 
-	// set up the request
-	recorder := httptest.NewRecorder()
-	ctx := suite.newContext(recorder, http.MethodPatch, instance.InstanceInformationPathV1, bodyBytes, w.FormDataContentType(), true)
-
-	// call the handler
-	suite.instanceModule.InstanceUpdatePATCHHandler(ctx)
-
-	// we should have OK because our request was valid
-	suite.Equal(http.StatusOK, recorder.Code)
-
-	result := recorder.Result()
-	defer result.Body.Close()
-
-	b, err := io.ReadAll(result.Body)
-	suite.NoError(err)
 	dst := new(bytes.Buffer)
-	err = json.Indent(dst, b, "", "  ")
-	suite.NoError(err)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
+
 	suite.Equal(`{
   "uri": "http://localhost:8080",
   "account_domain": "localhost:8080",
@@ -541,67 +493,40 @@ func (suite *InstancePatchTestSuite) TestInstancePatch6() {
 }
 
 func (suite *InstancePatchTestSuite) TestInstancePatch7() {
-	requestBody, w, err := testrig.CreateMultipartFormData(
-		"", "",
-		map[string]string{
-			"contact_email": "not.an.email.address",
-		})
-	if err != nil {
-		panic(err)
+	code, b := suite.instancePatch("", "", map[string]string{
+		"contact_email": "not.an.email.address",
+	})
+
+	if expectedCode := http.StatusBadRequest; code != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code)
 	}
-	bodyBytes := requestBody.Bytes()
 
-	// set up the request
-	recorder := httptest.NewRecorder()
-	ctx := suite.newContext(recorder, http.MethodPatch, instance.InstanceInformationPathV1, bodyBytes, w.FormDataContentType(), true)
-
-	// call the handler
-	suite.instanceModule.InstanceUpdatePATCHHandler(ctx)
-
-	suite.Equal(http.StatusBadRequest, recorder.Code)
-
-	result := recorder.Result()
-	defer result.Body.Close()
-
-	b, err := io.ReadAll(result.Body)
-	suite.NoError(err)
+	dst := new(bytes.Buffer)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
 
 	suite.Equal(`{"error":"Bad Request: mail: missing '@' or angle-addr"}`, string(b))
 }
 
 func (suite *InstancePatchTestSuite) TestInstancePatch8() {
-	requestBody, w, err := testrig.CreateMultipartFormData(
-		"thumbnail", "../../../../testrig/media/peglin.gif",
-		map[string]string{
-			"thumbnail_description": "A bouncing little green peglin.",
-		})
-	if err != nil {
-		panic(err)
+	code, b := suite.instancePatch("thumbnail", "../../../../testrig/media/peglin.gif", map[string]string{
+		"thumbnail_description": "A bouncing little green peglin."})
+
+	if expectedCode := http.StatusOK; code != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code)
 	}
-	bodyBytes := requestBody.Bytes()
 
-	// set up the request
-	recorder := httptest.NewRecorder()
-	ctx := suite.newContext(recorder, http.MethodPatch, instance.InstanceInformationPathV1, bodyBytes, w.FormDataContentType(), true)
-
-	// call the handler
-	suite.instanceModule.InstanceUpdatePATCHHandler(ctx)
-	suite.Equal(http.StatusOK, recorder.Code)
-
-	result := recorder.Result()
-	defer result.Body.Close()
+	dst := new(bytes.Buffer)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
 
 	instanceAccount, err := suite.db.GetInstanceAccount(context.Background(), "")
 	if err != nil {
 		suite.FailNow(err.Error())
 	}
-	suite.NotEmpty(instanceAccount.AvatarMediaAttachmentID)
 
-	b, err := io.ReadAll(result.Body)
-	suite.NoError(err)
-	dst := new(bytes.Buffer)
-	err = json.Indent(dst, b, "", "  ")
-	suite.NoError(err)
 	suite.Equal(`{
   "uri": "http://localhost:8080",
   "account_domain": "localhost:8080",
@@ -685,7 +610,7 @@ func (suite *InstancePatchTestSuite) TestInstancePatch8() {
 }`, dst.String())
 
 	// extra bonus: check the v2 model thumbnail after the patch
-	instanceV2, err := suite.processor.InstanceGetV2(ctx)
+	instanceV2, err := suite.processor.InstanceGetV2(context.Background())
 	if err != nil {
 		suite.FailNow(err.Error())
 	}
@@ -701,6 +626,118 @@ func (suite *InstancePatchTestSuite) TestInstancePatch8() {
   "thumbnail_description": "A bouncing little green peglin.",
   "blurhash": "LG9t;qRS4YtO.4WDRlt5IXoxtPj["
 }`, string(instanceV2ThumbnailJson))
+
+	// double extra special bonus: now update the image description without changing the image
+	code2, b2 := suite.instancePatch("", "", map[string]string{
+		"thumbnail_description": "updating the thumbnail description without changing anything else!",
+	})
+
+	if expectedCode := http.StatusOK; code2 != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code2)
+	}
+
+	// just extract the value we wanna check, no need to print the whole thing again
+	i := make(map[string]interface{})
+	if err := json.Unmarshal(b2, &i); err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.EqualValues("updating the thumbnail description without changing anything else!", i["thumbnail_description"])
+}
+
+func (suite *InstancePatchTestSuite) TestInstancePatch9() {
+	code, b := suite.instancePatch("", "", map[string]string{
+		"thumbnail_description": "setting a new description without having a custom image set; this should change nothing!",
+	})
+
+	if expectedCode := http.StatusOK; code != expectedCode {
+		suite.FailNowf("wrong status code", "expected %d but got %d", expectedCode, code)
+	}
+
+	dst := new(bytes.Buffer)
+	if err := json.Indent(dst, b, "", "  "); err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.Equal(`{
+  "uri": "http://localhost:8080",
+  "account_domain": "localhost:8080",
+  "title": "GoToSocial Testrig Instance",
+  "description": "\u003cp\u003eThis is the GoToSocial testrig. It doesn't federate or anything.\u003c/p\u003e\u003cp\u003eWhen the testrig is shut down, all data on it will be deleted.\u003c/p\u003e\u003cp\u003eDon't use this in production!\u003c/p\u003e",
+  "short_description": "\u003cp\u003eThis is the GoToSocial testrig. It doesn't federate or anything.\u003c/p\u003e\u003cp\u003eWhen the testrig is shut down, all data on it will be deleted.\u003c/p\u003e\u003cp\u003eDon't use this in production!\u003c/p\u003e",
+  "email": "admin@example.org",
+  "version": "0.0.0-testrig",
+  "registrations": true,
+  "approval_required": true,
+  "invites_enabled": false,
+  "configuration": {
+    "statuses": {
+      "max_characters": 5000,
+      "max_media_attachments": 6,
+      "characters_reserved_per_url": 25
+    },
+    "media_attachments": {
+      "supported_mime_types": [
+        "image/jpeg",
+        "image/gif",
+        "image/png",
+        "image/webp",
+        "video/mp4"
+      ],
+      "image_size_limit": 10485760,
+      "image_matrix_limit": 16777216,
+      "video_size_limit": 41943040,
+      "video_frame_rate_limit": 60,
+      "video_matrix_limit": 16777216
+    },
+    "polls": {
+      "max_options": 6,
+      "max_characters_per_option": 50,
+      "min_expiration": 300,
+      "max_expiration": 2629746
+    },
+    "accounts": {
+      "allow_custom_css": true,
+      "max_featured_tags": 10
+    },
+    "emojis": {
+      "emoji_size_limit": 51200
+    }
+  },
+  "urls": {
+    "streaming_api": "wss://localhost:8080"
+  },
+  "stats": {
+    "domain_count": 2,
+    "status_count": 16,
+    "user_count": 4
+  },
+  "thumbnail": "http://localhost:8080/assets/logo.png",
+  "contact_account": {
+    "id": "01F8MH17FWEB39HZJ76B6VXSKF",
+    "username": "admin",
+    "acct": "admin",
+    "display_name": "",
+    "locked": false,
+    "bot": false,
+    "created_at": "2022-05-17T13:10:59.000Z",
+    "note": "",
+    "url": "http://localhost:8080/@admin",
+    "avatar": "",
+    "avatar_static": "",
+    "header": "http://localhost:8080/assets/default_header.png",
+    "header_static": "http://localhost:8080/assets/default_header.png",
+    "followers_count": 1,
+    "following_count": 1,
+    "statuses_count": 4,
+    "last_status_at": "2021-10-20T10:41:37.000Z",
+    "emojis": [],
+    "fields": [],
+    "enable_rss": true,
+    "role": "admin"
+  },
+  "max_toot_chars": 5000
+}`, dst.String())
 }
 
 func TestInstancePatchTestSuite(t *testing.T) {
diff --git a/internal/processing/instance.go b/internal/processing/instance.go
index f05d83052..8cc7eaa64 100644
--- a/internal/processing/instance.go
+++ b/internal/processing/instance.go
@@ -221,8 +221,8 @@ func (p *processor) InstancePatch(ctx context.Context, form *apimodel.InstanceSe
 
 	var updateInstanceAccount bool
 
-	// process instance avatar if provided
 	if form.Avatar != nil && form.Avatar.Size != 0 {
+		// process instance avatar image + description
 		avatarInfo, err := p.accountProcessor.UpdateAvatar(ctx, form.Avatar, form.AvatarDescription, ia.ID)
 		if err != nil {
 			return nil, gtserror.NewErrorBadRequest(err, "error processing avatar")
@@ -230,10 +230,16 @@ func (p *processor) InstancePatch(ctx context.Context, form *apimodel.InstanceSe
 		ia.AvatarMediaAttachmentID = avatarInfo.ID
 		ia.AvatarMediaAttachment = avatarInfo
 		updateInstanceAccount = true
+	} else if form.AvatarDescription != nil && ia.AvatarMediaAttachment != nil {
+		// process just the description for the existing avatar
+		ia.AvatarMediaAttachment.Description = *form.AvatarDescription
+		if err := p.db.UpdateByID(ctx, ia.AvatarMediaAttachment, ia.AvatarMediaAttachmentID, "description"); err != nil {
+			return nil, gtserror.NewErrorInternalError(fmt.Errorf("db error updating instance avatar description: %s", err))
+		}
 	}
 
-	// process instance header if provided
 	if form.Header != nil && form.Header.Size != 0 {
+		// process instance header image
 		headerInfo, err := p.accountProcessor.UpdateHeader(ctx, form.Header, nil, ia.ID)
 		if err != nil {
 			return nil, gtserror.NewErrorBadRequest(err, "error processing header")