#48 exception handling#57
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR refactors exception handling by replacing Spring's generic Changes
Sequence DiagramssequenceDiagram
participant Client
participant Service as CaseService
participant ExceptionHandler as GlobalControllerAdvice
participant Response as HTTP Response
Note over Client,Response: REST Request Flow (GET /api/cases/{id})
Client->>Service: getCase(doctorActor, caseId)
alt Unauthorized Access
Service-->>ExceptionHandler: throws NotAuthorizedException
ExceptionHandler->>ExceptionHandler: isRestRequest() = true
ExceptionHandler->>Response: ResponseEntity.status(403).body({errorCode, message})
Response-->>Client: HTTP 403 + JSON
end
Note over Client,Response: Non-REST Request Flow (GET /ui/cases/{id})
Client->>Service: getCase(doctorActor, caseId)
alt Unauthorized Access
Service-->>ExceptionHandler: throws NotAuthorizedException
ExceptionHandler->>ExceptionHandler: isRestRequest() = false
ExceptionHandler->>ExceptionHandler: Extract `@ResponseStatus`(403)
ExceptionHandler->>Response: response.setStatus(403)<br/>model.addAttribute(status, message, errorCode)<br/>return "error" view
Response-->>Client: HTTP 403 + error.html page
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/common/NotAuthorizedException.java (1)
12-13:⚠️ Potential issue | 🟠 MajorStandardize unauthorized
errorCodeto one canonical value.Line 13 defaults to
"NOT_AUTHORIZED", while new tests and mocked throws in this PR use"NOT_ALLOWED". This creates inconsistent API error codes for the same class of failure.💡 Suggested fix
public class NotAuthorizedException extends AppException { + public static final String ERROR_CODE = "NOT_ALLOWED"; + public NotAuthorizedException(String errorCode, String message) { super(errorCode, message); } public NotAuthorizedException(String message) { - super("NOT_AUTHORIZED", message); + super(ERROR_CODE, message); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/common/NotAuthorizedException.java` around lines 12 - 13, Constructor in NotAuthorizedException currently passes "NOT_AUTHORIZED" as the errorCode which conflicts with the canonical code used elsewhere ("NOT_ALLOWED"); update the NotAuthorizedException(String message) constructor to call super with the canonical error code "NOT_ALLOWED" instead of "NOT_AUTHORIZED" (i.e., change the super("NOT_AUTHORIZED", message) call in class NotAuthorizedException to use "NOT_ALLOWED") so the API errorCode matches tests and mocked throws.
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java (2)
82-88: REST response schema inconsistent with other handlers.This handler omits the
errorCodefield from the JSON response, whilehandleAppException,handleAccessDeniedException, andhandleGeneralExceptionall include it. Clients expecting a uniform error schema may encounter issues.Consider adding a generic error code for consistency:
♻️ Proposed fix
if (isRestRequest(request)) { return ResponseEntity.status(code) .body(Map.of( + "errorCode", "RESPONSE_STATUS_ERROR", "message", reason, "status", code.value() )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java` around lines 82 - 88, The REST branch in GlobalControllerAdvice currently builds a Map with "message" and "status" but omits "errorCode", causing an inconsistent schema; update the isRestRequest(request) response construction (inside GlobalControllerAdvice) to include an "errorCode" entry (e.g. a generic value like "UNKNOWN_ERROR" or a constant used by other handlers) so it matches handleAppException/handleAccessDeniedException/handleGeneralException’s response shape.
119-121: Consider context path in REST request detection.If the application is deployed with a context path (e.g.,
/myapp), the URI would be/myapp/api/...and this check would fail. Usingrequest.getServletPath()or checking against the configured API base path would be more robust.♻️ Alternative using servlet path
private boolean isRestRequest(HttpServletRequest request) { - return request.getRequestURI().startsWith("/api/"); + return request.getServletPath().startsWith("/api/"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java` around lines 119 - 121, The isRestRequest(HttpServletRequest request) method incorrectly inspects request.getRequestURI() which breaks when the app has a context path; change the detection to use request.getServletPath() (e.g., check request.getServletPath().startsWith("/api/")) or construct the match using request.getContextPath() + "/api/" so the method correctly detects REST calls regardless of deployment context; update the isRestRequest implementation and any callers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 101-117: The global exception handler handleGeneralException in
GlobalControllerAdvice swallows exceptions without logging; add a private static
final Logger (LoggerFactory.getLogger(GlobalControllerAdvice.class)) to the
class and log the caught Exception e (including stack trace) at an appropriate
level (e.g., log.error) at the start of handleGeneralException before building
the response or model so production errors are recorded for debugging.
In `@src/main/resources/templates/error.html`:
- Line 11: Replace the current Thymeleaf attribute on the <p class="muted" ...>
element so it uses an explicit fallback expression (ternary or Elvis) instead of
plain th:text="${message}"; for example set th:text to "${message} ?: 'An
unexpected error occurred.'", ensuring the literal fallback text is shown when
message is null or empty.
---
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/common/NotAuthorizedException.java`:
- Around line 12-13: Constructor in NotAuthorizedException currently passes
"NOT_AUTHORIZED" as the errorCode which conflicts with the canonical code used
elsewhere ("NOT_ALLOWED"); update the NotAuthorizedException(String message)
constructor to call super with the canonical error code "NOT_ALLOWED" instead of
"NOT_AUTHORIZED" (i.e., change the super("NOT_AUTHORIZED", message) call in
class NotAuthorizedException to use "NOT_ALLOWED") so the API errorCode matches
tests and mocked throws.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 82-88: The REST branch in GlobalControllerAdvice currently builds
a Map with "message" and "status" but omits "errorCode", causing an inconsistent
schema; update the isRestRequest(request) response construction (inside
GlobalControllerAdvice) to include an "errorCode" entry (e.g. a generic value
like "UNKNOWN_ERROR" or a constant used by other handlers) so it matches
handleAppException/handleAccessDeniedException/handleGeneralException’s response
shape.
- Around line 119-121: The isRestRequest(HttpServletRequest request) method
incorrectly inspects request.getRequestURI() which breaks when the app has a
context path; change the detection to use request.getServletPath() (e.g., check
request.getServletPath().startsWith("/api/")) or construct the match using
request.getContextPath() + "/api/" so the method correctly detects REST calls
regardless of deployment context; update the isRestRequest implementation and
any callers accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f1cb877-04c4-4ca2-97a5-aedeeae466da
📒 Files selected for processing (9)
src/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/PatientService.javasrc/main/java/org/example/projektarendehantering/common/BadRequestException.javasrc/main/java/org/example/projektarendehantering/common/ConflictException.javasrc/main/java/org/example/projektarendehantering/common/NotAuthorizedException.javasrc/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.javasrc/main/resources/templates/error.htmlsrc/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.javasrc/test/java/org/example/projektarendehantering/presentation/web/UiControllerTest.java
| <main class="container page"> | ||
| <div class="panel panel-error" style="text-align: center; padding: 40px;"> | ||
| <h1 th:text="${status != null ? status : 'Error'}">Error</h1> | ||
| <p class="muted" style="font-size: 1.2rem; margin-top: 16px;" th:text="${message}">An unexpected error occurred.</p> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Thymeleaf, when th:text points to a null model attribute, does the element body fallback text render, or is it replaced by an empty string?
💡 Result:
In Thymeleaf, when th:text points to a null model attribute, the element body fallback text does NOT render. It is replaced by an empty string.
Citations:
- 1: https://stackoverflow.com/questions/36896482/thymeleaf-inline-text-shows-null-strings-as-null
- 2: https://northcoder.com/post/thymeleaf-expression-surprise
Use an explicit fallback expression to ensure message displays reliably.
Line 11: th:text="${message}" renders an empty string when message is null, so the literal fallback text inside the <p> tag is ignored. Use a ternary operator to provide an explicit fallback.
Suggested fix
- <p class="muted" style="font-size: 1.2rem; margin-top: 16px;" th:text="${message}">An unexpected error occurred.</p>
+ <p class="muted" style="font-size: 1.2rem; margin-top: 16px;"
+ th:text="${message != null and !#strings.isEmpty(message) ? message : 'An unexpected error occurred.'}">
+ An unexpected error occurred.
+ </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/error.html` at line 11, Replace the current
Thymeleaf attribute on the <p class="muted" ...> element so it uses an explicit
fallback expression (ternary or Elvis) instead of plain th:text="${message}";
for example set th:text to "${message} ?: 'An unexpected error occurred.'",
ensuring the literal fallback text is shown when message is null or empty.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #48
Summary by CodeRabbit
Bug Fixes
New Features
Tests