Logika w kontrolerze rest

Czy coś takiego w kontrolerze jest akceptowalne według was czy już za dużo logiki i kontroler za dużo robi?

@RestController
@RequestMapping("/payments")
@RequiredArgsConstructor
public class PaymentController {

    private final PaymentService paymentService;
    private final PaymentToRestModelConverter converter;

    @PostMapping("/startPayment")
    private PaymentStartResponse startPayment(@Valid @RequestBody PaymentStartRequest rq) {
        PaymentStarted paymentStarted = paymentService.startPayment(new PaymentData(rq.language, rq.country));
        return converter.convert(paymentStarted);
    }
}

1. Ale czemu tą konwersje robisz tutaj a nie w paymentService?
2. Nie podoba mi sie ze ta metoda zwraca PaymentStartResponse a nie ResponseEntity<PaymentStartResponse> i nie podoba mi się że startPayment zwraca PaymentStarted a nie Optional<PaymentStarted> (albo jakis Either) bo nie wierze że ta operacja nie może sfailować :P

@Shalom:

Tak wygląda mój paymentService:

public class PaymentService {

    private final TransactionService transactionService;
    private final TransactionRepository transactionRepository;
    private final TransactionFactory transactionFactory;

    public PaymentService(TransactionService transactionService,
                          TransactionRepository transactionRepository, TransactionFactory transactionFactory) {
        this.transactionService = transactionService;
        this.transactionRepository = transactionRepository;
        this.transactionFactory = transactionFactory;
    }

    public PaymentStarted startPayment(PaymentData paymentData) {
        Transaction transaction = transactionFactory
                .createTransactionForOneTimePayment(paymentData);
        try {
            save(transaction);
            PaymentLink paymentLink = transactionService.registerTransaction(transaction);
            transaction.setPaymentLink(paymentLink);
            transaction.startProcessing();
            return new PaymentStarted(transaction.getTransactionId(), paymentLink);
        } catch (TransactionServiceException e) {
            transaction.completeProcessingWithFailure(e.getMessage());
            throw e;
        } finally {
            save(transaction);
        }
    }

    private void save(Transaction transaction) {
        transactionRepository.saveTransaction(transaction);
    }
}

Nie zwracam tutaj Either bo jak coś tutaj się nie powiedzie to bardziej traktuję to jako wyjątek.
Mapowania na błędy api jeszcze nie mam ale de facto zamiast robić ResponseEntity można by też
jakiś globalny ControllerAdvice dać ;)

1. Wyjątek to jest np. internet umarł albo dysk padł albo RAM się skończył ;)
2. Jeśli bardzo nie chcesz tej konwersji mieć w tym serwisie bo uważasz ze to nie jest operacja domenowa, to możesz zrobić coś pomiędzy

@Shalom:

Wyjątek to jest np. internet umarł albo dysk padł albo RAM się skończył ;)

W podanym wyżej przypadku ja to interpretuje jako wyjątek bo ten exception nie zależy od danych wejściowych w request.
Jeżeli ten wyjątek leci to jest jakiś problem infrastrukturalny albo na styku integracji z bramką płatności i jakiś admin/dev powinien się tym zająć.
Jeżeli mamy np. SLA na poziomie 99.99% to dla mnie zdecydowanie jest to sytuacja wyjątkowa. Racja?

Jeśli bardzo nie chcesz tej konwersji mieć w tym serwisie bo uważasz ze to nie jest operacja domenowa, to możesz zrobić coś pomiędzy

Czyli np. między PaymentController a PaymentService jakiś PaymentApplicationService albo PaymentFacade, który realizuje mapowanie?
Co mi taki dodatkowy byt daje?

Nie podoba mi sie ze ta metoda zwraca PaymentStartResponse a nie ResponseEntity<PaymentStartResponse>

Używam czasem ResponseEntity ale jaka by była zaleta w tym przypadku?

Nic ci nie daje, ale to ty nie chciałeś tu wrzucić tej konwersji :)
Zaletą byłoby zwrócenie jakiegoś error code i error message związanego z tym co się stało. Generyczny exception handler jest o tyle słaby, że jest "ogólny" i nie wiesz za bardzo "skad" ten wyjątek przyszedł, chyba że masz tych wyjątków dziesiątki, ale wtedy to też jest bardzo dziwne podejście, takie w stylu AOP że kawałek logiki (w tym przypadku error message i error code) są gdzieś w zupełnie innym miejscu w kodzie.

Nic ci nie daje, ale to ty nie chciałeś tu wrzucić tej konwersji :)

Nie to, że nie chciałem tylko zapytałem czy jest to akceptowalne :)
Pewnie przy większych DTO masz rację żeby to wydzielić i stworzyć jakiś
ApplicationService pomiędzy tym, który tworzy mapuje z tego DTO na jakieś eventy/obiekty.

Generyczny exception handler jest o tyle słaby, że jest "ogólny" i nie wiesz za bardzo "skad" ten wyjątek przyszedł

Ja robiłem np. jakieś ApiErrorException jeden, który miał w sobie jaki status http trzeba zwrócić plus jakiś wewnętrzny kod błędu
i to potem w tym ControllerAdvice mapujesz na response ;)