Comments + codersdk.Response handling

Author: deansheather

App/Services/CredentialManager.cs

@@ -53,15 +53,15 @@ public interface ICredentialBackend
 }
 
 /// <summary>
-///     Implements ICredentialManager using the Windows Credential Manager to
-///     store credentials.
+///     Implements ICredentialManager using an ICredentialBackend to store
+///     credentials.
 /// </summary>
 public class CredentialManager : ICredentialManager
 {
     private const string CredentialsTargetName = "Coder.Desktop.App.Credentials";
 
     // _opLock is held for the full duration of SetCredentials, and partially
-    // during LoadCredentials. _lock protects _inFlightLoad, _loadCts, and
+    // during LoadCredentials. _opLock protects _inFlightLoad, _loadCts, and
     // writes to _latestCredentials.
     private readonly RaiiSemaphoreSlim _opLock = new(1, 1);
 
@@ -215,26 +215,29 @@ private async Task<CredentialModel> LoadCredentialsInner(CancellationToken ct)
             };
         }
 
-        // Grab the lock again so we can update the state. If we got cancelled
-        // due to a SetCredentials call, _latestCredentials will be populated so
-        // we just return that instead.
+        // Grab the lock again so we can update the state.
         using (await _opLock.LockAsync(ct))
         {
-            ct.ThrowIfCancellationRequested();
-
-            // _latestCredentials will be set if we were preempted.
-            var latestCreds = _latestCredentials;
-            if (latestCreds != null) return latestCreds;
-
-            // Prevent new LoadCredentials calls from returning this task since
-            // it's pretty much done.
+            // Prevent new LoadCredentials calls from returning this task.
             if (_loadCts != null)
             {
                 _loadCts.Dispose();
                 _loadCts = null;
                 _inFlightLoad = null;
             }
 
+            // If we were canceled but made it this far, try to return the
+            // latest credentials instead.
+            if (ct.IsCancellationRequested)
+            {
+                var latestCreds = _latestCredentials;
+                if (latestCreds is not null) return latestCreds;
+            }
+
+            // If there aren't any latest credentials after a cancellation, we
+            // most likely timed out and should throw.
+            ct.ThrowIfCancellationRequested();
+
             UpdateState(model);
             return model;
         }
@@ -259,6 +262,10 @@ private async Task<CredentialModel> PopulateModel(RawCredentials? credentials, C
             sdkClient.SetSessionToken(credentials.ApiToken);
             me = await sdkClient.GetUser(User.Me, ct);
         }
+        catch (CoderApiHttpException)
+        {
+            throw;
+        }
         catch (Exception e)
         {
             throw new InvalidOperationException("Could not connect to or verify Coder server", e);
@@ -281,7 +288,10 @@ private async Task<CredentialModel> PopulateModel(RawCredentials? credentials, C
     private void UpdateState(CredentialModel newModel)
     {
         _latestCredentials = newModel;
-        CredentialsChanged?.Invoke(this, newModel.Clone());
+        // Since the event handlers could block (or call back the
+        // CredentialManager and deadlock), we run these in a new task.
+        if (CredentialsChanged == null) return;
+        Task.Run(() => { CredentialsChanged?.Invoke(this, newModel); });
     }
 }
 

CoderSdk/CoderApiClient.cs

@@ -36,7 +36,9 @@ public override string ConvertName(string name)
 }
 
 [JsonSerializable(typeof(BuildInfo))]
+[JsonSerializable(typeof(Response))]
 [JsonSerializable(typeof(User))]
+[JsonSerializable(typeof(ValidationError))]
 public partial class CoderSdkJsonContext : JsonSerializerContext;
 
 /// <summary>
@@ -97,17 +99,21 @@ private async Task<TResponse> SendRequestAsync<TRequest, TResponse>(HttpMethod m
             }
 
             var res = await _httpClient.SendAsync(request, ct);
-            // TODO: this should be improved to try and parse a codersdk.Error response
-            res.EnsureSuccessStatusCode();
+            if (!res.IsSuccessStatusCode)
+                throw await CoderApiHttpException.FromResponse(res, ct);
 
             var content = await res.Content.ReadAsStringAsync(ct);
             var data = JsonSerializer.Deserialize<TResponse>(content, JsonOptions);
             if (data is null) throw new JsonException("Deserialized response is null");
             return data;
         }
+        catch (CoderApiHttpException)
+        {
+            throw;
+        }
         catch (Exception e)
         {
-            throw new Exception($"API Request: {method} {path} (req body: {payload is not null})", e);
+            throw new Exception($"Coder API Request failed: {method} {path}", e);
         }
     }
 }

CoderSdk/Errors.cs

@@ -0,0 +1,82 @@
+using System.Net;
+using System.Text.Json;
+
+namespace Coder.Desktop.CoderSdk;
+
+public class ValidationError
+{
+    public string Field { get; set; } = "";
+    public string Detail { get; set; } = "";
+}
+
+public class Response
+{
+    public string Message { get; set; } = "";
+    public string Detail { get; set; } = "";
+    public List<ValidationError> Validations { get; set; } = [];
+}
+
+public class CoderApiHttpException : Exception
+{
+    private static readonly Dictionary<HttpStatusCode, string> Helpers = new()
+    {
+        { HttpStatusCode.Unauthorized, "Try signing in again" },
+    };
+
+    public readonly HttpMethod? Method;
+    public readonly Uri? RequestUri;
+    public readonly HttpStatusCode StatusCode;
+    public readonly string? ReasonPhrase;
+    public readonly Response Response;
+
+    public CoderApiHttpException(HttpMethod? method, Uri? requestUri, HttpStatusCode statusCode, string? reasonPhrase,
+        Response response) : base(MessageFrom(method, requestUri, statusCode, reasonPhrase, response))
+    {
+        Method = method;
+        RequestUri = requestUri;
+        StatusCode = statusCode;
+        ReasonPhrase = reasonPhrase;
+        Response = response;
+    }
+
+    public static async Task<CoderApiHttpException> FromResponse(HttpResponseMessage response, CancellationToken ct)
+    {
+        var content = await response.Content.ReadAsStringAsync(ct);
+        Response? responseObject;
+        try
+        {
+            responseObject = JsonSerializer.Deserialize<Response>(content, CoderApiClient.JsonOptions);
+        }
+        catch (JsonException)
+        {
+            responseObject = null;
+        }
+
+        if (responseObject is null or { Message: null or "" })
+            responseObject = new Response
+            {
+                Message = "Could not parse response, or response has no message",
+                Detail = content,
+                Validations = [],
+            };
+
+        return new CoderApiHttpException(
+            response.RequestMessage?.Method,
+            response.RequestMessage?.RequestUri,
+            response.StatusCode,
+            response.ReasonPhrase,
+            responseObject);
+    }
+
+    private static string MessageFrom(HttpMethod? method, Uri? requestUri, HttpStatusCode statusCode,
+        string? reasonPhrase, Response response)
+    {
+        var message = $"Coder API Request: {method} '{requestUri}' failed with status code {(int)statusCode}";
+        if (!string.IsNullOrEmpty(reasonPhrase)) message += $" {reasonPhrase}";
+        message += $": {response.Message}";
+        if (Helpers.TryGetValue(statusCode, out var helperMessage)) message += $": {helperMessage}";
+        if (!string.IsNullOrEmpty(response.Detail)) message += $"\n\tError: {response.Detail}";
+        foreach (var validation in response.Validations) message += $"\n\t{validation.Field}: {validation.Detail}";
+        return message;
+    }
+}