From e46323c207230b01e48ae1b8e5117a9774d9f157 Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Thu, 6 Apr 2023 16:11:25 +0100 Subject: [PATCH] [bugfix] visibility caching and hometimeline (#1675) * fix visibility caching to use correct type key Signed-off-by: kim * check for ID check > max possible ID Signed-off-by: kim * update home timeline code to include relevant threads to owner (e.g. between mutuals/follows) Signed-off-by: kim --------- Signed-off-by: kim --- internal/db/bundb/timeline.go | 2 +- internal/visibility/account.go | 6 +- internal/visibility/home_timeline.go | 108 +++++++++++++++++++++---- internal/visibility/public_timeline.go | 8 +- internal/visibility/status.go | 12 +-- 5 files changed, 110 insertions(+), 26 deletions(-) diff --git a/internal/db/bundb/timeline.go b/internal/db/bundb/timeline.go index fabfe2797..87e7751d2 100644 --- a/internal/db/bundb/timeline.go +++ b/internal/db/bundb/timeline.go @@ -61,7 +61,7 @@ func (t *timelineDB) GetHomeTimeline(ctx context.Context, accountID string, maxI bun.Ident("follow.account_id"), accountID) - if maxID == "" || maxID == id.Highest { + if maxID == "" || maxID >= id.Highest { const future = 24 * time.Hour var err error diff --git a/internal/visibility/account.go b/internal/visibility/account.go index ca532f5dd..6c7a0059d 100644 --- a/internal/visibility/account.go +++ b/internal/visibility/account.go @@ -29,6 +29,8 @@ // AccountVisible will check if given account is visible to requester, accounting for requester with no auth (i.e is nil), suspensions, disabled local users and account blocks. func (f *Filter) AccountVisible(ctx context.Context, requester *gtsmodel.Account, account *gtsmodel.Account) (bool, error) { + const vtype = cache.VisibilityTypeAccount + // By default we assume no auth. requesterID := noauth @@ -48,10 +50,10 @@ func (f *Filter) AccountVisible(ctx context.Context, requester *gtsmodel.Account return &cache.CachedVisibility{ ItemID: account.ID, RequesterID: requesterID, - Type: cache.VisibilityTypeAccount, + Type: vtype, Value: visible, }, nil - }, "account", requesterID, account.ID) + }, vtype, requesterID, account.ID) if err != nil { return false, err } diff --git a/internal/visibility/home_timeline.go b/internal/visibility/home_timeline.go index 3f0f1f16a..1f61df916 100644 --- a/internal/visibility/home_timeline.go +++ b/internal/visibility/home_timeline.go @@ -30,6 +30,8 @@ // StatusHomeTimelineable checks if given status should be included on owner's home timeline. Primarily relying on status visibility to owner and the AP visibility setting, but also taking into account thread replies etc. func (f *Filter) StatusHomeTimelineable(ctx context.Context, owner *gtsmodel.Account, status *gtsmodel.Status) (bool, error) { + const vtype = cache.VisibilityTypeHome + // By default we assume no auth. requesterID := noauth @@ -49,10 +51,10 @@ func (f *Filter) StatusHomeTimelineable(ctx context.Context, owner *gtsmodel.Acc return &cache.CachedVisibility{ ItemID: status.ID, RequesterID: requesterID, - Type: cache.VisibilityTypeHome, + Type: vtype, Value: visible, }, nil - }, "home", requesterID, status.ID) + }, vtype, requesterID, status.ID) if err != nil { if err == cache.SentinelError { // Filter-out our temporary @@ -95,21 +97,22 @@ func (f *Filter) isStatusHomeTimelineable(ctx context.Context, owner *gtsmodel.A } var ( - parent *gtsmodel.Status - included bool + next *gtsmodel.Status oneAuthor bool + included bool + converstn bool ) - for parent = status; parent.InReplyToURI != ""; { + for next = status; next.InReplyToURI != ""; { // Fetch next parent to lookup. - parentID := parent.InReplyToID + parentID := next.InReplyToID if parentID == "" { - log.Warnf(ctx, "status not yet deref'd: %s", parent.InReplyToURI) + log.Warnf(ctx, "status not yet deref'd: %s", next.InReplyToURI) return false, cache.SentinelError } // Get the next parent in the chain from DB. - parent, err = f.state.DB.GetStatusByID( + next, err = f.state.DB.GetStatusByID( gtscontext.SetBarebones(ctx), parentID, ) @@ -117,28 +120,49 @@ func (f *Filter) isStatusHomeTimelineable(ctx context.Context, owner *gtsmodel.A return false, fmt.Errorf("isStatusHomeTimelineable: error getting status parent %s: %w", parentID, err) } - if (parent.AccountID == owner.ID) || - parent.MentionsAccount(owner.ID) { + // Populate account mention objects before account mention checks. + next.Mentions, err = f.state.DB.GetMentions(ctx, next.MentionIDs) + if err != nil { + return false, fmt.Errorf("isStatusHomeTimelineable: error populating status parent %s mentions: %w", parentID, err) + } + + if (next.AccountID == owner.ID) || + next.MentionsAccount(owner.ID) { // Owner is in / mentioned in - // this status thread. + // this status thread. They can + // see all future visible statuses. included = true break } + // Check whether this should be a visible conversation, i.e. + // is it between accounts on owner timeline that they follow? + converstn, err = f.isVisibleConversation(ctx, owner, next) + if err != nil { + return false, fmt.Errorf("isStatusHomeTimelineable: error checking conversation visibility: %w", err) + } + + if converstn { + // Owner is relevant to this conversation, + // i.e. between follows / mutuals they know. + break + } + if oneAuthor { // Check if this is a single-author status thread. - oneAuthor = (parent.AccountID == status.AccountID) + oneAuthor = (next.AccountID == status.AccountID) } } - if parent != status && !included && !oneAuthor { - log.Trace(ctx, "ignoring visible reply to conversation thread excluding owner") + if next != status && !oneAuthor && !included && !converstn { + log.Trace(ctx, "ignoring visible reply in conversation irrelevant to owner") return false, nil } // At this point status is either a top-level status, a reply in a single // author thread (e.g. "this is my weird-ass take and here is why 1/10 🧵"), - // or a thread mentioning / including timeline owner. + // a status thread *including* the owner, or a conversation thread between + // accounts the timeline owner follows. if status.Visibility == gtsmodel.VisibilityFollowersOnly || status.Visibility == gtsmodel.VisibilityMutualsOnly { @@ -163,3 +187,57 @@ func (f *Filter) isStatusHomeTimelineable(ctx context.Context, owner *gtsmodel.A return true, nil } + +func (f *Filter) isVisibleConversation(ctx context.Context, owner *gtsmodel.Account, status *gtsmodel.Status) (bool, error) { + // Check if status is visible to the timeline owner. + visible, err := f.StatusVisible(ctx, owner, status) + if err != nil { + return false, err + } + + if !visible { + // Invisible to + // timeline owner. + return false, nil + } + + if status.Visibility == gtsmodel.VisibilityUnlocked || + status.Visibility == gtsmodel.VisibilityPublic { + // NOTE: there is no need to check in the case of + // direct / follow-only / mutual-only visibility statuses + // as the above visibility check already handles this. + + // Check if owner follows the status author. + followAuthor, err := f.state.DB.IsFollowing(ctx, + owner.ID, + status.AccountID, + ) + if err != nil { + return false, fmt.Errorf("error checking follow %s->%s: %w", owner.ID, status.AccountID, err) + } + + if !followAuthor { + // Not a visible status + // in conversation thread. + return false, nil + } + } + + for _, mention := range status.Mentions { + // Check if timeline owner follows target. + follow, err := f.state.DB.IsFollowing(ctx, + owner.ID, + mention.TargetAccountID, + ) + if err != nil { + return false, fmt.Errorf("error checking mention follow %s->%s: %w", owner.ID, mention.TargetAccountID, err) + } + + if follow { + // Confirmed conversation. + return true, nil + } + } + + return false, nil +} diff --git a/internal/visibility/public_timeline.go b/internal/visibility/public_timeline.go index 13ac07831..77ce5760c 100644 --- a/internal/visibility/public_timeline.go +++ b/internal/visibility/public_timeline.go @@ -30,6 +30,8 @@ // StatusHomeTimelineable checks if given status should be included on requester's public timeline. Primarily relying on status visibility to requester and the AP visibility setting, and ignoring conversation threads. func (f *Filter) StatusPublicTimelineable(ctx context.Context, requester *gtsmodel.Account, status *gtsmodel.Status) (bool, error) { + const vtype = cache.VisibilityTypePublic + // By default we assume no auth. requesterID := noauth @@ -49,10 +51,10 @@ func (f *Filter) StatusPublicTimelineable(ctx context.Context, requester *gtsmod return &cache.CachedVisibility{ ItemID: status.ID, RequesterID: requesterID, - Type: cache.VisibilityTypePublic, + Type: vtype, Value: visible, }, nil - }, "public", requesterID, status.ID) + }, vtype, requesterID, status.ID) if err != nil { if err == cache.SentinelError { // Filter-out our temporary @@ -103,7 +105,7 @@ func (f *Filter) isStatusPublicTimelineable(ctx context.Context, requester *gtsm parentID, ) if err != nil { - return false, fmt.Errorf("isStatusHomeTimelineable: error getting status parent %s: %w", parentID, err) + return false, fmt.Errorf("isStatusPublicTimelineable: error getting status parent %s: %w", parentID, err) } if parent.AccountID != status.AccountID { diff --git a/internal/visibility/status.go b/internal/visibility/status.go index dc8261624..d41bbc80b 100644 --- a/internal/visibility/status.go +++ b/internal/visibility/status.go @@ -49,6 +49,8 @@ func (f *Filter) StatusesVisible(ctx context.Context, requester *gtsmodel.Accoun // StatusVisible will check if given status is visible to requester, accounting for requester with no auth (i.e is nil), suspensions, disabled local users, account blocks and status privacy. func (f *Filter) StatusVisible(ctx context.Context, requester *gtsmodel.Account, status *gtsmodel.Status) (bool, error) { + const vtype = cache.VisibilityTypeStatus + // By default we assume no auth. requesterID := noauth @@ -68,10 +70,10 @@ func (f *Filter) StatusVisible(ctx context.Context, requester *gtsmodel.Account, return &cache.CachedVisibility{ ItemID: status.ID, RequesterID: requesterID, - Type: cache.VisibilityTypeStatus, + Type: vtype, Value: visible, }, nil - }, "status", requesterID, status.ID) + }, vtype, requesterID, status.ID) if err != nil { return false, err } @@ -83,7 +85,7 @@ func (f *Filter) StatusVisible(ctx context.Context, requester *gtsmodel.Account, func (f *Filter) isStatusVisible(ctx context.Context, requester *gtsmodel.Account, status *gtsmodel.Status) (bool, error) { // Ensure that status is fully populated for further processing. if err := f.state.DB.PopulateStatus(ctx, status); err != nil { - return false, err + return false, fmt.Errorf("isStatusVisible: error populating status %s: %w", status.ID, err) } // Check whether status accounts are visible to the requester. @@ -185,7 +187,7 @@ func (f *Filter) areStatusAccountsVisible(ctx context.Context, requester *gtsmod // Check whether status author's account is visible to requester. visible, err := f.AccountVisible(ctx, requester, status.Account) if err != nil { - return false, err + return false, fmt.Errorf("error checking status author visibility: %w", err) } if !visible { @@ -204,7 +206,7 @@ func (f *Filter) areStatusAccountsVisible(ctx context.Context, requester *gtsmod // Check whether boosted status author's account is visible to requester. visible, err := f.AccountVisible(ctx, requester, status.BoostOfAccount) if err != nil { - return false, err + return false, fmt.Errorf("error checking boosted author visibility: %w", err) } if !visible {