Conversation
kegsay
left a comment
There was a problem hiding this comment.
I don't feel this PR should merge in its current state. I'd like to see more use cases where this functionality would be useful, and then we can see how this can be re-designed to work in a clear and consise way.
| } | ||
|
|
||
| // Check that the state for `roomID` has an event which passes the check function. | ||
| func SyncStateHas(roomID string, check func(gjson.Result) bool) SyncCheckOpt { |
There was a problem hiding this comment.
We need to be careful here, because this can be misused. The state section of the room contains the state at the start of the timeline, so it's entirely possible that the state to check is in the timeline and not the state. We should be very careful about checking one or the other. What is the use case here?
There was a problem hiding this comment.
Ultimately I would like to check that a lazy_load_members /sync response includes the membership of a given sender in the state section. The sender joined the room before the syncing user and their membership is not expected to appear in the timeline. The planned usage looks like this:
syncRes, _ := alice.MustSync(t,
client.SyncReq{
Since: previousSyncToken,
Filter: buildLazyLoadingSyncFilter(nil),
},
)
err = client.SyncTimelineHasEventID(serverRoom.RoomID, event.EventID())(alice.UserID, syncRes)
if err != nil {
t.Errorf("Did not find message in lazy-loading /sync response: %s", err)
}
err := client.SyncStateHasStateKey(serverRoom.RoomID, "m.room.member", event.Sender())(alice.UserID, syncRes)
if err != nil {
t.Errorf("Did not find %s's m.room.member event in lazy-loading /sync response: %s", event.Sender(), err)
}
I'm avoiding MustSyncUntil because it wouldn't fail immediately if event appears in the timeline without the corresponding sender membership, and the message is expected to already be available via /sync.
internal/client/client.go
Outdated
| } | ||
|
|
||
| // Check that the state for `roomID` has an event which matches the event ID. | ||
| func SyncStateHasEventID(roomID string, eventID string) SyncCheckOpt { |
There was a problem hiding this comment.
Please remove: this doesn't compose well at all. What if we wanted to check the timeline for an event ID? What about ephemeral data? Room account data? Then we'd end up with:
SyncStateHasEventIDSyncTimelineHasEventIDSyncEphemeralHasEventIDSyncRoomAccountDataHasEventID
I hope it's clear how unruly and unmanageable this ends up becoming, especially if you then want to check the type/state key like SyncStateHasStateKey does. It would be better if you added functions which match func(gjson.Result) bool and compose well e.g:
func CheckEventIDIs(eventID) func(gjson.Result) boolfunc CheckEventTypeIs(eventType) func(gjson.Result) bool
This would end up looking something like:
SyncStateHas(roomID, CheckEventIDIs(eventID), CheckEventTypeIs(eventType))
But this is also not great because often you want to check a few fields, though which fields depends on the test. Also, what is the relationship between the check functions: are they ANDed or ORd? At this point, it is often easier just to write the check function yourself:
SyncStateHas(roomID, func(ev gjson.Result) bool {
return ev.Get("type").Str == eventType && ev.Get("state_key").Str == stateKey
})
which also allows the flexibility to check say the content field easily enough. This is why we, as of yet, do not have per-event check functions: the benefits are unclear and the assertion itself is unclear.
There was a problem hiding this comment.
Do note that:
- The events in the
ephemeralandaccount_datasections do not haveevent_ids and we would not want to defineSyncEphemeralHasEventIDandSyncRoomAccountDataHasEventIDas the spec stands. SyncTimelineHasEventIDalready exists in Complement and this PR follows the existing pattern.
Shall we remove SyncTimelineHasEventID then?
|
Thanks for re-reviewing! |
These will turn out to be useful when testing lazy-loading
/syncs,among other things.