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

Migrate autograder to use Javalin #473

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Migrate autograder to use Javalin #473

wants to merge 38 commits into from

Conversation

ThanGerlek
Copy link
Contributor

@ThanGerlek ThanGerlek commented Nov 8, 2024

Migrate the Server class to use the Javalin web framework instead of SparkJava. The logic transfers over fairly neatly but with a few changes noted below. Closes #443.

Significant changes

  • Spark's Request and Response objects are both replaced with a single Context object
  • Handler methods (previously Route) can no longer have return values
  • No halt() method exists
  • ctx.json(responseObject) automatically serializes and sets the response body (using our Serializer class), and ctx.bodyAsClass(ClassName.class) deserializes the request
  • Error responses are now handled via thrown Exceptions (ctx.status() works, but this is the standard to simplify/standardize/DRYify code). As such, various additional Exception classes have been added, largely corresponding to HTTP codes

Dependencies

@ThanGerlek
Copy link
Contributor Author

ThanGerlek commented Nov 8, 2024

Still to do: unit and integration tests

EDIT: Unit tests are in #483.

…gration

# Conflicts:
#	src/main/java/edu/byu/cs/controller/AdminController.java
#	src/main/java/edu/byu/cs/controller/SubmissionController.java
#	src/main/java/edu/byu/cs/controller/httpexception/BadRequestException.java
#	src/main/java/edu/byu/cs/server/Server.java
#	src/main/java/edu/byu/cs/service/SubmissionService.java
#	src/main/java/edu/byu/cs/service/UserService.java
Base automatically changed from extract-service-logic-from-controllers to main November 14, 2024 02:58
# Conflicts:
#	src/main/java/edu/byu/cs/controller/CasController.java
#	src/main/java/edu/byu/cs/controller/UserController.java
#	src/main/java/edu/byu/cs/service/UserService.java
# Conflicts:
#	src/main/java/edu/byu/cs/server/Server.java
# Conflicts:
#	src/main/java/edu/byu/cs/server/Server.java
# Conflicts:
#	src/main/java/edu/byu/cs/server/Server.java
#	src/main/java/edu/byu/cs/server/endpointprovider/EndpointProviderImpl.java
Javalin doesn't have an equivalent of Spark's webSocketIdleTimeout method, so this serves the same purpose.
@@ -25,6 +26,7 @@ public static void onMessage(WsMessageContext ctx) {
Session session = ctx.session;
String message = ctx.message();
String netId;
ctx.enableAutomaticPings(20, TimeUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work properly even when the server is paused on a breakpoint, as if in a debug session?

My first guess is that the automatic pings would also be paused if the server is paused, which defeats the purpose of this timeout.

Copy link
Contributor Author

@ThanGerlek ThanGerlek Nov 19, 2024

Choose a reason for hiding this comment

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

I did some testing, and you're right--that's something we'll have to be aware of when debugging. Fortunately it seems like the only time it affects anything is if the server hits a breakpoint partway through grading a submission, which probably won't happen often. Either way, the original problem was WS timing out when unit tests take a long time to run, and it does fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to also increase the timeout to a large value? That would be helpful for any server debugging, should that arise. Even if we, the autograder team, don't typically use this much, the students will need an increased timeout limit because they will be extensively debugging their servers.

Copy link
Contributor

Choose a reason for hiding this comment

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

But students won't need it on the Autograder, will they? They're not debugging the code as its submitting to the autograder, the debug on their local machine, right?

Copy link
Contributor Author

@ThanGerlek ThanGerlek Nov 19, 2024

Choose a reason for hiding this comment

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

I couldn't find one, and this Javalin issue from last month seems to suggest there isn't one. So I don't think so but I'm not sure 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

But students won't need it on the Autograder, will they? They're not debugging the code as its submitting to the autograder, the debug on their local machine, right?

@webecke My bad, let me clarify. I didn't mean to suggest that the students will be debugging on the AutoGrader. I meant to make reference to future semester where they will be building their chess projects using a version of Javalin. At that point they will be debugging code and stepping through websocket endpoints, and they will need to be able to keep their websocket connections alive.

I couldn't find one, and javalin/javalin#2329 from last month seems to suggest there isn't one.

@ThanGerlek Thanks for referring to that Javaline issue. I just commented on it with my research on the WS timeout issue. From what I can tell without having actually used Javalin myself, the feature doesn't yet exist.

I see this as a symptom of using a premature product which is not yet fully featured. We should keep a list of this and any other weird things we discover about the platform. These kinds of details obviously don't make the entire decision, but the quantity and frequency and severity of multiple of them can influence our decision to look for a different platform as well.

We should take this is a reminder that this is just a community project. Upon reviewing the recently merged PRs, I see a total of 4 substantial PRs in the last 3 months. The professors suggested that this could be a good tool, but it may not be. We should keep our eyes and options open to switching to a different platform which may be better than Javalin.

@frozenfrank frozenfrank added the enhancement New feature or request label Nov 19, 2024
# Conflicts:
#	src/main/java/edu/byu/cs/controller/ConfigController.java
#	src/main/java/edu/byu/cs/server/Server.java
#	src/main/java/edu/byu/cs/server/endpointprovider/EndpointProvider.java
#	src/main/java/edu/byu/cs/server/endpointprovider/EndpointProviderImpl.java
#	src/main/java/edu/byu/cs/util/Serializer.java
@ThanGerlek ThanGerlek changed the base branch from main to reduce-endpoint-dependencies November 19, 2024 01:35
@ThanGerlek ThanGerlek marked this pull request as ready for review November 19, 2024 01:57
`GET /submission` and `GET /submission/-1` are now equivalent.
Base automatically changed from reduce-endpoint-dependencies to main November 25, 2024 18:12
# Conflicts:
#	src/main/java/edu/byu/cs/server/Server.java
#	src/main/java/edu/byu/cs/server/endpointprovider/EndpointProvider.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to JavaLin to match upcoming change to Class Project
3 participants