Page MenuHomePhabricator
Differential D8926

[keyserver] Return object from fetchEntryInfosByID
ClosedPublic
Actions

Authored by tomek on Aug 23 2023, 6:33 AM.
Tags
None
Referenced Files
F3271753: D8926.diff
Sat, Nov 16, 5:19 AM
Unknown Object (File)
Thu, Nov 14, 7:50 PM
Unknown Object (File)
Sun, Nov 10, 3:59 PM
Unknown Object (File)
Sun, Nov 10, 3:59 PM
Unknown Object (File)
Sun, Nov 10, 3:59 PM
Unknown Object (File)
Sun, Nov 10, 3:58 PM
Unknown Object (File)
Tue, Nov 5, 2:12 AM
Unknown Object (File)
Tue, Oct 29, 5:06 PM
View All Files
Subscribers
ashoat

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
No Lint Coverage
Unit
No Test Coverage

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.

tomek updated this revision to Diff 30553.Aug 30 2023, 7:06 AM

Convert to string explicitly and return null if the entry isn't found

Harbormaster completed remote builds in B22256: Diff 30553.Aug 30 2023, 7:35 AM
Closed by commit rCOMMc9f524a4dfb8: [keyserver] Return object from fetchEntryInfosByID (authored by tomek). · Explain WhyAug 30 2023, 9:16 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMMc9f524a4dfb8: [keyserver] Return object from fetchEntryInfosByID.

Revision Contents
Changeset List

PathSize
keyserver/
src/
creators/
8 lines
fetchers/
16 lines
socket/
14 lines
lib/
types/
3 lines

Diff 30553

View Options

keyserver/src/creators/update-creator.js

Loading...
View Options

keyserver/src/fetchers/entry-fetchers.js

Loading...
View Options

keyserver/src/socket/session-utils.js

Loading...
View Options

lib/types/entry-types.js

Loading...