From 5ba51adb1414ca8fb7175ccaebc6fbe2af85da9d Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Sat, 1 Mar 2025 10:48:36 +0100 Subject: [PATCH] When retraining ham/spam messages, don't make existence of the messages optional. If messages that should exist don't, that's a real error we don't want to hide. Part of larger changes. --- imapserver/replace.go | 2 +- imapserver/server.go | 8 ++++---- store/account.go | 22 ++++++++++++++-------- store/account_test.go | 6 +++--- store/train.go | 22 +++++++++++----------- webmail/api.go | 4 ++-- webops/xops.go | 8 ++++---- 7 files changed, 39 insertions(+), 33 deletions(-) diff --git a/imapserver/replace.go b/imapserver/replace.go index 2fb63c2..42bb28d 100644 --- a/imapserver/replace.go +++ b/imapserver/replace.go @@ -283,7 +283,7 @@ func (c *conn) cmdxReplace(isUID bool, tag, cmd string, p *parser) { // Undo any junk filter training for the old message. om.Junk = false om.Notjunk = false - err = c.account.RetrainMessages(context.TODO(), c.log, tx, []store.Message{om}, false) + err = c.account.RetrainMessages(context.TODO(), c.log, tx, []store.Message{om}) xcheckf(err, "untraining expunged messages") // Mark old message expunged. diff --git a/imapserver/server.go b/imapserver/server.go index b79cac1..1d42ee9 100644 --- a/imapserver/server.go +++ b/imapserver/server.go @@ -3842,7 +3842,7 @@ func (c *conn) xexpunge(uidSet *numSet, missingMailboxOK bool) (removed []store. removed[i].Junk = false removed[i].Notjunk = false } - err = c.account.RetrainMessages(context.TODO(), c.log, tx, removed, true) + err = c.account.RetrainMessages(context.TODO(), c.log, tx, removed) xcheckf(err, "untraining expunged messages") }) @@ -4210,7 +4210,7 @@ func (c *conn) cmdxCopy(isUID bool, tag, cmd string, p *parser) { xcheckf(err, "sync directory") } - err = c.account.RetrainMessages(context.TODO(), c.log, tx, nmsgs, false) + err = c.account.RetrainMessages(context.TODO(), c.log, tx, nmsgs) xcheckf(err, "train copied messages") }) @@ -4358,7 +4358,7 @@ func (c *conn) cmdxMove(isUID bool, tag, cmd string, p *parser) { err = tx.Update(&mbDst) xcheckf(err, "updating destination mailbox for uids, keywords and counts") - err = c.account.RetrainMessages(context.TODO(), c.log, tx, msgs, false) + err = c.account.RetrainMessages(context.TODO(), c.log, tx, msgs) xcheckf(err, "retraining messages after move") // Prepare broadcast changes to other connections. @@ -4578,7 +4578,7 @@ func (c *conn) cmdxStore(isUID bool, tag, cmd string, p *parser) { changes = append(changes, mb.ChangeKeywords()) } - err = c.account.RetrainMessages(context.TODO(), c.log, tx, updated, false) + err = c.account.RetrainMessages(context.TODO(), c.log, tx, updated) xcheckf(err, "training messages") }) diff --git a/store/account.go b/store/account.go index 046ebaa..22286b8 100644 --- a/store/account.go +++ b/store/account.go @@ -654,11 +654,17 @@ func (m Message) LoadPart(r io.ReaderAt) (message.Part, error) { // NeedsTraining returns whether message needs a training update, based on // TrainedJunk (current training status) and new Junk/Notjunk flags. func (m Message) NeedsTraining() bool { - untrain := m.TrainedJunk != nil - untrainJunk := untrain && *m.TrainedJunk - train := m.Junk != m.Notjunk - trainJunk := m.Junk - return untrain != train || untrain && train && untrainJunk != trainJunk + needs, _, _, _, _ := m.needsTraining() + return needs +} + +func (m Message) needsTraining() (needs, untrain, untrainJunk, train, trainJunk bool) { + untrain = m.TrainedJunk != nil + untrainJunk = untrain && *m.TrainedJunk + train = m.Junk != m.Notjunk + trainJunk = m.Junk + needs = untrain != train || untrain && train && untrainJunk != trainJunk + return } // JunkFlagsForMailbox sets Junk and Notjunk flags based on mailbox name if configured. Often @@ -1850,7 +1856,7 @@ func (a *Account) DeliverMessage(log mlog.Log, tx *bstore.Tx, m *Message, msgFil if !notrain && m.NeedsTraining() { l := []Message{*m} - if err := a.RetrainMessages(context.TODO(), log, tx, l, false); err != nil { + if err := a.RetrainMessages(context.TODO(), log, tx, l); err != nil { return fmt.Errorf("training junkfilter: %w", err) } *m = l[0] @@ -2422,7 +2428,7 @@ func (a *Account) rejectsRemoveMessages(ctx context.Context, log mlog.Log, tx *b expunged[i].Junk = false expunged[i].Notjunk = false } - if err := a.RetrainMessages(ctx, log, tx, expunged, true); err != nil { + if err := a.RetrainMessages(ctx, log, tx, expunged); err != nil { return nil, fmt.Errorf("retraining expunged messages: %w", err) } @@ -3134,7 +3140,7 @@ func (a *Account) MailboxDelete(ctx context.Context, log mlog.Log, tx *bstore.Tx } } remove = remove[:n] - if err := a.RetrainMessages(ctx, log, tx, remove, true); err != nil { + if err := a.RetrainMessages(ctx, log, tx, remove); err != nil { return nil, nil, false, fmt.Errorf("untraining deleted messages: %v", err) } } diff --git a/store/account_test.go b/store/account_test.go index 945cc53..f7d8531 100644 --- a/store/account_test.go +++ b/store/account_test.go @@ -127,7 +127,7 @@ func TestMailbox(t *testing.T) { err = acc.DB.Write(ctxbg, func(tx *bstore.Tx) error { m.Junk = true l := []Message{m} - err = acc.RetrainMessages(ctxbg, log, tx, l, false) + err = acc.RetrainMessages(ctxbg, log, tx, l) tcheck(t, err, "train as junk") m = l[0] return nil @@ -140,7 +140,7 @@ func TestMailbox(t *testing.T) { jf, _, err := acc.OpenJunkFilter(ctxbg, log) tcheck(t, err, "open junk filter") err = acc.DB.Write(ctxbg, func(tx *bstore.Tx) error { - return acc.RetrainMessage(ctxbg, log, tx, jf, &m, false) + return acc.RetrainMessage(ctxbg, log, tx, jf, &m) }) tcheck(t, err, "retraining as non-junk") err = jf.Close() @@ -148,7 +148,7 @@ func TestMailbox(t *testing.T) { m.Notjunk = false err = acc.DB.Write(ctxbg, func(tx *bstore.Tx) error { - return acc.RetrainMessages(ctxbg, log, tx, []Message{m}, false) + return acc.RetrainMessages(ctxbg, log, tx, []Message{m}) }) tcheck(t, err, "untraining non-junk") diff --git a/store/train.go b/store/train.go index fcfa05a..5cd002c 100644 --- a/store/train.go +++ b/store/train.go @@ -19,6 +19,11 @@ import ( // ErrNoJunkFilter indicates user did not configure/enable a junk filter. var ErrNoJunkFilter = errors.New("junkfilter: not configured") +func (a *Account) HasJunkFilter() bool { + conf, _ := a.Conf() + return conf.JunkFilter != nil +} + // OpenJunkFilter returns an opened junk filter for the account. // If the account does not have a junk filter enabled, ErrNotConfigured is returned. // Do not forget to save the filter after modifying, and to always close the filter when done. @@ -47,7 +52,7 @@ func (a *Account) OpenJunkFilter(ctx context.Context, log mlog.Log) (*junk.Filte // RetrainMessages (un)trains messages, if relevant given their flags. Updates // m.TrainedJunk after retraining. -func (a *Account) RetrainMessages(ctx context.Context, log mlog.Log, tx *bstore.Tx, msgs []Message, absentOK bool) (rerr error) { +func (a *Account) RetrainMessages(ctx context.Context, log mlog.Log, tx *bstore.Tx, msgs []Message) (rerr error) { if len(msgs) == 0 { return nil } @@ -78,7 +83,7 @@ func (a *Account) RetrainMessages(ctx context.Context, log mlog.Log, tx *bstore. } }() } - if err := a.RetrainMessage(ctx, log, tx, jf, &msgs[i], absentOK); err != nil { + if err := a.RetrainMessage(ctx, log, tx, jf, &msgs[i]); err != nil { return err } } @@ -87,16 +92,11 @@ func (a *Account) RetrainMessages(ctx context.Context, log mlog.Log, tx *bstore. // RetrainMessage untrains and/or trains a message, if relevant given m.TrainedJunk // and m.Junk/m.Notjunk. Updates m.TrainedJunk after retraining. -func (a *Account) RetrainMessage(ctx context.Context, log mlog.Log, tx *bstore.Tx, jf *junk.Filter, m *Message, absentOK bool) error { - untrain := m.TrainedJunk != nil - untrainJunk := untrain && *m.TrainedJunk - train := m.Junk != m.Notjunk - trainJunk := m.Junk - - if !untrain && !train || (untrain && train && untrainJunk == trainJunk) { +func (a *Account) RetrainMessage(ctx context.Context, log mlog.Log, tx *bstore.Tx, jf *junk.Filter, m *Message) error { + need, untrain, untrainJunk, train, trainJunk := m.needsTraining() + if !need { return nil } - log.Debug("updating junk filter", slog.Bool("untrain", untrain), slog.Bool("untrainjunk", untrainJunk), @@ -135,7 +135,7 @@ func (a *Account) RetrainMessage(ctx context.Context, log mlog.Log, tx *bstore.T } m.TrainedJunk = &trainJunk } - if err := tx.Update(m); err != nil && (!absentOK || err != bstore.ErrAbsent) { + if err := tx.Update(m); err != nil { return err } return nil diff --git a/webmail/api.go b/webmail/api.go index a0ac290..7c4ed0e 100644 --- a/webmail/api.go +++ b/webmail/api.go @@ -1063,7 +1063,7 @@ func (w Webmail) MessageSubmit(ctx context.Context, m SubmitMessage) { err = tx.Update(&rmb) xcheckf(ctx, err, "update modseqo of mailbox of replied/forwarded message") - err = acc.RetrainMessages(ctx, log, tx, []store.Message{rm}, false) + err = acc.RetrainMessages(ctx, log, tx, []store.Message{rm}) xcheckf(ctx, err, "retraining messages after reply/forward") } @@ -1337,7 +1337,7 @@ func (Webmail) MailboxEmpty(ctx context.Context, mailboxID int64) { err = acc.AddMessageSize(log, tx, -totalSize) xcheckf(ctx, err, "updating disk usage") - err = acc.RetrainMessages(ctx, log, tx, expunged, true) + err = acc.RetrainMessages(ctx, log, tx, expunged) xcheckf(ctx, err, "retraining expunged messages") chremove := store.ChangeRemoveUIDs{MailboxID: mb.ID, UIDs: uids, ModSeq: modseq} diff --git a/webops/xops.go b/webops/xops.go index b301b2d..c346097 100644 --- a/webops/xops.go +++ b/webops/xops.go @@ -139,7 +139,7 @@ func (x XOps) MessageDeleteTx(ctx context.Context, log mlog.Log, tx *bstore.Tx, remove[i].Junk = false remove[i].Notjunk = false } - err = acc.RetrainMessages(ctx, log, tx, remove, true) + err = acc.RetrainMessages(ctx, log, tx, remove) x.Checkf(ctx, err, "untraining deleted messages") for _, ch := range removeChanges { @@ -220,7 +220,7 @@ func (x XOps) MessageFlagsAdd(ctx context.Context, log mlog.Log, acc *store.Acco } } - err = acc.RetrainMessages(ctx, log, tx, retrain, false) + err = acc.RetrainMessages(ctx, log, tx, retrain) x.Checkf(ctx, err, "retraining messages") }) @@ -291,7 +291,7 @@ func (x XOps) MessageFlagsClear(ctx context.Context, log mlog.Log, acc *store.Ac // note: cannot remove keywords from mailbox by removing keywords from message. } - err = acc.RetrainMessages(ctx, log, tx, retrain, false) + err = acc.RetrainMessages(ctx, log, tx, retrain) x.Checkf(ctx, err, "retraining messages") }) @@ -487,7 +487,7 @@ func (x XOps) MessageMoveTx(ctx context.Context, log mlog.Log, acc *store.Accoun err = tx.Update(&mbDst) x.Checkf(ctx, err, "updating destination mailbox with uidnext and modseq") - err = acc.RetrainMessages(ctx, log, tx, retrain, false) + err = acc.RetrainMessages(ctx, log, tx, retrain) x.Checkf(ctx, err, "retraining messages after move") // Ensure UIDs of the removed message are in increasing order. It is quite common