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

19. Сделать ревью AccountResource и BalanceService

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

Код:

@Data
@Entity
public class Account {

    @Id
    private Long id;

    private String accountNumber;
    private String bic;
    private Double amount;
}

public interface AccountRepository
        extends JpaRepository<Account, Long> {

    List<Account> findByAccountNumber(String accountNumber);
}

@RestController
@RequiredArgsConstructor
public class AccountResource {

    private final AccountRepository accountRepository;
    private final BalanceService balanceService;

    @GetMapping(
            value = """"/accounts/add"""",
            produces = MediaType.APPLICATION_JSON_VALUE
    )
    Account add(Account account) {
        return accountRepository.save(account);
    }

    @GetMapping(
            value = """"/accounts/{accountNumber}"""",
            produces = MediaType.APPLICATION_JSON_VALUE
    )
    List<Account> findByAccountNumber(
            @PathVariable(""""accountNumber"""") String accountNumber
    ) {
        return accountRepository.findByAccountNumber(accountNumber);
    }

    @PutMapping(
            value = """"/accounts/{id}/change"""",
            produces = MediaType.APPLICATION_JSON_VALUE
    )
    Account changeBalance(
            @PathVariable(""""id"""") Long id,
            @RequestParam(""""diff"""") Double diff
    ) {
        return balanceService.change(id, diff);
    }
}

@Service
public class BalanceService {

    @Autowired
    AccountRepository accountRepository;

    public Account change(Long id, Double diff) {
        Account account = accountRepository.getOne(id);
        account.setAmount(account.getAmount() + diff);
        return accountRepository.save(account);
    }
}

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

Подсказки

💡 Создание ресурса не должно выполняться через GET.
💡 Для денежных значений нужен BigDecimal.
💡 Подумай, корректна ли схема прочитать баланс → изменить → сохранить при двух одновременных запросах.
💡 getOne() не подходит для явной обработки отсутствующей записи.

Решение

Проблемы:

создание счёта выполняется через GET, хотя это изменяющая операция;
Account напрямую используется как REST request/response;
контроллер напрямую вызывает AccountRepository;
отсутствует валидация входных данных;
Double используется для денег;
изменение баланса через PUT с diff неидемпотентно;
в BalanceService используется field injection;
нет явной транзакционной границы;
getOne() устарел и возвращает proxy вместо явной проверки существования записи;
возможен lost update при конкурентном изменении одного баланса;
не обработана ситуация отсутствующего счёта;
не определено поведение при отрицательном балансе;
на уровне БД не заданы ограничения для обязательных полей;
@Data нежелателен для JPA entity из-за автоматически генерируемых equals(), hashCode() и toString();
если accountNumber должен быть уникальным, это необходимо гарантировать в БД, а не только кодом.

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

Для создания счёта использовать POST и отдельный DTO:

@PostMapping("/accounts")
@ResponseStatus(HttpStatus.CREATED)
public AccountResponse create(
        @Valid @RequestBody CreateAccountRequest request
) {
    return accountService.create(request);
}

Для денег использовать BigDecimal, а изменение на величину diff логичнее оформить через PATCH или отдельную команду.

В сервисе — constructor injection и транзакция:

@Service
@RequiredArgsConstructor
public class BalanceService {

    private final AccountRepository accountRepository;

    @Transactional
    public Account change(Long id, BigDecimal diff) {
        Objects.requireNonNull(id, "id");
        Objects.requireNonNull(diff, "diff");

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

        account.setAmount(
                account.getAmount().add(diff)
        );

        return account;
    }
}

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

Для простой операции изменения суммы лучше выполнить атомарный UPDATE:

@Modifying
@Query("""
        update Account a
           set a.amount = a.amount + :diff
         where a.id = :id
        """)
int changeAmount(
        @Param("id") Long id,
        @Param("diff") BigDecimal diff
);

Если отрицательный баланс запрещён, проверку также стоит включить в этот запрос:

AND amount + :diff >= 0

Тогда проверка и изменение выполняются атомарно на стороне БД.

Для entity стоит использовать BigDecimal, добавить необходимые NOT NULL/UNIQUE ограничения согласно бизнес-требованиям и вместо @Data предпочесть @Getter / @Setter.