diff --git a/internal/api/client/admin/emojiupdate_test.go b/internal/api/client/admin/emojiupdate_test.go index 5df43d7ae..17eb05fd9 100644 --- a/internal/api/client/admin/emojiupdate_test.go +++ b/internal/api/client/admin/emojiupdate_test.go @@ -21,9 +21,9 @@ "context" "encoding/json" "io" - "io/ioutil" "net/http" "net/http/httptest" + "strings" "testing" "github.com/stretchr/testify/suite" @@ -68,7 +68,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateNewCategory() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.NotEmpty(b) @@ -145,7 +145,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateSwitchCategory() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.NotEmpty(b) @@ -223,7 +223,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateCopyRemoteToLocal() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.NotEmpty(b) @@ -299,7 +299,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateDisableEmoji() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.NotEmpty(b) @@ -338,12 +338,97 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateDisableLocalEmoji() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.Equal(`{"error":"Bad Request: emoji 01F8MH9H8E4VG3KDYJR9EGPXCQ is not a remote emoji, cannot disable it via this endpoint"}`, string(b)) } +func (suite *EmojiUpdateTestSuite) TestEmojiUpdateModify() { + testEmoji := >smodel.Emoji{} + *testEmoji = *suite.testEmojis["rainbow"] + + // set up the request + requestBody, w, err := testrig.CreateMultipartFormData( + testrig.FileToDataF("image", "../../../../testrig/media/kip-original.gif"), + map[string][]string{ + "type": {"modify"}, + }) + if err != nil { + panic(err) + } + bodyBytes := requestBody.Bytes() + recorder := httptest.NewRecorder() + ctx := suite.newContext(recorder, http.MethodPost, bodyBytes, admin.EmojiPathWithID, w.FormDataContentType()) + ctx.AddParam(apiutil.IDKey, testEmoji.ID) + + // call the handler + suite.adminModule.EmojiPATCHHandler(ctx) + suite.Equal(http.StatusOK, recorder.Code) + + // 2. we should have no error message in the result body + result := recorder.Result() + defer result.Body.Close() + + // check the response + b, err := io.ReadAll(result.Body) + suite.NoError(err) + suite.NotEmpty(b) + + // response should be an admin model emoji + adminEmoji := &apimodel.AdminEmoji{} + err = json.Unmarshal(b, adminEmoji) + suite.NoError(err) + + // appropriate fields should be set + suite.Equal("rainbow", adminEmoji.Shortcode) + suite.NotEmpty(adminEmoji.URL) + suite.NotEmpty(adminEmoji.StaticURL) + suite.True(adminEmoji.VisibleInPicker) + + // emoji should be in the db + dbEmoji, err := suite.db.GetEmojiByShortcodeDomain(context.Background(), adminEmoji.Shortcode, "") + suite.NoError(err) + + // check fields on the emoji + suite.NotEmpty(dbEmoji.ID) + suite.Equal("rainbow", dbEmoji.Shortcode) + suite.Empty(dbEmoji.Domain) + suite.Empty(dbEmoji.ImageRemoteURL) + suite.Empty(dbEmoji.ImageStaticRemoteURL) + suite.Equal(adminEmoji.URL, dbEmoji.ImageURL) + suite.Equal(adminEmoji.StaticURL, dbEmoji.ImageStaticURL) + + // Ensure image path as expected. + suite.NotEmpty(dbEmoji.ImagePath) + if !strings.HasPrefix(dbEmoji.ImagePath, suite.testAccounts["instance_account"].ID+"/emoji/original") { + suite.FailNow("", "image path %s not valid", dbEmoji.ImagePath) + } + + // Ensure static image path as expected. + suite.NotEmpty(dbEmoji.ImageStaticPath) + if !strings.HasPrefix(dbEmoji.ImageStaticPath, suite.testAccounts["instance_account"].ID+"/emoji/static") { + suite.FailNow("", "image path %s not valid", dbEmoji.ImageStaticPath) + } + + suite.Equal("image/gif", dbEmoji.ImageContentType) + suite.Equal("image/png", dbEmoji.ImageStaticContentType) + suite.Equal(1428, dbEmoji.ImageFileSize) + suite.Equal(1056, dbEmoji.ImageStaticFileSize) + suite.False(*dbEmoji.Disabled) + suite.NotEmpty(dbEmoji.URI) + suite.True(*dbEmoji.VisibleInPicker) + suite.NotEmpty(dbEmoji.CategoryID) + + // emoji should be in storage + entry, err := suite.storage.Storage.Stat(ctx, dbEmoji.ImagePath) + suite.NoError(err) + suite.Equal(int64(dbEmoji.ImageFileSize), entry.Size) + entry, err = suite.storage.Storage.Stat(ctx, dbEmoji.ImageStaticPath) + suite.NoError(err) + suite.Equal(int64(dbEmoji.ImageStaticFileSize), entry.Size) +} + func (suite *EmojiUpdateTestSuite) TestEmojiUpdateModifyRemoteEmoji() { testEmoji := >smodel.Emoji{} *testEmoji = *suite.testEmojis["yell"] @@ -404,7 +489,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateModifyNoParams() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.Equal(`{"error":"Bad Request: emoji action type was 'modify' but no image or category name was provided"}`, string(b)) @@ -438,7 +523,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateCopyLocalToLocal() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.Equal(`{"error":"Bad Request: target emoji is not remote; cannot copy to local"}`, string(b)) @@ -472,7 +557,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateCopyEmptyShortcode() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.Equal(`{"error":"Bad Request: shortcode did not pass validation, must be between 2 and 30 characters, letters, numbers, and underscores only"}`, string(b)) @@ -505,7 +590,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateCopyNoShortcode() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.Equal(`{"error":"Bad Request: emoji action type was 'copy' but no shortcode was provided"}`, string(b)) @@ -539,7 +624,7 @@ func (suite *EmojiUpdateTestSuite) TestEmojiUpdateCopyShortcodeAlreadyInUse() { defer result.Body.Close() // check the response - b, err := ioutil.ReadAll(result.Body) + b, err := io.ReadAll(result.Body) suite.NoError(err) suite.Equal(`{"error":"Conflict: emoji with shortcode already exists"}`, string(b)) diff --git a/internal/cleaner/media_test.go b/internal/cleaner/media_test.go index 46c6edcd4..6e653c07c 100644 --- a/internal/cleaner/media_test.go +++ b/internal/cleaner/media_test.go @@ -386,7 +386,7 @@ func (suite *MediaTestSuite) TestUncacheAndRecache() { testStatusAttachment, testHeader, } { - processing := suite.manager.RecacheMedia(original, data) + processing := suite.manager.CacheMedia(original, data) // synchronously load the recached attachment recachedAttachment, err := processing.Load(ctx) diff --git a/internal/federation/dereferencing/emoji.go b/internal/federation/dereferencing/emoji.go index 22b5a0442..3174fa2f9 100644 --- a/internal/federation/dereferencing/emoji.go +++ b/internal/federation/dereferencing/emoji.go @@ -134,11 +134,6 @@ func (d *Dereferencer) RefreshEmoji( *gtsmodel.Emoji, error, ) { - // Can't refresh local. - if emoji.IsLocal() { - return emoji, nil - } - // Check emoji is up-to-date // with provided extra info. switch { @@ -156,8 +151,18 @@ func (d *Dereferencer) RefreshEmoji( force = true } - // Check if needs updating. - if *emoji.Cached && !force { + // Check if needs + // force refresh. + if !force { + + // We still want to make sure + // the emoji is cached. Simply + // check whether emoji is cached. + return d.RecacheEmoji(ctx, emoji) + } + + // Can't refresh local. + if emoji.IsLocal() { return emoji, nil } @@ -191,8 +196,8 @@ func() (*media.ProcessingEmoji, error) { return tsport.DereferenceMedia(ctx, url, int64(maxsz)) } - // Refresh emoji with prepared info. - return d.mediaManager.RefreshEmoji(ctx, + // Update emoji with prepared info. + return d.mediaManager.UpdateEmoji(ctx, emoji, data, info, @@ -201,6 +206,72 @@ func() (*media.ProcessingEmoji, error) { ) } +// RecacheEmoji handles the simplest case which is that +// of an existing emoji that only needs to be recached. +// It handles the case of both local emojis, and those +// already cached as no-ops. +// +// Please note that even if an error is returned, +// an emoji model may still be returned if the error +// was only encountered during actual dereferencing. +// In this case, it will act as a placeholder. +func (d *Dereferencer) RecacheEmoji( + ctx context.Context, + emoji *gtsmodel.Emoji, +) ( + *gtsmodel.Emoji, + error, +) { + // Can't recache local. + if emoji.IsLocal() { + return emoji, nil + } + + if *emoji.Cached { + // Already cached. + return emoji, nil + } + + // Get shortcode domain for locks + logging. + shortcodeDomain := emoji.ShortcodeDomain() + + // Ensure we have a valid image remote URL. + url, err := url.Parse(emoji.ImageRemoteURL) + if err != nil { + err := gtserror.Newf("invalid image remote url %s for emoji %s: %w", emoji.ImageRemoteURL, shortcodeDomain, err) + return nil, err + } + + // Pass along for safe processing. + return d.processEmojiSafely(ctx, + shortcodeDomain, + func() (*media.ProcessingEmoji, error) { + + // Acquire new instance account transport for emoji dereferencing. + tsport, err := d.transportController.NewTransportForUsername(ctx, "") + if err != nil { + err := gtserror.Newf("error getting instance transport: %w", err) + return nil, err + } + + // Get maximum supported remote emoji size. + maxsz := config.GetMediaEmojiRemoteMaxSize() + + // Prepare data function to dereference remote emoji media. + data := func(context.Context) (io.ReadCloser, error) { + return tsport.DereferenceMedia(ctx, url, int64(maxsz)) + } + + // Recache emoji with prepared info. + return d.mediaManager.CacheEmoji(ctx, + emoji, + data, + ) + }, + ) + +} + // processingEmojiSafely provides concurrency-safe processing of // an emoji with given shortcode+domain. if a copy of the emoji is // not already being processed, the given 'process' callback will diff --git a/internal/federation/dereferencing/media.go b/internal/federation/dereferencing/media.go index 48c0e258e..d4f583735 100644 --- a/internal/federation/dereferencing/media.go +++ b/internal/federation/dereferencing/media.go @@ -172,7 +172,7 @@ func() (*media.ProcessingMedia, error) { // Recache media with prepared info, // this will also update media in db. - return d.mediaManager.RecacheMedia( + return d.mediaManager.CacheMedia( attach, func(ctx context.Context) (io.ReadCloser, error) { return tsport.DereferenceMedia(ctx, url, int64(maxsz)) diff --git a/internal/media/manager.go b/internal/media/manager.go index 0099dfe07..22afd0028 100644 --- a/internal/media/manager.go +++ b/internal/media/manager.go @@ -20,6 +20,7 @@ import ( "context" "io" + "strings" "time" "codeberg.org/gruf/go-iotools" @@ -161,14 +162,14 @@ func (m *Manager) CreateMedia( } // Pass prepared media as ready to be cached. - return m.RecacheMedia(attachment, data), nil + return m.CacheMedia(attachment, data), nil } -// RecacheMedia wraps a media model (assumed already +// CacheMedia wraps a media model (assumed already // inserted in the database!) with given data function // to perform a blocking dereference / decode operation // from the data stream returned. -func (m *Manager) RecacheMedia( +func (m *Manager) CacheMedia( media *gtsmodel.MediaAttachment, data DataFunc, ) *ProcessingMedia { @@ -220,7 +221,7 @@ func (m *Manager) CreateEmoji( } // Finally, create new emoji. - return m.createEmoji(ctx, + return m.createOrUpdateEmoji(ctx, m.state.DB.PutEmoji, data, emoji, @@ -228,12 +229,14 @@ func (m *Manager) CreateEmoji( ) } -// RefreshEmoji will prepare a recache operation -// for the given emoji, updating it with extra -// information, and in particular using new storage -// paths for the dereferenced media files to skirt -// around browser caching of the old files. -func (m *Manager) RefreshEmoji( +// UpdateEmoji prepares an update operation for the given emoji, +// which is assumed to already exist in the database. +// +// Calling load on the returned *ProcessingEmoji will update the +// db entry with provided extra information, ensure emoji images +// are cached, and use new storage paths for the dereferenced media +// files to skirt around browser caching of the old files. +func (m *Manager) UpdateEmoji( ctx context.Context, emoji *gtsmodel.Emoji, data DataFunc, @@ -289,8 +292,8 @@ func (m *Manager) RefreshEmoji( return rct, nil } - // Finally, create new emoji in database. - processingEmoji, err := m.createEmoji(ctx, + // Update existing emoji in database. + processingEmoji, err := m.createOrUpdateEmoji(ctx, func(ctx context.Context, emoji *gtsmodel.Emoji) error { return m.state.DB.UpdateEmoji(ctx, emoji) }, @@ -308,9 +311,49 @@ func(ctx context.Context, emoji *gtsmodel.Emoji) error { return processingEmoji, nil } -func (m *Manager) createEmoji( +// CacheEmoji wraps an emoji model (assumed already +// inserted in the database!) with given data function +// to perform a blocking dereference / decode operation +// from the data stream returned. +func (m *Manager) CacheEmoji( ctx context.Context, - putDB func(context.Context, *gtsmodel.Emoji) error, + emoji *gtsmodel.Emoji, + data DataFunc, +) ( + *ProcessingEmoji, + error, +) { + // Fetch the local instance account for emoji path generation. + instanceAcc, err := m.state.DB.GetInstanceAccount(ctx, "") + if err != nil { + return nil, gtserror.Newf("error fetching instance account: %w", err) + } + + var pathID string + + // Look for an emoji path ID that differs from its actual ID, this indicates + // a previous 'refresh'. We need to be sure to set this on the ProcessingEmoji{} + // so it knows to store the emoji under this path, and not default to emoji.ID. + if id := extractEmojiPathID(emoji.ImagePath); id != emoji.ID { + pathID = id + } + + return &ProcessingEmoji{ + newPathID: pathID, + instAccID: instanceAcc.ID, + emoji: emoji, + dataFn: data, + mgr: m, + }, nil +} + +// createOrUpdateEmoji updates the emoji according to +// provided additional data, and performs the actual +// database write, finally returning an emoji ready +// for processing (i.e. caching to local storage). +func (m *Manager) createOrUpdateEmoji( + ctx context.Context, + storeDB func(context.Context, *gtsmodel.Emoji) error, data DataFunc, emoji *gtsmodel.Emoji, info AdditionalEmojiInfo, @@ -351,8 +394,8 @@ func (m *Manager) createEmoji( emoji.CategoryID = *info.CategoryID } - // Store emoji in database in initial form. - if err := putDB(ctx, emoji); err != nil { + // Put or update emoji in database. + if err := storeDB(ctx, emoji); err != nil { return nil, err } @@ -367,17 +410,26 @@ func (m *Manager) createEmoji( return processingEmoji, nil } -// RecacheEmoji wraps an emoji model (assumed already -// inserted in the database!) with given data function -// to perform a blocking dereference / decode operation -// from the data stream returned. -func (m *Manager) RecacheEmoji( - emoji *gtsmodel.Emoji, - data DataFunc, -) *ProcessingEmoji { - return &ProcessingEmoji{ - emoji: emoji, - dataFn: data, - mgr: m, +// extractEmojiPathID pulls the ID used in the final path segment of an emoji path (can be URL). +func extractEmojiPathID(path string) string { + // Look for '.' indicating file ext. + i := strings.LastIndexByte(path, '.') + if i == -1 { + return "" } + + // Strip ext. + path = path[:i] + + // Look for '/' of final path sep. + i = strings.LastIndexByte(path, '/') + if i == -1 { + return "" + } + + // Strip up to + // final segment. + path = path[i+1:] + + return path } diff --git a/internal/media/manager_test.go b/internal/media/manager_test.go index 7a4c865f4..d988ae274 100644 --- a/internal/media/manager_test.go +++ b/internal/media/manager_test.go @@ -105,7 +105,7 @@ func (suite *ManagerTestSuite) TestEmojiProcessRefresh() { return io.NopCloser(bytes.NewBuffer(b)), nil } - processing, err := suite.manager.RefreshEmoji(ctx, + processing, err := suite.manager.UpdateEmoji(ctx, emojiToUpdate, data, media.AdditionalEmojiInfo{ diff --git a/internal/media/refetch.go b/internal/media/refetch.go index e5b91d56f..5531f6d97 100644 --- a/internal/media/refetch.go +++ b/internal/media/refetch.go @@ -115,7 +115,7 @@ func (m *Manager) RefetchEmojis(ctx context.Context, domain string, dereferenceM return dereferenceMedia(ctx, emojiImageIRI, int64(maxsz)) } - processingEmoji, err := m.RefreshEmoji(ctx, emoji, dataFunc, AdditionalEmojiInfo{ + processingEmoji, err := m.UpdateEmoji(ctx, emoji, dataFunc, AdditionalEmojiInfo{ Domain: &emoji.Domain, ImageRemoteURL: &emoji.ImageRemoteURL, ImageStaticRemoteURL: &emoji.ImageStaticRemoteURL, @@ -123,16 +123,16 @@ func (m *Manager) RefetchEmojis(ctx context.Context, domain string, dereferenceM VisibleInPicker: emoji.VisibleInPicker, }) if err != nil { - log.Errorf(ctx, "emoji %s could not be refreshed because of an error during processing: %s", shortcodeDomain, err) + log.Errorf(ctx, "emoji %s could not be updated because of an error during processing: %s", shortcodeDomain, err) continue } if _, err := processingEmoji.Load(ctx); err != nil { - log.Errorf(ctx, "emoji %s could not be refreshed because of an error during loading: %s", shortcodeDomain, err) + log.Errorf(ctx, "emoji %s could not be updated because of an error during loading: %s", shortcodeDomain, err) continue } - log.Tracef(ctx, "refetched emoji %s successfully from remote", shortcodeDomain) + log.Tracef(ctx, "refetched + updated emoji %s successfully from remote", shortcodeDomain) totalRefetched++ } diff --git a/internal/processing/admin/emoji.go b/internal/processing/admin/emoji.go index 66193ccfe..70e196b95 100644 --- a/internal/processing/admin/emoji.go +++ b/internal/processing/admin/emoji.go @@ -291,13 +291,8 @@ func (p *Processor) emojiUpdateCopy( } // Ensure target emoji is locally cached. - target, err := p.federator.RefreshEmoji( - ctx, + target, err := p.federator.RecacheEmoji(ctx, target, - - // no changes we want to make. - media.AdditionalEmojiInfo{}, - false, ) if err != nil { err := gtserror.Newf("error recaching emoji %s: %w", target.ImageRemoteURL, err) @@ -325,8 +320,8 @@ func (p *Processor) emojiUpdateCopy( // Attempt to create the new local emoji. emoji, errWithCode := p.createEmoji(ctx, - util.PtrOrValue(shortcode, ""), - util.PtrOrValue(categoryName, ""), + util.PtrOrZero(shortcode), + util.PtrOrZero(categoryName), data, ) if errWithCode != nil { @@ -401,35 +396,39 @@ func (p *Processor) emojiUpdateModify( return nil, gtserror.NewErrorBadRequest(errors.New(text), text) } - if categoryName != nil { - if *categoryName != "" { - // A category was provided, get / create relevant emoji category. - category, errWithCode := p.mustGetEmojiCategory(ctx, *categoryName) - if errWithCode != nil { - return nil, errWithCode - } + // Check if we need to + // set a new category ID. + var newCategoryID *string + switch { + case categoryName == nil: + // No changes. - if category.ID == emoji.CategoryID { - // There was no change, - // indicate this by unsetting - // the category name pointer. - categoryName = nil - } else { - // Update emoji category. - emoji.CategoryID = category.ID - emoji.Category = category - } - } else { - // Emoji category was unset. - emoji.CategoryID = "" - emoji.Category = nil + case *categoryName == "": + // Emoji category was unset. + newCategoryID = util.Ptr("") + emoji.CategoryID = "" + emoji.Category = nil + + case *categoryName != "": + // A category was provided, get or create relevant emoji category. + category, errWithCode := p.mustGetEmojiCategory(ctx, *categoryName) + if errWithCode != nil { + return nil, errWithCode + } + + // Update emoji category if + // it's different from before. + if category.ID != emoji.CategoryID { + newCategoryID = &category.ID + emoji.CategoryID = category.ID + emoji.Category = category } } // Check whether any image changes were requested. imageUpdated := (image != nil && image.Size > 0) - if !imageUpdated && categoryName != nil { + if !imageUpdated && newCategoryID != nil { // Only updating category; only a single database update required. if err := p.state.DB.UpdateEmoji(ctx, emoji, "category_id"); err != nil { err := gtserror.Newf("error updating emoji in db: %w", err) @@ -463,8 +462,17 @@ func (p *Processor) emojiUpdateModify( return rc, nil } - // Prepare emoji model for recache from new data. - processing := p.media.RecacheEmoji(emoji, data) + // Include category ID + // update if necessary. + ai := media.AdditionalEmojiInfo{} + ai.CategoryID = newCategoryID + + // Prepare emoji model for update+recache from new data. + processing, err := p.media.UpdateEmoji(ctx, emoji, data, ai) + if err != nil { + err := gtserror.Newf("error preparing recache: %w", err) + return nil, gtserror.NewErrorInternalError(err) + } // Load to trigger update + write. emoji, err = processing.Load(ctx) diff --git a/internal/processing/media/getfile.go b/internal/processing/media/getfile.go index 43de718f3..6962601f2 100644 --- a/internal/processing/media/getfile.go +++ b/internal/processing/media/getfile.go @@ -246,11 +246,9 @@ func (p *Processor) getEmojiContent( // Ensure that stored emoji is cached. // (this handles local emoji / recaches). - emoji, err = p.federator.RefreshEmoji( + emoji, err = p.federator.RecacheEmoji( ctx, emoji, - media.AdditionalEmojiInfo{}, - false, ) if err != nil { err := gtserror.Newf("error recaching emoji: %w", err) diff --git a/testrig/storage.go b/testrig/storage.go index 69cbb5b0c..1f833044f 100644 --- a/testrig/storage.go +++ b/testrig/storage.go @@ -29,7 +29,7 @@ // NewInMemoryStorage returns a new in memory storage with the default test config func NewInMemoryStorage() *gtsstorage.Driver { - storage := memory.Open(200, false) + storage := memory.Open(200, true) return >sstorage.Driver{ Storage: storage, }