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

UI activate Server Sent Events #1532

Merged
merged 18 commits into from
Jan 23, 2023
Merged

Conversation

thfries
Copy link
Contributor

@thfries thfries commented Nov 7, 2022

Resolves: #1494

First commit is just for preparation and splits things.js into smaller modules

The initial SSE implementation is quite simple, but I needed to introduced 2 new dependencies:

  • EventSourcePolyfill that support setting the authorization headers
  • Lodash to deep merge the event data into the thing json in the ui

ToDo:

  • Request and handle extra fields for _revision and _updated
  • The ace editors of a thing should warn the user if he is editing a field that is changed remotely
  • Split the SSE code into a separate js module
  • Test the refactoring of the first commit

- Enabled visual code type checking

Signed-off-by: thfries <thomas.fries0@gmail.com>
Signed-off-by: thfries <thomas.fries0@gmail.com>
@thjaeckle thjaeckle added the UI Issues related to the Ditto explorer UI label Nov 7, 2022
@thjaeckle
Copy link
Member

Hi @thfries .
Great you are working on this.

Regarding the eventsource polyfill I am not sure that you really need it.

AFAIK you can define "withCredentials" when creating the JS EventSource which sends along the used "Authorization" header for that domain (which was already set if you authenticated against HTTP before).

@thjaeckle thjaeckle added this to the 3.1.0 milestone Nov 7, 2022
@thfries
Copy link
Contributor Author

thfries commented Nov 7, 2022

I tried that with the standard EventSource and "withCredentials" option. But then the Browser owned modal dialog for username/password appeared. So I don't know how to make the browser aware of the Authorization header that is set in the fetch calls. Do you need a sparate authenticate call for a domain?

@thjaeckle
Copy link
Member

It should work the way we documented in the Ditto docs:
https://www.eclipse.org/ditto/httpapi-sse.html#example-for-sse-on-things

By defining { withCredentials: true } at the new EventSource(), the browser credentials (Authorization header) of the already authenticated browser against that domain are sent along, this works for Basic Auth as well as for JWT based authentication using a Bearer token.

A successfully authenticated to the same domain must be done before being able to open the Eventsource with credentials as far as I know.

@thjaeckle thjaeckle removed this from the 3.1.0 milestone Nov 18, 2022
@thfries
Copy link
Contributor Author

thfries commented Nov 18, 2022

Hi @thjaeckle,
just as an update: I'm struggling a bit with the events that are received while you are editing in the ui. That has some major effects to deal with "dirty" editors in general. So I'm currently thinking about to always check if the user forgot to save his changes and that will take some more time...

@thjaeckle
Copy link
Member

I see.

For updating scalar values (eg a single feature property) you could pass a condition, performing a conditional update of the property only if the property has still the value known to the model in the UI.
If it was changed in the meantime you would get a HTTP status "precondition failed" and show the user that the value was changed in the backend.
Then you could ask the user if the value should be set nevertheless, doing the same request without condition.

For the complete thing Json you could do it in a similar way, using the _revision of the loaded thing as If-Match header when updating the complete thing.

In either way, I think explicitly changing the state of a Form element explicitly to "write" could make sense (vs having always a two way binding and editable form elements).
And then ignoring updates via SSE for that level which the user wants to edit.

- Explicit editing for thing CRUD
- Split of thing.js file

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
Copy link
Contributor Author

thfries commented Dec 19, 2022

Hi again,

finally here is a proposal. The best solution I found is to introduce a new button that explicitly switches from readonly mode and ends up in a modal like editing mode - while still having kind of in place editing. Currently only implemented on Things CRUD (not feature or other objects yet). Tell me what you think.

I totally messed up with the merge of another branch in file searchFilter.js. I need to resolve that later. Currently this branch won't work, sorry.

Some issues I observed:

  • when opening an event source, there is one event per second without a data content (observed on the sandbox). Is this intended or what is that caused by?
  • when I do a change via the UI, I get a SSE for that change. So I end up with two UI updates. I would like to remove one of them. So is it intended that the source of a change receives a notification of his own change? In this case I would prevent the UI triggered update. That feels a bit risky, because the update is then dependent on the SSE.
  • I would like to get the extraFields revision and modified date because I would expect that update on the UI. But that seems not to work.

Thomas

@thjaeckle
Copy link
Member

Thanks @thfries
I will try to have a look soon.

Regarding your questions:

  • when opening an event source, there is one event per second without a data content (observed on the sandbox). Is this intended or what is that caused by?

That is intended - as a "heartbeat" mechanism to keep the connection open (as e.g. loadbalancers would close HTTP connections after a certain "idle time" without messages).
Empty SSE messages should therefore be ignored.

  • when I do a change via the UI, I get a SSE for that change. So I end up with two UI updates. I would like to remove one of them. So is it intended that the source of a change receives a notification of his own change? In this case I would prevent the UI triggered update. That feels a bit risky, because the update is then dependent on the SSE.

Different than a "connection" or a websocket session those 2 HTTP calls are already 2 different sources.
Currently I don't have a solution for that problem, I would not rely on the SSE event only to update the UIs state, the successful response is the more reliable way to update the UIs state.
I have to think about that.

  • I would like to get the extraFields revision and modified date because I would expect that update on the UI. But that seems not to work.

This should work.
I just verified against the sandbox (modifying an attribute "foo"):

curl --http2 -u ditto:ditto -H 'Accept:text/event-stream' -N https://ditto.eclipseprojects.io/api/2/things/:08672869-0539-44b9-964e-511bbfa1bdbf\?fields\=thingId,attributes,_revision,_modified

data:{"thingId":":08672869-0539-44b9-964e-511bbfa1bdbf","attributes":{"foo":"bar"},"_revision":23,"_modified":"2022-12-19T08:23:58.514052327Z"}

@thfries
Copy link
Contributor Author

thfries commented Dec 19, 2022

Thanks, @thjaeckle,

the erroneous merge is fixed now.

Thanks for clarifying the questions.

In your curl example you used fields and not extraFields. I was using extraFields because I expected that all changes are included by default if I do not specify field projection.
So let me try fields=thingId,attributes,features,_revision,_modified...

@thjaeckle
Copy link
Member

In your curl example you used fields and not extraFields. I was using extraFields because I expected that all changes are included by default if I do not specify field projection.

Ah, that explains it, yes.
extraFields are used to "fetch" stuff not included in an event (e.g. the attributes when only a feature was updated) - however the _revision and _modified timestamp are always included in every event.
They are just not returned (also when doing a GET /api/2/things/<thingId>) by default and must explicitly be asked for, using the fields param.

@thjaeckle
Copy link
Member

finally here is a proposal. The best solution I found is to introduce a new button that explicitly switches from readonly mode and ends up in a modal like editing mode - while still having kind of in place editing. Currently only implemented on Things CRUD (not feature or other objects yet). Tell me what you think.

@thfries
I like that approach 👍
I noticed that you do not yet use the "optimistic locking" approach I suggested using If-Match header. So when I start editing the thing and it is edited in the backend (e.g. by the device), once I "send" the changes from the UI, the updated state from the device would be lost.

A tip or two on the SSE API usage:
You can also open the SSE directly on a specific Thing:

/api/2/things/org.eclipse.ditto:thing-2

The syntax you used is beneficial when watching more than one Thing (e.g. all currently visible/fetched Things in the table could also be an option - which would allow to see update to values of many things in the table):

/api/2/things?ids=org.eclipse.ditto:thing-2,org.eclipse.ditto:thing-3,org.eclipse.ditto:thing-4

And it may even be omitted:

/api/2/things

In this case, you get all modifications for all the things you are allowed to see.

--
I am still not convinced that the "polyfill" is really required - maybe I can find some time testing the functionality without it.

@thfries
Copy link
Contributor Author

thfries commented Dec 20, 2022

Nice. Thanks for the feedback. Happy that you like the solution.
Yes I remember both issues and had both on my list (optimistic locking and to further test with the standard event source).
Thanks for the hints for the simpler syntax, too
I'll continue then. Hope to make some progress soon, now.

- custom web component for CRUD toolbar
- API allows to return headers if needed
- adapt editors with new toolbar and ETag handling
- fixed modified date and revision from SSE
- avoid feature editors jumping because of badge in header
- fixes some static type checks in utils
Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
Copy link
Contributor Author

thfries commented Jan 6, 2023

I finished a bigger commit that includes a new custom web component for the "new editor style". That caused a lot of changes in the editing codes for things, attributes and features.

Next to that, a way to handle the ETag headers was added.
It was required to add calls to load the feature or attribute to receive the hash values.

Finally optimistic locking works and prevents overwrites of conflicting updates by throwing an error.
There is no convenient way to resolve the conflict yet (like you proposed, @thjaeckle). So that is room for future improvement.

I'll continue with additional commits to apply the same editor style to all other places. These can be done independently. As this makes the whole UI more consistent, it is appropriate to include them in the same PR?

@thfries
Copy link
Contributor Author

thfries commented Jan 9, 2023

I have a question on the optimistic locking: It seems that ditto is responding with a "hash:xxx" ETag header in case of "small" features but with a W/"hash:xxx" in case of large features. Ditto does not accept a PUT on a large feature if the if-match header includes the W/ prefix.

Is it correct, if the PUT request just removes the W/ prefix in the if-match (in case it is there)?

@thjaeckle
Copy link
Member

I have a question on the optimistic locking: It seems that ditto is responding with a "hash:xxx" ETag header in case of "small" features but with a W/"hash:xxx" in case of large features. Ditto does not accept a PUT on a large feature if the if-match header includes the W/ prefix.

Is it correct, if the PUT request just removes the W/ prefix in the if-match (in case it is there)?

Hi @thfries

It seems that "weak entity tags" can never be used in an If-Match:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Match

You could try to remove the W/ prefix - I am currently not sure how and when this is set.
As far as I remember Ditto does not add the prefix (maybe nginx or the HTTP library does).

@thjaeckle
Copy link
Member

I'll continue with additional commits to apply the same editor style to all other places. These can be done independently. As this makes the whole UI more consistent, it is appropriate to include them in the same PR?

Sure, I think it is good to do all those changes in this PR.

- Added new view to see incoming updates for the selected thing
- Utils format date had wrong interface description

Signed-off-by: thfries <thomas.fries0@gmail.com>
- On feature update also weak ETags are allowed
- Improved cleaning up on environment change

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
Copy link
Contributor Author

thfries commented Jan 15, 2023

Added several improvements and also a new view that shows all incoming updates. That creates a bit more transparency what is going on.

  • Feature update now accepts also weak ETags. So if I understand right, it could happen that a conflicting change may be overwritten without notice, if the remote change and local change end up with the same hash accidentally (I assume a very small chance here)
  • Tested again the standard EventSource with withCredentials and I failed. Immediately before creating the EventSource, there is a successful call to the same domain with the right authorization header. Anyhow on creating the EventSource, the browser pops up the basic auth login dialog (and creation of the EventSource ends up with a 401). Tested:
    • with Chrome
    • with Basic auth and JWT authentication
    • with local ditto and remote ditto on a different domain
    • tried a credentials: 'include' parameter on the preceding fetch call
    • tried to use the thingId in the path instead of id= URL parameter (in order to make the full path of the fetch and the EventSource identical).
    • may be it is related to the issue that the domain of the UI is not the same as the API (browser build-in mechanisms seem only to work on the same domain). But then at least the test with the local ditto should work...

@thfries thfries marked this pull request as ready for review January 15, 2023 10:07
@thfries
Copy link
Contributor Author

thfries commented Jan 15, 2023

I took a look at the Policy editors. May we keep the policies as is for now and address this with a separate PR? I am also thinking about the policy imports, that require some additional extensions.

So I removed draft status now for this PR.

Remaining issues:

  • SSE only works with Polyfill EventSource
  • After update the UI is refreshed twice (one triggered by a GET request from the UI when the PUT was successful and a second one by the SSE)

- Changed CRUD buttons to text and showing dynamically
- wrong ETag header on feature/attribute creation
- incoming changes now also show attributes

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
Copy link
Contributor Author

thfries commented Jan 15, 2023

Hey @thjaeckle,
I played around with the edit buttons and made it more "standard bootstrap" style. (use of primary and secondary colors, text instead of icons, show buttons only if they are needed).

Found an issue with the If-Match header in case one creates a new feature or new attribute. This is fixed now.

Another thing I observed: If you delete a feature or attribute, the SSE does not show the change, so in these cases the UI does not automatically update.

@thfries thfries changed the title (WIP) UI activate sse UI activate Server Sent Events Jan 16, 2023
@thjaeckle
Copy link
Member

thjaeckle commented Jan 16, 2023

@thfries had a quick first look at the current state. Very promising 👍 - will have a deeper look at all modifications at a later point.

One quick feedback about the ACE editor for the complete thing JSON: I noticed that when not in "Edit" mode you are still able to edit the JSON - which could be confusing (as saving that is not possible).
Is it possible to render the editor as "read only" when not in edit mode?

Another thing I noticed is that custom table columns are not updated when a thing is modified (e.g. via "edit" function). Would it be possible to "sync" table and detail view model somehow?

@thjaeckle thjaeckle added this to the 3.2.0 milestone Jan 16, 2023
@thfries
Copy link
Contributor Author

thfries commented Jan 16, 2023

ACE editor should behave like you expect it. This must be a bug and will be fixed.

Update of the table columns is missing, yes. I can add that (at least for the selected thing. May be this can be reused for all things in the search result, may be?

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thjaeckle
Copy link
Member

Update of the table columns is missing, yes. I can add that (at least for the selected thing. May be this can be reused for all things in the search result, may be?

Then you would have to subscribe on the SSE for all of the thingIds currently shown in the table - this would of course be a great benefit when showing e.g. certain values in the table which update for different things, like e.g. an "Connection state".

- Update selected thing in search result table

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries thfries marked this pull request as draft January 19, 2023 06:36
@thfries
Copy link
Contributor Author

thfries commented Jan 19, 2023

Started a quick implementation for the update of the things search table. It is not very complicated so it makes sense to include that.

If I extend that to all things in the search result, should we limit that in some way? e.g.

  • only the first page of the search result and the pinned things only
  • specify the relevant fields in the SSE instead of subscribing to the full things (then we need 2 EventSources: one for the search table and one for the selected thing?)

Thanks for your advice.

@thjaeckle
Copy link
Member

If I extend that to all things in the search result, should we limit that in some way? e.g.

  • only the first page of the search result and the pinned things only
  • specify the relevant fields in the SSE instead of subscribing to the full things (then we need 2 EventSources: one for the search table and one for the selected thing?)

The only limiting factor here is probably the total length of the URL caused by the query param "ids" which get longer and longer the more pages are loaded ..
The backend does not have a problem if very many things are subscribed for.

I think it does make sense like you proposed it in the second point .. create 2 SSEs, one for the table (where e.g. all the loaded things are included) which uses the columns shown in the table.
And one SSE for the currently selected thing.

- create a second SSE that listens to all things of the search result
- avoid ace editors from creating endless undo histories
@thfries thfries marked this pull request as ready for review January 21, 2023 21:20
- WoT description for feature was referencing old dom field
- thing search more button changed SSE to new page and lost 1st page

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
Copy link
Contributor Author

thfries commented Jan 22, 2023

Hi @thjaeckle ,
so far so good. Updating the table works nicely as discussed.
It is now limited to the first page of the search so for max 25 things. Additional search pages are not automatically updated for now. I was thinking about adding some notice for that (like a warning pop up) but for now there is no warning.

I observed that there are messages, that contain only a thingId and nothing else. So I assume the fields parameter controls only the result that is transferred, but does not filter the messages. I think that does not harm for now, but causes unneeded traffic.

PR is ready from my point of view. Thanks

Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes and UI look good to me 👍
Thanks for that great contribution, @thfries

Together with the new "edit mode" this improves the UI a lot.

@thjaeckle thjaeckle merged commit fb742f8 into eclipse-ditto:master Jan 23, 2023
@thfries thfries deleted the ui_activate_sse branch January 29, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Issues related to the Ditto explorer UI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Ditto Explorer UI: Update UI automatically on changes using Server Sent Event
2 participants