fix(app): 修复 AppController 安全与查询问题
- 加强异常处理、类型安全和图片上传校验 - 优化设备相关查询，避免重复访问数据源 - 补充并记录并发测试与审查修复实施计划
This commit is contained in:
yuhaiming
@@ -0,0 +1,61 @@
|
||||
# AppController Concurrency Test Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Add repeatable 32-thread unit concurrency tests for the singleton `AppController` without live infrastructure.
|
||||
|
||||
**Architecture:** Create one standalone JUnit 5 test class with a shared controller instance and mocked collaborators. A fixed thread pool, ready latch, start latch, and bounded `Future#get` synchronize each test; concurrent collections retain observations for deterministic assertions after workers finish.
|
||||
|
||||
**Tech Stack:** Java 17, JUnit 5, Mockito, AssertJ, Spring `MockMultipartFile`, Maven Surefire.
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- Do not modify production code unless a concurrency failure is reproduced.
|
||||
- Use exactly 32 worker threads per test.
|
||||
- Do not connect to MySQL, Redis, MQTT, or OSS.
|
||||
- Every future must have a bounded timeout.
|
||||
- Use one shared `AppController` instance within each test.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Add AppController Concurrency Coverage
|
||||
|
||||
**Files:**
|
||||
- Create: `water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerConcurrencyTest.java`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `AppController.switchDevice(String)`, `uploadImage(MultipartFile)`, and `editScheduleStatus(AppScheduleBo)`.
|
||||
- Produces: three repeatable JUnit concurrency tests and shared executor helpers.
|
||||
|
||||
- [ ] **Step 1: Create the concurrency test fixture**
|
||||
|
||||
Add Mockito mocks for all constructor dependencies, construct one controller per test, register an `ObjectMapper` in a `GenericApplicationContext`, and add a helper that submits 32 workers behind ready/start latches.
|
||||
|
||||
- [ ] **Step 2: Add concurrent timestamp formatting test**
|
||||
|
||||
Run 32 workers with 100 calls each. Capture every `startTime`, require 3,200 successful responses and invocations, and parse every captured value with strict `yyyy-MM-dd HH:mm:ss` formatting.
|
||||
|
||||
- [ ] **Step 3: Add concurrent image validation test**
|
||||
|
||||
Run 32 workers with 50 uploads each, cycling through JPEG, PNG, GIF, WebP, and BMP signatures. Require 1,600 successful responses and OSS calls.
|
||||
|
||||
- [ ] **Step 4: Add concurrent schedule payload isolation test**
|
||||
|
||||
Run 32 workers with 20 unique schedules each. Scope `LoginHelper.getUserId()` to each worker, generate a unique device and detail marker per schedule, capture MQTT payloads in a `ConcurrentHashMap`, and verify all 640 payloads contain their matching values.
|
||||
|
||||
- [ ] **Step 5: Run the focused concurrency test**
|
||||
|
||||
```powershell
|
||||
mvn -pl water-modules/water-app -am "-DskipTests=false" "-Dmaven.test.skip=false" "-Dprofiles.active=dev" "-Dtest=AppControllerConcurrencyTest" "-Dsurefire.failIfNoSpecifiedTests=false" test
|
||||
```
|
||||
|
||||
Expected: 3 tests pass with zero failures, errors, and timeouts.
|
||||
|
||||
- [ ] **Step 6: Run the full water-app suite and diff checks**
|
||||
|
||||
```powershell
|
||||
mvn -pl water-modules/water-app -am "-DskipTests=false" "-Dmaven.test.skip=false" "-Dprofiles.active=dev" test
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Expected: reactor `BUILD SUCCESS`; all tests pass; `git diff --check` exits 0.
|
||||
@@ -0,0 +1,689 @@
|
||||
# AppController Review Remediation Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Fix the approved exception-safety, type-safety, N+1-query, and image-upload findings while preserving every existing `/app/v1/**` route.
|
||||
|
||||
**Architecture:** Keep `AppController` in place for this phase, but move database batching behind existing service interfaces. Route all non-degraded exceptions through `GlobalExceptionHandler`, validate image bytes before OSS, and cover behavior through focused unit and standalone MVC tests.
|
||||
|
||||
**Tech Stack:** Java 17, Spring Boot 3.5, Spring MVC, MyBatis Plus, Hutool, JUnit 5, Mockito, AssertJ, Maven.
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- Do not split `AppController` in this phase.
|
||||
- Preserve existing route paths, request field names, successful response payloads, MQTT topics, and database schema.
|
||||
- Unexpected system exceptions must not expose their original messages to clients.
|
||||
- Image uploads are limited to 20 MB and to JPEG, PNG, GIF, WebP, and BMP detected by file signature.
|
||||
- Keep the four intentional MQTT notification degradation catches and include the exception object in each log call.
|
||||
- Do not revert unrelated dirty-worktree changes.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Make Global Exception Responses Safe And Observable
|
||||
|
||||
**Files:**
|
||||
- Modify: `water-common/water-common-web/pom.xml`
|
||||
- Modify: `water-common/water-common-web/src/main/java/org/dromara/common/web/handler/GlobalExceptionHandler.java:60-167`
|
||||
- Create: `water-common/water-common-web/src/test/java/org/dromara/common/web/handler/GlobalExceptionHandlerTest.java`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `ServiceException`, `BaseException`, `R<Void>`, `HttpServletRequest`.
|
||||
- Produces: unchanged handler method signatures; generic system failures return `R.fail()` instead of `e.getMessage()`.
|
||||
|
||||
- [ ] **Step 1: Add the test dependency and failing exception-handler tests**
|
||||
|
||||
Add this dependency to `water-common-web/pom.xml`:
|
||||
|
||||
```xml
|
||||
<dependency>
|
||||
<groupId>org.springframework.boot</groupId>
|
||||
<artifactId>spring-boot-starter-test</artifactId>
|
||||
<scope>test</scope>
|
||||
</dependency>
|
||||
```
|
||||
|
||||
Create `GlobalExceptionHandlerTest` with these behaviors:
|
||||
|
||||
```java
|
||||
@ExtendWith(MockitoExtension.class)
|
||||
class GlobalExceptionHandlerTest {
|
||||
|
||||
@Mock
|
||||
private HttpServletRequest request;
|
||||
|
||||
private GlobalExceptionHandler handler;
|
||||
|
||||
@BeforeEach
|
||||
void setUp() {
|
||||
handler = new GlobalExceptionHandler();
|
||||
when(request.getRequestURI()).thenReturn("/app/v1/device/D01");
|
||||
}
|
||||
|
||||
@Test
|
||||
void handleRuntimeException_hidesInternalMessage() {
|
||||
R<Void> result = handler.handleRuntimeException(
|
||||
new IllegalStateException("jdbc:mysql://internal-host/water"), request);
|
||||
|
||||
assertThat(result.getCode()).isEqualTo(R.FAIL);
|
||||
assertThat(result.getMsg()).doesNotContain("internal-host");
|
||||
}
|
||||
|
||||
@Test
|
||||
void handleBaseException_logsThrowableAndRequestUri() {
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(GlobalExceptionHandler.class);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
try {
|
||||
handler.handleBaseException(new BaseException("user", "业务校验失败"), request);
|
||||
|
||||
ILoggingEvent event = appender.list.get(appender.list.size() - 1);
|
||||
assertThat(event.getLevel()).isEqualTo(Level.WARN);
|
||||
assertThat(event.getFormattedMessage()).contains("/app/v1/device/D01");
|
||||
assertThat(event.getThrowableProxy()).isNotNull();
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the tests and verify RED**
|
||||
|
||||
Run:
|
||||
|
||||
```powershell
|
||||
mvn -pl water-common/water-common-web -am "-DskipTests=false" "-Dmaven.test.skip=false" "-Dtest=GlobalExceptionHandlerTest" "-Dsurefire.failIfNoSpecifiedTests=false" test
|
||||
```
|
||||
|
||||
Expected: failures because runtime messages are currently returned unchanged and `BaseException` logs have no throwable at `ERROR` level.
|
||||
|
||||
- [ ] **Step 3: Implement the minimal handler changes**
|
||||
|
||||
Apply these handler rules:
|
||||
|
||||
```java
|
||||
@ExceptionHandler(ServiceException.class)
|
||||
public R<Void> handleServiceException(ServiceException e, HttpServletRequest request) {
|
||||
String requestURI = request.getRequestURI();
|
||||
String message = resolveErrorMessage(e.getMessage());
|
||||
log.warn("请求地址'{}',发生业务异常'{}'", requestURI, message, e);
|
||||
Integer code = e.getCode();
|
||||
return ObjectUtil.isNotNull(code) ? R.fail(code, message) : R.fail(message);
|
||||
}
|
||||
|
||||
@ExceptionHandler(BaseException.class)
|
||||
public R<Void> handleBaseException(BaseException e, HttpServletRequest request) {
|
||||
String requestURI = request.getRequestURI();
|
||||
String message = e.getMessage();
|
||||
log.warn("请求地址'{}',发生业务异常'{}'", requestURI, message, e);
|
||||
return R.fail(message);
|
||||
}
|
||||
|
||||
@ExceptionHandler(RuntimeException.class)
|
||||
public R<Void> handleRuntimeException(RuntimeException e, HttpServletRequest request) {
|
||||
String requestURI = request.getRequestURI();
|
||||
log.error("请求地址'{}',发生未知异常.", requestURI, e);
|
||||
return R.fail();
|
||||
}
|
||||
|
||||
@ExceptionHandler(Exception.class)
|
||||
public R<Void> handleException(Exception e, HttpServletRequest request) {
|
||||
String requestURI = request.getRequestURI();
|
||||
log.error("请求地址'{}',发生系统异常.", requestURI, e);
|
||||
return R.fail();
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run the focused tests and verify GREEN**
|
||||
|
||||
Run the command from Step 2. Expected: `GlobalExceptionHandlerTest` passes with zero failures.
|
||||
|
||||
- [ ] **Step 5: Record the Task 1 diff checkpoint**
|
||||
|
||||
```powershell
|
||||
git diff -- water-common/water-common-web/pom.xml water-common/water-common-web/src/main/java/org/dromara/common/web/handler/GlobalExceptionHandler.java water-common/water-common-web/src/test/java/org/dromara/common/web/handler/GlobalExceptionHandlerTest.java
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Validate Image Upload Content And Size
|
||||
|
||||
**Files:**
|
||||
- Modify: `water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerTest.java:131-169`
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java:593-612`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `MultipartFile` and existing `ISysOssService.upload(MultipartFile)`.
|
||||
- Produces: unchanged `POST /app/v1/uploadImage`; invalid files throw `ServiceException` before OSS is called.
|
||||
|
||||
- [ ] **Step 1: Write failing upload validation tests**
|
||||
|
||||
Replace the fake valid PNG bytes with a real signature helper and add forged, oversized, and SVG cases:
|
||||
|
||||
```java
|
||||
private byte[] pngBytes() {
|
||||
return new byte[]{
|
||||
(byte) 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A,
|
||||
0x00, 0x00, 0x00, 0x0D, 0x49, 0x48, 0x44, 0x52
|
||||
};
|
||||
}
|
||||
|
||||
@Test
|
||||
void uploadImage_rejectsForgedImageContent() {
|
||||
AppController controller = newController();
|
||||
MockMultipartFile file = new MockMultipartFile(
|
||||
"file", "plant.png", "image/png", "not-an-image".getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
assertThatThrownBy(() -> callWithLogin(1L, () -> controller.uploadImage(file)))
|
||||
.isInstanceOf(ServiceException.class)
|
||||
.hasMessage("图片文件内容不合法");
|
||||
verify(ossService, never()).upload(any(MultipartFile.class));
|
||||
}
|
||||
|
||||
@Test
|
||||
void uploadImage_rejectsFileLargerThanTwentyMegabytes() {
|
||||
AppController controller = newController();
|
||||
MultipartFile file = mock(MultipartFile.class);
|
||||
when(file.isEmpty()).thenReturn(false);
|
||||
when(file.getSize()).thenReturn(20L * 1024 * 1024 + 1);
|
||||
|
||||
assertThatThrownBy(() -> callWithLogin(1L, () -> controller.uploadImage(file)))
|
||||
.isInstanceOf(ServiceException.class)
|
||||
.hasMessage("图片大小不能超过20MB");
|
||||
verify(ossService, never()).upload(any(MultipartFile.class));
|
||||
}
|
||||
|
||||
@Test
|
||||
void uploadImage_rejectsSvg() {
|
||||
AppController controller = newController();
|
||||
MockMultipartFile file = new MockMultipartFile(
|
||||
"file", "plant.svg", "image/svg+xml", "<svg/>".getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
assertThatThrownBy(() -> callWithLogin(1L, () -> controller.uploadImage(file)))
|
||||
.isInstanceOf(ServiceException.class)
|
||||
.hasMessage("仅支持jpg、jpeg、png、gif、webp、bmp图片");
|
||||
verify(ossService, never()).upload(any(MultipartFile.class));
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run upload tests and verify RED**
|
||||
|
||||
Run:
|
||||
|
||||
```powershell
|
||||
mvn -pl water-modules/water-app -am "-DskipTests=false" "-Dmaven.test.skip=false" "-Dprofiles.active=dev" "-Dtest=AppControllerTest" "-Dsurefire.failIfNoSpecifiedTests=false" test
|
||||
```
|
||||
|
||||
Expected: the forged and oversized tests fail because current code trusts `Content-Type` and has no endpoint-specific limit.
|
||||
|
||||
- [ ] **Step 3: Implement image validation**
|
||||
|
||||
Add constants and mappings:
|
||||
|
||||
```java
|
||||
private static final long MAX_IMAGE_SIZE_BYTES = 20L * 1024 * 1024;
|
||||
private static final Set<String> ALLOWED_IMAGE_EXTENSIONS =
|
||||
Set.of("jpg", "jpeg", "png", "gif", "webp", "bmp");
|
||||
private static final Map<String, String> IMAGE_CONTENT_TYPES = Map.of(
|
||||
"jpg", "image/jpeg",
|
||||
"png", "image/png",
|
||||
"gif", "image/gif",
|
||||
"webp", "image/webp",
|
||||
"bmp", "image/bmp"
|
||||
);
|
||||
```
|
||||
|
||||
Call `validateImageFile(file)` before OSS. Implement it with `FileUtil.extName` and `FileTypeUtil.getType(file.getInputStream())`, normalize `jpeg` to `jpg`, and enforce extension/MIME/signature compatibility:
|
||||
|
||||
```java
|
||||
private void validateImageFile(MultipartFile file) {
|
||||
if (file == null || file.isEmpty()) {
|
||||
throw new ServiceException("上传图片不能为空");
|
||||
}
|
||||
if (file.getSize() > MAX_IMAGE_SIZE_BYTES) {
|
||||
throw new ServiceException("图片大小不能超过20MB");
|
||||
}
|
||||
String extension = StringUtils.lowerCase(FileUtil.extName(file.getOriginalFilename()));
|
||||
if (!ALLOWED_IMAGE_EXTENSIONS.contains(extension)) {
|
||||
throw new ServiceException("仅支持jpg、jpeg、png、gif、webp、bmp图片");
|
||||
}
|
||||
String contentType = StringUtils.lowerCase(file.getContentType());
|
||||
String normalizedExtension = "jpeg".equals(extension) ? "jpg" : extension;
|
||||
if (!IMAGE_CONTENT_TYPES.get(normalizedExtension).equals(contentType)) {
|
||||
throw new ServiceException("图片文件类型不匹配");
|
||||
}
|
||||
try (InputStream inputStream = file.getInputStream()) {
|
||||
String detectedType = StringUtils.lowerCase(FileTypeUtil.getType(inputStream));
|
||||
String normalizedDetectedType = "jpeg".equals(detectedType) ? "jpg" : detectedType;
|
||||
if (!normalizedExtension.equals(normalizedDetectedType)) {
|
||||
throw new ServiceException("图片文件内容不合法");
|
||||
}
|
||||
} catch (IOException e) {
|
||||
throw new ServiceException("读取图片文件失败");
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run AppController tests and verify GREEN**
|
||||
|
||||
Run the command from Step 2. Expected: all existing and new upload tests pass.
|
||||
|
||||
- [ ] **Step 5: Record the Task 2 diff checkpoint**
|
||||
|
||||
```powershell
|
||||
git diff -- water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerTest.java
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Add Bounded Batch Query Service APIs
|
||||
|
||||
**Files:**
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/service/IAppDeviceService.java`
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/service/impl/AppDeviceServiceImpl.java`
|
||||
- Modify: `water-modules/water-app/src/test/java/org/dromara/app/service/impl/AppDeviceServiceImplTest.java`
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/service/IAppSchedulingDeviceService.java`
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/service/impl/AppSchedulingDeviceServiceImpl.java`
|
||||
- Create: `water-modules/water-app/src/test/java/org/dromara/app/service/impl/AppSchedulingDeviceServiceImplTest.java`
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: `List<AppDeviceVo> queryByDeviceNos(Collection<String> deviceNos)`.
|
||||
- Produces: `Set<String> findBoundDeviceNos(Collection<String> deviceNos)`.
|
||||
|
||||
- [ ] **Step 1: Write failing service tests against the desired APIs**
|
||||
|
||||
Add to `AppDeviceServiceImplTest`:
|
||||
|
||||
```java
|
||||
@Test
|
||||
void queryByDeviceNos_delegatesToSingleBatchQuery() {
|
||||
AppDeviceServiceImpl service = new AppDeviceServiceImpl(
|
||||
appDeviceMapper, schedulingDeviceMapper, wateringLogMapper, wateringLogService);
|
||||
List<String> deviceNos = List.of("D01", "D02");
|
||||
List<AppDeviceVo> expected = List.of(new AppDeviceVo(), new AppDeviceVo());
|
||||
when(appDeviceMapper.selectVoByIds(deviceNos)).thenReturn(expected);
|
||||
|
||||
assertThat(service.queryByDeviceNos(deviceNos)).isSameAs(expected);
|
||||
verify(appDeviceMapper).selectVoByIds(deviceNos);
|
||||
}
|
||||
```
|
||||
|
||||
Create `AppSchedulingDeviceServiceImplTest`:
|
||||
|
||||
```java
|
||||
@ExtendWith(MockitoExtension.class)
|
||||
class AppSchedulingDeviceServiceImplTest {
|
||||
|
||||
@Mock
|
||||
private AppSchedulingDeviceMapper mapper;
|
||||
|
||||
@Test
|
||||
void findBoundDeviceNos_usesOneConstrainedQuery() {
|
||||
AppSchedulingDeviceServiceImpl service = new AppSchedulingDeviceServiceImpl(mapper);
|
||||
AppSchedulingDevice first = new AppSchedulingDevice();
|
||||
first.setDeviceNo("D01");
|
||||
AppSchedulingDevice duplicate = new AppSchedulingDevice();
|
||||
duplicate.setDeviceNo("D01");
|
||||
when(mapper.selectList(any(Wrapper.class))).thenReturn(List.of(first, duplicate));
|
||||
|
||||
Set<String> result = service.findBoundDeviceNos(List.of("D01", "D02"));
|
||||
|
||||
assertThat(result).containsExactly("D01");
|
||||
verify(mapper, times(1)).selectList(any(Wrapper.class));
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run service tests and verify RED**
|
||||
|
||||
Run:
|
||||
|
||||
```powershell
|
||||
mvn -pl water-modules/water-app -am "-DskipTests=false" "-Dmaven.test.skip=false" "-Dprofiles.active=dev" "-Dtest=AppDeviceServiceImplTest,AppSchedulingDeviceServiceImplTest" "-Dsurefire.failIfNoSpecifiedTests=false" test
|
||||
```
|
||||
|
||||
Expected: test compilation fails because the two batch APIs do not exist yet.
|
||||
|
||||
- [ ] **Step 3: Add the interfaces and minimal implementations**
|
||||
|
||||
Add to `IAppDeviceService` and `AppDeviceServiceImpl`:
|
||||
|
||||
```java
|
||||
List<AppDeviceVo> queryByDeviceNos(Collection<String> deviceNos);
|
||||
```
|
||||
|
||||
```java
|
||||
@Override
|
||||
public List<AppDeviceVo> queryByDeviceNos(Collection<String> deviceNos) {
|
||||
if (deviceNos == null || deviceNos.isEmpty()) {
|
||||
return List.of();
|
||||
}
|
||||
return baseMapper.selectVoByIds(deviceNos);
|
||||
}
|
||||
```
|
||||
|
||||
Add to `IAppSchedulingDeviceService` and `AppSchedulingDeviceServiceImpl`:
|
||||
|
||||
```java
|
||||
Set<String> findBoundDeviceNos(Collection<String> deviceNos);
|
||||
```
|
||||
|
||||
```java
|
||||
@Override
|
||||
public Set<String> findBoundDeviceNos(Collection<String> deviceNos) {
|
||||
if (deviceNos == null || deviceNos.isEmpty()) {
|
||||
return Set.of();
|
||||
}
|
||||
return baseMapper.selectList(
|
||||
Wrappers.<AppSchedulingDevice>lambdaQuery()
|
||||
.select(AppSchedulingDevice::getDeviceNo)
|
||||
.in(AppSchedulingDevice::getDeviceNo, deviceNos))
|
||||
.stream()
|
||||
.map(AppSchedulingDevice::getDeviceNo)
|
||||
.filter(StringUtils::isNotBlank)
|
||||
.collect(Collectors.toSet());
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run service tests and verify GREEN**
|
||||
|
||||
Run the command from Step 2. Expected: both service test classes pass.
|
||||
|
||||
- [ ] **Step 5: Record the Task 3 diff checkpoint**
|
||||
|
||||
```powershell
|
||||
git diff -- water-modules/water-app/src/main/java/org/dromara/app/service water-modules/water-app/src/test/java/org/dromara/app/service/impl
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Replace Controller N+1 Queries
|
||||
|
||||
**Files:**
|
||||
- Modify: `water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerTest.java`
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java:175-195`
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java:357-400`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `IAppDeviceService.queryByDeviceNos` from Task 3.
|
||||
- Consumes: `IAppSchedulingDeviceService.findBoundDeviceNos` from Task 3.
|
||||
- Preserves: `GET /app/v1/scheduleDeviceList/{scheduleId}` and `GET /app/v1/schedule/{id}` payloads.
|
||||
|
||||
- [ ] **Step 1: Write failing Controller interaction tests**
|
||||
|
||||
Add these tests:
|
||||
|
||||
```java
|
||||
@Test
|
||||
void scheduleDeviceList_usesSingleBatchBindingLookup() {
|
||||
AppController controller = newController();
|
||||
AppDeviceVo first = ownedDevice("D01", 99L);
|
||||
AppDeviceVo second = ownedDevice("D02", 99L);
|
||||
when(appDeviceService.queryList(any(AppDeviceBo.class))).thenReturn(List.of(first, second));
|
||||
when(appSchedulingDeviceService.findBoundDeviceNos(List.of("D01", "D02")))
|
||||
.thenReturn(Set.of("D01"));
|
||||
|
||||
R<Map<String, Object>> result = callWithLogin(99L, () -> controller.scheduleDeviceList(10L));
|
||||
|
||||
assertThat((List<AppDeviceVo>) result.getData().get("deviceList"))
|
||||
.extracting(AppDeviceVo::getDeviceNo)
|
||||
.containsExactly("D02");
|
||||
verify(appSchedulingDeviceService, never()).findByDeviceNo(anyString());
|
||||
}
|
||||
|
||||
@Test
|
||||
void getScheduleInfo_usesSingleBatchDeviceLookup() {
|
||||
AppController controller = newController();
|
||||
AppScheduleVo schedule = ownedSchedule(10L, 99L);
|
||||
AppSchedulingDeviceVo firstBinding = new AppSchedulingDeviceVo();
|
||||
firstBinding.setDeviceNo("D01");
|
||||
AppSchedulingDeviceVo secondBinding = new AppSchedulingDeviceVo();
|
||||
secondBinding.setDeviceNo("D02");
|
||||
List<AppDeviceVo> devices = List.of(ownedDevice("D01", 99L), ownedDevice("D02", 99L));
|
||||
when(appScheduleService.queryById(10L)).thenReturn(schedule);
|
||||
when(appScheduleDetailService.queryByScheduleIdByStatus(10L)).thenReturn(List.of());
|
||||
when(appSchedulingDeviceService.queryList(any(AppSchedulingDeviceBo.class)))
|
||||
.thenReturn(List.of(firstBinding, secondBinding));
|
||||
when(appDeviceService.queryByDeviceNos(List.of("D01", "D02"))).thenReturn(devices);
|
||||
|
||||
R<AppScheduleVo> result = callWithLogin(99L, () -> controller.getScheduleInfo(10L));
|
||||
|
||||
assertThat(result.getData().getDeviceNos()).isEqualTo(devices);
|
||||
verify(appDeviceService, never()).queryById("D01");
|
||||
verify(appDeviceService, never()).queryById("D02");
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run AppController tests and verify RED**
|
||||
|
||||
Run the Task 2 test command. Expected: interaction assertions fail because current methods query once per device/binding.
|
||||
|
||||
- [ ] **Step 3: Implement batch-backed Controller methods**
|
||||
|
||||
Use one binding lookup in `scheduleDeviceList`:
|
||||
|
||||
```java
|
||||
List<String> deviceNos = appDeviceVos.stream().map(AppDeviceVo::getDeviceNo).toList();
|
||||
Set<String> boundDeviceNos = appSchedulingDeviceService.findBoundDeviceNos(deviceNos);
|
||||
List<AppDeviceVo> availableDevices = appDeviceVos.stream()
|
||||
.filter(device -> !boundDeviceNos.contains(device.getDeviceNo()))
|
||||
.toList();
|
||||
Map<String, Object> result = new HashMap<>();
|
||||
result.put("deviceList", availableDevices);
|
||||
return R.ok(result);
|
||||
```
|
||||
|
||||
Use one batch device lookup in `getScheduleInfo`:
|
||||
|
||||
```java
|
||||
List<String> deviceNos = bindings.stream()
|
||||
.map(AppSchedulingDeviceVo::getDeviceNo)
|
||||
.filter(StringUtils::isNotBlank)
|
||||
.distinct()
|
||||
.toList();
|
||||
List<AppDeviceVo> devices = appDeviceService.queryByDeviceNos(deviceNos);
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run AppController tests and verify GREEN**
|
||||
|
||||
Run the Task 2 command. Expected: all `AppControllerTest` tests pass.
|
||||
|
||||
- [ ] **Step 5: Record the Task 4 diff checkpoint**
|
||||
|
||||
```powershell
|
||||
git diff -- water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerTest.java
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Reuse Schedule Details Across MQTT Notifications
|
||||
|
||||
**Files:**
|
||||
- Modify: `water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerTest.java`
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java:200-287`
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java:763-794`
|
||||
|
||||
**Interfaces:**
|
||||
- Preserves all schedule MQTT command payload fields.
|
||||
- Changes internal helper to consume prebuilt `List<Map<String, Object>> details`.
|
||||
|
||||
- [ ] **Step 1: Write a failing query-count test**
|
||||
|
||||
Add an open-schedule test with two bindings:
|
||||
|
||||
```java
|
||||
@Test
|
||||
void editScheduleStatus_queriesScheduleDetailsOnceForMultipleDevices() {
|
||||
AppController controller = newController();
|
||||
AppScheduleBo bo = new AppScheduleBo();
|
||||
bo.setId(10L);
|
||||
bo.setStatus("1");
|
||||
AppSchedulingDeviceVo first = new AppSchedulingDeviceVo();
|
||||
first.setDeviceNo("D01");
|
||||
AppSchedulingDeviceVo second = new AppSchedulingDeviceVo();
|
||||
second.setDeviceNo("D02");
|
||||
when(appScheduleService.queryById(10L)).thenReturn(ownedSchedule(10L, 99L));
|
||||
when(appScheduleService.updateStatusByBo(bo)).thenReturn(true);
|
||||
when(appSchedulingDeviceService.queryList(any(AppSchedulingDeviceBo.class)))
|
||||
.thenReturn(List.of(first, second));
|
||||
when(appScheduleDetailService.queryByScheduleIdByStatus(10L)).thenReturn(List.of());
|
||||
|
||||
callWithLogin(99L, () -> controller.editScheduleStatus(bo));
|
||||
|
||||
verify(appScheduleDetailService, times(1)).queryByScheduleIdByStatus(10L);
|
||||
verify(deviceCommandService, times(2)).sendScheduleBindCommand(anyString(), any());
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the test and verify RED**
|
||||
|
||||
Run the Task 2 command. Expected: detail service is called twice.
|
||||
|
||||
- [ ] **Step 3: Build details once and reuse them**
|
||||
|
||||
Change payload construction to:
|
||||
|
||||
```java
|
||||
private Map<String, Object> buildScheduleBindPayload(
|
||||
String deviceNo, List<Map<String, Object>> details) {
|
||||
Map<String, Object> payload = new HashMap<>();
|
||||
payload.put("deviceNo", deviceNo);
|
||||
payload.put("details", details);
|
||||
return payload;
|
||||
}
|
||||
|
||||
private List<Map<String, Object>> loadScheduleDetailPayload(Long scheduleId) {
|
||||
return toScheduleDetailPayload(appScheduleDetailService.queryByScheduleIdByStatus(scheduleId));
|
||||
}
|
||||
```
|
||||
|
||||
Call `loadScheduleDetailPayload(scheduleId)` once before each device loop in add/update/open-status notification paths, then pass the result into `buildScheduleBindPayload(deviceNo, details)`.
|
||||
|
||||
- [ ] **Step 4: Run AppController tests and verify GREEN**
|
||||
|
||||
Run the Task 2 command. Expected: all tests pass and the detail service is called once for two devices.
|
||||
|
||||
- [ ] **Step 5: Record the Task 5 diff checkpoint**
|
||||
|
||||
```powershell
|
||||
git diff -- water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerTest.java
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 6: Complete Type-Safety And Local Cleanup
|
||||
|
||||
**Files:**
|
||||
- Modify: `water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java`
|
||||
- Modify: `water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerTest.java`
|
||||
|
||||
**Interfaces:**
|
||||
- Removes the unused `AppScheduleServiceImpl` constructor dependency.
|
||||
- Preserves all endpoint return JSON shapes.
|
||||
|
||||
- [ ] **Step 1: Capture the current static-analysis failure**
|
||||
|
||||
Run:
|
||||
|
||||
```powershell
|
||||
rg -n '^import .*\*;|\b(Map|List|Set)\s+[a-zA-Z][a-zA-Z0-9_]*\s*=|R<Map>' water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java
|
||||
```
|
||||
|
||||
Expected: matches for wildcard imports, raw collections, and raw response maps.
|
||||
|
||||
- [ ] **Step 2: Replace raw types and wildcard imports**
|
||||
|
||||
Use these concrete types:
|
||||
|
||||
```java
|
||||
R<Map<String, Object>>
|
||||
List<AppDeviceVo>
|
||||
List<AppWateringLogVo>
|
||||
List<Map<String, Object>>
|
||||
List<Object>
|
||||
Map<String, Object>
|
||||
Map<String, Long>
|
||||
```
|
||||
|
||||
Replace `org.dromara.app.domain.vo.*`, `org.dromara.app.service.*`, `org.springframework.web.bind.annotation.*`, and `java.util.*` with explicit imports. Remove the unused `AppScheduleServiceImpl appScheduleServiceimpl` field and its test mock/constructor argument.
|
||||
|
||||
Replace Controller time formatting with:
|
||||
|
||||
```java
|
||||
private static final DateTimeFormatter SECOND_TIME_FORMATTER =
|
||||
DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
|
||||
|
||||
String startTime = LocalDateTime.now().format(SECOND_TIME_FORMATTER);
|
||||
|
||||
private String formatSecondTime(Date time) {
|
||||
return time == null ? null : time.toInstant()
|
||||
.atZone(ZoneId.systemDefault())
|
||||
.toLocalDateTime()
|
||||
.format(SECOND_TIME_FORMATTER);
|
||||
}
|
||||
```
|
||||
|
||||
Remove commented-out annotations and dead commented code only in sections touched by this plan.
|
||||
|
||||
- [ ] **Step 3: Re-run the static query and verify GREEN**
|
||||
|
||||
Run the Step 1 command. Expected: no output and exit code 1 from `rg` because there are no matches.
|
||||
|
||||
- [ ] **Step 4: Run all water-app tests**
|
||||
|
||||
```powershell
|
||||
mvn -pl water-modules/water-app -am "-DskipTests=false" "-Dmaven.test.skip=false" "-Dprofiles.active=dev" test
|
||||
```
|
||||
|
||||
Expected: reactor `BUILD SUCCESS`, zero test failures.
|
||||
|
||||
- [ ] **Step 5: Record the Task 6 diff checkpoint**
|
||||
|
||||
```powershell
|
||||
git diff -- water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java water-modules/water-app/src/test/java/org/dromara/app/controller/AppControllerTest.java
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 7: Final Verification Against The Review Standard
|
||||
|
||||
**Files:**
|
||||
- Verify only; no planned production edits.
|
||||
|
||||
**Interfaces:**
|
||||
- Verifies the deliverables from Tasks 1-6 together.
|
||||
|
||||
- [ ] **Step 1: Run common-web tests**
|
||||
|
||||
```powershell
|
||||
mvn -pl water-common/water-common-web -am "-DskipTests=false" "-Dmaven.test.skip=false" test
|
||||
```
|
||||
|
||||
Expected: `BUILD SUCCESS` and `GlobalExceptionHandlerTest` passes.
|
||||
|
||||
- [ ] **Step 2: Run water-app tests**
|
||||
|
||||
```powershell
|
||||
mvn -pl water-modules/water-app -am "-DskipTests=false" "-Dmaven.test.skip=false" "-Dprofiles.active=dev" test
|
||||
```
|
||||
|
||||
Expected: `BUILD SUCCESS` with zero failures and errors.
|
||||
|
||||
- [ ] **Step 3: Run structural and diff checks**
|
||||
|
||||
```powershell
|
||||
rg -n 'return fail\(|private <T> R<T> fail|toI18nErrorMessage|^import .*\*;|R<Map>|\b(Map|List|Set)\s+[a-zA-Z][a-zA-Z0-9_]*\s*=' water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java
|
||||
git diff --check
|
||||
git status --short
|
||||
```
|
||||
|
||||
Expected: no forbidden Controller matches, `git diff --check` exits 0, and status contains only intentional task files plus pre-existing unrelated changes.
|
||||
|
||||
- [ ] **Step 4: Review the final diff by task scope**
|
||||
|
||||
```powershell
|
||||
git diff -- water-common/water-common-web water-modules/water-app/src/main/java/org/dromara/app/controller/AppController.java water-modules/water-app/src/main/java/org/dromara/app/service water-modules/water-app/src/test
|
||||
```
|
||||
|
||||
Expected: no route changes, no MQTT topic changes, no schema changes, and no unrelated file modifications.
|
||||
Reference in New Issue
Block a user