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

16. Провести code review ProductService и ProductController

Условие задачи:
Дан Spring Boot-сервис работы с товарами. Нужно провести code review ProductController, ProductService и связанных классов: найти ошибки, риски для production и предложить рефакторинг.

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

Подсказки

💡 Проверь сигнатуру main(), Dependency Injection и final у Spring-сервиса.
💡 parallelStream() и Spring-транзакции плохо сочетаются: транзакционный контекст привязан к потоку.
💡 @Transactional на saveProduct() не сработает при self-invocation.
💡 Внешний вызов и отправку уведомления не стоит выполнять внутри долгой DB-транзакции.
💡 Для getAll() нужна пагинация.

Решение

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

main() должен принимать String[] args;
field injection вместо constructor injection;
@RequiredArgsConstructor в контроллере бесполезен, пока поле не final;
ProductService лучше не делать final, если используются proxy-based @Transactional;
parallelStream() выполняет работу в других потоках, поэтому одна транзакция createProducts() не охватывает весь batch;
@Transactional на saveProduct() не применяется при вызове this::saveProduct;
вызов inventoryService внутри транзакции увеличивает её длительность;
уведомление может уйти до commit, а БД затем откатиться;
нет полноценной Bean Validation;
findAll() без пагинации плохо масштабируется;
InterruptedException нужно обрабатывать с восстановлением interrupt flag;
@Data на JPA entity нежелателен из-за автоматически сгенерированных equals/hashCode/toString;
логировать целую entity обычно хуже, чем её идентификатор;
опечатки в Lombok-аннотациях не дадут коду скомпилироваться.

Исправленная точка входа:

@SpringBootApplication
public class TaskApp {

    public static void main(String[] args) {
        SpringApplication.run(TaskApp.class, args);
    }
}

Контроллер:

@RestController
@RequestMapping("/api/v1/products")
@RequiredArgsConstructor
class ProductController {

    private final ProductService productService;

    @PostMapping
    public List<ProductResponse> createProducts(
            @RequestBody @NotEmpty
            @Valid List<ProductCreationRequest> products
    ) {
        return productService.createProducts(products);
    }

    @GetMapping
    public Page<ProductResponse> fetchAll(Pageable pageable) {
        return productService.getAll(pageable);
    }
}

DTO лучше валидировать декларативно:

record ProductCreationRequest(
        @NotNull Long productId,
        @NotBlank String productName,
        @NotNull @PositiveOrZero Long price
) {
}

Сервис:

@Service
@RequiredArgsConstructor
@Slf4j
class ProductService {

    private final ProductRepository repository;
    private final InventoryService inventoryService;

    @Transactional
    public List<ProductResponse> createProducts(
            List<ProductCreationRequest> products
    ) {
        return products.stream()
                .map(this::saveProduct)
                .toList();
    }

    private ProductResponse saveProduct(
            ProductCreationRequest request
    ) {
        Product product =
                ProductMapper.fromRequest(request);

        product.setQuantity(
                inventoryService.getQuantityForProduct(
                        request.productId()
                )
        );

        log.info(
                "Saving product with id={}",
                product.getProductId()
        );

        return ProductMapper.fromEntity(
                repository.save(product)
        );
    }

    @Transactional(readOnly = true)
    public Page<ProductResponse> getAll(
            Pageable pageable
    ) {
        return repository.findAll(pageable)
                .map(ProductMapper::fromEntity);
    }
}

Главный момент: parallelStream() здесь лучше убрать. Spring-транзакция привязана к текущему потоку, поэтому worker-потоки parallelStream() не участвуют в одной транзакции createProducts().

Также уведомления лучше отправлять после успешного commit, например через @TransactionalEventListener(phase = AFTER_COMMIT) или outbox, иначе можно уведомить о товаре, сохранение которого затем откатилось.

Если inventoryService — действительно удалённый вызов, его желательно выполнять до открытия короткой DB-транзакции либо разделить сценарий на этапы, чтобы не держать соединение с БД во время ожидания сети.

Для InterruptedException:

catch (InterruptedException e) {
    Thread.currentThread().interrupt();
    throw new IllegalStateException(
            "Inventory request was interrupted",
            e
    );
}

Итого самые важные production-риски:

parallelStream() + транзакции;
self-invocation @Transactional;
долгий внешний вызов внутри транзакции;
уведомление до commit;
отсутствие пагинации и валидации.