Провести рефакторинг контроллера обработки задач

35. Провести рефакторинг контроллера обработки задач

Условие задачи:

Дан REST-контроллер для работы с задачами.

Контроллер:

обновляет и удаляет задачи;
получает сотрудников определённого департамента;
ищет их задачи;
запускает бизнес-логику обработки;
отправляет уведомления;
отмечает задачи как обработанные.

Необходимо провести рефакторинг кода, выявить архитектурные и технические проблемы и предложить более корректную реализацию.

Код:

@RestController
@RequestMapping("/task")
public class Maincontroller {

    @Autowired
    private NotificationFeignclient notificationfeignclient;

    @Autowired
    private Businesslogicservice businesslogicservice;

    @Autowired
    private TaskRepository taskRepository;

    @Autowired
    private UserRepository userRepository;

    @PostMapping("/process/{department}")
    public void processTasks(@PathVariable String department) {
        processDepartmentTasks(department);
    }

    @PostMapping
    public void updateTask(@RequestBody Task task) {
        taskRepository.updateTask(task);
    }

    @PostMapping("/{id}")
    public void deleteTask(@PathVariable Long id) {
        taskRepository.deleteById(id);
    }

    @Transactional
    public void processDepartmentTasks(String department) {
        var users = userRepository.findAll();

        for (User user : users) {
            if (user.getDepartment().equals(department)) {
                var tasks = taskRepository.findAll();

                for (Task task : tasks) {
                    if (Objects.equals(task.getUserId(), user.getId())) {
                        businessLogicService.process(task);
                        notificationFeignClient.sendProcessNotification(
                                task.getDescription()
                        );
                        taskRepository.setProcessed(task.getId());
                    }
                }
            }
        }
    }
}

Спойлеры к решению

Подсказки

💡 Контроллер должен отвечать преимущественно за HTTP-слой, а не содержать бизнес-логику.

💡 Обратите внимание на использование field injection через @Autowired.

💡 Необязательно загружать всех пользователей и все задачи, если база данных может сразу вернуть только необходимые записи.

💡 Вызов метода с @Transactional из другого метода того же объекта имеет важную особенность в Spring.

💡 HTTP-методы должны соответствовать выполняемым операциям: создание, изменение и удаление — это разные действия.

💡 Внешний HTTP-вызов через Feign внутри транзакции может привести к проблемам с производительностью и согласованностью.

Решение

В исходном коде присутствует сразу несколько проблем.

1. Бизнес-логика находится в контроллере

Контроллер одновременно занимается:

HTTP-запросами;
поиском пользователей;
поиском задач;
обработкой задач;
работой с транзакцией;
отправкой уведомлений.

Это нарушает принцип разделения ответственности.

Контроллер лучше оставить тонким, а обработку перенести в отдельный сервис.

2. Используется field injection

Исходный вариант:

@Autowired
private TaskRepository taskRepository;

Лучше использовать constructor injection.

Например:

private final TaskService taskService;

public TaskController(TaskService taskService) {
    this.taskService = taskService;
}

Это делает зависимости класса явными и упрощает тестирование.

3. Загружаются все пользователи

Используется:

userRepository.findAll();

после чего фильтрация выполняется в Java:

if (user.getDepartment().equals(department))

База данных может выполнить эту фильтрацию значительно эффективнее.

Например:

List<User> findAllByDepartment(String department);

4. Для каждого пользователя загружаются все задачи

Особенно проблемный участок:

for (User user : users) {
    var tasks = taskRepository.findAll();

    for (Task task : tasks) {
        ...
    }
}

Если имеется U пользователей и T задач, выполняется большое количество лишней работы.

Кроме того, findAll() вызывается заново для каждого пользователя.

Вместо этого необходимые данные следует получить непосредственно запросом к БД.

Например:

List<Task> findAllByUserDepartment(String department);

5. Некорректно используется @Transactional

В исходном коде:

public void processTasks(String department) {
    processDepartmentTasks(department);
}

@Transactional
public void processDepartmentTasks(String department) {
    ...
}

processDepartmentTasks() вызывается из другого метода того же объекта.

При стандартном proxy-based механизме Spring такой self-invocation обходит Spring-прокси, поэтому @Transactional может не сработать.

Транзакционную бизнес-операцию лучше вынести в отдельный @Service.

6. Для удаления используется POST

Исходный вариант:

@PostMapping("/{id}")
public void deleteTask(@PathVariable Long id)

Для удаления ресурса корректнее использовать:

@DeleteMapping("/{id}")

7. Для обновления задачи используется POST

Если существующая задача полностью обновляется, логичнее использовать PUT:

@PutMapping("/{id}")

Для частичного обновления можно использовать PATCH.

8. Контроллер напрямую работает с репозиториями

Например:

taskRepository.updateTask(task);

Контроллеру лучше обращаться к сервисному слою:

taskService.updateTask(...);

Тогда бизнес-правила изменения данных будут находиться в одном месте.

Вариант после рефакторинга

Контроллер можно сделать следующим:

@RestController
@RequestMapping("/tasks")
public class TaskController {

    private final TaskService taskService;

    public TaskController(TaskService taskService) {
        this.taskService = taskService;
    }

    @PostMapping("/process/{department}")
    public void processTasks(@PathVariable String department) {
        taskService.processDepartmentTasks(department);
    }

    @PutMapping("/{id}")
    public void updateTask(
            @PathVariable Long id,
            @RequestBody Task task
    ) {
        taskService.updateTask(id, task);
    }

    @DeleteMapping("/{id}")
    public void deleteTask(@PathVariable Long id) {
        taskService.deleteTask(id);
    }
}

Теперь контроллер отвечает только за HTTP-слой.

Основную бизнес-логику можно перенести в сервис:

@Service
public class TaskService {

    private final TaskRepository taskRepository;
    private final BusinessLogicService businessLogicService;

    public TaskService(
            TaskRepository taskRepository,
            BusinessLogicService businessLogicService
    ) {
        this.taskRepository = taskRepository;
        this.businessLogicService = businessLogicService;
    }

    @Transactional
    public void processDepartmentTasks(String department) {
        List<Task> tasks =
                taskRepository.findAllByUserDepartment(department);

        for (Task task : tasks) {
            businessLogicService.process(task);
            task.setProcessed(true);
        }
    }

    @Transactional
    public void updateTask(Long id, Task task) {
        Task existingTask = taskRepository.findById(id)
                .orElseThrow(() ->
                        new IllegalArgumentException(
                                "Task not found: " + id
                        )
                );

        existingTask.setTitle(task.getTitle());
        existingTask.setDescription(task.getDescription());
    }

    @Transactional
    public void deleteTask(Long id) {
        taskRepository.deleteById(id);
    }
}

Репозиторий должен получать сразу только необходимые задачи.

Например, если у Task есть связь с User:

public interface TaskRepository
        extends JpaRepository<Task, Long> {

    List<Task> findAllByUserDepartment(String department);
}

В JPQL запрос может выглядеть так:

@Query("""
       select t
       from Task t
       join t.user u
       where u.department = :department
       """)
List<Task> findAllByDepartment(
        @Param("department") String department
);

Таким образом, вместо:

получить всех пользователей
        ↓
найти нужный департамент в Java
        ↓
для каждого пользователя получить все задачи
        ↓
найти подходящие задачи в Java

получаем:

БД
 ↓
сразу получить задачи нужного департамента
 ↓
обработать их

Что делать с отправкой уведомления

В исходной реализации внешний Feign-вызов выполняется внутри транзакции:

businessLogicService.process(task);

notificationFeignClient.sendProcessNotification(
        task.getDescription()
);

taskRepository.setProcessed(task.getId());

Это может привести к проблеме согласованности.

Например:

1. Задача обработана
2. Уведомление успешно отправлено
3. При сохранении данных возникла ошибка
4. Транзакция откатилась

В результате уведомление уже отправлено, хотя изменения в базе данных не были зафиксированы.

Кроме того, внешний сервис может отвечать долго, из-за чего транзакция с БД будет оставаться открытой.

Один из вариантов — публиковать событие внутри транзакции, а уведомление отправлять только после успешного commit.

@Service
public class TaskService {

    private final TaskRepository taskRepository;
    private final BusinessLogicService businessLogicService;
    private final ApplicationEventPublisher eventPublisher;

    public TaskService(
            TaskRepository taskRepository,
            BusinessLogicService businessLogicService,
            ApplicationEventPublisher eventPublisher
    ) {
        this.taskRepository = taskRepository;
        this.businessLogicService = businessLogicService;
        this.eventPublisher = eventPublisher;
    }

    @Transactional
    public void processDepartmentTasks(String department) {
        List<Task> tasks =
                taskRepository.findAllByUserDepartment(department);

        for (Task task : tasks) {
            businessLogicService.process(task);

            task.setProcessed(true);

            eventPublisher.publishEvent(
                    new TaskProcessedEvent(
                            task.getId(),
                            task.getDescription()
                    )
            );
        }
    }
}

Обработчик уведомления:

@Component
public class TaskNotificationListener {

    private final NotificationFeignClient notificationFeignClient;

    public TaskNotificationListener(
            NotificationFeignClient notificationFeignClient
    ) {
        this.notificationFeignClient = notificationFeignClient;
    }

    @TransactionalEventListener(
            phase = TransactionPhase.AFTER_COMMIT
    )
    public void handle(TaskProcessedEvent event) {
        notificationFeignClient.sendProcessNotification(
                event.description()
        );
    }
}

Событие:

public record TaskProcessedEvent(
        Long taskId,
        String description
) {
}

Теперь уведомление отправляется только после успешного завершения транзакции.

Если требуется гарантированная доставка уведомлений между сервисами, более надёжным вариантом будет использование паттерна Transactional Outbox.

Итог

После рефакторинга:

контроллер отвечает только за HTTP;
бизнес-логика находится в сервисе;
используется constructor injection;
@Transactional корректно применяется на сервисном методе;
вместо findAll() используются специализированные запросы к БД;
исчезают вложенные циклы по всем пользователям и задачам;
используются подходящие HTTP-методы;
работа с репозиторием скрыта за сервисным слоем;
внешний вызов можно выполнять после успешного commit транзакции;
код становится проще тестировать и поддерживать.