Page MenuHomePhabricator

[lib] Add a function to DM operations spec checking if an operation can be applied
ClosedPublic

Authored by tomek on Jul 30 2024, 9:15 AM.
Tags
None
Referenced Files
F3366270: D12948.diff
Mon, Nov 25, 10:31 AM
Unknown Object (File)
Thu, Nov 21, 1:37 AM
Unknown Object (File)
Sat, Nov 9, 8:17 AM
Unknown Object (File)
Wed, Nov 6, 11:33 PM
Unknown Object (File)
Wed, Nov 6, 11:33 PM
Unknown Object (File)
Wed, Nov 6, 11:33 PM
Unknown Object (File)
Wed, Nov 6, 10:34 PM
Unknown Object (File)
Wed, Oct 30, 11:54 PM
Subscribers

Details

Summary

If this function returns false, we will queue the operation for later. Some logic in the processing code can be simplified as the result of this change.

https://linear.app/comm/issue/ENG-8909/modify-operations-specs-so-that-they-can-report-that-an-operation-cant

Depends on D12936

Test Plan

Tested this for one case - creating change_thread_settings update for a thread that isn't present in the store. Also tested that if the thread is in the store, the ops are being processed.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Jul 30 2024, 9:32 AM

I had previously suggested implemented a solution to this based on a tcomb validator. We'll need to specify IDs that have to be checked for collision again certain SQLite tables, and I was planning on using a tcomb validator approach for that. It seems like checking IDs for existence in certain SQLite tables is a very similar problem, and ought to be solved in a similar way.

This revision is now accepted and ready to land.Jul 31 2024, 3:00 AM

Not sure if @ashoat's comment demands changes to this diff, please make sure to address it

I had previously suggested implemented a solution to this based on a tcomb validator. We'll need to specify IDs that have to be checked for collision again certain SQLite tables, and I was planning on using a tcomb validator approach for that. It seems like checking IDs for existence in certain SQLite tables is a very similar problem, and ought to be solved in a similar way.

I'm not sure. It sounds similar, but there are important differences:

  1. If isProcessingPossible === false we don't say anything about the validity of the operation. We only know that it can't be processed now.
  2. Some specs will have a more complicated logic, e.g. add-members-spec. I'm not sure if it will be possible to express a thing like this in a validator.
  3. There are multiple possible results of canBeProcessed function - currently the only valid reason is that a thread is missing, but maybe in some new specs there will be more. Regardless, we would need a validator to return something more than just a valid / invalid status. I think we're doing some more complicated transformation with validators, e.g. while checking for the new user ID, so it can be done.

It feels to me that validation and checking if an operation can be processed are two distinct things and using validators instead of canBeProcessed will be more complicated. But I'm open to giving it a try. If we decide to do that I would rather land this diff and modify the logic later, to make sure we unblock other things (in this case the whole queueing logic).

Discussed this with @tomek in our 1:1 and we agreed to proceed with this approach for now.

I think that using validators for the most common cases makes sense, but I agree there are exceptions where we'll need to do something more complicated. For those complicated cases, I think having this canBeProcessed function on the spec makes sense

lib/types/dm-ops.js
288–291 ↗(On Diff #42973)

When I introduced this, it was intentionally not read-only. The idea is that the caller can do anything they want with the result

If there is a strong reason for making this read-only now, I would think we'd want $ReadOnlyArray instead of Array

But in that case, it's just ProcessDMOpsPayload, which is defined below

Can we revert this change?

lib/types/dm-ops.js
288–291 ↗(On Diff #42973)

The idea is that the caller can do anything they want with the result

I think that can be true for a lot of our types, except e.g. things returned from Redux and props. I'm not sure why we want to have a different approach here.

Can we revert this change?

Sure, I'll do that!

I think that can be true for a lot of our types, except e.g. things returned from Redux and props. I'm not sure why we want to have a different approach here.

I don't agree that this is a "different approach". I try to consistently use this approach, except in cases where the returned value really should not be modified.