Optimistic conflicts resolution mecanism

Basically:
- when there is a conflict (i.e. someone try to save with an old ETag)
- we compare new version with its reference version and with the latest known
- if we see only simple case (features added both side, removal on one side…),
  then we merge

umap/static/umap/js/umap.layer.js

@@ -1157,6 +1157,12 @@ L.U.DataLayer = L.Evented.extend({
     this.map.post(this.getSaveUrl(), {
       data: formData,
       callback: function (data, response) {
+        // Response contains geojson only if save has conflicted and conflicts have
+        // been resolved. So we need to reload to get extra data (saved from someone else)
+        if (data.geojson) {
+          this.clear()
+          this.fromGeoJSON(data.geojson)
+        }
         this._geojson = geojson
         this._last_modified = response.getResponseHeader('Last-Modified')
         this.setUmapId(data.id)

umap/tests/test_datalayer_views.py

@@ -1,7 +1,9 @@
 import json
+import time
 
 import pytest
 from django.core.files.base import ContentFile
+from django.core.files.uploadedfile import SimpleUploadedFile
 from django.urls import reverse
 
 from umap.models import DataLayer, Map
@@ -17,7 +19,10 @@ def post_data():
         "name": "name",
         "display_on_load": True,
         "rank": 0,
-        "geojson": '{"type":"FeatureCollection","features":[{"type":"Feature","geometry":{"type":"Polygon","coordinates":[[[-3.1640625,53.014783245859235],[-3.1640625,51.86292391360244],[-0.50537109375,51.385495069223204],[1.16455078125,52.38901106223456],[-0.41748046875,53.91728101547621],[-2.109375,53.85252660044951],[-3.1640625,53.014783245859235]]]},"properties":{"_umap_options":{},"name":"Ho god, sounds like a polygouine"}},{"type":"Feature","geometry":{"type":"LineString","coordinates":[[1.8017578124999998,51.16556659836182],[-0.48339843749999994,49.710272582105695],[-3.1640625,50.0923932109388],[-5.60302734375,51.998410382390325]]},"properties":{"_umap_options":{},"name":"Light line"}},{"type":"Feature","geometry":{"type":"Point","coordinates":[0.63720703125,51.15178610143037]},"properties":{"_umap_options":{},"name":"marker he"}}],"_umap_options":{"displayOnLoad":true,"name":"new name","id":1668,"remoteData":{},"color":"LightSeaGreen","description":"test"}}',
+        "geojson": SimpleUploadedFile(
+            "name.json",
+            b'{"type":"FeatureCollection","features":[{"type":"Feature","geometry":{"type":"Polygon","coordinates":[[[-3.1640625,53.014783245859235],[-3.1640625,51.86292391360244],[-0.50537109375,51.385495069223204],[1.16455078125,52.38901106223456],[-0.41748046875,53.91728101547621],[-2.109375,53.85252660044951],[-3.1640625,53.014783245859235]]]},"properties":{"_umap_options":{},"name":"Ho god, sounds like a polygouine"}},{"type":"Feature","geometry":{"type":"LineString","coordinates":[[1.8017578124999998,51.16556659836182],[-0.48339843749999994,49.710272582105695],[-3.1640625,50.0923932109388],[-5.60302734375,51.998410382390325]]},"properties":{"_umap_options":{},"name":"Light line"}},{"type":"Feature","geometry":{"type":"Point","coordinates":[0.63720703125,51.15178610143037]},"properties":{"_umap_options":{},"name":"marker he"}}],"_umap_options":{"displayOnLoad":true,"name":"new name","id":1668,"remoteData":{},"color":"LightSeaGreen","description":"test"}}',
+        ),
     }
 
 
@@ -170,3 +175,57 @@ def test_update_readonly(client, datalayer, map, post_data, settings):
     client.login(username=map.owner.username, password="123123")
     response = client.post(url, post_data, follow=True)
     assert response.status_code == 403
+
+
+def test_optimistic_merge(client, datalayer, map):
+    url = reverse("datalayer_update", args=(map.pk, datalayer.pk))
+    client.login(username=map.owner.username, password="123123")
+    # Save once
+    post_data = {
+        "name": "name",
+        "display_on_load": True,
+        "rank": 0,
+        "geojson": SimpleUploadedFile(
+            "foo.json",
+            b'{"type":"FeatureCollection","features":[{"type":"Feature","geometry":{"type":"Point","coordinates":[-1,2]},"properties":{"_umap_options":{},"name":"foo"}},{"type":"Feature","geometry":{"type":"LineString","coordinates": [2,3]},"properties":{"_umap_options":{},"name":"bar"}},{"type":"Feature","geometry":{"type":"Point","coordinates":[3,4]},"properties":{"_umap_options":{},"name":"marker"}}],"_umap_options":{"displayOnLoad":true,"name":"new name","id":1668,"remoteData":{},"color":"LightSeaGreen","description":"test"}}',
+        ),
+    }
+    response = client.post(url, post_data, follow=True)
+    assert response.status_code == 200
+    # Get reference last_modified
+    response = client.get(reverse("datalayer_view", args=(datalayer.pk,)))
+    last_modified = response["Last-Modified"]
+    # Pretend someone else is adding one data
+    time.sleep(1)
+    post_data["geojson"] = SimpleUploadedFile(
+        "foo.json",
+        b'{"type":"FeatureCollection","features":[{"type":"Feature","geometry":{"type":"Point","coordinates":[-1,2]},"properties":{"_umap_options":{},"name":"foo"}},{"type":"Feature","geometry":{"type":"LineString","coordinates": [2,3]},"properties":{"_umap_options":{},"name":"bar"}},{"type":"Feature","geometry":{"type":"Point","coordinates":[3,4]},"properties":{"_umap_options":{},"name":"marker"}},{"type":"Feature","geometry":{"type":"Point","coordinates":[3,4]},"properties":{"_umap_options":{},"name":"new from someone else"}}],"_umap_options":{"displayOnLoad":true,"name":"new name","id":1668,"remoteData":{},"color":"LightSeaGreen","description":"test"}}',
+    )
+    response = client.post(
+        url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=last_modified
+    )
+    assert response.status_code == 200
+    # Now save a new version adding data to the reference
+    post_data["geojson"] = SimpleUploadedFile(
+        "foo.json",
+        b'{"type":"FeatureCollection","features":[{"type":"Feature","geometry":{"type":"Point","coordinates":[-1,2]},"properties":{"_umap_options":{},"name":"foo"}},{"type":"Feature","geometry":{"type":"LineString","coordinates": [2,3]},"properties":{"_umap_options":{},"name":"bar"}},{"type":"Feature","geometry":{"type":"Point","coordinates":[3,4]},"properties":{"_umap_options":{},"name":"marker"}},{"type":"Feature","geometry":{"type":"Point","coordinates":[3,4]},"properties":{"_umap_options":{},"name":"new from us"}}],"_umap_options":{"displayOnLoad":true,"name":"new name","id":1668,"remoteData":{},"color":"LightSeaGreen","description":"test"}}',
+    )
+    response = client.post(
+        url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=last_modified
+    )
+    assert response.status_code == 200
+    modified_datalayer = DataLayer.objects.get(pk=datalayer.pk)
+    assert modified_datalayer.geojson.read().decode() == (
+        '{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {'
+        '"type": "Point", "coordinates": [-1, 2]}, "properties": {"_umap_options": {}'
+        ', "name": "foo"}}, {"type": "Feature", "geometry": {"type": "LineString", "c'
+        'oordinates": [2, 3]}, "properties": {"_umap_options": {}, "name": "bar"}}, {'
+        '"type": "Feature", "geometry": {"type": "Point", "coordinates": [3, 4]}, "pr'
+        'operties": {"_umap_options": {}, "name": "marker"}}, {"type": "Feature", "ge'
+        'ometry": {"type": "Point", "coordinates": [3, 4]}, "properties": {"_umap_opt'
+        'ions": {}, "name": "new from someone else"}}, {"type": "Feature", "geometry"'
+        ': {"type": "Point", "coordinates": [3, 4]}, "properties": {"_umap_options": '
+        '{}, "name": "new from us"}}], "_umap_options": {"displayOnLoad": true, "name'
+        '": "new name", "id": 1668, "remoteData": {}, "color": "LightSeaGreen", "desc'
+        'ription": "test"}}'
+    )

umap/tests/test_merge_conflicts.py

@@ -0,0 +1,61 @@
+from umap.utils import merge_conflicts
+
+
+def test_adding_one_element():
+    assert merge_conflicts(["A", "B"], ["A", "B", "C"], ["A", "B", "D"]) == [
+        "A",
+        "B",
+        "C",
+        "D",
+    ]
+
+
+def test_adding_elements():
+    assert merge_conflicts(["A", "B"], ["A", "B", "C", "D"], ["A", "B", "E", "F"]) == [
+        "A",
+        "B",
+        "C",
+        "D",
+        "E",
+        "F",
+    ]
+    # Order does not count
+    assert merge_conflicts(["A", "B"], ["B", "C", "D", "A"], ["A", "B", "E", "F"]) == [
+        "B",
+        "C",
+        "D",
+        "A",
+        "E",
+        "F",
+    ]
+
+
+def test_adding_one_removing_one():
+    assert merge_conflicts(["A", "B"], ["A", "C"], ["A", "B", "D"]) == [
+        "A",
+        "C",
+        "D",
+    ]
+
+
+def test_removing_same_element():
+    # No added element (otherwise we cannot know if "new" elements are old modified
+    # or old removed and new added).
+    assert merge_conflicts(["A", "B", "C"], ["A", "B"], ["A", "B"]) == [
+        "A",
+        "B",
+    ]
+
+
+def test_removing_changed_element():
+    assert merge_conflicts(["A", "B"], ["A", "C"], ["A"]) is False
+
+
+def test_changing_removed_element():
+    assert merge_conflicts(["A", "B"], ["A"], ["A", "C"]) is False
+
+
+def test_changing_same_element():
+    assert merge_conflicts(["A", "B"], ["A", "D"], ["A", "C"]) is False
+    # Order does not count
+    assert merge_conflicts(["A", "B", "C"], ["B", "D", "A"], ["A", "E", "B"]) is False

umap/utils.py

@@ -117,3 +117,38 @@ def gzip_file(from_path, to_path):
 
 def is_ajax(request):
     return request.headers.get("x-requested-with") == "XMLHttpRequest"
+
+
+def merge_conflicts(reference, latest, entrant):
+
+    # Just in case (eg. both removed the same element, or changed only metadatas)
+    if latest == entrant:
+        return latest
+
+    # Remove common features between entrant and reference versions (unchanged ones).
+    for feature in reference[:]:
+        for other in entrant[:]:
+            if feature == other:
+                entrant.remove(feature)
+                reference.remove(feature)
+                break
+
+    # Now make sure remaining features are still in latest version
+    for feature in reference:
+        found = False
+        for other in latest:
+            if other == feature:
+                found = True
+                break
+        if not found:
+            # We cannot distinguish the case where both deleted the same
+            # element and added others, or where both modified the same
+            # element, so let's raise a conflict by caution.
+            return False
+
+    # We can merge.
+    for feature in reference:
+        latest.remove(feature)
+    for feature in entrant:
+        latest.append(feature)
+    return latest

umap/views.py

@@ -3,6 +3,7 @@
 import os
 import re
 import socket
+from io import BytesIO
 from pathlib import Path
 
 from django.conf import settings
@@ -46,7 +47,7 @@
     UpdateMapPermissionsForm,
 )
 from .models import DataLayer, Licence, Map, Pictogram, TileLayer
-from .utils import get_uri_template, gzip_file, is_ajax
+from .utils import get_uri_template, gzip_file, is_ajax, merge_conflicts
 
 try:
     # python3
@@ -684,7 +685,10 @@ def path(self):
 
     @property
     def last_modified(self):
-        stat = os.stat(self.path)
+        return self.compute_last_modified(self.path)
+
+    def compute_last_modified(self, path):
+        stat = os.stat(path)
         return http_date(stat.st_mtime)
 
 
@@ -749,7 +753,11 @@ def form_valid(self, form):
         self.object = form.save()
         # Simple response with only metadatas (client should not reload all data
         # on save)
-        response = simple_json_response(**self.object.metadata)
+        data = {**self.object.metadata}
+        if self.request.session.get("needs_reload"):
+            data["geojson"] = json.loads(self.object.geojson.read().decode())
+            self.request.session["needs_reload"] = False
+        response = simple_json_response(**data)
         response["Last-Modified"] = self.last_modified
         return response
 
@@ -762,13 +770,51 @@ def is_unmodified(self):
                 modified = False
         return modified
 
+    def merge(self):
+        """Try to merge conflicts."""
+
+        # Reference data source of the edition.
+        modified_at = self.request.META.get("HTTP_IF_UNMODIFIED_SINCE")
+
+        for name in self.object.get_versions():
+            path = os.path.join(settings.MEDIA_ROOT, self.object.get_version_path(name))
+            if modified_at == self.compute_last_modified(path):
+                with open(path) as f:
+                    reference = json.loads(f.read())
+                break
+        else:
+            # No reference version found, can't merge.
+            return False
+
+        # New data received in the request.
+        entrant = json.loads(self.request.FILES["geojson"].read())
+
+        # Latest known version of the data.
+        with open(self.path) as f:
+            latest = json.loads(f.read())
+
+        merge = merge_conflicts(
+            reference["features"], latest["features"], entrant["features"]
+        )
+        if merge is False:
+            return merge
+        latest["features"] = merge
+
+        # Update request data.
+        self.request.FILES["geojson"].file = BytesIO(json.dumps(latest).encode("utf-8"))
+
+        return True
+
     def post(self, request, *args, **kwargs):
         self.object = self.get_object()
         if self.object.map != self.kwargs["map_inst"]:
             return HttpResponseForbidden()
         if not self.is_unmodified():
-            return HttpResponse(status=412)
-        return super(DataLayerUpdate, self).post(request, *args, **kwargs)
+            if not self.merge():
+                return HttpResponse(status=412)
+            # We merged, let's ask the frontend to reload data.
+            self.request.session["needs_reload"] = True
+        return super().post(request, *args, **kwargs)
 
 
 class DataLayerDelete(DeleteView):