Optional i wyjątki

Sprawa wygląda następująco, mam metodę która zwraca Optionala, którego wartość muszę użyć w kolejnej metodzie(makeRequest), która znowu może rzucić wyjątek. Próbowałem na wszelkie możliwe sposoby z map, ifPresent, i innymi, ale żaden sposób nie chcę działać. W końcu zrobiłem coś takiego

 ActiveUserDTO getActiveUser(String sessionID) throws Exception {
        Optional<OAuth2AccessToken> accessToken = authorizationService.getAccessToken(UUID.fromString(sessionID));
        
        if(accessToken.isPresent()) {
            Response response = authorizationService.makeRequest(Verb.GET, Constants.ACTIVE_USER_URL, accessToken.get());
            return new ActiveUserDTO(response.getBody());
        } else {
            throw new Exception("Something went wrong...");
        }
    }

W teorii robi to co chcę, jeżeli optional zwróci pustą wartość, poleci wyjątek który sobie jakoś obsłużę w fasadzie/controllerze, jeżeli uda się znaleźć wartość, strzelam do zewnętrznej usługi i znów, jeżeli uda się, zwracam wynik, jeżeli nie, rzucam wyjątek.

Za pewne jednak, da się to zrobić dużo ładniej niż optional.isPresent() - pytanie tylko jak, ktoś mógłby pomóc?

Nie widzę nic tragicznego w tym kodzie. Chyba, że zaraz rzucą się puryści, którym nic nie pasuje.
Jak używasz JDK >= 9, to użyj sobie metody ifPresentOrElse​(...)

Problem w tym, że ifPresentOrElse, itp, krzyczy że muszę wewnątrz niego obsłużyć wyjątki. A ja ewentualny wyjątek który poleci w makeRequest, chcę przekazać do klasy wyżej.

No to złap wyjątek i rzuć sobie nowy RuntimeException może.

Optional ma jeszcze taką fajną metodę jak orElseThrow.

OAuth2AccessToken accessToken = authorizationService.getAccessToken(UUID.fromString(sessionID))
                                                    .orElseThrow(() -> new Exception("Something went wrong..."));
Response response = authorizationService.makeRequest(Verb.GET, Constants.ACTIVE_USER_URL, accessToken);
(...)

Jeszcze tylko pytanie na kiego grzyba ten wyjątek i co z nim robisz. Bo może lepiej po prostu zwrócić tego Optionala? Optional<ActiveUserDTO>

jarekr000000 napisał(a):

Jeszcze tylko pytanie na kiego grzyba ten wyjątek i co z nim robisz. Bo może lepiej po prostu zwrócić tego Optionala? Optional<ActiveUserDTO>

Wyjątek może polecieć bo:
-komuś wygasł token, czyli ten Optional jest na starcie pusty
-coś poszło nie tak przy wywołaniu RESTa

Zwracając po prostu Optionala na dobrą sprawę nie wiem co się stało, a to istotne z punktu widzenia klienta aplikacji.

Wymyśliłem coś takiego

ActiveUserDTO getActiveUser(String sessionID) throws RuntimeException {
     Optional<OAuth2AccessToken> accessToken = authorizationService.getAccessToken(UUID.fromString(sessionID));
     return accessToken.map(this::callMethod)
             .orElseThrow(() -> new RuntimeException("Token expired"));

 }

 private ActiveUserDTO callMethod(OAuth2AccessToken token) {
     try {
         Response response = authorizationService.makeRequest(Verb.GET, Constants.ACTIVE_USER_URL, token);
         return new ActiveUserDTO(response.getBody());
     } catch (Exception e) {
         throw new RuntimeException("Error while invoking " + Constants.ALL_USERS_DATA_URL + e.getMessage());
     }
 }

Ale to w sumie rozwiązuje problem nieco prościej i czytelniej

OAuth2AccessToken accessToken = authorizationService
                .getAccessToken(UUID.fromString(sessionID))
                .orElseThrow(() -> new RuntimeException("Token expired"));

Szybkim rozwiązaniem mogłoby być nie rzucanie wyjątkiem, a użycie Try. W twoim przypadku mogłoby to wyglądać np. tak:

 Try<ActiveUserDTO> getActiveUser(String sessionID) {
       authorizationService.getAccessToken(UUID.fromString(sessionID)).map(token -> {
          Response response = authorizationService.makeRequest(Verb.GET, Constants.ACTIVE_USER_URL, token);
          return new ActiveUserDTO(response.getBody());
       }).toTry(() -> new Exception("Something went wrong"));

Nie mniej nie jest to wielce optymalne rozwiązanie, brnąc w Try'a to kod pi razy drzwi zapewne powinien mieć mniej więcej taką postać:

Try<ActiveUserDTO> getActiveUser(String sessionID) {
    authorizationService
        .getAccessToken(UUID.fromString(sessionID))
        .toTry(() -> new Exception("could not get access token"))
        .flatMap(token -> authorizationService.makeRequest(Verb.GET, Constants.ACTIVE_USER_URL, token))
        .map(response -> new ActiveUserDTO(response.getBody());
}

Nadal modelowanie potencjalnych błędów kuleje ale przynajmniej w jakiś tam sposób zabezpieczasz się przed błędami związanymi z wysyłaniem requestów.

Pytanie czego oczekujesz od tej metody. Jeśli nie interesuje Cię dlaczego coś się nie powiodło, a jedynie ostateczny wynik to wtedy Optional ma sens. Mówi bezpośrednio, że albo coś jest albo tego nie ma, bez żadnych szczegółów. Jeśli interesuje Cię przyczyna niepowodzenia to wtedy zdecydowanie bardziej nadaje się Try albo Either.

Twoja sygnatura metody (ActiveUserDTO getActiveUser(String sessionID) throws Exception ) zaprzecza temu po co zostały stworzone Optional, Try czy Either. Poinformuj potencjalnego użytkownika metody bezpośrednio o tym, że może coś pójść nie tak czy o tym, że może nie być wyniku.

Czyli zlym rozwiązaniem byłoby stworzenie dedykowanego wyjątku np. TokenExpiredException i rzucanie go w razie potrzeby z tej metody? Dodam ze to serwis zamknięty wewnątrz pakietu, dostępny tylko poprzez fasade.

Optional tutaj odpada z tego względu ze chce wiedzieć co konkretnie poszło nie tak. Zastanawia mnie jedynie przewaga Try badz Either nad zwykłym wyjątkiem pod ten konkretny przypadek.

1. Zrobienie TokenExpiredException jest ok. W zamyśle twórców javy to powinno być zrobione jako checked exception (żeby klient wiedział, że ma obsłużyć). (ale małym programie możesz sobie zrobić dziedziczenie z RuntimeException jeśli skutki nieobsłużenia są niewielkie - przy runtime można łatwo zapomnieć).
2. Skoro jest tylko jeden rodzaj problemu to Optional jest ok. Jak dostajesz empty to wiesz, że expired :-)
3. Gdyby było więcej rodzajów problemów to możesz użyć Either<Problem, ActiveUserDTO>
   Gdzie Problem to np.:

enum Problem {
 TokenExpired,
 BadToken,
 EvenWorseToken
}

(nie musi być enum, to może być dowolna klasa, np. rekord ze szczegółami błedu).
Przewagi Eithera nad CheckedException są takie:

• referential transparency -> łatwiej testować - można np testować parametrycznie. Testowanie, że poleciał wyjątek wymaga ( dodatkowej gimnastyki, (małej ale jednak). Konsekwencje referential transparency są takie, że wyjątkiem/Either posługujesz sie jak zwykła wartością - możesz łatwiej przekazać do innych metod (np. jako listę). Natomiast klasyczne Exceptiony zawsze mają trochę osobny flow.
• Rzucanie wyjątku jest troszkę bardziej kosztowne z punktu widzenia JVM (trzeba zapenić odtworzenie Stack Trace) - u Ciebie ten argument nie ma znaczenia, raczej nie będziesz miał 10k expired tokenów na sekundę,
• klasa Left nie musi dziedziczyć z Exception/Throwable czyli np. łatwo zrobić enum, dużo mniej pitolenia jak masz dużo różnych błedów,
  (ale to pomniejszy problem, bo w wersji Checked Exceptiony opakowujesz dodatkowo tego enuma Exceptionem i masz prawie to samo... ).

4. Try to taki ograniczony Either, gdzie problem (czyli Left) to Exception
   czyli:
   Try<T> =~= Either<Exception, T>
   (mniej więcnie równa się).
   Try ma raczej sens tam gdzie już są Exceptiony, bo lecą z istniejącego API i chcemy je opakować, bo lubimy referential transparency.

5. Wszelkie błedy typu Panic - czyli np. co tam zły token, jak sieci nie ma - nadal najlepiej modelować jako RuntimeException, w założeniu, że w takiej sytuacji i tak obsługa błędu nie ma sensu. Można co najwyżej wywalić info na ekran, do logów lub zakończyć program.

6. (uzupełnienie) Dla szeleńców, którzy kochają sterowanie (flow) przy pomocy Exceptionów, a płaczą nad wydajnością jest ratunek w postaci writableStackTrace (false)
   https://stackoverflow.com/questions/11434431/exception-without-stack-trace-in-java
   Nie polecam, ale jak ktoś bardzo lubi programować dyskunkcyjnie, to ma możliwość od Javy 7.

@jarekr000000:
Mnie zawsze zastanawiało - jak zarządzać tranzakcjami (oczywiście w Springu), zrobić np. rollbacka, gdy zamiast wyjątków używa się Either lub Try? Czy trzeba w takim robić to ręcznie z pomocą np. TransactionTemplate? Jeśli tak, to wydaje się to być mega uciążliwe.

Nie, nie będzie złym, tylko innym. Generalnie załóż że rzucenie wyjątku robisz kiedy handling jest obsługiwany gdzieś bardzo daleko. Np. robisz jakieś czary-mary w warstwie logiki i jak tam trafisz na to swoje TokenExpired to chcesz wylecieć w górę aż do Kontrolera i tam masz zapięty handler który taki wyjątek tłumaczy na odpowiedni kod błędu HTTP i msg. W takiej sytuacji zabawa w przepychanie Either przez X warstw w górę może zwyczajnie nie mieć sensu i niepotrzebnie komplikować kod.
Ale jeśli handling chcesz robić "blisko" np. jeden poziom wyżej, to rzucanie wyjątku, żeby go zaraz złapać, nie ma za bardzo sensu.

CountZero napisał(a):

@jarekr000000:
Mnie zawsze zastanawiało - jak zarządzać tranzakcjami (oczywiście w Springu), zrobić np. rollbacka, gdy zamiast wyjątków używa się Either lub Try? Czy trzeba w takim robić to ręcznie z pomocą np. TransactionTemplate? Jeśli tak, to wydaje się to być mega uciążliwe.

Paradoks polega na tym, że jak używasz Either z @Transactional to się rollbackuje, nawet jak chcesz żeby się nie rollbackowało (bo obsłużyłeś błąd)...

TransactionTemplate uciążliwe?
Zamiast

@Transactional
metoda (..) {
 kod
}

masz

metoda (..) {
doInNewTransaction(() -> {
               kod
 }
}

Dochodzi linijka z klamrą. (Fakt, mamy TransactionTemplate pod tym doInNewTransaction schowany, coby jeszcze 2 linijki ocalić).

Paradoks polega na tym, że jak używasz Either z @Transactional to się rollbackuje, nawet jak chcesz żeby się nie rollbackowało (bo obsłużyłeś błąd).

Co masz na myśli?

Hmm, no dobra, może nie jakoś bardzo uciążliwe, ale za każdym razem wstrzykiwane serwisu i wrapowanie metod w lambdę jest wkurzające.

CountZero napisał(a):

Co masz na myśli?

Transaction marked as rollback only już ileś razy się rozczarowałem.
Niby robię catch. Obsługuje wyjątek JPA (np. typu ConstraintViolationException).
Łapie go sobie i chcę zwrócić Optional lub Either...
A tu jeb - na wyjściu z metody Spring widzi, że transakcja jest oznaczona jako rollback only i rzuca za mnie exceptionem.
Bardzo dziękuję :-)

Typowe, ale wkurza, bo z tego co pamiętam to słabo jest udokumentowane i czasem całkiem niegroźne problemy JPA skutkują wymuszonym rollbackiem, i exceptionem rzuconym z d...

Mam mieszane uczucia co do Either... prawdopodobnie używam go w zły sposób, ale wydaje mi się to bardziej skomplikowane niż powinno... Ktoś spojrzy krytycznym okiem?

 Either<ErrorDTO, ActiveUserDTO> getActiveUser(String sessionID) {
       return authorizationService
               .getAccessToken(UUID.fromString(sessionID))
               .map(this::callMethod)
               .orElse(Either.left(new TokenNotFoundDTO()));
   }

   private Either<RemoteMethodInvokeFailedDTO, ActiveUserDTO> callMethod(OAuth2AccessToken token) {
       try {
           Response response = authorizationService.makeRequest(Verb.GET, Constants.ACTIVE_USER_URL, token);
           return Either.right(new ActiveUserDTO(response.getBody()));
       } catch (Exception e) {
           return Either.left(new RemoteMethodInvokeFailedDTO());
       }
   }

Dla porównania wersja z RuntimeException

ActiveUserDTO getActiveUser(String sessionID) throws RuntimeException {
       OAuth2AccessToken accessToken = authorizationService
               .getAccessToken(UUID.fromString(sessionID))
               .orElseThrow(TokenNotFoundException::new);
       try {
           Response response = authorizationService.makeRequest(Verb.GET, Constants.ACTIVE_USER_URL, accessToken);
           return new ActiveUserDTO(response.getBody());
       } catch (Exception e) {
           throw new RemoteMethodInvokeFailedException(e.getMessage());
       }
   }

Problem masz tu:

try {
            Response response = authorizationService.makeRequest(Verb.GET, Constants.ACTIVE_USER_URL, token);
            return Either.right(new ActiveUserDTO(response.getBody()));
        } catch (Exception e) {
            return Either.left(new RemoteMethodInvokeFailedDTO());
        }

Jakiego rodzaju błedy wyrzuca authorizationService.makeRequest i co możesz z nimi zrobić?
Czy jest możliwośc zmiany tej metody makeRequest?
catch (Exception e) to taki trochę rak (tylko czasami ma sens).

Edit: w zasadzie duży rak

catch (Exception e) {
            throw new RemoteMethodInvokeFailedException(e.getMessage());
        }

Tego typu kod powoduje, że dowolny exception zostanie częściowo zgubiony (stack trace przepadnie i zostanie nam tylko message).
Jak już się tak bawimy exceptionami (łapiemy cokolwiek catch Exception) to warto oryginalny bład przechować.

catch (Exception e) {
            throw new RruntimeException(e); // to nieco mniejszy rak
        }

Shalom napisał(a):

Nie, nie będzie złym, tylko innym. Generalnie załóż że rzucenie wyjątku robisz kiedy handling jest obsługiwany gdzieś bardzo daleko. Np. robisz jakieś czary-mary w warstwie logiki i jak tam trafisz na to swoje TokenExpired to chcesz wylecieć w górę aż do Kontrolera i tam masz zapięty handler który taki wyjątek tłumaczy na odpowiedni kod błędu HTTP i msg. W takiej sytuacji zabawa w przepychanie Either przez X warstw w górę może zwyczajnie nie mieć sensu i niepotrzebnie komplikować kod.

Primo, TokenExpired to powinien być rzucany przed wejściem w logikę biznesową.
Secundo, sporo frameworków i aplikacji ma tendencję do tworzenia głębokich drzew wywołań funkcji. Często stosuje się tego typu schematy:

A method1(params) {
// coś tam
method2(params)
// coś tam
}

B method2(params) {
// coś tam
method3(params)
// coś tam
}

C method3(params) {
// coś tam
method4(params)
// coś tam
}

D method4(params) {
// coś tam
method5(params)
// coś tam
}

Zamiast:

A method1(params) {
// coś tam
B result2 = method2(params)
// coś tam
C result3 = method3(params)
// coś tam
D result4 = method4(params)
// coś tam
}

B method2(params) {
// coś tam
}

C method3(params) {
// coś tam
}

D method4(params) {
// coś tam
}

Nie zawsze da się tak spłaszczyć drzewo wywołań, ale próbować warto. Checked exceptions i przepychanie Eitherów przy rozwlekłej Javowej składni do tego powinny skłaniać.

Właśnie druga rzecz która mnie zastanawia to organizacja tego wewnątrz całego pakietu, gdzie zwrócić Eithera, gdzie rzucić bądź obsłużyć wyjątek.
Na dnie mam "techniczny" AuthorizationService, który przechowuje informację o tokenach i strzela do zewnętrznego API.
Poziom wyżej są bardziej biznesowe serwisy jak np UserService którego kod tu pokazałem.
Dalej fasada - punkt wejścia do całego pakietu, inne paczki wewnątrz aplikacji komunikują się właśnie przez fasadę.
No i controller, RESTowa nakładka na fasade dla niektórych jej usług.

Staram się to napisać korzystając z możliwie jak najlepszych wzorców i praktyk, jednak im dalej w las tym ciemniej :)