Metody serwisów zwracające ResponseEntity

Hej,

piszę sobie backend RESTowy do pewnej aplikacji. Żeby uniknąć wrzucania logiki w kontrolerach metody serwisów mają typ zwracany np. ResponseEntity<Something> w przypadku metod GET, lub ResponseEntity<?> w przypadku POST, PATCH itd. Zrobiłem to w taki sposób, ponieważ nie chciałem sprawdzać, czy Optional z obiektem jest pusty w kontrolerze, tylko w serwisie.

Fajnie to wygląda, bo dzięki temu metoda dodawania w serwisie wygląda tak:

public ResponseEntity<?> addNewDiskType(DiskType diskType){
        Set<ConstraintViolation<DiskType>> validationErrors = validator.validate(diskType);
        if(!validationErrors.isEmpty())
            return new ResponseEntity<>(HttpStatus.BAD_REQUEST);
        if(getDiskTypeByName(diskType.getDiskType()).isPresent())
            return new ResponseEntity<>(HttpStatus.CONFLICT);

        diskTypeRepository.save(diskType);
        return new ResponseEntity<>(HttpStatus.CREATED);
    }

natomiast kontroler jest treściwy i schludny:

@PostMapping(consumes = MediaType.APPLICATION_JSON_VALUE)
    public ResponseEntity<?> addDiskType(@RequestBody DiskType diskType){
        return diskTypeService.addNewDiskType(diskType);
    }

Jednak nauczyłem się przedwczoraj testowania kodu i te testy dziwnie wyglądają. Boję się, że będę miał trudność z wyciąganiem np. ilości obiektów z obiektu ResponseEntity itd. Spójrzcie jak wyglądają moje testy:

assertEquals(HttpStatus.OK,diskTypeService.getResponseWithDisk(1L).getStatusCode());
        assertEquals(new ResponseEntity(HttpStatus.NOT_FOUND), diskTypeService.getResponseWithDisk(3L));
        assertEquals(new ResponseEntity(HttpStatus.CREATED), diskTypeService.addNewDiskType(testDisk));
        assertEquals(new ResponseEntity(HttpStatus.CONFLICT), diskTypeService.addNewDiskType(testDisk2));

        assertEquals(new ResponseEntity(HttpStatus.BAD_REQUEST), diskTypeService.addNewDiskType(testDisk3));

Potrzebuje wskazówki, czy wycofywać się z takiego projektowania backendu? Łatwiej mi będzie zrefaktoryzować kod teraz, niż później :) Z góry dzięki za pomoc i pozdrawiam Was w ten piątkowy poranek.

To co mi się nie podoba, to że w serwisie, który powinien zajmować się logiką biznesową robisz/zwracasz rzeczy, które są zależne od konkretnego frameworka HTTP. Serwis powinien zwracać obiekt domenowy (lub błąd), a właśnie kontroler niech zajmie się tłumaczeniem tego co dostał z serwisu na język HTTP. Jeśli piszesz swoje serwisy, to od razu zakładaj, że możesz mieć różne rodzaje adapterów (serwer HTTP, GRPC, JMS itd) i nie powinno mieć znaczenia jaki konkretnie jest to adapter - logika biznesowa powinna móc bez zmian pracować z każdym rodzajem. Jest to krok w stronę "clean architecture", gdzie warstwa wewnętrzna (logika biznesowa) nie ma żadnych dependencji na warstwę zewnętrzną (adapter), a strzałki biegną "do środka"

Według mnie kontroler to właśnie miejsce które powinno zajmować się mapowaniem wyników logiki aplikacji na odpowiednie obiekty do http. W Twoim przypadku, service mógłby zwracać jakąś informacje o powodzeniu bądź nie (i powód) jakieś akcji i to kontroler w tego zrobi odpowiednie ResponseEntity

Wtedy w teście sprawdzasz po prostu co wróciła metoda (czy to jakieś enum czy Either czy cokolwiek)

Warstwa serwisowa nie powinna zwracać statusów HTTP - od tego jest kontroler. Zwracaj z serwisu Optionala, w kontrolerze sprawdź czy jest pusty i zwróć odpowiedni status.

Okej, refaktoryzuje zatem projekt.
W przypadku GET-ów będę sobie sprawdzał zawartość Optionala i na tej podstawie w kontrolerze zwrócę odpowiednią odpowiedź na front. Co jednak w przypadku POSTów, PATCH, itd?
Czy dobrą praktyką będzie zrobienie pomocniczego Enuma i zwracanie jakiejś jego wartości do kontrolera, a w kontrolerze sprawdzanie co się zwróciło i generowanie odpowiedniej RepsonseEntity?

Może być enum, może być Either może być dowolny dwój obiekt. Tak żeby Ci było wygodnie potem to obsłużyć.

Okej, wyszło mi coś takiego:

HttpStatusEnum status = diskTypeService.addNewDiskType(diskType);
        if(status == HttpStatusEnum.CREATED)
            return new ResponseEntity<>(HttpStatus.CREATED);

        if(status == HttpStatusEnum.BADREQUEST)
            return new ResponseEntity<>(HttpStatus.BAD_REQUEST);

        if(status == HttpStatusEnum.CONFLICT)
            return new ResponseEntity<>(HttpStatus.CONFLICT);

Nie wygląda to najzgrabniej, więc domyślam się, że da się to zrobić lepiej. Jeżeli nie, to tak to zostawię. Jeśli jednak ma ktoś ciekawą wskazówkę, to chętnie ją przyjmę :)

U mnie robię coś w tym stylu. Może Ci się przyda ;)

danek napisał(a):

U mnie robię coś w tym stylu. Może Ci się przyda ;)

Zgrabnie zrobione, muszę w końcu zgłębić wiedzę o Vavr, bo się naprawdę przydaje. Póki co jednak, żeby mieć szybko jakieś rozwiązanie zrobiłem to w taki sposób:

public enum HttpStatusEnum {
    OK, CONFLICT, BADREQUEST, NOTFOUND, CREATED;

    public static ResponseEntity<?> isHttpStatusEquals(HttpStatusEnum status){
        if(status == HttpStatusEnum.BADREQUEST)
            return new ResponseEntity<>(HttpStatus.BAD_REQUEST);

        if(status == HttpStatusEnum.CONFLICT)
            return new ResponseEntity<>(HttpStatus.CONFLICT);

        return new ResponseEntity<>(HttpStatus.CREATED);
    }
}

W enumie statyczna funkcja, do której przekazuje status zwrócony w serwisie. W razie czego zmieniam tylko w tym miejscu, a powyższa funkcja wrzucona jest do niektórych metod kontrolera.

A czemu to enum nie może mieć pola typu HttpStatus? ;)