Code review

This commit is contained in:
Asaf Gartner 2022-02-07 14:21:40 +02:00
parent 92d6a31aa9
commit 43651d98e8
4 changed files with 49 additions and 50 deletions

View File

@ -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
}
}

View File

@ -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)

View File

@ -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)

View File

@ -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 {