[bugfix] Fix Postgres emoji delete, emoji category change (#2570)

* [bugfix] Fix Postgres emoji delete, emoji category change

* revert trace logging

* caching issue

* update tests

internal/api/client/admin/emojicreate.go

@@ -125,7 +125,7 @@ func (m *Module) EmojiCreatePOSTHandler(c *gin.Context) {
 		return
 	}
 
-	apiEmoji, errWithCode := m.processor.Admin().EmojiCreate(c.Request.Context(), authed.Account, authed.User, form)
+	apiEmoji, errWithCode := m.processor.Admin().EmojiCreate(c.Request.Context(), authed.Account, form)
 	if errWithCode != nil {
 		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
 		return

internal/api/client/admin/emojidelete_test.go

@@ -87,7 +87,7 @@ func (suite *EmojiDeleteTestSuite) TestEmojiDelete2() {
 	suite.NoError(err)
 	suite.NotNil(b)
 
-	suite.Equal(`{"error":"Bad Request: EmojiDelete: emoji with id 01GD5KP5CQEE1R3X43Y1EHS2CW was not a local emoji, will not delete"}`, string(b))
+	suite.Equal(`{"error":"Bad Request: emoji with id 01GD5KP5CQEE1R3X43Y1EHS2CW was not a local emoji, will not delete"}`, string(b))
 
 	// emoji should still be in the db
 	dbEmoji, err := suite.db.GetEmojiByID(context.Background(), testEmoji.ID)

internal/api/client/admin/emojiget.go

@@ -89,7 +89,7 @@ func (m *Module) EmojiGETHandler(c *gin.Context) {
 		return
 	}
 
-	emoji, errWithCode := m.processor.Admin().EmojiGet(c.Request.Context(), authed.Account, authed.User, emojiID)
+	emoji, errWithCode := m.processor.Admin().EmojiGet(c.Request.Context(), authed.Account, emojiID)
 	if errWithCode != nil {
 		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
 		return

internal/api/client/admin/emojisget.go

@@ -197,7 +197,17 @@ func (m *Module) EmojisGETHandler(c *gin.Context) {
 		includeEnabled = true
 	}
 
-	resp, errWithCode := m.processor.Admin().EmojisGet(c.Request.Context(), authed.Account, authed.User, domain, includeDisabled, includeEnabled, shortcode, maxShortcodeDomain, minShortcodeDomain, limit)
+	resp, errWithCode := m.processor.Admin().EmojisGet(
+		c.Request.Context(),
+		authed.Account,
+		domain,
+		includeDisabled,
+		includeEnabled,
+		shortcode,
+		maxShortcodeDomain,
+		minShortcodeDomain,
+		limit,
+	)
 	if errWithCode != nil {
 		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
 		return

internal/api/client/admin/emojiupdate_test.go

@@ -339,7 +339,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateDisableLocalEmoji() {
 	b, err := ioutil.ReadAll(result.Body)
 	suite.NoError(err)
 
-	suite.Equal(`{"error":"Bad Request: emojiUpdateDisable: emoji 01F8MH9H8E4VG3KDYJR9EGPXCQ is not a remote emoji, cannot disable it via this endpoint"}`, string(b))
+	suite.Equal(`{"error":"Bad Request: emoji 01F8MH9H8E4VG3KDYJR9EGPXCQ is not a remote emoji, cannot disable it via this endpoint"}`, string(b))
 }
 
 func (suite *EmojiUpdateTestSuite) TestEmojiUpdateModifyRemoteEmoji() {
@@ -372,7 +372,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateModifyRemoteEmoji() {
 	b, err := ioutil.ReadAll(result.Body)
 	suite.NoError(err)
 
-	suite.Equal(`{"error":"Bad Request: emojiUpdateModify: emoji 01GD5KP5CQEE1R3X43Y1EHS2CW is not a local emoji, cannot do a modify action on it"}`, string(b))
+	suite.Equal(`{"error":"Bad Request: emoji 01GD5KP5CQEE1R3X43Y1EHS2CW is not a local emoji, cannot update it via this endpoint"}`, string(b))
 }
 
 func (suite *EmojiUpdateTestSuite) TestEmojiUpdateModifyNoParams() {
@@ -439,7 +439,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateCopyLocalToLocal() {
 	b, err := ioutil.ReadAll(result.Body)
 	suite.NoError(err)
 
-	suite.Equal(`{"error":"Bad Request: emojiUpdateCopy: emoji 01F8MH9H8E4VG3KDYJR9EGPXCQ is not a remote emoji, cannot copy it to local"}`, string(b))
+	suite.Equal(`{"error":"Bad Request: emoji 01F8MH9H8E4VG3KDYJR9EGPXCQ is not a remote emoji, cannot copy it to local"}`, string(b))
 }
 
 func (suite *EmojiUpdateTestSuite) TestEmojiUpdateCopyEmptyShortcode() {
@@ -540,7 +540,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateCopyShortcodeAlreadyInUse() {
 	b, err := ioutil.ReadAll(result.Body)
 	suite.NoError(err)
 
-	suite.Equal(`{"error":"Conflict: emojiUpdateCopy: emoji 01GD5KP5CQEE1R3X43Y1EHS2CW could not be copied, emoji with shortcode rainbow already exists on this instance"}`, string(b))
+	suite.Equal(`{"error":"Conflict: emoji with shortcode rainbow already exists on this instance"}`, string(b))
 }
 
 func TestEmojiUpdateTestSuite(t *testing.T) {

internal/db/bundb/emoji.go

@@ -132,28 +132,32 @@ func (e *emojiDB) DeleteEmojiByID(ctx context.Context, id string) error {
 		}
 
 		for _, statusID := range statusIDs {
-			var emojiIDs []string
+			status := new(gtsmodel.Status)
 
-			// Select statuses with ID.
-			if _, err := tx.NewSelect().
-				Table("statuses").
+			// Select status emoji IDs.
+			if err := tx.NewSelect().
+				Model(status).
 				Column("emojis").
 				Where("? = ?", bun.Ident("id"), statusID).
-				Exec(ctx); err != nil &&
+				Scan(ctx); err != nil &&
 				err != sql.ErrNoRows {
 				return err
 			}
 
-			// Delete all instances of this emoji ID from status emojis.
-			emojiIDs = slices.DeleteFunc(emojiIDs, func(emojiID string) bool {
+			// Delete all instances of this
+			// emoji ID from status emoji IDs.
+			status.EmojiIDs = slices.DeleteFunc(
+				status.EmojiIDs,
+				func(emojiID string) bool {
 					return emojiID == id
-			})
+				},
+			)
 
 			// Update status emoji IDs.
 			if _, err := tx.NewUpdate().
-				Table("statuses").
+				Model(status).
 				Where("? = ?", bun.Ident("id"), statusID).
-				Set("emojis = ?", emojiIDs).
+				Column("emojis").
 				Exec(ctx); err != nil &&
 				err != sql.ErrNoRows {
 				return err
@@ -161,35 +165,39 @@ func (e *emojiDB) DeleteEmojiByID(ctx context.Context, id string) error {
 		}
 
 		for _, accountID := range accountIDs {
-			var emojiIDs []string
+			account := new(gtsmodel.Account)
 
-			// Select account with ID.
-			if _, err := tx.NewSelect().
-				Table("accounts").
+			// Select account emoji IDs.
+			if err := tx.NewSelect().
+				Model(account).
 				Column("emojis").
 				Where("? = ?", bun.Ident("id"), accountID).
-				Exec(ctx); err != nil &&
+				Scan(ctx); err != nil &&
 				err != sql.ErrNoRows {
 				return err
 			}
 
-			// Delete all instances of this emoji ID from account emojis.
-			emojiIDs = slices.DeleteFunc(emojiIDs, func(emojiID string) bool {
+			// Delete all instances of this
+			// emoji ID from account emoji IDs.
+			account.EmojiIDs = slices.DeleteFunc(
+				account.EmojiIDs,
+				func(emojiID string) bool {
 					return emojiID == id
-			})
+				},
+			)
 
 			// Update account emoji IDs.
 			if _, err := tx.NewUpdate().
-				Table("accounts").
+				Model(account).
 				Where("? = ?", bun.Ident("id"), accountID).
-				Set("emojis = ?", emojiIDs).
+				Column("emojis").
 				Exec(ctx); err != nil &&
 				err != sql.ErrNoRows {
 				return err
 			}
 		}
 
-		// Delete emoji from database.
+		// Finally, delete emoji from database.
 		if _, err := tx.NewDelete().
 			Table("emojis").
 			Where("? = ?", bun.Ident("id"), id).

internal/db/bundb/emoji_test.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/stretchr/testify/suite"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
+	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/testrig"
 )
 
@@ -160,6 +161,16 @@ func (suite *EmojiTestSuite) TestGetEmojiCategory() {
 	suite.NotNil(category)
 }
 
+func (suite *EmojiTestSuite) TestUpdateEmojiCategory() {
+	testEmoji := new(gtsmodel.Emoji)
+	*testEmoji = *suite.testEmojis["rainbow"]
+
+	testEmoji.CategoryID = ""
+
+	err := suite.db.UpdateEmoji(context.Background(), testEmoji, "category_id")
+	suite.NoError(err)
+}
+
 func TestEmojiTestSuite(t *testing.T) {
 	suite.Run(t, new(EmojiTestSuite))
 }

internal/gtsmodel/emoji.go

@@ -44,3 +44,10 @@ type Emoji struct {
 	CategoryID             string         `bun:"type:CHAR(26),nullzero"`                                      // ID of the category this emoji belongs to.
 	Cached                 *bool          `bun:",nullzero,notnull,default:false"`
 }
+
+// IsLocal returns true if the emoji is
+// local to this instance., ie., it did
+// not originate from a remote instance.
+func (e *Emoji) IsLocal() bool {
+	return e.Domain == ""
+}

internal/media/processingemoji.go

@@ -158,7 +158,7 @@ func (p *ProcessingEmoji) store(ctx context.Context) error {
 
 	var maxSize bytesize.Size
 
-	if p.emoji.Domain == "" {
+	if p.emoji.IsLocal() {
 		// this is a local emoji upload
 		maxSize = config.GetMediaEmojiLocalMaxSize()
 	} else {

internal/media/refetch.go

@@ -61,7 +61,7 @@ func (m *Manager) RefetchEmojis(ctx context.Context, domain string, dereferenceM
 		}
 
 		for _, emoji := range emojis {
-			if emoji.Domain == "" {
+			if emoji.IsLocal() {
 				// never try to refetch local emojis
 				continue
 			}

internal/processing/admin/admin_test.go

@@ -62,6 +62,7 @@ type AdminStandardTestSuite struct {
 	testFollows      map[string]*gtsmodel.Follow
 	testAttachments  map[string]*gtsmodel.MediaAttachment
 	testStatuses     map[string]*gtsmodel.Status
+	testEmojis       map[string]*gtsmodel.Emoji
 
 	// module being tested
 	adminProcessor *admin.Processor
@@ -76,6 +77,7 @@ func (suite *AdminStandardTestSuite) SetupSuite() {
 	suite.testFollows = testrig.NewTestFollows()
 	suite.testAttachments = testrig.NewTestAttachments()
 	suite.testStatuses = testrig.NewTestStatuses()
+	suite.testEmojis = testrig.NewTestEmojis()
 }
 
 func (suite *AdminStandardTestSuite) SetupTest() {

internal/processing/admin/emoji.go

@@ -36,37 +36,39 @@ import (
 )
 
 // EmojiCreate creates a custom emoji on this instance.
-func (p *Processor) EmojiCreate(ctx context.Context, account *gtsmodel.Account, user *gtsmodel.User, form *apimodel.EmojiCreateRequest) (*apimodel.Emoji, gtserror.WithCode) {
-	if !*user.Admin {
-		return nil, gtserror.NewErrorUnauthorized(fmt.Errorf("user %s not an admin", user.ID), "user is not an admin")
-	}
-
+func (p *Processor) EmojiCreate(
+	ctx context.Context,
+	account *gtsmodel.Account,
+	form *apimodel.EmojiCreateRequest,
+) (*apimodel.Emoji, gtserror.WithCode) {
+	// Ensure emoji with this shortcode
+	// doesn't already exist on the instance.
 	maybeExisting, err := p.state.DB.GetEmojiByShortcodeDomain(ctx, form.Shortcode, "")
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("error checking existence of emoji with shortcode %s: %w", form.Shortcode, err)
+		return nil, gtserror.NewErrorInternalError(err)
+	}
+
 	if maybeExisting != nil {
-		return nil, gtserror.NewErrorConflict(fmt.Errorf("emoji with shortcode %s already exists", form.Shortcode), fmt.Sprintf("emoji with shortcode %s already exists", form.Shortcode))
+		err := fmt.Errorf("emoji with shortcode %s already exists", form.Shortcode)
+		return nil, gtserror.NewErrorConflict(err, err.Error())
 	}
 
-	if err != nil && err != db.ErrNoEntries {
-		return nil, gtserror.NewErrorInternalError(fmt.Errorf("error checking existence of emoji with shortcode %s: %s", form.Shortcode, err))
-	}
-
-	emojiID, err := id.NewRandomULID()
-	if err != nil {
-		return nil, gtserror.NewErrorInternalError(fmt.Errorf("error creating id for new emoji: %s", err), "error creating emoji ID")
-	}
-
-	emojiURI := uris.URIForEmoji(emojiID)
-
+	// Prepare data function for emoji processing
+	// (just read data from the submitted form).
 	data := func(innerCtx context.Context) (io.ReadCloser, int64, error) {
 		f, err := form.Image.Open()
 		return f, form.Image.Size, err
 	}
 
+	// If category was supplied on the form,
+	// ensure the category exists and provide
+	// it as additional info to emoji processing.
 	var ai *media.AdditionalEmojiInfo
 	if form.CategoryName != "" {
 		category, err := p.getOrCreateEmojiCategory(ctx, form.CategoryName)
 		if err != nil {
-			return nil, gtserror.NewErrorInternalError(fmt.Errorf("error putting id in category: %s", err), "error putting id in category")
+			return nil, gtserror.NewErrorInternalError(err)
 		}
 
 		ai = &media.AdditionalEmojiInfo{
@@ -74,73 +76,68 @@ func (p *Processor) EmojiCreate(ctx context.Context, account *gtsmodel.Account,
 		}
 	}
 
-	processingEmoji, err := p.mediaManager.PreProcessEmoji(ctx, data, form.Shortcode, emojiID, emojiURI, ai, false)
+	// Generate new emoji ID and URI.
+	emojiID, err := id.NewRandomULID()
 	if err != nil {
-		return nil, gtserror.NewErrorInternalError(fmt.Errorf("error processing emoji: %s", err), "error processing emoji")
+		err := gtserror.Newf("error creating id for new emoji: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
+	emojiURI := uris.URIForEmoji(emojiID)
+
+	// Begin media processing.
+	processingEmoji, err := p.mediaManager.PreProcessEmoji(ctx,
+		data, form.Shortcode, emojiID, emojiURI, ai, false,
+	)
+	if err != nil {
+		err := gtserror.Newf("error processing emoji: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
+	}
+
+	// Complete processing immediately.
 	emoji, err := processingEmoji.LoadEmoji(ctx)
 	if err != nil {
-		return nil, gtserror.NewErrorInternalError(fmt.Errorf("error loading emoji: %s", err), "error loading emoji")
+		err := gtserror.Newf("error loading emoji: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	apiEmoji, err := p.converter.EmojiToAPIEmoji(ctx, emoji)
 	if err != nil {
-		return nil, gtserror.NewErrorInternalError(fmt.Errorf("error converting emoji: %s", err), "error converting emoji to api representation")
+		err := gtserror.Newf("error converting emoji: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	return &apiEmoji, nil
 }
 
-// EmojisGet returns an admin view of custom emojis, filtered with the given parameters.
-func (p *Processor) EmojisGet(
-	ctx context.Context,
-	account *gtsmodel.Account,
-	user *gtsmodel.User,
+// emojisGetFilterParams builds extra
+// query parameters to return as part
+// of an Emojis pageable response.
+//
+// The returned string will look like:
+//
+// "filter=domain:all,enabled,shortcode:example"
+func emojisGetFilterParams(
+	shortcode string,
 	domain string,
 	includeDisabled bool,
 	includeEnabled bool,
-	shortcode string,
-	maxShortcodeDomain string,
-	minShortcodeDomain string,
-	limit int,
-) (*apimodel.PageableResponse, gtserror.WithCode) {
-	if !*user.Admin {
-		return nil, gtserror.NewErrorUnauthorized(fmt.Errorf("user %s not an admin", user.ID), "user is not an admin")
-	}
-
-	emojis, err := p.state.DB.GetEmojisBy(ctx, domain, includeDisabled, includeEnabled, shortcode, maxShortcodeDomain, minShortcodeDomain, limit)
-	if err != nil && !errors.Is(err, db.ErrNoEntries) {
-		err := fmt.Errorf("EmojisGet: db error: %s", err)
-		return nil, gtserror.NewErrorInternalError(err)
-	}
-
-	count := len(emojis)
-	if count == 0 {
-		return util.EmptyPageableResponse(), nil
-	}
-
-	items := make([]interface{}, 0, count)
-	for _, emoji := range emojis {
-		adminEmoji, err := p.converter.EmojiToAdminAPIEmoji(ctx, emoji)
-		if err != nil {
-			err := fmt.Errorf("EmojisGet: error converting emoji to admin model emoji: %s", err)
-			return nil, gtserror.NewErrorInternalError(err)
-		}
-		items = append(items, adminEmoji)
-	}
-
-	filterBuilder := strings.Builder{}
+) string {
+	var filterBuilder strings.Builder
 	filterBuilder.WriteString("filter=")
 
 	switch domain {
 	case "", "local":
+		// Local emojis only.
 		filterBuilder.WriteString("domain:local")
+
 	case db.EmojiAllDomains:
+		// Local or remote.
 		filterBuilder.WriteString("domain:all")
+
 	default:
-		filterBuilder.WriteString("domain:")
-		filterBuilder.WriteString(domain)
+		// Specific domain only.
+		filterBuilder.WriteString("domain:" + domain)
 	}
 
 	if includeDisabled != includeEnabled {
@@ -153,8 +150,53 @@ func (p *Processor) EmojisGet(
 	}
 
 	if shortcode != "" {
-		filterBuilder.WriteString(",shortcode:")
-		filterBuilder.WriteString(shortcode)
+		// Specific shortcode only.
+		filterBuilder.WriteString(",shortcode:" + shortcode)
+	}
+
+	return filterBuilder.String()
+}
+
+// EmojisGet returns an admin view of custom
+// emojis, filtered with the given parameters.
+func (p *Processor) EmojisGet(
+	ctx context.Context,
+	account *gtsmodel.Account,
+	domain string,
+	includeDisabled bool,
+	includeEnabled bool,
+	shortcode string,
+	maxShortcodeDomain string,
+	minShortcodeDomain string,
+	limit int,
+) (*apimodel.PageableResponse, gtserror.WithCode) {
+	emojis, err := p.state.DB.GetEmojisBy(ctx,
+		domain,
+		includeDisabled,
+		includeEnabled,
+		shortcode,
+		maxShortcodeDomain,
+		minShortcodeDomain,
+		limit,
+	)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
+	}
+
+	count := len(emojis)
+	if count == 0 {
+		return util.EmptyPageableResponse(), nil
+	}
+
+	items := make([]interface{}, 0, count)
+	for _, emoji := range emojis {
+		adminEmoji, err := p.converter.EmojiToAdminAPIEmoji(ctx, emoji)
+		if err != nil {
+			err := gtserror.Newf("error converting emoji to admin model emoji: %w", err)
+			return nil, gtserror.NewErrorInternalError(err)
+		}
+		items = append(items, adminEmoji)
 	}
 
 	return util.PackagePageableResponse(util.PageableResponseParams{
@@ -165,96 +207,125 @@ func (p *Processor) EmojisGet(
 		PrevMinIDKey:   "min_shortcode_domain",
 		PrevMinIDValue: util.ShortcodeDomain(emojis[0]),
 		Limit:          limit,
-		ExtraQueryParams: []string{filterBuilder.String()},
+		ExtraQueryParams: []string{
+			emojisGetFilterParams(
+				shortcode,
+				domain,
+				includeDisabled,
+				includeEnabled,
+			),
+		},
 	})
 }
 
-// EmojiGet returns the admin view of one custom emoji with the given id.
-func (p *Processor) EmojiGet(ctx context.Context, account *gtsmodel.Account, user *gtsmodel.User, id string) (*apimodel.AdminEmoji, gtserror.WithCode) {
-	if !*user.Admin {
-		return nil, gtserror.NewErrorUnauthorized(fmt.Errorf("user %s not an admin", user.ID), "user is not an admin")
+// EmojiGet returns the admin view of
+// one custom emoji with the given id.
+func (p *Processor) EmojiGet(
+	ctx context.Context,
+	account *gtsmodel.Account,
+	id string,
+) (*apimodel.AdminEmoji, gtserror.WithCode) {
+	emoji, err := p.state.DB.GetEmojiByID(ctx, id)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error: %w", err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	emoji, err := p.state.DB.GetEmojiByID(ctx, id)
-	if err != nil {
-		if errors.Is(err, db.ErrNoEntries) {
-			err = fmt.Errorf("EmojiGet: no emoji with id %s found in the db", id)
+	if emoji == nil {
+		err := gtserror.Newf("no emoji with id %s found in the db", id)
 		return nil, gtserror.NewErrorNotFound(err)
 	}
-		err := fmt.Errorf("EmojiGet: db error: %s", err)
-		return nil, gtserror.NewErrorInternalError(err)
-	}
 
 	adminEmoji, err := p.converter.EmojiToAdminAPIEmoji(ctx, emoji)
 	if err != nil {
-		err = fmt.Errorf("EmojiGet: error converting emoji to admin api emoji: %s", err)
+		err := gtserror.Newf("error converting emoji to admin api emoji: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	return adminEmoji, nil
 }
 
-// EmojiDelete deletes one emoji from the database, with the given id.
-func (p *Processor) EmojiDelete(ctx context.Context, id string) (*apimodel.AdminEmoji, gtserror.WithCode) {
+// EmojiDelete deletes one *local* emoji
+// from the database, with the given id.
+func (p *Processor) EmojiDelete(
+	ctx context.Context,
+	id string,
+) (*apimodel.AdminEmoji, gtserror.WithCode) {
 	emoji, err := p.state.DB.GetEmojiByID(ctx, id)
-	if err != nil {
-		if errors.Is(err, db.ErrNoEntries) {
-			err = fmt.Errorf("EmojiDelete: no emoji with id %s found in the db", id)
-			return nil, gtserror.NewErrorNotFound(err)
-		}
-		err := fmt.Errorf("EmojiDelete: db error: %s", err)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	if emoji.Domain != "" {
-		err = fmt.Errorf("EmojiDelete: emoji with id %s was not a local emoji, will not delete", id)
+	if emoji == nil {
+		err := gtserror.Newf("no emoji with id %s found in the db", id)
+		return nil, gtserror.NewErrorNotFound(err)
+	}
+
+	if !emoji.IsLocal() {
+		err := fmt.Errorf("emoji with id %s was not a local emoji, will not delete", id)
 		return nil, gtserror.NewErrorBadRequest(err, err.Error())
 	}
 
+	// Convert to admin emoji before deletion,
+	// so we can return the deleted emoji.
 	adminEmoji, err := p.converter.EmojiToAdminAPIEmoji(ctx, emoji)
 	if err != nil {
-		err = fmt.Errorf("EmojiDelete: error converting emoji to admin api emoji: %s", err)
+		err := gtserror.Newf("error converting emoji to admin api emoji: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	if err := p.state.DB.DeleteEmojiByID(ctx, id); err != nil {
-		err := fmt.Errorf("EmojiDelete: db error: %s", err)
+		err := gtserror.Newf("db error deleting emoji %s: %w", id, err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	return adminEmoji, nil
 }
 
-// EmojiUpdate updates one emoji with the given id, using the provided form parameters.
-func (p *Processor) EmojiUpdate(ctx context.Context, id string, form *apimodel.EmojiUpdateRequest) (*apimodel.AdminEmoji, gtserror.WithCode) {
+// EmojiUpdate updates one emoji with the
+// given id, using the provided form parameters.
+func (p *Processor) EmojiUpdate(
+	ctx context.Context,
+	id string,
+	form *apimodel.EmojiUpdateRequest,
+) (*apimodel.AdminEmoji, gtserror.WithCode) {
 	emoji, err := p.state.DB.GetEmojiByID(ctx, id)
-	if err != nil {
-		if errors.Is(err, db.ErrNoEntries) {
-			err = fmt.Errorf("EmojiUpdate: no emoji with id %s found in the db", id)
-			return nil, gtserror.NewErrorNotFound(err)
-		}
-		err := fmt.Errorf("EmojiUpdate: db error: %s", err)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	switch form.Type {
+	if emoji == nil {
+		err := gtserror.Newf("no emoji with id %s found in the db", id)
+		return nil, gtserror.NewErrorNotFound(err)
+	}
+
+	switch t := form.Type; t {
+
 	case apimodel.EmojiUpdateCopy:
 		return p.emojiUpdateCopy(ctx, emoji, form.Shortcode, form.CategoryName)
+
 	case apimodel.EmojiUpdateDisable:
 		return p.emojiUpdateDisable(ctx, emoji)
+
 	case apimodel.EmojiUpdateModify:
 		return p.emojiUpdateModify(ctx, emoji, form.Image, form.CategoryName)
+
 	default:
-		err := errors.New("unrecognized emoji action type")
+		err := fmt.Errorf("unrecognized emoji action type %s", t)
 		return nil, gtserror.NewErrorBadRequest(err, err.Error())
 	}
 }
 
-// EmojiCategoriesGet returns all custom emoji categories that exist on this instance.
-func (p *Processor) EmojiCategoriesGet(ctx context.Context) ([]*apimodel.EmojiCategory, gtserror.WithCode) {
+// EmojiCategoriesGet returns all custom emoji
+// categories that exist on this instance.
+func (p *Processor) EmojiCategoriesGet(
+	ctx context.Context,
+) ([]*apimodel.EmojiCategory, gtserror.WithCode) {
 	categories, err := p.state.DB.GetEmojiCategories(ctx)
 	if err != nil {
-		err := fmt.Errorf("EmojiCategoriesGet: db error: %s", err)
+		err := gtserror.Newf("db error getting emoji categories: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
@@ -262,7 +333,7 @@ func (p *Processor) EmojiCategoriesGet(ctx context.Context) ([]*apimodel.EmojiCa
 	for _, category := range categories {
 		apiCategory, err := p.converter.EmojiCategoryToAPIEmojiCategory(ctx, category)
 		if err != nil {
-			err := fmt.Errorf("EmojiCategoriesGet: error converting emoji category to api emoji category: %s", err)
+			err := gtserror.Newf("error converting emoji category to api emoji category: %w", err)
 			return nil, gtserror.NewErrorInternalError(err)
 		}
 		apiCategories = append(apiCategories, apiCategory)
@@ -275,22 +346,35 @@ func (p *Processor) EmojiCategoriesGet(ctx context.Context) ([]*apimodel.EmojiCa
 	UTIL FUNCTIONS
 */
 
-func (p *Processor) getOrCreateEmojiCategory(ctx context.Context, name string) (*gtsmodel.EmojiCategory, error) {
+// getOrCreateEmojiCategory either gets an existing
+// category with the given name from the database,
+// or, if the category doesn't yet exist, it creates
+// the category and then returns it.
+func (p *Processor) getOrCreateEmojiCategory(
+	ctx context.Context,
+	name string,
+) (*gtsmodel.EmojiCategory, error) {
 	category, err := p.state.DB.GetEmojiCategoryByName(ctx, name)
-	if err == nil {
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		return nil, gtserror.Newf(
+			"database error trying get emoji category %s: %w",
+			name, err,
+		)
+	}
+
+	if category != nil {
+		// We had it already.
 		return category, nil
 	}
 
-	if err != nil && !errors.Is(err, db.ErrNoEntries) {
-		err = fmt.Errorf("GetOrCreateEmojiCategory: database error trying get emoji category by name: %s", err)
-		return nil, err
-	}
-
-	// we don't have the category yet, just create it with the given name
+	// We don't have the category yet,
+	// create it with the given name.
 	categoryID, err := id.NewRandomULID()
 	if err != nil {
-		err = fmt.Errorf("GetOrCreateEmojiCategory: error generating id for new emoji category: %s", err)
-		return nil, err
+		return nil, gtserror.Newf(
+			"error generating id for new emoji category %s: %w",
+			name, err,
+		)
 	}
 
 	category = &gtsmodel.EmojiCategory{
@@ -299,54 +383,85 @@ func (p *Processor) getOrCreateEmojiCategory(ctx context.Context, name string) (
 	}
 
 	if err := p.state.DB.PutEmojiCategory(ctx, category); err != nil {
-		err = fmt.Errorf("GetOrCreateEmojiCategory: error putting new emoji category in the database: %s", err)
-		return nil, err
+		return nil, gtserror.Newf(
+			"db error putting new emoji category %s: %w",
+			name, err,
+		)
 	}
 
 	return category, nil
 }
 
-// copy an emoji from remote to local
-func (p *Processor) emojiUpdateCopy(ctx context.Context, emoji *gtsmodel.Emoji, shortcode *string, categoryName *string) (*apimodel.AdminEmoji, gtserror.WithCode) {
-	if emoji.Domain == "" {
-		err := fmt.Errorf("emojiUpdateCopy: emoji %s is not a remote emoji, cannot copy it to local", emoji.ID)
+// emojiUpdateCopy copies and stores the given
+// *remote* emoji as a *local* emoji, preserving
+// the same image, and using the provided shortcode.
+//
+// The provided emoji model must correspond to an
+// emoji already stored in the database + storage.
+func (p *Processor) emojiUpdateCopy(
+	ctx context.Context,
+	targetEmoji *gtsmodel.Emoji,
+	shortcode *string,
+	category *string,
+) (*apimodel.AdminEmoji, gtserror.WithCode) {
+	if targetEmoji.IsLocal() {
+		err := fmt.Errorf("emoji %s is not a remote emoji, cannot copy it to local", targetEmoji.ID)
 		return nil, gtserror.NewErrorBadRequest(err, err.Error())
 	}
 
 	if shortcode == nil {
-		err := fmt.Errorf("emojiUpdateCopy: emoji %s could not be copied, no shortcode provided", emoji.ID)
+		err := errors.New("no shortcode provided")
 		return nil, gtserror.NewErrorBadRequest(err, err.Error())
 	}
 
-	maybeExisting, err := p.state.DB.GetEmojiByShortcodeDomain(ctx, *shortcode, "")
+	sc := *shortcode
+	if sc == "" {
+		err := errors.New("empty shortcode provided")
+		return nil, gtserror.NewErrorBadRequest(err, err.Error())
+	}
+
+	// Ensure we don't already have an emoji
+	// stored locally with this shortcode.
+	maybeExisting, err := p.state.DB.GetEmojiByShortcodeDomain(ctx, sc, "")
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error checking for emoji with shortcode %s: %w", sc, err)
+		return nil, gtserror.NewErrorInternalError(err)
+	}
+
 	if maybeExisting != nil {
-		err := fmt.Errorf("emojiUpdateCopy: emoji %s could not be copied, emoji with shortcode %s already exists on this instance", emoji.ID, *shortcode)
+		err := fmt.Errorf("emoji with shortcode %s already exists on this instance", sc)
 		return nil, gtserror.NewErrorConflict(err, err.Error())
 	}
 
-	if err != nil && err != db.ErrNoEntries {
-		err := fmt.Errorf("emojiUpdateCopy: emoji %s could not be copied, error checking existence of emoji with shortcode %s: %s", emoji.ID, *shortcode, err)
-		return nil, gtserror.NewErrorInternalError(err)
+	// We don't have an emoji with this
+	// shortcode yet! Prepare to create it.
+
+	// Data function for copying just streams media
+	// out of storage into an additional location.
+	//
+	// This means that data for the copy persists even
+	// if the remote copied emoji gets deleted at some point.
+	data := func(ctx context.Context) (io.ReadCloser, int64, error) {
+		rc, err := p.state.Storage.GetStream(ctx, targetEmoji.ImagePath)
+		return rc, int64(targetEmoji.ImageFileSize), err
 	}
 
-	newEmojiID, err := id.NewRandomULID()
+	// Generate new emoji ID and URI.
+	emojiID, err := id.NewRandomULID()
 	if err != nil {
-		err := fmt.Errorf("emojiUpdateCopy: emoji %s could not be copied, error creating id for new emoji: %s", emoji.ID, err)
+		err := gtserror.Newf("error creating id for new emoji: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	newEmojiURI := uris.URIForEmoji(newEmojiID)
-
-	data := func(ctx context.Context) (reader io.ReadCloser, fileSize int64, err error) {
-		rc, err := p.state.Storage.GetStream(ctx, emoji.ImagePath)
-		return rc, int64(emoji.ImageFileSize), err
-	}
+	emojiURI := uris.URIForEmoji(emojiID)
 
+	// If category was supplied, ensure the
+	// category exists and provide it as
+	// additional info to emoji processing.
 	var ai *media.AdditionalEmojiInfo
-	if categoryName != nil {
-		category, err := p.getOrCreateEmojiCategory(ctx, *categoryName)
+	if category != nil && *category != "" {
+		category, err := p.getOrCreateEmojiCategory(ctx, *category)
 		if err != nil {
-			err = fmt.Errorf("emojiUpdateCopy: error getting or creating category: %s", err)
 			return nil, gtserror.NewErrorInternalError(err)
 		}
 
@@ -355,126 +470,173 @@ func (p *Processor) emojiUpdateCopy(ctx context.Context, emoji *gtsmodel.Emoji,
 		}
 	}
 
-	processingEmoji, err := p.mediaManager.PreProcessEmoji(ctx, data, *shortcode, newEmojiID, newEmojiURI, ai, false)
+	// Begin media processing.
+	processingEmoji, err := p.mediaManager.PreProcessEmoji(ctx,
+		data, sc, emojiID, emojiURI, ai, false,
+	)
 	if err != nil {
-		err = fmt.Errorf("emojiUpdateCopy: error processing emoji %s: %s", emoji.ID, err)
+		err := gtserror.Newf("error processing emoji: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
+	// Complete processing immediately.
 	newEmoji, err := processingEmoji.LoadEmoji(ctx)
 	if err != nil {
-		err = fmt.Errorf("emojiUpdateCopy: error loading processed emoji %s: %s", emoji.ID, err)
+		err := gtserror.Newf("error loading emoji: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	adminEmoji, err := p.converter.EmojiToAdminAPIEmoji(ctx, newEmoji)
 	if err != nil {
-		err = fmt.Errorf("emojiUpdateCopy: error converting updated emoji %s to admin emoji: %s", emoji.ID, err)
+		err := gtserror.Newf("error converting emoji %s to admin emoji: %w", newEmoji.ID, err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	return adminEmoji, nil
 }
 
-// disable a remote emoji
-func (p *Processor) emojiUpdateDisable(ctx context.Context, emoji *gtsmodel.Emoji) (*apimodel.AdminEmoji, gtserror.WithCode) {
-	if emoji.Domain == "" {
-		err := fmt.Errorf("emojiUpdateDisable: emoji %s is not a remote emoji, cannot disable it via this endpoint", emoji.ID)
+// emojiUpdateDisable marks the given *remote*
+// emoji as disabled by setting disabled = true.
+//
+// The provided emoji model must correspond to an
+// emoji already stored in the database + storage.
+func (p *Processor) emojiUpdateDisable(
+	ctx context.Context,
+	emoji *gtsmodel.Emoji,
+) (*apimodel.AdminEmoji, gtserror.WithCode) {
+	if emoji.IsLocal() {
+		err := fmt.Errorf("emoji %s is not a remote emoji, cannot disable it via this endpoint", emoji.ID)
 		return nil, gtserror.NewErrorBadRequest(err, err.Error())
 	}
 
-	emojiDisabled := true
-	emoji.Disabled = &emojiDisabled
-	err := p.state.DB.UpdateEmoji(ctx, emoji, "disabled")
-	if err != nil {
-		err = fmt.Errorf("emojiUpdateDisable: error updating emoji %s: %s", emoji.ID, err)
+	// Only bother with a db call
+	// if emoji not already disabled.
+	if !*emoji.Disabled {
+		emoji.Disabled = util.Ptr(true)
+		if err := p.state.DB.UpdateEmoji(ctx, emoji, "disabled"); err != nil {
+			err := gtserror.Newf("db error updating emoji %s: %w", emoji.ID, err)
 			return nil, gtserror.NewErrorInternalError(err)
 		}
+	}
 
 	adminEmoji, err := p.converter.EmojiToAdminAPIEmoji(ctx, emoji)
 	if err != nil {
-		err = fmt.Errorf("emojiUpdateDisable: error converting updated emoji %s to admin emoji: %s", emoji.ID, err)
+		err := gtserror.Newf("error converting emoji %s to admin emoji: %w", emoji.ID, err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	return adminEmoji, nil
 }
 
-// modify a local emoji
-func (p *Processor) emojiUpdateModify(ctx context.Context, emoji *gtsmodel.Emoji, image *multipart.FileHeader, categoryName *string) (*apimodel.AdminEmoji, gtserror.WithCode) {
-	if emoji.Domain != "" {
-		err := fmt.Errorf("emojiUpdateModify: emoji %s is not a local emoji, cannot do a modify action on it", emoji.ID)
+// emojiUpdateModify modifies the given *local* emoji.
+//
+// Either one of image or category must be non-nil,
+// otherwise there's nothing to modify. If category
+// is non-nil and dereferences to an empty string,
+// category will be cleared.
+//
+// The provided emoji model must correspond to an
+// emoji already stored in the database + storage.
+func (p *Processor) emojiUpdateModify(
+	ctx context.Context,
+	emoji *gtsmodel.Emoji,
+	image *multipart.FileHeader,
+	category *string,
+) (*apimodel.AdminEmoji, gtserror.WithCode) {
+	if !emoji.IsLocal() {
+		err := fmt.Errorf("emoji %s is not a local emoji, cannot update it via this endpoint", emoji.ID)
 		return nil, gtserror.NewErrorBadRequest(err, err.Error())
 	}
 
-	// keep existing categoryID unless a new one is defined
+	// Ensure there's actually something to update.
+	if image == nil && category == nil {
+		err := errors.New("neither new category nor new image set, cannot update")
+		return nil, gtserror.NewErrorBadRequest(err, err.Error())
+	}
+
+	// Only update category
+	// if it's changed.
 	var (
-		updatedCategoryID = emoji.CategoryID
+		newCategory      *gtsmodel.EmojiCategory
+		newCategoryID    string
 		updateCategoryID bool
 	)
-	if categoryName != nil {
-		category, err := p.getOrCreateEmojiCategory(ctx, *categoryName)
+
+	if category != nil {
+		catName := *category
+		if catName != "" {
+			// Set new category.
+			var err error
+			newCategory, err = p.getOrCreateEmojiCategory(ctx, catName)
 			if err != nil {
-			err = fmt.Errorf("emojiUpdateModify: error getting or creating category: %s", err)
+				err := gtserror.Newf("error getting or creating category: %w", err)
 				return nil, gtserror.NewErrorInternalError(err)
 			}
 
-		updatedCategoryID = category.ID
-		updateCategoryID = true
+			newCategoryID = newCategory.ID
+		} else {
+			// Clear existing category.
+			newCategoryID = ""
 		}
 
-	// only update image if provided with one
+		updateCategoryID = emoji.CategoryID != newCategoryID
+	}
+
+	// Only update image
+	// if one is provided.
 	var updateImage bool
 	if image != nil && image.Size != 0 {
 		updateImage = true
 	}
 
-	if !updateImage {
-		// only updating fields, we only need
-		// to do a database update for this
-		var columns []string
-
-		if updateCategoryID {
-			emoji.CategoryID = updatedCategoryID
-			columns = append(columns, "category_id")
-		}
-
-		var err error
-		err = p.state.DB.UpdateEmoji(ctx, emoji, columns...)
-		if err != nil {
-			err = fmt.Errorf("emojiUpdateModify: error updating emoji %s: %s", emoji.ID, err)
+	if updateCategoryID && !updateImage {
+		// Only updating category; we only
+		// need to do a db update for this.
+		emoji.CategoryID = newCategoryID
+		emoji.Category = newCategory
+		if err := p.state.DB.UpdateEmoji(ctx, emoji, "category_id"); err != nil {
+			err := gtserror.Newf("db error updating emoji %s: %w", emoji.ID, err)
 			return nil, gtserror.NewErrorInternalError(err)
 		}
-	} else {
-		// new image, so we need to reprocess the emoji
-		data := func(ctx context.Context) (reader io.ReadCloser, fileSize int64, err error) {
+	} else if updateImage {
+		// Updating image and maybe categoryID.
+		// We can do both at the same time :)
+
+		// Set data function to provided image.
+		data := func(ctx context.Context) (io.ReadCloser, int64, error) {
 			i, err := image.Open()
 			return i, image.Size, err
 		}
 
+		// If necessary, include
+		// update to categoryID too.
 		var ai *media.AdditionalEmojiInfo
 		if updateCategoryID {
 			ai = &media.AdditionalEmojiInfo{
-				CategoryID: &updatedCategoryID,
+				CategoryID: &newCategoryID,
 			}
 		}
 
-		processingEmoji, err := p.mediaManager.PreProcessEmoji(ctx, data, emoji.Shortcode, emoji.ID, emoji.URI, ai, true)
+		// Begin media processing.
+		processingEmoji, err := p.mediaManager.PreProcessEmoji(ctx,
+			data, emoji.Shortcode, emoji.ID, emoji.URI, ai, false,
+		)
 		if err != nil {
-			err = fmt.Errorf("emojiUpdateModify: error processing emoji %s: %s", emoji.ID, err)
+			err := gtserror.Newf("error processing emoji: %w", err)
 			return nil, gtserror.NewErrorInternalError(err)
 		}
 
+		// Replace emoji ptr with newly-processed version.
 		emoji, err = processingEmoji.LoadEmoji(ctx)
 		if err != nil {
-			err = fmt.Errorf("emojiUpdateModify: error loading processed emoji %s: %s", emoji.ID, err)
+			err := gtserror.Newf("error loading emoji: %w", err)
 			return nil, gtserror.NewErrorInternalError(err)
 		}
 	}
 
 	adminEmoji, err := p.converter.EmojiToAdminAPIEmoji(ctx, emoji)
 	if err != nil {
-		err = fmt.Errorf("emojiUpdateModify: error converting updated emoji %s to admin emoji: %s", emoji.ID, err)
+		err := gtserror.Newf("error converting emoji %s to admin emoji: %w", emoji.ID, err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 

internal/processing/admin/emoji_test.go

@@ -0,0 +1,67 @@
+// GoToSocial
+// Copyright (C) GoToSocial Authors admin@gotosocial.org
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package admin_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
+	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/util"
+)
+
+type EmojiTestSuite struct {
+	AdminStandardTestSuite
+}
+
+func (suite *EmojiTestSuite) TestUpdateEmojiCategory() {
+	ctx := context.Background()
+	testEmoji := new(gtsmodel.Emoji)
+	*testEmoji = *suite.testEmojis["rainbow"]
+
+	// Toggle the emoji category around.
+	for _, categoryName := range []string{
+		"",
+		"newCategory",
+		"newCategory",
+		"newCategory2",
+		"",
+		"reactions",
+		"",
+		"",
+	} {
+		emoji, err := suite.adminProcessor.EmojiUpdate(ctx,
+			testEmoji.ID,
+			&apimodel.EmojiUpdateRequest{
+				Type:         apimodel.EmojiUpdateModify,
+				CategoryName: util.Ptr(categoryName),
+			},
+		)
+		if err != nil {
+			suite.FailNow(err.Error())
+		}
+
+		suite.Equal(categoryName, emoji.Category)
+	}
+}
+
+func TestEmojiTestSuite(t *testing.T) {
+	suite.Run(t, new(EmojiTestSuite))
+}

internal/processing/fedi/emoji.go

@@ -36,7 +36,7 @@ func (p *Processor) EmojiGet(ctx context.Context, requestedEmojiID string) (inte
 		return nil, gtserror.NewErrorNotFound(fmt.Errorf("database error getting emoji with id %s: %s", requestedEmojiID, err))
 	}
 
-	if requestedEmoji.Domain != "" {
+	if !requestedEmoji.IsLocal() {
 		return nil, gtserror.NewErrorNotFound(fmt.Errorf("emoji with id %s doesn't belong to this instance (domain %s)", requestedEmojiID, requestedEmoji.Domain))
 	}
 

internal/typeutils/internaltofrontend.go

@@ -612,7 +612,7 @@ func (c *Converter) EmojiToAdminAPIEmoji(ctx context.Context, e *gtsmodel.Emoji)
 		return nil, err
 	}
 
-	if e.Domain != "" {
+	if !e.IsLocal() {
 		// Domain may be in Punycode,
 		// de-punify it just in case.
 		var err error