From bfed6edf415e62769d2b8c5f198870bf6db3a1ac Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 7 Jun 2022 11:16:53 +0100 Subject: [PATCH] Refactor Relations to not be per-EventTimelineSet (#2412) * Refactor Relations to not be per-EventTimelineSet * Fix comment and relations-container init * Revert timing tweaks * Fix relations order test * Add test and simplify thread relations handling * Fix order of initialising a room object * Fix test * Re-add thread handling for relations of unloaded threads * Ditch confusing experimental getter `MatrixEvent::isThreadRelation` * Fix room handling in RelationsContainer * Iterate PR * Tweak method naming to closer match spec --- spec/unit/event-timeline-set.spec.ts | 12 +- spec/unit/relations.spec.ts | 22 ++-- spec/unit/room.spec.ts | 4 +- src/client.ts | 9 -- src/models/event-timeline-set.ts | 184 +++------------------------ src/models/event.ts | 7 - src/models/relations-container.ts | 155 ++++++++++++++++++++++ src/models/relations.ts | 14 +- src/models/room.ts | 33 +---- src/models/thread.ts | 15 +-- src/sync.ts | 2 - 11 files changed, 214 insertions(+), 243 deletions(-) create mode 100644 src/models/relations-container.ts diff --git a/spec/unit/event-timeline-set.spec.ts b/spec/unit/event-timeline-set.spec.ts index eaa723940..053a78e46 100644 --- a/spec/unit/event-timeline-set.spec.ts +++ b/spec/unit/event-timeline-set.spec.ts @@ -40,8 +40,8 @@ describe('EventTimelineSet', () => { const itShouldReturnTheRelatedEvents = () => { it('should return the related events', () => { - eventTimelineSet.aggregateRelations(messageEvent); - const relations = eventTimelineSet.getRelationsForEvent( + eventTimelineSet.relations.aggregateChildEvent(messageEvent); + const relations = eventTimelineSet.relations.getChildEventsForEvent( messageEvent.getId(), "m.in_reply_to", EventType.RoomMessage, @@ -55,9 +55,7 @@ describe('EventTimelineSet', () => { beforeEach(() => { client = utils.mock(MatrixClient, 'MatrixClient'); room = new Room(roomId, client, userA); - eventTimelineSet = new EventTimelineSet(room, { - unstableClientRelationAggregation: true, - }); + eventTimelineSet = new EventTimelineSet(room); eventTimeline = new EventTimeline(eventTimelineSet); messageEvent = utils.mkMessage({ room: roomId, @@ -189,8 +187,8 @@ describe('EventTimelineSet', () => { }); it('should not return the related events', () => { - eventTimelineSet.aggregateRelations(messageEvent); - const relations = eventTimelineSet.getRelationsForEvent( + eventTimelineSet.relations.aggregateChildEvent(messageEvent); + const relations = eventTimelineSet.relations.getChildEventsForEvent( messageEvent.getId(), "m.in_reply_to", EventType.RoomMessage, diff --git a/spec/unit/relations.spec.ts b/spec/unit/relations.spec.ts index c1e1dc827..091d95ea9 100644 --- a/spec/unit/relations.spec.ts +++ b/spec/unit/relations.spec.ts @@ -96,19 +96,14 @@ describe("Relations", function() { }, }); - // Stub the room - - const room = new Room("room123", null, null); - // Add the target event first, then the relation event { + const room = new Room("room123", null, null); const relationsCreated = new Promise(resolve => { targetEvent.once(MatrixEventEvent.RelationsCreated, resolve); }); - const timelineSet = new EventTimelineSet(room, { - unstableClientRelationAggregation: true, - }); + const timelineSet = new EventTimelineSet(room); timelineSet.addLiveEvent(targetEvent); timelineSet.addLiveEvent(relationEvent); @@ -117,13 +112,12 @@ describe("Relations", function() { // Add the relation event first, then the target event { + const room = new Room("room123", null, null); const relationsCreated = new Promise(resolve => { targetEvent.once(MatrixEventEvent.RelationsCreated, resolve); }); - const timelineSet = new EventTimelineSet(room, { - unstableClientRelationAggregation: true, - }); + const timelineSet = new EventTimelineSet(room); timelineSet.addLiveEvent(relationEvent); timelineSet.addLiveEvent(targetEvent); @@ -131,6 +125,14 @@ describe("Relations", function() { } }); + it("should re-use Relations between all timeline sets in a room", async () => { + const room = new Room("room123", null, null); + const timelineSet1 = new EventTimelineSet(room); + const timelineSet2 = new EventTimelineSet(room); + expect(room.relations).toBe(timelineSet1.relations); + expect(room.relations).toBe(timelineSet2.relations); + }); + it("should ignore m.replace for state events", async () => { const userId = "@bob:example.com"; const room = new Room("room123", null, userId); diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 921feae1a..3807795d1 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -2334,7 +2334,7 @@ describe("Room", function() { const thread = threadRoot.getThread(); expect(thread.rootEvent).toBe(threadRoot); - const rootRelations = thread.timelineSet.getRelationsForEvent( + const rootRelations = thread.timelineSet.relations.getChildEventsForEvent( threadRoot.getId(), RelationType.Annotation, EventType.Reaction, @@ -2344,7 +2344,7 @@ describe("Room", function() { expect(rootRelations[0][1].size).toEqual(1); expect(rootRelations[0][1].has(rootReaction)).toBeTruthy(); - const responseRelations = thread.timelineSet.getRelationsForEvent( + const responseRelations = thread.timelineSet.relations.getChildEventsForEvent( threadResponse.getId(), RelationType.Annotation, EventType.Reaction, diff --git a/src/client.ts b/src/client.ts index 9a2a0e27d..304655eac 100644 --- a/src/client.ts +++ b/src/client.ts @@ -323,13 +323,6 @@ export interface ICreateClientOpts { */ sessionStore?: SessionStore; - /** - * Set to true to enable client-side aggregation of event relations - * via `EventTimelineSet#getRelationsForEvent`. - * This feature is currently unstable and the API may change without notice. - */ - unstableClientRelationAggregation?: boolean; - verificationMethods?: Array; /** @@ -903,7 +896,6 @@ export class MatrixClient extends TypedEventEmitter } = {}; - public unstableClientRelationAggregation = false; public identityServer: IIdentityServerProvider; public sessionStore: SessionStore; // XXX: Intended private, used in code. public http: MatrixHttpApi; // XXX: Intended private, used in code. @@ -1035,7 +1027,6 @@ export class MatrixClient extends TypedEventEmitter { + public readonly relations?: RelationsContainer; private readonly timelineSupport: boolean; - private unstableClientRelationAggregation: boolean; - private displayPendingEvents: boolean; + private readonly displayPendingEvents: boolean; private liveTimeline: EventTimeline; private timelines: EventTimeline[]; private _eventIdToTimeline: Record; private filter?: Filter; - private relations: Record>>; /** * Construct a set of EventTimeline objects, typically on behalf of a given @@ -121,17 +119,18 @@ export class EventTimelineSet extends TypedEventEmittereventId, relationType or eventType - * are not valid. - * - * @returns {?Relations} - * A container for relation events or undefined if there are no relation events for - * the relationType. - */ - public getRelationsForEvent( - eventId: string, - relationType: RelationType | string, - eventType: EventType | string, - ): Relations | undefined { - if (!this.unstableClientRelationAggregation) { - throw new Error("Client-side relation aggregation is disabled"); - } - - if (!eventId || !relationType || !eventType) { - throw new Error("Invalid arguments for `getRelationsForEvent`"); - } - - // debuglog("Getting relations for: ", eventId, relationType, eventType); - - const relationsForEvent = this.relations[eventId] || {}; - const relationsWithRelType = relationsForEvent[relationType] || {}; - return relationsWithRelType[eventType]; - } - - public getAllRelationsEventForEvent(eventId: string): MatrixEvent[] { - const relationsForEvent = this.relations?.[eventId] || {}; - const events = []; - for (const relationsRecord of Object.values(relationsForEvent)) { - for (const relations of Object.values(relationsRecord)) { - events.push(...relations.getRelations()); - } - } - return events; - } - - /** - * Set an event as the target event if any Relations exist for it already - * - * @param {MatrixEvent} event - * The event to check as relation target. - */ - public setRelationsTarget(event: MatrixEvent): void { - if (!this.unstableClientRelationAggregation) { - return; - } - - const relationsForEvent = this.relations[event.getId()]; - if (!relationsForEvent) { - return; - } - - for (const relationsWithRelType of Object.values(relationsForEvent)) { - for (const relationsWithEventType of Object.values(relationsWithRelType)) { - relationsWithEventType.setTargetEvent(event); - } - } - } - - /** - * Add relation events to the relevant relation collection. - * - * @param {MatrixEvent} event - * The new relation event to be aggregated. - */ - public aggregateRelations(event: MatrixEvent): void { - if (!this.unstableClientRelationAggregation) { - return; - } - - if (event.isRedacted() || event.status === EventStatus.CANCELLED) { - return; - } - - const onEventDecrypted = (event: MatrixEvent) => { - if (event.isDecryptionFailure()) { - // This could for example happen if the encryption keys are not yet available. - // The event may still be decrypted later. Register the listener again. - event.once(MatrixEventEvent.Decrypted, onEventDecrypted); - return; - } - - this.aggregateRelations(event); - }; - - // If the event is currently encrypted, wait until it has been decrypted. - if (event.isBeingDecrypted() || event.shouldAttemptDecryption()) { - event.once(MatrixEventEvent.Decrypted, onEventDecrypted); - return; - } - - const relation = event.getRelation(); - if (!relation) { - return; - } - - const relatesToEventId = relation.event_id; - const relationType = relation.rel_type; - const eventType = event.getType(); - - // debuglog("Aggregating relation: ", event.getId(), eventType, relation); - - let relationsForEvent: Record>> = this.relations[relatesToEventId]; - if (!relationsForEvent) { - relationsForEvent = this.relations[relatesToEventId] = {}; - } - let relationsWithRelType = relationsForEvent[relationType]; - if (!relationsWithRelType) { - relationsWithRelType = relationsForEvent[relationType] = {}; - } - let relationsWithEventType = relationsWithRelType[eventType]; - - if (!relationsWithEventType) { - relationsWithEventType = relationsWithRelType[eventType] = new Relations( - relationType, - eventType, - this.room, - ); - const relatesToEvent = this.findEventById(relatesToEventId) || this.room.getPendingEvent(relatesToEventId); - if (relatesToEvent) { - relationsWithEventType.setTargetEvent(relatesToEvent); - } - } - - relationsWithEventType.addEvent(event); - } } /** diff --git a/src/models/event.ts b/src/models/event.ts index 7e4de8e22..15d0649c4 100644 --- a/src/models/event.ts +++ b/src/models/event.ts @@ -514,13 +514,6 @@ export class MatrixEvent extends TypedEventEmittereventId, relationType or eventType + * are not valid. + * + * @returns {?Relations} + * A container for relation events or undefined if there are no relation events for + * the relationType. + */ + public getChildEventsForEvent( + eventId: string, + relationType: RelationType | string, + eventType: EventType | string, + ): Relations | undefined { + return this.relations[eventId]?.[relationType]?.[eventType]; + } + + public getAllChildEventsForEvent(parentEventId: string): MatrixEvent[] { + const relationsForEvent = this.relations[parentEventId] ?? {}; + const events: MatrixEvent[] = []; + for (const relationsRecord of Object.values(relationsForEvent)) { + for (const relations of Object.values(relationsRecord)) { + events.push(...relations.getRelations()); + } + } + return events; + } + + /** + * Set an event as the target event if any Relations exist for it already. + * Child events can point to other child events as their parent, so this method may be + * called for events which are also logically child events. + * + * @param {MatrixEvent} event The event to check as relation target. + */ + public aggregateParentEvent(event: MatrixEvent): void { + const relationsForEvent = this.relations[event.getId()]; + if (!relationsForEvent) return; + + for (const relationsWithRelType of Object.values(relationsForEvent)) { + for (const relationsWithEventType of Object.values(relationsWithRelType)) { + relationsWithEventType.setTargetEvent(event); + } + } + } + + /** + * Add relation events to the relevant relation collection. + * + * @param {MatrixEvent} event The new child event to be aggregated. + * @param {EventTimelineSet} timelineSet The event timeline set within which to search for the related event if any. + */ + public aggregateChildEvent(event: MatrixEvent, timelineSet?: EventTimelineSet): void { + if (event.isRedacted() || event.status === EventStatus.CANCELLED) { + return; + } + + const relation = event.getRelation(); + if (!relation) return; + + const onEventDecrypted = () => { + if (event.isDecryptionFailure()) { + // This could for example happen if the encryption keys are not yet available. + // The event may still be decrypted later. Register the listener again. + event.once(MatrixEventEvent.Decrypted, onEventDecrypted); + return; + } + + this.aggregateChildEvent(event, timelineSet); + }; + + // If the event is currently encrypted, wait until it has been decrypted. + if (event.isBeingDecrypted() || event.shouldAttemptDecryption()) { + event.once(MatrixEventEvent.Decrypted, onEventDecrypted); + return; + } + + const { event_id: relatesToEventId, rel_type: relationType } = relation; + const eventType = event.getType(); + + let relationsForEvent = this.relations[relatesToEventId]; + if (!relationsForEvent) { + relationsForEvent = this.relations[relatesToEventId] = {}; + } + + let relationsWithRelType = relationsForEvent[relationType]; + if (!relationsWithRelType) { + relationsWithRelType = relationsForEvent[relationType] = {}; + } + + let relationsWithEventType = relationsWithRelType[eventType]; + if (!relationsWithEventType) { + relationsWithEventType = relationsWithRelType[eventType] = new Relations( + relationType, + eventType, + this.client, + ); + + const room = this.room ?? timelineSet?.room; + const relatesToEvent = timelineSet?.findEventById(relatesToEventId) + ?? room?.findEventById(relatesToEventId) + ?? room?.getPendingEvent(relatesToEventId); + if (relatesToEvent) { + relationsWithEventType.setTargetEvent(relatesToEvent); + } + } + + relationsWithEventType.addEvent(event); + } +} diff --git a/src/models/relations.ts b/src/models/relations.ts index b3d0d2351..21d323907 100644 --- a/src/models/relations.ts +++ b/src/models/relations.ts @@ -15,10 +15,11 @@ limitations under the License. */ import { EventStatus, IAggregatedRelation, MatrixEvent, MatrixEventEvent } from './event'; -import { Room } from './room'; import { logger } from '../logger'; import { RelationType } from "../@types/event"; import { TypedEventEmitter } from "./typed-event-emitter"; +import { MatrixClient } from "../client"; +import { Room } from "./room"; export enum RelationsEvent { Add = "Relations.add", @@ -48,6 +49,7 @@ export class Relations extends TypedEventEmitter][] = []; private targetEvent: MatrixEvent = null; private creationEmitted = false; + private readonly client: MatrixClient; /** * @param {RelationType} relationType @@ -55,16 +57,16 @@ export class Relations extends TypedEventEmitter * prefer getLiveTimeline().getState(EventTimeline.FORWARDS). */ public currentState: RoomState; + public readonly relations = new RelationsContainer(this.client, this); /** * @experimental @@ -338,10 +339,6 @@ export class Room extends TypedEventEmitter * "chronological". * @param {boolean} [opts.timelineSupport = false] Set to true to enable improved * timeline support. - * @param {boolean} [opts.unstableClientRelationAggregation = false] - * Optional. Set to true to enable client-side aggregation of event relations - * via `EventTimelineSet#getRelationsForEvent`. - * This feature is currently unstable and the API may change without notice. */ constructor( public readonly roomId: string, @@ -1737,7 +1734,7 @@ export class Room extends TypedEventEmitter } // A thread relation is always only shown in a thread - if (event.isThreadRelation) { + if (event.isRelation(THREAD_RELATION_TYPE.name)) { return { shouldLiveInRoom: false, shouldLiveInThread: true, @@ -1816,8 +1813,7 @@ export class Room extends TypedEventEmitter toStartOfTimeline: boolean, ): Thread { if (rootEvent) { - const tl = this.getTimelineForEvent(rootEvent.getId()); - const relatedEvents = tl?.getTimelineSet().getAllRelationsEventForEvent(rootEvent.getId()); + const relatedEvents = this.relations.getAllChildEventsForEvent(rootEvent.getId()); if (relatedEvents?.length) { // Include all relations of the root event, given it'll be visible in both timelines, // except `m.replace` as that will already be applied atop the event using `MatrixEvent::makeReplaced` @@ -2102,24 +2098,7 @@ export class Room extends TypedEventEmitter * @param {module:models/event.MatrixEvent} event the relation event that needs to be aggregated. */ private aggregateNonLiveRelation(event: MatrixEvent): void { - const { shouldLiveInRoom, threadId } = this.eventShouldLiveIn(event); - const thread = this.getThread(threadId); - thread?.timelineSet.aggregateRelations(event); - - if (shouldLiveInRoom) { - // TODO: We should consider whether this means it would be a better - // design to lift the relations handling up to the room instead. - for (let i = 0; i < this.timelineSets.length; i++) { - const timelineSet = this.timelineSets[i]; - if (timelineSet.getFilter()) { - if (timelineSet.getFilter().filterRoomTimeline([event]).length) { - timelineSet.aggregateRelations(event); - } - } else { - timelineSet.aggregateRelations(event); - } - } - } + this.relations.aggregateChildEvent(event); } public getEventForTxnId(txnId: string): MatrixEvent { @@ -2405,7 +2384,7 @@ export class Room extends TypedEventEmitter private findThreadRoots(events: MatrixEvent[]): Set { const threadRoots = new Set(); for (const event of events) { - if (event.isThreadRelation) { + if (event.isRelation(THREAD_RELATION_TYPE.name)) { threadRoots.add(event.relationEventId); } } diff --git a/src/models/thread.ts b/src/models/thread.ts index 4fb8bed42..28bb9933d 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -88,7 +88,6 @@ export class Thread extends TypedEventEmitter { this.room = opts.room; this.client = opts.client; this.timelineSet = new EventTimelineSet(this.room, { - unstableClientRelationAggregation: true, timelineSupport: true, pendingEvents: true, }); @@ -166,6 +165,7 @@ export class Thread extends TypedEventEmitter { private onEcho = (event: MatrixEvent) => { if (event.threadRootId !== this.id) return; // ignore echoes for other timelines if (this.lastEvent === event) return; + if (!event.isRelation(THREAD_RELATION_TYPE.name)) return; // There is a risk that the `localTimestamp` approximation will not be accurate // when threads are used over federation. That could result in the reply @@ -229,13 +229,6 @@ export class Thread extends TypedEventEmitter { this._currentUserParticipated = true; } - if ([RelationType.Annotation, RelationType.Replace].includes(event.getRelation()?.rel_type as RelationType)) { - // Apply annotations and replace relations to the relations of the timeline only - this.timelineSet.setRelationsTarget(event); - this.timelineSet.aggregateRelations(event); - return; - } - // Add all incoming events to the thread's timeline set when there's no server support if (!Thread.hasServerSideSupport) { // all the relevant membership info to hydrate events with a sender @@ -251,6 +244,11 @@ export class Thread extends TypedEventEmitter { ) { this.fetchEditsWhereNeeded(event); this.addEventToTimeline(event, false); + } else if (event.isRelation(RelationType.Annotation) || event.isRelation(RelationType.Replace)) { + // Apply annotations and replace relations to the relations of the timeline only + this.timelineSet.relations.aggregateParentEvent(event); + this.timelineSet.relations.aggregateChildEvent(event, this.timelineSet); + return; } // If no thread support exists we want to count all thread relation @@ -293,6 +291,7 @@ export class Thread extends TypedEventEmitter { // XXX: Workaround for https://github.com/matrix-org/matrix-spec-proposals/pull/2676/files#r827240084 private async fetchEditsWhereNeeded(...events: MatrixEvent[]): Promise { return Promise.all(events.filter(e => e.isEncrypted()).map((event: MatrixEvent) => { + if (event.isRelation()) return; // skip - relations don't get edits return this.client.relations(this.roomId, event.getId(), RelationType.Replace, event.getType(), { limit: 1, }).then(relations => { diff --git a/src/sync.ts b/src/sync.ts index 69da493eb..4abd4fb5b 100644 --- a/src/sync.ts +++ b/src/sync.ts @@ -202,13 +202,11 @@ export class SyncApi { const client = this.client; const { timelineSupport, - unstableClientRelationAggregation, } = client; const room = new Room(roomId, client, client.getUserId(), { lazyLoadMembers: this.opts.lazyLoadMembers, pendingEventOrdering: this.opts.pendingEventOrdering, timelineSupport, - unstableClientRelationAggregation, }); client.reEmitter.reEmit(room, [ RoomEvent.Name,