Page MenuHomePhabricator

[lib] Make sure we run ID conversion on unsupportedMessageInfo
ClosedPublic

Authored by ashoat on May 3 2024, 7:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 9:21 AM
Unknown Object (File)
Fri, Nov 22, 2:10 AM
Unknown Object (File)
Thu, Nov 21, 8:39 PM
Unknown Object (File)
Mon, Nov 11, 8:42 AM
Unknown Object (File)
Mon, Nov 11, 5:28 AM
Unknown Object (File)
Sun, Nov 10, 9:58 PM
Unknown Object (File)
Sun, Nov 10, 11:41 AM
Unknown Object (File)
Fri, Nov 8, 7:18 PM
Subscribers
None

Details

Summary

When we "shim" a message type into an UNSUPPORTED message, we put the original message we are wrapping into the unsupportedMessageInfo field.

Because the validators have this field type as t.Object, no ID conversion is done on it. This means that when the client unshims the UNSUPPORTED message, it will see a message where the ID conversion logic has not been run, and the id and threadID fields are not prefixed with the keyserver ID.

This causes ENG-8038, where the message disappears from the client UI once unshimmed. This diff solves that by modifying the validators.

The change here should actually not change how "permissive" the validators are; they should continue to succeed and fail in the same scenarios.

However, by listing earlier options in the t.union, we allow validators to perform ID conversion when those options match.

I excluded validators that are only shimming for codeVersions prior to the ID conversion.

This solution is unfortunately rather hacky, and forces us to have to maintain this list whenever introducing new types to shim. I considered specifying this on the message specs, but there's no way to do that without leading to an import cycle.

Open to other solutions, but if they require substantially more work I'd prefer to land this one for now, as the current behavior on master is broken.

Depends on D11870

Test Plan

I delivered a shimmed farcaster_mutual message to a local client, and then I triggered a migration to unshim it. I confirmed that it's now visible and rendered correctly, whereas it was previously being ignored by message-reducer.js's isThreadWatched check

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat requested review of this revision.May 3 2024, 7:42 PM

I considered specifying this on the message specs, but there's no way to do that without leading to an import cycle.

It would be great if we could make a corresponding change to RawUnsupportedMessageInfo type (update unsupportedMessageInfo). Maybe we can define this type in another / new file? The problem with the current solution is that we have to remember to update the validator and our types don't protect us against forgetting about this.

This revision is now accepted and ready to land.May 6 2024, 12:56 AM

I considered specifying this on the message specs, but there's no way to do that without leading to an import cycle.

It would be great if we could make a corresponding change to RawUnsupportedMessageInfo type (update unsupportedMessageInfo). Maybe we can define this type in another / new file? The problem with the current solution is that we have to remember to update the validator and our types don't protect us against forgetting about this.

I like this idea! By having Flow print an error, we make it more likely that the developer will realize that the validators need to be updated as well.

One thing to consider is that we'll have to extend the list a little bit. I didn't include REACTION or TOGGLE_PIN in this list because they were introduced before ID conversion, and as such the clients for which they need to be shimmed don't need ID conversion. However, if we enforce this at the typesystem level, I suppose it doesn't hurt to include them in this list as well.

Maybe we can define this type in another / new file?

Assuming you suggested this because of concerns about import cycles, I don't think it's necessary. In my experience, Flow seems to have no trouble with import cycles.

Make corresponding updates to Flow types. Add TOGGLE_PIN and REACTION

Thanks for making this change!

Assuming you suggested this because of concerns about import cycles, I don't think it's necessary.

Yes, that was the reason.