ResponseEntity i obsługa błędów

0

Hej
chciałbym się podzielić swoimi przemyśleniami i popytać was o opinie, jak wy to widzicie.
Ostatnio siadłem do tematu controllerów u nas w aplikacji, zastanawiałem się w jaki sposób ładnie obsługiwać błędy.
Projekt został tworzony od zera więc przepchaliśmy Eithera przez wszystkie warstwy aby zapewnić dobrą obsługę błedów.
Na warstwie controllerów dostajemy eithera, jeżeli dostajemy Issue rzucamy wyjątek które jest łapany w ControllerAdvice. Trochę mi się to nie podoba, nie przepadam nad rzucaniem wyjątków i łapaniem go gdzieś wyżej np ApiErrorHandler. Coraz większa ilość @ExceptionHandler zachęciła nas do zmiany obsługi błedów więc postanowiliśmy stworzyć klase pomocniczą :

class SuccessOrError <T, K extends ErrorResponse>>

przykładowa metoda kontrolera

 public ResponseEntity<SuccessOrError<JakaśDataDto, ErrorDto>>

Serializacja też ogarnięta. Fabryka dla ErrorDto przyjmuje w parametrze Issue<?> który pozwala na zbudowanie odpowiedniej odpowiedzi.
Fabryka również ma dostarczoną listę typów Issuesów z których w razie wystąpienia ma wyciągnąć message i zwrócić użytkownikowi.
Np chcemy powiedzieć użytkownikowi że:

{
"type": "CREATE_TEMPLATE_ERROR",
"message": "Nie udało się utworzyć szablonu."
"Caused by": [
 "Boś głupi",
 "Zła wartość dla pola: mahmol",
 "Nierozpoznany atrybut: bbebrre"
]
}

I jeżeli wszystko dobrze to :

{
"template":[...]
}

Sam issue pozwala na wyszukanie root messegów dla zdefiniowanego typu

public interface Issue<T extends Enum<?>>
{
    String getMessage();

    T getIssueType();

    Optional<Exception> getRootCause();

    Set<? extends Issue<?>> getRootIssue();

 default Set<Issue<?>> findRootIssuesByTypes(Set<Enum<?>> issueTypes)

Ogólnie taki sposób mi się podoba, bo Issuesy w core rzucają błąd, nic ich nie obchodzi. Każda warstwa wyżej tworzy swój Issue z informacją czego nie udało się zrobić w jej warstwie wraz z informacją z dołu. Powstaje wtedy graf przeważnie z pojedynczymi wiązaniami (Wiele wiązań w przypadku gdy wiemy że użytkownik podał kilka błędnych atrybutów, zwracamy wtedy informacje o wszystkich złych):

- Nie udało się edytować szablonu *1                 (API)
  -> Nie można wczytać szablonu                       (CORE) 
    -> Wystąpił błąd ładowania atrybutów *2             (CORE)
      -> Nie można odczytać atrybutu dla cośtamcośtam        (CORE)
        -> Błąd odczytania pliku: ścieżka, Plik nie istnieje   (INFRA)

Wtedy na warstwie api możemy decydować którą informację możemy wyświetlić użytkownikowi np *1 i *2 , jednocześnie logująć cały stack i informacje krok po kroku co się wydarzyło.
Co o tym myślicie ? Jakie znacie fajne sposoby obsługi błędów ?

1

Na warstwie controllerów dostajemy eithera, jeżeli dostajemy Issue rzucamy wyjątek które jest łapany w ControllerAdvice.

To trochę tak jakbyś biegł maraton i na ostatniej prostej się przewrócił ;) Jak już się bawicie w Eithery, to obsługę możecie zrobić w pełni manualnie.

W sumie skoro macie serializację własnej klasy, to dlaczego nie napisaliście serializacji dla Eithera? Może już nawet są gotowe adaptery Vavr-Spring MVC. Jaka jest przewaga własnego typu?

Z punktu widzenia klienta (np. UI) chciałbyś wypluć strukturę danych z jakimś userMessage (przemyśleliście i18n?) oraz errorCode, żeby pozapinać warunki. Nie wiem czy takie zbieranie po drodze skutków błędu ma sens - co miałby zrobić UI z takim grafem? Po drugie pewnie w 99% przypadków ludzie i tak będą przerzucać błąd bez dodawania pośredniego kontekstu, wiec zamiast grafu będzie pojedyncza wartość.

To na co bym zwrócił uwagę:

  1. Error cody
  2. i18n
  3. Przemyśleć obsługę od strony klienta
  4. Łatwość użycia z perspektywy usługi - idealnie jakby można było zwrócić z kontrolera po prostu Either. Przykładowo, pusty Optional już jest mapowany przez Springa na 404 - coś w ten deseń
1

Chyba masz na myśli obsługę wyjątków?

Bo możesz zwrócić błąd klientowi, nawet jak nie poleciał żaden wyjątek; i możesz też nie zwrócić błędu nawet jesli poleciał.

0

@TomRiddle: Nieee, aktualnie zawsze rzucamy wyjątek w kontrolerze. Dlatego chcemy zastąpić rzucanie wyjątków, zwykłym budowaniem odpowiedzi

1

Jak masz Either to możesz przecież zrobić mapLeft i mapRight to ResponseEntity<?>

0

Ja chyba nie bardzo rozumiem ten pomysł z rzucaniem wyjątku na koniec. Czemu nie jakieś:

    public static <T, E extends SomeDomainError> ResponseEntity<?> convert(Either<E, T> result) {
        return result.fold(
             e -> ResponseEntity.status(e.getErrorCode()).body(new SomeErrorResponse(e.getMessage())),
             body -> ResponseEntity.ok(body)
        );
    }
0

@Shalom: Rzucanie wyjątku wymyślone było na samym początku, dlatego chcemy to zmienić. Ten przykład co dałeś jest spoko. Tylko dochodzi jeszcze ustalenie odpowiedniego errorCodu i messega który nie jest tylko przekazywany z najwyższej warstwy tzn Controller z endpointem Post /template nie może utworzyć szablonu, no właśnie z jakiego powodu ? Dostaje Issue z warstwy niżej, więc tworzy swoje issue z message: "Uhu uhu nie mogę utworzyć szablony" typ:"CREATE_TEMPLATE_ERROR" rootIssue:"Issue z niższej warstwy" , w tej sytuacji aby wydedukować co się stało i jaki error code zwrócić, musimy przejrzeć cały stack issuesa(Interface wyżej). Przydał by się jakiś resolver strategii, i każda strategia sprawdza czy rozpoznaje jakieś issuesy w grafie i jeżeli rozpoznaje to zajmie się tworzeniem ErrorDto z odpowiednim statusem i messagem.
Przykłady:

Strategia dla  errorCode 404  (* zwracamy message na front)

- Nie udało się stworzyć szablonu *                (API)
  -> Wystąpił błąd ładowania atrybutów              (CORE)
    -> Nie można odczytać atrybutu dla cośtamcośtam  *      (CORE)
      -> Błąd odczytania pliku: ścieżka, Plik nie istnieje   (INFRA)

Strategia dla errorCode 400 (* zwracamy message na front)

- Nie udało się stworzyć szablonu *                (API)
  -> Wystąpił błąd ładowania atrybutów              (CORE)
    -> Błąd rozpoznania atrybutu nr1 *      (CORE)

Strategia dla errorCode 422 (* zwracamy message na front)

- Nie udało się stworzyć szablonu *                (API)
  -> Wystąpił błąd ładowania atrybutów              (CORE)
    -> Dla takiego szablonu atrybut jest niedostępny *      (CORE)
0

Wygodniej gdzieś w bebechach rzucić wyjątek NotFoundException, złapać go w controllerAdvice i zwrócić 404 :D

0

Przyznam ze nie bardzo rozumiem po co chcesz takie dziwne rzeczy robić. Error jest istotny dla frontu i tyle. Nie wiem co ty chcesz robić w tej swojej strategii za bardzo. Jak dla mnie to mylisz poziom abstrakcji i kwestię logowania błędów z wyświetlaniem userowi odpowiedniej informacji. Usera w ogóle nie obchodzi ze error jest dlatego ze query do bazy sie popsuło. To kod domenowy powinien sterować tym co leci jako Left, a nie że budujesz jakiś wielki graf a potem jakaś złożona logika robi analizę.

Więc po prostu gdzieśtam w kodzie jest jakieś peekLeft() które loguje error warstwy niżej a następnie mapLeft() które zamienia jakiś niskopoziomowy error na error domenowy który dostanie user. I robisz to w takim miejscu gdzie wiadomo co zwrócić, czyli zamiast twojego gdzieś w bebechach rzucić wyjątek NotFoundException robisz po prostu return Either.Left(new SomeError(418, "I'm a teapot!")). Gdzie ty widzisz jakąś różnicę? Może tak być że dany error nie wie co powinno polecieć w górę, ale to też nie problem, bo warstwa która wie zrobi sobie mapLeft() i ustawi co potrzebuje.

0

Usera w ogóle nie obchodzi ze error jest dlatego ze query do bazy sie popsuło

no zgadza się (nie wspominałem że usera takie informacje interesują) a nawet jeśli, to core nie powinien o tym decydować. Api powinno decydować co zwrócić użytkownikowi, jeżeli walidacja inputu użytkownika jest weryfikowana nisko np w warstwie core(wynika to z domeny), kilka warstw wyżej tak na strzała nie można powiedzieć czy to walidacja się nie udała czy połączenie do bazy danych. Ale idąc po stacku issuesów można to stwierdzić.
Dla błędu walidacji zwrócić info dla usera bez logów w pliku(bo to błąd użytkownika) albo że baza danych leży i wtedy leci spam do loggów a user dostaje info się nie udało stworzyć szablonu.

a nie że budujesz jakiś wielki graf a potem jakaś złożona logika robi analizę.

Nie buduję wielkiego grafu, tyle ile jest warstw taki może być ciąg wiązań.

a potem jakaś złożona logika robi analizę.

class StrategiaMapingu implement StrategiaObsługiIsuesa{

public ErrorDto prepareErrorDto(ApiIssue<?> issue){
  return ErrorDto.builder()
    .type(issue.getType())
    .message(issue.getIssue())
    .causeBy(issue.findRootIssuesByTypes(Set.of(ZCORA.TO_ATTRIBUTE_MAPPING_ISSUE, ZCORA.TO_GENERIC_MAPPING_ISSUE)))
}

 public boolean canHandle(Issue<?> issue){
  return  ! issue.findRootIssuesByTypes(Set.of(ZCORA.TO_ATTRIBUTE_MAPPING_ISSUE, ZCORA.TO_GENERIC_MAPPING_ISSUE)).isEmpty()
}
}

"gdzieś w bebechach rzucić wyjątek NotFoundException" - to był nieśmieszny joke.

I robisz to w takim miejscu gdzie wiadomo co zwrócić, czyli zamiast twojego gdzieś w bebechach rzucić wyjątek NotFoundException robisz po prostu return Either.Left(new SomeError(418, "I'm a teapot!")). Gdzie ty widzisz jakąś różnicę?

No właśnie nie zawsze wiadomo co zwrócić, nie zrobisz Either.Left(new SomeError(418, "I'm a teapot!")) . W warstwie o jeden niżej poleciał błąd, nie oznacza że błąd wynika z jej warstwy, nie wiesz co poleciało, czy DatabaseIssue 10 warstw niżej czy ValidationIssue 3 warstwy niżej. Może powinno polecieć 500 a może 400

0
Shalom napisał(a):

robisz po prostu return Either.Left(new SomeError(418, "I'm a teapot!")).

Nie wiem czy dobrze zrozumiałem, ale sugerujesz, żeby zrobić to w ten sposób? Żeby inne warstwy wiedziały, że błąd będzie propagowany do HTTP i dlatego potrzebuje statusu? Ten błąd powinien być interpretowany na poziomie API (np. REST) i tam zamieniany na odpowiedni status i message.

1

@VeloxDigitis chodzi mi o to, żeby dana warstwa zwracała w górę odpowiedni error, a nie pchała jakiś niskopoziomowy śmieć. Więc np. warstwa domeny woła jakieś repository.saveCośtam i się wywaliło, to nie pchamy dalej jakiegoś SQLException bo akurat nasze repository wrappuje bazę danych, tylko robimy sobie peekLeft(), logujemy co tam przyszło a potem robimy mapLeft() i zwracamy wyżej jakiś bardziej domenowy błąd, który to znów warstwa wyżej będzie umiała zinterpretować. W efekcie wiele różnych niskpoziomowych błędów może wychodzić z domeny jako ten sam typ błędu.
W domenie pewnie nie chcesz mieć http error code, ale nikt nie broni ci mieć hierarchii jakichś DomainError gdzie będzie ich kilka typów, albo mieć jakiegoś enuma który określa czy błąd jest w danych które przyszły, czy może infra nam sie wywaliła.

Dla mnie pomysł żeby pchać jako error cały stack (czyli de facto pchać tego niskopoziomowego śmiecia, bo przecież cała reszta z niego wynika) a potem nagle gdzieś na poziomie API pisać jakąś złożoną logikę która na podstawie tego stacku rozkmini co to za error, jest strasznie dziwne i wróżę że skończy się niewyobrażalnym spaghetti w tym twoim StrategiaMapingu. Bo na dobrą sprawę będziesz tam robić jakiś mirror logiki domenowej.

1 użytkowników online, w tym zalogowanych: 0, gości: 1