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

Development: Support native stomp for websockets #10268

Closed
wants to merge 1 commit into from

Conversation

janthoXO
Copy link
Contributor

@janthoXO janthoXO commented Feb 5, 2025

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Motivation and Context

SockJS does not support custom header on the websocket handshake. Therefore clients which are not able to use cookies (VSCode Extensions, ...) can not connect to our websockets.

Description

As SockJS has the purpose of supporting none websocket browser with active polling or a like, but we manually disabled this feature, there is no need to use SockJS. On the other hand it as it restricts use from using headers, we should support native STOMP. To be backwards compatible and have a time where all clients can adjust to native stomp, we only add the same endpoint without SockJS to support both. This endpoint can then be removed as soon as all clients have adapted

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 2 Students
  • 1 Programming Exercise with Complaints enabled
  1. Log in to Artemis
  2. Navigate to Course Administration
  3. ...

Exam Mode Testing

Prerequisites:

  • 1 Instructor
  • 2 Students
  • 1 Exam with a Programming Exercise
  1. Log in to Artemis
  2. Participate in the exam as a student
  3. Make sure that the UI of the programming exercise in the exam mode stays unchanged. You can use the exam mode documentation as reference.
  4. ...

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.







Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Test Coverage

@janthoXO janthoXO self-assigned this Feb 5, 2025
@github-actions github-actions bot added server Pull requests that update Java code. (Added Automatically!) core Pull requests that affect the corresponding module labels Feb 5, 2025
Copy link

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 13, 2025
@krusche
Copy link
Member

krusche commented Feb 15, 2025

We will investigate the underlying issue and if possible modernize websockets communication so that sockjs is not necessary anymore

@krusche krusche closed this Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Pull requests that affect the corresponding module server Pull requests that update Java code. (Added Automatically!) stale
Projects
Status: Work In Progress
Development

Successfully merging this pull request may close these issues.

2 participants