From 6bf39d0fc1286bdf2f4760adab52c6eff234d01d Mon Sep 17 00:00:00 2001 From: tsmethurst Date: Sat, 15 Jan 2022 17:36:15 +0100 Subject: [PATCH] emoji code passing muster --- internal/api/client/admin/emojicreate.go | 10 ++-- internal/api/client/admin/emojicreate_test.go | 50 ++++++++++++++++--- internal/gtserror/withcode.go | 13 +++++ internal/media/processingemoji.go | 22 +++++--- internal/media/processingmedia.go | 25 +++++----- internal/processing/admin.go | 2 +- internal/processing/admin/admin.go | 2 +- internal/processing/admin/emoji.go | 17 ++++--- internal/processing/processor.go | 2 +- 9 files changed, 104 insertions(+), 39 deletions(-) diff --git a/internal/api/client/admin/emojicreate.go b/internal/api/client/admin/emojicreate.go index de7a2979a..ef42d0a13 100644 --- a/internal/api/client/admin/emojicreate.go +++ b/internal/api/client/admin/emojicreate.go @@ -73,6 +73,8 @@ // description: forbidden // '400': // description: bad request +// '409': +// description: conflict -- domain/shortcode combo for emoji already exists func (m *Module) EmojiCreatePOSTHandler(c *gin.Context) { l := logrus.WithFields(logrus.Fields{ "func": "emojiCreatePOSTHandler", @@ -116,10 +118,10 @@ func (m *Module) EmojiCreatePOSTHandler(c *gin.Context) { return } - apiEmoji, err := m.processor.AdminEmojiCreate(c.Request.Context(), authed, form) - if err != nil { - l.Debugf("error creating emoji: %s", err) - c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + apiEmoji, errWithCode := m.processor.AdminEmojiCreate(c.Request.Context(), authed, form) + if errWithCode != nil { + l.Debugf("error creating emoji: %s", errWithCode.Error()) + c.JSON(errWithCode.Code(), gin.H{"error": errWithCode.Safe()}) return } diff --git a/internal/api/client/admin/emojicreate_test.go b/internal/api/client/admin/emojicreate_test.go index 14b83b534..2b7476da1 100644 --- a/internal/api/client/admin/emojicreate_test.go +++ b/internal/api/client/admin/emojicreate_test.go @@ -25,7 +25,7 @@ func (suite *EmojiCreateTestSuite) TestEmojiCreate() { requestBody, w, err := testrig.CreateMultipartFormData( "image", "../../../../testrig/media/rainbow-original.png", map[string]string{ - "shortcode": "rainbow", + "shortcode": "new_emoji", }) if err != nil { panic(err) @@ -55,24 +55,24 @@ func (suite *EmojiCreateTestSuite) TestEmojiCreate() { suite.NoError(err) // appropriate fields should be set - suite.Equal("rainbow", apiEmoji.Shortcode) + suite.Equal("new_emoji", apiEmoji.Shortcode) suite.NotEmpty(apiEmoji.URL) suite.NotEmpty(apiEmoji.StaticURL) suite.True(apiEmoji.VisibleInPicker) // emoji should be in the db dbEmoji := >smodel.Emoji{} - err = suite.db.GetWhere(context.Background(), []db.Where{{Key: "shortcode", Value: "rainbow"}}, dbEmoji) + err = suite.db.GetWhere(context.Background(), []db.Where{{Key: "shortcode", Value: "new_emoji"}}, dbEmoji) suite.NoError(err) // check fields on the emoji suite.NotEmpty(dbEmoji.ID) - suite.Equal("rainbow", dbEmoji.Shortcode) + suite.Equal("new_emoji", dbEmoji.Shortcode) suite.Empty(dbEmoji.Domain) suite.Empty(dbEmoji.ImageRemoteURL) suite.Empty(dbEmoji.ImageStaticRemoteURL) suite.Equal(apiEmoji.URL, dbEmoji.ImageURL) - suite.Equal(apiEmoji.StaticURL, dbEmoji.ImageURL) + suite.Equal(apiEmoji.StaticURL, dbEmoji.ImageStaticURL) suite.NotEmpty(dbEmoji.ImagePath) suite.NotEmpty(dbEmoji.ImageStaticPath) suite.Equal("image/png", dbEmoji.ImageContentType) @@ -82,7 +82,45 @@ func (suite *EmojiCreateTestSuite) TestEmojiCreate() { suite.False(dbEmoji.Disabled) suite.NotEmpty(dbEmoji.URI) suite.True(dbEmoji.VisibleInPicker) - suite.Empty(dbEmoji.CategoryID)aaaaaaaaa + suite.Empty(dbEmoji.CategoryID) + + // emoji should be in storage + emojiBytes, err := suite.storage.Get(dbEmoji.ImagePath) + suite.NoError(err) + suite.Len(emojiBytes, dbEmoji.ImageFileSize) + emojiStaticBytes, err := suite.storage.Get(dbEmoji.ImageStaticPath) + suite.NoError(err) + suite.Len(emojiStaticBytes, dbEmoji.ImageStaticFileSize) +} + +func (suite *EmojiCreateTestSuite) TestEmojiCreateAlreadyExists() { + // set up the request -- use a shortcode that already exists for an emoji in the database + requestBody, w, err := testrig.CreateMultipartFormData( + "image", "../../../../testrig/media/rainbow-original.png", + map[string]string{ + "shortcode": "rainbow", + }) + if err != nil { + panic(err) + } + bodyBytes := requestBody.Bytes() + recorder := httptest.NewRecorder() + ctx := suite.newContext(recorder, http.MethodPost, bodyBytes, admin.EmojiPath, w.FormDataContentType()) + + // call the handler + suite.adminModule.EmojiCreatePOSTHandler(ctx) + + suite.Equal(http.StatusConflict, recorder.Code) + + result := recorder.Result() + defer result.Body.Close() + + // check the response + b, err := ioutil.ReadAll(result.Body) + suite.NoError(err) + suite.NotEmpty(b) + + suite.Equal(`{"error":"conflict: emoji with shortcode rainbow already exists"}`, string(b)) } func TestEmojiCreateTestSuite(t *testing.T) { diff --git a/internal/gtserror/withcode.go b/internal/gtserror/withcode.go index a00cc8503..34889b961 100644 --- a/internal/gtserror/withcode.go +++ b/internal/gtserror/withcode.go @@ -122,3 +122,16 @@ func NewErrorInternalError(original error, helpText ...string) WithCode { code: http.StatusInternalServerError, } } + +// NewErrorConflict returns an ErrorWithCode 409 with the given original error and optional help text. +func NewErrorConflict(original error, helpText ...string) WithCode { + safe := "conflict" + if helpText != nil { + safe = safe + ": " + strings.Join(helpText, ": ") + } + return withCode{ + original: original, + safe: errors.New(safe), + code: http.StatusConflict, + } +} diff --git a/internal/media/processingemoji.go b/internal/media/processingemoji.go index eeccdb281..467b500fc 100644 --- a/internal/media/processingemoji.go +++ b/internal/media/processingemoji.go @@ -72,6 +72,9 @@ type ProcessingEmoji struct { storage *kv.KVStore err error // error created during processing, if any + + // track whether this emoji has already been put in the databse + insertedInDB bool } // EmojiID returns the ID of the underlying emoji without blocking processing. @@ -94,6 +97,16 @@ func (p *ProcessingEmoji) LoadEmoji(ctx context.Context) (*gtsmodel.Emoji, error return nil, err } + // store the result in the database before returning it + p.mu.Lock() + defer p.mu.Unlock() + if !p.insertedInDB { + if err := p.database.Put(ctx, p.emoji); err != nil { + return nil, err + } + p.insertedInDB = true + } + return p.emoji, nil } @@ -127,13 +140,6 @@ func (p *ProcessingEmoji) loadStatic(ctx context.Context) (*ImageMeta, error) { // set appropriate fields on the emoji based on the static version we derived p.emoji.ImageStaticFileSize = len(static.image) - // update the emoji in the db - if err := putOrUpdate(ctx, p.database, p.emoji); err != nil { - p.err = err - p.staticState = errored - return nil, err - } - // set the static on the processing emoji p.static = static @@ -197,7 +203,7 @@ func (p *ProcessingEmoji) loadFullSize(ctx context.Context) (*ImageMeta, error) } // fetchRawData calls the data function attached to p if it hasn't been called yet, -// and updates the underlying attachment fields as necessary. +// and updates the underlying emoji fields as necessary. // It should only be called from within a function that already has a lock on p! func (p *ProcessingEmoji) fetchRawData(ctx context.Context) error { // check if we've already done this and bail early if we have diff --git a/internal/media/processingmedia.go b/internal/media/processingmedia.go index 1bfd7b629..fade64c24 100644 --- a/internal/media/processingmedia.go +++ b/internal/media/processingmedia.go @@ -70,6 +70,9 @@ type ProcessingMedia struct { storage *kv.KVStore err error // error created during processing, if any + + // track whether this media has already been put in the databse + insertedInDB bool } // AttachmentID returns the ID of the underlying media attachment without blocking processing. @@ -92,6 +95,16 @@ func (p *ProcessingMedia) LoadAttachment(ctx context.Context) (*gtsmodel.MediaAt return nil, err } + // store the result in the database before returning it + p.mu.Lock() + defer p.mu.Unlock() + if !p.insertedInDB { + if err := p.database.Put(ctx, p.attachment); err != nil { + return nil, err + } + p.insertedInDB = true + } + return p.attachment, nil } @@ -143,12 +156,6 @@ func (p *ProcessingMedia) loadThumb(ctx context.Context) (*ImageMeta, error) { } p.attachment.Thumbnail.FileSize = len(thumb.image) - if err := putOrUpdate(ctx, p.database, p.attachment); err != nil { - p.err = err - p.thumbstate = errored - return nil, err - } - // set the thumbnail of this media p.thumb = thumb @@ -216,12 +223,6 @@ func (p *ProcessingMedia) loadFullSize(ctx context.Context) (*ImageMeta, error) p.attachment.File.UpdatedAt = time.Now() p.attachment.Processing = gtsmodel.ProcessingStatusProcessed - if err := putOrUpdate(ctx, p.database, p.attachment); err != nil { - p.err = err - p.fullSizeState = errored - return nil, err - } - // set the fullsize of this media p.fullSize = decoded diff --git a/internal/processing/admin.go b/internal/processing/admin.go index c70bd79d0..764e6d302 100644 --- a/internal/processing/admin.go +++ b/internal/processing/admin.go @@ -26,7 +26,7 @@ "github.com/superseriousbusiness/gotosocial/internal/oauth" ) -func (p *processor) AdminEmojiCreate(ctx context.Context, authed *oauth.Auth, form *apimodel.EmojiCreateRequest) (*apimodel.Emoji, error) { +func (p *processor) AdminEmojiCreate(ctx context.Context, authed *oauth.Auth, form *apimodel.EmojiCreateRequest) (*apimodel.Emoji, gtserror.WithCode) { return p.adminProcessor.EmojiCreate(ctx, authed.Account, authed.User, form) } diff --git a/internal/processing/admin/admin.go b/internal/processing/admin/admin.go index 27a7da47a..bdb586588 100644 --- a/internal/processing/admin/admin.go +++ b/internal/processing/admin/admin.go @@ -38,7 +38,7 @@ type Processor interface { DomainBlocksGet(ctx context.Context, account *gtsmodel.Account, export bool) ([]*apimodel.DomainBlock, gtserror.WithCode) DomainBlockGet(ctx context.Context, account *gtsmodel.Account, id string, export bool) (*apimodel.DomainBlock, gtserror.WithCode) DomainBlockDelete(ctx context.Context, account *gtsmodel.Account, id string) (*apimodel.DomainBlock, gtserror.WithCode) - EmojiCreate(ctx context.Context, account *gtsmodel.Account, user *gtsmodel.User, form *apimodel.EmojiCreateRequest) (*apimodel.Emoji, error) + EmojiCreate(ctx context.Context, account *gtsmodel.Account, user *gtsmodel.User, form *apimodel.EmojiCreateRequest) (*apimodel.Emoji, gtserror.WithCode) } type processor struct { diff --git a/internal/processing/admin/emoji.go b/internal/processing/admin/emoji.go index 77fa5102b..fcc17c4be 100644 --- a/internal/processing/admin/emoji.go +++ b/internal/processing/admin/emoji.go @@ -26,14 +26,16 @@ "io" apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model" + "github.com/superseriousbusiness/gotosocial/internal/db" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/id" "github.com/superseriousbusiness/gotosocial/internal/uris" ) -func (p *processor) EmojiCreate(ctx context.Context, account *gtsmodel.Account, user *gtsmodel.User, form *apimodel.EmojiCreateRequest) (*apimodel.Emoji, error) { +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, fmt.Errorf("user %s not an admin", user.ID) + return nil, gtserror.NewErrorNotAuthorized(fmt.Errorf("user %s not an admin", user.ID), "user is not an admin") } data := func(innerCtx context.Context) ([]byte, error) { @@ -56,24 +58,27 @@ func (p *processor) EmojiCreate(ctx context.Context, account *gtsmodel.Account, emojiID, err := id.NewRandomULID() if err != nil { - return nil, fmt.Errorf("error creating id for new emoji: %s", err) + return nil, gtserror.NewErrorInternalError(fmt.Errorf("error creating id for new emoji: %s", err), "error creating emoji ID") } emojiURI := uris.GenerateURIForEmoji(emojiID) processingEmoji, err := p.mediaManager.ProcessEmoji(ctx, data, form.Shortcode, emojiID, emojiURI, nil) if err != nil { - return nil, err + return nil, gtserror.NewErrorInternalError(fmt.Errorf("error processing emoji: %s", err), "error processing emoji") } emoji, err := processingEmoji.LoadEmoji(ctx) if err != nil { - return nil, err + if err == db.ErrAlreadyExists { + return nil, gtserror.NewErrorConflict(fmt.Errorf("emoji with shortcode %s already exists", form.Shortcode), fmt.Sprintf("emoji with shortcode %s already exists", form.Shortcode)) + } + return nil, gtserror.NewErrorInternalError(fmt.Errorf("error loading emoji: %s", err), "error loading emoji") } apiEmoji, err := p.tc.EmojiToAPIEmoji(ctx, emoji) if err != nil { - return nil, fmt.Errorf("error converting emoji to apitype: %s", err) + return nil, gtserror.NewErrorInternalError(fmt.Errorf("error converting emoji: %s", err), "error converting emoji to api representation") } return &apiEmoji, nil diff --git a/internal/processing/processor.go b/internal/processing/processor.go index 2626c1fea..2406681ea 100644 --- a/internal/processing/processor.go +++ b/internal/processing/processor.go @@ -96,7 +96,7 @@ type Processor interface { AccountBlockRemove(ctx context.Context, authed *oauth.Auth, targetAccountID string) (*apimodel.Relationship, gtserror.WithCode) // AdminEmojiCreate handles the creation of a new instance emoji by an admin, using the given form. - AdminEmojiCreate(ctx context.Context, authed *oauth.Auth, form *apimodel.EmojiCreateRequest) (*apimodel.Emoji, error) + AdminEmojiCreate(ctx context.Context, authed *oauth.Auth, form *apimodel.EmojiCreateRequest) (*apimodel.Emoji, gtserror.WithCode) // AdminDomainBlockCreate handles the creation of a new domain block by an admin, using the given form. AdminDomainBlockCreate(ctx context.Context, authed *oauth.Auth, form *apimodel.DomainBlockCreateRequest) (*apimodel.DomainBlock, gtserror.WithCode) // AdminDomainBlocksImport handles the import of multiple domain blocks by an admin, using the given form.