From 4d9acadd7223d2157c269f8881645c4c484d7e94 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Thu, 13 Oct 2022 10:55:31 +0100 Subject: [PATCH] Review comments --- syncapi/routing/routing.go | 8 +-- syncapi/storage/postgres/relations_table.go | 77 +++++++------------- syncapi/storage/shared/storage_consumer.go | 3 +- syncapi/storage/shared/storage_sync.go | 33 +++++---- syncapi/storage/sqlite3/relations_table.go | 78 ++++++++------------- syncapi/storage/tables/interface.go | 2 +- 6 files changed, 81 insertions(+), 120 deletions(-) diff --git a/syncapi/routing/routing.go b/syncapi/routing/routing.go index 115925942..145fcbd7c 100644 --- a/syncapi/routing/routing.go +++ b/syncapi/routing/routing.go @@ -45,7 +45,7 @@ func Setup( lazyLoadCache caching.LazyLoadCache, fts *fulltext.Search, ) { - v1mux := csMux.PathPrefix("/v1/").Subrouter() + v1unstablemux := csMux.PathPrefix("/{apiversion:(?:v1|unstable)}/").Subrouter() v3mux := csMux.PathPrefix("/{apiversion:(?:r0|v3)}/").Subrouter() // TODO: Add AS support for all handlers below. @@ -111,7 +111,7 @@ func Setup( }), ).Methods(http.MethodGet, http.MethodOptions) - v1mux.Handle("/rooms/{roomId}/relations/{eventId}", + v1unstablemux.Handle("/rooms/{roomId}/relations/{eventId}", httputil.MakeAuthAPI(gomatrixserverlib.Join, userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse { vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) if err != nil { @@ -125,7 +125,7 @@ func Setup( }), ).Methods(http.MethodGet, http.MethodOptions) - v1mux.Handle("/rooms/{roomId}/relations/{eventId}/{relType}", + v1unstablemux.Handle("/rooms/{roomId}/relations/{eventId}/{relType}", httputil.MakeAuthAPI(gomatrixserverlib.Join, userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse { vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) if err != nil { @@ -139,7 +139,7 @@ func Setup( }), ).Methods(http.MethodGet, http.MethodOptions) - v1mux.Handle("/rooms/{roomId}/relations/{eventId}/{relType}/{eventType}", + v1unstablemux.Handle("/rooms/{roomId}/relations/{eventId}/{relType}/{eventType}", httputil.MakeAuthAPI(gomatrixserverlib.Join, userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse { vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) if err != nil { diff --git a/syncapi/storage/postgres/relations_table.go b/syncapi/storage/postgres/relations_table.go index 830158921..aead4bb46 100644 --- a/syncapi/storage/postgres/relations_table.go +++ b/syncapi/storage/postgres/relations_table.go @@ -1,5 +1,4 @@ -// Copyright 2017-2018 New Vector Ltd -// Copyright 2019-2020 The Matrix.org Foundation C.I.C. +// Copyright 2022 The Matrix.org Foundation C.I.C. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -42,34 +41,33 @@ const insertRelationSQL = "" + "INSERT INTO syncapi_relations (" + " room_id, event_id, child_event_id, rel_type" + ") VALUES ($1, $2, $3, $4) " + - " ON CONFLICT ON CONSTRAINT syncapi_relations_unique DO UPDATE SET event_id=EXCLUDED.event_id" + - " RETURNING id" + " ON CONFLICT DO NOTHING" const deleteRelationSQL = "" + "DELETE FROM syncapi_relations WHERE room_id = $1 AND child_event_id = $2" const selectRelationsInRangeAscSQL = "" + - "SELECT id, room_id, child_event_id, rel_type FROM syncapi_relations" + + "SELECT id, child_event_id, rel_type FROM syncapi_relations" + " WHERE room_id = $1 AND event_id = $2 AND id > $3 AND id <= $4" + " ORDER BY id ASC LIMIT $5" const selectRelationsInRangeDescSQL = "" + - "SELECT id, room_id, child_event_id, rel_type FROM syncapi_relations" + + "SELECT id, child_event_id, rel_type FROM syncapi_relations" + " WHERE room_id = $1 AND event_id = $2 AND id >= $3 AND id < $4" + " ORDER BY id DESC LIMIT $5" const selectRelationsByTypeInRangeAscSQL = "" + - "SELECT id, room_id, child_event_id, rel_type FROM syncapi_relations" + + "SELECT id, child_event_id, rel_type FROM syncapi_relations" + " WHERE room_id = $1 AND event_id = $2 AND rel_type = $3 AND id > $4 AND id <= $5" + " ORDER BY id ASC LIMIT $6" const selectRelationsByTypeInRangeDescSQL = "" + - "SELECT id, room_id, child_event_id, rel_type FROM syncapi_relations" + + "SELECT id, child_event_id, rel_type FROM syncapi_relations" + " WHERE room_id = $1 AND event_id = $2 AND rel_type = $3 AND id >= $4 AND id < $5" + " ORDER BY id DESC LIMIT $6" const selectMaxRelationIDSQL = "" + - "SELECT MAX(id) FROM syncapi_relations" + "SELECT COALESCE(MAX(id), 0) FROM syncapi_relations" type relationsStatements struct { insertRelationStmt *sql.Stmt @@ -87,36 +85,23 @@ func NewPostgresRelationsTable(db *sql.DB) (tables.Relations, error) { if err != nil { return nil, err } - if s.insertRelationStmt, err = db.Prepare(insertRelationSQL); err != nil { - return nil, err - } - if s.selectRelationsInRangeAscStmt, err = db.Prepare(selectRelationsInRangeAscSQL); err != nil { - return nil, err - } - if s.selectRelationsInRangeDescStmt, err = db.Prepare(selectRelationsInRangeDescSQL); err != nil { - return nil, err - } - if s.selectRelationsByTypeInRangeAscStmt, err = db.Prepare(selectRelationsByTypeInRangeAscSQL); err != nil { - return nil, err - } - if s.selectRelationsByTypeInRangeDescStmt, err = db.Prepare(selectRelationsByTypeInRangeDescSQL); err != nil { - return nil, err - } - if s.deleteRelationStmt, err = db.Prepare(deleteRelationSQL); err != nil { - return nil, err - } - if s.selectMaxRelationIDStmt, err = db.Prepare(selectMaxRelationIDSQL); err != nil { - return nil, err - } - return s, nil + return s, sqlutil.StatementList{ + {&s.insertRelationStmt, insertRelationSQL}, + {&s.selectRelationsInRangeAscStmt, selectRelationsInRangeAscSQL}, + {&s.selectRelationsInRangeDescStmt, selectRelationsInRangeDescSQL}, + {&s.selectRelationsByTypeInRangeAscStmt, selectRelationsByTypeInRangeAscSQL}, + {&s.selectRelationsByTypeInRangeDescStmt, selectRelationsByTypeInRangeDescSQL}, + {&s.deleteRelationStmt, deleteRelationSQL}, + {&s.selectMaxRelationIDStmt, selectMaxRelationIDSQL}, + }.Prepare(db) } func (s *relationsStatements) InsertRelation( ctx context.Context, txn *sql.Tx, roomID, eventID, childEventID, relType string, -) (streamPos types.StreamPosition, err error) { - err = sqlutil.TxStmt(txn, s.insertRelationStmt).QueryRowContext( +) (err error) { + _, err = sqlutil.TxStmt(txn, s.insertRelationStmt).ExecContext( ctx, roomID, eventID, childEventID, relType, - ).Scan(&streamPos) + ) return } @@ -127,9 +112,6 @@ func (s *relationsStatements) DeleteRelation( _, err := stmt.ExecContext( ctx, roomID, childEventID, ) - if err == sql.ErrNoRows { - return nil - } return err } @@ -162,20 +144,19 @@ func (s *relationsStatements) SelectRelationsInRange( } defer internal.CloseAndLogIfError(ctx, rows, "selectRelationsInRange: rows.close() failed") result := map[string][]types.RelationEntry{} + var ( + id types.StreamPosition + childEventID string + relationType string + ) for rows.Next() { - var ( - id types.StreamPosition - roomID string - childEventID string - relType string - ) - if err = rows.Scan(&id, &roomID, &childEventID, &relType); err != nil { + if err = rows.Scan(&id, &childEventID, &relationType); err != nil { return nil, lastPos, err } if id > lastPos { lastPos = id } - result[relType] = append(result[relType], types.RelationEntry{ + result[relationType] = append(result[relationType], types.RelationEntry{ Position: id, EventID: childEventID, }) @@ -189,11 +170,7 @@ func (s *relationsStatements) SelectRelationsInRange( func (s *relationsStatements) SelectMaxRelationID( ctx context.Context, txn *sql.Tx, ) (id int64, err error) { - var nullableID sql.NullInt64 stmt := sqlutil.TxStmt(txn, s.selectMaxRelationIDStmt) - err = stmt.QueryRowContext(ctx).Scan(&nullableID) - if nullableID.Valid { - id = nullableID.Int64 - } + err = stmt.QueryRowContext(ctx).Scan(&id) return } diff --git a/syncapi/storage/shared/storage_consumer.go b/syncapi/storage/shared/storage_consumer.go index 8292e81f7..0a83b18e0 100644 --- a/syncapi/storage/shared/storage_consumer.go +++ b/syncapi/storage/shared/storage_consumer.go @@ -604,11 +604,10 @@ func (d *Database) UpdateRelations(ctx context.Context, event *gomatrixserverlib return nil default: return d.Writer.Do(d.DB, nil, func(txn *sql.Tx) error { - _, err := d.Relations.InsertRelation( + return d.Relations.InsertRelation( ctx, txn, event.RoomID(), content.Relations.EventID, event.EventID(), content.Relations.RelationType, ) - return err }) } } diff --git a/syncapi/storage/shared/storage_sync.go b/syncapi/storage/shared/storage_sync.go index 4a9d7143d..0f329a334 100644 --- a/syncapi/storage/shared/storage_sync.go +++ b/syncapi/storage/shared/storage_sync.go @@ -605,18 +605,23 @@ func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID, To: to, Backwards: backwards, } - if r.Backwards { - if r.From == 0 { - if r.From, err = d.MaxStreamPositionForRelations(ctx); err != nil { - return nil, "", "", fmt.Errorf("d.MaxStreamPositionForRelations: %w", err) - } - r.From++ + + if r.Backwards && r.From == 0 { + // If we're working backwards (dir=b) and there's no ?from= specified then + // we will automatically want to work backwards from the current position, + // so find out what that is. + if r.From, err = d.MaxStreamPositionForRelations(ctx); err != nil { + return nil, "", "", fmt.Errorf("d.MaxStreamPositionForRelations: %w", err) } - } else { - if r.To == 0 { - if r.To, err = d.MaxStreamPositionForRelations(ctx); err != nil { - return nil, "", "", fmt.Errorf("d.MaxStreamPositionForRelations: %w", err) - } + // The result normally isn't inclusive of the event *at* the ?from= + // position, so add 1 here so that we include the most recent relation. + r.From++ + } else if !r.Backwards && r.To == 0 { + // If we're working forwards (dir=f) and there's no ?to= specified then + // we will automatically want to work forwards towards the current position, + // so find out what that is. + if r.To, err = d.MaxStreamPositionForRelations(ctx); err != nil { + return nil, "", "", fmt.Errorf("d.MaxStreamPositionForRelations: %w", err) } } @@ -645,7 +650,9 @@ func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID, } // Otherwise, let's try and work out what sensible prev_batch and next_batch values - // could be. + // could be. We've requested an extra event by adding one to the limit already so + // that we can determine whether or not to provide a "next_batch", so trim off that + // event off the end if needs be. if len(entries) > limit { entries = entries[:len(entries)-1] nextBatch = fmt.Sprintf("%d", entries[len(entries)-1].Position) @@ -653,7 +660,7 @@ func (d *DatabaseTransaction) RelationsFor(ctx context.Context, roomID, eventID, // TODO: set prevBatch? doesn't seem to affect the tests... // Extract all of the event IDs from the relation entries so that we can pull the - // events out of the database. + // events out of the database. Then go and fetch the events. for _, entry := range entries { eventIDs = append(eventIDs, entry.EventID) } diff --git a/syncapi/storage/sqlite3/relations_table.go b/syncapi/storage/sqlite3/relations_table.go index 349e5171a..6791d94da 100644 --- a/syncapi/storage/sqlite3/relations_table.go +++ b/syncapi/storage/sqlite3/relations_table.go @@ -1,5 +1,4 @@ -// Copyright 2017-2018 New Vector Ltd -// Copyright 2019-2020 The Matrix.org Foundation C.I.C. +// Copyright 2022 The Matrix.org Foundation C.I.C. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -40,34 +39,33 @@ const insertRelationSQL = "" + "INSERT INTO syncapi_relations (" + " id, room_id, event_id, child_event_id, rel_type" + ") VALUES ($1, $2, $3, $4, $5) " + - " ON CONFLICT (room_id, event_id, child_event_id, rel_type) DO UPDATE SET event_id=EXCLUDED.event_id" + - " RETURNING id" + " ON CONFLICT DO NOTHING" const deleteRelationSQL = "" + "DELETE FROM syncapi_relations WHERE room_id = $1 AND child_event_id = $2" const selectRelationsInRangeAscSQL = "" + - "SELECT id, room_id, child_event_id, rel_type FROM syncapi_relations" + + "SELECT id, child_event_id, rel_type FROM syncapi_relations" + " WHERE room_id = $1 AND event_id = $2 AND id > $3 AND id <= $4" + " ORDER BY id ASC LIMIT $5" const selectRelationsInRangeDescSQL = "" + - "SELECT id, room_id, child_event_id, rel_type FROM syncapi_relations" + + "SELECT id, child_event_id, rel_type FROM syncapi_relations" + " WHERE room_id = $1 AND event_id = $2 AND id >= $3 AND id < $4" + " ORDER BY id DESC LIMIT $5" const selectRelationsByTypeInRangeAscSQL = "" + - "SELECT id, room_id, child_event_id, rel_type FROM syncapi_relations" + + "SELECT id, child_event_id, rel_type FROM syncapi_relations" + " WHERE room_id = $1 AND event_id = $2 AND rel_type = $3 AND id > $4 AND id <= $5" + " ORDER BY id ASC LIMIT $6" const selectRelationsByTypeInRangeDescSQL = "" + - "SELECT id, room_id, child_event_id, rel_type FROM syncapi_relations" + + "SELECT id, child_event_id, rel_type FROM syncapi_relations" + " WHERE room_id = $1 AND event_id = $2 AND rel_type = $3 AND id >= $4 AND id < $5" + " ORDER BY id DESC LIMIT $6" const selectMaxRelationIDSQL = "" + - "SELECT MAX(id) FROM syncapi_relations" + "SELECT COALESCE(MAX(id), 0) FROM syncapi_relations" type relationsStatements struct { streamIDStatements *StreamIDStatements @@ -88,39 +86,27 @@ func NewSqliteRelationsTable(db *sql.DB, streamID *StreamIDStatements) (tables.R if err != nil { return nil, err } - if s.insertRelationStmt, err = db.Prepare(insertRelationSQL); err != nil { - return nil, err - } - if s.selectRelationsInRangeAscStmt, err = db.Prepare(selectRelationsInRangeAscSQL); err != nil { - return nil, err - } - if s.selectRelationsInRangeDescStmt, err = db.Prepare(selectRelationsInRangeDescSQL); err != nil { - return nil, err - } - if s.selectRelationsByTypeInRangeAscStmt, err = db.Prepare(selectRelationsByTypeInRangeAscSQL); err != nil { - return nil, err - } - if s.selectRelationsByTypeInRangeDescStmt, err = db.Prepare(selectRelationsByTypeInRangeDescSQL); err != nil { - return nil, err - } - if s.deleteRelationStmt, err = db.Prepare(deleteRelationSQL); err != nil { - return nil, err - } - if s.selectMaxRelationIDStmt, err = db.Prepare(selectMaxRelationIDSQL); err != nil { - return nil, err - } - return s, nil + return s, sqlutil.StatementList{ + {&s.insertRelationStmt, insertRelationSQL}, + {&s.selectRelationsInRangeAscStmt, selectRelationsInRangeAscSQL}, + {&s.selectRelationsInRangeDescStmt, selectRelationsInRangeDescSQL}, + {&s.selectRelationsByTypeInRangeAscStmt, selectRelationsByTypeInRangeAscSQL}, + {&s.selectRelationsByTypeInRangeDescStmt, selectRelationsByTypeInRangeDescSQL}, + {&s.deleteRelationStmt, deleteRelationSQL}, + {&s.selectMaxRelationIDStmt, selectMaxRelationIDSQL}, + }.Prepare(db) } func (s *relationsStatements) InsertRelation( ctx context.Context, txn *sql.Tx, roomID, eventID, childEventID, relType string, -) (streamPos types.StreamPosition, err error) { +) (err error) { + var streamPos types.StreamPosition if streamPos, err = s.streamIDStatements.nextRelationID(ctx, txn); err != nil { return } - err = sqlutil.TxStmt(txn, s.insertRelationStmt).QueryRowContext( + _, err = sqlutil.TxStmt(txn, s.insertRelationStmt).ExecContext( ctx, streamPos, roomID, eventID, childEventID, relType, - ).Scan(&streamPos) + ) return } @@ -131,9 +117,6 @@ func (s *relationsStatements) DeleteRelation( _, err := stmt.ExecContext( ctx, roomID, childEventID, ) - if err == sql.ErrNoRows { - return nil - } return err } @@ -166,20 +149,19 @@ func (s *relationsStatements) SelectRelationsInRange( } defer internal.CloseAndLogIfError(ctx, rows, "selectRelationsInRange: rows.close() failed") result := map[string][]types.RelationEntry{} + var ( + id types.StreamPosition + childEventID string + relationType string + ) for rows.Next() { - var ( - id types.StreamPosition - roomID string - childEventID string - relType string - ) - if err = rows.Scan(&id, &roomID, &childEventID, &relType); err != nil { + if err = rows.Scan(&id, &childEventID, &relationType); err != nil { return nil, lastPos, err } if id > lastPos { lastPos = id } - result[relType] = append(result[relType], types.RelationEntry{ + result[relationType] = append(result[relationType], types.RelationEntry{ Position: id, EventID: childEventID, }) @@ -193,11 +175,7 @@ func (s *relationsStatements) SelectRelationsInRange( func (s *relationsStatements) SelectMaxRelationID( ctx context.Context, txn *sql.Tx, ) (id int64, err error) { - var nullableID sql.NullInt64 stmt := sqlutil.TxStmt(txn, s.selectMaxRelationIDStmt) - err = stmt.QueryRowContext(ctx).Scan(&nullableID) - if nullableID.Valid { - id = nullableID.Int64 - } + err = stmt.QueryRowContext(ctx).Scan(&id) return } diff --git a/syncapi/storage/tables/interface.go b/syncapi/storage/tables/interface.go index 51de0965b..4e29a217b 100644 --- a/syncapi/storage/tables/interface.go +++ b/syncapi/storage/tables/interface.go @@ -208,7 +208,7 @@ type Presence interface { } type Relations interface { - InsertRelation(ctx context.Context, txn *sql.Tx, roomID, eventID, childEventID, relType string) (streamPos types.StreamPosition, err error) + InsertRelation(ctx context.Context, txn *sql.Tx, roomID, eventID, childEventID, relType string) (err error) DeleteRelation(ctx context.Context, txn *sql.Tx, roomID, childEventID string) error SelectRelationsInRange(ctx context.Context, txn *sql.Tx, roomID, eventID, relType string, r types.Range, limit int) (map[string][]types.RelationEntry, types.StreamPosition, error) SelectMaxRelationID(ctx context.Context, txn *sql.Tx) (id int64, err error)