Page MenuHomePhabricator

[lib] Update `convertThreadStoreThreadInfosToNewIDSchema` migration to ensure `rawThreadInfos` are NOT minimally encoded
ClosedPublic

Authored by atul on Dec 10 2023, 3:01 PM.
Tags
None
Referenced Files
F3158228: D10283.id34674.diff
Tue, Nov 5, 9:39 PM
Unknown Object (File)
Mon, Oct 28, 6:38 AM
Unknown Object (File)
Mon, Oct 28, 6:38 AM
Unknown Object (File)
Mon, Oct 28, 6:38 AM
Unknown Object (File)
Mon, Oct 28, 6:37 AM
Unknown Object (File)
Mon, Oct 28, 6:37 AM
Unknown Object (File)
Tue, Oct 15, 10:25 PM
Unknown Object (File)
Tue, Oct 15, 10:25 PM

Details

Summary

convertThreadStoreThreadInfosToNewIDSchema calls the utility function convertRawThreadInfoToNewIDSchema where both the input and output is any.

convertThreadStoreThreadInfosToNewIDSchema is called from createUpdateDBOpsForThreadStoreThreadInfos which expects a migrationFunc: RawThreadInfos => RawThreadInfos.

Rather than updating convertThreadStoreThreadInfosToNewIDSchema and all of those re-usable migration utils to support this legacy migration, I typed convertThreadStoreThreadInfosToNewIDSchema as accepting RawThreadInfos and have an invariant that enforces that we'll never actually pass a minimallyEncodedRawThreadInfo through this function.

NOTE: I think this might be one of the more "controversial" changes I'm making to flow issues. It's kind of a lazy approach since I'm assuming that it's reasonable to assume convertRawThreadInfoToNewIDSchema won't be used in new migrations... so we're "lying" to flow by saying it accepts MinimallyEncodedRawThreadInfos and then immediately failing with an invariant if one is actually passed through. I still think this is cleaner than some of the other refactoring approaches I tried which required more involved changes to eg createUpdateDBOpsForThreadStoreThreadInfos. Would love to hear thoughts from @ashoat, @tomek, @michal if there are any approaches that would be better.

[skip-ci]


Depends on D10281

Test Plan

Flow/CI/etc. I think in an ideal world we'd have unit tests for migrations to ensure that changes to the utility functions we consume in them don't lead to older migrations changing, but for now I just thought through it and read carefeully.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 24929
Build 216919: arc lint + arc unit

Event Timeline

atul requested review of this revision.Dec 10 2023, 3:01 PM
atul created this revision.

I think this is fine

lib/utils/migration-utils.js
78

Can you update this to "threadInfo at the time of new ID schema migration should not be minimallyEncoded"?

This revision is now accepted and ready to land.Dec 11 2023, 11:16 AM
lib/utils/migration-utils.js
78

Yeah, shortened it a bit to fit under 80 character length, let me know if you want me to preserve original text and split on multiple lines or whatever:

2a9dc0.png (790×1 px, 155 KB)

address feedback: more specific invariant message