Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sync) Add a belongsTo property in the schema #1828

Open
wants to merge 56 commits into
base: master
Choose a base branch
from

Conversation

almet
Copy link
Member

@almet almet commented May 16, 2024

This new property allows to know what's the relation with the schema property and the different object types (map, datalayer, feature).

This serves two goals:

  1. Check that the incoming messages make sense (it's possible to change this property in this object)
  2. Document where the property is used in practice. Because there is a lot of properties, it's hard to keep track of where they are used. This makes it explicit.

This is to be merged after #1754

almet added 30 commits May 15, 2024 12:24
This makes it possible to use them in standalone scripts, when using
`django.settings.configure(**settings_dict)`.
There is one "room" per map, and the server relays messages to all the
other connected peers.

Messages are checked for compliance with what's allowed as a security
measure. They should also be checked in the clients to avoid potential
attack vectors.
A new SyncEngine module has been added to the JavaScript code. It aims
to sync the local changes with remote ones. This first implementation
relies on a websocket connection.
Authentication is now done using a signed token provided by the Django
server, sent by the JS client and checked by the WebSocket server.

The token contains a `permissions` key that's checked to ensure the user
has access to the map "room", where events will be shared by the peers.
Synced objects now expose different methods, such as:
- `getSyncEngine` which returns the location of the sync object.
- `getMetadata` which returns the associated metadata with the object.

Hooks have been added when features are created or changed, so the
changes can be synced with other peers.
All keystrokes are currently sent, which is not ideal because it will
use a lot of bandwidth.
Added a new `geometryToFeature` method in `umap.layer.js` which can
update a given geometry if needed.

A new `id` property can also be passed to the features on creation, to
make it possible to have the same features `id` on different peers.
This also changes the interface between the synced classes and the sync
engine.

The sync engines only requires two methods now:

- `getSyncMetadata()` which returns all the metadata needed, including
the `engine`.
- `render()` which renders the object, updating the needed parts.
Otherwise, an event can come from the websocket, trying to update an
`undefined` property.
This changes how the syncEngine works. At the moment, it's always
instanciated, even if no syncing is configured. It just does nothing.

This is to avoid doing `if (engine) engine.update()` calls everywhere
we use it.

You now need to `start()` and `stop()` it.
`WEBSOCKET_ENABLED`, `WEBSOCKET_HOST`, `WEBSOCKET_PORT` and
`WEBSOCKET_URI` have been added.

They are being transmitted to the client via the `map.options`.
When `WEBSOCKET_ENABLED` is set to `False`, the client doesn't have
the ability to be synced.
The goal being for it to be hidden for now.

- Add a `is_owner` method on the map and use it in the view
- Remove duplicated line in `global.js`
- Rename `Datalayer` to `DataLayer` everywhere
- Move the sync flag in the map options (next to slideshow)
The is done to save server resources, for accessed maps which
aren't currently being edited.
It wasn't passed previously, so objects where duplicated.
Prior to these changes, the data wasn't transmitted over WebSocket, and
even if present it wasn't taken into account.
It's less spectacular than sending the position as it changes, but takes
less bandwidth and seems good enough.
Removing the `id` from the feature when cloning makes it generate a new
one. Without this change, the cloned feature keep the already existing
`id`, and during sync, the original object is lost (replaced by the
clone).

Nobody wants to be replaced by a clone.
The goal is to use this as a security measure, to check that the
updated properties belong the the "subject" when receiving sync
operations.
When receiving a message, this checks the given properties belong to
the "subject" before applying the message.
`this.options.overlay` is now set during `map.initialize()`, which makes
it possible to be set by incoming websocket operations.
It makes it possible to set these values on a remote peer.
They will be synced on their own, and we dont want them to be present
twice on the map.
By defining the `geometry` property in the schema.
almet added 24 commits May 15, 2024 12:25
Because we're relying on the `geoJSONToFeatures` method, we don't need
anymore updaters, the default ones (map, datalayer, feature) are enough.

It also makes the codebase compatible with our eslint configuration.
The new implementation uses `reduce`, in the hopes of producing a more
readable version than old-style loops and reassignment of object values.
As "collaboration" can be mistaken between "websocket" and
"server-merge". This naming makes it explicit.
This commit handles the start and stop of the websocket server during
the tests, using the xprocess library
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 new property allows to know what's the relation with the schema
property and the different object types (map, datalayer, feature).

This serves two goals:

1. Check that the incoming messages make sense (it's possible to change
   this property in this object)
2. Document where the property is used in practice. Because there is a
   lot of properties, it's hard to keep track of where they are used.
   This makes it explicit.
@yohanboniface
Copy link
Member

I think the way to go here is to have a proper schema, with nesting (and I suggest yaml format, more readable, and to have anchors/aliases), enforced by the backend, and versioned, as suggested in #1635.

I imagine something like this:

version: 1.0
ui: &ui
  browsable:
    impacts: ['ui']
    type: Boolean
  captionBar:
    type: Boolean
    impacts: ['ui']
    label: Do you want to display a caption bar?
    default: false
  captionMenus:
    type: Boolean
    impacts: ['ui']
    label: Do you want to display caption menus?
    default: true
style: &style
  color:
    type: String
    impacts: ['data']
    handler: 'ColorPicker'
    label: color
    helpEntries: 'colorValue'
    inheritable: true
    default: 'DarkBlue'
pathstyle: &pathstyle
  dashArray:
    type: String
    impacts: ['data']
    label: dash array
    helpEntries: 'dashArray'
    inheritable: true
map:
  options:
    <<: *style
    <<: *ui
    facetKey:
      type: String
      impacts: ['ui']
datalayer:
  options:
    <<: *style
  choropleth:
    type: Object
    impacts: ['data']
  cluster:
    type: Object
    impacts: ['data']
  displayOnLoad:
    type: Boolean
    impacts: []
marker:
  options:
    <<: *style
polyline:
  options:
    <<: *style
    <<: *pathstyle
polygon:
  options:
    <<: *style
    <<: *pathstyle

@almet almet force-pushed the websockets branch 2 times, most recently from 0757d3f to 7990ac3 Compare May 31, 2024 09:54
Base automatically changed from websockets to master June 7, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants