18. Сделать ревью кода ProductService и ProductController
Условие задачи:
🔥 Сделай ревью кода ниже. Нужно найти проблемы в ProductService, ProductController и связанных классах. На что обратить внимание, что исправить, какие риски и улучшения возможны?
@SpringBootApplication
public class TaskApp {
public static void main() {
SpringApplication.run(TaskApp.class);
}
}
@RestController
@RequestMapping("/api/v1/products")
@RequiredArgsConstructor
class ProductController {
@Autowired
private ProductService productService;
@PostMapping
public List<ProductResponse> createProducts(
@RequestBody List<ProductCreationRequest> products
) {
return productService.createProducts(products);
}
@GetMapping
public List<ProductResponse> fetchAll() {
return productService.getAll();
}
}
@Service
@Slf4j
final class ProductService {
@Autowired
private ProductRepository repository;
@Autowired
private InventoryService inventoryService;
@Autowired
private NotificationService notificationService;
@Transactional(rollbackFor = Exception.class)
public List<ProductResponse> createProducts(List<ProductCreationRequest> products) {
return products.parallelStream().map(
this::saveProduct
).toList();
}
@Transactional(rollbackFor = Exception.class)
public ProductResponse saveProduct(ProductCreationRequest product) {
Product toSave = ProductMapper.fromRequest(product);
toSave.setQuantity(inventoryService.getQuantityForProduct(product.productId()));
log.info("Saving product {}", toSave);
ProductResponse response = ProductMapper.fromEntity(
repository.save(toSave)
);
notificationService.sendNotification(response);
return response;
}
public List<ProductResponse> getAll() {
return repository.findAll().stream().map(ProductMapper::fromEntity).toList();
}
}
Спойлеры к решению
Подсказки
public static void main() написан неправильно: у main должен быть параметр String[] args.💡 Нельзя одновременно использовать
@RequiredArgsConstructor и field injection через @Autowired на поле. Лучше constructor injection.💡
ProductService объявлен final, из-за этого Spring может не создать proxy для @Transactional.💡
@Transactional на saveProduct() может не сработать при вызове из того же класса через this::saveProduct.💡
parallelStream() внутри транзакционного сервиса — плохая идея: транзакционный контекст не переносится безопасно между потоками.💡 Внутри транзакции вызывается внешний сервис
inventoryService, который может долго отвечать и падать. Это держит транзакцию открытой дольше нужного.💡 Уведомления нельзя отправлять до финального commit транзакции: можно отправить уведомление о товаре, который потом откатится.
💡
InterruptedException нельзя молча игнорировать, нужно восстановить interrupt flag через Thread.currentThread().interrupt().💡 В коде есть опечатки:
@NoArgsContructor, @AllArgsContructor. Должно быть @NoArgsConstructor, @AllArgsConstructor.💡 В
ProductMapper.fromRequest() код обрезан и, скорее всего, не компилируется.💡 Нужна валидация входных данных:
products, productId, productName, price.💡
repository.findAll() без пагинации может положить сервис на больших объёмах данных.💡
@Data на JPA entity может быть опасен из-за equals, hashCode, toString. Лучше использовать @Getter / @Setter.Решение
@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 @Valid List<ProductCreationRequest> products
) {
return productService.createProducts(products);
}
@GetMapping
public List<ProductResponse> fetchAll() {
return productService.getAll();
}
}
@Service
@Slf4j
@RequiredArgsConstructor
class ProductService {
private final ProductRepository repository;
private final InventoryService inventoryService;
private final NotificationService notificationService;
@Transactional
public List<ProductResponse> createProducts(List<ProductCreationRequest> products) {
if (products == null || products.isEmpty()) {
return List.of();
}
return products.stream()
.map(this::saveProductInternal)
.toList();
}
private ProductResponse saveProductInternal(ProductCreationRequest product) {
validate(product);
Product toSave = ProductMapper.fromRequest(product);
Integer quantity = inventoryService.getQuantityForProduct(product.productId());
toSave.setQuantity(quantity);
log.info("Saving product with id={}", toSave.getProductId());
Product saved = repository.save(toSave);
ProductResponse response = ProductMapper.fromEntity(saved);
// В простом варианте оставляем здесь,
// но в реальном проекте лучше отправлять после commit транзакции.
notificationService.sendNotification(response);
return response;
}
@Transactional(readOnly = true)
public List<ProductResponse> getAll() {
return repository.findAll()
.stream()
.map(ProductMapper::fromEntity)
.toList();
}
private void validate(ProductCreationRequest product) {
if (product == null) {
throw new IllegalArgumentException("Product request must not be null");
}
if (product.productId() == null) {
throw new IllegalArgumentException("Product id must not be null");
}
if (product.productName() == null || product.productName().isBlank()) {
throw new IllegalArgumentException("Product name must not be blank");
}
if (product.price() == null || product.price() < 0) {
throw new IllegalArgumentException("Product price must not be null or negative");
}
}
}
@Service
@Slf4j
class NotificationService {
public void sendNotification(ProductResponse product) {
log.info("New product: {} in our shop! Come check it out!", product.productName());
}
}
@Service
class InventoryService {
private final Random random = new Random();
public Integer getQuantityForProduct(Long productId) {
try {
Thread.sleep(random.nextLong(500L, 2000L));
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new IllegalStateException("Inventory request was interrupted", e);
}
Integer quantity = random.nextInt(0, 20);
if (quantity > 15) {
throw new IllegalStateException("Inventory service error for productId=" + productId);
}
return quantity;
}
}
@Repository
interface ProductRepository extends JpaRepository<Product, Long> {
}
@Entity
@Table(name = "products")
@Getter
@Setter
@NoArgsConstructor
@AllArgsConstructor
class Product {
@Id
@Column(name = "product_id", nullable = false)
private Long productId;
@Column(name = "product_name", unique = true, nullable = false)
private String productName;
@Column(name = "product_quantity")
private Integer quantity;
@Column(name = "product_price", nullable = false)
private Long price;
@Column(name = "created_at", updatable = false)
@CreationTimestamp
private LocalDateTime createdAt;
@Column(name = "updated_at")
@UpdateTimestamp
private LocalDateTime updatedAt;
}
record ProductResponse(
Long productId,
String productName,
Integer quantity,
Long price
) {
}
record ProductCreationRequest(
Long productId,
String productName,
Long price
) {
}
@UtilityClass
class ProductMapper {
public ProductResponse fromEntity(Product product) {
return new ProductResponse(
product.getProductId(),
product.getProductName(),
product.getQuantity(),
product.getPrice()
);
}
public Product fromRequest(ProductCreationRequest request) {
Product product = new Product();
product.setProductId(request.productId());
product.setProductName(request.productName());
product.setPrice(request.price());
return product;
}
}
Что было не так #
1. Некорректный main
#
Было:
public static void main() {
SpringApplication.run(TaskApp.class);
}
Правильно:
public static void main(String[] args) {
SpringApplication.run(TaskApp.class, args);
}
Без String[] args это не стандартная точка входа Java-приложения.
2. Смешан @RequiredArgsConstructor и @Autowired на поле
#
В контроллере есть @RequiredArgsConstructor, но зависимость внедряется через поле:
@RequiredArgsConstructor
class ProductController {
@Autowired
private ProductService productService;
}
@RequiredArgsConstructor здесь бесполезен, потому что поле не final.
Лучше так:
@RequiredArgsConstructor
class ProductController {
private final ProductService productService;
}
То же самое в ProductService: лучше заменить field injection на constructor injection.
3. ProductService объявлен final
#
final class ProductService
Это рискованно для Spring-сервисов, особенно если используются AOP-механизмы: @Transactional, кеширование, security-аннотации. Spring часто создаёт proxy-класс, а final может этому помешать.
Лучше:
@Service
class ProductService
4. @Transactional на saveProduct() может не работать
#
@Transactional
public ProductResponse saveProduct(ProductCreationRequest product) {
...
}
Метод вызывается внутри того же класса:
products.parallelStream().map(this::saveProduct).toList();
Это self-invocation. В таком случае вызов не проходит через Spring proxy, поэтому @Transactional на saveProduct() может не примениться.
Если нужна транзакция на весь batch, достаточно оставить @Transactional на createProducts().
Если нужна отдельная транзакция на каждый продукт, нужно выносить сохранение одного продукта в отдельный Spring bean.
5. parallelStream() внутри транзакции
#
@Transactional
public List<ProductResponse> createProducts(List<ProductCreationRequest> products) {
return products.parallelStream().map(this::saveProduct).toList();
}
Это одна из главных проблем.
parallelStream() использует несколько потоков. Транзакционный контекст Spring обычно привязан к текущему потоку через ThreadLocal. Из-за этого код может вести себя непредсказуемо: часть операций может выполняться вне ожидаемой транзакции, ошибки могут обрабатываться неудобно, а нагрузка на БД может резко вырасти.
Лучше использовать обычный stream():
return products.stream()
.map(this::saveProductInternal)
.toList();
6. Внешний вызов внутри транзакции #
toSave.setQuantity(inventoryService.getQuantityForProduct(product.productId()));
InventoryService симулирует внешний сетевой вызов:
Thread.sleep(random.nextLong(500L, 2000L));
Если делать такой вызов внутри транзакции, соединение с БД и транзакционные ресурсы могут удерживаться слишком долго.
Лучше получать данные из внешнего сервиса до открытия транзакции или разделять процесс на этапы:
1. Провалидировать входные данные
2. Получить остатки из InventoryService
3. Открыть транзакцию
4. Сохранить продукты
5. После commit отправить уведомления
7. Уведомление отправляется до commit #
ProductResponse response = ProductMapper.fromEntity(repository.save(toSave));
notificationService.sendNotification(response);
return response;
Проблема: уведомление уже отправлено, но транзакция ещё может откатиться.
Например:
1. Product сохранён
2. Notification отправлен
3. Позже произошла ошибка
4. Транзакция откатилась
5. Пользователь получил уведомление о товаре, которого нет в БД
В реальном проекте лучше отправлять уведомления после успешного commit. Например через domain events, @TransactionalEventListener(phase = AFTER_COMMIT) или outbox pattern.
8. Игнорируется InterruptedException
#
Было:
try {
Thread.sleep(random.nextLong(500L, 2000L));
} catch (InterruptedException _) {
}
Так делать нельзя. Поток попросили прервать, а код это проигнорировал.
Лучше:
catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new IllegalStateException("Inventory request was interrupted", e);
}
9. Нет валидации входных данных #
Контроллер принимает список:
@RequestBody List<ProductCreationRequest> products
Но нигде не проверяется:
products != null
product != null
productId != null
productName не пустой
price != null
price >= 0
Лучше добавить Bean Validation:
record ProductCreationRequest(
@NotNull Long productId,
@NotBlank String productName,
@NotNull @PositiveOrZero Long price
) {
}
И в контроллере:
public List<ProductResponse> createProducts(
@RequestBody @Valid List<ProductCreationRequest> products
)
10. getAll() без пагинации
#
public List<ProductResponse> getAll() {
return repository.findAll().stream().map(ProductMapper::fromEntity).toList();
}
На маленьких данных это работает. На проде таблица может быть большой, и findAll() начнёт грузить всё в память.
Лучше использовать Pageable:
@GetMapping
public Page<ProductResponse> fetchAll(Pageable pageable) {
return productService.getAll(pageable);
}
@Transactional(readOnly = true)
public Page<ProductResponse> getAll(Pageable pageable) {
return repository.findAll(pageable)
.map(ProductMapper::fromEntity);
}
11. Опечатки в Lombok-аннотациях #
Было:
@NoArgsContructor
@AllArgsContructor
Правильно:
@NoArgsConstructor
@AllArgsConstructor
12. @Data на JPA entity
#
@Data
class Product
@Data генерирует toString, equals, hashCode, getters, setters. Для JPA entity это часто опасно, особенно если потом появятся связи @OneToMany, @ManyToOne.
Лучше использовать:
@Getter
@Setter
@NoArgsConstructor
@AllArgsConstructor
13. ProductResponse.price лучше сделать Long, а не long
#
Сейчас:
record ProductResponse(
Long productId,
String productName,
Integer quantity,
long price
)
А в entity:
private Long price;
Если price == null, при маппинге в long будет NullPointerException.
Лучше:
record ProductResponse(
Long productId,
String productName,
Integer quantity,
Long price
)
14. Логирование всей entity #
log.info("Saving product {}", toSave);
На первый взгляд удобно, но на проде это может стать проблемой:
- в лог может попасть лишняя бизнес-информация;
- если entity большая, лог будет шумным;
- если появятся lazy-связи, можно случайно спровоцировать их загрузку;
- при @Data toString может быть тяжёлым или рекурсивным.
Лучше логировать идентификатор:
log.info("Saving product with id={}", toSave.getProductId());
Основные риски #
Главные проблемы в этом коде не синтаксические, а архитектурные:
parallelStream + @Transactional
self-invocation @Transactional
final service class
внешний вызов внутри транзакции
notification до commit
отсутствие валидации
findAll без пагинации
На тестовых данных код может выглядеть рабочим, но на проде он будет нестабильным: долгие транзакции, непредсказуемое поведение при ошибках, лишняя нагрузка на БД и уведомления о товарах, которые фактически не сохранились.