diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js
index cc05a5c1..8624597c 100644
--- a/umap/static/umap/js/umap.layer.js
+++ b/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
- ? { 'If-Unmodified-Since': this._last_modified }
+ const headers = this._reference_version
+ ? { '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()
diff --git a/umap/templates/umap/js.html b/umap/templates/umap/js.html
index 86ea502e..cc78ea9f 100644
--- a/umap/templates/umap/js.html
+++ b/umap/templates/umap/js.html
@@ -2,7 +2,9 @@
-
+
{% if locale %}
{% with "umap/locale/"|add:locale|add:".js" as path %}
@@ -44,7 +46,6 @@
-
diff --git a/umap/tests/integration/test_collaborative_editing.py b/umap/tests/integration/test_collaborative_editing.py
index 26f6f527..d6daf27d 100644
--- a/umap/tests/integration/test_collaborative_editing.py
+++ b/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},
+ }
diff --git a/umap/tests/test_datalayer_views.py b/umap/tests/test_datalayer_views.py
index 22ca9b95..39374221 100644
--- a/umap/tests/test_datalayer_views.py
+++ b/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
diff --git a/umap/views.py b/umap/views.py
index 4d1033f2..b9769599 100644
--- a/umap/views.py
+++ b/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):
- stat = os.stat(path)
- return http_date(stat.st_mtime)
+ def read_version(self, path):
+ # Remove optional .gz, then .geojson, then return the trailing version from path.
+ 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(
- root=settings.MEDIA_ROOT,
- path=self.object.get_version_path(self.kwargs["name"]),
+ return Path(settings.MEDIA_ROOT) / 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):
- return if_unmodified_since and self.last_modified != if_unmodified_since
+ def has_changes_since(self, incoming_version):
+ 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
- then merge it with the last document we have on storage.
+ Attempt to apply the incoming changes to the reference, and then merge it
+ 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))
- if if_unmodified_since == self.compute_last_modified(path):
+ path = Path(settings.MEDIA_ROOT) / self.object.get_version_path(name)
+ 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")
-
- if self.has_been_modified_since(ius_header):
- merged = self.merge(ius_header)
+ reference_version = self.request.headers.get("X-Datalayer-Reference")
+ if self.has_changes_since(reference_version):
+ merged = self.merge(reference_version)
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["Last-Modified"] = self.last_modified
+ response["X-Datalayer-Version"] = self.version
return response