fix: align API status codes and swagger#132
Conversation
📝 WalkthroughWalkthroughThis PR standardizes error handling across the application by introducing a new ChangesError Handling Standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/iemr/ecd/controller/associate/BeneficiaryRegistrationController.java`:
- Around line 59-66: The OpenAPI annotations in
BeneficiaryRegistrationController declare two separate ApiResponse entries for
the same HTTP status (CustomExceptionResponse.INTERNAL_SERVER_ERROR_SC_V and
CustomExceptionResponse.DB_EXCEPTION_SC_V), which conflicts with OpenAPI keys;
update the annotation block to merge these into a single ApiResponse for the 500
status (use either INTERNAL_SERVER_ERROR_SC_V or DB_EXCEPTION_SC_V constant) and
combine their descriptions into one unified message (or reference a shared
schema/ErrorResponse) so only one ApiResponse exists for 500; apply the same
consolidation pattern wherever you see duplicate 500 ApiResponse constants.
In
`@src/main/java/com/iemr/ecd/utils/advice/exception_handler/EcdGlobalExceptionHandler.java`:
- Around line 87-98: In EcdGlobalExceptionHandler, add specific
`@ExceptionHandler` methods for HttpMessageNotReadableException,
MethodArgumentTypeMismatchException, MissingServletRequestParameterException,
and ConstraintViolationException that construct and return an ErrorResponse
(same shape as used in handleGeneralException) with HttpStatus.BAD_REQUEST and
the request path, and place these handlers before the existing
handleGeneralException(Exception ex, HttpServletRequest request) catch-all so
client input errors return 400 instead of falling through to 500; reuse the
ErrorResponse.builder() pattern and logger to log the exception details in each
new handler.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 138bd272-49fb-4405-b2f3-5bd7b14b049c
📒 Files selected for processing (4)
src/main/java/com/iemr/ecd/controller/associate/BeneficiaryRegistrationController.javasrc/main/java/com/iemr/ecd/dto/ErrorResponse.javasrc/main/java/com/iemr/ecd/utils/advice/exception_handler/CustomExceptionResponse.javasrc/main/java/com/iemr/ecd/utils/advice/exception_handler/EcdGlobalExceptionHandler.java
| @ApiResponse(responseCode = CustomExceptionResponse.NOT_FOUND_SC_V, description = CustomExceptionResponse.NOT_FOUND_SC, content = { | ||
| @Content(mediaType = "application/json", schema = @Schema(implementation = ErrorResponse.class)) }), | ||
| @ApiResponse(responseCode = CustomExceptionResponse.INTERNAL_SERVER_ERROR_SC_V, description = CustomExceptionResponse.INTERNAL_SERVER_ERROR_SC, content = { | ||
| @Content(mediaType = "application/json", schema = @Schema(implementation = ErrorResponse.class)) }), | ||
| @ApiResponse(responseCode = CustomExceptionResponse.DB_EXCEPTION_SC_V, description = CustomExceptionResponse.DB_EXCEPTION_SC, content = { | ||
| @Content(mediaType = "application/json", schema = @Schema(implementation = ErrorResponse.class)) }), | ||
| @ApiResponse(responseCode = CustomExceptionResponse.BAD_REQUEST_SC_V, description = CustomExceptionResponse.BAD_REQUEST_SC, content = { | ||
| @Content(mediaType = "application/json", schema = @Schema(implementation = ErrorResponse.class)) }) }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenAPI 3.x, if an operation defines two responses with the same HTTP status code (e.g., two "500" entries), how is that represented in the final schema and UI?
💡 Result:
In OpenAPI 3.x, the operation’s responses are a map keyed by HTTP status code (plus ranges/default). That means you cannot represent “two separate 500 response objects” under the same status code key; only one responses['500'] entry can exist in the resulting parsed schema/JSON/YAML model because each status code key is unique in the map [1][2]. So if you try to write two responses: { '500': ... , '500': ... } in YAML/JSON, the spec structure can only keep one of them (the later one overwrites the earlier one in typical parsers), and there is no native way in OAS3 to merge them as two distinct response definitions under the same HTTP status code. How to model “multiple shapes/examples for the same status code” instead 1) Put the variability inside the single responses['500'] object: - Use a single response with one content[<media-type>].schema that can represent alternatives, e.g. oneOf (or anyOf / discriminators) [3][4]. - Optionally provide multiple examples within that one response/media type (supported by Swagger UI for “multiple examples” in the cited discussions) [5]. 2) In the UI (e.g., Swagger UI): - The UI will show one response block for status 500, because there is only one response entry keyed by 500 in the parsed spec [1][2]. - If you use oneOf/anyOf at the response schema level, many UIs will typically render/select one alternative schema (often the first item) rather than two distinct 500 responses; Swagger UI’s behavior is discussed in its issue/PR history for oneOf/anyOf handling [6]. - If you instead supply multiple examples for that same 500 response, some Swagger UI versions show them as selectable alternatives (again per the cited guidance) [5]. Practical takeaway - You should not define two 500 entries at the top level under responses. - Define a single 500 response, and then model multiple possible body shapes and/or examples within it using oneOf (and/or discriminator mapping) and examples [3][5].
Citations:
- 1: https://spec.openapis.org/oas/v3.2.0.html
- 2: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md
- 3: https://stackoverflow.com/questions/36576447/swagger-specify-two-responses-with-same-code-based-on-optional-parameter
- 4: https://www.baeldung.com/swagger-two-responses-one-response-code
- 5: https://stackoverflow.com/questions/47447403/how-to-define-different-responses-for-same-http-status-code-in-openapi-swagger
- 6: Ft/3803 multiple responses using one of attribute do not appear in ui swagger-api/swagger-ui#5530
🏁 Script executed:
fd -t f CustomExceptionResponseRepository: PSMRI/ECD-API
Length of output: 144
🏁 Script executed:
find . -name "CustomExceptionResponse*" -type fRepository: PSMRI/ECD-API
Length of output: 146
🏁 Script executed:
rg -t java "INTERNAL_SERVER_ERROR_SC_V|DB_EXCEPTION_SC_V" -A 1 -B 1Repository: PSMRI/ECD-API
Length of output: 50369
🏁 Script executed:
wc -l src/main/java/com/iemr/ecd/controller/associate/BeneficiaryRegistrationController.javaRepository: PSMRI/ECD-API
Length of output: 147
🏁 Script executed:
sed -n '50,90p' src/main/java/com/iemr/ecd/controller/associate/BeneficiaryRegistrationController.javaRepository: PSMRI/ECD-API
Length of output: 3661
Merge duplicate 500 responses into a single entry.
Both INTERNAL_SERVER_ERROR_SC_V and DB_EXCEPTION_SC_V map to 500. In OpenAPI 3.x, response objects are keyed by HTTP status code—having two entries with the same code violates the spec structure, with the latter overwriting the former in generated documentation, resulting in inconsistent Swagger output.
Consolidate the two 500 responses into one with a unified description indicating both error scenarios, or use a single schema that can represent both error types.
This pattern appears across multiple endpoints in the codebase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/iemr/ecd/controller/associate/BeneficiaryRegistrationController.java`
around lines 59 - 66, The OpenAPI annotations in
BeneficiaryRegistrationController declare two separate ApiResponse entries for
the same HTTP status (CustomExceptionResponse.INTERNAL_SERVER_ERROR_SC_V and
CustomExceptionResponse.DB_EXCEPTION_SC_V), which conflicts with OpenAPI keys;
update the annotation block to merge these into a single ApiResponse for the 500
status (use either INTERNAL_SERVER_ERROR_SC_V or DB_EXCEPTION_SC_V constant) and
combine their descriptions into one unified message (or reference a shared
schema/ErrorResponse) so only one ApiResponse exists for 500; apply the same
consolidation pattern wherever you see duplicate 500 ApiResponse constants.
| @ExceptionHandler(Exception.class) | ||
| public ResponseEntity<ErrorResponse> handleGeneralException(Exception ex, HttpServletRequest request) { | ||
| logger.error("Unexpected error: ", ex); | ||
| ErrorResponse error = ErrorResponse.builder() | ||
| .timestamp(LocalDateTime.now()) | ||
| .status(HttpStatus.INTERNAL_SERVER_ERROR.value()) | ||
| .error(HttpStatus.INTERNAL_SERVER_ERROR.getReasonPhrase()) | ||
| .message("An unexpected error occurred. Please contact administrator.") | ||
| .path(request.getRequestURI()) | ||
| .build(); | ||
| return new ResponseEntity<>(error, HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether explicit handlers exist for common client-input exceptions
# Expected: no matches currently, indicating they fall through to generic Exception->500.
rg -nC2 --type=java '(`@ExceptionHandler`\()|(HttpMessageNotReadableException|MethodArgumentTypeMismatchException|MissingServletRequestParameterException|ConstraintViolationException)' \
src/main/java/com/iemr/ecd/utils/advice/exception_handler/EcdGlobalExceptionHandler.javaRepository: PSMRI/ECD-API
Length of output: 1103
🏁 Script executed:
cat -n src/main/java/com/iemr/ecd/utils/advice/exception_handler/EcdGlobalExceptionHandler.javaRepository: PSMRI/ECD-API
Length of output: 4781
Add explicit handlers for client-input exceptions before generic 500 fallback.
Malformed JSON, type mismatches, missing parameters, and constraint violations currently fall through the generic Exception handler and return 500. These should return 400 to preserve REST error semantics.
Add handlers for HttpMessageNotReadableException, MethodArgumentTypeMismatchException, MissingServletRequestParameterException, and ConstraintViolationException with HttpStatus.BAD_REQUEST before the catch-all @ExceptionHandler(Exception.class).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/iemr/ecd/utils/advice/exception_handler/EcdGlobalExceptionHandler.java`
around lines 87 - 98, In EcdGlobalExceptionHandler, add specific
`@ExceptionHandler` methods for HttpMessageNotReadableException,
MethodArgumentTypeMismatchException, MissingServletRequestParameterException,
and ConstraintViolationException that construct and return an ErrorResponse
(same shape as used in handleGeneralException) with HttpStatus.BAD_REQUEST and
the request path, and place these handlers before the existing
handleGeneralException(Exception ex, HttpServletRequest request) catch-all so
client input errors return 400 instead of falling through to 500; reuse the
ErrorResponse.builder() pattern and logger to log the exception details in each
new handler.
1 participant
📋 Description
JIRA ID: PSMRI/AMRIT#113
Previously, the API was returning an
HTTP 200 OKstatus for almost all responses, even when an error or exception occurred. This made it difficult for client applications to properly handle errors and broke standard REST semantics.This PR resolves this by introducing a centralized standard for error handling:
ErrorResponseDTO.EcdGlobalExceptionHandlerto catch exceptions and return the actual proper HTTP status codes (400 for bad requests, 401 for unauthorized, 500 for internal errors, etc.).CustomExceptionResponseto use standard HTTP values instead of arbitrary internal integers (5008,5002). This automatically fixes our Swagger documentation so that the@ApiResponsesfinally reflect the true status codes.✅ Type of Change
ℹ️ Additional Information
Testing & Context:
mvn clean compileto ensure no checkstyle violations.SQLException) are safely caught by the global handler and masked behind a generic "Database error occurred" message with a 500 status code, ensuring we don't leak internal DB structures to the client.Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes