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
Unknown Object (File)
Wed, Nov 13, 5:10 AM
Unknown Object (File)
Wed, Nov 13, 5:10 AM
Unknown Object (File)
Wed, Nov 13, 5:10 AM
Unknown Object (File)
Wed, Nov 13, 5:10 AM
Unknown Object (File)
Wed, Nov 13, 5:10 AM
Unknown Object (File)
Thu, Nov 7, 3:45 PM
Unknown Object (File)
Wed, Nov 6, 1:54 PM
Unknown Object (File)
Tue, Nov 5, 11:36 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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 ↗(On Diff #34463)

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 ↗(On Diff #34463)

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