[Feature Request] Authenticator success/failure callback hooks.

[diegotori]

Right now, Chopper has no means of notifying its Authenticator as to whether a re-authenticated request succeeded or failed.

Consider the following scenario: Suppose you have an Authenticator that keeps track of the number times you retry an unauthorized request so that you can fail it gracefully due to it running out of tries. Currently, you would have to keep track of a retry counter every time authenticate is called. That means if you burned through your retries, you have to wait until the next call to authenticate before it sends back null due to incrementing the counter beforehand.

Now this may not seem as a major issue. However, consider an Authenticator that wants to reset its counter once that retried request succeeds. As it's currently written, it would have to wait for the next authenticate call to return a 200 in order to reset the counter, which may or may not occur if you're not expected to make any calls after re-trying the failed one.

Bottom line, it would be nice if Authenticator had optional callback hooks that would get called, so that subclasses can track whether that call succeeded or failed. This would open doors for things like analytics use cases where you want to know if a user was unable to authenticate despite re-authenticating, or if a user was able to successfully recover their session after re-authenticating a failed request, in addition to keeping track of the current number of retries.

For example, it can look something like this:

abstract class Authenticator {
  FutureOr<Request?> authenticate(
    Request request,
    Response response, [
    Request? originalRequest,
  ]);

  FutureOr<void> onAuthenticationSuccessful(Request request,
      Response response, [
        Request? originalRequest,
      ]) {}

  FutureOr<void> onAuthenticationFailed(Request request,
      Response response, [
        Request? originalRequest,
      ]) {}
}

And they can be called after evaluating a re-authenticated request from Chopper's provided Authenticator:

      final Request? updatedRequest =
          await authenticator!.authenticate(req, res, request);

      if (updatedRequest != null) {
        res = await send<BodyType, InnerType>(
          updatedRequest,
          requestConverter: requestConverter,
          responseConverter: responseConverter,
        );
        // To prevent double call with typed response
        if (_responseIsSuccessful(res.statusCode)) {
          await authenticator!.onAuthenticationSuccessful(updatedRequest, res, request);
          return _processResponse(res);
        } else {
          res = await _handleErrorResponse<BodyType, InnerType>(res);
          await authenticator!.onAuthenticationFailed(updatedRequest, res, request);
          return _processResponse(res);
        }
      }

I will submit a PR to this effect very shortly.

[techouse]

Hey! I think this is over-engineered.

The way I've always solved it was by simply modifying the returned Request of the authenticate method with an additional header, say x-no-retry. Then you can just check that header's existence and fail an authentication.

[diegotori]

@techouse I'm trying not to adulterate the outgoing request in any way.

Also, the issue still remains where even if you successfully authenticate, you're unable to reset your state until the next call to authenticate, which may never happen unless you send another network call and it goes through the same cycle.

Bottom line, the PR adds two lines of code in order to make the Authenticator deterministic. That and if the user doesn't care about them, it won't impact them in any way, hence they're optional hooks instead of a required method.

I've already solved it without having to add a new header field. However, since that callback isn't deterministic, it leaves subclasses in the dark as to whether their attempts to re-authenticate were actually successful.