fix: Replace Last-Modified with custom headers.

`X-Datalayer-Version` and `X-Datalayer-Reference` are now used instead
of the `Last-Modified` and `If-Unmodified-Since` headers.

`Last-Modified` is granular to the second, which led to problems with
the versionning. The new system uses timestamps instead.

This commit also changes the way versions were created. Previously,
the associated version was coming from two different places:
the last modified time from the filesystem and a `time.time()` call
done when saving the model, which could result in the two getting
out of sync.

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

@@ -677,7 +677,7 @@ U.DataLayer = L.Evented.extend({
     this._loading = true
     const [geojson, response, error] = await this.map.server.get(this._dataUrl())
     if (!error) {
-      this._last_modified = response.headers.get('last-modified')
+      this._reference_version = response.headers.get('X-Datalayer-Version')
       // FIXME: for now this property is set dynamically from backend
       // And thus it's not in the geojson file in the server
       // So do not let all options to be reset
@@ -1459,7 +1459,7 @@ U.DataLayer = L.Evented.extend({
     if (!this.isVisible()) return
     const bounds = this.layer.getBounds()
     if (bounds.isValid()) {
-      const options = {maxZoom: this.getOption("zoomTo")}
+      const options = { maxZoom: this.getOption('zoomTo') }
       this.map.fitBounds(bounds, options)
     }
   },
@@ -1577,8 +1577,8 @@ U.DataLayer = L.Evented.extend({
       map_id: this.map.options.umap_id,
       pk: this.umap_id,
     })
-    const headers = this._last_modified
+    const headers = this._reference_version
-      ? { 'If-Unmodified-Since': this._last_modified }
+      ? { 'X-Datalayer-Reference': this._reference_version }
       : {}
     await this._trySave(saveUrl, headers, formData)
     this._geojson = geojson
@@ -1596,7 +1596,7 @@ U.DataLayer = L.Evented.extend({
             label: L._('Save anyway'),
             callback: async () => {
               // Save again,
-              // but do not pass If-Unmodified-Since this time
+              // but do not pass the reference version this time
               await this._trySave(url, {}, formData)
             },
           },
@@ -1619,7 +1619,7 @@ U.DataLayer = L.Evented.extend({
         this.fromGeoJSON(data.geojson)
         delete data.geojson
       }
-      this._last_modified = response.headers.get('last-modified')
+      this._reference_version = response.headers.get('X-Datalayer-Version')
       this.setUmapId(data.id)
       this.updateOptions(data)
       this.backupOptions()

umap/templates/umap/js.html

@@ -2,7 +2,9 @@
 <script type="module"
         src="{% static 'umap/vendors/leaflet/leaflet-src.esm.js' %}"
         defer></script>
-<script type="module" src="{% static 'umap/js/modules/leaflet-configure.js' %}" defer></script>
+<script type="module"
+        src="{% static 'umap/js/modules/leaflet-configure.js' %}"
+        defer></script>
 {% if locale %}
   {% with "umap/locale/"|add:locale|add:".js" as path %}
     <script src="{% static path %}" defer></script>
@@ -44,7 +46,6 @@
 <script src="{% static 'umap/vendors/colorbrewer/colorbrewer.js' %}" defer></script>
 <script src="{% static 'umap/vendors/simple-statistics/simple-statistics.min.js' %}"
         defer></script>
-
 <script src="{% static 'umap/js/umap.core.js' %}" defer></script>
 <script src="{% static 'umap/js/umap.autocomplete.js' %}" defer></script>
 <script src="{% static 'umap/js/umap.popup.js' %}" defer></script>

umap/tests/integration/test_collaborative_editing.py

@@ -196,3 +196,72 @@ def test_empty_datalayers_can_be_merged(context, live_server, tilelayer):
         sleep(1)
 
     expect(marker_pane_p2).to_have_count(2)
+
+
+def test_same_second_edit_doesnt_conflict(context, live_server, tilelayer):
+    # Let's create a new map with an empty datalayer
+    map = MapFactory(name="collaborative editing")
+    datalayer = DataLayerFactory(map=map, edit_status=DataLayer.ANONYMOUS, data={})
+
+    # Open the created map on two pages.
+    page_one = context.new_page()
+    page_one.goto(f"{live_server.url}{map.get_absolute_url()}?edit")
+    page_two = context.new_page()
+    page_two.goto(f"{live_server.url}{map.get_absolute_url()}?edit")
+
+    save_p1 = page_one.get_by_role("button", name="Save")
+    expect(save_p1).to_be_visible()
+
+    save_p2 = page_two.get_by_role("button", name="Save")
+    expect(save_p2).to_be_visible()
+
+    # Create a point on the first map
+    create_marker_p1 = page_one.get_by_title("Draw a marker")
+    expect(create_marker_p1).to_be_visible()
+    create_marker_p1.click()
+
+    # Check no marker is present by default.
+    marker_pane_p1 = page_one.locator(".leaflet-marker-pane > div")
+    expect(marker_pane_p1).to_have_count(0)
+
+    # Click on the map, it will place a marker at the given position.
+    map_el_p1 = page_one.locator("#map")
+    map_el_p1.click(position={"x": 200, "y": 200})
+    expect(marker_pane_p1).to_have_count(1)
+
+    # And add one on the second map as well.
+    create_marker_p2 = page_two.get_by_title("Draw a marker")
+    expect(create_marker_p2).to_be_visible()
+    create_marker_p2.click()
+
+    marker_pane_p2 = page_two.locator(".leaflet-marker-pane > div")
+
+    # Click on the map, it will place a marker at the given position.
+    map_el_p2 = page_two.locator("#map")
+    map_el_p2.click(position={"x": 220, "y": 220})
+    expect(marker_pane_p2).to_have_count(1)
+
+    # Save the two tabs at the same time
+    with page_one.expect_response(DATALAYER_UPDATE):
+        save_p1.click()
+        sleep(0.2)  # Needed to avoid having multiple requests coming at the same time.
+        save_p2.click()
+
+    # Now create another marker in the first tab
+    create_marker_p1.click()
+    map_el_p1.click(position={"x": 150, "y": 150})
+    expect(marker_pane_p1).to_have_count(2)
+    with page_one.expect_response(DATALAYER_UPDATE):
+        save_p1.click()
+
+    # Should now get the other marker too
+    expect(marker_pane_p1).to_have_count(3)
+    assert DataLayer.objects.get(pk=datalayer.pk).settings == {
+        "browsable": True,
+        "displayOnLoad": True,
+        "name": "test datalayer",
+        "inCaption": True,
+        "editMode": "advanced",
+        "id": str(datalayer.pk),
+        "permissions": {"edit_status": 1},
+    }

umap/tests/test_datalayer_views.py

@@ -35,7 +35,7 @@ def test_get_with_public_mode(client, settings, datalayer, map):
     url = reverse("datalayer_view", args=(map.pk, datalayer.pk))
     response = client.get(url)
     assert response.status_code == 200
-    assert response["Last-Modified"] is not None
+    assert response["X-Datalayer-Version"] is not None
     assert response["Cache-Control"] is not None
     assert "Content-Encoding" not in response
     j = json.loads(response.content.decode())
@@ -154,48 +154,50 @@ def test_should_not_be_possible_to_delete_with_wrong_map_id_in_url(
     assert DataLayer.objects.filter(pk=datalayer.pk).exists()
 
 
-def test_optimistic_concurrency_control_with_good_last_modified(
+def test_optimistic_concurrency_control_with_good_version(
     client, datalayer, map, post_data
 ):
     map.share_status = Map.PUBLIC
     map.save()
-    # Get Last-Modified
+    # Get reference version
     url = reverse("datalayer_view", args=(map.pk, datalayer.pk))
     response = client.get(url)
-    last_modified = response["Last-Modified"]
+    reference_version = response["X-Datalayer-Version"]
     url = reverse("datalayer_update", args=(map.pk, datalayer.pk))
     client.login(username=map.owner.username, password="123123")
     name = "new name"
     post_data["name"] = "new name"
     response = client.post(
-        url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=last_modified
+        url, post_data, follow=True, HTTP_X_DATALAYER_REFERENCE=reference_version
     )
     assert response.status_code == 200
     modified_datalayer = DataLayer.objects.get(pk=datalayer.pk)
     assert modified_datalayer.name == name
 
 
-def test_optimistic_concurrency_control_with_bad_last_modified(
+def test_optimistic_concurrency_control_with_bad_version(
     client, datalayer, map, post_data
 ):
     url = reverse("datalayer_update", args=(map.pk, datalayer.pk))
     client.login(username=map.owner.username, password="123123")
     name = "new name"
     post_data["name"] = name
-    response = client.post(url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE="xxx")
+    response = client.post(
+        url, post_data, follow=True, HTTP_X_DATALAYER_REFERENCE="xxx"
+    )
     assert response.status_code == 412
     modified_datalayer = DataLayer.objects.get(pk=datalayer.pk)
     assert modified_datalayer.name != name
 
 
-def test_optimistic_concurrency_control_with_empty_last_modified(
+def test_optimistic_concurrency_control_with_empty_version(
     client, datalayer, map, post_data
 ):
     url = reverse("datalayer_update", args=(map.pk, datalayer.pk))
     client.login(username=map.owner.username, password="123123")
     name = "new name"
     post_data["name"] = name
-    response = client.post(url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=None)
+    response = client.post(url, post_data, follow=True, X_DATALAYER_REFERENCE=None)
     assert response.status_code == 200
     modified_datalayer = DataLayer.objects.get(pk=datalayer.pk)
     assert modified_datalayer.name == name
@@ -479,7 +481,7 @@ def test_optimistic_merge_both_added(client, datalayer, map, reference_data):
     assert response.status_code == 200
 
     response = client.get(reverse("datalayer_view", args=(map.pk, datalayer.pk)))
-    reference_timestamp = response["Last-Modified"]
+    reference_version = response.headers.get("X-Datalayer-Version")
 
     # Client 1 adds "Point 5, 6" to the existing data
     client1_feature = {
@@ -489,14 +491,16 @@ def test_optimistic_merge_both_added(client, datalayer, map, reference_data):
     }
     client1_data = deepcopy(reference_data)
     client1_data["features"].append(client1_feature)
-    # Sleep to change the current timestamp (used in the If-Unmodified-Since header)
+
-    time.sleep(1)
     post_data["geojson"] = SimpleUploadedFile(
         "foo.json",
         json.dumps(client1_data).encode("utf-8"),
     )
     response = client.post(
-        url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=reference_timestamp
+        url,
+        post_data,
+        follow=True,
+        headers={"X-Datalayer-Reference": reference_version},
     )
     assert response.status_code == 200
 
@@ -514,7 +518,10 @@ def test_optimistic_merge_both_added(client, datalayer, map, reference_data):
         json.dumps(client2_data).encode("utf-8"),
     )
     response = client.post(
-        url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=reference_timestamp
+        url,
+        post_data,
+        follow=True,
+        headers={"X-Datalayer-Reference": reference_version},
     )
     assert response.status_code == 200
     modified_datalayer = DataLayer.objects.get(pk=datalayer.pk)
@@ -548,20 +555,22 @@ def test_optimistic_merge_conflicting_change_raises(
     assert response.status_code == 200
 
     response = client.get(reverse("datalayer_view", args=(map.pk, datalayer.pk)))
-    reference_timestamp = response["Last-Modified"]
+
+    reference_version = response.headers.get("X-Datalayer-Version")
 
     # First client changes the first feature.
     client1_data = deepcopy(reference_data)
     client1_data["features"][0]["geometry"] = {"type": "Point", "coordinates": [5, 6]}
 
-    # Sleep to change the current timestamp (used in the If-Unmodified-Since header)
-    time.sleep(1)
     post_data["geojson"] = SimpleUploadedFile(
         "foo.json",
         json.dumps(client1_data).encode("utf-8"),
     )
     response = client.post(
-        url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=reference_timestamp
+        url,
+        post_data,
+        follow=True,
+        headers={"X-Datalayer-Reference": reference_version},
     )
     assert response.status_code == 200
 
@@ -574,7 +583,10 @@ def test_optimistic_merge_conflicting_change_raises(
         json.dumps(client2_data).encode("utf-8"),
     )
     response = client.post(
-        url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=reference_timestamp
+        url,
+        post_data,
+        follow=True,
+        headers={"X-Datalayer-Reference": reference_version},
     )
     assert response.status_code == 412
 

umap/views.py

@@ -969,20 +969,20 @@ class GZipMixin(object):
 
     @property
     def path(self):
-        return self.object.geojson.path
+        return Path(self.object.geojson.path)
 
     @property
     def gzip_path(self):
         return Path(f"{self.path}{self.EXT}")
 
-    def compute_last_modified(self, path):
+    def read_version(self, path):
-        stat = os.stat(path)
+        # Remove optional .gz, then .geojson, then return the trailing version from path.
-        return http_date(stat.st_mtime)
+        return str(path.with_suffix("").with_suffix("")).split("_")[-1]
 
     @property
-    def last_modified(self):
+    def version(self):
         # Prior to 1.3.0 we did not set gzip mtime as geojson mtime,
-        # but we switched from If-Match header to IF-Unmodified-Since
+        # but we switched from If-Match header to If-Unmodified-Since
         # and when users accepts gzip their last modified value is the gzip
         # (when umap is served by nginx and X-Accel-Redirect)
         # one, so we need to compare with that value in that case.
@@ -992,7 +992,7 @@ class GZipMixin(object):
             if self.accepts_gzip and self.gzip_path.exists()
             else self.path
         )
-        return self.compute_last_modified(path)
+        return self.read_version(path)
 
     @property
     def accepts_gzip(self):
@@ -1023,7 +1023,7 @@ class DataLayerView(GZipMixin, BaseDetailView):
             with open(path, "rb") as f:
                 # Should not be used in production!
                 response = HttpResponse(f.read(), content_type="application/geo+json")
-            response["Last-Modified"] = self.last_modified
+            response["X-Datalayer-Version"] = self.version
             response["Content-Length"] = statobj.st_size
         return response
 
@@ -1031,9 +1031,8 @@ class DataLayerView(GZipMixin, BaseDetailView):
 class DataLayerVersion(DataLayerView):
     @property
     def path(self):
-        return "{root}/{path}".format(
+        return Path(settings.MEDIA_ROOT) / self.object.get_version_path(
-            root=settings.MEDIA_ROOT,
+            self.kwargs["name"]
-            path=self.object.get_version_path(self.kwargs["name"]),
         )
 
 
@@ -1044,11 +1043,11 @@ class DataLayerCreate(FormLessEditMixin, GZipMixin, CreateView):
     def form_valid(self, form):
         form.instance.map = self.kwargs["map_inst"]
         self.object = form.save()
-        # Simple response with only metadatas (including new id)
+        # Simple response with only metadata (including new id)
         response = simple_json_response(
             **self.object.metadata(self.request.user, self.request)
         )
-        response["Last-Modified"] = self.last_modified
+        response["X-Datalayer-Version"] = self.version
         return response
 
 
@@ -1056,30 +1055,29 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView):
     model = DataLayer
     form_class = DataLayerForm
 
-    def has_been_modified_since(self, if_unmodified_since):
+    def has_changes_since(self, incoming_version):
-        return if_unmodified_since and self.last_modified != if_unmodified_since
+        return incoming_version and self.version != incoming_version
 
-    def merge(self, if_unmodified_since):
+    def merge(self, reference_version):
         """
-        Attempt to apply the incoming changes to the document the client was using, and
+        Attempt to apply the incoming changes to the reference, and then merge it
-        then merge it with the last document we have on storage.
+        with the last document we have on storage.
 
         Returns either None (if the merge failed) or the merged python GeoJSON object.
         """
 
-        # Use If-Modified-Since to find the correct version in our storage.
+        # Use the provided info to find the correct version in our storage.
         for name in self.object.get_versions():
-            path = os.path.join(settings.MEDIA_ROOT, self.object.get_version_path(name))
+            path = Path(settings.MEDIA_ROOT) / self.object.get_version_path(name)
-            if if_unmodified_since == self.compute_last_modified(path):
+            if reference_version == self.read_version(path):
                 with open(path) as f:
                     reference = json.loads(f.read())
                 break
         else:
             # If the document is not found, we can't merge.
             return None
-
         # New data received in the request.
-        entrant = json.loads(self.request.FILES["geojson"].read())
+        incoming = json.loads(self.request.FILES["geojson"].read())
 
         # Latest known version of the data.
         with open(self.path) as f:
@@ -1089,7 +1087,7 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView):
             merged_features = merge_features(
                 reference.get("features", []),
                 latest.get("features", []),
-                entrant.get("features", []),
+                incoming.get("features", []),
             )
             latest["features"] = merged_features
             return latest
@@ -1104,10 +1102,9 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView):
         if not self.object.can_edit(user=self.request.user, request=self.request):
             return HttpResponseForbidden()
 
-        ius_header = self.request.META.get("HTTP_IF_UNMODIFIED_SINCE")
+        reference_version = self.request.headers.get("X-Datalayer-Reference")
-
+        if self.has_changes_since(reference_version):
-        if self.has_been_modified_since(ius_header):
+            merged = self.merge(reference_version)
-            merged = self.merge(ius_header)
             if not merged:
                 return HttpResponse(status=412)
 
@@ -1127,8 +1124,7 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView):
             data["geojson"] = json.loads(self.object.geojson.read().decode())
             self.request.session["needs_reload"] = False
         response = simple_json_response(**data)
-
+        response["X-Datalayer-Version"] = self.version
-        response["Last-Modified"] = self.last_modified
         return response