From 43651d98e808deccdc961e43cf6c64816d0f7482 Mon Sep 17 00:00:00 2001 From: Asaf Gartner Date: Mon, 7 Feb 2022 14:21:40 +0200 Subject: [PATCH] Code review --- src/discord/gateway.go | 16 ++++----- src/discord/history.go | 58 ++++++++++++++++++--------------- src/discord/message_handling.go | 20 ++++-------- src/website/discord.go | 5 ++- 4 files changed, 49 insertions(+), 50 deletions(-) diff --git a/src/discord/gateway.go b/src/discord/gateway.go index 0d6cf44..55e63cd 100644 --- a/src/discord/gateway.go +++ b/src/discord/gateway.go @@ -597,7 +597,7 @@ func (bot *botInstance) messageCreateOrUpdate(ctx context.Context, msg *Message) } // NOTE(asaf): Since any error from HandleIncomingMessage is an internal error and not a discord - // error, we only want to log it and not restart the bot. So we're not returning the error. + // error, we only want to log it and not restart the bot. So we're not returning the error. return nil } @@ -606,15 +606,15 @@ func (bot *botInstance) messageDelete(ctx context.Context, msgDelete MessageDele interned, err := FetchInternedMessage(ctx, bot.dbConn, msgDelete.ID) if err != nil { - log.Error().Err(err).Msg("failed to fetch interned message") + if !errors.Is(err, db.NotFound) { + log.Error().Err(err).Msg("failed to fetch interned message") + } return } - if interned != nil { - err = DeleteInternedMessage(ctx, bot.dbConn, interned) - if err != nil { - log.Error().Err(err).Msg("failed to delete interned message") - return - } + err = DeleteInternedMessage(ctx, bot.dbConn, interned) + if err != nil { + log.Error().Err(err).Msg("failed to delete interned message") + return } } diff --git a/src/discord/history.go b/src/discord/history.go index b5b94ca..138bae7 100644 --- a/src/discord/history.go +++ b/src/discord/history.go @@ -30,15 +30,28 @@ func RunHistoryWatcher(ctx context.Context, dbConn *pgxpool.Pool) <-chan struct{ done <- struct{}{} }() - backfillInterval := 1 * time.Hour - newUserTicker := time.NewTicker(5 * time.Second) - // NOTE(asaf): 5 seconds to ensure this runs on start, we then reset it to the correct - // interval after the first run. - backfillTicker := time.NewTicker(5 * time.Second) + backfillFirstRun := make(chan struct{}, 1) + backfillFirstRun <- struct{}{} + backfillTicker := time.NewTicker(1 * time.Hour) + + lastBackfillTime := time.Now().Add(-3 * time.Hour) + + runBackfill := func() { + log.Info().Msg("Running backfill") + // Run a backfill to patch up places where the Discord bot missed (does create snippets) + now := time.Now() + done := Scrape(ctx, dbConn, + config.Config.Discord.ShowcaseChannelID, + lastBackfillTime, + true, + ) + if done { + lastBackfillTime = now + } + } - lastBackfillTime := time.Now().Add(-backfillInterval) for { select { case <-ctx.Done(): @@ -46,17 +59,10 @@ func RunHistoryWatcher(ctx context.Context, dbConn *pgxpool.Pool) <-chan struct{ case <-newUserTicker.C: // Get content for messages when a user links their account (but do not create snippets) fetchMissingContent(ctx, dbConn) + case <-backfillFirstRun: + runBackfill() case <-backfillTicker.C: - backfillTicker.Reset(backfillInterval) - // TODO(asaf): Do we need to update lastBackfillTime here? Otherwise we'll be rescraping - // from (start up time - 1 hour) every time. - - // Run a backfill to patch up places where the Discord bot missed (does create snippets) - Scrape(ctx, dbConn, - config.Config.Discord.ShowcaseChannelID, - lastBackfillTime, - true, - ) + runBackfill() } } }() @@ -107,11 +113,11 @@ func fetchMissingContent(ctx context.Context, dbConn *pgxpool.Pool) { // This message has apparently been deleted; delete it from our database interned, err := FetchInternedMessage(ctx, dbConn, msg.ID) if err != nil { - log.Error().Err(err).Msg("failed to fetch interned message") - continue - } - if interned == nil { - log.Error().Msg("couldn't find interned message") + if !errors.Is(err, db.NotFound) { + log.Error().Str("Message ID", msg.ID).Msg("couldn't find interned message") + } else { + log.Error().Err(err).Msg("failed to fetch interned message") + } continue } err = DeleteInternedMessage(ctx, dbConn, interned) @@ -138,7 +144,7 @@ func fetchMissingContent(ctx context.Context, dbConn *pgxpool.Pool) { } } -func Scrape(ctx context.Context, dbConn *pgxpool.Pool, channelID string, earliestMessageTime time.Time, createSnippets bool) { +func Scrape(ctx context.Context, dbConn *pgxpool.Pool, channelID string, earliestMessageTime time.Time, createSnippets bool) bool { log := logging.ExtractLogger(ctx) log.Info().Msg("Starting scrape") @@ -152,19 +158,19 @@ func Scrape(ctx context.Context, dbConn *pgxpool.Pool, channelID string, earlies }) if err != nil { logging.Error().Err(err).Msg("failed to get messages while scraping") - return + return false } if len(msgs) == 0 { logging.Debug().Msg("out of messages, stopping scrape") - return + return true } for _, msg := range msgs { select { case <-ctx.Done(): log.Info().Msg("Scrape was canceled") - return + return false default: } @@ -172,7 +178,7 @@ func Scrape(ctx context.Context, dbConn *pgxpool.Pool, channelID string, earlies if !earliestMessageTime.IsZero() && msg.Time().Before(earliestMessageTime) { logging.ExtractLogger(ctx).Info().Time("earliest", earliestMessageTime).Msg("Saw a message before the specified earliest time; exiting") - return + return true } err := HandleIncomingMessage(ctx, dbConn, &msg, createSnippets) diff --git a/src/discord/message_handling.go b/src/discord/message_handling.go index 0522197..29c511e 100644 --- a/src/discord/message_handling.go +++ b/src/discord/message_handling.go @@ -151,6 +151,8 @@ func MaybeInternMessage(ctx context.Context, dbConn db.ConnOrTx, msg *Message) e return nil } +var errNotEnoughInfo = errors.New("Discord didn't send enough info in this event for us to do this") + /* Ensures that a Discord message is stored in the database. This function is idempotent and can be called regardless of whether the item already exists in @@ -158,8 +160,6 @@ the database. This does not create snippets or save content or do anything besides save the message itself. */ -var errNotEnoughInfo = errors.New("Discord didn't send enough info in this event for us to do this") - func InternMessage( ctx context.Context, dbConn db.ConnOrTx, @@ -233,11 +233,7 @@ func FetchInternedMessage(ctx context.Context, dbConn db.ConnOrTx, msgId string) msgId, ) if err != nil { - if errors.Is(err, db.NotFound) { - return nil, nil - } else { - return nil, err - } + return nil, err } interned := result.(*InternedMessage) @@ -256,11 +252,9 @@ func HandleInternedMessage(ctx context.Context, dbConn db.ConnOrTx, msg *Message defer tx.Rollback(ctx) interned, err := FetchInternedMessage(ctx, tx, msg.ID) - if err != nil { + if err != nil && !errors.Is(err, db.NotFound) { return err - } - - if interned != nil { + } else if err == nil { if !deleted { err = SaveMessageContents(ctx, tx, interned, msg) if err != nil { @@ -717,8 +711,8 @@ func HandleSnippetForInternedMessage(ctx context.Context, dbConn db.ConnOrTx, in // TODO(asaf): We're not handling the case where embeds were removed or modified. // Also not handling the case where a message had both an attachment and an embed // and the attachment was removed (leaving only the embed). - LinkedUserIsSnippetOwner := existingSnippet.OwnerID == interned.DiscordUser.HMNUserId - if LinkedUserIsSnippetOwner && !existingSnippet.EditedOnWebsite { + linkedUserIsSnippetOwner := existingSnippet.OwnerID == interned.DiscordUser.HMNUserId + if linkedUserIsSnippetOwner && !existingSnippet.EditedOnWebsite { contentMarkdown := interned.MessageContent.LastContent contentHTML := parsing.ParseMarkdown(contentMarkdown, parsing.DiscordMarkdown) diff --git a/src/website/discord.go b/src/website/discord.go index a5f774e..d8e58ad 100644 --- a/src/website/discord.go +++ b/src/website/discord.go @@ -168,10 +168,9 @@ func DiscordShowcaseBacklog(c *RequestContext) ResponseData { } for _, msgID := range msgIDs { interned, err := discord.FetchInternedMessage(c.Context(), c.Conn, msgID) - if err != nil { + if err != nil && !errors.Is(err, db.NotFound) { return c.ErrorResponse(http.StatusInternalServerError, err) - } - if interned != nil { + } else if err == nil { // NOTE(asaf): Creating snippet even if the checkbox is off because the user asked us to. err = discord.HandleSnippetForInternedMessage(c.Context(), c.Conn, interned, true) if err != nil {