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

Use Javalin's built-in Response exceptions #484

Closed
wants to merge 2 commits into from

Conversation

ThanGerlek
Copy link
Contributor

@ThanGerlek ThanGerlek commented Nov 19, 2024

Replaces our inhouse HTTP Exception classes with Javalin's default response exceptions. Javalin automatically catches these and sends the right status code, with the default message being the status code string (e.g. "Bad Request", "Server Error").

Obviously this depends on #473.

Should we do this?

Pros:

  • Much neater, shorter code
  • Offloads Exception handling
  • Prioritizes nicely with other .exception() clauses we might add

Cons:

  • We have less control
  • Greater dependency on Javalin
  • Doesn't handle null messages well (see below)

Null messages

Throwing one of Javalin's default response exceptions with a null message string will simply return a nasty "Null value!" error message to the client (the no-parameter constructor works fine though). The message parameter in the constructor is annotated @NotNull, so it'll usually warn you if you try something like throw new BadRequestResponse(null). However, this can be accidentally subverted with code like catch (Exception e) {throw new BadRequestResponse(e.getMessage());} (which happens in several places in the code) if the caught exception has no error message.

Copy link
Contributor

@webecke webecke left a comment

Choose a reason for hiding this comment

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

LGTM!

I'm going to cast my vote in favor of using Javalin's Exceptions. I get the argument against it, but the code looks nice this way and it means we don't have to worry about translating exceptions leaving the server. I'm not worried about the null thing, since we should be giving messages anyways with our exceptions. Great work!

Copy link
Collaborator

@19mdavenport 19mdavenport left a comment

Choose a reason for hiding this comment

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

An argument against doing this is now the Services now depend on Javalin, which would be not ideal if we ever need to be switching to a different protocol or HTTP library (like switching spark to javalin). How likely do you think those would be going forwards?

@ThanGerlek
Copy link
Contributor Author

ThanGerlek commented Nov 21, 2024

Probably not too likely I'd think, but either way it wouldn't be too hard to switch back to custom ones if we need to. And actually the Services don't have a strong dependency on them; they do in a one or two places I think but it's mainly the Handlers themselves

@ThanGerlek
Copy link
Contributor Author

Will not implement. Turns out this would introduce deeper Service-level dependencies on Javalin than we expected, so we decided it's not worth it.

@ThanGerlek ThanGerlek deleted the update-exceptions branch December 11, 2024 21:24
@frozenfrank
Copy link
Contributor

That decision is consistent with the principles Prof. Wilkerson covered today in CS 340. It should be pretty easy to convert our exceptions into Javalin exceptions, and I like the pattern of only performing this transformation when it leaves our system, and not within system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants