From 29992e10e6053f8811bf60e6c13a5c1b624f6d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexis=20M=C3=A9taireau?= Date: Thu, 29 Feb 2024 22:30:04 +0100 Subject: [PATCH] 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 | 12 ++-- umap/templates/umap/js.html | 5 +- .../integration/test_collaborative_editing.py | 69 +++++++++++++++++++ umap/tests/test_datalayer_views.py | 50 +++++++++----- umap/views.py | 56 +++++++-------- 5 files changed, 135 insertions(+), 57 deletions(-) diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index 8fd86b97..4ddc69f0 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 05187268..84bca3b3 100644 --- a/umap/views.py +++ b/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): - 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. @@ -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( - 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"] ) @@ -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): - 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: @@ -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") - - 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) @@ -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["Last-Modified"] = self.last_modified + response["X-Datalayer-Version"] = self.version return response