Fix removal of deprecated patch call adjacent to other patch #587
Conversation
|
I admit not understanding the problem well enough (but my understanding may not be required, but if my understanding is required, problematic TS/Python code on patch change could help me). I'll defer review.
Sounds like this is a harmless change simply because you're loosening restrictions, correct? |
Yes. I wouldn't call it completely harmless. The potential harm is in making the reason something has gone wrong somewhat less obvious - but it's not harmful from a "bad things will happen" perspective. |
| }; | ||
| let patch_details = event.get_patch_marker_details(); | ||
| fn skip_one_or_two_events(next_event: Option<&HistoryEvent>) -> Result<EventHandlingOutcome> { | ||
| // Also ignore the subsequent upsert event if present |
There was a problem hiding this comment.
Not introduced in this PR, but...
Are there cases where upsert search attributes follows a patch but the patch machine is not the one that issued that command?
I think it's only possible if a patch was emitted before we started emitting SAs on patches but still worth supporting for correctness.
There was a problem hiding this comment.
It's probably technically possible, but, this check looks to see for the specific attribute key for versioning. So this could only happen if the user explicitly did that themselves somehow, which: A) would be super unlikely and B) means I have no way to differentiate anyways so nothing can be done.
(cherry picked from commit 8411990)
3 participants
What was changed
Permit the removal of deprecated patch calls which are immediately adjacent to another patch call.
This was done by allowing patch-id-mistmaches in when checking a patch event against a patch machine, if the event is deprecated. I do not see this as a risky change primarily because nondeterminism checking of this kind purely serves as an "early warning" against other subsequent nondeterminism which would happen as a result of taking the incorrect branch. So, although this change technically means you could swap out one deprecated patch for another and there would be no error on that line - you'd still likely encounter another one later as if you hadn't used patching at all.
I think this is an acceptable tradeoff, since I cannot think of any other way to make this work and at least a couple people have run into it.
Why?
Without this, it can be quite challenging to remove deprecated patches in some circumstances.
Checklist
Closes [Feature Request] Permit the removal + change of adjacent deprecated patches #535
How was this tested:
Added IT, existing tests.
Any docs updates needed?