Провести code review сервиса создания заказа

14. Провести code review сервиса создания заказа

Условие задачи:
Дан Spring-сервис создания заказа.

Метод createOrder():

создаёт заказ со статусом NEW;
сохраняет его в БД;
синхронно вызывает внешний платёжный сервис;
в зависимости от результата выставляет PAID или FAILED;
сохраняет новый статус;
отправляет событие в Kafka.

Необходимо провести code review: определить сильные и слабые стороны реализации, возможные проблемы в production и предложить более надёжную архитектуру.

Код:

@Service
public class OrderService {

    @Autowired
    public OrderRepository orderRepository;

    @Autowired
    public PaymentClient paymentClient;

    @Autowired
    public KafkaTemplate<String, String> kafkaTemplate;

    public String createOrder(CreateOrderRequest request) {

        Order order = new Order();
        order.setUserId(request.getUserId());
        order.setAmount(request.getAmount());
        order.setStatus("NEW");

        orderRepository.save(order);

        boolean paid = paymentClient.charge(
                request.getUserId(),
                request.getAmount()
        );

        if (paid) {
            order.setStatus("PAID");
            orderRepository.save(order);

            kafkaTemplate.send(
                    "orders-topic",
                    "Order paid: " + order.getId()
            );
        } else {
            order.setStatus("FAILED");
            orderRepository.save(order);

            kafkaTemplate.send(
                    "orders-topic",
                    "Order failed: " + order.getId()
            );
        }

        return "OK";
    }
}

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

Подсказки

💡 Используй constructor injection вместо публичных полей с @Autowired.
💡 Статусы лучше представить через enum.
💡 Для денежных значений нужен BigDecimal.
💡 Подумай, что произойдёт, если платёж прошёл, а запись PAID в БД упала.
💡 Отдельно подумай о сценарии: БД обновилась, но событие в Kafka не отправилось.
💡 @Transactional вокруг всего метода не делает БД, платёжный сервис и Kafka одной транзакцией.
💡 Повторный HTTP-запрос может привести к созданию второго заказа и повторному списанию денег.

Решение

Плюс исходного кода — бизнес-сценарий легко читается:

создать заказ
→ провести оплату
→ обновить статус
→ отправить событие

Но для production в нём есть несколько серьёзных проблем.

1. Field injection и публичные зависимости

Сейчас:

@Autowired
public OrderRepository orderRepository;

Лучше:

private final OrderRepository orderRepository;
private final PaymentClient paymentClient;

public OrderService(
        OrderRepository orderRepository,
        PaymentClient paymentClient
) {
    this.orderRepository = orderRepository;
    this.paymentClient = paymentClient;
}

Зависимости не должны быть публичным изменяемым состоянием объекта.

2. Нет валидации входных данных

Нужно проверить как минимум:

request != null;
userId != null;
amount != null;
amount > 0.

Для REST DTO это обычно делается через Bean Validation:

public class CreateOrderRequest {

    @NotNull
    private Long userId;

    @NotNull
    @DecimalMin(value = "0.01")
    private BigDecimal amount;
}

и:

@PostMapping
public CreateOrderResponse create(
        @Valid @RequestBody CreateOrderRequest request
) {
    return orderService.createOrder(request);
}

3. Для денег нельзя использовать double

Если amount имеет тип double, его лучше заменить на:

BigDecimal

Денежные расчёты через double могут накапливать ошибки представления чисел с плавающей точкой.

4. Магические строки статусов

Вместо:

order.setStatus("NEW");
order.setStatus("PAID");
order.setStatus("FAILED");

лучше:

public enum OrderStatus {
    NEW,
    PAYMENT_PENDING,
    PAID,
    PAYMENT_DECLINED,
    PAYMENT_ERROR
}

и:

@Enumerated(EnumType.STRING)
private OrderStatus status;

При этом полезно различать:

PAYMENT_DECLINED

и:

PAYMENT_ERROR

false от платёжной системы может означать отказ в оплате, а timeout или HTTP 500 не означают, что платёж точно не состоялся.

5. Главная проблема — несколько независимых систем

Метод одновременно изменяет:

PostgreSQL
Payment Service
Kafka

Обычная:

@Transactional

не способна сделать эти три операции одной атомарной транзакцией.

Например:

1. Order NEW сохранён
2. paymentClient.charge() успешно списал деньги
3. обновление Order → PAID упало

Получаем:

деньги списаны
Order = NEW

Это уже неконсистентное состояние.

6. Просто добавить @Transactional на createOrder() — не решение

Такой вариант:

@Transactional
public void createOrder(...) {
    orderRepository.save(...);

    paymentClient.charge(...);

    ...
}

не делает вызов платёжного сервиса частью JDBC-транзакции.

Если после успешного:

paymentClient.charge(...)

транзакция откатится, платёж обратно автоматически не откатится.

Кроме того, мы будем держать DB connection и транзакцию открытыми, пока ждём сетевой ответ:

BEGIN TRANSACTION
      ↓
INSERT
      ↓
HTTP payment call
      ↓
ждём сеть...
      ↓
UPDATE
      ↓
COMMIT

Для высоконагруженного приложения это нежелательно.

7. Есть проблема DB + Kafka — dual write

Сценарий:

Order → PAID
        ↓
COMMIT в БД успешен
        ↓
Kafka send падает

Получаем:

БД: PAID
Kafka: события нет

И обратная проблема тоже возможна: событие может быть опубликовано, а последующее изменение состояния завершиться ошибкой.

Для связи БД и Kafka обычно используют Transactional Outbox.

В одной DB-транзакции:

UPDATE order
+
INSERT outbox_event
+
COMMIT

После этого отдельный publisher читает outbox и отправляет события в Kafka.

8. Outbox не решает проблему платежа

Важно разделять две проблемы.

Outbox решает:

DB ↔ Kafka

но не делает атомарными:

DB ↔ Payment Service

Для платежей обычно нужны:

идемпотентный payment API;
idempotency key;
явные состояния заказа;
retry;
reconciliation;
иногда compensation/refund;
Saga / state machine для более сложного процесса.

9. Нет идемпотентности

Представим:

Клиент → createOrder()
              ↓
          платёж прошёл
              ↓
       ответ потерялся по сети

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

createOrder()

Без idempotency key можно:

создать второй Order
+
списать деньги второй раз

Поэтому запрос на создание заказа должен иметь уникальный ключ, например:

private String idempotencyKey;

На уровне БД для него желательно иметь UNIQUE.

Этот же идентификатор либо отдельный paymentId следует передавать платёжной системе, если она поддерживает идемпотентные запросы.

10. Возвращать “OK” недостаточно

Сейчас метод всегда завершает успешный сценарий так:

return "OK";

Даже если:

paid == false

клиент получает "OK".

Лучше вернуть DTO:

public record CreateOrderResponse(
        Long orderId,
        OrderStatus status
) {
}

Например:

orderId = 123
status = PAYMENT_PENDING

или итоговый статус, если API действительно синхронно ожидает оплату.

11. OrderService делает слишком много

Сейчас он одновременно:

создаёт Order;
управляет persistence;
вызывает payment service;
определяет состояния;
публикует Kafka event.

Это нарушение SRP / Separation of Concerns.

Лучше разделить:

OrderService
    → orchestration

OrderRepository
    → persistence

PaymentService
    → работа с PaymentClient

OrderTransactionService
    → локальные DB-транзакции

OutboxService
    → запись событий

OutboxPublisher
    → Kafka

Один из вариантов организации сценария:

@Service
public class OrderService {

    private final OrderTransactionService orderTxService;
    private final PaymentService paymentService;

    public OrderService(
            OrderTransactionService orderTxService,
            PaymentService paymentService
    ) {
        this.orderTxService = orderTxService;
        this.paymentService = paymentService;
    }

    public CreateOrderResponse createOrder(
            CreateOrderRequest request
    ) {
        Order order =
                orderTxService.createPendingOrder(request);

        try {
            boolean paid = paymentService.charge(
                    order.getId(),
                    request.getUserId(),
                    request.getAmount()
            );

            if (paid) {
                orderTxService.markPaid(order.getId());

                return new CreateOrderResponse(
                        order.getId(),
                        OrderStatus.PAID
                );
            }

            orderTxService.markDeclined(order.getId());

            return new CreateOrderResponse(
                    order.getId(),
                    OrderStatus.PAYMENT_DECLINED
            );

        } catch (Exception e) {
            orderTxService.markPaymentError(
                    order.getId()
            );

            throw e;
        }
    }
}

Здесь внешний вызов:

paymentService.charge(...)

выполняется между короткими DB-транзакциями, а не внутри одной длинной транзакции.

Транзакционный компонент:

@Service
public class OrderTransactionService {

    private final OrderRepository orderRepository;
    private final OutboxRepository outboxRepository;

    public OrderTransactionService(
            OrderRepository orderRepository,
            OutboxRepository outboxRepository
    ) {
        this.orderRepository = orderRepository;
        this.outboxRepository = outboxRepository;
    }

    @Transactional
    public Order createPendingOrder(
            CreateOrderRequest request
    ) {
        Order order = new Order();

        order.setUserId(request.getUserId());
        order.setAmount(request.getAmount());
        order.setStatus(
                OrderStatus.PAYMENT_PENDING
        );

        return orderRepository.save(order);
    }

    @Transactional
    public void markPaid(Long orderId) {
        Order order = getOrder(orderId);

        order.setStatus(OrderStatus.PAID);

        outboxRepository.save(
                OutboxEvent.orderPaid(orderId)
        );
    }

    @Transactional
    public void markDeclined(Long orderId) {
        Order order = getOrder(orderId);

        order.setStatus(
                OrderStatus.PAYMENT_DECLINED
        );

        outboxRepository.save(
                OutboxEvent.orderPaymentDeclined(orderId)
        );
    }

    @Transactional
    public void markPaymentError(Long orderId) {
        Order order = getOrder(orderId);

        order.setStatus(
                OrderStatus.PAYMENT_ERROR
        );

        outboxRepository.save(
                OutboxEvent.orderPaymentError(orderId)
        );
    }

    private Order getOrder(Long orderId) {
        return orderRepository.findById(orderId)
                .orElseThrow(() ->
                        new EntityNotFoundException(
                                "Order not found: " + orderId
                        )
                );
    }
}

Внутри markPaid() не обязательно явно повторно вызывать:

orderRepository.save(order);

если Order был загружен внутри текущего persistence context.

JPA dirty checking сохранит изменение при flush/commit.

Для Kafka схема становится такой:

markPaid()
    │
    ├── UPDATE orders
    │
    ├── INSERT outbox
    │
    └── COMMIT
              ↓
       OutboxPublisher
              ↓
            Kafka

Если Kafka временно недоступна, запись остаётся в outbox и может быть отправлена повторно.

Consumer события тоже желательно делать идемпотентным, потому что delivery обычно проектируют как at-least-once.

Отдельно нужно продумать неопределённый результат платежа.

Например:

paymentClient.charge()
        ↓
timeout

Мы не знаем наверняка:

платёж не прошёл

или:

платёж прошёл,
но ответ не дошёл

Поэтому автоматически считать такой заказ:

FAILED

опасно.

Лучше состояние вроде:

PAYMENT_ERROR
PAYMENT_UNKNOWN

и последующая сверка статуса платежа по paymentId.

Основные проблемы исходного решения:

public field injection;
нет валидации;
возможное использование double для денег;
магические строки статусов;
слабый контракт "OK";
нет обработки исключений paymentClient;
false и техническая ошибка платежа не различаются;
нет идемпотентности;
возможен двойной платёж при retry;
DB и payment service невозможно объединить обычным @Transactional;
DB-транзакцию не стоит держать во время сетевого вызова;
возможна потеря события между DB и Kafka;
нужен Transactional Outbox для DB + Kafka;
после успешного платежа и ошибки БД нужна стратегия восстановления;
класс имеет слишком много ответственностей.

Главная идея архитектуры:

Request
   ↓
создать PAYMENT_PENDING
   ↓
COMMIT
   ↓
идемпотентный Payment API
   ↓
PAID / DECLINED / ERROR
   ↓
короткая DB-транзакция:
status + outbox
   ↓
COMMIT
   ↓
Kafka publisher

То есть в таком сервисе важнее всего правильно организовать согласованность между независимыми системами, а не просто добавить @Transactional вокруг createOrder().