Merge pull request #1666 from umap-project/almet/fix-same-second-last-modified

fix: Replace Last-Modified with custom headers

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

@@ -975,20 +975,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.
@@ -998,7 +998,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):
@@ -1029,7 +1029,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
 
@@ -1037,9 +1037,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"]),
         )
 
 
@@ -1050,11 +1049,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
 
 
@@ -1062,30 +1061,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:
@@ -1095,7 +1093,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
@@ -1110,10 +1108,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)
 
@@ -1133,8 +1130,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