Implement SSE server endpoint - [merged] #273

Closed paulmis closed 2 years ago

In GitLab by @rstular on Mar 6, 2022, 08:02

Merges 105-sse-endpoint -> dev

This MR introduces a new API endpoint, /api/sse/open. In order to use this endpoint, the client must be authenticated and in an "ONGOING" game (the latter is potentially subject to change).

Upon receiving a request, a new SseEmitter will be opened, and registered with the SSEManager of the related game. All communication with individual clients can be done via SSEManager instance.

Closes #105

In GitLab by @rstular on Mar 6, 2022, 08:02

changed target branch from main to dev

In GitLab by @gpezzali on Mar 6, 2022, 09:00

Commented on server/src/main/java/server/services/SSEManager.java line 115

Should this require a certain game id? I don't think we need to send a message to every connected user

In GitLab by @gpezzali on Mar 6, 2022, 09:18

Commented on server/src/main/java/server/services/SSEManager.java line 39

Should this call unregister instead?

In GitLab by @rstular on Mar 6, 2022, 10:28

added 1 commit

f91d1dad - feat(sse): Add SSE Controller.

Compare with previous version

In GitLab by @rstular on Mar 6, 2022, 10:36

Commented on server/src/main/java/server/services/SSEManager.java line 39

While the functionality is not quite the same (unregister doesn't call complete() on the SseEmitter), I'll try to implement this to some extent, as I agree with your assessment.

In GitLab by @rstular on Mar 6, 2022, 10:36

Commented on server/src/main/java/server/services/SSEManager.java line 115

The code has been modified. Now each SSEManager is bound to a game instance, meaning that it will only contain emitters for users in the same game. sendAll will there notify all users in one game.

In GitLab by @rstular on Mar 6, 2022, 10:42

added 1 commit

2d2baef1 - fix(sse): Reuse functions in SSEManager

Compare with previous version

In GitLab by @rstular on Mar 6, 2022, 10:42

resolved all threads

In GitLab by @rstular on Mar 6, 2022, 10:42

marked this merge request as ready

In GitLab by @gpezzali on Mar 6, 2022, 11:12

approved this merge request

In GitLab by @rstular on Mar 6, 2022, 11:16

requested review from @aakankshsingh and @paulmis

The log messages should be returned as a response.

Write a JPQL for this.

You should use AuthContext.get() instead of principal.getName().

Could use some comments.

In GitLab by @rstular on Mar 6, 2022, 12:14

Commented on server/src/main/java/server/api/SSEController.java line 43

changed this line in version 6 of the diff

In GitLab by @rstular on Mar 6, 2022, 12:14

Commented on server/src/main/java/server/api/SSEController.java line 39

changed this line in version 6 of the diff

In GitLab by @rstular on Mar 6, 2022, 12:14

resolved all threads

In GitLab by @rstular on Mar 6, 2022, 12:14

added 1 commit

2b4ad13a - fix(sse): Workaround for lazily-fetched entities, fixed test

Compare with previous version

In GitLab by @rstular on Mar 6, 2022, 12:53

added 1 commit

08141e67 - fix(sse): Search players by UUID instead of email

Compare with previous version

I don't think this has been resolved?

In GitLab by @rstular on Mar 6, 2022, 17:07

added 1 commit

bb4e88c6 - fix(sse): Fix SSEController test mocking function

Compare with previous version

In GitLab by @rstular on Mar 6, 2022, 17:25

Commented on server/src/main/java/server/api/SSEController.java line 81

Has been resolved in 2b4ad13ad072ca31ca054ebb34d6f99eace5c6cc.

In GitLab by @gpezzali on Mar 6, 2022, 17:48

Commented on server/src/main/java/server/services/SSEManager.java line 13

This used to be a service, is there a reason it isn't anymore?

In GitLab by @rstular on Mar 6, 2022, 17:58

Commented on server/src/main/java/server/services/SSEManager.java line 13

This class is always bound to a Game entity, so we don't actually need it to be managed by Spring.

In GitLab by @rstular on Mar 6, 2022, 20:57

Commented on server/src/main/java/server/services/SSEManager.java line 13

Just to cover all bases, is this correct @paulmis ?

In GitLab by @aakankshsingh on Mar 7, 2022, 01:02

I think it looks good. Not sure if the exceptions are sent to the client. Can you confirm this @rstular. Then I can approve.

In GitLab by @rstular on Mar 7, 2022, 06:16

Currently, there are no exceptions to be thrown, and if we want to send them to the client, we would need to implement logic for that. SSE exceptions are handled by the server, but we can implement and send specific "exception", if we ever need to.

In GitLab by @gpezzali on Mar 7, 2022, 13:05

approved this merge request

@Service is just a fancy @Component that signals the component handles business logic. If it doesn't need to be managed by Spring, then there's no need.

approved this merge request

In GitLab by @rstular on Mar 7, 2022, 23:48

added 31 commits

bb4e88c6...095d765c - 30 commits from branch dev
a0011736 - Merge branch 'dev' into 105-sse-endpoint

Compare with previous version

In GitLab by @rstular on Mar 7, 2022, 23:49

added 5 commits

a0011736...66c3c0bc - 4 commits from branch dev
bfa81967 - Merge branch 'dev' into '105-sse-endpoint'

Compare with previous version

In GitLab by @rstular on Mar 7, 2022, 23:56

Fixed merge conflicts, should be mergeable into dev now.

approved this merge request

In GitLab by @gpezzali on Mar 8, 2022, 24:09

approved this merge request

In GitLab by @gpezzali on Mar 8, 2022, 24:09

mentioned in commit a47c5e9252ce3158400631e66d53a0c6689d648a

paulmis / qz

Implement SSE server endpoint - [merged] #273