[bugfix] visibility caching and hometimeline (#1675)

* fix visibility caching to use correct type key Signed-off-by: kim <[email protected]> * check for ID check > max possible ID Signed-off-by: kim <[email protected]> * update home timeline code to include relevant threads to owner (e.g. between mutuals/follows) Signed-off-by: kim <[email protected]> --------- Signed-off-by: kim <[email protected]>
superseriousbusiness · Apr 6, 2023 · e46323c · e46323c
1 parent 3510454
commit e46323c
Show file tree

Hide file tree

Showing 5 changed files with 110 additions and 26 deletions.
diff --git 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
@@ -29,6 +29,8 @@ import (
 
 // 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
@@ -30,6 +30,8 @@ import (
 
 // 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,50 +97,72 @@ 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,
 		)
 		if err != nil {
 			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
@@ -30,6 +30,8 @@ import (
 
 // 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
@@ -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 {