From db57a92a7e216b3984e3bb333f9c8664b088c9a4 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Tue, 16 Jun 2015 22:29:02 +0930 Subject: [PATCH 1/4] sketching out the exception necessary when raising specific merge exceptions --- Octokit/Clients/PullRequestsClient.cs | 19 ++++++- .../PullRequestMismatchException.cs | 52 +++++++++++++++++++ .../PullRequestNotMergeableException.cs | 48 +++++++++++++++++ Octokit/Octokit-Mono.csproj | 2 + Octokit/Octokit-MonoAndroid.csproj | 4 +- Octokit/Octokit-Monotouch.csproj | 2 + Octokit/Octokit-Portable.csproj | 2 + Octokit/Octokit-netcore45.csproj | 4 +- Octokit/Octokit.csproj | 2 + 9 files changed, 132 insertions(+), 3 deletions(-) create mode 100644 Octokit/Exceptions/PullRequestMismatchException.cs create mode 100644 Octokit/Exceptions/PullRequestNotMergeableException.cs diff --git a/Octokit/Clients/PullRequestsClient.cs b/Octokit/Clients/PullRequestsClient.cs index 3cce9f30..fc1aeb35 100644 --- a/Octokit/Clients/PullRequestsClient.cs +++ b/Octokit/Clients/PullRequestsClient.cs @@ -115,7 +115,24 @@ namespace Octokit Ensure.ArgumentNotNullOrEmptyString(name, "name"); Ensure.ArgumentNotNull(mergePullRequest, "mergePullRequest"); - return ApiConnection.Put(ApiUrls.MergePullRequest(owner, name, number), mergePullRequest); + try + { + return ApiConnection.Put(ApiUrls.MergePullRequest(owner, name, number), mergePullRequest); + } + catch (ApiException ex) + { + if (ex.StatusCode == HttpStatusCode.MethodNotAllowed) + { + throw new PullRequestNotMergeableException(); + } + + if (ex.StatusCode == HttpStatusCode.Conflict) + { + throw new PullRequestMismatchException(); + } + + throw; + } } /// diff --git a/Octokit/Exceptions/PullRequestMismatchException.cs b/Octokit/Exceptions/PullRequestMismatchException.cs new file mode 100644 index 00000000..8a77d6dc --- /dev/null +++ b/Octokit/Exceptions/PullRequestMismatchException.cs @@ -0,0 +1,52 @@ +using System; +using System.Runtime.Serialization; + +namespace Octokit +{ + /// + /// Represents an error that occurs when the specified SHA + /// doesn't match the current pull request's HEAD + /// +#if !NETFX_CORE + [Serializable] +#endif + public class PullRequestMismatchException : Exception + { + public PullRequestMismatchException() + : base("The merge operation specified a SHA which didn't match " + + "the SHA of the pull request's HEAD") + { + } + + public PullRequestMismatchException(string message) + : base(message) + { + } + + + public PullRequestMismatchException(string message, Exception innerException) + : base(message, innerException) + { + } + +#if !NETFX_CORE + /// + /// Constructs an instance of PullRequestNotMergeableException. + /// + /// + /// The that holds the + /// serialized object data about the exception being thrown. + /// + /// + /// The that contains + /// contextual information about the source or destination. + /// + protected PullRequestMismatchException(SerializationInfo info, StreamingContext context) + : base(info, context) + { + } +#endif + } + + +} diff --git a/Octokit/Exceptions/PullRequestNotMergeableException.cs b/Octokit/Exceptions/PullRequestNotMergeableException.cs new file mode 100644 index 00000000..e24c2d36 --- /dev/null +++ b/Octokit/Exceptions/PullRequestNotMergeableException.cs @@ -0,0 +1,48 @@ +using System; +using System.Runtime.Serialization; + +namespace Octokit +{ + /// + /// Represents an error that occurs when the pull request is in an + /// unmergeable state + /// +#if !NETFX_CORE + [Serializable] +#endif + public class PullRequestNotMergeableException : Exception + { + public PullRequestNotMergeableException() + : base("The pull request is not in a mergeable state") + { + } + + public PullRequestNotMergeableException(string message) + : base(message) + { + } + + public PullRequestNotMergeableException(string message, Exception innerException) + : base(message, innerException) + { + } + +#if !NETFX_CORE + /// + /// Constructs an instance of PullRequestNotMergeableException. + /// + /// + /// The that holds the + /// serialized object data about the exception being thrown. + /// + /// + /// The that contains + /// contextual information about the source or destination. + /// + protected PullRequestNotMergeableException(SerializationInfo info, StreamingContext context) + : base(info, context) + { + } +#endif + } +} diff --git a/Octokit/Octokit-Mono.csproj b/Octokit/Octokit-Mono.csproj index 3d6206fc..7c25f716 100644 --- a/Octokit/Octokit-Mono.csproj +++ b/Octokit/Octokit-Mono.csproj @@ -403,6 +403,8 @@ + + diff --git a/Octokit/Octokit-MonoAndroid.csproj b/Octokit/Octokit-MonoAndroid.csproj index ea68c7d3..97b56f7b 100644 --- a/Octokit/Octokit-MonoAndroid.csproj +++ b/Octokit/Octokit-MonoAndroid.csproj @@ -411,6 +411,8 @@ + + - \ No newline at end of file + diff --git a/Octokit/Octokit-Monotouch.csproj b/Octokit/Octokit-Monotouch.csproj index 7b228d6d..cd135963 100644 --- a/Octokit/Octokit-Monotouch.csproj +++ b/Octokit/Octokit-Monotouch.csproj @@ -407,6 +407,8 @@ + + diff --git a/Octokit/Octokit-Portable.csproj b/Octokit/Octokit-Portable.csproj index 28280e54..c5f57eb6 100644 --- a/Octokit/Octokit-Portable.csproj +++ b/Octokit/Octokit-Portable.csproj @@ -400,6 +400,8 @@ + + diff --git a/Octokit/Octokit-netcore45.csproj b/Octokit/Octokit-netcore45.csproj index 6f98071f..8bf0101a 100644 --- a/Octokit/Octokit-netcore45.csproj +++ b/Octokit/Octokit-netcore45.csproj @@ -404,6 +404,8 @@ + + @@ -418,4 +420,4 @@ --> - \ No newline at end of file + diff --git a/Octokit/Octokit.csproj b/Octokit/Octokit.csproj index 3708c95b..46462ba3 100644 --- a/Octokit/Octokit.csproj +++ b/Octokit/Octokit.csproj @@ -72,6 +72,8 @@ + + From 37c7049f0ee543c1dd03de5c9b06e603a36479ad Mon Sep 17 00:00:00 2001 From: Victor Castillo Escoto Date: Thu, 8 Oct 2015 12:49:19 -0500 Subject: [PATCH 2/4] Add System.Net namespace used to check for HttpStatusCode in PullRequestClient.Task Merge(string, string, int, MergePullRequest) --- Octokit/Clients/PullRequestsClient.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Octokit/Clients/PullRequestsClient.cs b/Octokit/Clients/PullRequestsClient.cs index fc1aeb35..b650be46 100644 --- a/Octokit/Clients/PullRequestsClient.cs +++ b/Octokit/Clients/PullRequestsClient.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Net; using System.Threading.Tasks; namespace Octokit From 590d01a06b4547984f801e6c9ec69535d87cfaaa Mon Sep 17 00:00:00 2001 From: Victor Castillo Escoto Date: Thu, 8 Oct 2015 13:33:10 -0500 Subject: [PATCH 3/4] Add tests for merge exceptions to PullRequestsClientTests --- .../Clients/PullRequestsClientTests.cs | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs b/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs index 003d9bcb..d021995f 100644 --- a/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs +++ b/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs @@ -227,7 +227,7 @@ public class PullRequestsClientTests : IDisposable } [IntegrationTest] - public async Task CannotBeMerged() + public async Task CannotBeMergedDueMismatchConflict() { await CreateTheWorld(); var fakeSha = new string('f', 40); @@ -236,9 +236,29 @@ public class PullRequestsClientTests : IDisposable var pullRequest = await _fixture.Create(Helper.UserName, _context.RepositoryName, newPullRequest); var merge = new MergePullRequest { Sha = fakeSha }; - var ex = await Assert.ThrowsAsync(() => _fixture.Merge(Helper.UserName, _context.RepositoryName, pullRequest.Number, merge)); + var ex = await Assert.ThrowsAsync(() => _fixture.Merge(Helper.UserName, _context.RepositoryName, pullRequest.Number, merge)); - Assert.True(ex.ApiError.Message.StartsWith("Head branch was modified")); + //merge exceptions don't inherit from ApiException so ApiError is not available + Assert.True(ex.Message.StartsWith("The merge operation specified a SHA which didn't match")); + } + + [IntegrationTest] + public async Task CannotBeMergedDueNotInMergeableState() + { + await CreateTheWorld(); + + var master = await _github.GitDatabase.Reference.Get(Helper.UserName, _context.RepositoryName, "heads/master"); + var newMasterTree = await CreateTree(new Dictionary { { "README.md", "Hello World, we meet again!" } }); + await CreateCommit("baseline for pull request", newMasterTree.Sha, master.Object.Sha); + + var newPullRequest = new NewPullRequest("a pull request", branchName, "master"); + var pullRequest = await _fixture.Create(Helper.UserName, _context.RepositoryName, newPullRequest); + + var merge = new MergePullRequest { Sha = pullRequest.Head.Sha }; + var ex = await Assert.ThrowsAsync(() => _fixture.Merge(Helper.UserName, _context.RepositoryName, pullRequest.Number, merge)); + + //merge exceptions don't inherit from ApiException so ApiError is not available + Assert.True(ex.Message.Equals("The pull request is not in a mergeable state")); } [IntegrationTest] From 25d39a055e459b45a6564d4c7a883491ca2f6ac9 Mon Sep 17 00:00:00 2001 From: Victor Castillo Escoto Date: Tue, 13 Oct 2015 19:51:08 -0500 Subject: [PATCH 4/4] Make new merge exceptions inherit from 'Octokit.ApiException' Affect 'Octokit.PullRequestNotMergeableException' and 'Octokit.PullRequestMismatchException' --- .../Clients/PullRequestsClientTests.cs | 6 ++-- Octokit/Clients/PullRequestsClient.cs | 4 +-- .../PullRequestMismatchException.cs | 36 ++++++++++++------- .../PullRequestNotMergeableException.cs | 32 ++++++++++++----- 4 files changed, 52 insertions(+), 26 deletions(-) diff --git a/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs b/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs index d021995f..94e6387b 100644 --- a/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs +++ b/Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs @@ -238,8 +238,7 @@ public class PullRequestsClientTests : IDisposable var merge = new MergePullRequest { Sha = fakeSha }; var ex = await Assert.ThrowsAsync(() => _fixture.Merge(Helper.UserName, _context.RepositoryName, pullRequest.Number, merge)); - //merge exceptions don't inherit from ApiException so ApiError is not available - Assert.True(ex.Message.StartsWith("The merge operation specified a SHA which didn't match")); + Assert.True(ex.Message.StartsWith("Head branch was modified")); } [IntegrationTest] @@ -257,8 +256,7 @@ public class PullRequestsClientTests : IDisposable var merge = new MergePullRequest { Sha = pullRequest.Head.Sha }; var ex = await Assert.ThrowsAsync(() => _fixture.Merge(Helper.UserName, _context.RepositoryName, pullRequest.Number, merge)); - //merge exceptions don't inherit from ApiException so ApiError is not available - Assert.True(ex.Message.Equals("The pull request is not in a mergeable state")); + Assert.True(ex.Message.Equals("Pull Request is not mergeable")); } [IntegrationTest] diff --git a/Octokit/Clients/PullRequestsClient.cs b/Octokit/Clients/PullRequestsClient.cs index b650be46..b73da1fb 100644 --- a/Octokit/Clients/PullRequestsClient.cs +++ b/Octokit/Clients/PullRequestsClient.cs @@ -124,12 +124,12 @@ namespace Octokit { if (ex.StatusCode == HttpStatusCode.MethodNotAllowed) { - throw new PullRequestNotMergeableException(); + throw new PullRequestNotMergeableException(ex.HttpResponse); } if (ex.StatusCode == HttpStatusCode.Conflict) { - throw new PullRequestMismatchException(); + throw new PullRequestMismatchException(ex.HttpResponse); } throw; diff --git a/Octokit/Exceptions/PullRequestMismatchException.cs b/Octokit/Exceptions/PullRequestMismatchException.cs index 8a77d6dc..f16504b5 100644 --- a/Octokit/Exceptions/PullRequestMismatchException.cs +++ b/Octokit/Exceptions/PullRequestMismatchException.cs @@ -1,4 +1,7 @@ using System; +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Net; using System.Runtime.Serialization; namespace Octokit @@ -10,28 +13,39 @@ namespace Octokit #if !NETFX_CORE [Serializable] #endif - public class PullRequestMismatchException : Exception + [SuppressMessage("Microsoft.Design", "CA1032:ImplementStandardExceptionConstructors", + Justification = "These exceptions are specific to the GitHub API and not general purpose exceptions")] + public class PullRequestMismatchException : ApiException { - public PullRequestMismatchException() - : base("The merge operation specified a SHA which didn't match " + - "the SHA of the pull request's HEAD") + /// + /// Constructs an instace of . + /// + /// + public PullRequestMismatchException(IResponse response) : this(response, null) { } - public PullRequestMismatchException(string message) - : base(message) + /// + /// Constructs an instance of . + /// + /// The HTTP payload from the server + /// The inner exception + public PullRequestMismatchException(IResponse response, Exception innerException) + : base(response, innerException) { + Debug.Assert(response != null && response.StatusCode == HttpStatusCode.Conflict, + "PullRequestMismatchException created with the wrong HTTP status code"); } - - public PullRequestMismatchException(string message, Exception innerException) - : base(message, innerException) + public override string Message { + //https://developer.github.com/v3/pulls/#response-if-sha-was-provided-and-pull-request-head-did-not-match + get { return ApiErrorMessageSafe ?? "Head branch was modified. Review and try the merge again."; } } #if !NETFX_CORE /// - /// Constructs an instance of PullRequestNotMergeableException. + /// Constructs an instance of . /// /// /// The that holds the @@ -47,6 +61,4 @@ namespace Octokit } #endif } - - } diff --git a/Octokit/Exceptions/PullRequestNotMergeableException.cs b/Octokit/Exceptions/PullRequestNotMergeableException.cs index e24c2d36..ca4143dc 100644 --- a/Octokit/Exceptions/PullRequestNotMergeableException.cs +++ b/Octokit/Exceptions/PullRequestNotMergeableException.cs @@ -1,4 +1,7 @@ using System; +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Net; using System.Runtime.Serialization; namespace Octokit @@ -10,26 +13,39 @@ namespace Octokit #if !NETFX_CORE [Serializable] #endif - public class PullRequestNotMergeableException : Exception + [SuppressMessage("Microsoft.Design", "CA1032:ImplementStandardExceptionConstructors", + Justification = "These exceptions are specific to the GitHub API and not general purpose exceptions")] + public class PullRequestNotMergeableException : ApiException { - public PullRequestNotMergeableException() - : base("The pull request is not in a mergeable state") + /// + /// Constructs an instance of the class. + /// + /// The HTTP payload from the server + public PullRequestNotMergeableException(IResponse response) : this(response, null) { } - public PullRequestNotMergeableException(string message) - : base(message) + /// + /// Constructs an instance of the class. + /// + /// The HTTP payload from the server + /// The inner exception + public PullRequestNotMergeableException(IResponse response, Exception innerException) + : base(response, innerException) { + Debug.Assert(response != null && response.StatusCode == HttpStatusCode.MethodNotAllowed, + "PullRequestNotMergeableException created with the wrong HTTP status code"); } - public PullRequestNotMergeableException(string message, Exception innerException) - : base(message, innerException) + public override string Message { + //https://developer.github.com/v3/pulls/#response-if-merge-cannot-be-performed + get { return ApiErrorMessageSafe ?? "Pull Request is not mergeable"; } } #if !NETFX_CORE /// - /// Constructs an instance of PullRequestNotMergeableException. + /// Constructs an instance of . /// /// /// The that holds the