diff options
| -rw-r--r-- | syncapi/storage/postgres/output_room_events_topology_table.go | 8 | ||||
| -rw-r--r-- | syncapi/storage/sqlite3/output_room_events_topology_table.go | 8 | ||||
| -rw-r--r-- | syncapi/storage/storage_test.go | 49 |
3 files changed, 57 insertions, 8 deletions
diff --git a/syncapi/storage/postgres/output_room_events_topology_table.go b/syncapi/storage/postgres/output_room_events_topology_table.go index 8a9cc49c..9fc5f519 100644 --- a/syncapi/storage/postgres/output_room_events_topology_table.go +++ b/syncapi/storage/postgres/output_room_events_topology_table.go @@ -48,17 +48,17 @@ const insertEventInTopologySQL = "" + const selectEventIDsInRangeASCSQL = "" + "SELECT event_id FROM syncapi_output_room_events_topology" + - " WHERE room_id = $1 AND" + + " WHERE room_id = $1 AND (" + "(topological_position > $2 AND topological_position < $3) OR" + "(topological_position = $4 AND stream_position <= $5)" + - " ORDER BY topological_position ASC, stream_position ASC LIMIT $6" + ") ORDER BY topological_position ASC, stream_position ASC LIMIT $6" const selectEventIDsInRangeDESCSQL = "" + "SELECT event_id FROM syncapi_output_room_events_topology" + - " WHERE room_id = $1 AND" + + " WHERE room_id = $1 AND (" + "(topological_position > $2 AND topological_position < $3) OR" + "(topological_position = $4 AND stream_position <= $5)" + - " ORDER BY topological_position DESC, stream_position DESC LIMIT $6" + ") ORDER BY topological_position DESC, stream_position DESC LIMIT $6" const selectPositionInTopologySQL = "" + "SELECT topological_position, stream_position FROM syncapi_output_room_events_topology" + diff --git a/syncapi/storage/sqlite3/output_room_events_topology_table.go b/syncapi/storage/sqlite3/output_room_events_topology_table.go index f25ac623..22779238 100644 --- a/syncapi/storage/sqlite3/output_room_events_topology_table.go +++ b/syncapi/storage/sqlite3/output_room_events_topology_table.go @@ -45,17 +45,17 @@ const insertEventInTopologySQL = "" + const selectEventIDsInRangeASCSQL = "" + "SELECT event_id FROM syncapi_output_room_events_topology" + - " WHERE room_id = $1 AND" + + " WHERE room_id = $1 AND (" + "(topological_position > $2 AND topological_position < $3) OR" + "(topological_position = $4 AND stream_position <= $5)" + - " ORDER BY topological_position ASC, stream_position ASC LIMIT $6" + ") ORDER BY topological_position ASC, stream_position ASC LIMIT $6" const selectEventIDsInRangeDESCSQL = "" + "SELECT event_id FROM syncapi_output_room_events_topology" + - " WHERE room_id = $1 AND" + + " WHERE room_id = $1 AND (" + "(topological_position > $2 AND topological_position < $3) OR" + "(topological_position = $4 AND stream_position <= $5)" + - " ORDER BY topological_position DESC, stream_position DESC LIMIT $6" + ") ORDER BY topological_position DESC, stream_position DESC LIMIT $6" const selectPositionInTopologySQL = "" + "SELECT topological_position, stream_position FROM syncapi_output_room_events_topology" + diff --git a/syncapi/storage/storage_test.go b/syncapi/storage/storage_test.go index 69ac5e6f..6b045852 100644 --- a/syncapi/storage/storage_test.go +++ b/syncapi/storage/storage_test.go @@ -405,6 +405,55 @@ func TestGetEventsInRangeWithEventsSameDepth(t *testing.T) { } } +// The purpose of this test is to make sure that the query to pull out events is honouring the room ID correctly. +// It works by creating two rooms with the same events in them, then selecting events by topological range. +// Specifically, we know that events with the same depth but lower stream positions are selected, and it's possible +// that this check isn't using the room ID if the brackets are wrong in the SQL query. +func TestGetEventsInTopologicalRangeMultiRoom(t *testing.T) { + t.Parallel() + db := MustCreateDatabase(t) + + makeEvents := func(roomID string) (events []gomatrixserverlib.HeaderedEvent) { + events = append(events, MustCreateEvent(t, roomID, nil, &gomatrixserverlib.EventBuilder{ + Content: []byte(fmt.Sprintf(`{"room_version":"4","creator":"%s"}`, testUserIDA)), + Type: "m.room.create", + StateKey: &emptyStateKey, + Sender: testUserIDA, + Depth: int64(len(events) + 1), + })) + events = append(events, MustCreateEvent(t, roomID, []gomatrixserverlib.HeaderedEvent{events[len(events)-1]}, &gomatrixserverlib.EventBuilder{ + Content: []byte(fmt.Sprintf(`{"membership":"join"}`)), + Type: "m.room.member", + StateKey: &testUserIDA, + Sender: testUserIDA, + Depth: int64(len(events) + 1), + })) + return + } + + roomA := "!room_a:" + string(testOrigin) + roomB := "!room_b:" + string(testOrigin) + eventsA := makeEvents(roomA) + eventsB := makeEvents(roomB) + MustWriteEvents(t, db, eventsA) + MustWriteEvents(t, db, eventsB) + from, err := db.MaxTopologicalPosition(ctx, roomB) + if err != nil { + t.Fatalf("failed to get MaxTopologicalPosition: %s", err) + } + // head towards the beginning of time + to := types.NewTopologyToken(0, 0) + + // Query using room B as room A was inserted first and hence A will have lower stream positions but identical depths, + // allowing this bug to surface. + paginatedEvents, err := db.GetEventsInTopologicalRange(ctx, &from, &to, roomB, 5, true) + if err != nil { + t.Fatalf("GetEventsInRange returned an error: %s", err) + } + gots := gomatrixserverlib.HeaderedToClientEvents(db.StreamEventsToEvents(&testUserDeviceA, paginatedEvents), gomatrixserverlib.FormatAll) + assertEventsEqual(t, "", true, gots, reversed(eventsB)) +} + // The purpose of this test is to make sure that events are returned in the right *order* when they have been inserted in a manner similar to // how any kind of backfill operation will insert the events. This test inserts the SimpleRoom events in a manner similar to how backfill over // federation would: |