[keyserver] Introduce keyserver state sync spec
ClosedPublic
Actions

Authored by tomek on Aug 23 2023, 7:13 AM.

Details

Reviewers

kamil
bartek
inka

Commits

rCOMM85ed6a9b0696: [keyserver] Introduce keyserver state sync spec

Summary

Fetch functions require viewer as an argument, but Viewer is defined in keyserver - that's why we need to define this spec outside of lib.
Currently, we're accessing serverStateSyncSpecs props directly, but later in the stack this is going to be replaced by iterating over the specs.

https://linear.app/comm/issue/ENG-4631/migrate-checkstate-from-session-utils-to-a-spec

Depends on D8926

Test Plan

Modify threads, entries and users in the db and check if state check fixes client state. Also check if after fixing there are no invalidKeys.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Aug 23 2023, 7:13 AM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 23 2023, 7:13 AM

tomek added inline comments.Aug 23 2023, 7:15 AM

keyserver/src/shared/state-sync/entries-state-sync-spec.js
14 ↗	(On Diff #30254)	I've moved `serverEntryInfosObject` here so that all the specs have a similar interface when hashes are computed.
keyserver/src/socket/session-utils.js
393 ↗	(On Diff #30254)	This will be also needed for other `if` branches in this function.

Looks like Phabricator went out of sync with Buildkite, cc @atul

The code is starting to look a lot better!

keyserver/src/shared/state-sync/state-sync-spec.js
10 ↗	(On Diff #30254)	could you remind me, why `calendarQuery` can not be used from `viewer` object? based on the call sites in this diff it's taken from the `viewer` anyway

This revision is now accepted and ready to land.Aug 24 2023, 3:17 PM

tomek added a child revision: D8941: [keyserver] Merge `fetchAll` and `fetchByID` functions.Aug 25 2023, 2:58 AM

bartek accepted this revision.Aug 25 2023, 7:12 AM

tomek added inline comments.Aug 30 2023, 7:12 AM

keyserver/src/shared/state-sync/state-sync-spec.js
10 ↗	(On Diff #30254)	This is a really interesting observation - I don't know why we're using this pattern. I created a task https://linear.app/comm/issue/ENG-4822/consider-using-calendarquery-from-viewer-inside-state-sync-specs for it. This will be a big simplification.