Fail-fast - jak podejść do tematu walidacji?

Cześć,
czytam właśnie o podejściu Fail Fast i chcę je wdrożyć w swoim pet projekcie. Postanowiłem to zrobić na przykładzie - Integracja z Spotify API w celu uzyskania tokenu dostępu.

Po pomyślnej weryfikacji Spotify API zwróci response, w którym interesuję mnie:

• access token,
• refresh token,
• scope.

Refresh token nie ma expiration time, dlatego zdecydowałem się przechowywać go w Redis. Potrzebuję klucza identyfikującego użytkownika, wybór padł na ID które będzie pobierane z JWT tokena, który z kolei zostanie odczytany z cookie.

Tu kontroler który to robi:
Jak token będzie null, adnotacja @CookieValue sama rzuci wyjątek. Postanowiłem jeszcze dodać walidację, która sprawdzi, czy token nie jest pusty, no bo po co kontynuować pracę z pustym tokenem.

@RestController
@RequestMapping("/api/v1/spotify/auth")
public class SpotifyCallbackController {

    private final SpotifyAccessTokenHandler tokenHandler;
    private final CookieUtils cookieUtils;

    public SpotifyCallbackController(SpotifyAccessTokenHandler tokenHandler,
                                     CookieUtils cookieUtils) {
        this.tokenHandler = tokenHandler;
        this.cookieUtils = cookieUtils;
    }

    @GetMapping("/callback")
    public ResponseEntity<ApiResponse> handleSpotifyCallback(@RequestParam("code") String authCode,
                                                             @CookieValue(name = "jwtToken") String jwtToken) {

        // dodana walidacja
        if (jwtToken.isEmpty()) {
            throw new IllegalArgumentException("Jwt is empty");
        }

        String accessToken = tokenHandler.retrieveSpotifyAccessToken(authCode, jwtToken);

        ResponseCookie cookie = cookieUtils.generateCookie("spotifyAccessToken", accessToken, "/api/v1/spotify", 3600);

        return ResponseEntity.ok()
                .header(HttpHeaders.SET_COOKIE, cookie.toString())
                .body(new ApiResponse("Spotify authorization successful."));
    }
}

Kolejna klasa handler - zawiera komentarze z numerem walidacji, ich opis znajduję się pod kodem.

@Service
public class SpotifyAccessTokenHandler {
    private final String redirectUri;
    private final JwtTokenManager jwtTokenManager;
    private final RedisTokenStore redisTokenStore;
    private final SpotifyTokenApi spotifyTokenApi;

    public SpotifyAccessTokenHandler(@Value("${REDIRECT_URI}") String redirectUri,
                                     JwtTokenManager jwtTokenManager,
                                     RedisTokenStore redisTokenStore,
                                     SpotifyTokenApi spotifyTokenApi) {
        this.redirectUri = redirectUri;
        this.jwtTokenManager = jwtTokenManager;
        this.redisTokenStore = redisTokenStore;
        this.spotifyTokenApi = spotifyTokenApi;
    }

    public String retrieveSpotifyAccessToken(String authCode, String jwtToken) {
        // 1. walidacja id
        Integer userId = extractAndValidateUserId(jwtToken);

        // 2. walidacja requestu i responsu wewnątrz dto
        MultiValueMap<String, String> formData = new SpotifyAccessTokenRequest(
                "authorization_code", authCode, redirectUri).toMultiValueMap();
        SpotifyTokenResponse response = spotifyTokenApi.sendTokenRequest(formData);

        redisTokenStore.saveRefreshToken(userId, response.refreshToken());

        return response.accessToken();
    }

    private Integer extractAndValidateUserId(String jwtToken) {
        Integer userId = jwtTokenManager.extractUserId(jwtToken);
        if (userId == null || userId <= 0) {
            throw new IllegalArgumentException("User id is null or invalid");
        }
        return userId;
    }
}

1. Zapisuję refresh token w Redis i potrzebuję klucza identyfikującego - id, uznałem że na samym początku zwaliduję id. No bo po co dalej kontynuować pracę jak coś z nim jest nie tak - nie będę mógł przypisać do użytkownika refresh tokenu.
2. Tworzę formularz przy użyciu SpotifyAccessTokenRequest i dodaję prostą walidację na null i empty w konstruktorze.

public record SpotifyAccessTokenRequest(@JsonProperty("grant_type") String grantType,
                                        @JsonProperty("code") String authCode,
                                        @JsonProperty("redirect_uri") String redirectUri) {

    public SpotifyAccessTokenRequest {
        if (grantType == null || grantType.isEmpty()) {
            throw new IllegalArgumentException("Grant type cannot be null or empty");
        }
        if (authCode == null || authCode.isEmpty()) {
            throw new IllegalArgumentException("Authorization code annot be null or empty");
        }
        if (redirectUri == null || redirectUri.isEmpty()) {
            throw new IllegalArgumentException("Redirect uri cannot be null or empty");
         }
    }

    public MultiValueMap<String, String> toMultiValueMap() {
        //
    }
}

W SpotifyTokenResponse w konstruktorze waliduję czy zwracane wartości nie są null i empty.

public record SpotifyTokenResponse(@JsonProperty("access_token") String accessToken,
                                   @JsonProperty("refresh_token") String refreshToken,
                                   @JsonProperty("scope") String scope) {

    public SpotifyTokenResponse {
        if (accessToken == null || accessToken.isEmpty()) {
            throw new IllegalArgumentException("Access token cannot be null or empty");
        }
        if (refreshToken == null || refreshToken.isEmpty()) {
            throw new IllegalArgumentException("Refresh token cannot be null or empty");
        }
        if (scope == null || scope.isEmpty()) {
            throw new IllegalArgumentException("Scope cannot be null or empty");
        }
     }
}

Klasa odpowiadająca za wysyłanie requestu do API:
Sprawdzam czy bodyData nie jest pusty.

@Service
@Slf4j
public class SpotifyTokenApi {
    private final String spotifyClientId;
    private final String spotifyClientSecret;
    private final WebClient webClient;

    public SpotifyTokenApi(@Value("${SPOTIFY_CLIENT_ID}") String spotifyClientId,
                           @Value("${SPOTIFY_CLIENT_SECRET}") String spotifyClientSecret,
                           @Qualifier("spotifyWebClient") WebClient webClient) {
        this.spotifyClientId = spotifyClientId;
        this.spotifyClientSecret = spotifyClientSecret;
        this.webClient = webClient;
    }

    public SpotifyTokenResponse sendTokenRequest(MultiValueMap<String, String> bodyData) {
        //walidacja
        if (bodyData == null || bodyData.isEmpty()) {
            throw new IllegalArgumentException("Request body data cannot be null or empty");
        }
        // kod
    }
}

Skoro API przy requescie oczekuję konkretnych kluczy, może powinienem je zwalidować zamiast tak ogólnie bodyData?
Dodałbym takie coś:

     if (!bodyData.containsKey("grant_type") || bodyData.getFirst("grant_type") == null || bodyData.getFirst("grant_type").isEmpty()) {
         throw new IllegalArgumentException("Missing or empty 'grant_type'");
        }
        if (!bodyData.containsKey("code") || bodyData.getFirst("code") == null || bodyData.getFirst("code").isEmpty()) {
            throw new IllegalArgumentException("Missing or empty 'code'");
        }
        if (!bodyData.containsKey("redirect_uri") || bodyData.getFirst("redirect_uri") == null || bodyData.getFirst("redirect_uri").isEmpty()) {
            throw new IllegalArgumentException("Missing or empty 'redirect_uri'");
        }

Czy to co napisałem na sens? Co myślicie?
Z góry dziękuję za pomoc.

Konceptualnie tak - o to chodzi, aby sprawdzać czy stan systemu/obiektu/czegokolwiek jest sensowny zanim się zacznie na nim operować, a jeżeli nie, to odpowiednio to obsłużyć jak najszybciej się da.

Niestety "programowanie wyjątkami" sprawia że tutaj ta obsługa błędu jest ukryta z perspektywy callera, ale to już inny temat.

Usunąłem poprzednią odpowiedź. Ja bym zrobił teraz tak:

MultiValueMap<String, String> bodyData

zamienił na konkretną klasę z 3 ma polami.

Klasa powinna być tak zrobiona że w ctor waliduje że są dane.

Mapa jest tworzona w SpotifyTokenApi z tej klasy (nie trzeba już go walidować bo dane w klasie były poprawne).

Należy unikać stringozy gdzie po stringach wyciągasz dane z mapy lub struktury JSON.

Należy się upewnić że API poprawnie tłumaczy wyjątki na jakiś spójny format typu { errorCode: 4848, errorMessage: 'not working' }

WeiXiao napisał(a):

Niestety "programowanie wyjątkami" sprawia że tutaj ta obsługa błędu jest ukryta z perspektywy callera, ale to już inny temat.

Widziałem, że wyżej w komentarzu pisałeś o Result. Trochę o tym poczytałem, podejście dość ciekawe. Zacznę je chyba wdrażać u siebie w projekcie.

99xmarcin napisał(a):

Usunąłem poprzednią odpowiedź. Ja bym zrobił teraz tak:

MultiValueMap<String, String> bodyData

zamienił na konkretną klasę z 3 ma polami.

Klasa powinna być tak zrobiona że w ctor waliduje że są dane.

[WeiXiao napisał(a)]

a jak z ctora zwrócisz Resulta? no chyba nie chcesz rzucać wyjątku

A gdybym w tej klasie stworzył metodę fabryczną create, która będzie walidowała dane. Dobre rozwiązanie?

public class SpotifyAccessTokenRequest {
    private final String grantType;
    private final String authCode;
    private final String redirectUri;

    private SpotifyAccessTokenRequest(@JsonProperty("grant_type") String grantType, 
                                      @JsonProperty("code") String authCode, 
                                      @JsonProperty("redirect_uri") String redirectUri) {
        this.grantType = grantType;
        this.authCode = authCode;
        this.redirectUri = redirectUri;
    }
    
    public static Result<SpotifyAccessTokenRequest> create(String grantType, String authCode, String redirectUri) {
        if (grantType == null || grantType.isEmpty()) {
            return Result.failure("Grant type cannot be null or empty");
        }
        if (authCode == null || authCode.isEmpty()) {
            return Result.failure("Authorization code cannot be null or empty");
        }
        if (redirectUri == null || redirectUri.isEmpty()) {
            return Result.failure("Redirect URI cannot be null or empty");
        }
        return Result.success(new SpotifyAccessTokenRequest(grantType, authCode, redirectUri));
    }
    
    public MultiValueMap<String, String> toMultiValueMap() {
        MultiValueMap<String, String> map = new LinkedMultiValueMap<>();
        map.add("grant_type", this.grantType);
        map.add("code", this.authCode);
        map.add("redirect_uri", this.redirectUri);
        return map;
    }
}

Ornstein napisał(a):

WeiXiao napisał(a):

public static Result<SpotifyAccessTokenRequest> create(String grantType, String authCode, String redirectUri) {
    if (grantType == null || grantType.isEmpty()) {
        return Result.failure("Grant type cannot be null or empty");
    }
    if (authCode == null || authCode.isEmpty()) {
        return Result.failure("Authorization code cannot be null or empty");
    }
    if (redirectUri == null || redirectUri.isEmpty()) {
        return Result.failure("Redirect URI cannot be null or empty");
    }
    return Result.success(new SpotifyAccessTokenRequest(grantType, authCode, redirectUri));
}

Dobre rozwiązanie?

Jak na moje to tak, ja jestem fanem builderów+resultów, bo wtedy obsługa błędów jest przyjemniejsza i bardziej przewidywalna, a to przecież znacząca część każdego softu.

Żadnych dzikich skoków za pomocą wyjątków (programowania wyjątkami) itd

Dla mnie ta logika w SpotifyAccessTokenRequest wygląda dziwnie, ja bym chyba wolał mieć osobną klasę walidującą do tego np SpotifyAccessTokenValidator.
DTO to jest worek na dane czyli struktura danych bez zbędnej logiki czasem też do testów przydaje się tworzenie błędnych requestów. Zależy też czy chcesz wszystko walidować ręcznie czy część walidacji ogarnie Ci framework.