Conversation
- Rewrote interface-level Javadoc with full usage example, error response format, and type parameter explanation - Updated method-level Javadoc for getTargetException(), getStatus(), and getMessage() with detailed descriptions - Added @Version, @SInCE, and cross-reference @see tags - Bumped documented version to 1.4.0 Work in progress - not final release
feat(exception): update ApiExceptionTranslator Javadoc
…c exception mapping and pass any strings to AutoResponse annotation for showing custom messages
…into feature/v1.5.0
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/OG4Dev/og4dev-spring-response/sessions/dbd24aed-9c2e-4212-aaa6-a7aa14a09561 Co-authored-by: PasinduOG <126347762+PasinduOG@users.noreply.github.com>
…ing-response into feature/v1.5.0
Update version to 1.5.0
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughPR v1.5.0 introduces a new ChangesCode of Conduct & Governance
Build & Infrastructure
Dynamic Exception Registry (New Feature)
Response Wrapper & Annotation Enhancements
Documentation & Version Updates
Test Coverage
Sequence DiagramsequenceDiagram
participant Client
participant Handler as GlobalExceptionHandler
participant Registry as ApiExceptionRegistry
participant Res as HTTP_Response
Client->>Handler: throws Exception
Handler->>Handler: getErrorLocation(ex)
Handler->>Registry: getRule(exceptionClass)
alt registry has rule
Registry-->>Handler: ExceptionRule(status,message)
Handler->>Res: ProblemDetail(status,message)+traceId+timestamp
else no rule
Handler->>Res: ProblemDetail(500,"An unexpected error occurred")+traceId+timestamp
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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. Comment |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/io/github/og4dev/advice/GlobalResponseWrapper.java (1)
146-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSet JSON content-type only after successful String serialization.
If serialization throws, the method returns the raw string fallback, but the response header may already be
application/json, which can mislead clients.Suggested fix
if (body instanceof String) { try { - response.getHeaders().setContentType(MediaType.APPLICATION_JSON); assert apiResponse != null; - return objectMapper.writeValueAsString(apiResponse); + String serialized = objectMapper.writeValueAsString(apiResponse); + response.getHeaders().setContentType(MediaType.APPLICATION_JSON); + return serialized; } catch (JacksonException e) { return body; } }🤖 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/io/github/og4dev/advice/GlobalResponseWrapper.java` around lines 146 - 153, In GlobalResponseWrapper, avoid setting the JSON content-type before serialization; call objectMapper.writeValueAsString(apiResponse) first and only if it succeeds set response.getHeaders().setContentType(MediaType.APPLICATION_JSON) and return the serialized String, and if objectMapper throws (catching JacksonException) return the original body without modifying headers so the header change happens only on successful serialization.README.md (1)
166-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the corrupted character in the project structure tree.
The
dto/line contains mojibake (├─�� dto/), which breaks readability and copy/paste of the package tree.Suggested fix
-├─�� dto/ +├── dto/🤖 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 `@README.md` around lines 166 - 184, The project tree in README.md contains a corrupted character on the dto entry ("├─�� dto/"); open the README.md and replace that mojibake with the correct box-drawing segment used elsewhere (e.g., "├── dto/") so the line matches the other directory rows and restores copy/paste/readability; update the entry that references ApiResponse.java (the dto/ line) to use the proper characters and save the file.
🧹 Nitpick comments (2)
CODE_OF_CONDUCT.md (1)
51-51: 💤 Low valueOptional: Add hyphen to compound adjective.
When "mutually agreed upon" modifies a noun as a compound adjective, it should be hyphenated: "mutually agreed-upon resolution."
✏️ Proposed grammar fix
-Community Moderators take reports of violations seriously and will make every effort to respond in a timely manner. They will investigate all reports of code of conduct violations, reviewing messages, logs, and recordings, or interviewing witnesses and other participants. Community Moderators will keep investigation and enforcement actions as transparent as possible while prioritizing safety and confidentiality. In order to honor these values, enforcement actions are carried out in private with the involved parties, but communicating to the whole community may be part of a mutually agreed upon resolution. +Community Moderators take reports of violations seriously and will make every effort to respond in a timely manner. They will investigate all reports of code of conduct violations, reviewing messages, logs, and recordings, or interviewing witnesses and other participants. Community Moderators will keep investigation and enforcement actions as transparent as possible while prioritizing safety and confidentiality. In order to honor these values, enforcement actions are carried out in private with the involved parties, but communicating to the whole community may be part of a mutually agreed-upon resolution.As per coding guidelines, this suggestion comes from the static analysis hint regarding grammar conventions for compound adjectives.
🤖 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 `@CODE_OF_CONDUCT.md` at line 51, In the paragraph that ends with "mutually agreed upon resolution", change the compound adjective to use a hyphen: replace "mutually agreed upon resolution" with "mutually agreed-upon resolution" so the phrase correctly hyphenates the compound modifier.src/main/java/io/github/og4dev/config/ApiResponseAutoConfiguration.java (1)
97-99: ⚡ Quick winUse
ObjectProviderorOptionalinstead of@Autowired(required = false)for the optional registry parameter.While
@Autowired(required = false)on@Beanmethod parameters does work in Spring, the official Spring team guidance (GitHub issue#33710) explicitly recommends usingObjectProviderorOptionalto model optional dependencies.ObjectProvideris the preferred approach for@Beanparameters as it provides clearer intent and better support for optional injection scenarios.Suggested change
- public GlobalExceptionHandler apiResponseAdvisor(`@Autowired`(required = false) ApiExceptionRegistry registry) { - return new GlobalExceptionHandler(registry); + public GlobalExceptionHandler apiResponseAdvisor(ObjectProvider<ApiExceptionRegistry> registryProvider) { + return new GlobalExceptionHandler(registryProvider.getIfAvailable()); }🤖 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/io/github/og4dev/config/ApiResponseAutoConfiguration.java` around lines 97 - 99, The apiResponseAdvisor `@Bean` method in ApiResponseAutoConfiguration currently uses `@Autowired`(required = false) on the ApiExceptionRegistry parameter; replace that with an ObjectProvider<ApiExceptionRegistry> (preferred) or Optional<ApiExceptionRegistry> parameter so the optional dependency is modeled explicitly, then obtain the registry via provider.getIfAvailable() (or optional.orElse(null)) and pass it into the GlobalExceptionHandler constructor to preserve the existing behavior.
🤖 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 `@CODE_OF_CONDUCT.md`:
- Line 49: Complete the incomplete reporting sentence in CODE_OF_CONDUCT.md by
adding a clear verb and instruction; replace "To report a possible violation,
**pasinduogdev@gmail.com**" with a full sentence such as "To report a possible
violation, please email pasinduogdev@gmail.com" (or "please contact
pasinduogdev@gmail.com") so the action to take is explicit.
- Line 56: Remove the stray bold-marker artifact "****" from CODE_OF_CONDUCT.md
(the empty bold markers left in the document) so the file contains no empty
formatting; simply delete those four asterisks to eliminate the unintended bold
fragment and ensure surrounding markdown remains unchanged.
In `@src/main/java/io/github/og4dev/config/ApiResponseAutoConfiguration.java`:
- Around line 218-220: The thrown IllegalArgumentException in
ApiResponseAutoConfiguration (the XSS check: throw new
IllegalArgumentException(...)) causes
GlobalExceptionHandler.handleHttpMessageNotReadableException() to return a
generic malformed-JSON message and so violates the `@XssCheck` contract; fix this
by introducing a dedicated exception type (e.g., XssValidationException) and
throw that from the XSS check in ApiResponseAutoConfiguration, then update
GlobalExceptionHandler.handleHttpMessageNotReadableException() (or add a
specific `@ExceptionHandler` for XssValidationException) to return the documented
400 response with detail "Security Error: HTML tags or XSS payloads are not
allowed in the request." so the root-cause message in XssCheck.java is
preserved.
In `@src/main/java/io/github/og4dev/exception/GlobalExceptionHandler.java`:
- Around line 99-105: Add a no-arg constructor to GlobalExceptionHandler that
delegates to the existing constructor which accepts ApiExceptionRegistry so
existing callers/tests can still instantiate it; specifically, add a public
GlobalExceptionHandler() that calls the current constructor (the one with
parameter ApiExceptionRegistry) passing null (or Optional.empty) so behavior is
unchanged while preserving binary compatibility with direct instantiation.
---
Outside diff comments:
In `@README.md`:
- Around line 166-184: The project tree in README.md contains a corrupted
character on the dto entry ("├─�� dto/"); open the README.md and replace that
mojibake with the correct box-drawing segment used elsewhere (e.g., "├── dto/")
so the line matches the other directory rows and restores
copy/paste/readability; update the entry that references ApiResponse.java (the
dto/ line) to use the proper characters and save the file.
In `@src/main/java/io/github/og4dev/advice/GlobalResponseWrapper.java`:
- Around line 146-153: In GlobalResponseWrapper, avoid setting the JSON
content-type before serialization; call
objectMapper.writeValueAsString(apiResponse) first and only if it succeeds set
response.getHeaders().setContentType(MediaType.APPLICATION_JSON) and return the
serialized String, and if objectMapper throws (catching JacksonException) return
the original body without modifying headers so the header change happens only on
successful serialization.
---
Nitpick comments:
In `@CODE_OF_CONDUCT.md`:
- Line 51: In the paragraph that ends with "mutually agreed upon resolution",
change the compound adjective to use a hyphen: replace "mutually agreed upon
resolution" with "mutually agreed-upon resolution" so the phrase correctly
hyphenates the compound modifier.
In `@src/main/java/io/github/og4dev/config/ApiResponseAutoConfiguration.java`:
- Around line 97-99: The apiResponseAdvisor `@Bean` method in
ApiResponseAutoConfiguration currently uses `@Autowired`(required = false) on the
ApiExceptionRegistry parameter; replace that with an
ObjectProvider<ApiExceptionRegistry> (preferred) or
Optional<ApiExceptionRegistry> parameter so the optional dependency is modeled
explicitly, then obtain the registry via provider.getIfAvailable() (or
optional.orElse(null)) and pass it into the GlobalExceptionHandler constructor
to preserve the existing behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 085d68e0-3720-4e7e-aa57-8cb34d71aeaf
📒 Files selected for processing (20)
CODE_OF_CONDUCT.mdREADME.mdpom.xmlsrc/main/java/io/github/og4dev/advice/GlobalResponseWrapper.javasrc/main/java/io/github/og4dev/advice/package-info.javasrc/main/java/io/github/og4dev/annotation/AutoResponse.javasrc/main/java/io/github/og4dev/annotation/AutoTrim.javasrc/main/java/io/github/og4dev/annotation/XssCheck.javasrc/main/java/io/github/og4dev/annotation/package-info.javasrc/main/java/io/github/og4dev/config/ApiResponseAutoConfiguration.javasrc/main/java/io/github/og4dev/config/package-info.javasrc/main/java/io/github/og4dev/dto/ApiResponse.javasrc/main/java/io/github/og4dev/dto/package-info.javasrc/main/java/io/github/og4dev/exception/ApiException.javasrc/main/java/io/github/og4dev/exception/ApiExceptionRegistry.javasrc/main/java/io/github/og4dev/exception/GlobalExceptionHandler.javasrc/main/java/io/github/og4dev/exception/package-info.javasrc/main/java/io/github/og4dev/filter/TraceIdFilter.javasrc/main/java/io/github/og4dev/filter/package-info.javasrc/main/java/io/github/og4dev/package-info.java
|
✅ Created PR with unit tests: #38 |
…or location extraction
…lerTest to use parameterized tests for better coverage
Add unit tests and enhance error logging in GlobalExceptionHandler
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/io/github/og4dev/config/AdvancedStringDeserializerTest.java (1)
138-138: ⚡ Quick winBuild JSON test payloads via Jackson APIs, not string concatenation.
Current JSON construction is brittle for escaped characters (quotes, backslashes) and can fail for parser reasons unrelated to the behavior under test.
Safer payload builder for tests
+ private String jsonValue(String field, String value) { + var node = objectMapper.createObjectNode(); + if (value == null) { + node.putNull(field); + } else { + node.put(field, value); + } + return node.toString(); + } - String json = "{\"value\": \"" + maliciousPayload + "\"}"; + String json = jsonValue("value", maliciousPayload);Also applies to: 171-171, 197-197, 204-204, 242-242, 260-260, 271-271, 280-280, 289-289, 300-300
🤖 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/test/java/io/github/og4dev/config/AdvancedStringDeserializerTest.java` at line 138, The test constructs JSON payloads via string concatenation which breaks on escaped characters; instead use Jackson to build and serialize the payload: create an ObjectNode (or Map) and call objectMapper.writeValueAsString(...) for a node with key "value" and the test variable input, then use that resulting string for parsing; update all occurrences that build json (the String json = "{\"value\": \"" + input + "\"}"; lines in AdvancedStringDeserializerTest, including the other occurrences at the listed locations) to use ObjectMapper/ObjectNode.put("value", input) and writeValueAsString to produce safe, escaped JSON.src/test/java/io/github/og4dev/advice/GlobalResponseWrapperTest.java (1)
300-315: ⚡ Quick winStrengthen the String-response test by asserting parsed JSON fields.
On line 311, the test comment claims "valid JSON," but the current substring-based assertions (
contains()) can pass for malformed payloads. Parse the JSON and assert concrete field values instead.Proposed test hardening
`@Test` void beforeBodyWrite_returnsJsonString_whenBodyIsString() throws Exception { MethodParameter returnType = returnTypeOf(NoAnnotationController.class, "methodAnnotatedDefault"); ServerHttpResponse response = mockResponse(200); Object result = wrapper.beforeBodyWrite( "Hello World", returnType, MediaType.APPLICATION_JSON, null, mock(ServerHttpRequest.class), response); assertThat(result).isInstanceOf(String.class); String jsonResult = (String) result; - // Should be a valid JSON string containing the ApiResponse structure - assertThat(jsonResult).contains("\"message\""); - assertThat(jsonResult).contains("\"status\""); - assertThat(jsonResult).contains("Hello World"); + var json = objectMapper.readTree(jsonResult); + assertThat(json.get("message").asText()).isEqualTo("Success"); + assertThat(json.get("status").asInt()).isEqualTo(HttpStatus.OK.value()); + assertThat(json.get("content").asText()).isEqualTo("Hello World"); }🤖 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/test/java/io/github/og4dev/advice/GlobalResponseWrapperTest.java` around lines 300 - 315, Update the test beforeBodyWrite_returnsJsonString_whenBodyIsString to parse the returned JSON string and assert concrete fields rather than using contains(); call wrapper.beforeBodyWrite with "Hello World" (as already done), cast the result to String, parse it into a JSON object (e.g., via Jackson's ObjectMapper), and assert that the parsed object's "message" equals "Hello World" and "status" equals the expected ApiResponse status; locate this behavior around the test method in GlobalResponseWrapperTest and reference the returnTypeOf(NoAnnotationController.class, "methodAnnotatedDefault") and wrapper.beforeBodyWrite invocation to implement the stronger assertions.
🤖 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/test/java/io/github/og4dev/config/AdvancedStringDeserializerTest.java`:
- Around line 172-174: The test currently uses
assertThatThrownBy(...).isInstanceOf(Exception.class) which is too broad; update
each failing assertion in AdvancedStringDeserializerTest to expect the specific
exception thrown by the XSS rejection path (locate the assertions around
objectMapper.readValue(json, XssCheckFieldDto.class) and similar calls) —
replace Exception.class with the concrete exception your deserializer throws
(e.g., JsonMappingException or your custom XssValidationException) and also
assert the exception message or rootCause contains the XSS-specific indicator
(e.g., "XSS", "invalid input", or your validator message) so the test verifies
the correct failure mode rather than any Exception.
---
Nitpick comments:
In `@src/test/java/io/github/og4dev/advice/GlobalResponseWrapperTest.java`:
- Around line 300-315: Update the test
beforeBodyWrite_returnsJsonString_whenBodyIsString to parse the returned JSON
string and assert concrete fields rather than using contains(); call
wrapper.beforeBodyWrite with "Hello World" (as already done), cast the result to
String, parse it into a JSON object (e.g., via Jackson's ObjectMapper), and
assert that the parsed object's "message" equals "Hello World" and "status"
equals the expected ApiResponse status; locate this behavior around the test
method in GlobalResponseWrapperTest and reference the
returnTypeOf(NoAnnotationController.class, "methodAnnotatedDefault") and
wrapper.beforeBodyWrite invocation to implement the stronger assertions.
In `@src/test/java/io/github/og4dev/config/AdvancedStringDeserializerTest.java`:
- Line 138: The test constructs JSON payloads via string concatenation which
breaks on escaped characters; instead use Jackson to build and serialize the
payload: create an ObjectNode (or Map) and call
objectMapper.writeValueAsString(...) for a node with key "value" and the test
variable input, then use that resulting string for parsing; update all
occurrences that build json (the String json = "{\"value\": \"" + input + "\"}";
lines in AdvancedStringDeserializerTest, including the other occurrences at the
listed locations) to use ObjectMapper/ObjectNode.put("value", input) and
writeValueAsString to produce safe, escaped JSON.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8eeec5a3-9eb4-4192-a16b-7edb8b0ad508
📒 Files selected for processing (7)
pom.xmlsrc/main/java/io/github/og4dev/exception/GlobalExceptionHandler.javasrc/test/java/io/github/og4dev/advice/GlobalResponseWrapperTest.javasrc/test/java/io/github/og4dev/annotation/AutoResponseAnnotationTest.javasrc/test/java/io/github/og4dev/config/AdvancedStringDeserializerTest.javasrc/test/java/io/github/og4dev/exception/ApiExceptionRegistryTest.javasrc/test/java/io/github/og4dev/exception/GlobalExceptionHandlerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
| assertThatThrownBy(() -> objectMapper.readValue(json, XssCheckFieldDto.class)) | ||
| .isInstanceOf(Exception.class); | ||
| } |
There was a problem hiding this comment.
Use specific failure assertions instead of Exception.class.
Asserting Exception is too broad and can produce false positives from unrelated failures. Tighten these to the expected exception type/cause from XSS rejection.
Suggested tightening pattern
- assertThatThrownBy(() -> objectMapper.readValue(json, XssCheckFieldDto.class))
- .isInstanceOf(Exception.class);
+ assertThatThrownBy(() -> objectMapper.readValue(json, XssCheckFieldDto.class))
+ .hasRootCauseInstanceOf(IllegalArgumentException.class);Also applies to: 205-207, 243-245, 261-263, 281-283
🤖 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/test/java/io/github/og4dev/config/AdvancedStringDeserializerTest.java`
around lines 172 - 174, The test currently uses
assertThatThrownBy(...).isInstanceOf(Exception.class) which is too broad; update
each failing assertion in AdvancedStringDeserializerTest to expect the specific
exception thrown by the XSS rejection path (locate the assertions around
objectMapper.readValue(json, XssCheckFieldDto.class) and similar calls) —
replace Exception.class with the concrete exception your deserializer throws
(e.g., JsonMappingException or your custom XssValidationException) and also
assert the exception message or rootCause contains the XSS-specific indicator
(e.g., "XSS", "invalid input", or your validator message) so the test verifies
the correct failure mode rather than any Exception.
…s and add XssValidationException
…or unreadable HTTP message exceptions
…tion and XSS exceptions
…rove readability for malformed JSON requests
refactor: update version references to 1.5.0-RC1 across multiple files and add XssValidationException
This pull request introduces version 1.5.0 of the library, bringing a major new feature for exception handling and several documentation improvements. The highlight is the addition of a dynamic exception registry, enabling developers to map third-party or framework-specific exceptions to standardized HTTP responses without custom handler code. The documentation and build files are updated to reflect the new version and features, and a Contributor Covenant Code of Conduct is added to the repository.
Key changes:
New Features
Introduced
ApiExceptionRegistryfor centralized, thread-safe mapping of third-party exceptions (e.g., SQL, Security) to standard HTTP responses using a builder pattern. This allows external exceptions to be handled automatically by the global exception handler, reducing boilerplate and improving maintainability. [1] [2]Documentation Updates
@AutoTrim,@XssCheck) were introduced in v1.3.0, not v1.4.0.Build and Versioning
pom.xmlto version 1.5.0.Community and Governance
CODE_OF_CONDUCT.mdusing Contributor Covenant 3.0 to establish clear community standards and enforcement guidelines.Summary by CodeRabbit
New Features
Improvements
Documentation
Dependencies
Tests