[strayByte2022]

No description provided.

[deeppro-code-reviewer-beta]

🤖 DeepPro Organization Code Review

Review

The PR moves TaskStatus into a proper enum and changes the status endpoint to a 204 No Content response, which is a good direction. However, there are several issues that should be fixed before merging.

1. @NotBlank is invalid on an enum field

@NotBlank is only valid for CharSequence fields (String, StringBuilder, etc.). Applying it to an enum will cause a UnexpectedTypeException if Bean Validation is triggered for that object.

Fix: Use @NotNull instead.

public record ChangeTaskStatusCommand(
        @NotNull UUID taskId,
        @NotNull TaskStatus status
) {}

2. @Valid is missing on the request body

ChangeStatusRequest has validation annotations (@NotBlank, @Size), but they will not be triggered unless the controller parameter is annotated with @Valid (or @Validated).

Fix:

@PatchMapping("/{id}/status")
@PreAuthorize("hasAuthority('PERMISSION_TASK_STATUS_CHANGE')")
public ResponseEntity<Void> updateStatus(@PathVariable UUID id,
                                         @Valid @RequestBody ChangeStatusRequest request) {
    changeTaskStatusService.handle(new ChangeTaskStatusCommand(id, TaskStatus.valueOf(request.status())));
    return ResponseEntity.noContent().build();
}

3. TaskStatus.valueOf(...) in the controller can cause a 500

If the client sends an invalid status string, TaskStatus.valueOf throws IllegalArgumentException, which by default returns 500 Internal Server Error. That should be a 400 Bad Request.

A cleaner approach is to use TaskStatus directly in the request DTO, so Jackson handles invalid values with a 400.

public record ChangeStatusRequest(@NotNull TaskStatus status) {}

Then the controller simplifies to:

@PatchMapping("/{id}/status")
@PreAuthorize("hasAuthority('PERMISSION_TASK_STATUS_CHANGE')")
public ResponseEntity<Void> updateStatus(@PathVariable UUID id,
                                         @Valid @RequestBody ChangeStatusRequest request) {
    changeTaskStatusService.handle(new ChangeTaskStatusCommand(id, request.status()));
    return ResponseEntity.noContent().build();
}

If you need to keep String status in the DTO for compatibility, add a pattern constraint:

public record ChangeStatusRequest(
        @NotBlank @Pattern(regexp = "TODO|IN_PROGRESS|DONE|BLOCKED") @Size(max = 500) String status
) {}

But using the enum directly is preferable.

4. Business rule violation throws IllegalArgumentException

validateChangeStatus throws IllegalArgumentException when an invalid transition occurs. This will also produce a 500 Internal Server Error. A status transition violation is a client error, so it should map to 400 or 409.

Suggested change: introduce a domain exception.

public class InvalidTaskStatusTransitionException extends RuntimeException {
    public InvalidTaskStatusTransitionException(TaskStatus oldStatus, TaskStatus newStatus) {
        super(String.format("Cannot change task status from %s to %s", oldStatus, newStatus));
    }
}

Then use it in the service:

throw new InvalidTaskStatusTransitionException(oldStatus, newStatus);

And handle it globally with @ExceptionHandler, returning 400:

@ExceptionHandler(InvalidTaskStatusTransitionException.class)
public ResponseEntity<ErrorResponse> handleInvalidStatusTransition(
        InvalidTaskStatusTransitionException ex) {
    return ResponseEntity.badRequest().body(new ErrorResponse(ex.getMessage()));
}

5. validateChangeStatus relies on enum ordinals

The check newStatus.ordinal() < oldStatus.ordinal() is fragile. Reordering enum constants will silently change business rules. It also makes the allowed transitions hard to reason about, especially the special BLOCKED case.

A clearer approach is to define an explicit transition map:

private static final Map<TaskStatus, Set<TaskStatus>> ALLOWED_TRANSITIONS =
        Map.of(
                TaskStatus.TODO, Set.of(TaskStatus.IN_PROGRESS, TaskStatus.BLOCKED),
                TaskStatus.IN_PROGRESS, Set.of(TaskStatus.DONE, TaskStatus.BLOCKED),
                TaskStatus.DONE, Set.of(TaskStatus.BLOCKED),
                TaskStatus.BLOCKED, Set.of(TaskStatus.IN_PROGRESS)
        );

Then:

private void validateChangeStatus(TaskStatus oldStatus, TaskStatus newStatus) {
    if (oldStatus == newStatus) {
        return;
    }

    Set<TaskStatus> allowed = ALLOWED_TRANSITIONS.getOrDefault(oldStatus, Set.of());
    if (!allowed.contains(newStatus)) {
        throw new InvalidTaskStatusTransitionException(oldStatus, newStatus);
    }
}

This makes the workflow explicit and independent of enum declaration order.

6. Same-status transition still saves, updates updatedAt, and publishes an event

Currently if oldStatus equals newStatus, validateChangeStatus returns early, but handle still continues to updateTask and publishes a TaskStatusChangedEvent. This will overwrite updatedAt and emit a no-op event.

If same-status changes should be idempotent, return early:

@Override
@Transactional
public void handle(ChangeTaskStatusCommand command) {
    Task task = taskRepository.findById(command.taskId())
            .orElseThrow(() -> new ResourceNotFoundException("Task", command.taskId()));

    TaskStatus oldStatus = TaskStatus.valueOf(task.getStatus());
    TaskStatus newStatus = command.status();

    if (oldStatus == newStatus) {
        log.info("task_status_same id={} status={}", command.taskId(), oldStatus);
        return;
    }

    validateChangeStatus(oldStatus, newStatus);
    updateTask(task, newStatus);
    eventPublisher.publishEvent(new TaskStatusChangedEvent(
            command.taskId(),
            oldStatus.name(),
            newStatus.name(),
            Instant.now()));
    log.info("task_status_changed id={} {} -> {}", command.taskId(), oldStatus, newStatus);
}

7. Log message is misleading in CreateTaskServiceImpl

The log declares status={} but passes command.title():

log.info("task_created id={} status={}", task.getId(), command.title());

This is incorrect. Fix either by passing the actual status:

log.info("task_created id={} status={}", task.getId(), task.getStatus());

or change the placeholder back to title={}:

log.info("task_created id={} title={}", task.getId(), command.title());

Given that the surrounding code originally logged the title, the second option is probably what was intended.

8. Missing tests for invalid transitions

The service now enforces forward-only transitions, but the tests only cover success and missing task. Add tests for:

• backward transition should throw InvalidTaskStatusTransitionException
• transition to BLOCKED is allowed
• invalid old status string should be handled (or impossible if repository always returns valid values)

Example:

@Test
void throwsWhenTransitionIsBackwards() {
    ChangeTaskStatusCommand cmd = new ChangeTaskStatusCommand(TASK_ID, TaskStatus.TODO);
    Task task = Task.builder().id(TASK_ID).title("Task").status("IN_PROGRESS").build();
    when(taskRepository.findById(TASK_ID)).thenReturn(Optional.of(task));

    assertThatThrownBy(() -> service.handle(cmd))
            .isInstanceOf(InvalidTaskStatusTransitionException.class);
}

Also update the controller test to verify invalid status strings return 400, if you keep the String DTO approach.

---

Summary

The main blockers are:

• @NotBlank on enum — use @NotNull
• Missing @Valid on request body
• IllegalArgumentException for invalid transitions maps to 500
• Log message mistake in CreateTaskServiceImpl

The larger design issue is the ordinal-based transition validation. Replacing it with an explicit map will make the state machine clear and safe.