gnodet commented on PR #21905:
URL: https://github.com/apache/camel/pull/21905#issuecomment-4030537030
Issues found
Moderate:
1. No tests for DiagnoseResources — The new resource endpoints have no
tests. At minimum should
cover: exceptionCatalog() returns valid JSON, exceptionDetail() for a
known exception, and the
not-found case.
2. Constant duplication — CAMEL_DOC_BASE, CAMEL_COMPONENT_DOC, and
CAMEL_EIP_DOC are defined in
both DiagnoseData and DiagnoseTools. Should be consolidated into one place
(e.g., DiagnoseData with
public access).
3. JSON serialization duplication — The ExceptionInfo → JSON logic is
repeated across
DiagnoseTools.camel_error_diagnose(),
DiagnoseResources.exceptionCatalog(), and
DiagnoseResources.exceptionDetail(). A helper like ExceptionInfo.toJson()
would reduce this.
Nits:
4. Arrays.asList() vs List.of() — Project targets JDK 21+, List.of() would
be more idiomatic for
these immutable lists.
Positives
- Clean refactoring, good motivation (shared data as reusable bean + MCP
resources)
- Good use of records for ExceptionInfo
- exceptionDetail() properly handles not-found case
- Existing DiagnoseToolsTest updated to inject DiagnoseData directly
Overall a solid PR, the main gap is missing test coverage for the new
DiagnoseResources class.
--
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]
