Core: Improve token exchange handling when token expires

[nastra]

No description provided.

[rdblue]

I think that @Immutable is being overused for cases like this. This class isn't complex and using it forces a lot of logic to go into default methods and the of static method. While it is useful in some cases, I think that there should be a significant savings from using immutables; we should not use them by default.

[rdblue]

I think this should be rewritten to remove @Immutable.

[nastra]

I understand your argument about potentially over-using @Immutable. However, I think there's rarely a case where the non-@Immutable version is better than the @Immutable one. In fact I initially put the JWT parsing code first into a method, then extracted it into a non-@Immutable class like the one below and eventually made it @Immutable.
The non-immutable version of that class will be longer due to boiler-plate code. Technically the constructor might even have a precondition null check for the token (which we don't need in this particular example, because we already know the token is non-null from the static method). In the @Immutable we get this stuff for free because the API definition is explicit (aka the token isn't marked as optional or nullable and therefore must always be non-null).

public class NonImmutableJwt {

  private final String token;
  private final long expiresAtEpochMillis;

  private Jwt(String token, long expiresAtEpochMillis) {
    this.token = token;
    this.expiresAtEpochMillis = expiresAtEpochMillis;
  }

  public String token() {
    return token;
  }

  public long expiresAtEpochMillis() {
    return expiresAtEpochMillis;
  }

  public long expiresInMillis() {
    return expiresAtEpochMillis() - Instant.now().toEpochMilli();
  }

  public boolean isExpired() {
    return expiresAtEpochMillis() <= Instant.now().toEpochMilli();
  }

  static Optional<Jwt> of(String token) {
    try {
      Preconditions.checkArgument(null != token, "Invalid JWT: null");
      String[] parts = token.split("\\.");
      Preconditions.checkArgument(parts.length == 3, "Invalid JWT: %s", token);

      // Parse the payload JSON
      JsonNode jsonNode = JsonUtil.mapper().readTree(Base64.getUrlDecoder().decode(parts[1]));
      Long expiresAt = JsonUtil.getLongOrNull("exp", jsonNode);
      return Optional.of(
          new Jwt(
              token,
              null == expiresAt
                  ? Long.MAX_VALUE
                  : Instant.ofEpochSecond(expiresAt).toEpochMilli()));
    } catch (Exception e) {
      return Optional.empty();
    }
  }
}

If you're strong on this one, then I can change this to a non-@Immutable version but as I've tried to outline above, I don't think there's any advantage in doing so.

[PraveenNanda124]

Looks good

[rdblue]

@nastra, I opened a PR against your branch with the remaining changes I think we should make. Please take a look: nastra#72

[nastra]

@nastra, I opened a PR against your branch with the remaining changes I think we should make. Please take a look: nastra#72

Thanks @rdblue I've applied your changes and cleaned up some tiny bits to make CI pass. I've also made sure that the changes here are compatible for #6169.

[rdblue]

The reason why I didn't add a method like this is that it isn't correct when expiresAtMillis is null. It returns false, but the token may still be expired. I think it's misleading to have a method with this name, so I think we should revert the change to add this.

[nastra]

thanks for bringing this up. I'll look into that as part of #6562

[rdblue]

I think that we need to fix https://github.com/apache/iceberg/pull/6489/files#r1072451113 in a follow-up, but that's minor so I'm going to merge this to unblock the next steps. Thanks, @nastra!