Skip to content

Add UnexpectedServerException for centralized server error handling#13740

Open
CodeByAfroj wants to merge 40 commits intoTEAMMATES:masterfrom
CodeByAfroj:fix/unexpected-server-exception
Open

Add UnexpectedServerException for centralized server error handling#13740
CodeByAfroj wants to merge 40 commits intoTEAMMATES:masterfrom
CodeByAfroj:fix/unexpected-server-exception

Conversation

@CodeByAfroj
Copy link
Copy Markdown

Fixes #13733

Outline of Solution

This PR introduces a new UnexpectedServerException to handle unexpected server-side errors in a centralized way.

Changes made:

  • Added a new custom exception UnexpectedServerException
  • Mapped it to HTTP 500 in WebApiServlet
  • Replaced direct JsonResult(..., HttpStatus.SC_INTERNAL_SERVER_ERROR) returns with throwing UnexpectedServerException in CreateAccountAction

Rationale:

Previously, some parts of the code returned HTTP 500 responses directly when unexpected checked exceptions occurred. This bypassed the centralized error handling in WebApiServlet.

This change ensures:

  • Centralized error handling
  • Consistent HTTP 500 responses
  • Cleaner and more maintainable code

Testing:

  • Verified that the flow still returns HTTP 500
  • Confirmed errors are handled via WebApiServlet

Replaced JsonResult error handling with UnexpectedServerException for better error management.
Added handling for UnexpectedServerException in WebApiServlet.
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 6, 2026

Hi @CodeByAfroj, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contributing guidelines:

  • Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]

Please address the above before we proceed to review your PR.

@CodeByAfroj
Copy link
Copy Markdown
Author

Hi, I’ve submitted a PR for this issue. Please review it.

} catch (InvalidParametersException | EntityAlreadyExistsException | EntityDoesNotExistException e) {
// There should not be any invalid parameter or entity conflict here
log.severe("Unexpected error", e);
return new JsonResult(e.getMessage(), HttpStatus.SC_INTERNAL_SERVER_ERROR);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this is not the only place this occurrence of returning a JsonResult with the HTTP 500 directly. For one, UnpublishFeedbackSessionAction has one of this instance.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for giving me a feedback . I will Do the required changes ASAP.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@CodeByAfroj

Please check the codebase for ALL instances of directly returning the error. You can try searching for the pattern e.g. the log.severe part or the return new JsonResult part. One example: teammates/ui/webapi/JoinCourseAction.java:58

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Give me some time i will fix it ASAP.

@@ -73,7 +74,7 @@ public JsonResult execute() throws InvalidHttpRequestBodyException, InvalidOpera
} catch (InvalidParametersException | EntityAlreadyExistsException | EntityDoesNotExistException e) {
// There should not be any invalid parameter or entity conflict here
log.severe("Unexpected error", e);
Copy link
Copy Markdown
Contributor

@iZUMi-kyouka iZUMi-kyouka Apr 6, 2026

Choose a reason for hiding this comment

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

I suppose these logs were initially there because we bypassed the centralised error handling, which will otherwise have logged this unexpected error.

These redundant log.severe calls should be removed too since the centralised error handling already logs the error.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I got this and thanks again , I will fix them .

@iZUMi-kyouka
Copy link
Copy Markdown
Contributor

Hi @CodeByAfroj, thank you for your interest in contributing to TEAMMATES! However, your PR does not appear to follow our contributing guidelines:

* Title must start with the issue number the PR is fixing in square brackets, e.g. `[#<issue-number>]`

Please address the above before we proceed to review your PR.

Please address this too @CodeByAfroj

@CodeByAfroj
Copy link
Copy Markdown
Author

Hi, I have addressed all the feedback and ensured that all checks are passing now. Kindly review.

@CodeByAfroj
Copy link
Copy Markdown
Author

I Have gone through the All project files and changed the return format according to your expectations please review and let me know , if there any feedback .

log.severe(e.getClass().getSimpleName() + " caught by WebApiServlet: " + e.getMessage(), e);
throwError(resp, statusCode, e.getMessage());
String message;
if (e.getMessage() != null && !e.getMessage().isEmpty()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should use StringHelper.isEmpty instead

} catch (InvalidParametersException ipe) {
// There should not be any invalid parameter here
log.severe("Unexpected error", ipe);
return new JsonResult(ipe.getMessage(), HttpStatus.SC_INTERNAL_SERVER_ERROR);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes not done? Please do not remove comments.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you so much for your feedback, I wiil respond you ASAP. Your feedback really helping me

@CodeByAfroj
Copy link
Copy Markdown
Author

I have made the changes please review and let me know if any suggessions

@CodeByAfroj
Copy link
Copy Markdown
Author

i have correct all the changes please review

@iZUMi-kyouka
Copy link
Copy Markdown
Contributor

Are you using an IDE to make the fix? JoinCourseAction still needs this fix. @CodeByAfroj

@CodeByAfroj
Copy link
Copy Markdown
Author

Are you using an IDE to make the fix? JoinCourseAction still needs this fix. @CodeByAfroj
I am only using the vs code editor , I got this , I try to fix it

@CodeByAfroj
Copy link
Copy Markdown
Author

Are you using an IDE to make the fix? JoinCourseAction still needs this fix. @CodeByAfroj

this action returns a JsonResult as expected , But for previous Actions they directly throwing Some different exceptions, so we made a custom exception.Now for this action Changing it to throw UnexpectedServerException causes test failures, so I have retained the current behavior here.

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.

Add UnexpectedServerException for unexpected server side errors

2 participants