Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Draft: Make sure historical state events don't come down /transactions for application services (MSC2716) #221

Closed
wants to merge 11 commits into from
Closed

Draft: Make sure historical state events don't come down /transactions for application services (MSC2716) #221

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6c1f6a0
Also make sure historical state events don't come down /sync
MadLittleMods Nov 5, 2021
eeb5076
Clean up test
MadLittleMods Nov 5, 2021
865848d
Add comment for includes function
MadLittleMods Nov 5, 2021
7ab71d4
Make sure historical events aren't sent as AS transactions
MadLittleMods Nov 6, 2021
06935a7
Use random port for application service
MadLittleMods Nov 6, 2021
4268459
Explain registration regex
MadLittleMods Nov 6, 2021
c40bdf1
Unshadow variable
MadLittleMods Nov 6, 2021
d920d70
Merge branch 'master' into madlittlemods/msc2716-no-push-historical-s…
MadLittleMods Nov 11, 2021
7ce9749
Merge branch 'master' into madlittlemods/msc2716-no-push-historical-s…
MadLittleMods Nov 24, 2021
4e00b9f
Merge branch 'main' into madlittlemods/msc2716-no-push-historical-state
MadLittleMods Nov 23, 2022
12ef80b
Fix tech debt
MadLittleMods Nov 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions tests/msc2716_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ func TestImportHistoricalMessages(t *testing.T) {
})
})

t.Run("Historical events from /batch_send do not come down in an incremental sync", func(t *testing.T) {
t.Run("Historical events from batch_send do not come down in an incremental sync", func(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the / in the test name so I can actually run it individually, COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestImportHistoricalMessages/parallel/Historical_events_from_batch_send_do_not_come_down_in_an_incremental_sync

See https://gist.github.com/MadLittleMods/4ab08f51609fab759247f299a4e33406 for why there is a problem using a / to match a single test.

t.Parallel()

roomID := as.CreateRoom(t, createPublicRoomOpts)
Expand All @@ -269,6 +269,10 @@ func TestImportHistoricalMessages(t *testing.T) {
// Create some "live" events to saturate and fill up the /sync response
createMessagesInRoom(t, alice, roomID, 5)

// Get a /sync `since` pagination token we can try paginating from later
// on
since := doInitialSync(t, alice)

// Import a historical event
batchSendRes := batchSendHistoricalMessages(
t,
Expand All @@ -283,19 +287,20 @@ func TestImportHistoricalMessages(t *testing.T) {
)
batchSendResBody := client.ParseJSON(t, batchSendRes)
historicalEventIDs := client.GetJSONFieldStringArray(t, batchSendResBody, "event_ids")
historicalEventId := historicalEventIDs[0]
historicalStateEventIDs := client.GetJSONFieldStringArray(t, batchSendResBody, "state_event_ids")

// This is just a dummy event we search for after the historicalEventId
// This is just a dummy event we search for after the historicalEventIDs/historicalStateEventIDs
eventIDsAfterHistoricalImport := createMessagesInRoom(t, alice, roomID, 1)
eventIDAfterHistoricalImport := eventIDsAfterHistoricalImport[0]

// Sync until we find the eventIDAfterHistoricalImport.
// If we're able to see the eventIDAfterHistoricalImport that occurs after
// the historicalEventId without seeing eventIDAfterHistoricalImport in
// between, we're probably safe to assume it won't sync
alice.SyncUntil(t, "", `{ "room": { "timeline": { "limit": 3 } } }`, "rooms.join."+client.GjsonEscape(roomID)+".timeline.events", func(r gjson.Result) bool {
if r.Get("event_id").Str == historicalEventId {
t.Fatalf("We should not see the %s historical event in /sync response but it was present", historicalEventId)
// Sync from before we did any batch sending until we find the
// eventIDAfterHistoricalImport. If we're able to see
// eventIDAfterHistoricalImport without any the
// historicalEventIDs/historicalStateEventIDs in between, we're probably
// safe to assume it won't sync.
alice.SyncUntil(t, since, "", "rooms.join."+client.GjsonEscape(roomID)+".timeline.events", func(r gjson.Result) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this test more clear on what's happening. We now paginate sync from before we /batch_send until an event that occurred after /batch_send and just make sure we didnt' see any historical state or events in between.

if includes(r.Get("event_id").Str, historicalEventIDs) || includes(r.Get("event_id").Str, historicalStateEventIDs) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is where we additionally check for the historicalStateEventIDs now

t.Fatalf("We should not see the %s historical event in /sync response but it was present", r.Get("event_id").Str)
}

return r.Get("event_id").Str == eventIDAfterHistoricalImport
Expand Down Expand Up @@ -926,6 +931,16 @@ func reversed(in []string) []string {
return out
}

func includes(needle string, haystack []string) bool {
for _, item := range haystack {
if needle == item {
return true
}
}

return false
}

func fetchUntilMessagesResponseHas(t *testing.T, c *client.CSAPI, roomID string, check func(gjson.Result) bool) {
t.Helper()
start := time.Now()
Expand Down