(Federation guid optimization) Change processing works incorrectly for reverse synchronization

[KyryloVolotovskyi]

Describe the bug I think the issue is related to the rule which states that the starting changeset index of the source IModel in the change processing workflow must be one higher than the changeset index used for the previous transformation. Consider a scenario where we create an IModel A and introduce an element into it. We perform a transformation, such as FilterByView, to generate IModel B, which includes this element. Currently, both IModels contain the element. Subsequently, we remove the element from IModel B, push the changeset, and proceed with the reverse synchronization (merge) process. The anticipated outcome is the removal of the element in IModel A following the merge. However, currently, the element remains preserved after the merge. This is likely because during changeset processing, it is observed that the element was added and subsequently deleted, resulting in no actual changes upon analyzing the modifications.

To Reproduce The reproduction can be found in the test added here.

Expected behavior Element is removed from IModel A after reverse synchronization.

[MichaelBelousov]

This looks like #93 might fix it

EDIT: it doesn't

[MichaelBelousov]

This might be a regression because it works on main, but I'm not sure I'd consider it a bug. It's a misuage of the API.

Your test case does:

1. create empty master
2. create empty branch (no initialize provenance but we don't have to anyway)
3. update master
4. filter-by-view-transform master elements to branch, note that this is not a synchronization of changes because you use processAll instead of processChanges
5. delete the newly added element that came from master (without synchronization)
6. reverse synchronize your edit to branch back into master

Because you never established a "synchronized" relationship between the elements, the transformer skips the add+delete that occurs purely on the branch.

Simply changing the processAll to processChanges makes this pass.

[MichaelBelousov]

I believe the only reason this worked on main was due to the previous "brute force deletion" check, which we dropped in the federation-guid-branch for being a confusing feature that is difficult to support (especially after the new changes) and slow by design.

Is this somehow important to your workflows? Can you not switch to properly synchronizing those kinds of changes? If this way works for you, please close this issue.

EDIT: turns out reverse synchronization disables the brute force detect deletes, so I'm not actually sure why this worked on main but will consider investigating if you provide a strong reason this is a necessary behavior. It might have worked because processChanges without a specified starting changeset only synchronizes the latest changeset, which means it wouldn't have seen the "add" changeset, just the latest "delete" changeset, and deleted the element, but I'm not entirely sure.

[ViliusRuskys]

This worked on main and doesn't work now, because right now we pass an already processed changeset to the processChanges as startChangesetId. Because we started passing startchangeset.index-1, it will now see the change as +1Element and -1Element and skip the deletion of the element, where previously it only saw the -1Element.

Currently our service executes all initial transformations using processAll. We do not know beforehand if the user wants to use that target as a branch or not. I don't get why invoking processAll has to break this workflow. Can't we initialize the changesetData on processAll as well? Changing how the initial transformation should be run based on the user intended workflow seems hacky to me.

[MichaelBelousov]

After discussing, this usage is technically unsupported and I will close this issue.

Only processChanges knows (now) how to skip synchronization changesets correctly, the previous behavior (currently on main) requires you to figure out how to do it yourself, and the provided reproduction included flagrantly disregarding a changeset while syncing.

In our call, you demonstrated you have a workflow where you were ignoring a specific changeset because it was part of branch initialization to you, but the new logic isn't aware of that, since processAll is inherently raw data manipulation, unsynchronized, and not possible to track without user knowledge. The only allowed processAll usage is the initial creation of branch provenance on exact cloned iModels.

The supported workflow is to filter your master into an intermediate iModel, then use that as the seed/version0 for the branch you create. That way you can use the normal provenance initialization and processChanges without every needing processAll which cannot be tracked.

If you wanted to, but it sounds error-prone, you could implement a custom transformer that updates the synchronization version (see IModelTransformer._updateSynchronizationVersion) during a processAll call, which could get the behavior you're looking for, and should be equivalent to the previous behavior of main.