[keyserver] Return object from fetchEntryInfosByID
ClosedPublic
Actions

Authored by tomek on Aug 23 2023, 6:33 AM.

Details

Reviewers

kamil
bartek
inka

Commits

rCOMMc9f524a4dfb8: [keyserver] Return object from fetchEntryInfosByID

Summary

All the others state syncs use objects instead of array when fetching entries, so it makes sense to make this consistent. Additionally, it is more efficient to access by id instead of iterating over array.

https://linear.app/comm/issue/ENG-4654/unify-interfaces-in-session-utils

Depends on D8925

Test Plan

Modify a couple of calendar entries in the keyserver db and check if the state is correctly fixed.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Aug 23 2023, 6:33 AM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 23 2023, 6:33 AM

Harbormaster completed remote builds in B22037: Diff 30253.Aug 23 2023, 6:59 AM

tomek requested review of this revision.Aug 23 2023, 6:59 AM

tomek added a child revision: D8927: [keyserver] Introduce keyserver state sync spec.Aug 23 2023, 7:13 AM

bartek added inline comments.Aug 23 2023, 12:43 PM

keyserver/src/fetchers/entry-fetchers.js
42 ↗	(On Diff #30253)	Should we return null as we did before? I don't know how much we care about `null` vs `undefined` but it can cause surprises.

kamil accepted this revision.Aug 24 2023, 3:05 PM

kamil added inline comments.

keyserver/src/fetchers/entry-fetchers.js
42 ↗	(On Diff #30253)	I think it shouldn't cause any harm when we use `undefined`, but in fact `null` seems more appropriate logically, to express there is no entry with given ID, rather than there is something not defined yet. But I think we are mixing this approach a lot in the codebase 😄
81 ↗	(On Diff #30253)	I assume it will converted and will work, but isn't this more correct?

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

tomek added inline comments.Aug 30 2023, 6:43 AM

keyserver/src/fetchers/entry-fetchers.js
42 ↗	(On Diff #30253)	Agree, will change it to null
81 ↗	(On Diff #30253)	Interestingly it seems to be working correctly, but probably shouldn't. Obviously, converting to string is the right thing to do.