Include resource version along with the SquashedLocalChange in the FhirEngine.syncUpload.

[aditya-07]

As I've been going through the current implementation of server sync, and thinking about approaches to peer-to-peer sync, I've come across a potential pitfall which I think needs to be highlighted.

The current FHIR engine implementation tracks local changes using JSON Patches, and synchronises these with the server using the FHIR patch interaction.

I've written up the following scenario to try and illustrate the issue:

---

When starting with this example patient:

{
  "resourceType": "Patient",
  "id": "1",
  "meta": {
    "versionId": "1"
  },
  "telecom": [
    {
      "system": "phone",
      "value": "1111111111",
      "use": "home"
    },
    {
      "system": "phone",
      "value": "2222222222",
      "use": "work"
    },
    {
      "system": "email",
      "value": "test@example.com"
    }
  ]
}

If one client changes the patient's work phone number it results in the following patch:

[
  {
    "op": "replace",
    "path": "/telecom/1/value",
    "value": "3333333333"
  }
]

And if a different client removes the patient's home phone number it results in the following patch:

[
  {
    "op": "remove",
    "path": "/telecom/0"
  }
]

Then there are two possible outcomes depending on the order in which the patches are synced to the server or from peer to peer.

If they are synced in the order listed above, then the result is what we'd expect:

(The home phone number has been removed, and the work phone number has been updated.)

{
  "resourceType": "Patient",
  "id": "1",
  "meta": {
    "versionId": "3"
  },
  "telecom": [
    {
      "system": "phone",
      "value": "3333333333",
      "use": "work"
    },
    {
      "system": "email",
      "value": "test@example.com"
    }
  ]
}

But if they're synced in the opposite order, then we end up with an invalid result:

(The home phone number has been removed, but the value of the email address was updated instead of that of the work phone number.)

{
  "resourceType": "Patient",
  "id": "1",
  "meta": {
    "versionId": "3"
  },
  "telecom": [
    {
      "system": "phone",
      "value": "2222222222",
      "use": "work"
    },
    {
      "system": "email",
      "value": "3333333333"
    }
  ]
}

It does not matter when each of the changes was made, only the order in which they were synced.

---

To avoid ending up with resources in unexpected states, patches need to be applied conditionally based on the version of the resource which each client was making changes to. This is pointed out in the FHIR docs:

Processing PATCH operations may be very version sensitive. For this reason, servers SHALL support conditional PATCH, which works exactly the same as specified for update in Concurrency Management. Clients SHOULD always consider using version specific PATCH operations so that inappropriate actions are not executed.

Doing this, however, introduces the possibility of conflicts, and the need for conflict resolution. It also negates some of the benefits I'd perceived in using PATCH over PUT.

I've read through the discussion in #472 relating to conflicts where the point was made that conflicts are rare and can be dealt with operationally. I agree with the decision there, but as pointed out above - without special consideration by implementers, the current API allows for implementations which can result in data corruption rather than conflicts. At the moment neither the reference app nor the other implementations which I've looked at take this behaviour into consideration. So I see this as a bit of a footgun, and I'm wondering if there's something which can be done to guide implementers into the pit of success.

Is there a good place to document this? And should the reference app be updated to apply optimistic locking as a good practice?

Any other thoughts are welcome too since this is something which needs to be considered in the implementation of P2P sync.

Originally posted by @bausmeier in #806

[f-odhiambo]

Hi @aditya-07 This looks like a compliment/enhancement to #1091 . Correct ?