Fine-grained HTTP error exceptions with client and server errors

[jerzykrlk]

No description provided.

[kdavisk6]

@jerzykrlk

Looking over this PR and reviewing the comments, I think that you've discovered why something like this has not been added before. It's been our opinion that decisions such as what exception should be surfaced back to the caller and whether that exception should be retried should be left to the user of Feign. I made this same comment in #823.

I think that exposing this level of detail back to the caller leaks too much of Feign's internals. Let's take a look at your example from #823

try {
   return this.library.getBook(1);
} catch (HttpNotFoundException hce) {
   // handle not found.
}

HttpNotFoundException should be part of your application's domain, as it controls the behavior of your application. By asking Feign to manage this, your application's behavior now relies on Feign's internal models. The ErrorDecoder exists for this exact reason, to provide a way to hook your application's domain and behavior without requiring your application to know or understand Feign's internal model.

With that said, I do see the value in having more concrete error types in Feign, but I recommend that we limit it's visibility. One suggestion is to move this logic into a new StatusCodeErrorDecoder, which provides hooks for users to specify which status codes should be mapped and which ones should be retried. Here is a quick example:

public class StatusCodeErrorDecoder implements ErrorDecoder {
   private final List<Class<StatusCodeException>> retryableStatusCodes = new ArrayList();
   public StatusCodeErrorDecoder(Class<StatusCodeExceptions> ... retryableCodes) {
      this.retryableStatusCodes = Arrays.asList(retryableCodes);
   }

   @Override
   public Exception decode(String methodKey, Response response) {
        StatusCodeException statusException = this.getStatusCodeExceptionFromResponse(response);
        if (this.retryableStatusExceptions.contains(statusException.getClass()) {
           // retry
           throw new RetryableException(statusException);
        } else {
           // decode the status code exception into a domain exception
           return this.decodeStatusException(methodKey, response, statusException);
        }
   }

   protected Exception decodeStatusException(String methodKey, Response response, StatusCodeException statusException {
      // default is to wrap and throw the status as a Feign exception
      throw new FeignException(statusException);
   }

   // base class for all status code exceptions, encapsulated into this decoder to 
   // intentionally limit visibility and isolate usage to this decoder.
   static class StatusCodeException {
   }
  
   // additional specific status code exceptions are defined here.
   
}

Users can use this decoder in place of the default encoder or extend it to create their own, more specific decoders:

public class CustomStatusCodeErrorDecoder extends StatusCodeErrorDecoder {
   public CustomStatusCodeErrorDecoder() {
      // retry service unavailable and timeouts
      super(ServiceUnavailableException.class, GatewayTimeoutException.class);
   }

   @Override
   protected Exception decodeStatusException(String methodKey, Response response, StatusCodeException statusException {
      // in this case, I want to return a domain exception when the object was not found
      if (statusException instanceof HttpNotFoundException) {
         throw new BookNotFoundException();
      }

      // all others log and return a generic exception
      throw new BookException(statusException);
   }
}

Using this approach, we can allow users of Feign to create more expressive ErrorDecoders without pushing this responsibility too far down into Feign.

Thoughts?

[jerzykrlk]

Hi @kdavisk6

It's just a response to @rage-shadowman comment here: #825 (comment) . I am not sure if it is absolutely necessary to have these, but they might be useful.

In my opinion, this pull request should not go anywhere beyond distinguising 4xx and 5xx errors. Just as they are in Spring Framework - a purely technical distinction. Personally, I'd stick to that., but the discussion grew exponentially. :)

In terms of retrying - I think that it is a different, more complex thing. And should be moved into a different discussion. And probably be followed by a separate pull request.

[kdavisk6]

@jerzykrlk If others are ok with moving forward without the retry decision, this is good from my perspective.

[kdavisk6]

@jerzykrlk

I'd like to accept this change for 10.3.0. However, this is a conflict. Can you please take a look?

[jerzykrlk]

Hi. I've rebased. I think it should be fine now.