From 878c67a65f8793c4ee5d2c422b55b80174019258 Mon Sep 17 00:00:00 2001 From: Bennor McCarthy Date: Fri, 27 Jun 2014 20:03:22 +1000 Subject: [PATCH 1/3] Stop eating the error detail. Some APIs return useful messages in 401 responses, etc, but HttpResponseMessage.EnsureStatusCode() throws away the response, and only includes the status code in the message of the exception it throws. This throws a custom ApiException with enough information in it to deal with the error properly. --- Refit/RequestBuilderImplementation.cs | 57 +++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/Refit/RequestBuilderImplementation.cs b/Refit/RequestBuilderImplementation.cs index ebc294473..f277a04b8 100644 --- a/Refit/RequestBuilderImplementation.cs +++ b/Refit/RequestBuilderImplementation.cs @@ -1,14 +1,16 @@ using System; +using System.Net; using System.Net.Http; using System.Collections.Generic; using System.Linq; +using System.Net.Http.Headers; using System.Reflection; using System.Threading.Tasks; using System.Text.RegularExpressions; using System.Text; using Newtonsoft.Json; using System.IO; -using System.Web; +using HttpUtility = System.Web.HttpUtility; using System.Threading; namespace Refit @@ -180,7 +182,9 @@ Func buildVoidTaskFuncForMethod(RestMethodInfo restM var rq = factory(paramList); var resp = await client.SendAsync(rq); - resp.EnsureSuccessStatusCode(); + if (!resp.IsSuccessStatusCode) { + throw await ApiException.Create(resp); + } }; } @@ -195,7 +199,9 @@ Func> buildTaskFuncForMethod(RestMethodInfo res return resp as T; } - resp.EnsureSuccessStatusCode(); + if (!resp.IsSuccessStatusCode) { + throw await ApiException.Create(resp); + } var content = await resp.Content.ReadAsStringAsync(); if (restMethod.SerializedReturnType == typeof(string)) { @@ -511,4 +517,49 @@ void determineReturnTypeInfo(MethodInfo methodInfo) throw new ArgumentException("All REST Methods must return either Task or IObservable"); } } + + public class ApiException : Exception + { + public HttpStatusCode StatusCode { get; private set; } + public string ReasonPhrase { get; private set; } + public HttpResponseHeaders Headers { get; private set; } + + public HttpContentHeaders ContentHeaders { get; private set; } + public string Content { get; private set; } + public bool HasContent + { + get { return !string.IsNullOrWhiteSpace(Content); } + } + + private ApiException(HttpStatusCode statusCode, string reasonPhrase, HttpResponseHeaders headers) + : base(createMessage(statusCode, reasonPhrase)) { + StatusCode = statusCode; + ReasonPhrase = reasonPhrase; + Headers = headers; + } + + public T GetContentAs() + { + return HasContent + ? JsonConvert.DeserializeObject(Content) + : default(T); + } + + public static async Task Create(HttpResponseMessage response) { + var exception = new ApiException(response.StatusCode, response.ReasonPhrase, response.Headers); + + if (response.Content == null) return exception; + + exception.ContentHeaders = response.Content.Headers; + exception.Content = await response.Content.ReadAsStringAsync(); + response.Content.Dispose(); + + return exception; + } + + static string createMessage(HttpStatusCode statusCode, string reasonPhrase) + { + return string.Format("Response status code does not indicate success: {0} ({1}).", (int)statusCode, reasonPhrase); + } + } } From 3f9b2e37188df1984816c07a7d76d196fe240eaa Mon Sep 17 00:00:00 2001 From: Bennor McCarthy Date: Sat, 28 Jun 2014 21:00:06 +1000 Subject: [PATCH 2/3] Added integration test which uses custom exception. Had to modify the PostToRequestBin test too, but I'm not sure what the point of that is since it will always fail. Maybe we should just remove it? --- Refit-Tests/RestService.cs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/Refit-Tests/RestService.cs b/Refit-Tests/RestService.cs index 8d4be1b3e..722e88944 100644 --- a/Refit-Tests/RestService.cs +++ b/Refit-Tests/RestService.cs @@ -1,4 +1,5 @@ using System; +using System.Net; using System.Net.Http; using NUnit.Framework; using System.Threading.Tasks; @@ -46,6 +47,9 @@ public interface IGitHubApi [Get("/")] Task GetIndex(); + + [Get("/give-me-some-404-action")] + Task NothingToSeeHere(); } public class RootObject @@ -111,7 +115,7 @@ public void PostToRequestBin() ae.Handle( x => { - if (x is HttpRequestException) + if (x is ApiException) { // we should be good but maybe a 404 occurred return true; @@ -123,6 +127,22 @@ public void PostToRequestBin() } } + [Test] + public async Task CanGetDataOutOfErrorResponses() + { + var fixture = RestService.For("https://api.github.com"); + try { + await fixture.NothingToSeeHere(); + Assert.Fail(); + } + catch (ApiException exception) { + Assert.AreEqual(HttpStatusCode.NotFound, exception.StatusCode); + var content = exception.GetContentAs(); + Assert.AreEqual("Not Found", (string)content.message); + Assert.IsNotNull((string)content.documentation_url); + } + } + public interface IRequestBin { [Post("/1h3a5jm1")] From e7c53ad23a860d91d340041c35476ed7f0454c55 Mon Sep 17 00:00:00 2001 From: Bennor McCarthy Date: Mon, 30 Jun 2014 23:21:25 +1000 Subject: [PATCH 3/3] Handle exceptions when reading error content. --- Refit/RequestBuilderImplementation.cs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/Refit/RequestBuilderImplementation.cs b/Refit/RequestBuilderImplementation.cs index f277a04b8..634740ea5 100644 --- a/Refit/RequestBuilderImplementation.cs +++ b/Refit/RequestBuilderImplementation.cs @@ -549,11 +549,18 @@ public static async Task Create(HttpResponseMessage response) { var exception = new ApiException(response.StatusCode, response.ReasonPhrase, response.Headers); if (response.Content == null) return exception; - - exception.ContentHeaders = response.Content.Headers; - exception.Content = await response.Content.ReadAsStringAsync(); - response.Content.Dispose(); - + + try { + exception.ContentHeaders = response.Content.Headers; + exception.Content = await response.Content.ReadAsStringAsync(); + response.Content.Dispose(); + } + catch { + // We're already handling an exception at this point, + // so we want to make sure we don't throw another one + // that hides the real error. + } + return exception; }