Allow real-time collaboration #1754
Conversation
almet
changed the title
|
💡 The way things are currently implemented, the synced objects (map, datalayers, features) expose a way to get back the Another way to do it is to pass the |
almet
changed the title
almet
force-pushed
the
websockets
branch
2 times, most recently
from
d58a144 to
1dc4901
Compare
almet
force-pushed
the
websockets
branch
2 times, most recently
from
e1629ba to
5ad3da5
Compare
|
This works as intended, except for:
|
Security-related stuffWebSocket settings ( I was worried this can open security considerations, especially we need to ensure locally defined values can't take precedence over the one served by the server. In the end, it turns out to be safe, because the way it's done, the values from the local |
almet
changed the title
almet
force-pushed
the
websockets
branch
4 times, most recently
from
9ed6b4a to
98c3849
Compare
umap/settings/__init__.py
Outdated
| @@ -43,3 +43,6 @@ | |||
| globals()["STATICFILES_DIRS"].insert(0, value) | |||
| else: | |||
| globals()[key] = value | |||
|
|
|||
| # Expose these settings for consumption by e.g. django.settings.configure. | |||
| settings_as_dict = {k: v for k, v in globals().items() if k.isupper()} | |||
There was a problem hiding this comment.
Were are we with the idea of using a Django management command ?
There was a problem hiding this comment.
I believe it could be done on a separate pull request, would that work for you?
There was a problem hiding this comment.
OK, up to you. I think it's just a matter of copy pasting the ws server code in a file at the right place and using the Command class, and this may make your life easier to run the websocket server during the tests (but may also not :p ).
There was a problem hiding this comment.
I've updated the pull request to run the WebSocket server via a django management command (it was easier than expected !).
See commit 38d19db (#1754) for more info.
| @@ -34,11 +34,13 @@ dependencies = [ | |||
| "django-probes==1.7.0", | |||
| "Pillow==10.3.0", | |||
| "psycopg==3.1.18", | |||
| "pydantic==2.7.0", | |||
There was a problem hiding this comment.
Do we still need this new dependency ?
There was a problem hiding this comment.
Yes, it's still used by the websocket server to check messages are conform
umap/ws.py
Outdated
| peers = CONNECTIONS[map_id] - {websocket} | ||
| # Only relay valid "operation" messages | ||
| try: | ||
| OperationMessage.model_validate_json(raw_message) |
There was a problem hiding this comment.
I'd the the server blink here, and let the frontend know about the internal of a message, write it, and validate it before consuming it.
There was a problem hiding this comment.
It will be useful later, because some messages will not be relayed to all the other connected parties, so we will need to inspect the messages. A bit early, but I really believe it will be useful very soon.
There was a problem hiding this comment.
If it's about checking one key, we may do it explicitly ? Without the need and the new layer of pydantic. At list, pydantic seems overkill to me here.
There was a problem hiding this comment.
You can see pydantic as doing form validation. In our case, It's more than checking just one key, because each message type might have a different format, that we need to ensure (some messages will be for the server itself, for instance if we decide the server saves the status of the connected peers)
There was a problem hiding this comment.
It is true that Pydantic in the current code is not doing much.
In the future, as we will add other messages types, we will want to ensure the messages match a format expected by the server, for instance when the clients will send disconnect messages that we don't want to relay on the other parties, or when they will send messages for just one peer, which should not be broadcasted.
In these cases, Pydantic will prove really useful than now, as we will have to check complex messages, and ensure their format match what is expected by the server.
It is worth noting that it already is useful, as it makes sure that the "join" message is holding the proper "token" key, and that only operation messages are valid in the "room".
If it's not too much of a hassle, I would like to keep it in place to avoid doing back and forth between implementation types for the message validation logic.
docs/config/settings.md
Outdated
| #### WEBSOCKET_URI | ||
|
|
||
| The connection string that will be used by the client to connect to the websocket server. | ||
| Use `wss://host:port` if the server is behind TLS, and `ws://host:port` otherwise. |
There was a problem hiding this comment.
Maybe WEBSOCKET_TLS instead of duplicating HOST and PORT in this string ?
There was a problem hiding this comment.
I see why we would like this, as it would avoid duplicating the information in the settings.
We need to take in consideration that the "front" WebSocket address (that clients will connect to) might be different than the "back" ip and port which are bound in the host.
This happens for instance for reverse proxies, or when running inside a container. In this case, magically guessing the "front" address based on WEBSOCKET_HOST, WEBSOCKET_PORT and WEBSOCKET_TLS might not be enough, and we would need to introduce a WEBSOCKET_URI setting like the one that's already present in this PR.
I've changed the settings to reflect this BACK/FRONT difference, and updated the documentation to make it clearer, see commit 7990ac3 (#1754)
almet
force-pushed
the
websockets
branch
3 times, most recently
from
d27e806 to
de59190
Compare
This makes it possible to use them in standalone scripts, when using `django.settings.configure(**settings_dict)`.
This tests that the name of the map, and that zoom-control visibility is properly synced over websockets.
In some cases, you want to stop the propagation of events. The previous code was using `fromSync=true` and `sync=false` interchangeably. This makes it use `sync=false` everywhere.
It's now it's responsability to get the authentication token from the http server and pass it to the websocket server, it will make it possible to redo the roundtrip when getting disconnected.
Messages are now checked for conformity with the procol we defined, but stop at the `operation` boundary. Values aren't checked.
(but I would really like to see what web socker would look like)
As it requires more discussion, it will happen in a separate pull-request.
This is currently a bug in the current implementation. Hopefully fixed in later commits.
This allows the merge algorithm to not be lost when receiving changes. Without this change, the optimistic merge algorithm isn't able to make the distinction between peers, and features end up duplicated.
It is now using `WEBSOCKET_BACK_HOST`, `WEBSOCKET_BACK_PORT` and `WEBSOCKET_FRONT_URI`. We need to take in consideration that the "front" WebSocket address (that clients will connect to) might be different than the "back" ip and port which are bound in the host. This happens for instance for reverse proxies, or when running inside a container. We considered using a `WEBSOCKET_TLS` setting, to try guessing the "front" address based on `WEBSOCKET_HOST`, `WEBSOCKET_PORT` and `WEBSOCKET_TLS`, but as the back and front address can differ, this would need to introduce a `WEBSOCKET_URI` in any case, so we went with just using it, and not adding an extra `WEBSOCKET_TLS`.
This allows to handle the loading of the settings in a consistant way,
and aditionnaly to provide a way to override the `WEBSOCKET_BACK_HOST`
and `WEBSOCKET_BACK_PORT` settings with arg commands `--host` and
`--port`.
Without this change, because of how we are currently loading our
settings, we would require the settings the be exposed by the
`umap.settings.__init__` file.
Previous implementations were exposing these settings, with the
following code:
```python
settings_as_dict = {k: v for k, v in globals().items() if k.isupper()}
```
Because this `syncUpdatedProperties` function is only called once, it didn't trigger any issue in practice (as the check was always returning true).
The function was only used once, so removing it simplified the whole flow.
They are now handled in the `render()` call, so there is no more need for them here.
It is now possible to create proxy objects using `sync_engine.proxy(object)`.
The returned proxy object will automatically inject `metadata` and
`subject` parameters, after looking for them in the `getSyncMetadata`
method (these are only known to the synced objects).
As a result, the calls are now simplified:
```
this.sync.update("key", "value")
```
Because we are dealing with technologies using overlapping vocabulary, it is easy to get lost. Hopefully this change makes it clear that it converts geoJSON inputs in Leaflet / uMap objects.
Rather than having it done inside the datalayer itself. This gives us more control.
Hopefully this is clearer :-)
Using [pytest-xprocess](https://pytest-xprocess.readthedocs.io/) proved not being as useful as I thought at first, because it was causing intermitent failures when starting the process. The code now directly uses `subprocess.popen` calls to start the server. The tests are grouped together using the following decorator: `@pytest.mark.xdist_group(name="websockets")` Tests now need to be run with the `pytest --dist loadgroup` so that all tests of the same group happen on the same process. More details on this blogpost: https://blog.notmyidea.org/start-a-process-when-using-pytest-xdist.html
Add the ability to sync map changes with other connected peers.
choropleth-sync.webm
This pull request contains
On the server side:
On the client side:
Note
This PR isn't the full story (see the "To be handled later" section below). Currently:
What works?
How to make it work locally?
In order to make it work locally, just update your local settings with the following ones:
And then update your dependencies and run the websocket server alongside the django server:
Steps:
UMAP_SETTINGSENABLE_WEBSOCKETSflag in the settings (and probably other configuration entries)startMarkersyncEnabled = TrueTests
Unit tests:
undefinedFunctional tests, with two browser tabs, and the websocket server running:
To be handled later
The following is also required, but probably in other pull requests:
Communication with other peers
There are multiple scenarii where a communication with other peers might prove useful. It also depends how we want to deal with this in general, as another way to handle this is to have the information stored on the server.
When entering the room:
When saving the map:
In these two cases, it might be helpful to have a 1-1 channel with another peer (and not only do broadcasting). The server might just do the signaling (give the list of connected peers/ids), and the peer might chose to discuss with a privileged one.
To be able to trust the messages are coming from the same client, each peer might get assigned an unique UUID. The server would refuse new connections with this same ID, and messages can be channeled to another peer via the server.