Skip to content

Fix: Status is missing while creating FirestoreException from ApiException #2107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 9, 2025

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented May 7, 2025

While creating a FirestoeException from an ApiException, it does not set status property, which is required by client side metrics feature.

To be able to correctly collect an operation status(OK, DEADLINE_EXCEEDED,ABORTED...) instead of defaulting it to UNKNOWN, this PR maps StatusCode to Status, based on the 1:1 mapping relationship between enum StatusCode.Code and Status.Code, while creating FirestoreException from an ApiException.

@milaGGL milaGGL requested a review from a team as a code owner May 7, 2025 18:03
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/java-firestore API. labels May 7, 2025
return grpcCode.toStatus();
} catch (IllegalArgumentException e) {
return Status.UNKNOWN.withDescription("Unrecognized StatusCode: " + code);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I use explicit 1:1 mapping instead:

    switch (code) {
      case OK:
        return Status.OK;
      case CANCELLED:
        return Status.CANCELLED;
      case UNKNOWN:
        return Status.UNKNOWN;
      case INVALID_ARGUMENT:
        return Status.INVALID_ARGUMENT;
      case DEADLINE_EXCEEDED:
        return Status.DEADLINE_EXCEEDED;
      case NOT_FOUND:
        return Status.NOT_FOUND;
      case ALREADY_EXISTS:
        return Status.ALREADY_EXISTS;
      case PERMISSION_DENIED:
        return Status.PERMISSION_DENIED;
      case RESOURCE_EXHAUSTED:
        return Status.RESOURCE_EXHAUSTED;
      case FAILED_PRECONDITION:
        return Status.FAILED_PRECONDITION;
      case ABORTED:
        return Status.ABORTED;
      case OUT_OF_RANGE:
        return Status.OUT_OF_RANGE;
      case UNIMPLEMENTED:
        return Status.UNIMPLEMENTED;
      case INTERNAL:
        return Status.INTERNAL;
      case UNAVAILABLE:
        return Status.UNAVAILABLE;
      case DATA_LOSS:
        return Status.DATA_LOSS;
      case UNAUTHENTICATED:
        return Status.UNAUTHENTICATED;
      default:
        return Status.UNKNOWN.withDescription("Unknown StatusCode: " + code);
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StatusCode comes from the com.google.api.gax.rpc package, and Status comes from the io.grpc package. It's unlikely, but possible, that enums change in one release of one package, and not get updated at the same time in the other package, or the names don't match, or that we don't upgrade our dependencies at the same time for both packages, etc. The downside of adding the switch statement is having to upgrade the switch statement every time there's a change in the enums.

I'm leaning towards keeping it the way you have done it, especially because we're not going to keep an eye on the status code changes (for example if a new code is added, we'll forget about adding it here). Without the switch statement things should settle down and continue to work together after both packages are upgraded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense. thank you Ehsan

@cloud-java-bot cloud-java-bot requested a review from a team as a code owner May 7, 2025 18:06
@milaGGL milaGGL requested a review from ehsannas May 8, 2025 15:33
@milaGGL milaGGL added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 8, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 8, 2025
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One nit pointed out below.

return grpcCode.toStatus();
} catch (IllegalArgumentException e) {
return Status.UNKNOWN.withDescription("Unrecognized StatusCode: " + code);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StatusCode comes from the com.google.api.gax.rpc package, and Status comes from the io.grpc package. It's unlikely, but possible, that enums change in one release of one package, and not get updated at the same time in the other package, or the names don't match, or that we don't upgrade our dependencies at the same time for both packages, etc. The downside of adding the switch statement is having to upgrade the switch statement every time there's a change in the enums.

I'm leaning towards keeping it the way you have done it, especially because we're not going to keep an eye on the status code changes (for example if a new code is added, we'll forget about adding it here). Without the switch statement things should settle down and continue to work together after both packages are upgraded.


package com.google.cloud.firestore;

import static org.junit.Assert.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style guide disallows using .* imports.

@milaGGL milaGGL added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2025
@milaGGL milaGGL merged commit 00a6f54 into main May 9, 2025
25 checks passed
@milaGGL milaGGL deleted the mila/fix-missing-Status-in-FirestoreException branch May 9, 2025 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants