ComputeEngineCredentials incorrectly throws RuntimeException

[elharo]

Unless we really think getDefaultServiceAccount can't fail here, we should not convert the IOException to a RuntimeException.

  @Override
  public String getAccount() {
    if (serviceAccountEmail == null) {
      try {
        serviceAccountEmail = getDefaultServiceAccount();
      } catch (IOException ex) {
        throw new RuntimeException("Failed to to get service account", ex);
      }
    }
    return serviceAccountEmail;
}

[elharo]

getAccount() is called by sign, which throws SigningException but does not catch this RuntimeException! SigningException is also a runtime exception and shouldn't be.

[elharo]

Since this library is pre-1.0 we're allowed to change the API. We should make SigningException (and possibly others) checked since they can fail unexpectedly due to problems external to the program itself, and therefore client code should write handlers.

Also, getAccount should throw IOException.

[elharo]

AppEngineCredential creates its service account when the object is created and does properly throw IOException if this fails. That's probably what we should do here.

[elharo]

serviceAccountEmail is transient:

  private transient String serviceAccountEmail;

so if it's serialized it will be looked up again when needed. Can it have a different value then?

[chingor13]

If we make SigningException a checked exception, we will require anyone who is using credentials.sign() to add exception handling.

Amongst Google libraries, I only see one use (albeit an important one) https://github.com/googleapis/java-storage/blob/6f172eba6fd6e9c11a1f49569249ea6e714ea91f/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java#L781

[elharo]

That is a good thing and is why we should make this a checked exception.

Everyone now has to handle an unexpected runtime exception. In most cases they're not so they're shipping buggy, unreliable code that fails unexpectedly.

The alternative is to figure out how to make getDefaultServiceAccount or getAccount not fail. That is, what it can do that is correct behavior instead of throwing a runtime exception? But if there is no such behavior it needs to be a checked exception.

[JustinBeckwith]

Downgrading this to a P2 since it's not blocking any critical functionality at this stage. If that should change, let me know :)

[danoscarmike]

Also adjusting this to feature request. It is not clear whether we can make this change given the breaking nature. Adding to backlog for discussion and prioritization.

[elharo]

This library is pre-1.0. We should be able to make this change.

[silvolu]

@elharo technically yes, but I chatted with Alan and Justin and we should consider this library common law GA and avoid breaking changes. We'll bump to 1.0 soon.

[elharo]

I think we're going to regret that decision, and it is going to cause problems for customers. How significant I don't know, but ignoring failure modes is not a path to reliable systems.