MSC2716 federation changes by MadLittleMods · Pull Request #175 · matrix-org/complement

MadLittleMods · 2021-07-23T05:19:20Z

MSC2716 federation changes

Unskip the rest of the federation tests
Refactor batchSendHistoricalMessages to be able to compose state and message events
Add test to ensure it is possible to batch send in a private room
Add test to ensure we only pass back state_events that we passed in via ?state_events_at_start, Only return state events that the AS passed in via state_events_at_start (MSC2716) synapse#10552

Dev notes

COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestBackfillingHistory

messagesResAsdf := alice.MustDoFunc(t, "GET", []string{"_matrix", "client", "r0", "rooms", roomID, "messages"}, client.WithContentType("application/json"), client.WithQueries(url.Values{
	"dir":   []string{"b"},
	"limit": []string{"100"},
}))
messagesResAsdfBody := client.ParseJSON(t, messagesResAsdf)
eventDebugStringsFromResponseAsdf := getRelevantEventDebugStringsFromMessagesResponse(t, messagesResAsdfBody)
logrus.WithFields(logrus.Fields{
	"eventDebugStringsFromResponseAsdf": eventDebugStringsFromResponseAsdf,
}).Error("see messages")

Todo

Should work with private room where the state_events_before includes invitations and join events
~~Redacting an insertion, chunk, marker so remove it from our new tracking tables~~
- Redacting MSC2716 related events synapse#10521
- When redacting, keep event fields around that maintain the historical event structure intact (MSC2716) synapse#10538
Two insertion events with the same next_chunk_id will reject. ((room_id, next_chunk_id) should be unique))
- Marked as TODO tests for another iteration
What happens when two chunks try to point at the same insertion event?
- Marked as TODO tests for another iteration

Conflicts: tests/msc2716_test.go

MadLittleMods · 2021-08-04T03:34:10Z

tests/msc2716_test.go

 		})

 		t.Run("Historical messages are visible when joining on federated server - auto-generated base insertion event", func(t *testing.T) {
-			t.Skip("Skipping until federation is implemented")


Removing as matrix-org/synapse#10245 is now merged

tests/msc2716_test.go

…a composable functions

tests/msc2716_test.go

Synapse support was added in matrix-org/synapse#10498

tests/msc2716_test.go

MadLittleMods · 2021-08-10T22:39:57Z

tests/msc2716_test.go

 		})

 		t.Run("When messages have already been scrolled back through, new historical messages are visible in next scroll back on federated server", func(t *testing.T) {
-			t.Skip("Skipping until federation is implemented")


Removing as matrix-org/synapse#10498 is now merged

Synapse change added in matrix-org/synapse#10552

MadLittleMods · 2021-08-10T23:49:46Z

tests/msc2716_test.go

 			})
 		})

+		t.Run("Batch send endpoint only returns state events that we passed in via state_events_at_start", func(t *testing.T) {


Test added to verify matrix-org/synapse#10552

tests/msc2716_test.go

kegsay · 2021-09-08T15:37:09Z

tests/msc2716_test.go

+			// We only expect 1 state event to be returned because we only passed in 1
+			// event into `?state_events_at_start`
+			if len(stateEventIDs) != 1 {
+				t.Fatalf("Expected only 1 state event to be returned but received %d: %s", len(stateEventIDs), stateEventIDs)


Suggested change

t.Fatalf("Expected only 1 state event to be returned but received %d: %s", len(stateEventIDs), stateEventIDs)

t.Fatalf("Expected only 1 state event to be returned but received %d: %v", len(stateEventIDs), stateEventIDs)

For my own reference, what's the benefit of using %v vs %s here?

They both appear like:

Expected only 1 state event to be returned but received 1: [$EVEa7PzafkdmDIu4kEYwlqJFSSKV_pj5h5xYfRspFs4]

tests/msc2716_test.go

kegsay · 2021-09-08T15:42:31Z

tests/msc2716_test.go

+		},
+	}
+	// We can't use as.SendEventSynced(...) because application services can't use the /sync API
+	markerSendRes := as.MustDoFunc(t, "PUT", []string{"_matrix", "client", "r0", "rooms", roomID, "send", markerEvent.Type, "txn-m123"}, client.WithJSONBody(t, markerEvent.Content))


Is it a problem that the txn id never changes?

Since the homeserver is new every run, I think it's fine. I did notice that txn-m123 was re-used once, so I updated to something different.

Might be nice to add a helper function to client to get a random unique one easily (using txnId in there). I added an internal one in #192

tests/msc2716_test.go

kegsay

Otherwise LGTM

MadLittleMods · 2021-09-08T16:35:18Z

Thanks for the review @kegsay and catching the Python patterns that slipped in 🐆

Update MSC2716 federation test cases

048887b

MadLittleMods marked this pull request as draft July 23, 2021 05:21

MadLittleMods added 5 commits July 26, 2021 16:35

Uncomment some federated test cases

d385044

Merge branch 'master' into madlittlemods/msc2716-federation-changes

493f7ea

Conflicts: tests/msc2716_test.go

More proper waiting to avoid flakey test

faec5ca

Get both tests working and de-duplicate marker sending verifying

edade9c

Make error comment match helper function name

b72cc1f

MadLittleMods commented Aug 4, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods commented Aug 4, 2021

View reviewed changes

tests/msc2716_test.go Outdated Show resolved Hide resolved

MadLittleMods commented Aug 4, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods added 2 commits August 4, 2021 01:57

Add test for private room backfilling

6bc06f7

Simplify batchSendHistoricalMessages function signature by using sort…

3b2358f

…a composable functions

MadLittleMods commented Aug 4, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods commented Aug 4, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

Remove test skips now that it's supported in Synapse

637ac8f

Synapse support was added in matrix-org/synapse#10498

MadLittleMods commented Aug 10, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods commented Aug 10, 2021

View reviewed changes

MadLittleMods changed the title ~~Draft: MSC2716 federation changes~~ MSC2716 federation changes Aug 10, 2021

MadLittleMods marked this pull request as ready for review August 10, 2021 22:44

MadLittleMods requested a review from kegsay August 10, 2021 22:47

Verify we only return state events that we pass in

b06fc95

Synapse change added in matrix-org/synapse#10552

MadLittleMods commented Aug 10, 2021

View reviewed changes

MadLittleMods commented Aug 11, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods commented Aug 11, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods commented Aug 11, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods commented Aug 11, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods commented Aug 11, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

MadLittleMods mentioned this pull request Aug 18, 2021

Add tests for MSC2716 being backwards compatible with existing room versions #192

Merged

Add helper decorations

a958b34

MadLittleMods mentioned this pull request Sep 4, 2021

Backfill should gracefully handle the case where no one has the complete state for a given event matrix-org/synapse#10764

Open

kegsay reviewed Sep 8, 2021

View reviewed changes

tests/msc2716_test.go Show resolved Hide resolved

kegsay reviewed Sep 8, 2021

View reviewed changes

tests/msc2716_test.go Outdated Show resolved Hide resolved

kegsay reviewed Sep 8, 2021

View reviewed changes

tests/msc2716_test.go Outdated Show resolved Hide resolved

kegsay reviewed Sep 8, 2021

View reviewed changes

tests/msc2716_test.go Outdated Show resolved Hide resolved

kegsay approved these changes Sep 8, 2021

View reviewed changes

More idiomatic Go from review

858aef6

MadLittleMods merged commit 735548e into master Sep 8, 2021

	t.Fatalf("Expected only 1 state event to be returned but received %d: %s", len(stateEventIDs), stateEventIDs)
	t.Fatalf("Expected only 1 state event to be returned but received %d: %v", len(stateEventIDs), stateEventIDs)

Conversation

MadLittleMods commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev notes

Todo

Uh oh!

MadLittleMods Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kegsay Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kegsay Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kegsay left a comment

Choose a reason for hiding this comment

Uh oh!

MadLittleMods commented Sep 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MadLittleMods commented Jul 23, 2021 •

edited

Loading

MadLittleMods Sep 8, 2021 •

edited

Loading

MadLittleMods Sep 8, 2021 •

edited

Loading