Code review ClientController и связанных классов

30. Провести ревью получения полисов клиента

Условие задачи:
Дан REST-контроллер, который должен вернуть оплаченные полисы указанного клиента с ограничением на количество результатов.

Нужно провести code review: найти ошибки компиляции, проблемы в REST API, Dependency Injection, транзакционности и фильтрации данных, а затем предложить более корректную реализацию.

Код:

@RestController
public class ClientController {

    @Value(""policy.limit"")
    private int policyLimit;

    @Autowired
    private PolicyService policyService;

    @RequestMapping(path = ""client/{clientId}/policies"", method = RequestMethod.POST)
    public Response getClientPolicies(@PathVariable(""clientId"") String clientId) {
        List<PolicyDTO> policies = getPolicies().stream()
            .limit(policyLimit)
            .filter(p -> p.getClientIds().contains(clientId))
            .filter(p -> p.isPaid())
            .toList();
        return new Response(policies);
    }

    @Transactional
    private List<PolicyDTO> getPolicies() {
        return policyService.getPolicies();
    }

    @Data
    @AllArgsConstructor
    public class Response {
        private List<PolicyDTO> policies;
    }
}

@Data
public class PolicyDTO {
    private String id;
    private String name;
    private List<String> clientId;
    private Boolean isPaid;
}

@RequiredArgsConstructor
@Component
public class PolicyService {
    private final PolicyDbRepository repository;
    /**
     *  Читает полисы из БД
     */
    List<PolicyDTO> getPolicies() {
        return repository.getPolicies();
    }
}

public interface PolicyDbRepository {
    List<PolicyDTO> getPolicies();
}

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

Подсказки

💡 Проверь выражение в @Value и HTTP-метод для операции чтения.
💡 Сравни поля PolicyDTO с методами, которые вызываются в Stream.
💡 Обрати внимание на порядок limit() и filter().
💡 @Transactional работает через Spring proxy.
💡 Лучше не загружать все полисы из БД, если нужен небольшой отфильтрованный набор.

Решение

Проблемы:

@Value("policy.limit") содержит литерал вместо ${...};
используется field injection;
для чтения данных выбран POST вместо GET;
@Transactional стоит на private-методе и вызывается внутри того же класса, поэтому через Spring AOP не сработает;
сначала выполняется limit(), а потом фильтрация — часть подходящих полисов может не попасть в результат;
PolicyDTO содержит поле clientId, но вызывается getClientIds() — код не компилируется;
для Boolean isPaid вызов isPaid() также не соответствует обычному Lombok-геттеру;
clientIds потенциально может быть null;
контроллер содержит бизнес-логику фильтрации;
все полисы загружаются из БД и фильтруются в памяти;
без явной сортировки набор первых policyLimit записей может быть недетерминированным;
Response лучше сделать отдельным record или static-классом;
отрицательный policyLimit приведёт к ошибке и должен быть исключён конфигурацией/валидацией.

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

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

@RestController
@RequiredArgsConstructor
@RequestMapping("/clients")
public class ClientController {

    private final PolicyService policyService;

    @GetMapping("/{clientId}/policies")
    public Response getClientPolicies(
            @PathVariable String clientId
    ) {
        return new Response(
                policyService.getClientPolicies(clientId)
        );
    }

    public record Response(List<PolicyDTO> policies) {
    }
}

DTO лучше привести к именам, которые соответствуют его смыслу:

@Data
public class PolicyDTO {

    private String id;
    private String name;
    private List<String> clientIds;
    private boolean paid;
}

Лимит и транзакцию перенести в сервис:

@Service
@RequiredArgsConstructor
public class PolicyService {

    private final PolicyDbRepository repository;

    @Value("${policy.limit:100}")
    private int policyLimit;

    @Transactional(readOnly = true)
    public List<PolicyDTO> getClientPolicies(
            String clientId
    ) {
        if (clientId == null || clientId.isBlank()) {
            throw new IllegalArgumentException(
                    "clientId must not be blank"
            );
        }

        return repository.getPolicies().stream()
                .filter(p -> p.getClientIds() != null)
                .filter(p ->
                        p.getClientIds().contains(clientId)
                )
                .filter(PolicyDTO::isPaid)
                .limit(policyLimit)
                .toList();
    }
}

Но это всё ещё загружает все полисы в память. Лучше сразу фильтровать и ограничивать выборку в БД:

public List<PolicyDTO> getClientPolicies(
        String clientId
) {
    return repository.findPaidByClientId(
            clientId,
            policyLimit
    );
}

Репозиторий должен выполнять запрос примерно по смыслу:

SELECT ...
FROM policy
...
WHERE client_id = :clientId
  AND paid = true
ORDER BY ...
LIMIT :limit

Так БД сразу вернёт только нужные записи, вместо загрузки всей таблицы и последующей фильтрации в Java.

Главные исправления — перенести фильтрацию из контроллера, применять лимит после отбора подходящих записей и по возможности выполнять фильтрацию/лимит непосредственно в БД.