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.
Details
Details
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| 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 | |
| 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:
| |
| 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 | |
| lib/shared/relationship-utils.test.js | ||
|---|---|---|
| 9 ↗ | (On Diff #37386) | |