Use the new Server Side Event support in ipaas-rest to automatically reload updated entity lists. #186

chirino · 2017-02-22T22:26:49Z

Works in conjunction with the following PR:
syndesisio/syndesis-rest#115

jimmibot · 2017-02-22T22:26:52Z

@chirino, thanks! @kahboom, please review this.

jimmidyson · 2017-02-22T23:04:45Z

@dsimansk @jludvice Stuff like this EventSource usage highlights the need for cross-browser test suites... Please have a think about how we can do that.

chirino · 2017-02-23T18:50:50Z

retest this please

jimmidyson · 2017-02-23T19:24:12Z

Does EventSource support authorization headers? Our REST API currently only authenticates via validation of JWT contained in authorization header...

chirino · 2017-02-23T19:38:03Z

@jimmidyson Looks we have to pass it via cookies.. is that a deal breaker for SSE?

kahboom

@chirino That'd be a no-go for IE support out of the box (this is pretty useful for determining what you can use), but check out this polyfill. Honestly, I say we get rid of the WS (WSS preferably), use SSE from the REST API and a really nice service in the UI to update the model on an as-needed basis via regular HTTP requests.

Basically, I think we only really need one or the other, not both. And if I had to choose, I'd normally go with Websockets, but in this case SSEs may make more sense. It's not without its caveats though. On the Java side, SSE allows you to override the header, but JS side you can't. A way around that is to use query parameters. Not my best idea, but, hey.

SSE does use regular HTTP, so the polyfill may be worth looking into. If we went with Websockets we'd really not be taking advantage of client > server communication because we don't need to (unless we of course decide to add a chat or something of that nature). As much as I like working with Websockets and its quirky complexities I don't see the benefit of dealing with it in this case. We'd mostly be looking for responses from the server, not bi-directional communication in realtime.

An example of what we need is that the client be notified when we delete a connection or integration, which could take a while for a 'delete completed' response from the server. Again, I'd usually opt for Websockets for specific events like this, not something like arbitrarily updating the server on any model change. And with the polyfill we just don't need it.

For most client > server model updates (e.g. creating a connection) we can just send manual HTTP requests, not leaving connections open as with Websockets, constantly checking for connection/disconnection. This will also be a pain when it comes to checking user inactivity later on. SSEs offer a ton of relevant things that WebSockets don't by design, like automatic reconnection, event IDs, and the ability to send arbitrary events.

Keep in mind that I'm more comfortable using Websockets, but I mean, it should be more about the right tool for the job I think? WDYT @chirino @jimmidyson ?

kahboom · 2017-02-23T20:15:58Z

src/app/store/entity/entity.store.ts


-  get resource() { return this._current.asObservable(); }
+  get resource() {


Not sure if this was you or ts-lint.json, but either way, ty.

kahboom · 2017-02-23T21:02:21Z

If something doesn't make sense in my comment, just know I'm running on 3 hours of sleep, which may or may not be relevant. ;D

kahboom · 2017-02-23T21:11:25Z

Even better, a Node.js implementation that can be used to override headers from the "JS side", polyfill included: eventsource

chirino · 2017-02-23T21:51:17Z

@kahboom, @jimmidyson I think SSE security should be sorted now without needing to do the cookie thing. Basically I first do a POST to request a event stream ID, at that point the user is associated with the stream, and then then we setup the SSE connection against that registered stream id (a random UUID) which does not get re-used.

I kind like the SSE first then fallback to WS scenario, since if we ever move to HTTP2, then we are using the preferred strategy to begin with.

also take a peek at:
http://caniuse.com/#feat=eventsource
http://caniuse.com/#feat=websockets

Notice we would be using SSE on most browsers except IE and Edge, and both have good WebSocket support.

kahboom · 2017-02-23T22:43:20Z

Yeah Websockets have the advantage of wider browser support. I'm not opposed to falling back on WS, or at least don't feel strongly about it. @jimmidyson I'd like to hear your thoughts on this. @chirino once tests are resolved I'll approve it, then merge based on what Jimmi prefers.

jimmidyson · 2017-02-24T16:52:11Z

add to whitelist

jimmidyson · 2017-02-24T16:54:34Z

src/app/store/entity/events.service.ts

+      // , fallback to using a WebSocket
+      let wsurl = config.getSettings().apiEndpoint;
+      // I know hacky!  need to figure out how to put the wsevents resource under the apiEndpoint
+      wsurl = wsurl.replace( /^https?/, 'ws' ).replace( /\/api\/v\d+$/, '/wsevents' );


this would always fall back to insecure ws rather than wss if https is used

Right, we'd absolutely need WSS considering we'd be opening connections quite frequently if it's to update models on any change.

will fix in a bit.

kahboom · 2017-02-24T17:01:20Z

Providing Websocket support as a fallback means we would need to maintain it along with the SSE implementation as the application scales, otherwise it'd be for naught. Just something to consider, but again, not opposed to it at all.

jimmidyson

Looks good, minor issues need updating. Will probably need tweaking once we start using it properly.

jimmidyson · 2017-02-24T17:06:52Z

src/app/store/entity/events.service.ts

@@ -0,0 +1,90 @@
+import {Injectable} from '@angular/core';
+import {Subject} from 'rxjs/Subject';
+import {UUID} from 'angular2-uuid';


This doesn't seem to be used?

yep.. don't need the uuid anymore. hangover from previous iteration.

jimmidyson · 2017-02-24T17:10:22Z

Can't remember if the server side even supports websockets right now - @chirino?

kahboom · 2017-02-24T17:13:44Z

@jimmidyson - Now it does I believe, see syndesisio/syndesis-rest#115. More specifically.

kahboom · 2017-02-24T17:18:55Z

Also of relevance #114 .

chirino · 2017-02-24T18:22:57Z

Squashed and rebased off master.

chirino · 2017-02-24T18:23:59Z

@kahboom, @jimmidyson: yep ipaas-reset supports both SSE and WebSockets.

jimmidyson · 2017-02-24T18:46:55Z

src/app/store/entity/events.service.ts

+      // , fallback to using a WebSocket
+      let wsurl = config.getSettings().apiEndpoint;
+      // I know hacky!  need to figure out how to put the wsevents resource under the apiEndpoint
+      wsurl = wsurl.replace( /^http/, 'ws' ).replace( /\/api\/v\d+$/, '/wsevents' );


hmm this is a bit pants and means we need to set up another subpath route in openshift. what's the problem putting it under the API endpoint?

Yep that's what I initially tried but the websocket handler would not trigger if not at a top level path. Not sure why yet. Perhaps if I use a different WS handling strategy it might work better (I'm currently using the SB abstractions).

…matically reload updated entity lists. (cherry picked from commit a000eb5)

angular2-uuid was dropped.

jimmidyson · 2017-02-28T13:05:15Z

@kahboom Are you happy with this? LGTM

kahboom

LGTM! Thanks for fixing the tests. 👍

jimmibot assigned kahboom Feb 22, 2017

jimmibot requested a review from kahboom February 22, 2017 22:26

chirino force-pushed the sse branch from a486866 to 5d62068 Compare February 23, 2017 03:35

chirino mentioned this pull request Feb 23, 2017

Support sending event notifications to clients via Server Side Events/WebSockets syndesisio/syndesis-rest#115

Merged

chirino force-pushed the sse branch 2 times, most recently from 4339a7d to e8e807f Compare February 23, 2017 16:31

kahboom reviewed Feb 23, 2017

View reviewed changes

chirino force-pushed the sse branch from d477d7f to b7c85bc Compare February 23, 2017 23:42

chirino requested a review from jimmidyson February 24, 2017 16:35

jimmidyson reviewed Feb 24, 2017

View reviewed changes

jimmidyson previously requested changes Feb 24, 2017

View reviewed changes

kahboom added this to In Progress in Syndesis UI Feb 24, 2017

kahboom added this to the Sprint 9 milestone Feb 24, 2017

kahboom added the enhancement label Feb 24, 2017

chirino force-pushed the sse branch 2 times, most recently from a000eb5 to cb88c6d Compare February 24, 2017 18:22

jimmidyson reviewed Feb 24, 2017

View reviewed changes

Use the new Server Side Event/WebSocket support in ipaas-rest to auto…

d3342f6

…matically reload updated entity lists. (cherry picked from commit a000eb5)

chirino force-pushed the sse branch from cb88c6d to d3342f6 Compare February 24, 2017 19:44

kahboom approved these changes Feb 28, 2017

View reviewed changes

kahboom merged commit 81d4fbc into syndesisio:master Feb 28, 2017

kahboom moved this from In Progress to Sprint 9 Release Candidate/Complete in Syndesis UI Mar 1, 2017

kahboom removed this from Sprint 9 Release Candidate/Complete in Syndesis UI Oct 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the new Server Side Event support in ipaas-rest to automatically reload updated entity lists. #186

Use the new Server Side Event support in ipaas-rest to automatically reload updated entity lists. #186

chirino commented Feb 22, 2017

jimmibot commented Feb 22, 2017

jimmidyson commented Feb 22, 2017

chirino commented Feb 23, 2017

jimmidyson commented Feb 23, 2017

chirino commented Feb 23, 2017

kahboom left a comment

kahboom Feb 23, 2017

kahboom commented Feb 23, 2017

kahboom commented Feb 23, 2017

chirino commented Feb 23, 2017

kahboom commented Feb 23, 2017

jimmidyson commented Feb 24, 2017

jimmidyson Feb 24, 2017

kahboom Feb 24, 2017

chirino Feb 24, 2017

kahboom commented Feb 24, 2017 •

edited

jimmidyson left a comment

jimmidyson Feb 24, 2017

chirino Feb 24, 2017

jimmidyson commented Feb 24, 2017

kahboom commented Feb 24, 2017 •

edited

kahboom commented Feb 24, 2017

chirino commented Feb 24, 2017

chirino commented Feb 24, 2017

jimmidyson Feb 24, 2017

chirino Feb 24, 2017

jimmidyson commented Feb 28, 2017

kahboom left a comment


		get resource() { return this._current.asObservable(); }
		get resource() {

Use the new Server Side Event support in ipaas-rest to automatically reload updated entity lists. #186

Use the new Server Side Event support in ipaas-rest to automatically reload updated entity lists. #186

Conversation

chirino commented Feb 22, 2017

jimmibot commented Feb 22, 2017

jimmidyson commented Feb 22, 2017

chirino commented Feb 23, 2017

jimmidyson commented Feb 23, 2017

chirino commented Feb 23, 2017

kahboom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kahboom commented Feb 23, 2017

kahboom commented Feb 23, 2017

chirino commented Feb 23, 2017

kahboom commented Feb 23, 2017

jimmidyson commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kahboom commented Feb 24, 2017 • edited

jimmidyson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmidyson commented Feb 24, 2017

kahboom commented Feb 24, 2017 • edited

kahboom commented Feb 24, 2017

chirino commented Feb 24, 2017

chirino commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmidyson commented Feb 28, 2017

kahboom left a comment

Choose a reason for hiding this comment

kahboom commented Feb 24, 2017 •

edited

kahboom commented Feb 24, 2017 •

edited