More review comments

syncapi/routing/relations.go

@@ -20,6 +20,7 @@ import (
 
 	"github.com/matrix-org/gomatrixserverlib"
 	"github.com/matrix-org/util"
+	"github.com/sirupsen/logrus"
 
 	"github.com/matrix-org/dendrite/clientapi/jsonerror"
 	"github.com/matrix-org/dendrite/internal/sqlutil"
@@ -70,6 +71,7 @@ func Relations(req *http.Request, device *api.Device, syncDB storage.Database, r
 
 	snapshot, err := syncDB.NewDatabaseSnapshot(req.Context())
 	if err != nil {
+		logrus.WithError(err).Error("Failed to get snapshot for relations")
 		return jsonerror.InternalServerError()
 	}
 	var succeeded bool

syncapi/storage/shared/storage_sync.go

@@ -598,8 +598,6 @@ func (d *DatabaseTransaction) MaxStreamPositionForRelations(ctx context.Context)
 func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID, relType, eventType string, from, to types.StreamPosition, backwards bool, limit int) (
 	clientEvents []gomatrixserverlib.ClientEvent, prevBatch, nextBatch string, err error,
 ) {
-	clientEvents = []gomatrixserverlib.ClientEvent{}
-	eventIDs := []string{}
 	r := types.Range{
 		From:      from,
 		To:        to,
@@ -625,7 +623,7 @@ func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID,
 		}
 	}
 
-	// First up look up any relations from the database. We add one to the limit here
+	// First look up any relations from the database. We add one to the limit here
 	// so that we can tell if we're overflowing, as we will only set the "next_batch"
 	// in the response if we are.
 	relations, _, err := d.Relations.SelectRelationsInRange(ctx, d.txn, roomID, eventID, relType, r, limit+1)
@@ -646,7 +644,7 @@ func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID,
 
 	// If there were no entries returned, there were no relations, so stop at this point.
 	if len(entries) == 0 {
-		return clientEvents, "", "", nil
+		return []gomatrixserverlib.ClientEvent{}, "", "", nil
 	}
 
 	// Otherwise, let's try and work out what sensible prev_batch and next_batch values
@@ -661,6 +659,7 @@ func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID,
 
 	// Extract all of the event IDs from the relation entries so that we can pull the
 	// events out of the database. Then go and fetch the events.
+	eventIDs := make([]string, 0, len(entries))
 	for _, entry := range entries {
 		eventIDs = append(eventIDs, entry.EventID)
 	}
@@ -671,6 +670,7 @@ func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID,
 
 	// Convert the events into client events, and optionally filter based on the event
 	// type if it was specified.
+	clientEvents = make([]gomatrixserverlib.ClientEvent, 0, len(events))
 	for _, event := range events {
 		if eventType != "" && event.Type() != eventType {
 			continue

syncapi/storage/tables/interface.go

@@ -208,8 +208,19 @@ type Presence interface {
 }
 
 type Relations interface {
+	// Inserts a relation which refers from the child event ID to the event ID in the given room.
+	// If the relation already exists then this function will do nothing and return no error.
 	InsertRelation(ctx context.Context, txn *sql.Tx, roomID, eventID, childEventID, relType string) (err error)
+	// Deletes a relation which already exists as the result of an event redaction. If the relation
+	// does not exist then this function will do nothing and return no error.
 	DeleteRelation(ctx context.Context, txn *sql.Tx, roomID, childEventID string) error
+	// SelectRelationsInRange will return relations grouped by relation type within the given range.
+	// The map is relType -> []entry. If a relType parameter is specified then the results will only
+	// contain relations of that type, otherwise if "" is specified then all relations in the range
+	// will be returned. The stream position returned is the maximum position of the returned results.
 	SelectRelationsInRange(ctx context.Context, txn *sql.Tx, roomID, eventID, relType string, r types.Range, limit int) (map[string][]types.RelationEntry, types.StreamPosition, error)
+	// SelectMaxRelationID returns the maximum ID of all relations, used to determine what the boundaries
+	// should be if there are no boundaries supplied (i.e. we want to work backwards but don't have a
+	// "from" or want to work forwards and don't have a "to").
 	SelectMaxRelationID(ctx context.Context, txn *sql.Tx) (id int64, err error)
 }