Fix room summary returning wrong heroes (#2930)

This should fix #2910.
Probably makes Sytest/Complement a bit upset, since this not using
`sort.Strings` anymore.

syncapi/storage/sqlite3/current_room_state_table.go

@@ -19,6 +19,7 @@ import (
 	"context"
 	"database/sql"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -95,6 +96,15 @@ const selectSharedUsersSQL = "" +
 	"	SELECT DISTINCT room_id FROM syncapi_current_room_state WHERE state_key = $1 AND membership='join'" +
 	") AND type = 'm.room.member' AND state_key IN ($2) AND membership IN ('join', 'invite');"
 
+const selectMembershipCount = `SELECT count(*) FROM syncapi_current_room_state WHERE type = 'm.room.member' AND room_id = $1 AND membership = $2`
+
+const selectRoomHeroes = `
+SELECT state_key FROM syncapi_current_room_state
+WHERE type = 'm.room.member' AND room_id = $1 AND state_key != $2 AND membership IN ($3)
+ORDER BY added_at, state_key
+LIMIT 5
+`
+
 type currentRoomStateStatements struct {
 	db                                 *sql.DB
 	streamIDStatements                 *StreamIDStatements
@@ -107,6 +117,8 @@ type currentRoomStateStatements struct {
 	//selectJoinedUsersInRoomStmt      *sql.Stmt - prepared at runtime due to variadic
 	selectStateEventStmt *sql.Stmt
 	//selectSharedUsersSQL             *sql.Stmt - prepared at runtime due to variadic
+	selectMembershipCountStmt *sql.Stmt
+	//selectRoomHeroes          *sql.Stmt - prepared at runtime due to variadic
 }
 
 func NewSqliteCurrentRoomStateTable(db *sql.DB, streamID *StreamIDStatements) (tables.CurrentRoomState, error) {
@@ -129,31 +141,16 @@ func NewSqliteCurrentRoomStateTable(db *sql.DB, streamID *StreamIDStatements) (t
 		return nil, err
 	}
 
-	if s.upsertRoomStateStmt, err = db.Prepare(upsertRoomStateSQL); err != nil {
-		return nil, err
-	}
-	if s.deleteRoomStateByEventIDStmt, err = db.Prepare(deleteRoomStateByEventIDSQL); err != nil {
-		return nil, err
-	}
-	if s.deleteRoomStateForRoomStmt, err = db.Prepare(deleteRoomStateForRoomSQL); err != nil {
-		return nil, err
-	}
-	if s.selectRoomIDsWithMembershipStmt, err = db.Prepare(selectRoomIDsWithMembershipSQL); err != nil {
-		return nil, err
-	}
-	if s.selectRoomIDsWithAnyMembershipStmt, err = db.Prepare(selectRoomIDsWithAnyMembershipSQL); err != nil {
-		return nil, err
-	}
-	if s.selectJoinedUsersStmt, err = db.Prepare(selectJoinedUsersSQL); err != nil {
-		return nil, err
-	}
-	//if s.selectJoinedUsersInRoomStmt, err = db.Prepare(selectJoinedUsersInRoomSQL); err != nil {
-	//	return nil, err
-	//}
-	if s.selectStateEventStmt, err = db.Prepare(selectStateEventSQL); err != nil {
-		return nil, err
-	}
-	return s, nil
+	return s, sqlutil.StatementList{
+		{&s.upsertRoomStateStmt, upsertRoomStateSQL},
+		{&s.deleteRoomStateByEventIDStmt, deleteRoomStateByEventIDSQL},
+		{&s.deleteRoomStateForRoomStmt, deleteRoomStateForRoomSQL},
+		{&s.selectRoomIDsWithMembershipStmt, selectRoomIDsWithMembershipSQL},
+		{&s.selectRoomIDsWithAnyMembershipStmt, selectRoomIDsWithAnyMembershipSQL},
+		{&s.selectJoinedUsersStmt, selectJoinedUsersSQL},
+		{&s.selectStateEventStmt, selectStateEventSQL},
+		{&s.selectMembershipCountStmt, selectMembershipCount},
+	}.Prepare(db)
 }
 
 // SelectJoinedUsers returns a map of room ID to a list of joined user IDs.
@@ -485,3 +482,53 @@ func (s *currentRoomStateStatements) SelectSharedUsers(
 
 	return result, err
 }
+
+func (s *currentRoomStateStatements) SelectRoomHeroes(ctx context.Context, txn *sql.Tx, roomID, excludeUserID string, memberships []string) ([]string, error) {
+	params := make([]interface{}, len(memberships)+2)
+	params[0] = roomID
+	params[1] = excludeUserID
+	for k, v := range memberships {
+		params[k+2] = v
+	}
+
+	query := strings.Replace(selectRoomHeroes, "($3)", sqlutil.QueryVariadicOffset(len(memberships), 2), 1)
+	var stmt *sql.Stmt
+	var err error
+	if txn != nil {
+		stmt, err = txn.Prepare(query)
+	} else {
+		stmt, err = s.db.Prepare(query)
+	}
+	if err != nil {
+		return []string{}, err
+	}
+	defer internal.CloseAndLogIfError(ctx, stmt, "selectRoomHeroes: stmt.close() failed")
+
+	rows, err := stmt.QueryContext(ctx, params...)
+	if err != nil {
+		return nil, err
+	}
+	defer internal.CloseAndLogIfError(ctx, rows, "selectRoomHeroes: rows.close() failed")
+
+	var stateKey string
+	result := make([]string, 0, 5)
+	for rows.Next() {
+		if err = rows.Scan(&stateKey); err != nil {
+			return nil, err
+		}
+		result = append(result, stateKey)
+	}
+	return result, rows.Err()
+}
+
+func (s *currentRoomStateStatements) SelectMembershipCount(ctx context.Context, txn *sql.Tx, roomID, membership string) (count int, err error) {
+	stmt := sqlutil.TxStmt(txn, s.selectMembershipCountStmt)
+	err = stmt.QueryRowContext(ctx, roomID, membership).Scan(&count)
+	if err != nil {
+		if errors.Is(err, sql.ErrNoRows) {
+			return 0, nil
+		}
+		return 0, err
+	}
+	return count, nil
+}

syncapi/storage/sqlite3/memberships_table.go

@@ -18,11 +18,9 @@ import (
 	"context"
 	"database/sql"
 	"fmt"
-	"strings"
 
 	"github.com/matrix-org/gomatrixserverlib"
 
-	"github.com/matrix-org/dendrite/internal"
 	"github.com/matrix-org/dendrite/internal/sqlutil"
 	"github.com/matrix-org/dendrite/syncapi/storage/tables"
 	"github.com/matrix-org/dendrite/syncapi/types"
@@ -64,9 +62,6 @@ const selectMembershipCountSQL = "" +
 	" SELECT * FROM syncapi_memberships WHERE room_id = $1 AND stream_pos <= $2 GROUP BY user_id HAVING(max(stream_pos))" +
 	") t WHERE t.membership = $3"
 
-const selectHeroesSQL = "" +
-	"SELECT DISTINCT user_id FROM syncapi_memberships WHERE room_id = $1 AND user_id != $2 AND membership IN ($3) LIMIT 5"
-
 const selectMembershipBeforeSQL = "" +
 	"SELECT membership, topological_pos FROM syncapi_memberships WHERE room_id = $1 and user_id = $2 AND topological_pos <= $3 ORDER BY topological_pos DESC LIMIT 1"
 
@@ -99,7 +94,6 @@ func NewSqliteMembershipsTable(db *sql.DB) (tables.Memberships, error) {
 		{&s.selectMembershipCountStmt, selectMembershipCountSQL},
 		{&s.selectMembershipForUserStmt, selectMembershipBeforeSQL},
 		{&s.selectMembersStmt, selectMembersSQL},
-		// {&s.selectHeroesStmt, selectHeroesSQL}, - prepared at runtime due to variadic
 	}.Prepare(db)
 }
 
@@ -131,39 +125,6 @@ func (s *membershipsStatements) SelectMembershipCount(
 	return
 }
 
-func (s *membershipsStatements) SelectHeroes(
-	ctx context.Context, txn *sql.Tx, roomID, userID string, memberships []string,
-) (heroes []string, err error) {
-	stmtSQL := strings.Replace(selectHeroesSQL, "($3)", sqlutil.QueryVariadicOffset(len(memberships), 2), 1)
-	stmt, err := s.db.PrepareContext(ctx, stmtSQL)
-	if err != nil {
-		return
-	}
-	defer internal.CloseAndLogIfError(ctx, stmt, "SelectHeroes: stmt.close() failed")
-	params := []interface{}{
-		roomID, userID,
-	}
-	for _, membership := range memberships {
-		params = append(params, membership)
-	}
-
-	stmt = sqlutil.TxStmt(txn, stmt)
-	var rows *sql.Rows
-	rows, err = stmt.QueryContext(ctx, params...)
-	if err != nil {
-		return
-	}
-	defer internal.CloseAndLogIfError(ctx, rows, "SelectHeroes: rows.close() failed")
-	var hero string
-	for rows.Next() {
-		if err = rows.Scan(&hero); err != nil {
-			return
-		}
-		heroes = append(heroes, hero)
-	}
-	return heroes, rows.Err()
-}
-
 // SelectMembershipForUser returns the membership of the user before and including the given position. If no membership can be found
 // returns "leave", the topological position and no error. If an error occurs, other than sql.ErrNoRows, returns that and an empty
 // string as the membership.