gapic-generator-java icon indicating copy to clipboard operation
gapic-generator-java copied to clipboard

HTTP exception mapping missing ALREADY_EXISTS

Open kolea2 opened this issue 1 year ago • 5 comments

On HTTP exceptions, exceptions that should be ALREADY_EXISTS (409) are being propagated as ABORTED (also 409).

This code change seems to have removed the behavior: https://github.com/googleapis/sdk-platform-java/commit/cdf461425812f5392436fcd8b4dc6096347036c9#diff-ecd504e117f5a7eedb2f2df5df3a32ac6bc85e21c870ff702c0de2771b33a21eL125-L129

image

Is there a reason for this, or can we restore this logic?

Thanks!

kolea2 avatar Apr 04 '24 18:04 kolea2

The pull request associated to the commit points to a wrong URL due to repository relocation. The correct pull request is https://github.com/googleapis/gax-java/pull/1570

gax-java surfaces errors in terms of ApiExceptions, which are modeled precisely following the canonical status codes (go/canonical-codes). (That is, there's a one-to-one correspondence between the canonical error codes and the Java exceptions.)

go/canonical-codes does have ALREADY_EXISTS.

@chanseokoh Do you happen to remember why the condition for ALREADY_EXISTS was removed at that time?

suztomo avatar Apr 11 '24 15:04 suztomo

ALREADY_EXISTS (409) are being propagated

409, which is an HTTP response status code, is CONFLICT, not ALREADY_EXISTS: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/409

as ABORTED (also 409).

OTOH, ABORTED is a canonical error code, not an HTTP status code. And its value is 10, not 409.

The conversion mapping between the two code spaces is defined by go/http-canonical-mapping.

However, I just realized that the doc also says

The guiding principle is that a mapping is only defined when it is unequivocally correct. In particular, there may be some circumstances where there is a more appropriate HTTP status code for a canonical code (because HTTP codes are finer-grained), and when you know that is the case it is fine to substitute an alternative code.

, so maybe it's fine to restore the previous best-effort logic to identify ALREADY_EXISTS?

chanseokoh avatar Apr 11 '24 15:04 chanseokoh

But I don't have the expertise on this subject. At least I think the current behavior isn't wrong based on go/http-canonical-mapping.

chanseokoh avatar Apr 11 '24 15:04 chanseokoh

Memo: "It was compatibility testing errors for gRPC and HTTP for Datastore. gRPC throws ALREADY_EXISTS, but I ran into this issue for HTTP (reGAPIC)." (Kristen)

suztomo avatar Apr 23 '24 17:04 suztomo

Discussed offline with @suztomo, I will try to pick this up to fix in future cycles (but if someone needs this sooner, feel free to start work on it 😄 )

kolea2 avatar Apr 23 '24 17:04 kolea2