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

[http-client] Exception Mapper/Async Retry #274

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Aug 24, 2023

Fixes that long-standing async retry capability gap and adds exception mapping for requests

  • now can configure a global exception mapper that maps HttpException to other kinds of exception
  • can set error handlers on a per-request basis to override the global mapper
  • Modify generator to read throws statement on Client interfaces, if the Exception has a constructor that accepts HttpException, an error mapper will auto generate for the request
  • Async Requests now propagate connection errors as HttpException matching sync behavior
  • Async requests can now use retry handlers
  • upgrade to prisms 1.12
@Client
interface ApiClient {

  @Get("/mapped")
  String mapped(@Header("Accept") String accept) throws MappedException;
}
class MappedException extends RuntimeException {

  public MappedException(HttpException e) {}
}

The following impl method will generate

  // GET /mapped
  @Override
  public String mapped(String accept) {
    return client.request()
      .header("Accept", accept)
      .path("mapped")
      .errorMapper(MappedException::new)
      .GET()
      .asPlainString().body();
  }

fixes #273

try {
response = performSend(responseHandler);
} catch (final HttpException e) {
throw errorMapper.apply(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

This errorMapper.apply() isn't where I was expecting it to be.

I was thinking it would be part of checkMaybeThrow() ... so I'm pondering this. This does not look right in that we want to let the response be decoded if necessary and HttpException is thrown later?

I'll pull this and see how it works and why it's not what I was expecting - looks cool otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is so it'll work for connection exceptions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, wait you might be right

Copy link
Contributor

@rbygrave rbygrave Aug 24, 2023

Choose a reason for hiding this comment

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

Yeah, we need the mapping to occur also in the checkResponse() and checkMaybeThrow() (as well as in the sendWith())

See the additional commits at: #275

@rbygrave rbygrave merged commit 4737dbe into avaje:master Aug 24, 2023
2 checks passed
@rbygrave rbygrave added this to the 2.0 milestone Aug 24, 2023
@rbygrave
Copy link
Contributor

Awesome work btw !!!

@SentryMan SentryMan deleted the decoder branch August 24, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[http client] Add support for "error decoder" Function<HttpException, DistanceException>
2 participants