Сделать ревью кода

17. Провести code review сервиса изменения баланса

Дан сервис UserBalanceService. Нужно сделать code review: найти проблемы, риски и предложить улучшения.

Код:

public class UserBalanceService {

    @Autowired
    UserBalanceRepository repository;

    @Autowired
    BalanceOperationsRepository balanceOperationsRepository;

    @Autowired
    CurrencyConvertRestClient currencyConvertRestClient;

    @Autowired
    NotificationRestClient notificationRestClient;

    @Autowired
    OperationLogRepository operationLogRepository;

    @Value(""""app.settings.currency-default"""")
    String defaultCurrency;

    @Transactional
    public void changeBalance(UUID userId, Double balance, String currency) {
        if (userId == null || balance == null) {
            throw new IllegalArgumentException(
                    """"Один или несколько аргументов равны null""""
            );
        }

        if (currency == null) {
            currency = defaultCurrency;
        }

        UserBalance userBalance = repository.getUserBalanceById(userId);

        Double convertedAmount = currencyConvertRestClient.convert(
                balance,
                currency,
                userBalance.getCurrency()
        );

        userBalance.setBalance(
                userBalance.getAmount() + convertedAmount
        );

        try {
            saveToLog(userId, balance, currency, null, true);
            repository.save(userBalance);
        } catch (DataAccessException e) {
            saveToLog(
                    userId,
                    balance,
                    currency,
                    e.getMessage(),
                    false
            );

            notificationRestClient.notifyErrorBalanceChange(
                    userId,
                    balance,
                    currency
            );

            throw e;
        }

        notificationRestClient.notifyBalanceChangeOk(
                userId,
                balance,
                currency
        );
    }

    @Transactional
    void saveToLog(
            UUID userId,
            Double balance,
            String currency,
            String errorMessage,
            Boolean ok
    ) {
        if (ok) {
            operationLogRepository.addBalanceChangeOk(
                    userId,
                    balance,
                    currency
            );
        } else {
            operationLogRepository.addBalanceChangeError(
                    userId,
                    balance,
                    errorMessage,
                    currency
            );
        }
    }
}

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

Подсказки

💡 Для денег лучше использовать BigDecimal.
💡 Проверь @Value, границы транзакции и self-invocation.
💡 Два параллельных запроса могут потерять одно из изменений баланса.
💡 Внешние REST-вызовы лучше не держать внутри DB-транзакции.

Решение

Проблемы:

нет @Service или другой явной регистрации класса как Spring-бина;
field injection вместо constructor injection;
Double используется для денег;
в @Value отсутствует ${...};
не обработано отсутствие UserBalance;
setBalance() и getAmount() выглядят несогласованно;
REST-вызов выполняется внутри открытой транзакции;
возможен lost update при параллельных изменениях баланса;
@Transactional на saveToLog() не создаёт отдельную транзакцию из-за self-invocation;
error log откатится вместе с основной транзакцией;
success log вызывается ещё до успешного сохранения;
ошибка БД может возникнуть не в save(), а только на flush/commit, поэтому текущий catch поймает не все ошибки сохранения;
success notification вызывается до commit;
ошибка notification может скрыть исходное исключение;
Boolean ok здесь не нужен;
BalanceOperationsRepository не используется.

Как исправить

Для зависимостей использовать constructor injection, для денег — BigDecimal:

@Value("${app.settings.currency-default}")
private String defaultCurrency;

Внешнюю конвертацию валюты лучше выполнять до короткой write-транзакции.

Конкурентное изменение баланса лучше делать атомарно на уровне БД:

@Modifying
@Query("""
        update UserBalance b
           set b.amount = b.amount + :amount
         where b.id = :userId
        """)
int addAmount(
        @Param("userId") UUID userId,
        @Param("amount") BigDecimal amount
);

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

Основной сервис можно оставить координатором:

@Service
@RequiredArgsConstructor
public class UserBalanceService {

    private final UserBalanceRepository repository;
    private final CurrencyConvertRestClient currencyClient;
    private final BalanceTransactionService balanceService;
    private final OperationLogService operationLogService;
    private final NotificationRestClient notificationClient;

    @Value("${app.settings.currency-default}")
    private String defaultCurrency;

    public void changeBalance(
            UUID userId,
            BigDecimal amount,
            String currency
    ) {
        Objects.requireNonNull(userId, "userId");
        Objects.requireNonNull(amount, "amount");

        String sourceCurrency =
                currency == null || currency.isBlank()
                        ? defaultCurrency
                        : currency;

        String targetCurrency = repository.findCurrencyById(userId)
                .orElseThrow(() ->
                        new EntityNotFoundException(
                                "Balance not found: " + userId
                        )
                );

        BigDecimal converted = currencyClient.convert(
                amount,
                sourceCurrency,
                targetCurrency
        );

        try {
            balanceService.apply(
                    userId,
                    converted,
                    amount,
                    sourceCurrency
            );
        } catch (DataAccessException e) {
            operationLogService.saveError(
                    userId,
                    amount,
                    sourceCurrency,
                    e.getMessage()
            );

            throw e;
        }

        notificationClient.notifyBalanceChangeOk(
                userId,
                amount,
                sourceCurrency
        );
    }
}

Изменение баланса и success log выполняются одной короткой транзакцией:

@Service
@RequiredArgsConstructor
public class BalanceTransactionService {

    private final UserBalanceRepository repository;
    private final OperationLogRepository logRepository;

    @Transactional
    public void apply(
            UUID userId,
            BigDecimal convertedAmount,
            BigDecimal originalAmount,
            String currency
    ) {
        if (repository.addAmount(userId, convertedAmount) != 1) {
            throw new EntityNotFoundException(
                    "Balance not found: " + userId
            );
        }

        logRepository.addBalanceChangeOk(
                userId,
                originalAmount,
                currency
        );
    }
}

Если error log должен сохраниться независимо от rollback, его нужно вынести в отдельный Spring-бин:

@Transactional(propagation = Propagation.REQUIRES_NEW)
public void saveError(...) {
    repository.addBalanceChangeError(...);
}

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