Generic method to call API functions with simple retrial on errors

Question

I have to develop a small security management application that relies on a Web API. Based on that API, I have generated the REST client classes (Open API generator), but I still have to code on top to provide things like retrials and token management (avoid getting a token for each call).

Code

///<inheritdoc/>
public class DataLakeSecurityApiTokenService : IDataLakeSecurityApiTokenService
{
    private const int TokenRefreshCoolDownPeriod = 3600;    // seconds

    private static readonly SemaphoreSlim NewTokenMutex = new SemaphoreSlim(1, 1);
    private static string Token { get; set; }
    private static readonly Stopwatch StopWatch= new Stopwatch();

    private IConfigurationService ConfigurationService { get; }
    private ILoggingService Logger { get; }

    private async Task RefreshToken()
    {
        var defaultApiClient = new DefaultApi(ConfigurationService.DataLakeBasePath);
        var tokenObj = (JObject)await defaultApiClient.LoginTokenPostAsync(
            ConfigurationService.DataLakeUser, ConfigurationService.DataLakePassword);
        Token = tokenObj["access_token"].ToString();
        StopWatch.Restart();
    }

    ///<inheritdoc/>
    public DataLakeSecurityApiTokenService(IConfigurationService configurationService, ILoggingService logger)
    {
        ConfigurationService = configurationService;
        Logger = logger;
    }

    ///<inheritdoc/>
    public async Task<ValidationResult<string>> GetLoginToken(bool force = false)
    {
        await NewTokenMutex.WaitAsync();
        try
        {
            bool expired = StopWatch.Elapsed.Seconds > TokenRefreshCoolDownPeriod;
            if (expired || force || string.IsNullOrWhiteSpace(Token))
                await RefreshToken();

            return new ValidationResult<string> {Payload = Token};
        }
        catch (ApiException exc)
        {
            Logger.LogError(exc, "Failed to get login token");
            return new ValidationResult<string> { IsError = true, Message = exc.Message};
        }
        finally
        {
            NewTokenMutex.Release();
        }
    }

    ///<inheritdoc/>
    public async Task<ValidationResult<TRes>> ExecuteWithTokenRefresh<TApiClient, TRes>(
        string context, string functionName, params object[] functionParams)
    {
        for (int retryIndex = 0; retryIndex < 2; retryIndex++)
        {
            var tokenRes = await GetLoginToken(retryIndex > 0);
            if (tokenRes.IsError)
                return ValidationResult<TRes>.Create(tokenRes);

            try
            {
                var defaultConfiguration = GetDefaultConfiguration(tokenRes.Payload);
                var apiClient = Activator.CreateInstance(typeof(TApiClient), args: defaultConfiguration);
                // ReSharper disable once PossibleNullReferenceException
                var apiResponse = await (Task<ApiResponse<TRes>>) typeof(TApiClient).GetMethod(functionName)
                    ?.Invoke(apiClient, functionParams);
                // ReSharper disable once PossibleNullReferenceException
                return new ValidationResult<TRes> {Payload = apiResponse.Data};
            }
            catch (ApiException exc)
            {
                Logger.LogError(exc, $"{context} error, retry index = {retryIndex}");
                if (retryIndex > 0)
                    return new ValidationResult<TRes> { IsError = true, Message = exc.Message }; 
            }
        } 

        // normally it should not ever reach this point
        return new ValidationResult<TRes>();
    }

    //TODO: move to a more appropriate place when a common service is defined
    ///<inheritdoc/>
    public Configuration GetDefaultConfiguration(string token)
    {
        return new Configuration
        {
            BasePath = ConfigurationService.DataLakeBasePath,
            AccessToken = token
        };
    }
}

This code does the following:

• caches the last valid token for a period
• provides a way to quickly execute an API client method without worrying about getting the token, by fetching the token if necessary and also performing a retrial if an error has occurred. Unfortunately the generated code does not allow to discriminate between access denied errors (invalid or expired token) and a real internal server error (500), so I have to retry regardless of the error to be certain I catch access denied errors.

Usage

///<inheritdoc/>
public async Task<ValidationResult<List<Tag>>> GetTableLevelTagList()
{
    var ret = await DataLakeSecurityApiTokenService.ExecuteWithTokenRefresh<SecurityTagsApi, List<Tag>>(
        "Get table level tag list", 
        nameof(SecurityTagsApi.ListTableLevelSecurityTagsTagTableGetAsyncWithHttpInfo));
    return ret;
}

The usage is pretty straightforward, but what I particularly dislike is that I am using dynamic invocation. If the consumer works with nameof it is pretty safe, but it still quite messy when working with methods with parameters which are not validated at compile-time.

I am wondering if there is way to improve this code.

Answer 1

Inversion-of-control

I wouldn't use reflection using error-prone member-by-string lookup, when a simple Func provides type-safe behavior. This signature could be used to have the error callback, client factory and operation provided using inversion-of-control.

public async Task<ValidationResult<TRes>> ExecuteWithTokenRefresh<TApiClient, TRes>(
    Action<ApiException, int> errorCallback,
    Func<Configuration, TApiClient> clientFactory,
    Func<TApiClient, Task<ValidationResult<TRes>> operation) { // ..

Creating the client:

var apiClient = clientFactory(defaultConfiguration);

instead of:

 var apiClient = Activator.CreateInstance(typeof(TApiClient), 
    args: defaultConfiguration);

Getting the response then becomes:

var apiResponse = await operation(apiClient);

instead of:

var apiResponse = await (Task<ApiResponse<TRes>>) 
    typeof(TApiClient).GetMethod(functionName)
        ?.Invoke(apiClient, functionParams);

And the error gets handled as:

catch (ApiException exc)
{
    errorCallback(exc, retryIndex);
    if (retryIndex > 0)
        return new ValidationResult<TRes> { IsError = true, Message = exc.Message }; 
}

so you no longer require that context string:

Logger.LogError(exc, $"{context} error, retry index = {retryIndex}");

This is only one possible way to refactor the code. Perhaps you would like to have improved error flow management and have the error callback control retry management.

Answer 2

Definitely agree with dfhwze's suggestion above. A few other minor points to consider:

1. Define TokenRefreshCoolDownPeriod as a TimeSpan since that is what it represents. This simplifies the stopwatch comparison slightly and improves readability.

private static readonly TimeSpan TokenRefreshCoolDownPeriod = TimeSpan.FromHours(1);

// ...

var expired = StopWatch.Elapsed > TokenRefreshCoolDownPeriod;

2. The if (retryIndex > 0) statement in your catch block will always match on the second attempt. This is not currently a problem because you're only attempting a maximum of two times, but may introduce a subtle bug if you later decide to increase the maximum retry count. Consider defining a MaxAttemptCount field and using it in the for loop and if statement instead, e.g.

private const int MaxAttemptCount = 2;

// ...

// Note indexing starts from 1 now, i.e. the first attempt is attempt #1, and the
// final iteration will be when the attempt number equals the maximum defined above.
for (int attemptNumber = 1; attemptNumber <= MaxAttemptCount; attemptNumber++)
{
    // Only force the token refresh on attempts after the first.
    var tokenRes = await GetLoginToken(attemptNumber > 1);

    // ...

    try
    {
        // ...
    }
    catch (ApiException exc)
    {
        Logger.LogError(exc, $"{context} error, attempt number = {attemptNumber}");

        // If this is the last attempt, return the error.
        if (attemptNumber == MaxAttemptCount)
            return new ValidationResult<TRes> { IsError = true, Message = exc.Message }; 
    }
}

Answer 3

There are dragons in your code I must say you beat me with this API. I usually build strange things but the way you misuse the ValidationResult here is remarkable. Every single method returns a ValidationResult<T> or some kind. I needed a while to figure out that you aren't actually validating anything.

Use a true monad If you want this kind of monad for results that may fail or not then you should use something that is built for this purpose and doesn't confuse the user. Like the Task that you are already using. In this case you wrapped one of them in another one. You can however create your own like the Either Monad.

Read You can also learn a lot about this topic from this question: A Failable that allows safe returning of exceptions .