Add support for platform specific event indexing and search #3550

poljar · 2019-10-11T15:05:12Z

This patch-set adds support for search in E2E encrypted rooms.

We start of by extending our BasePlatform to have the necessary function prototypes. After that, support to create an index and start room history crawling. Followed by adding live events, deleting the index, and finally extending our search to make use of our event index.

This is crucially still missing any type of UI but is in a usable and seemingly mostly-stable state and ready for a first look.

This patch adds support to create an event index if the clients platform supports it and starts an event crawler. The event crawler goes through the room history of encrypted rooms and eventually indexes the whole room history of such rooms. It does this by first creating crawling checkpoints and storing them inside a database. A checkpoint consists of a room_id, direction and token. After the checkpoints are added the client starts a crawler method in the background. The crawler goes through checkpoints in a round-robin way and uses them to fetch historic room messages using the rooms/roomId/messages API endpoint. Every time messages are fetched a new checkpoint is created that will be stored in the database with the fetched events in an atomic way, the old checkpoint is deleted at the same time as well.

…ng out.

A sync call may not have all events that happened since the last time the client synced. In such a case the room is marked as limited and events need to be fetched separately. When such a sync call happens our event index will have a gap. To close the gap checkpoints are added to start crawling our room again. Unnecessary full re-crawls are prevented by checking if our current /room/roomId/messages request contains only events that were already present in our event index.

This patch extends our search to include our platform specific event index. There are 3 search scenarios and are handled differently when platform support for indexing is present: - Search a single non-encrypted room: Use the server-side search like before. - Search a single encrypted room: Search using our platform specific event index. - Search across all rooms: Search encrypted rooms using our local event index. Search non-encrypted rooms using the classic server-side search. Combine the results. The combined search will result in having twice the amount of search results since comparing the scores fairly wasn't deemed sensible.

… response.

bwindels

Some initial style comments

bwindels · 2019-10-28T11:13:42Z

src/BasePlatform.js

@@ -151,4 +151,44 @@ export default class BasePlatform {
    async setMinimizeToTrayEnabled(enabled: boolean): void {
        throw new Error("Unimplemented");
    }
+
+    supportsEventIndexing(): boolean {


Suggestion: perhaps we can put all the methods here related to event indexing into an object returned from the platform? E.g. platform.getEventIndexingManager or whatever, returning null on unsupported platforms?

Sure, one way to abstract it. Depends on if we want to declare the event plumbing (convert the MatrixEvent into a plain json object) platform specific to Seshat or if we want it to be more of a guidance for other indexer implementations ("hey your platform specific indexer needs to accept these types and return such a search result").

Yes, I agree with @bwindels... Since it's expected that all these methods would be either implemented or unimplemented as a set, it would be good to package them in single object and have one accessor here to retrieve it.

bwindels · 2019-10-28T11:14:51Z

src/components/structures/MatrixChat.js

@@ -1262,6 +1270,8 @@ export default createReactClass({
        // to do the first sync
        this.firstSyncComplete = false;
        this.firstSyncPromise = Promise.defer();
+        this.crawlerChekpoints = [];


crawlerCheckpoints?

bwindels · 2019-10-28T11:19:24Z

src/components/structures/MatrixChat.js

            console.log("Request to reset timeline in room ", roomId, " viewing:", self.state.currentRoomId);
+            // TODO is there a better place to plug this in


There's the Room.timelineReset event on the client you could listen for. It's probably best to avoid making this callback async as well.

I tried that event but I don't think it fired reliably, or maybe it doesn't like multiple event listeners? I can retry moving it there. Any more info why not make it async?

I tried that event but I don't think it fired reliably

Hmm, interesting. It does seem to be fired unconditionally in EventTimelineSet. Any particular case you found it didn't fire?

Any more info why not make it async?

Just feels like this "informational" callback shouldn't have too many side-effects, let alone async ones. There's a slight risk of introducing race conditions as well in case multiple syncs would come in fast. And it would need some plumbing in the js-sdk, as the callback currently isn't expected to return a promise.

I don't really remember the specifics anymore, might have imagined it or done something stupid. Will change it up and see if it works.

bwindels · 2019-10-28T11:25:12Z

src/components/structures/MatrixChat.js

@@ -1930,4 +2058,220 @@ export default createReactClass({
            {view}
        </ErrorBoundary>;
    },
+
+    async addLiveEventToIndex(ev) {


I wonder if it would make sense to move most of this code that's been added to MatrixChat into a separate file/class, given how large it is already, and how independent the seshat integration seems.

Yes, that should happen. It's kept mostly in a single file for this PR to make it easier to review. @dbkr mentioned that already and I agreed.

We should definitely separate this out to a src/EventIndexing.js or similar file.

jryans

Looks like a good start! 😁 It may take a few rounds to get the right architecture in place. I'm happy chat in the room etc. if that would help.

Once we've got the right design, we should be able to merge this React SDK side, since it's all guarded on having an indexer from the platform.

jryans · 2019-11-06T11:31:24Z

src/BasePlatform.js

@@ -151,4 +151,44 @@ export default class BasePlatform {
    async setMinimizeToTrayEnabled(enabled: boolean): void {
        throw new Error("Unimplemented");
    }
+
+    supportsEventIndexing(): boolean {


Yes, I agree with @bwindels... Since it's expected that all these methods would be either implemented or unimplemented as a set, it would be good to package them in single object and have one accessor here to retrieve it.

jryans · 2019-11-06T11:48:04Z

src/MatrixClientPeg.js

@@ -222,6 +223,9 @@ class MatrixClientPeg {

        this.matrixClient = createMatrixClient(opts);

+        const platform = PlatformPeg.get();
+        if (platform.supportsEventIndexing()) platform.initEventIndex(creds.userId);


Assuming you don't need this to happen precisely here for some reason, I'd suggest moving this to Lifecycle.js which has a startMatrixClient function that should work well for starting a service such as this.

You could perhaps craft a start method on the new indexer object suggested above, and then internally call MatrixClientPeg.get().getUserId() and pass it to initEventIndex.

jryans · 2019-11-06T11:53:21Z

src/components/structures/MatrixChat.js

+        if (platform.supportsEventIndexing()) {
+            console.log("Seshat: Deleting event index.");
+            this.crawlerRef.cancel();
+            await platform.deleteEventIndex();


The delete index step would be more natural in Lifecycle.js as part of the stopMatrixClient. As above, maybe it happens in a new stop method on the indexer object.

stopMatrixClient is called on a soft logout as well, there is a function called onLoggedOut in Lifecycle.js which clears the storage. Would that be an ok place to put it as well?

Ah, fair enough... We do have a bit of mess of lifecycle hooks... 😓 I guess _clearStorage (called by onLoggedOut) is likely the best place, as that's where we'd also clear the existing storage of message keys, etc.

jryans · 2019-11-06T11:56:10Z

src/components/structures/MatrixChat.js

@@ -1930,4 +2058,220 @@ export default createReactClass({
            {view}
        </ErrorBoundary>;
    },
+
+    async addLiveEventToIndex(ev) {


We should definitely separate this out to a src/EventIndexing.js or similar file.

jryans · 2019-11-06T12:00:55Z

src/components/structures/RoomView.js

@@ -1140,12 +1141,116 @@ module.exports = createReactClass({
        }

        debuglog("sending search request");
+        const platform = PlatformPeg.get();


These bits would also be better off in some kind of "searching" module so that here we're just calling search effectively and the change will approximately be switching to the new module instead of the client directly.

…h function.

…ame.

…rted.

We need to support the deletion of the event index even if it's not currently initialized, therefore the deletion ended up in the EventIndexPeg class.

…class.

poljar · 2019-11-19T13:36:34Z

Everything should be fixed/updated, except the "M of N" log line and publicizing of the private js-sdk methods.

The merge conflicts should be easy to fix since they seem to be related to labs flags only.

src/indexing/EventIndex.js

src/settings/Settings.js

jryans · 2019-11-20T11:07:17Z

src/EventIndexing.js

+            // here.
+            await sleep(this._crawlerTimeout);
+
+            console.log("EventIndex: Running the crawler loop.");


Okay, sounds like Matthew, Nad, and others will help advise on what's needed. Now that we have the feature flag, it feels safer to land with out it for now, since you have to opt-in.

jryans

Thanks for the continued effort here! 😁

This looks ready to merge from my side (only the feature flag level seems like a required fix). Since we have both the feature flag and testing the platform at runtime, it's decoupled from the riot-web portion and disabled by default, so should be safe to land when you're ready.

jryans · 2019-11-20T11:44:00Z

src/indexing/EventIndex.js

+import PlatformPeg from "../PlatformPeg";
+import MatrixClientPeg from "../MatrixClientPeg";
+
+/**


Ah, it's tagging the property functions at this class level... I think the JSDoc lint is a bit confused, but maybe you can workaround it by making this not a JSDoc comment by changing this to a single star here...?

poljar added 9 commits October 11, 2019 16:55

BasePlatform: Add prototype methods for event indexing.

b8a3ee1

MatrixChat: Stop the crawler function and delete the index when loggi…

b23ba5f

…ng out.

MatrixChat: Add live events to the event index as well.

5e7076e

MatrixChat: Add more detailed logging to the event crawler.

1b63886

MatrixChat: Catch errors when fetching room messages in the crawler.

89f14e5

MatrixChat: Check if our state array is empty in the crawled messages…

6406117

… response.

dbkr mentioned this pull request Oct 24, 2019

As a user I want to find old messages by searching, in encrypted and unencrypted rooms element-hq/user-stories#5

Open

jryans requested a review from a team October 28, 2019 11:08

bwindels reviewed Oct 28, 2019

View reviewed changes

bwindels requested a review from a team October 28, 2019 17:11

jryans changed the title ~~[RFC] Add support for platform specific event indexing and search.~~ Add support for platform specific event indexing and search Oct 28, 2019

turt2live requested review from bwindels and jryans and removed request for a team November 4, 2019 17:22

jryans removed the request for review from bwindels November 6, 2019 12:01

jryans suggested changes Nov 6, 2019

View reviewed changes

lampholder added the phase:5 label Nov 7, 2019

poljar added 10 commits November 11, 2019 15:08

MatrixChat: Add some missing semicolons.

2c5565e

MatrixChat: Move the event indexing logic into separate modules.

cfdcf45

RoomView: Move the search logic into a separate module.

e296fd0

MatrixChat: Move the indexing limited room logic to a different event.

d911055

EventIndexing: Rename the stop method.

ecbc47c

EventIndexing: Add a missing platform getting.

d69eb78

LifeCycle: Stop the crawler and delete the index when whe log out.

3502454

Lifecycle: Move the event index deletion into the clear storage method.

0085544

Searching: Move the small helper functions out of the eventIndexSearc…

1cc64f2

…h function.

Fix some lint errors.

1df28c7

poljar added 20 commits November 18, 2019 10:58

EventIndexPeg: Move a docstring to the correct place.

ddb536e

EventIndexPeg: Treat both cases of unavailable platform support the s…

050e52c

…ame.

EventIndexing: Don't capitalize homeserver.

3b06c68

Searching: Move a comment to the correct place.

b4a6123

Searching: Use the short form to build the search arguments object.

a4ad815

Searching: Remove the func suffix from our search functions.

0e3a000

Searching: Fix a typo.

2bb331c

EventIndexPeg: Rewrite the module documentation.

579cbef

EventIndexing: Rename our EventIndexer class.

45e7aab

EventIndex: Rename the file to be consistent with the class.

b983eaa

EventIndex: Remove some unnecessary checks if event indexing is suppo…

c48ccf9

…rted.

EventIndex: Remove the unused deleteEventIndex method.

8d7e7d0

We need to support the deletion of the event index even if it's not currently initialized, therefore the deletion ended up in the EventIndexPeg class.

EventIndex: Rework the crawler cancellation.

4a6623b

EventIndex: Fix some spelling errors.

21f00aa

EventIndex: Remove some unused variables and some trailing whitespace.

d2a9918

EventIndex: Move the event listener registration into the EventIndex …

6017473

…class.

Lifecycle: Make the clear storage method async.

9798037

EventIndex: Hide the feature behind a labs flag.

f776bdc

EventIndex: Move the event indexing files into a separate folder.

e9df973

Fix the translations en_EN file by regenerating it.

27d1e4f

poljar requested a review from jryans November 19, 2019 13:34

jryans reviewed Nov 20, 2019

View reviewed changes

jryans approved these changes Nov 20, 2019

View reviewed changes

poljar added 2 commits November 20, 2019 12:30

EventIndex: Use property initializer style for the bound callbacks.

2f5b0a9

Settings: Fix the supportedLevels for event indexing feature.

0631faf

jryans reviewed Nov 20, 2019

View reviewed changes

EventIndex: Silence the linter complaining about missing docs.

4bd46f9

poljar merged commit 4bd46f9 into develop Nov 20, 2019

poljar deleted the poljar/seshat-pr branch June 21, 2020 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for platform specific event indexing and search #3550

Add support for platform specific event indexing and search #3550

poljar commented Oct 11, 2019

bwindels left a comment

bwindels Oct 28, 2019

poljar Oct 28, 2019

jryans Nov 6, 2019

bwindels Oct 28, 2019

bwindels Oct 28, 2019

poljar Oct 28, 2019

bwindels Oct 28, 2019

poljar Oct 28, 2019

bwindels Oct 28, 2019

poljar Oct 28, 2019

jryans Nov 6, 2019

jryans left a comment

jryans Nov 6, 2019

jryans Nov 6, 2019

jryans Nov 6, 2019

poljar Nov 12, 2019

jryans Nov 12, 2019

jryans Nov 6, 2019

jryans Nov 6, 2019

poljar commented Nov 19, 2019

jryans Nov 20, 2019

jryans left a comment

jryans Nov 20, 2019

		console.log("Request to reset timeline in room ", roomId, " viewing:", self.state.currentRoomId);
		// TODO is there a better place to plug this in

Add support for platform specific event indexing and search #3550

Add support for platform specific event indexing and search #3550

Conversation

poljar commented Oct 11, 2019

bwindels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar commented Nov 19, 2019

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment