From b8db07a4ce4fe253acc9035d36d787f6520229ec Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 7 Feb 2025 16:47:45 +0100 Subject: [PATCH 1/7] chore: sync save state When a peer save the map, other peers in dirty state should not need to save the map anymore. That implementation uses the lastKnownHLC as a reference, but it changes all dirty states at once. Another impementation could be to have each object sync its dirty state, but in this case we do not have a HLC per object as reference, and it also creates more messages. Co-authored-by: David Larlet --- umap/static/umap/js/modules/saving.js | 5 +++ umap/static/umap/js/modules/sync/engine.js | 19 +++++++- umap/static/umap/js/modules/umap.js | 1 + umap/sync/app.py | 5 +++ umap/sync/payloads.py | 9 +++- umap/tests/integration/test_websocket_sync.py | 43 +++++++++++++++++++ 6 files changed, 80 insertions(+), 2 deletions(-) diff --git a/umap/static/umap/js/modules/saving.js b/umap/static/umap/js/modules/saving.js index c478c494..501fe19a 100644 --- a/umap/static/umap/js/modules/saving.js +++ b/umap/static/umap/js/modules/saving.js @@ -10,6 +10,11 @@ export async function save() { } } +export function clear() { + _queue.clear() + onUpdate() +} + function add(obj) { _queue.add(obj) onUpdate() diff --git a/umap/static/umap/js/modules/sync/engine.js b/umap/static/umap/js/modules/sync/engine.js index 62106352..fcb05b1d 100644 --- a/umap/static/umap/js/modules/sync/engine.js +++ b/umap/static/umap/js/modules/sync/engine.js @@ -2,6 +2,7 @@ import * as Utils from '../utils.js' import { HybridLogicalClock } from './hlc.js' import { DataLayerUpdater, FeatureUpdater, MapUpdater } from './updaters.js' import { WebSocketTransport } from './websocket.js' +import * as SaveManager from '../saving.js' // Start reconnecting after 2 seconds, then double the delay each time // maxing out at 32 seconds. @@ -125,6 +126,13 @@ export class SyncEngine { this._send({ verb: 'delete', subject, metadata, key }) } + saved() { + this.transport.send('SavedMessage', { + sender: this.peerId, + lastKnownHLC: this._operations.getLastKnownHLC(), + }) + } + _send(inputMessage) { const message = this._operations.addLocal(inputMessage) @@ -168,6 +176,8 @@ export class SyncEngine { } else if (payload.message.verb === 'ListOperationsResponse') { this.onListOperationsResponse(payload) } + } else if (kind === 'SavedMessage') { + this.onSavedMessage(payload) } else { throw new Error(`Received unknown message from the websocket server: ${kind}`) } @@ -280,6 +290,13 @@ export class SyncEngine { // Else: apply } + onSavedMessage({ sender, lastKnownHLC }) { + debug(`received saved message from peer ${sender}`, lastKnownHLC) + if (lastKnownHLC === this._operations.getLastKnownHLC() && SaveManager.isDirty) { + SaveManager.clear() + } + } + /** * Send a message to another peer (via the transport layer) * @@ -350,7 +367,7 @@ export class Operations { } /** - * Tick the clock and add store the passed message in the operations list. + * Tick the clock and store the passed message in the operations list. * * @param {*} inputMessage * @returns {*} clock-aware message diff --git a/umap/static/umap/js/modules/umap.js b/umap/static/umap/js/modules/umap.js index 740cc9b9..234409e1 100644 --- a/umap/static/umap/js/modules/umap.js +++ b/umap/static/umap/js/modules/umap.js @@ -684,6 +684,7 @@ export default class Umap extends ServerStored { Alert.success(translate('Map has been saved!')) }) } + this.sync.saved() this.fire('saved') } diff --git a/umap/sync/app.py b/umap/sync/app.py index 6ef0d72f..1d66b648 100644 --- a/umap/sync/app.py +++ b/umap/sync/app.py @@ -14,6 +14,7 @@ from .payloads import ( OperationMessage, PeerMessage, Request, + SavedMessage, ) @@ -165,6 +166,10 @@ class Peer: case OperationMessage(): await self.broadcast(text_data) + # Broadcast the new map state to connected peers + case SavedMessage(): + await self.broadcast(text_data) + # Send peer messages to the proper peer case PeerMessage(): await self.send_to(incoming.root.recipient, text_data) diff --git a/umap/sync/payloads.py b/umap/sync/payloads.py index 9ab2bf1a..f5e167e4 100644 --- a/umap/sync/payloads.py +++ b/umap/sync/payloads.py @@ -30,10 +30,17 @@ class PeerMessage(BaseModel): message: dict +class SavedMessage(BaseModel): + kind: Literal["SavedMessage"] = "SavedMessage" + lastKnownHLC: str + + class Request(RootModel): """Any message coming from the websocket should be one of these, and will be rejected otherwise.""" - root: Union[PeerMessage, OperationMessage] = Field(discriminator="kind") + root: Union[PeerMessage, OperationMessage, SavedMessage] = Field( + discriminator="kind" + ) class JoinResponse(BaseModel): diff --git a/umap/tests/integration/test_websocket_sync.py b/umap/tests/integration/test_websocket_sync.py index 13b647b0..3c42a6e1 100644 --- a/umap/tests/integration/test_websocket_sync.py +++ b/umap/tests/integration/test_websocket_sync.py @@ -557,3 +557,46 @@ def test_create_and_sync_map(new_page, asgi_live_server, tilelayer, login, user) peerA.get_by_role("button", name="Edit").click() expect(markersA).to_have_count(2) expect(markersB).to_have_count(2) + + +@pytest.mark.xdist_group(name="websockets") +def test_should_sync_saved_status(new_page, asgi_live_server, tilelayer): + map = MapFactory(name="sync", edit_status=Map.ANONYMOUS) + map.settings["properties"]["syncEnabled"] = True + map.save() + + # Create two tabs + peerA = new_page("Page A") + peerA.goto(f"{asgi_live_server.url}{map.get_absolute_url()}?edit") + peerB = new_page("Page B") + peerB.goto(f"{asgi_live_server.url}{map.get_absolute_url()}?edit") + + # Create a new marker from peerA + peerA.get_by_title("Draw a marker").click() + peerA.locator("#map").click(position={"x": 220, "y": 220}) + + # Peer A should be in dirty state + expect(peerA.locator("body")).to_have_class(re.compile(".*umap-is-dirty.*")) + + # Peer B should not be in dirty state + expect(peerB.locator("body")).not_to_have_class(re.compile(".*umap-is-dirty.*")) + + # Create a new marker from peerB + peerB.get_by_title("Draw a marker").click() + peerB.locator("#map").click(position={"x": 200, "y": 250}) + + # Peer B should be in dirty state + expect(peerB.locator("body")).to_have_class(re.compile(".*umap-is-dirty.*")) + + # Peer A should still be in dirty state + expect(peerA.locator("body")).to_have_class(re.compile(".*umap-is-dirty.*")) + + # Save layer to the server from peerA + with peerA.expect_response(re.compile(".*/datalayer/create/.*")): + peerA.get_by_role("button", name="Save").click() + + # Peer B should not be in dirty state + expect(peerB.locator("body")).not_to_have_class(re.compile(".*umap-is-dirty.*")) + + # Peer A should not be in dirty state + expect(peerA.locator("body")).not_to_have_class(re.compile(".*umap-is-dirty.*")) From a172c4abeaf1c952bc62de0df145814c89e12664 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 7 Feb 2025 17:53:48 +0100 Subject: [PATCH 2/7] fixup: do not try to sync saved state when not in sync mode Co-authored-by: David Larlet --- umap/static/umap/js/modules/sync/engine.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/umap/static/umap/js/modules/sync/engine.js b/umap/static/umap/js/modules/sync/engine.js index fcb05b1d..c8451392 100644 --- a/umap/static/umap/js/modules/sync/engine.js +++ b/umap/static/umap/js/modules/sync/engine.js @@ -127,10 +127,13 @@ export class SyncEngine { } saved() { - this.transport.send('SavedMessage', { - sender: this.peerId, - lastKnownHLC: this._operations.getLastKnownHLC(), - }) + if (this.offline) return + if (this.transport) { + this.transport.send('SavedMessage', { + sender: this.peerId, + lastKnownHLC: this._operations.getLastKnownHLC(), + }) + } } _send(inputMessage) { From eca7ad47721fb1aa6831272ebf0d304cd3a36cc2 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 7 Feb 2025 17:54:33 +0100 Subject: [PATCH 3/7] fixup: prevent to reload a datalayer after other peer has saved it The scenario to reproduce is: - peer A creates a datalayer - peer B add a marker on that datalayer - peer B saves the datalayer Before this fix, after the save, peer A would get a new _referenceVersion for this datalayer, and the render method would make a hide/show, which would refetch the data from the server, duplicating it. So forcing the _loaded to be true in this situation prevent this. We may want to rework the "_loaded" pattern, maybe with the server returning in a datalayer metadata the length of the data it get for this given datalayer, so the client knows if it is worth getting data for a layer when itself (the client) does not have any. Co-authored-by: David Larlet --- umap/static/umap/js/modules/sync/updaters.js | 5 +++-- umap/tests/integration/test_websocket_sync.py | 8 +++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/umap/static/umap/js/modules/sync/updaters.js b/umap/static/umap/js/modules/sync/updaters.js index 5ab97375..f694d23e 100644 --- a/umap/static/umap/js/modules/sync/updaters.js +++ b/umap/static/umap/js/modules/sync/updaters.js @@ -63,6 +63,7 @@ export class DataLayerUpdater extends BaseUpdater { update({ key, metadata, value }) { const datalayer = this.getDataLayerFromID(metadata.id) if (fieldInSchema(key)) { + datalayer._loaded = true this.updateObjectValue(datalayer, key, value) } else { console.debug( @@ -92,7 +93,7 @@ export class FeatureUpdater extends BaseUpdater { upsert({ metadata, value }) { const { id, layerId } = metadata const datalayer = this.getDataLayerFromID(layerId) - const feature = this.getFeatureFromMetadata(metadata, value) + const feature = this.getFeatureFromMetadata(metadata) if (feature) { feature.geometry = value.geometry @@ -109,7 +110,7 @@ export class FeatureUpdater extends BaseUpdater { return } if (key === 'geometry') { - const feature = this.getFeatureFromMetadata(metadata, value) + const feature = this.getFeatureFromMetadata(metadata) feature.geometry = value } else { this.updateObjectValue(feature, key, value) diff --git a/umap/tests/integration/test_websocket_sync.py b/umap/tests/integration/test_websocket_sync.py index 3c42a6e1..dc1ae21a 100644 --- a/umap/tests/integration/test_websocket_sync.py +++ b/umap/tests/integration/test_websocket_sync.py @@ -421,9 +421,11 @@ def test_should_sync_datalayers(new_page, asgi_live_server, tilelayer): 1 ).click() - # Now peerA saves the layer 2 to the server - with peerA.expect_response(re.compile(".*/datalayer/update/.*")): - peerA.get_by_role("button", name="Save").click() + # Peer A should not be in dirty state + expect(peerA.locator("body")).not_to_have_class(re.compile(".*umap-is-dirty.*")) + + # Peer A should only have two markers + expect(peerA.locator(".leaflet-marker-icon")).to_have_count(2) assert DataLayer.objects.count() == 2 From ba0582deb18199d8c6dbcc17b6775d87ce332cb1 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 10 Feb 2025 13:13:56 +0100 Subject: [PATCH 4/7] chore: refactor layer.isLoaded() --- umap/static/umap/js/modules/data/layer.js | 14 +++++--------- umap/static/umap/js/modules/sync/updaters.js | 4 ---- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/umap/static/umap/js/modules/data/layer.js b/umap/static/umap/js/modules/data/layer.js index 1ca1b785..7208f06b 100644 --- a/umap/static/umap/js/modules/data/layer.js +++ b/umap/static/umap/js/modules/data/layer.js @@ -45,7 +45,6 @@ export class DataLayer extends ServerStored { this._features = {} this._geojson = null this._propertiesIndex = [] - this._loaded = false // Are layer metadata loaded this._dataloaded = false // Are layer data loaded this._leafletMap = leafletMap @@ -85,6 +84,7 @@ export class DataLayer extends ServerStored { this.connectToMap() this.permissions = new DataLayerPermissions(this._umap, this) + this._needsFetch = this.createdOnServer if (!this.createdOnServer) { if (this.showAtLoad()) this.show() } @@ -268,7 +268,7 @@ export class DataLayer extends ServerStored { if (geojson._umap_options) this.setOptions(geojson._umap_options) if (this.isRemoteLayer()) await this.fetchRemoteData() else this.fromGeoJSON(geojson, false) - this._loaded = true + this._needsFetch = false } clear() { @@ -345,7 +345,7 @@ export class DataLayer extends ServerStored { } isLoaded() { - return !this.createdOnServer || this._loaded + return !this._needsFetch } hasDataLoaded() { @@ -633,7 +633,6 @@ export class DataLayer extends ServerStored { this.propagateDelete() this._leaflet_events_bk = this._leaflet_events this.clear() - delete this._loaded delete this._dataloaded } @@ -652,7 +651,6 @@ export class DataLayer extends ServerStored { this.hide() if (this.isRemoteLayer()) this.fetchRemoteData() else if (this._geojson_bk) this.fromGeoJSON(this._geojson_bk) - this._loaded = true this.show() this.isDirty = false } @@ -1108,9 +1106,7 @@ export class DataLayer extends ServerStored { async save() { if (this.isDeleted) return await this.saveDelete() - if (!this.isLoaded()) { - return - } + if (!this.isLoaded()) return const geojson = this.umapGeoJSON() const formData = new FormData() formData.append('name', this.options.name) @@ -1172,7 +1168,7 @@ export class DataLayer extends ServerStored { this.backupOptions() this.backupData() this.connectToMap() - this._loaded = true + this._needsFetch = false this.redraw() // Needed for reordering features return true } diff --git a/umap/static/umap/js/modules/sync/updaters.js b/umap/static/umap/js/modules/sync/updaters.js index f694d23e..9c6ff196 100644 --- a/umap/static/umap/js/modules/sync/updaters.js +++ b/umap/static/umap/js/modules/sync/updaters.js @@ -55,15 +55,11 @@ export class DataLayerUpdater extends BaseUpdater { upsert({ value }) { // Upsert only happens when a new datalayer is created. const datalayer = this._umap.createDataLayer(value, false) - // Prevent the layer to get data from the server, as it will get it - // from the sync. - datalayer._loaded = true } update({ key, metadata, value }) { const datalayer = this.getDataLayerFromID(metadata.id) if (fieldInSchema(key)) { - datalayer._loaded = true this.updateObjectValue(datalayer, key, value) } else { console.debug( From 175e27a535793c6d4c839f2490bc884326190e4d Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 10 Feb 2025 15:44:41 +0100 Subject: [PATCH 5/7] chore: remove DataLayer._dataloaded in favor of isLoaded() At the end, we only need two states: has this datalayer loaded the data it should load ? yes / no. Whether it local or remote data should not be a matter. --- umap/static/umap/js/modules/data/layer.js | 13 +++---------- .../umap/js/modules/rendering/layers/classified.js | 2 +- umap/tests/integration/test_websocket_sync.py | 4 +++- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/umap/static/umap/js/modules/data/layer.js b/umap/static/umap/js/modules/data/layer.js index 7208f06b..04ef3757 100644 --- a/umap/static/umap/js/modules/data/layer.js +++ b/umap/static/umap/js/modules/data/layer.js @@ -45,7 +45,6 @@ export class DataLayer extends ServerStored { this._features = {} this._geojson = null this._propertiesIndex = [] - this._dataloaded = false // Are layer data loaded this._leafletMap = leafletMap this.parentPane = this._leafletMap.getPane('overlayPane') @@ -243,7 +242,7 @@ export class DataLayer extends ServerStored { } dataChanged() { - if (!this.hasDataLoaded()) return + if (!this.isLoaded()) return this._umap.onDataLayersChanged() this.layer.dataChanged() } @@ -252,13 +251,13 @@ export class DataLayer extends ServerStored { if (!geojson) return [] const features = this.addData(geojson, sync) this._geojson = geojson + this._needsFetch = false this.onDataLoaded() this.dataChanged() return features } onDataLoaded() { - this._dataloaded = true this.renderLegend() } @@ -268,7 +267,6 @@ export class DataLayer extends ServerStored { if (geojson._umap_options) this.setOptions(geojson._umap_options) if (this.isRemoteLayer()) await this.fetchRemoteData() else this.fromGeoJSON(geojson, false) - this._needsFetch = false } clear() { @@ -320,7 +318,7 @@ export class DataLayer extends ServerStored { async fetchRemoteData(force) { if (!this.isRemoteLayer()) return - if (!this.hasDynamicData() && this.hasDataLoaded() && !force) return + if (!this.hasDynamicData() && this.isLoaded() && !force) return if (!this.isVisible()) return // Keep non proxied url for later use in Alert. const remoteUrl = this._umap.renderUrl(this.options.remoteData.url) @@ -348,10 +346,6 @@ export class DataLayer extends ServerStored { return !this._needsFetch } - hasDataLoaded() { - return this._dataloaded - } - backupOptions() { this._backupOptions = Utils.CopyJSON(this.options) } @@ -633,7 +627,6 @@ export class DataLayer extends ServerStored { this.propagateDelete() this._leaflet_events_bk = this._leaflet_events this.clear() - delete this._dataloaded } reset() { diff --git a/umap/static/umap/js/modules/rendering/layers/classified.js b/umap/static/umap/js/modules/rendering/layers/classified.js index 138f53b0..6b8f39aa 100644 --- a/umap/static/umap/js/modules/rendering/layers/classified.js +++ b/umap/static/umap/js/modules/rendering/layers/classified.js @@ -75,7 +75,7 @@ const ClassifiedMixin = { }, renderLegend: function (container) { - if (!this.datalayer.hasDataLoaded()) return + if (!this.datalayer.isLoaded()) return const parent = DomUtil.create('ul', '', container) const items = this.getLegendItems() for (const [color, label] of items) { diff --git a/umap/tests/integration/test_websocket_sync.py b/umap/tests/integration/test_websocket_sync.py index dc1ae21a..e0c19d47 100644 --- a/umap/tests/integration/test_websocket_sync.py +++ b/umap/tests/integration/test_websocket_sync.py @@ -398,7 +398,9 @@ def test_should_sync_datalayers(new_page, asgi_live_server, tilelayer): peerA.locator("#map").click() # Make sure this new marker is in Layer 2 for peerB - expect(peerB.get_by_text("Layer 2")).to_be_visible() + # Show features for this layer in the brower. + peerB.get_by_role("heading", name="Layer 2").locator(".datalayer-name").click() + expect(peerB.locator("li").filter(has_text="Layer 2")).to_be_visible() peerB.locator(".panel.left").get_by_role("button", name="Show/hide layer").nth( 1 ).click() From a8e18c167ca1cc18c8ec09f7c9e25b339236a11c Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 10 Feb 2025 16:30:00 +0100 Subject: [PATCH 6/7] chore: no need to reset datalayer._needsFetch after a save It is reset only after loading the layer data. Co-authored-by: David Larlet --- umap/static/umap/js/modules/data/layer.js | 1 - 1 file changed, 1 deletion(-) diff --git a/umap/static/umap/js/modules/data/layer.js b/umap/static/umap/js/modules/data/layer.js index 04ef3757..b8f06bb5 100644 --- a/umap/static/umap/js/modules/data/layer.js +++ b/umap/static/umap/js/modules/data/layer.js @@ -1161,7 +1161,6 @@ export class DataLayer extends ServerStored { this.backupOptions() this.backupData() this.connectToMap() - this._needsFetch = false this.redraw() // Needed for reordering features return true } From 9b01a4b77a88a3197d718a28f047dcff7911c063 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 10 Feb 2025 16:56:15 +0100 Subject: [PATCH 7/7] chore: read datalayer metadata from file if missing in DB When we introduced the DataLayer.settings property, we did not run a migration for existing datalayers (because there are millions in the French server). Now that we simplified the `DataLayer.isLoaded()` logic in the client, we want to be sure that created DataLayer on the client have full metadata. Co-authored-by: David Larlet --- umap/models.py | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/umap/models.py b/umap/models.py index e6367fe6..265c52eb 100644 --- a/umap/models.py +++ b/umap/models.py @@ -523,17 +523,27 @@ class DataLayer(NamedModel): def metadata(self, request=None): # Retrocompat: minimal settings for maps not saved after settings property # has been introduced - obj = self.settings or { - "name": self.name, - "displayOnLoad": self.display_on_load, - } + metadata = self.settings + if not metadata: + # Fallback to file for old datalayers. + data = json.loads(self.geojson.read().decode()) + metadata = data.get("_umap_options") + if not metadata: + metadata = { + "name": self.name, + "displayOnLoad": self.display_on_load, + } + # Save it to prevent file reading at each map load. + self.settings = metadata + # Do not update the modified_at. + self.save(update_fields=["settings"]) if self.old_id: - obj["old_id"] = self.old_id - obj["id"] = self.pk - obj["permissions"] = {"edit_status": self.edit_status} - obj["editMode"] = "advanced" if self.can_edit(request) else "disabled" - obj["_referenceVersion"] = self.reference_version - return obj + metadata["old_id"] = self.old_id + metadata["id"] = self.pk + metadata["permissions"] = {"edit_status": self.edit_status} + metadata["editMode"] = "advanced" if self.can_edit(request) else "disabled" + metadata["_referenceVersion"] = self.reference_version + return metadata def clone(self, map_inst=None): new = self.__class__.objects.get(pk=self.pk)