Skip to content

Commit 6e3efef

Browse files
toger5toger5
authored
Fix empty string to room compatibility trick to only apply to m.call (#5172)
* Fix empty string to room compatibility trick to only apply to m.call * add logging * fix linter * Add tests * limit logging.
1 parent fb59062 commit 6e3efef

File tree

4 files changed

+114
-20
lines changed

4 files changed

+114
-20
lines changed

‎spec/unit/matrixrtc/CallMembership.spec.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,10 +158,35 @@ describe("CallMembership", () => {
158158
expect(membership.eventId).toBe("$eventid");
159159
});
160160
it("returns correct slot_id", () => {
161+
// slot_id is application and call_id dependent. So we create
162+
// a membership for each possible combination
163+
164+
// non call application (should not alter call_id even with empty string)
165+
const nonCallMembership = createCallMembership(makeMockEvent(), {
166+
...membershipTemplate,
167+
application: "m.not.a.call",
168+
call_id: "",
169+
});
170+
// non "" call id should not be altered
171+
const callMembershipCustomId = createCallMembership(makeMockEvent(), {
172+
...membershipTemplate,
173+
call_id: "customCallId",
174+
});
175+
176+
// for membership (application = m.call and call_id = "") we expect "" -> ROOM
161177
// for legacy events we expect the room to be added automagically
162178
// See INFO_SLOT_ID_LEGACY_CASE comments
163179
expect(membership.slotId).toBe("m.call#ROOM");
164180
expect(membership.slotDescription).toStrictEqual({ id: "ROOM", application: "m.call" });
181+
182+
expect(nonCallMembership.slotId).toBe("m.not.a.call#");
183+
expect(nonCallMembership.slotDescription).toStrictEqual({ id: "", application: "m.not.a.call" });
184+
185+
expect(callMembershipCustomId.slotId).toBe("m.call#customCallId");
186+
expect(callMembershipCustomId.slotDescription).toStrictEqual({
187+
id: "customCallId",
188+
application: "m.call",
189+
});
165190
});
166191
it("returns correct deviceId", () => {
167192
expect(membership.deviceId).toBe("AAAAAAA");

‎spec/unit/matrixrtc/MembershipManager.spec.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ describe("MembershipManager", () => {
139139
"org.matrix.msc3401.call.member",
140140
{
141141
application: "m.call",
142+
// This tests INFO_SLOT_ID_LEGACY_CASE because it is using callSession = { id: "ROOM", application: "m.call" }
142143
call_id: "",
143144
device_id: "AAAAAAA",
144145
expires: 14400000,
@@ -147,6 +148,7 @@ describe("MembershipManager", () => {
147148
focus_active: focusActive,
148149
scope: "m.room",
149150
},
151+
// This tests INFO_SLOT_ID_LEGACY_CASE because it is using callSession = { id: "ROOM", application: "m.call" }
150152
"_@alice:example.org_AAAAAAA_m.call",
151153
);
152154
restartScheduledDelayedEventHandle.resolve?.();
@@ -160,6 +162,45 @@ describe("MembershipManager", () => {
160162
expect(client._unstable_sendDelayedStateEvent).toHaveBeenCalledTimes(1);
161163
});
162164

165+
it("sends correct call_id and state key when using non empty string. Not using empty string -> ROOM hack. See: INFO_SLOT_ID_LEGACY_CASE", async () => {
166+
// Spys/Mocks
167+
168+
const customCallSession = { id: "custom", application: "m.call" };
169+
const restartScheduledDelayedEventHandle = createAsyncHandle<void>(
170+
client._unstable_restartScheduledDelayedEvent,
171+
);
172+
173+
// Test
174+
const memberManager = new MembershipManager(undefined, room, client, customCallSession);
175+
memberManager.join([focus], undefined);
176+
// expects
177+
await waitForMockCall(client.sendStateEvent, Promise.resolve({ event_id: "id" }));
178+
expect(client.sendStateEvent).toHaveBeenCalledWith(
179+
room.roomId,
180+
"org.matrix.msc3401.call.member",
181+
{
182+
application: "m.call",
183+
call_id: "custom",
184+
device_id: "AAAAAAA",
185+
expires: 14400000,
186+
foci_preferred: [focus],
187+
membershipID: "@alice:example.org:AAAAAAA",
188+
focus_active: focusActive,
189+
scope: "m.room",
190+
},
191+
"_@alice:example.org_AAAAAAA_m.callcustom",
192+
);
193+
restartScheduledDelayedEventHandle.resolve?.();
194+
expect(client._unstable_sendDelayedStateEvent).toHaveBeenCalledWith(
195+
room.roomId,
196+
{ delay: 8000 },
197+
"org.matrix.msc3401.call.member",
198+
{},
199+
"_@alice:example.org_AAAAAAA_m.callcustom",
200+
);
201+
expect(client._unstable_sendDelayedStateEvent).toHaveBeenCalledTimes(1);
202+
});
203+
163204
it("reschedules delayed leave event if sending state cancels it", async () => {
164205
const memberManager = new MembershipManager(undefined, room, client, callSession);
165206
const waitForSendState = waitForMockCall(client.sendStateEvent);

‎src/matrixrtc/CallMembership.ts‎

Lines changed: 43 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -372,28 +372,53 @@ export class CallMembership {
372372
*/
373373
public get slotId(): string {
374374
const { kind, data } = this.membershipData;
375+
if (data.application === "m.call") {
376+
switch (kind) {
377+
case "rtc":
378+
return data.slot_id;
379+
case "session":
380+
default: {
381+
const [application, id] = [this.application, data.call_id];
382+
383+
// INFO_SLOT_ID_LEGACY_CASE (search for all occurances of this INFO to get the full picture)
384+
// The spec got changed to use `"ROOM"` instead of `""` empyt string for the implicit default call.
385+
// State events still are sent with `""` however. To find other events that should end up in the same call,
386+
// we use the slotId.
387+
// Since the CallMembership is the public representation of a rtc.member event, we just pretend it is a
388+
// "ROOM" slotId/call_id.
389+
// This makes all the remote members work with just this simple trick.
390+
//
391+
// We of course now need to be careful when sending legacy events (state events)
392+
// They get a slotDescription containing "ROOM" since this is what we use starting at the time this comment
393+
// is commited.
394+
//
395+
// See the Other INFO_SLOT_ID_LEGACY_CASE comments to see where we revert back to "" just before sending the event.
396+
let compatibilityAdaptedId: string;
397+
if (id === "") {
398+
compatibilityAdaptedId = "ROOM";
399+
this.logger?.info("use slotId compat hack emptyString -> ROOM");
400+
} else {
401+
compatibilityAdaptedId = id;
402+
}
403+
return slotDescriptionToId({
404+
application,
405+
id: compatibilityAdaptedId,
406+
});
407+
}
408+
}
409+
}
410+
411+
this.logger?.info("NOT using slotId compat hack emptyString -> ROOM");
412+
// This is what the function should look like for any other application that did not
413+
// go through a `""`=> `"ROOM"` rename
375414
switch (kind) {
376415
case "rtc":
377416
return data.slot_id;
378417
case "session":
379-
default:
380-
// INFO_SLOT_ID_LEGACY_CASE (search for all occurances of this INFO to get the full picture)
381-
// The spec got changed to use `"ROOM"` instead of `""` empyt string for the implicit default call.
382-
// State events still are sent with `""` however. To find other events that should end up in the same call,
383-
// we use the slotId.
384-
// Since the CallMembership is the public representation of a rtc.member event, we just pretend it is a
385-
// "ROOM" slotId/call_id.
386-
// This makes all the remote members work with just this simple trick.
387-
//
388-
// We of course now need to be careful when sending legacy events (state events)
389-
// They get a slotDescription containing "ROOM" since this is what we use starting at the time this comment
390-
// is commited.
391-
//
392-
// See the Other INFO_SLOT_ID_LEGACY_CASE comments to see where we revert back to "" just before sending the event.
393-
return slotDescriptionToId({
394-
application: this.application,
395-
id: data.call_id === "" ? "ROOM" : data.call_id,
396-
});
418+
default: {
419+
const [application, id] = [this.application, data.call_id];
420+
return slotDescriptionToId({ application, id });
421+
}
397422
}
398423
}
399424

‎src/matrixrtc/MembershipManager.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -777,7 +777,8 @@ export class MembershipManager
777777
// INFO_SLOT_ID_LEGACY_CASE (search for all occurances of this INFO to get the full picture)
778778
// Revert back to "" just for the state key (state keys are always legacy. we use sticky events for non legacy events)
779779
const application = this.slotDescription.application;
780-
const slotId = this.slotDescription.id === "ROOM" ? "" : this.slotDescription.id;
780+
const needsEmptyStringRoomFix = application === "m.call" && this.slotDescription.id === "ROOM";
781+
const slotId = needsEmptyStringRoomFix ? "" : this.slotDescription.id;
781782
const stateKey = `${localUserId}_${localDeviceId}_${application}${slotId}`;
782783
if (/^org\.matrix\.msc(3757|3779)\b/.exec(this.room.getVersion())) {
783784
return stateKey;
@@ -791,6 +792,8 @@ export class MembershipManager
791792
*/
792793
protected makeMyMembership(expires: number): SessionMembershipData | RtcMembershipData {
793794
const ownMembership = this.ownMembership;
795+
const needsEmptyStringRoomFix =
796+
this.slotDescription.application === "m.call" && this.slotDescription.id === "ROOM";
794797

795798
const focusObjects =
796799
this.rtcTransport === undefined
@@ -806,7 +809,7 @@ export class MembershipManager
806809
"application": this.slotDescription.application,
807810
// INFO_SLOT_ID_LEGACY_CASE (search for all occurances of this INFO to get the full picture)
808811
// Revert back to "" just for the sending the event.
809-
"call_id": this.slotDescription.id === "ROOM" ? "" : this.slotDescription.id,
812+
"call_id": needsEmptyStringRoomFix ? "" : this.slotDescription.id,
810813
"scope": "m.room",
811814
"device_id": this.deviceId,
812815
// DO NOT use this.memberId here since that is the state key (using application...)

0 commit comments

Comments
 (0)