Page MenuHomePhabricator
Differential D11104

[keyserver][lib] Make updates code aware that ids are strings, not numbers
ClosedPublic
Actions

Authored by inka on Feb 19 2024, 4:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 1, 8:47 AM
Unknown Object (File)
Tue, Apr 1, 8:47 AM
Unknown Object (File)
Wed, Mar 12, 1:45 PM
Unknown Object (File)
Wed, Mar 12, 3:14 AM
Unknown Object (File)
Wed, Mar 12, 1:48 AM
Unknown Object (File)
Tue, Mar 11, 4:06 PM
Unknown Object (File)
Mar 2 2025, 12:48 AM
Unknown Object (File)
Feb 22 2025, 5:22 AM
View All 98 Files
Subscribers
ashoat

Details

Reviewers
tomek
michal
Commits
rCOMM0bdd01b5fe49: [keyserver][lib] Make updates code aware that ids are strings, not numbers
Summary

issue: https://linear.app/comm/issue/ENG-6822/creating-db-fails-when-using-identity-ids
sortIDs has two branches, because the old code would sotr ids by converting them to numbers, comparing like numbers, and converting back to ids. Now that ids can be strings, other than stringified numbers, we need to compare them as strings. But this means that when the old logic would sort 99 before 100, the new logic would do the opposite.
And we need the old logic and the new logic to return the same order, because based on this order key of update is created, and based on the key we decide which updates eliminate each other.

Test Plan

Tested that sortIDs doesn't turn uuids into NaNs anymore, but correctly sorts them with other ids.
Tested that a newly created db has the key column of the updates table be a varchar(255)
Tested migrating from a db that had the key column bigint(20) - all ids got correctly translated, no errors showed up.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka created this revision.Feb 19 2024, 4:52 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFeb 19 2024, 4:52 AM
inka added a parent revision: D11075: [scripts] Create a script for configuring user credentials.Feb 19 2024, 4:52 AM
inka added inline comments.Feb 19 2024, 4:58 AM
keyserver/src/database/setup-db.js
174

Based on the fact that we put ids as the key, and user ids are varchar(255) CHARSET latin1 COLLATE latin1_bin

inka added a child revision: D11105: [keyserver] Replace ashoat with admin.Feb 19 2024, 5:03 AM
Harbormaster completed remote builds in B26962: Diff 37321.Feb 19 2024, 5:07 AM
inka requested review of this revision.Feb 19 2024, 5:07 AM
ashoat added inline comments.Feb 19 2024, 3:53 PM
lib/shared/relationship-utils.js
15

For this sorting algorithm: if a > b and b > c, is it always the case that a > c?

15–23

I don't think we should have a single generic function that handles two completely different kinds of sorts. This hurts readability at the callsite

I think there are two potential solutions here:

  1. You can introduce another function that handles sorting IDs using a lexicographical order, and then all the relevant callsites switch between the two options
  2. Perhaps better would just be to rename this function to something less generic, such as sortUserIDs
inka added inline comments.Feb 21 2024, 12:58 AM
lib/shared/relationship-utils.js
15

I don't think this is needed in any of the places we currently use this function, but I agree that since this function is called "sort", it should probably be transitive, I'll fix that

inka updated this revision to Diff 37386.Feb 21 2024, 12:58 AM

Rename sortIDs to sortUserIDs, make the relationship transitive

tomek accepted this revision.Feb 21 2024, 1:03 AM
tomek added inline comments.
lib/shared/relationship-utils.test.js
9 ↗(On Diff #37386)
This revision is now accepted and ready to land.Feb 21 2024, 1:03 AM
inka updated this revision to Diff 37387.Feb 21 2024, 1:04 AM

Fix test description

Harbormaster completed remote builds in B27019: Diff 37386.Feb 21 2024, 1:14 AM
Harbormaster completed remote builds in B27020: Diff 37387.Feb 21 2024, 1:19 AM
Closed by commit rCOMM0bdd01b5fe49: [keyserver][lib] Make updates code aware that ids are strings, not numbers (authored by inka). · Explain WhyFeb 23 2024, 4:35 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMM0bdd01b5fe49: [keyserver][lib] Make updates code aware that ids are strings, not numbers.

Revision Contents
Changeset List

PathSize
keyserver/
src/
database/
11 lines
2 lines
lib/
shared/
10 lines

Diff 37321

View Options

keyserver/src/database/migration-config.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

keyserver/src/database/setup-db.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

lib/shared/relationship-utils.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines