Add error handler

[edwardaux]

Is your feature request related to a problem? Please describe. At the moment, one of the challenges is that callers of the generated Retrofit.dart code have to know about things like a DioError. I feel like this is an implementation detail of the networking layer, and it would be nice to be able to (within the Retrofit file) catch and provide common error handling and/or possible modification to any errors.

Describe the solution you'd like I was thinking that we could add another parameter to the RestApi annotation which is a function reference. Something like:

@RestApi(onError: handleError)
abstract class MyAPI {
  @POST('/login')
  Future<User> login({String userid, String password});

  Exception handleError(Exception e) {
    // this is where I would do any common error handling
    return MyNewException();
  }
}

And then, in the generated code, if the onError parameter has been set, then it would call this function before rethrowing.

Note that I think that the error handling should also encompass both the network call and also any calls that convert to/from JSON.

Describe alternatives you've considered I did think about adding the following function in the generated class:

class _MyAPI implements MyAPI {
  ...

  Exception handleError(Exception e) {
    // standard error handling just returns the error untouched
    return e;
  }
}

And then the app's class could override handleError(). The same changes would be required in the generated code, however, I feel like that isn't super discoverable for users of the Retrofit library.

At the moment, the only "workaround" I can find is that every caller has to apply their own common error handling. That behaviour can be extracted into a common function, but I do believe that the domain-specific business logic should not know about things like DioError.

Additional context None

[pythonhubdev]

I think handling errors for each method would be better because sometimes we also throw errors based on the API response that comes from the server. For more information I have filed a issue #357

[ipcjs]

I think that error handling should be done by the dio interceptor. Dio interceptor only supports throwing DioError. Therefore, we can make custom exception implement the DioError interface :) This is a example: https://gist.github.com/ipcjs/c0896bf90effe955a863ed9813d006c5

[TekExplorer]

I think that error handling should be done by the dio interceptor. Dio interceptor only supports throwing DioError. Therefore, we can make custom exception implement the DioError interface :) This is a example: https://gist.github.com/ipcjs/c0896bf90effe955a863ed9813d006c5

The issue I see with this is that I cant throw custom exceptions specific to the API call.

[TekExplorer]

I suppose I could use @Extra(), but then it becomes boilerplate-heavy, which is exactly what I'm trying to avoid.

I'd rather specify an @ErrorHandler(onError) on the method

[TekExplorer]

I think theres no reason not to support this. In fact, we could have 2 handlers. one common to everything, and one for the individual calls. Theres quite a few ways we could do this.

All methods:

• Have an arbitrary method in the class annotated with @ErrorHandler() which generates a try catch for every method, passing the exception into it. (we could name the method itself whatever we want.) (a potentially overridable method) (better?)
• Have an onError in the annotation @RestApi(onError: handleError) (always a static method)
• Alternatively, define an annotation that wraps all methods in this one

-- additionally, give methods some way to opt out of this behavior, in case its relevant.

Per method:

• Have an @ErrorHandler(onError: (e, s) => do something) (always static)
• Perhaps some kind of consistent naming scheme for per-method error handlers?
  • for a getTasks could be maybe a _catchGetTasks? (feels arbitrary)
  • for a getTasks define a method that handles errors and call _getTasks()?
  • alternatively; document that users should simply locally wrap their call as follows:

    @Get(whatever)
    Future<Something> _getTasks();
    Future<Something> getTasks() => _getTasks()..onError // or whatever -> arbitrary.

I believe that the best way to do this is to define an @ErrorHandler method, which simply requires that the method (or hell, function variable) be of an appropriate function type. (a typedef would be useful here.) and all it does is apply a try catch to all generated methods (except for those who opt out somehow - another annotation?)

This could look like this:

@Retrofit() //< whatever
abstract class MyClient {
    @ErrorHandler() // any useful params?
    // what return type? does it need one besides void?
    // should it support being a future?
    SomeReturnType myErrorHandler(dioError, stacktrace) { // what signature?
        if (dioError is SocketException) throw MyNoConnectionException()
    }

    ...
    getTasks();
}

class GeneratedClass {
    getTasks() async {
    try{
      ...
    } catch (e, s) {
        // maybe should return something to tell wether we should rethrow?
        myErrorHandler(e,s);
        rethrow; // in case we dont handle that particular exception.
    }
}

Alternatively:

@Retrofit() //< whatever
abstract class MyClient {
    @FutureWrapper() // any useful params? better name?
    // what return type? does it need one besides void?
    // should it support being a future?
    Future<T> myErrorHandler<T>(FutureOr<T> future) async { // what signature?
        return await future; // do whatever you like to the future. try catch, future.onError, etc.
    }

    @MethodWrapper() // better name?
    //alternatively (more control over WHEN the api call is made)
    Future<T> myErrorHandler<T>(Future<T> Function() callback) async { // what signature?
        return await callback(); // do whatever you like to the future. try catch, future.onError, etc.
    }

    ...
    getTasks();

    @DontWrap()
    getSomething();
}

class GeneratedClass {
    getTasks() => exceptionHandler(_$getTasks());
    _$getTasks() => // logic. expose some way to change this name? annotation? directly wrap the handler?
    // alternatively
    getTasks() => exceptionHandler(() => async {
        // logic
    });
    getSomething() => // logic
}

[TekExplorer]

Updated the above with some possibilities

[ekuleshov]

I also find myself wrapping every rest client method call into something like this:

_restClient.apiMethodCall(...).catchError((ex) => errorHandler(ex));

That is a lot of boiler plate code and the same errorHandler is shared between multiple unrelated services.

Currently it is not possible to replace this with a Dio interceptor, because some exceptions, such as data conversions or SocketException are not being routed there.

Also the proposed @ErrorHandler() methods in the rest client is not ideal, because those methods may have to use framework specific code, which shouldn't be in the rest client (eg. Bloc or other state UI or navigation) and also this won't allow to share error handlers.

It would be neat if you could just specify an optional onError parameter in the rest client constructor declaration with the same signature as used by Future.catchError():

factory RestClient(Dio dio, {String baseUrl, Function onError}) = _RestClient;

and then have retrofit_generator would generate and wire in the above .catchError(onError) in the rest client implementation.

[trevorwang]

I also find myself wrapping every rest client method call into something like this:

_restClient.apiMethodCall(...).catchError((ex) => errorHandler(ex));

That is a lot of boiler plate code and the same errorHandler is shared between multiple unrelated services.

Currently it is not possible to replace this with a Dio interceptor, because some exceptions, such as data conversions or SocketException are not being routed there.

Also the proposed @ErrorHandler() methods in the rest client is not ideal, because those methods may have to use framework specific code, which shouldn't be in the rest client (eg. Bloc or other state UI or navigation) and also this won't allow to share error handlers.

It would be neat if you could just specify an optional onError parameter in the rest client constructor declaration with the same signature as used by Future.catchError():

factory RestClient(Dio dio, {String baseUrl, Function onError}) = _RestClient;

and then have retrofit_generator would generate and wire in the above .catchError(onError) in the rest client implementation.

It sounds like more ideal and makes a minimum change. @ekuleshov @TekExplorer

[TekExplorer]

That might be generally useful, but doesn't actually help convert responses into custom MyApiExceptions internally

If a method within the client could be set to wrap all methods generated by retrofit, that would fix it (as then you could have that reference a passed-in method or a static function or anything at all)

Additionally, if a wrapper method is defined, you could do a lot more before and after the actual API call is made

Really all I propose is a way to wrap all API calls, which automatically gives us the ability to handle errors as we please

[ekuleshov]

That might be generally useful, but doesn't actually help convert responses into custom MyApiExceptions internally

But it does. That is all I'm doing in my current boiler plate code for all rest API calls:

_restClient.apiMethodCall(...).catchError((ex) => errorHandler(ex));

where errorHandler is implemented something like this (removed all custom logic for simplicity):

T errorHandler<T>(dynamic ex) {
  throw MyException(message: ex.toString();
}

or it can return response with the error state:

T errorHandler<T extends MyResponse>(dynamic ex) {
  return MyResponse(error: ex.toString());
}

Generally it is the same idea and API as used in Future.then(..).onError(..).catchError(..)

Having a wrapper for each rest service method would provide similar features. But there is a problem with input and output types. I don't see how they can be mapped in the generated API, because rest API declares specific type - you can't change it in the generated code.

Also specifying any wrappers or handlers with annotations is too restrictive. Personally I'd prefer them specified as the factory/constructor parameters. That is also inline how freezed generates its code.

[TekExplorer]

That might be generally useful, but doesn't actually help convert responses into custom MyApiExceptions internally

But it does. That is all I'm doing in my current boiler plate code for all rest API calls:

_restClient.apiMethodCall(...).catchError((ex) => errorHandler(ex));

where errorHandler is implemented something like this (removed all custom logic for simplicity):

T errorHandler<T>(dynamic ex) {
  throw MyException(message: ex.toString();
}

or it can return response with the error state:

T errorHandler<T extends MyResponse>(dynamic ex) {
  return MyResponse(error: ex.toString());
}

Generally it is the same idea and API as used in Future.then(..).onError(..).catchError(..)

Having a wrapper for each rest service method would provide similar features. But there is a problem with input and output types. I don't see how they can be mapped in the generated API, because rest API declares specific type - you can't change it in the generated code.

Also specifying any wrappers or handlers with annotations is too restrictive. Personally I'd prefer them specified as the factory/constructor parameters. That is also inline how freezed generates its code.

but thats the thing. with a wrapper function, you can do that.

class MyApi {

MyApi._({this.onError});
// passes along parameters that the private constructor has? 
// could alternatively have a builder-style mutable variable
factory MyApi(dio, {baseUrl, onError}) = _MyApi;

final Function? onError;

// maybe include invocation details for more control? probably not.
@wrapper
Future<T> wrapper<T>(Future<void> Function() cb) => cb().onError(onError);
...
}

this not only gives you the ability to handle errors, but opens the door to do just about anything you want, including multi-step handlers. it also ensures that you actually see whats happening and no behavior is hidden.

this specific boilerplate could also be abstracted into a different annotation if you dont need this control.

[ekuleshov]

Have you missed this.onError in the constructor? The @wrapper specification will likely going to be prohibitive due to types mismatch. It doesn't specify parameters or return types and Future<void> is likely invalid type in general. So, it is unclear if you can wire it like that with generated retrofit implementation.

All in all, as @trevorwang said, the onError handler is a smaller change.

[TekExplorer]

Have you missed this.onError in the constructor? The @wrapper specification will likely going to be prohibitive due to types mismatch. It doesn't specify parameters or return types and Future<void> is likely invalid type in general. So, it is unclear if you can wire it like that with generated retrofit implementation.

All in all, as @trevorwang said, the onError handler is a smaller change.

what do you mean type mismatch? what do you mean Future being invalid? none of that seems to actually reference anything

[ekuleshov]

@TekExplorer all I'm saying your examples are incomplete and it is unclear (at least to me) how implementation can be wired together for them. You have declaration for the rest API methods and your wrapper or error handler. But what has to be in between? So, perhaps such approach is over-generalized...

Can you write method(s) that will call the rest API methods and then "wraps" that call into your wrapper/handler one to demonstrate how it is supposed to work for various method parameters and return types? The retrofit_generator will essentially have to do just that.

My suggestion is much simpler in that regard. It simply chains to the Future API which retrofit_generator already handling.

[TekExplorer]

@TekExplorer all I'm saying your examples are incomplete and it is unclear (at least to me) how implementation can be wired together for them. You have declaration for the rest API methods and your wrapper or error handler. But what has to be in between? So, perhaps such approach is over-generalized...

Can you write method(s) that will call the rest API methods and then "wraps" that call into your wrapper/handler one to demonstrate how it is supposed to work for various method parameters and return types? The retrofit_generator will essentially have to do just that.

My suggestion is much simpler in that regard. It simply chains to the Future API which retrofit_generator already handling.

my idea is extremely simple.

1 method which wraps the rest

@wrapper
Future<T> wrapper<T>(Future<T> Function() cb) => cb().onError(...);
// or
Future<T> wrapper<T>(Future<T> Function() cb) {
  // do something custom.
  try {
    final data = await cb();
   return data
  } catch (e) {
    print('something went wrong: $e')
  } finally {
    // something custom. releasing a lock?
  }
}

// perhaps you want to do something with the response.
// routes that just want the data can use the data getter
Future<T> wrapper<T>(Future<HttpResponse<T>> Function() cb);

@GET
Future<Todo> getTodo(id);

Future<Todo> getTodo(id) {
  return wrapper<Todo>(() async {
     ...
     return dio.get(id)
  });
}

// or
Future<Todo> _getTodo(id) async {
  return dio.whatever
}
Future<Todo> getTodo(id) => wrapper(() => __getTodo(id));

[ekuleshov]

@TekExplorer you said you want to do "multi step handlers" or perhaps have access to the rest method parameters in your wrapper, but your example does not reflect any of that and doesn't do anything that can't be done in a regular Future.onError() callback.

Future<T> wrapper<T>(Future<T> Function() cb) ...

Also your example also binds your wrapper return type to the rest api method return type, so wrapper won't be able to alter the return type of API response.

[trevorwang]

I have the same opinion with @ekuleshov . It seems that It doesn't provide more benefits than a normal error handle from you proposal. @TekExplorer

[TekExplorer]

I think it makes sense. but honestly, if only there was a way to "pass along" parameters instead of having to specify them manually it would be a non-issue

[ebwood]

I implement a simple wrapper generator to handle error. It will be nice if the library can support this feature.

[yeikel16]

I implement a simple wrapper generator to handle error. It will be nice if the library can support this feature.

This is amazing, work very good @ebwood, you have intention to publish into pub.dev?