search

getodk / central

ODK Central is a server that is easy to use, very fast, and stuffed with features that make data collection easier. Contribute and make the world a better place! ✨🗄✨
https://docs.getodk.org/central-intro/
Apache License 2.0
127 stars 157 forks source link

Update entity conflict logic for additional case #805

Open matthew-white opened 4 days ago

matthew-white commented 4 days ago

Right now, there are exactly three situations in which a new entity version is supposed to be marked as a conflict. Two of them are new as of v2024.3.

  1. The base version of an entity update is different from the current version on the server. This is the classic case of an entity conflict.
  2. An offline entity create was delayed, then was later applied as an update. In that case, the update should be marked as a soft conflict.
    • Introduced in #702
  3. An entity update was made on an offline branch, but there was another update made from outside the branch between the trunk version of the branch and when the new update from the branch was processed. In other words, the new version is not contiguous with its trunk version: there was an interleaving update. In that case, the new version from the branch is marked as a soft conflict. The most interesting example of this is when the new version immediately follows its base version (i.e., the previous update from the branch). If the new version doesn't immediately follow its base version (if the new version is not the subsequent version), that's already a conflict due to rule 1 above.
    • Introduced in #698

Note one important situation in which a new entity version is not marked as a conflict. If an offline update is force-applied from the backlog, that is not considered to be a conflict unless rule 3 applies. For example, let's say I create an entity, then make two offline updates to the entity. I submit both updates, but the first is delayed, so the second ends up being force-applied from the backlog and processed before the first. Assuming that there was no other update from another source, the second update should not be marked as a conflict. Its base version according to the server is v1, and it immediately follows it (1 ✅); it was not a create applied as an update (2 ✅); and it is contiguous with its trunk version (3 ✅). If the first update does eventually arrive, the first update will be marked as a conflict due to rule 1. However, the second update (the one processed first) is not considered to be a conflict.

This issue has to do with a related situation in which a new entity version is marked as a soft conflict when it is not intended to be. This situation came up as part of #698:

- Create an entity (v1). - Download the entity to Collect, then create 4 offline updates to the entity. Don't send the updates yet. - Send the 2nd update first. Wait 5 days or force-process the update immediately. The update will be v2 on the server. It is not a conflict: its exact base version was not found, so it was applied on top of the latest version. - Now send the 1st update. It will be immediately applied, as its base version (v1) is on the server. The update will be v3 on the server. It is a conflict because there was another version between it and its base version. - Now send the 4th update. Wait 5 days or force-process the update immediately. The update will be v4 on the server. It is not a conflict, for the same reasons as v2. In this example, even though v3 and v4 are from the same branch, and v3 is a conflict, we do not expect v4 to be a conflict. v2, v3, and v4 are all contiguous with the trunk version (v1): there's no conflict with some interweaving update from another source, as in the first example above. To summarize: if the versions from an offline branch were force-processed out of order, then even if all the versions are contiguous with their trunk version, it may be the case that an earlier version from the branch is a conflict while a later version is not.

According to the three rules above, v4 should not be marked as a conflict. However, right now, it is being marked as a conflict. That can be seen in the Backend test here.

matthew-white commented 4 days ago

@ktuite and I explored this issue a little while reviewing getodk/central-backend#1187. We ended up suspecting that there was an issue with these lines of code:

add(version) {
  if (version.baseVersion === this.lastContiguousWithTrunk &&
    version.version === version.baseVersion + 1)
    this.lastContiguousWithTrunk = version.version;
}

When this method is called for v3 (the 1st update) in the scenario above, version.baseVersion will be 1, which differs from this.lastContiguousWithTrunk, which will be 2. In other words, add() considers the branch to have been interrupted. What's actually happening is that all the updates are contiguous with the trunk version, but the updates have been applied out of order. In other words, in the normal case where updates are applied in order, add() can tell whether a branch is interrupted, but in the case where an update is force-applied from the backlog, add() breaks down.

add() was originally written for Frontend and was adapted for Backend. If we patch it in Backend, we should probably also patch it in Frontend: there may be a problem there as well. In Frontend, I think lastContiguousWithTrunk is just used to show the accuracy warning ("In this case, the author’s view may not be accurate.").