Code review PaymentService: транзакции, DI и логические баги

28. Провести ревью PaymentService и исправить перевод между счетами

Условие задачи:
Дан сервис перевода денег между двумя счетами. Метод должен получить счета из БД, изменить их балансы и отправить уведомления владельцам.

Нужно провести code review: найти ошибки компиляции и логики, проверить Dependency Injection и транзакционность операции, а также предложить исправленный вариант с сохранением общей структуры сервиса.

Код:

@Component
public class PaymentService{

    @Autowired
    private ComissionServiceImpl comissionService;

    private NotificationService notificationService = new NotificationServiceImpl();

    @Autowired
    private AccountRepository accountRepository;

    public void makePayment(AccountDto acc1, AccountDto acc2, Integer moneyAmount){
        System.out.println(""Start payment"");
        var account1 = accountRepository.findById(acc1.getAccountId());
        var account2 = accountRepository.findById(acc1.getAccountId());
        transfer(account1, account2, moneyAmount);
        notificationService.sendNotification(account1.getAccountId());
        notificationService.sendNotification(account2.getAccountId());
        System.out.println(""End payment"");
    }

    @Transactional
    public void transfer(Account acc1, Account acc2, Integer moneyAmount){
        account1.setMoneyAmount(account1.getMoneyAmount() - moneyAmount);
        account2.setMoneyAmount(account2.getMoneyAmount() + moneyAmount);
    }

}

public class AccountDto {
    private UUID accountId;
    private Integer moneyAmount;
}

@Entity
@Table
public class Account{

    @Id
    private UUID accountId;
    private Integer moneyAmount;
}

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

Подсказки

💡 Проверь, какой тип возвращает findById().
💡 Обрати внимание, какой accountId используется при загрузке второго счёта.
💡 @Transactional вызывается через Spring proxy — внутренний вызов метода имеет значение.
💡 Для всех зависимостей лучше использовать один способ DI.
💡 Подумай, что произойдёт при двух одновременных переводах с одного счёта.

Решение

Проблемы:

смешаны field injection и ручное создание зависимости через new;
сервис зависит от конкретного ComissionServiceImpl, а не от интерфейса;
comissionService вообще не используется;
при загрузке второго счёта ошибочно используется acc1.getAccountId();
Spring Data findById() возвращает Optional<Account>, а код работает с результатом как с Account;
в transfer() используются переменные account1 и account2, которых в этом методе нет;
@Transactional на transfer() не сработает при вызове из makePayment() того же объекта;
нет проверки входных DTO, идентификаторов и суммы;
не проверяется достаточность средств;
не обработан перевод на тот же счёт;
конкурентные переводы могут привести к lost update;
уведомления нельзя считать частью транзакции БД: они могут уйти до commit;
System.out.println() лучше заменить логгером;
AccountDto.moneyAmount для команды перевода не нужен, если сумма передаётся отдельным параметром.

Как лучше исправить

Транзакционную операцию с БД лучше вынести в отдельный сервис. Это одновременно решает проблему self-invocation и позволяет не держать отправку уведомлений внутри транзакции.

@Service
@RequiredArgsConstructor
public class PaymentService {

    private final PaymentTransactionService transactionService;
    private final NotificationService notificationService;

    public void makePayment(
            AccountDto from,
            AccountDto to,
            Integer amount
    ) {
        if (from == null || to == null
                || from.getAccountId() == null
                || to.getAccountId() == null) {
            throw new IllegalArgumentException(
                    "Accounts must be specified"
            );
        }

        if (amount == null || amount <= 0) {
            throw new IllegalArgumentException(
                    "Amount must be positive"
            );
        }

        if (from.getAccountId().equals(to.getAccountId())) {
            throw new IllegalArgumentException(
                    "Accounts must be different"
            );
        }

        transactionService.transfer(
                from.getAccountId(),
                to.getAccountId(),
                amount
        );

        notificationService.sendNotification(
                from.getAccountId()
        );
        notificationService.sendNotification(
                to.getAccountId()
        );
    }
}

Транзакционная часть:

@Service
@RequiredArgsConstructor
public class PaymentTransactionService {

    private final AccountRepository accountRepository;

    @Transactional
    public void transfer(
            UUID fromId,
            UUID toId,
            Integer amount
    ) {
        Account from = accountRepository.findById(fromId)
                .orElseThrow(() ->
                        new EntityNotFoundException(
                                "Account not found: " + fromId
                        )
                );

        Account to = accountRepository.findById(toId)
                .orElseThrow(() ->
                        new EntityNotFoundException(
                                "Account not found: " + toId
                        )
                );

        if (from.getMoneyAmount() < amount) {
            throw new IllegalStateException(
                    "Insufficient funds"
            );
        }

        from.setMoneyAmount(
                from.getMoneyAmount() - amount
        );

        to.setMoneyAmount(
                to.getMoneyAmount() + amount
        );
    }
}

Так как Account загружены внутри транзакции и остаются managed entity, отдельный save() обычно не требуется — изменения будут сохранены через dirty checking.

При реальных конкурентных переводах одной @Transactional недостаточно. Два запроса могут одновременно прочитать один баланс и потерять обновление. Для банковского сценария нужна стратегия конкурентного доступа: например, pessimistic locking с одинаковым порядком блокировки счетов либо optimistic locking через @Version и retry.

Уведомление также лучше отправлять только после успешного commit — через @TransactionalEventListener(AFTER_COMMIT) или outbox, если требуется надёжная доставка.

Для денежных значений тип должен соответствовать модели хранения: если Integer означает минимальные денежные единицы, например копейки, это допустимо; если хранятся дробные суммы, нужен BigDecimal.