wy471x commented on PR #6457: URL: https://github.com/apache/shenyu/pull/6457#issuecomment-5158343522
> Reviewed #6457. The core fix is sound: `EmailAlertNotifyStrategy.buildAlertHtmlTemplate` previously dereferenced `alert.getContent()`/`alert.getDateCreated()` _before_ the dead `Objects.isNull(alert)` guard; the new ordering with `dateCreated == null → new Date()` is correct and is reachable via the HTTP report path (a caller omitting `dateCreated`). `EmailAlertNotifyStrategyTest.testNullDateCreatedShouldNotThrowNpe` backs that path. Moving `@NotBlank` from the shared `AlarmContent` DTO to a controller-layer `AlertReportRequest` is the right call — it avoids silently dropping gateway-generated alerts (which bypass the controller via `AlarmSender` → `AlertDispatchService.dispatchAlert`) when content is null. > > Non-blocking issues worth addressing before/after merge: > > 1. **PR description is stale.** The body still claims `@NotBlank` was added to `AlarmContent`, that `jakarta.validation-api` was added to `shenyu-common/pom.xml`, and that `AlarmContentTest` verifies `@NotBlank`. None of that is true in the final diff — `AlarmContent` is unmodified, the only pom change is a trailing-whitespace trim, and `AlarmContentTest` only tests builder/setters. Please rewrite the body to match the merged approach (controller-layer `AlertReportRequest`). > 2. **No test backs the new validation or the request→AlarmContent mapping.** `AlertReportController.reportAlert` now manually copies 7 fields and relies on `@NotBlank`, but there is no `AlertReportControllerTest` (the repo has many `*ControllerTest` examples). A `MockMvc` test asserting (a) blank title/body → 400 with the validation message and (b) a valid request maps through to `dispatchAlert` with the right fields would close the gap. > 3. **`shenyu-common/pom.xml` whitespace-only hunk** — drop it; it serves no purpose in the final approach. > 4. Minor: the `alert == null` branch in `buildAlertHtmlTemplate` is unreachable (`AlertDispatchServiceImpl.DispatchTask` already guards `Objects.nonNull(alert)` before invoking handlers), while `DingTalkRobotAlertNotifyStrategy.send()` still dereferences `alert.getTitle()` unguarded. Either apply the guard consistently or drop the unreachable `alert == null` part and keep only the real `dateCreated == null` fix. > > CI note: `it` / `shenyu-integrated-test-combination` failed (other `it` jobs cancelled); `ci` and `e2e` are green. Please confirm the combination failure is unrelated/flaky before merging. I have fixed all the issues you mentioned, as well as the CI problems. I have already verified that they pass in my own fork repository. Thank you for the code review. -- 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]
