RockteMQ-AI commented on PR #2949: URL: https://github.com/apache/rocketmq-dashboard/pull/2949#issuecomment-5502579594
## Review ### 1. Summary The PR hardens `BusinessException` so that its status code is always in the HTTP 4xx/5xx error band. Codes outside 400–599 are silently normalized to 400 before `GlobalExceptionHandler` uses them in `ResponseEntity.status(...)`, preventing `HttpStatus.valueOf(...)` from throwing on invalid codes and avoiding success/redirect HTTP statuses for business errors. A focused unit-test class is added to cover pass-through, fallback, and message preservation. ### 2. Bugs / logic errors / potential issues - **Malformed test annotations in the diff.** The three test methods appear to be annotated with a file path (`@server/src/main/java/org/apache/rocketmq/studio/settings/DataSourceTestDTO.java`) instead of `@Test`. If this is not just a diff-rendering artifact, the test class will not run at all. Please verify the actual file uses `@Test`. - **Silent fallback may hide caller bugs.** Normalizing `0`, `200`, or `600` to `400` is the right HTTP-layer behavior, but it makes it harder to notice that a caller passed a nonsensical code. Consider adding a `log.warn(...)` when normalization happens. - **Magic numbers.** `400` and `599` are literal; extracting named constants (e.g. `MIN_ERROR_CODE`, `MAX_ERROR_CODE`, `FALLBACK_CODE`) would make the guard condition self-documenting. ### 3. Thread safety / resource leaks No concerns. `BusinessException` is immutable (`code` is `private final`), has no shared mutable state, and introduces no I/O or resources. ### 4. Backward compatibility Safe. I checked all production call sites and existing test usages; every one already uses codes in the 400–599 range (400, 401, 403, 404, 409, 422, 429, 500, 501, 502, 503, 504). No existing behavior changes. ### 5. Actionable suggestions - `BusinessException.java:25-28` — verify the final constructor uses a clean expression; optionally replace the inline ternary with a small `normalizeCode(int code)` helper and constants. - `BusinessExceptionTest.java` — ensure all three test methods are annotated with `@Test`. Also add boundary assertions for `399` and `600` to make the off-by-one behavior explicit. - `GlobalExceptionHandlerTest.java:94-97` — consider adding a case in `FailingController` that throws an out-of-band code (e.g. `0` or `600`) and assert the HTTP status is 400, covering the handler path end-to-end. Overall this is a small, defensive fix with good test coverage once the annotation issue is confirmed fixed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
