From a14b3d8aa24a6e8bbf66ee5a7fb10608f67f6942 Mon Sep 17 00:00:00 2001
From: John Lambert <john_lambert@sil.org>
Date: Tue, 16 Jan 2024 10:01:23 -0500
Subject: [PATCH] reviewer comments.

---
 .../IMachineBuilderExtensions.cs              |  4 +-
 .../Services/ClearMLAuthenticationService.cs  | 45 +++++++++----------
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs b/src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs
index 47a585be6..ade0347d9 100644
--- a/src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs
+++ b/src/SIL.Machine.AspNetCore/Configuration/IMachineBuilderExtensions.cs
@@ -384,7 +384,9 @@ public static IMachineBuilder AddBuildJobService(this IMachineBuilder builder)
 
             var smtTransferEngineOptions = new SmtTransferEngineOptions();
             builder.Configuration.GetSection(SmtTransferEngineOptions.Key).Bind(smtTransferEngineOptions);
-            string driveLetter = Path.GetPathRoot(smtTransferEngineOptions.EnginesDir)![..1];
+            string? driveLetter = Path.GetPathRoot(smtTransferEngineOptions.EnginesDir)?[..1];
+            if(driveLetter is null)
+                throw new InvalidOperationException("SMT Engine directory is required");
             // add health check for disk storage capacity
             builder.Services
                 .AddHealthChecks()
diff --git a/src/SIL.Machine.AspNetCore/Services/ClearMLAuthenticationService.cs b/src/SIL.Machine.AspNetCore/Services/ClearMLAuthenticationService.cs
index 172c9b772..721d68868 100644
--- a/src/SIL.Machine.AspNetCore/Services/ClearMLAuthenticationService.cs
+++ b/src/SIL.Machine.AspNetCore/Services/ClearMLAuthenticationService.cs
@@ -1,42 +1,34 @@
 namespace SIL.Machine.AspNetCore.Services;
 
-public class ClearMLAuthenticationService : RecurrentTask, IClearMLAuthenticationService
+public class ClearMLAuthenticationService(
+    IServiceProvider services,
+    IHttpClientFactory httpClientFactory,
+    IOptionsMonitor<ClearMLOptions> options,
+    ILogger<ClearMLAuthenticationService> logger
+    ) : RecurrentTask("ClearML authentication service", services, RefreshPeriod, logger), IClearMLAuthenticationService
 {
-    private readonly HttpClient _httpClient;
-    private readonly IOptionsMonitor<ClearMLOptions> _options;
-    private readonly ILogger<ClearMLAuthenticationService> _logger;
+    private readonly HttpClient _httpClient = httpClientFactory.CreateClient("ClearML");
+    private readonly IOptionsMonitor<ClearMLOptions> _options = options;
+    private readonly ILogger<ClearMLAuthenticationService> _logger = logger;
     private readonly AsyncLock _lock = new();
 
     // technically, the token should be good for 30 days, but let's refresh each hour
     // to know well ahead of time if something is wrong.
     private static readonly TimeSpan RefreshPeriod = TimeSpan.FromSeconds(3600);
-    private string? _authToken = "";
-
-    public ClearMLAuthenticationService(
-        IServiceProvider services,
-        IHttpClientFactory httpClientFactory,
-        IOptionsMonitor<ClearMLOptions> options,
-        ILogger<ClearMLAuthenticationService> logger
-    )
-        : base("ClearML authentication service", services, RefreshPeriod, logger)
-    {
-        _httpClient = httpClientFactory.CreateClient("ClearML");
-        _options = options;
-        _logger = logger;
-    }
+    private string _authToken = "";
 
     public async Task<string> GetAuthTokenAsync(CancellationToken cancellationToken = default)
     {
         using (await _lock.LockAsync(cancellationToken))
         {
-            if (_authToken is null || _authToken is "")
+            if (_authToken is "")
             {
                 //Should only happen once, so no different in cost than previous solution
                 _logger.LogInformation("Token was empty; refreshing");
                 await AuthorizeAsync(cancellationToken);
             }
         }
-        return _authToken ?? throw new Exception("ClearML authentication token not found in response.");
+        return _authToken;
     }
 
     protected override async Task DoWorkAsync(IServiceScope scope, CancellationToken cancellationToken)
@@ -48,10 +40,14 @@ protected override async Task DoWorkAsync(IServiceScope scope, CancellationToken
         }
         catch (Exception e)
         {
-            _logger.LogError(e, "Error occurred while refreshing ClearML authentication token.");
-            if (_authToken is null || _authToken is "")
+            if (_authToken is ""){
+                _logger.LogError(e, "Error occurred while aquiring ClearML authentication token for the first time.");
                 // The ClearML token never was set.  We can't continue without it.
                 throw;
+            }
+            else
+                _logger.LogError(e, "Error occurred while refreshing ClearML authentication token.");
+
         }
     }
 
@@ -66,9 +62,10 @@ private async Task AuthorizeAsync(CancellationToken cancellationToken)
         request.Headers.Add("Authorization", $"Basic {base64EncodedAuthenticationString}");
         HttpResponseMessage response = await _httpClient.SendAsync(request, cancellationToken);
         string result = await response.Content.ReadAsStringAsync(cancellationToken);
-        _authToken = (string)((JsonObject?)JsonNode.Parse(result))?["data"]?["token"]!;
-        if (_authToken is null || _authToken is "")
+        string? refreshedToken = (string?)((JsonObject?)JsonNode.Parse(result))?["data"]?["token"];
+        if (refreshedToken is null || refreshedToken is "")
             throw new Exception($"ClearML authentication failed - {response.StatusCode}: {response.ReasonPhrase}");
+        _authToken = refreshedToken;
         _logger.LogInformation("ClearML Authentication Token Refresh Successful.");
     }
 }