diff --git a/Octokit.Reactive/IObservableRepositoriesClient.cs b/Octokit.Reactive/IObservableRepositoriesClient.cs index ea3a1b8e..7be823b1 100644 --- a/Octokit.Reactive/IObservableRepositoriesClient.cs +++ b/Octokit.Reactive/IObservableRepositoriesClient.cs @@ -21,7 +21,7 @@ namespace Octokit.Reactive /// /// The default page size on GitHub.com is 30. /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A of . [SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification = "Makes a network request")] diff --git a/Octokit.Reactive/IObservableSshKeysClient.cs b/Octokit.Reactive/IObservableSshKeysClient.cs index dd8dab14..7052686e 100644 --- a/Octokit.Reactive/IObservableSshKeysClient.cs +++ b/Octokit.Reactive/IObservableSshKeysClient.cs @@ -25,7 +25,7 @@ namespace Octokit.Reactive /// /// Retrieves the for the specified id. /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A of . [SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification = "Makes a network request")] @@ -35,7 +35,7 @@ namespace Octokit.Reactive /// Update the specified . /// /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A IObservable Create(SshKeyUpdate key); @@ -44,7 +44,7 @@ namespace Octokit.Reactive /// /// /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A IObservable Update(int id, SshKeyUpdate key); @@ -52,7 +52,7 @@ namespace Octokit.Reactive /// Update the specified . /// /// The id of the SSH key - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A IObservable Delete(int id); } diff --git a/Octokit.Reactive/IObservableUsersClient.cs b/Octokit.Reactive/IObservableUsersClient.cs index 0372e596..8440c1ce 100644 --- a/Octokit.Reactive/IObservableUsersClient.cs +++ b/Octokit.Reactive/IObservableUsersClient.cs @@ -18,7 +18,7 @@ namespace Octokit.Reactive /// /// Returns a for the current authenticated user. /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A IObservable Current(); @@ -26,7 +26,7 @@ namespace Octokit.Reactive /// Update the specified . /// /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A IObservable Update(UserUpdate user); } diff --git a/Octokit.Tests.Integration/UsersClientTests.cs b/Octokit.Tests.Integration/UsersClientTests.cs index e7eadb67..023e3ff8 100644 --- a/Octokit.Tests.Integration/UsersClientTests.cs +++ b/Octokit.Tests.Integration/UsersClientTests.cs @@ -53,7 +53,7 @@ namespace Octokit.Tests.Integration Bio = "UPDATED BIO" }; - var e = await AssertEx.Throws(async + var e = await AssertEx.Throws(async () => await github.User.Update(userUpdate)); Assert.Equal(HttpStatusCode.Unauthorized, e.StatusCode); } @@ -71,7 +71,7 @@ namespace Octokit.Tests.Integration Bio = "UPDATED BIO" }; - var e = await AssertEx.Throws(async + var e = await AssertEx.Throws(async () => await github.User.Update(userUpdate)); Assert.Equal(HttpStatusCode.Unauthorized, e.StatusCode); } diff --git a/Octokit.Tests/Exceptions/ApiValidationExceptionTests.cs b/Octokit.Tests/Exceptions/ApiValidationExceptionTests.cs new file mode 100644 index 00000000..a6b532ac --- /dev/null +++ b/Octokit.Tests/Exceptions/ApiValidationExceptionTests.cs @@ -0,0 +1,63 @@ +using System.IO; +using System.Linq; +using System.Runtime.Serialization.Formatters.Binary; +using NSubstitute; +using Octokit.Http; +using Xunit; +using Xunit.Extensions; + +namespace Octokit.Tests.Exceptions +{ + public class ApiValidationExceptionTests + { + public class TheConstructor + { + [Fact] + public void CreatesGitHubErrorFromJsonResponse() + { + var response = Substitute.For(); + response.Body.Returns(@"{""errors"":[{""code"":""custom"",""field"":""key"",""message"":""key is " + + @"already in use"",""resource"":""PublicKey""}],""message"":""Validation Failed""}"); + + var exception = new ApiValidationException(response); + + Assert.Equal("Validation Failed", exception.ApiValidationError.Message); + Assert.Equal("key is already in use", exception.ApiValidationError.Errors.First().Message); + } + + [Theory] + [InlineData("")] + [InlineData(null)] + [InlineData("{{{{{")] + public void CreatesGitHubErrorIfResponseMessageIsNotValidJson(string responseContent) + { + var response = Substitute.For(); + response.Body.Returns(responseContent); + + var exception = new ApiValidationException(response); + + Assert.Equal(responseContent, exception.ApiValidationError.Message); + } + + [Fact] + public void CanPopulateObjectFromSerializedData() + { + var response = Substitute.For(); + response.Body.Returns(@"{""errors"":[{""code"":""custom"",""field"":""key"",""message"":""key is " + + @"already in use"",""resource"":""PublicKey""}],""message"":""Validation Failed""}"); + + var exception = new ApiValidationException(response); + + using (var stream = new MemoryStream()) + { + var formatter = new BinaryFormatter(); + formatter.Serialize(stream, exception); + stream.Position = 0; + var deserialized = (ApiValidationException)formatter.Deserialize(stream); + Assert.Equal("Validation Failed", deserialized.ApiValidationError.Message); + Assert.Equal("key is already in use", exception.ApiValidationError.Errors.First().Message); + } + } + } + } +} \ No newline at end of file diff --git a/Octokit.Tests/Http/ConnectionTests.cs b/Octokit.Tests/Http/ConnectionTests.cs index d6b6a161..13b20e19 100644 --- a/Octokit.Tests/Http/ConnectionTests.cs +++ b/Octokit.Tests/Http/ConnectionTests.cs @@ -120,25 +120,46 @@ namespace Octokit.Tests.Http Assert.Equal("user", resp.ApiInfo.AcceptedOauthScopes.First()); } - [Theory] - [InlineData(HttpStatusCode.Forbidden)] - [InlineData(HttpStatusCode.Unauthorized)] - public async Task ThrowsAuthenticationExceptionExceptionForAppropriateStatusCodes(HttpStatusCode statusCode) + [Fact] + public async Task ThrowsAuthorizationExceptionExceptionForUnauthorizedResponse() { var httpClient = Substitute.For(); - IResponse response = new ApiResponse { StatusCode = statusCode}; + IResponse response = new ApiResponse { StatusCode = HttpStatusCode.Unauthorized}; httpClient.Send(Args.Request).Returns(Task.FromResult(response)); var connection = new Connection(ExampleUri, Substitute.For(), httpClient, Substitute.For()); - var exception = await AssertEx.Throws( + var exception = await AssertEx.Throws( async () => await connection.GetAsync(new Uri("/endpoint", UriKind.Relative))); Assert.Equal("You must be authenticated to call this method. Either supply a login/password or an " + "oauth token.", exception.Message); } + + [Fact] + public async Task ThrowsApiValidationExceptionFor422Response() + { + var httpClient = Substitute.For(); + IResponse response = new ApiResponse + { + StatusCode = (HttpStatusCode)422, + Body = @"{""errors"":[{""code"":""custom"",""field"":""key"",""message"":""key is " + + @"already in use"",""resource"":""PublicKey""}],""message"":""Validation Failed""}" + }; + httpClient.Send(Args.Request).Returns(Task.FromResult(response)); + var connection = new Connection(ExampleUri, + Substitute.For(), + httpClient, + Substitute.For()); + + var exception = await AssertEx.Throws( + async () => await connection.GetAsync(new Uri("/endpoint", UriKind.Relative))); + + Assert.Equal("Validation Failed", exception.Message); + Assert.Equal("key is already in use", exception.ApiValidationError.Errors[0].Message); + } } public class TheGetHtmlMethod diff --git a/Octokit.Tests/Octokit.Tests.csproj b/Octokit.Tests/Octokit.Tests.csproj index 6d92611b..1a2d582e 100644 --- a/Octokit.Tests/Octokit.Tests.csproj +++ b/Octokit.Tests/Octokit.Tests.csproj @@ -52,6 +52,7 @@ + diff --git a/Octokit/AuthenticationException.cs b/Octokit/AuthenticationException.cs deleted file mode 100644 index 4de141ff..00000000 --- a/Octokit/AuthenticationException.cs +++ /dev/null @@ -1,54 +0,0 @@ -using System; -using System.Net; -using System.Runtime.Serialization; - -namespace Octokit -{ -#if !NETFX_CORE - [Serializable] -#endif - public class AuthenticationException : Exception - { - public AuthenticationException() - { - } - - public AuthenticationException(string message) - : base(message) - { - } - - public AuthenticationException(string message, HttpStatusCode statusCode) - : base(message) - { - StatusCode = statusCode; - } - - public AuthenticationException(string message, Exception innerException) - : base(message, innerException) - { - } - - public AuthenticationException(string message, Exception innerException, HttpStatusCode statusCode) - : base(message, innerException) - { - StatusCode = statusCode; - } - - public HttpStatusCode StatusCode { get; private set; } - -#if !NETFX_CORE - protected AuthenticationException(SerializationInfo info, StreamingContext context) : base(info, context) - { - if (info == null) return; - StatusCode = (HttpStatusCode)(info.GetInt32("StatusCode")); - } - - public override void GetObjectData(SerializationInfo info, StreamingContext context) - { - base.GetObjectData(info, context); - info.AddValue("StatusCode", StatusCode); - } -#endif - } -} diff --git a/Octokit/Clients/UsersClient.cs b/Octokit/Clients/UsersClient.cs index e210b4af..a8e67a3c 100644 --- a/Octokit/Clients/UsersClient.cs +++ b/Octokit/Clients/UsersClient.cs @@ -36,7 +36,7 @@ namespace Octokit.Clients /// /// Returns a for the current authenticated user. /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A public async Task Current() { @@ -47,7 +47,7 @@ namespace Octokit.Clients /// Update the specified . /// /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A public async Task Update(UserUpdate user) { diff --git a/Octokit/Exceptions/ApiException.cs b/Octokit/Exceptions/ApiException.cs new file mode 100644 index 00000000..9085c0a3 --- /dev/null +++ b/Octokit/Exceptions/ApiException.cs @@ -0,0 +1,58 @@ +using System; +using System.Net; +using System.Runtime.Serialization; + +namespace Octokit +{ +#if !NETFX_CORE + [Serializable] +#endif + public class ApiException : Exception + { + public ApiException() + { + } + + public ApiException(HttpStatusCode statusCode) + { + StatusCode = statusCode; + } + + public ApiException(string message) : base(message) + { + } + + public ApiException(string message, Exception innerException) + : base(message, innerException) + { + } + + public ApiException(string message, Exception innerException, HttpStatusCode statusCode) + : base(message, innerException) + { + StatusCode = statusCode; + } + + public ApiException(string message, HttpStatusCode statusCode) : base(message) + { + StatusCode = statusCode; + } + + public HttpStatusCode StatusCode { get; private set; } + +#if !NETFX_CORE + protected ApiException(SerializationInfo info, StreamingContext context) + : base(info, context) + { + if (info == null) return; + StatusCode = (HttpStatusCode)(info.GetInt32("HttpStatusCode")); + } + + public override void GetObjectData(SerializationInfo info, StreamingContext context) + { + base.GetObjectData(info, context); + info.AddValue("HttpStatusCode", StatusCode); + } +#endif + } +} diff --git a/Octokit/Exceptions/ApiValidationException.cs b/Octokit/Exceptions/ApiValidationException.cs new file mode 100644 index 00000000..d9b1ed99 --- /dev/null +++ b/Octokit/Exceptions/ApiValidationException.cs @@ -0,0 +1,87 @@ +using System; +using System.Diagnostics.CodeAnalysis; +using System.Runtime.Serialization; +using Octokit.Http; + +namespace Octokit +{ + /// + /// Represents a validation exception (HTTP STATUS: 422) returned from the API. + /// +#if !NETFX_CORE + [Serializable] +#endif + public class ApiValidationException : ApiException + { + // This needs to be hard-coded for translating GitHub error messages. + static readonly IJsonSerializer _jsonSerializer = new SimpleJsonSerializer(); + + public ApiValidationException() + { + } + + public ApiValidationException(string message) + : base(message) + { + } + + public ApiValidationException(string message, Exception innerException) + : base(message, innerException) + { + } + + public ApiValidationException(IResponse response) + : this(response, null) + { + } + + public ApiValidationException(IResponse response, Exception innerException) + : this(GetApiErrorFromExceptionMessage(response), innerException) + { + } + + protected ApiValidationException(ApiError apiValidationError, Exception innerException) + : base(apiValidationError != null ? apiValidationError.Message : "An API error occurred", innerException) + { + ApiValidationError = apiValidationError; + } + +#if !NETFX_CORE + protected ApiValidationException(SerializationInfo info, StreamingContext context) + : base(info, context) + { + ApiValidationError = GetApiErrorFromExceptionMessage(info.GetString("ApiValidationError")); + } + + public override void GetObjectData(SerializationInfo info, StreamingContext context) + { + base.GetObjectData(info, context); + + info.AddValue("ApiValidationError", _jsonSerializer.Serialize(ApiValidationError)); + } +#endif + + public ApiError ApiValidationError { get; private set; } + + static ApiError GetApiErrorFromExceptionMessage(IResponse response) + { + string responseBody = response != null ? response.Body : null; + return GetApiErrorFromExceptionMessage(responseBody); + } + + [SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes")] + static ApiError GetApiErrorFromExceptionMessage(string responseContent) + { + try + { + if (responseContent != null) + return _jsonSerializer.Deserialize(responseContent) ?? new ApiError { Message = responseContent }; + } + catch (Exception) + { + } + + return new ApiError { Message = responseContent }; + } + } +} diff --git a/Octokit/Exceptions/AuthorizationException.cs b/Octokit/Exceptions/AuthorizationException.cs new file mode 100644 index 00000000..55e3e616 --- /dev/null +++ b/Octokit/Exceptions/AuthorizationException.cs @@ -0,0 +1,36 @@ +using System; +using System.Net; +using System.Runtime.Serialization; + +namespace Octokit +{ + /// + /// Exception thrown when we receive an HttpStatusCode.Unauthorized (HTTP 401) response. + /// +#if !NETFX_CORE + [Serializable] +#endif + public class AuthorizationException : ApiException + { + public AuthorizationException() + { + } + + public AuthorizationException(string message) + : base(message, null, HttpStatusCode.Unauthorized) + { + } + + public AuthorizationException(string message, Exception innerException) + : base(message, innerException, HttpStatusCode.Unauthorized) + { + } + +#if !NETFX_CORE + protected AuthorizationException(SerializationInfo info, StreamingContext context) + : base(info, context) + { + } +#endif + } +} diff --git a/Octokit/GitHubModels.cs b/Octokit/GitHubModels.cs index 4d6b00f5..8d0895de 100644 --- a/Octokit/GitHubModels.cs +++ b/Octokit/GitHubModels.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Text; using System.Threading.Tasks; @@ -452,4 +453,24 @@ namespace Octokit public bool Primary { get; set; } } + + public class ApiError + { + public string Message { get; set; } + + // TODO: This ought to be an IReadOnlyList but we need to add support to SimpleJson for that. + [SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")] + public IList Errors { get; set; } + } + + public class ApiErrorDetail + { + public string Message { get; set; } + + public string Code { get; set; } + + public string Field { get; set; } + + public string Resource { get; set; } + } } diff --git a/Octokit/Http/Connection.cs b/Octokit/Http/Connection.cs index 1fd4c589..f2d535ba 100644 --- a/Octokit/Http/Connection.cs +++ b/Octokit/Http/Connection.cs @@ -176,13 +176,28 @@ namespace Octokit.Http static void HandleErrors(IResponse response) { - if (response.StatusCode == HttpStatusCode.Unauthorized || response.StatusCode == HttpStatusCode.Forbidden) - throw new AuthenticationException("You must be authenticated to call this method. Either supply a " + - "login/password or an oauth token.", response.StatusCode); + if (response.StatusCode == HttpStatusCode.Unauthorized) + throw new AuthorizationException("You must be authenticated to call this method. Either supply a " + + "login/password or an oauth token."); + + if (response.StatusCode == HttpStatusCode.Forbidden) + { + throw new ApiException("Request Forbidden", response.StatusCode); + } if (response.StatusCode == HttpStatusCode.RequestTimeout) { - throw new WebException("Request Timed Out", WebExceptionStatus.Timeout); + throw new ApiException("Request Timed Out", response.StatusCode); + } + + if ((int)response.StatusCode == 422) + { + throw new ApiValidationException(response); + } + + if ((int)response.StatusCode >= 400) + { + throw new ApiException(response.Body, response.StatusCode); } } } diff --git a/Octokit/IRepositoriesClient.cs b/Octokit/IRepositoriesClient.cs index b417028b..347e1ef8 100644 --- a/Octokit/IRepositoriesClient.cs +++ b/Octokit/IRepositoriesClient.cs @@ -21,7 +21,7 @@ namespace Octokit /// /// The default page size on GitHub.com is 30. /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A of . [SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification = "Makes a network request")] diff --git a/Octokit/ISshKeysClient.cs b/Octokit/ISshKeysClient.cs index c94aa2f3..ec4eb68e 100644 --- a/Octokit/ISshKeysClient.cs +++ b/Octokit/ISshKeysClient.cs @@ -24,7 +24,7 @@ namespace Octokit /// /// Retrieves the for the specified id. /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A of . [SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification = "Makes a network request")] @@ -34,7 +34,7 @@ namespace Octokit /// Update the specified . /// /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A Task Create(SshKeyUpdate key); @@ -43,7 +43,7 @@ namespace Octokit /// /// /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A Task Update(int id, SshKeyUpdate key); @@ -51,7 +51,7 @@ namespace Octokit /// Update the specified . /// /// The id of the SSH key - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A Task Delete(int id); } diff --git a/Octokit/IUsersClient.cs b/Octokit/IUsersClient.cs index ed6a9cd1..824a4325 100644 --- a/Octokit/IUsersClient.cs +++ b/Octokit/IUsersClient.cs @@ -17,7 +17,7 @@ namespace Octokit /// /// Returns a for the current authenticated user. /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A Task Current(); @@ -25,7 +25,7 @@ namespace Octokit /// Update the specified . /// /// - /// Thrown if the client is not authenticated. + /// Thrown if the client is not authenticated. /// A Task Update(UserUpdate user); diff --git a/Octokit/Octokit.csproj b/Octokit/Octokit.csproj index 887fc4b6..dd4104b5 100644 --- a/Octokit/Octokit.csproj +++ b/Octokit/Octokit.csproj @@ -77,6 +77,7 @@ + @@ -86,6 +87,7 @@ + @@ -119,7 +121,7 @@ - + Code diff --git a/Octokit/OctokitRT.csproj b/Octokit/OctokitRT.csproj index c9654279..3e9254aa 100644 --- a/Octokit/OctokitRT.csproj +++ b/Octokit/OctokitRT.csproj @@ -113,6 +113,9 @@ + + + @@ -138,7 +141,6 @@ -