Провести code review PaymentService

41. Провести code review PaymentService

Условие задачи:
Дан Spring-сервис обработки платежей.

Необходимо провести code review, найти проблемы в реализации и предложить исправления.

Код:

@Service
public class PaymentService implements Status {

    @Autowired
    private PaymentRepository repo;

    @Autowired
    private KafkaTemplate<String, String> kafka;

    @Transactional
    public void processPayment(PaymentRequest request) {
        Payment payment = new Payment();
        payment.setAmount(request.getAmount());
        payment.setStatus("NEW");
        repo.save(payment);

        try {
            kafka.send(
                    "payments",
                    new ObjectMapper()
                            .writeValueAsString(payment)
            );
        } catch (Exception e) {
            e.printStackTrace();
        }

        payment.setStatus("SENT");
        repo.save(payment);
    }

    Integer getStatus() {
        new RuntimeException();
    }
}

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

Подсказки

💡 Проверь Dependency Injection и создание ObjectMapper.
💡 Отправка через KafkaTemplate.send() выполняется асинхронно.
💡 БД и Kafka не участвуют в одной обычной @Transactional.
💡 Посмотри, когда устанавливается статус SENT.
💡 Проверь реализацию getStatus().

Решение

Основные проблемы:

используется field injection вместо constructor injection;
ObjectMapper создаётся при каждом вызове;
KafkaTemplate.send() асинхронный, поэтому try/catch не гарантирует обработку ошибки отправки;
даже при неуспешной отправке платеж получает статус SENT;
e.printStackTrace() не подходит для серверного приложения;
транзакция БД не делает сохранение в БД и отправку в Kafka одной атомарной операцией;
сообщение может быть отправлено до успешного commit транзакции;
статус лучше хранить через enum, а не строкой;
второй repo.save(payment) обычно не нужен для managed entity внутри транзакции;
getStatus() не возвращает значение;
new RuntimeException() только создаёт исключение, но не выбрасывает его.

Исправление базовых проблем:

@Service
@RequiredArgsConstructor
@Slf4j
public class PaymentService {

    private final PaymentRepository repo;
    private final KafkaTemplate<String, String> kafka;
    private final ObjectMapper objectMapper;

    @Transactional
    public void processPayment(PaymentRequest request) {
        Payment payment = new Payment();
        payment.setAmount(request.getAmount());
        payment.setStatus(PaymentStatus.NEW);

        repo.save(payment);

        try {
            String message =
                    objectMapper.writeValueAsString(payment);

            kafka.send("payments", message);

            payment.setStatus(PaymentStatus.SENT);
        } catch (JsonProcessingException e) {
            log.error(
                    "Failed to serialize payment {}",
                    payment.getId(),
                    e
            );

            throw new IllegalStateException(
                    "Failed to serialize payment",
                    e
            );
        }
    }
}

Однако даже такой вариант не решает главную проблему согласованности:

БД commit
+
Kafka send

не являются одной транзакцией.

Например:

payment сохранён в БД
↓
Kafka недоступна
↓
сообщение потеряно

Или наоборот — сообщение может уйти, а транзакция БД затем откатится.

Для production-сценария обычно применяют Transactional Outbox: вместе с Payment в одной DB-транзакции сохраняется событие в outbox-таблицу, а отдельный процесс публикует его в Kafka.

Метод:

Integer getStatus() {
    new RuntimeException();
}

не компилируется, поскольку отсутствует return.

Если нужно выбросить исключение:

public Integer getStatus() {
    throw new RuntimeException();
}

Если метод должен возвращать статус, необходимо вернуть соответствующее значение согласно контракту интерфейса Status.