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

Fix issues with implementation for MSC3981 #3448

Closed
wants to merge 7 commits into from
Closed

Fix issues with implementation for MSC3981 #3448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
db6a8ae
Set UNSIGNED_THREAD_ID on messages retrieved in thread timelines
justjanne Jun 6, 2023
d003122
If recursive relations are available, request the entire thread for t…
justjanne Jun 6, 2023
3061aca
chore: fixed lint issues
justjanne Jun 6, 2023
ffa2366
Adapt tests to new changed behaviour
justjanne Jun 6, 2023
62c4770
Add new tests to increase test coverage
justjanne Jun 6, 2023
0d114ef
chore: fixed lint issues
justjanne Jun 6, 2023
e948f90
increase test coverage
justjanne Jun 6, 2023
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
115 changes: 107 additions & 8 deletions spec/integ/matrix-client-event-timeline.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
PendingEventOrdering,
RelationType,
Room,
UNSIGNED_THREAD_ID_FIELD,
Copy link
Member

Choose a reason for hiding this comment

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

generally, I think that using constants like this in tests is an antipattern. It means that the tests don't help us spot typos or unexpected changes in the behaviour of the constant, and it makes the tests harder to grok.

It would be better just to hardcode org.matrix.msc4023.thread_id.

} from "../../src/matrix";
import { logger } from "../../src/logger";
import { encodeParams, encodeUri, QueryDict, replaceParam } from "../../src/utils";
Expand All @@ -52,12 +53,16 @@ const withoutRoomId = (e: Partial<IEvent>): Partial<IEvent> => {
/**
* Our httpBackend only allows matching calls if we have the exact same query, in the exact same order
* This method allows building queries with the exact same parameter order as the fetchRelations method in client
* @param client Matrix client to mock the request for
* @param params query parameters
*/
const buildRelationPaginationQuery = (params: QueryDict): string => {
const buildRelationPaginationQuery = (client: MatrixClient, params: QueryDict): string => {
if (Thread.hasServerSideFwdPaginationSupport === FeatureSupport.Experimental) {
params = replaceParam("dir", "org.matrix.msc3715.dir", params);
}
if (client.canSupport.get(Feature.RelationsRecursion) === ServerSupport.Unstable) {
params = replaceParam("recurse", "org.matrix.msc3981.recurse", params);
}
Comment on lines +63 to +65
Copy link
Member

Choose a reason for hiding this comment

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

this feels like a sledgehammer to crack a nut. Shouldn't the calling test know if we expect recurse or org.matrix.msc3981.recurse?

return "?" + encodeParams(params).toString();
};

Expand Down Expand Up @@ -179,6 +184,9 @@ const THREAD_REPLY = utils.mkEvent({
event_id: THREAD_ROOT.event_id,
},
},
unsigned: {
[UNSIGNED_THREAD_ID_FIELD.name]: THREAD_ROOT.event_id,
},
event: false,
});

Expand Down Expand Up @@ -622,7 +630,7 @@ describe("MatrixClient event timelines", function () {
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }),
buildRelationPaginationQuery(client, { dir: Direction.Backward, limit: 1 }),
)
.respond(200, function () {
return {
Expand Down Expand Up @@ -1020,6 +1028,87 @@ describe("MatrixClient event timelines", function () {
]);
});

it("should use recursive relations to paginate thread timelines", async function () {
function respondToThreads(
response = {
chunk: [THREAD_ROOT],
state: [],
next_batch: null,
},
): ExpectedHttpRequest {
const request = httpBackend.when(
"GET",
encodeUri("/_matrix/client/v1/rooms/$roomId/threads", {
$roomId: roomId,
}),
);
request.respond(200, response);
return request;
}

function respondToThread(
root: Partial<IEvent>,
replies: Partial<IEvent>[],
limit?: number,
): ExpectedHttpRequest {
const request = httpBackend.when(
"GET",
"/_matrix/client/v1/rooms/!foo%3Abar/relations/" +
encodeURIComponent(root.event_id!) +
buildRelationPaginationQuery(client, {
dir: Direction.Backward,
limit: limit,
recurse: true,
}),
);
request.respond(200, function () {
return {
original_event: root,
chunk: replies,
// no next batch as this is the oldest end of the timeline
};
});
return request;
}

function respondToEvent(event: Partial<IEvent> = THREAD_ROOT): ExpectedHttpRequest {
const request = httpBackend.when(
"GET",
encodeUri("/_matrix/client/r0/rooms/$roomId/event/$eventId", {
$roomId: roomId,
$eventId: event.event_id!,
}),
);
request.respond(200, event);
return request;
}

// Setup
// @ts-ignore
client.clientOpts.threadSupport = true;
Thread.setServerSideSupport(FeatureSupport.Stable);
Thread.setServerSideListSupport(FeatureSupport.Stable);
Thread.setServerSideFwdPaginationSupport(FeatureSupport.Stable);
client.canSupport.set(Feature.RelationsRecursion, ServerSupport.Unstable);
const room = client.getRoom(roomId)!;
await room!.createThreadsTimelineSets();
respondToThreads();
respondToThreads();
respondToEvent();
respondToEvent();
respondToEvent();
respondToEvent();
respondToEvent();
respondToThread(THREAD_ROOT, [THREAD_REPLY], 1);
await flushHttp(room.fetchRoomThreads());
const thread = room.getThread(THREAD_ROOT.event_id!)!;
expect(thread).not.toBeNull();
respondToThread(THREAD_ROOT, [THREAD_REPLY], 1);
expect(thread.timelineSet.thread).toBe(thread);
expect(Thread.hasServerSideSupport).toBe(FeatureSupport.Stable);
await flushHttp(client.getLatestTimeline(thread.timelineSet));
});

it("should create threads for thread roots discovered", function () {
const room = client.getRoom(roomId)!;
const timelineSet = room.getTimelineSets()[0];
Expand Down Expand Up @@ -1091,6 +1180,9 @@ describe("MatrixClient event timelines", function () {
event_id: THREAD_ROOT.event_id,
},
},
unsigned: {
[UNSIGNED_THREAD_ID_FIELD.name]: THREAD_ROOT.event_id,
},
Comment on lines +1183 to +1185
Copy link
Member

Choose a reason for hiding this comment

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

I'm somewhat concerned that by changing all these, we never test the fallback behaviour. Do we have any separate tests for the fallback? Do we actually need to change all these definitions?

event: true,
});
THREAD_REPLY2.localTimestamp += 1000;
Expand All @@ -1109,6 +1201,9 @@ describe("MatrixClient event timelines", function () {
event_id: THREAD_ROOT.event_id,
},
},
unsigned: {
[UNSIGNED_THREAD_ID_FIELD.name]: THREAD_ROOT.event_id,
},
event: true,
});
THREAD_REPLY3.localTimestamp += 2000;
Expand Down Expand Up @@ -1182,6 +1277,9 @@ describe("MatrixClient event timelines", function () {
event_id: THREAD_ROOT.event_id,
},
},
unsigned: {
[UNSIGNED_THREAD_ID_FIELD.name]: THREAD_ROOT.event_id,
},
event: true,
});
THREAD_REPLY2.localTimestamp += 1000;
Expand Down Expand Up @@ -1214,6 +1312,9 @@ describe("MatrixClient event timelines", function () {
event_id: THREAD_ROOT.event_id,
},
},
unsigned: {
[UNSIGNED_THREAD_ID_FIELD.name]: THREAD_ROOT.event_id,
},
event: true,
});
THREAD_REPLY3.localTimestamp += 3000;
Expand Down Expand Up @@ -1254,9 +1355,7 @@ describe("MatrixClient event timelines", function () {
"GET",
"/_matrix/client/v1/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT_UPDATED.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildRelationPaginationQuery({
buildRelationPaginationQuery(client, {
dir: Direction.Backward,
limit: 3,
recurse: true,
Expand Down Expand Up @@ -1904,7 +2003,7 @@ describe("MatrixClient event timelines", function () {
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }),
buildRelationPaginationQuery(client, { dir: Direction.Backward, limit: 1 }),
)
.respond(200, function () {
return {
Expand Down Expand Up @@ -1957,7 +2056,7 @@ describe("MatrixClient event timelines", function () {
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildRelationPaginationQuery({
buildRelationPaginationQuery(client, {
dir: Direction.Backward,
from: "start_token",
}),
Expand All @@ -1974,7 +2073,7 @@ describe("MatrixClient event timelines", function () {
encodeURIComponent(THREAD_ROOT.event_id!) +
"/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) +
buildRelationPaginationQuery({ dir: Direction.Forward, from: "end_token" }),
buildRelationPaginationQuery(client, { dir: Direction.Forward, from: "end_token" }),
)
.respond(200, function () {
return {
Expand Down
4 changes: 4 additions & 0 deletions spec/unit/room.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
RelationType,
RoomEvent,
RoomMember,
UNSIGNED_THREAD_ID_FIELD,
} from "../../src";
import { EventTimeline } from "../../src/models/event-timeline";
import { NotificationCountType, Room } from "../../src/models/room";
Expand Down Expand Up @@ -133,6 +134,9 @@ describe("Room", function () {
"rel_type": "m.thread",
},
},
unsigned: {
[UNSIGNED_THREAD_ID_FIELD.name]: root.getId(),
},
},
room.client,
);
Expand Down
62 changes: 43 additions & 19 deletions src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ import {
UNSTABLE_MSC3088_PURPOSE,
UNSTABLE_MSC3089_TREE_SUBTYPE,
MSC3912_RELATION_BASED_REDACTIONS_PROP,
UNSIGNED_THREAD_ID_FIELD,
} from "./@types/event";
import { IdServerUnbindResult, IImageInfo, Preset, Visibility } from "./@types/partials";
import { EventMapper, eventMapperFor, MapperOpts } from "./event-mapper";
Expand Down Expand Up @@ -5750,18 +5751,19 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
throw new Error("could not get thread timeline: not a thread timeline");
}

const relType = recurse ? null : THREAD_RELATION_TYPE.name;
const thread = timelineSet.thread;
const resOlder: IRelationsResponse = await this.fetchRelations(
timelineSet.room.roomId,
thread.id,
THREAD_RELATION_TYPE.name,
relType,
null,
{ dir: Direction.Backward, from: res.start, recurse: recurse || undefined },
);
const resNewer: IRelationsResponse = await this.fetchRelations(
timelineSet.room.roomId,
thread.id,
THREAD_RELATION_TYPE.name,
relType,
null,
{ dir: Direction.Forward, from: res.end, recurse: recurse || undefined },
);
Expand All @@ -5774,6 +5776,10 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
...resOlder.chunk.map(mapper),
];
for (const event of events) {
event.setUnsigned({
...event.getUnsigned(),
[UNSIGNED_THREAD_ID_FIELD.name]: timelineSet.thread.id,
Copy link
Member

Choose a reason for hiding this comment

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

isn't this overriding the response from the server? Why are we doing that?

});
await timelineSet.thread?.processEvent(event);
}

Expand Down Expand Up @@ -5805,21 +5811,20 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
// XXX: workaround for https://github.com/vector-im/element-meta/issues/150

const thread = timelineSet.thread;
const relType = recurse ? null : THREAD_RELATION_TYPE.name;

const resOlder = await this.fetchRelations(
timelineSet.room.roomId,
thread.id,
THREAD_RELATION_TYPE.name,
null,
{ dir: Direction.Backward, from: res.start, recurse: recurse || undefined },
);
const resOlder = await this.fetchRelations(timelineSet.room.roomId, thread.id, relType, null, {
dir: Direction.Backward,
from: res.start,
recurse: recurse || undefined,
});
const eventsNewer: IEvent[] = [];
let nextBatch: Optional<string> = res.end;
while (nextBatch) {
const resNewer: IRelationsResponse = await this.fetchRelations(
timelineSet.room.roomId,
thread.id,
THREAD_RELATION_TYPE.name,
relType,
null,
{ dir: Direction.Forward, from: nextBatch, recurse: recurse || undefined },
);
Expand All @@ -5835,6 +5840,12 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
...resOlder.chunk.map(mapper),
];
for (const event of events) {
// Fallback to set unsigned thread id if the server doesn't support it
// We know the thread id because we just requested this event via /relations
event.setUnsigned({
[UNSIGNED_THREAD_ID_FIELD.name]: timelineSet.thread.id,
...event.getUnsigned(),
});
await timelineSet.thread?.processEvent(event);
}

Expand Down Expand Up @@ -5881,10 +5892,12 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
}

let event;
const recurse = this.canSupport.get(Feature.RelationsRecursion) !== ServerSupport.Unsupported;
const relType = recurse ? null : THREAD_RELATION_TYPE.name;
if (timelineSet.threadListType !== null) {
const res = await this.createThreadListMessagesRequest(
timelineSet.room.roomId,
null,
relType,
1,
Direction.Backward,
timelineSet.threadListType,
Expand All @@ -5893,14 +5906,18 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
event = res.chunk?.[0];
} else if (timelineSet.thread && Thread.hasServerSideSupport) {
const recurse = this.canSupport.get(Feature.RelationsRecursion) !== ServerSupport.Unsupported;
const res = await this.fetchRelations(
timelineSet.room.roomId,
timelineSet.thread.id,
THREAD_RELATION_TYPE.name,
null,
{ dir: Direction.Backward, limit: 1, recurse: recurse || undefined },
);
const res = await this.fetchRelations(timelineSet.room.roomId, timelineSet.thread.id, relType, null, {
dir: Direction.Backward,
limit: 1,
recurse: recurse || undefined,
});
event = res.chunk?.[0];
// Fallback to set unsigned thread id if the server doesn't support it
// We know the thread id because we just requested this event via /relations
event.unsigned = {
[UNSIGNED_THREAD_ID_FIELD.name]: timelineSet.thread.id,
...event.unsigned,
};
} else {
const messagesPath = utils.encodeUri("/rooms/$roomId/messages", {
$roomId: timelineSet.room.roomId,
Expand Down Expand Up @@ -6174,7 +6191,8 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
}

const recurse = this.canSupport.get(Feature.RelationsRecursion) !== ServerSupport.Unsupported;
promise = this.fetchRelations(eventTimeline.getRoomId() ?? "", thread.id, THREAD_RELATION_TYPE.name, null, {
const relType = recurse ? null : THREAD_RELATION_TYPE.name;
promise = this.fetchRelations(eventTimeline.getRoomId() ?? "", thread.id, relType, null, {
dir,
limit: opts.limit,
from: token ?? undefined,
Expand All @@ -6186,6 +6204,12 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa

// Process latest events first
for (const event of matrixEvents.slice().reverse()) {
// Fallback to set unsigned thread id if the server doesn't support it
// We know the thread id because we just requested this event via /relations
event.setUnsigned({
[UNSIGNED_THREAD_ID_FIELD.name]: thread.id,
...event.getUnsigned(),
});
await thread?.processEvent(event);
const sender = event.getSender()!;
if (!backwards || thread?.getEventReadUpTo(sender) === null) {
Expand Down