Skip to content
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

fix: update exception mapping on HTTP error responses #1570

merged 5 commits into from Dec 2, 2021


Copy link

@chanseokoh chanseokoh commented Nov 22, 2021

The HTTP-to-Canonical code conversion (HttpJsonStatusCode.httpStatusToStatusCode()) is used only for error situations where a server returns a non-2xx HTTP status code. 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.) That's why and the only reason we need to interpret HTTP codes as canonical codes.

The previous implementation wasn't following the standardized mapping (go/http-canonical-mapping). Now we are trying to make this consistent across all languages. Therefore, this does introduce a breaking change in terms of which HTTP server response code is surfaced as which Java exception.

However, for the conventional commit message and the PR title, I'm not marking as a breaking change, as gax-httpjson hasn't GA'ed.



The locations where the HTTP-to-canonical conversion takes place:

    public void onFailure(Throwable throwable) {
      if (throwable instanceof HttpResponseException) {
        HttpResponseException e = (HttpResponseException) throwable;
        // HttpResponseException.getStatusCode() returns HTTP code
        StatusCode statusCode = HttpJsonStatusCode.of(e.getStatusCode());
        boolean canRetry = retryableCodes.contains(statusCode.getCode());
        String message = e.getStatusMessage();
        ApiException newException =
            message == null
                ? ApiExceptionFactory.createException(throwable, statusCode, canRetry)
                : ApiExceptionFactory.createException(message, throwable, statusCode, canRetry);
  • HttpJsonOperationSnapshot.Builder.setError(): the builder takes an HTTP code. Note there's no usage of setError() within gax-java. It is supposed to take HTTP code, but for some legacy reason, it treats 0 as a special case to indicate success (which is not ideal).
    public Builder setError(int httpStatus, String errorMessage) {
      this.errorCode =
          httpStatus == 0 ? HttpJsonStatusCode.of(Code.OK) : HttpJsonStatusCode.of(httpStatus);
      this.errorMessage = errorMessage;
      return this;

chanseokoh added 2 commits Nov 22, 2021
Interprets HTTP error reponse codes as defined by the canonical error code mapping. Not marking as breaking changes as it affects only httpjson that is not GA-ed.
@google-cla google-cla bot added the cla: yes label Nov 22, 2021
@chanseokoh chanseokoh marked this pull request as ready for review Nov 22, 2021
@chanseokoh chanseokoh requested review from as code owners Nov 22, 2021
@chanseokoh chanseokoh requested review from vam-google and meltsufin Nov 22, 2021
Copy link
Contributor Author

@chanseokoh chanseokoh commented Nov 22, 2021

@meltsufin @vam-google ready for review. Check out the PR description for details.

Copy link

@meltsufin meltsufin left a comment


Copy link

@vam-google vam-google left a comment

LGTM, but we need to test it with java-compute properly before GA, as it may affect its behavior and retries in particular.

return Code.UNKNOWN;
Copy link

@vam-google vam-google Nov 29, 2021

Choose a reason for hiding this comment

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

Is mapping 300s to unknown intentional and canonical behavior here?

Copy link
Contributor Author

@chanseokoh chanseokoh Nov 30, 2021

Choose a reason for hiding this comment

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

Yes, All 3xx's are mapped to UNKNOWN according to go/http-canonical-mapping.

Copy link
Contributor Author

@chanseokoh chanseokoh commented Dec 2, 2021

As mentioned in b/205195666, I have conducted multiple tests for this change, whose results seem all good. @vam-google merging this now, but if you want me to do some other tests, just let me know.

@chanseokoh chanseokoh merged commit 8a170d1 into main Dec 2, 2021
6 checks passed
@chanseokoh chanseokoh deleted the canonical-code branch Dec 2, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue Dec 2, 2021
🤖 I have created a release \*beep\* \*boop\*
### [2.7.1]( (2021-12-02)

### Bug Fixes

* fix gRPC code conversion ([#1555]( ([09b99d5](
* pass error message when creating ApiException ([#1556]( ([918ae41](
* revert generics syntax change in MockHttpService test utility ([#1574]( ([b629488](
* update exception mapping on HTTP error responses ([#1570]( ([8a170d1](

### Dependencies

* update grpc to 1.42.1 ([#1559]( ([92b7632](
* upgrade protobuf to 3.19.1 ([#1571]( ([7b354e7](

This PR was generated with [Release Please]( See [documentation](
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cla: yes
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants