From d458f8c83db25bf278e24787cd9a26934e9d2e36 Mon Sep 17 00:00:00 2001 From: David Alpert Date: Fri, 31 Jul 2015 13:40:54 -0500 Subject: [PATCH 1/5] adding a test: TeamClient.AddMembership(..) using a real ApiConnection throws an ArgumentNullExceptionValue --- Octokit.Tests/Clients/TeamsClientTests.cs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Octokit.Tests/Clients/TeamsClientTests.cs b/Octokit.Tests/Clients/TeamsClientTests.cs index 52fbe985a8..cf49349aab 100644 --- a/Octokit.Tests/Clients/TeamsClientTests.cs +++ b/Octokit.Tests/Clients/TeamsClientTests.cs @@ -152,6 +152,22 @@ public async Task RequestsTheCorrectUrl() Args.Object); } + [Fact] + public async Task AllowsEmptyBody() + { + var connection = Substitute.For(); + + var apiConnection = new ApiConnection(connection); + + var client = new TeamsClient(apiConnection); + + await client.AddMembership(1, "user"); + + connection.Received().Put>( + Arg.Is(u => u.ToString() == "teams/1/memberships/user"), + Arg.Is(u => u == null)); + } + [Fact] public async Task EnsuresNonNullOrEmptyLogin() { From 3f1bd13bb57e0e6db9d529b62b9b071732119a53 Mon Sep 17 00:00:00 2001 From: David Alpert Date: Fri, 31 Jul 2015 17:48:10 -0500 Subject: [PATCH 2/5] replacing null with an empty object hung off ApiConnection as suggested by @haacked --- Octokit.Tests/Clients/TeamsClientTests.cs | 2 +- Octokit/Clients/TeamsClient.cs | 2 +- Octokit/Http/ApiConnection.cs | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Octokit.Tests/Clients/TeamsClientTests.cs b/Octokit.Tests/Clients/TeamsClientTests.cs index cf49349aab..a99513e06b 100644 --- a/Octokit.Tests/Clients/TeamsClientTests.cs +++ b/Octokit.Tests/Clients/TeamsClientTests.cs @@ -165,7 +165,7 @@ public async Task AllowsEmptyBody() connection.Received().Put>( Arg.Is(u => u.ToString() == "teams/1/memberships/user"), - Arg.Is(u => u == null)); + Arg.Is(u => u == ApiConnection.EmptyBody)); } [Fact] diff --git a/Octokit/Clients/TeamsClient.cs b/Octokit/Clients/TeamsClient.cs index 4afab495d2..d2a5288cf5 100644 --- a/Octokit/Clients/TeamsClient.cs +++ b/Octokit/Clients/TeamsClient.cs @@ -164,7 +164,7 @@ public async Task AddMembership(int id, string login) try { - response = await ApiConnection.Put>(endpoint, null); + response = await ApiConnection.Put>(endpoint, Octokit.ApiConnection.EmptyBody); } catch (NotFoundException) { diff --git a/Octokit/Http/ApiConnection.cs b/Octokit/Http/ApiConnection.cs index 9b1656f5bf..b9db71d347 100644 --- a/Octokit/Http/ApiConnection.cs +++ b/Octokit/Http/ApiConnection.cs @@ -15,6 +15,9 @@ public class ApiConnection : IApiConnection { readonly IApiPagination _pagination; + [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2211:NonConstantFieldsShouldNotBeVisible")] + public static object EmptyBody = new object(); + /// /// Initializes a new instance of the class. /// From ef8a6fe2cccf7a817799a3d5b8acba2c0a672612 Mon Sep 17 00:00:00 2001 From: David Alpert Date: Fri, 31 Jul 2015 14:25:52 -0500 Subject: [PATCH 3/5] refactoring ThePutMethod tests to clearly use the same body throughout --- Octokit.Tests/Http/ConnectionTests.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Octokit.Tests/Http/ConnectionTests.cs b/Octokit.Tests/Http/ConnectionTests.cs index 02f1601ee7..d969cbef4a 100644 --- a/Octokit.Tests/Http/ConnectionTests.cs +++ b/Octokit.Tests/Http/ConnectionTests.cs @@ -347,7 +347,8 @@ public class ThePutMethod [Fact] public async Task MakesPutRequestWithData() { - string data = SimpleJson.SerializeObject(new object()); + var body = new object(); + var expectedBody = SimpleJson.SerializeObject(body); var httpClient = Substitute.For(); IResponse response = new Response(); httpClient.Send(Args.Request, Args.CancellationToken).Returns(Task.FromResult(response)); @@ -357,11 +358,11 @@ public async Task MakesPutRequestWithData() httpClient, Substitute.For()); - await connection.Put(new Uri("endpoint", UriKind.Relative), new object()); + await connection.Put(new Uri("endpoint", UriKind.Relative), body); httpClient.Received(1).Send(Arg.Is(req => req.BaseAddress == _exampleUri && - (string)req.Body == data && + (string)req.Body == expectedBody && req.Method == HttpMethod.Put && req.ContentType == "application/x-www-form-urlencoded" && req.Endpoint == new Uri("endpoint", UriKind.Relative)), Args.CancellationToken); @@ -370,7 +371,8 @@ public async Task MakesPutRequestWithData() [Fact] public async Task MakesPutRequestWithDataAndTwoFactor() { - string data = SimpleJson.SerializeObject(new object()); + var body = new object(); + var expectedBody = SimpleJson.SerializeObject(body); var httpClient = Substitute.For(); IResponse response = new Response(); httpClient.Send(Args.Request, Args.CancellationToken).Returns(Task.FromResult(response)); @@ -380,11 +382,11 @@ public async Task MakesPutRequestWithDataAndTwoFactor() httpClient, Substitute.For()); - await connection.Put(new Uri("endpoint", UriKind.Relative), new object(), "two-factor"); + await connection.Put(new Uri("endpoint", UriKind.Relative), body, "two-factor"); httpClient.Received(1).Send(Arg.Is(req => req.BaseAddress == _exampleUri && - (string)req.Body == data && + (string)req.Body == expectedBody && req.Method == HttpMethod.Put && req.Headers["X-GitHub-OTP"] == "two-factor" && req.ContentType == "application/x-www-form-urlencoded" && From c54dfa196fb2fb42504c66743010c3ca38427617 Mon Sep 17 00:00:00 2001 From: David Alpert Date: Fri, 31 Jul 2015 14:55:52 -0500 Subject: [PATCH 4/5] adding passing tests: ConnectionTests.ThePutMethod can submit PUT requests with no data (i.e. empty body). --- Octokit.Tests/Http/ConnectionTests.cs | 49 +++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/Octokit.Tests/Http/ConnectionTests.cs b/Octokit.Tests/Http/ConnectionTests.cs index d969cbef4a..64fae48a84 100644 --- a/Octokit.Tests/Http/ConnectionTests.cs +++ b/Octokit.Tests/Http/ConnectionTests.cs @@ -368,6 +368,31 @@ public async Task MakesPutRequestWithData() req.Endpoint == new Uri("endpoint", UriKind.Relative)), Args.CancellationToken); } + [Fact] + public async Task MakesPutRequestWithNoData() + { + var body = ApiConnection.EmptyBody; + var expectedBody = SimpleJson.SerializeObject(body); + var httpClient = Substitute.For(); + IResponse response = new Response(); + httpClient.Send(Args.Request, Args.CancellationToken).Returns(Task.FromResult(response)); + var connection = new Connection(new ProductHeaderValue("OctokitTests"), + _exampleUri, + Substitute.For(), + httpClient, + Substitute.For()); + + await connection.Put(new Uri("endpoint", UriKind.Relative), body); + + Console.WriteLine(expectedBody); + + httpClient.Received(1).Send(Arg.Is(req => + req.BaseAddress == _exampleUri && + (string)req.Body == expectedBody && + req.Method == HttpMethod.Put && + req.Endpoint == new Uri("endpoint", UriKind.Relative)), Args.CancellationToken); + } + [Fact] public async Task MakesPutRequestWithDataAndTwoFactor() { @@ -392,6 +417,30 @@ public async Task MakesPutRequestWithDataAndTwoFactor() req.ContentType == "application/x-www-form-urlencoded" && req.Endpoint == new Uri("endpoint", UriKind.Relative)), Args.CancellationToken); } + + [Fact] + public async Task MakesPutRequestWithNoDataAndTwoFactor() + { + var body = ApiConnection.EmptyBody ; + var expectedBody = SimpleJson.SerializeObject(body); + var httpClient = Substitute.For(); + IResponse response = new Response(); + httpClient.Send(Args.Request, Args.CancellationToken).Returns(Task.FromResult(response)); + var connection = new Connection(new ProductHeaderValue("OctokitTests"), + _exampleUri, + Substitute.For(), + httpClient, + Substitute.For()); + + await connection.Put(new Uri("endpoint", UriKind.Relative), body, "two-factor"); + + httpClient.Received(1).Send(Arg.Is(req => + req.BaseAddress == _exampleUri && + (string)req.Body == expectedBody && + req.Method == HttpMethod.Put && + req.Headers["X-GitHub-OTP"] == "two-factor" && + req.Endpoint == new Uri("endpoint", UriKind.Relative)), Args.CancellationToken); + } } public class ThePostMethod From b4d71c8fe936565ffec36e5b8d2f6df988d8873d Mon Sep 17 00:00:00 2001 From: David Alpert Date: Fri, 31 Jul 2015 23:43:37 -0500 Subject: [PATCH 5/5] refactoring ApiConnection.EmptyBody to RequestBody.Empty to break the inverse dependency from Connection to ApiConnection --- Octokit.Tests/Clients/TeamsClientTests.cs | 2 +- Octokit.Tests/Http/ConnectionTests.cs | 4 ++-- Octokit/Clients/TeamsClient.cs | 2 +- Octokit/Http/ApiConnection.cs | 3 --- Octokit/Http/RequestBody.cs | 12 ++++++++++++ Octokit/Octokit-Mono.csproj | 1 + Octokit/Octokit-MonoAndroid.csproj | 1 + Octokit/Octokit-Monotouch.csproj | 1 + Octokit/Octokit-Portable.csproj | 1 + Octokit/Octokit-netcore45.csproj | 1 + Octokit/Octokit.csproj | 1 + 11 files changed, 22 insertions(+), 7 deletions(-) create mode 100644 Octokit/Http/RequestBody.cs diff --git a/Octokit.Tests/Clients/TeamsClientTests.cs b/Octokit.Tests/Clients/TeamsClientTests.cs index a99513e06b..3ca76d5f8a 100644 --- a/Octokit.Tests/Clients/TeamsClientTests.cs +++ b/Octokit.Tests/Clients/TeamsClientTests.cs @@ -165,7 +165,7 @@ public async Task AllowsEmptyBody() connection.Received().Put>( Arg.Is(u => u.ToString() == "teams/1/memberships/user"), - Arg.Is(u => u == ApiConnection.EmptyBody)); + Arg.Is(u => u == RequestBody.Empty)); } [Fact] diff --git a/Octokit.Tests/Http/ConnectionTests.cs b/Octokit.Tests/Http/ConnectionTests.cs index 64fae48a84..eebc34d821 100644 --- a/Octokit.Tests/Http/ConnectionTests.cs +++ b/Octokit.Tests/Http/ConnectionTests.cs @@ -371,7 +371,7 @@ public async Task MakesPutRequestWithData() [Fact] public async Task MakesPutRequestWithNoData() { - var body = ApiConnection.EmptyBody; + var body = RequestBody.Empty; var expectedBody = SimpleJson.SerializeObject(body); var httpClient = Substitute.For(); IResponse response = new Response(); @@ -421,7 +421,7 @@ public async Task MakesPutRequestWithDataAndTwoFactor() [Fact] public async Task MakesPutRequestWithNoDataAndTwoFactor() { - var body = ApiConnection.EmptyBody ; + var body = RequestBody.Empty; var expectedBody = SimpleJson.SerializeObject(body); var httpClient = Substitute.For(); IResponse response = new Response(); diff --git a/Octokit/Clients/TeamsClient.cs b/Octokit/Clients/TeamsClient.cs index d2a5288cf5..002358142f 100644 --- a/Octokit/Clients/TeamsClient.cs +++ b/Octokit/Clients/TeamsClient.cs @@ -164,7 +164,7 @@ public async Task AddMembership(int id, string login) try { - response = await ApiConnection.Put>(endpoint, Octokit.ApiConnection.EmptyBody); + response = await ApiConnection.Put>(endpoint, RequestBody.Empty); } catch (NotFoundException) { diff --git a/Octokit/Http/ApiConnection.cs b/Octokit/Http/ApiConnection.cs index b9db71d347..9b1656f5bf 100644 --- a/Octokit/Http/ApiConnection.cs +++ b/Octokit/Http/ApiConnection.cs @@ -15,9 +15,6 @@ public class ApiConnection : IApiConnection { readonly IApiPagination _pagination; - [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2211:NonConstantFieldsShouldNotBeVisible")] - public static object EmptyBody = new object(); - /// /// Initializes a new instance of the class. /// diff --git a/Octokit/Http/RequestBody.cs b/Octokit/Http/RequestBody.cs new file mode 100644 index 0000000000..346e71d120 --- /dev/null +++ b/Octokit/Http/RequestBody.cs @@ -0,0 +1,12 @@ +namespace Octokit +{ + /// + /// Container for the static method that represents an + /// intentional empty request body to avoid overloading null. + /// + public static class RequestBody + { + [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2211:NonConstantFieldsShouldNotBeVisible")] + public static object Empty = new object(); + } +} \ No newline at end of file diff --git a/Octokit/Octokit-Mono.csproj b/Octokit/Octokit-Mono.csproj index 64daa00563..8b5eedaca9 100644 --- a/Octokit/Octokit-Mono.csproj +++ b/Octokit/Octokit-Mono.csproj @@ -399,6 +399,7 @@ + \ No newline at end of file diff --git a/Octokit/Octokit-MonoAndroid.csproj b/Octokit/Octokit-MonoAndroid.csproj index 9002587a63..a34a7b1ef9 100644 --- a/Octokit/Octokit-MonoAndroid.csproj +++ b/Octokit/Octokit-MonoAndroid.csproj @@ -415,6 +415,7 @@ + \ No newline at end of file diff --git a/Octokit/Octokit-Monotouch.csproj b/Octokit/Octokit-Monotouch.csproj index 607ec59518..3f4b6f9cfa 100644 --- a/Octokit/Octokit-Monotouch.csproj +++ b/Octokit/Octokit-Monotouch.csproj @@ -408,6 +408,7 @@ + diff --git a/Octokit/Octokit-Portable.csproj b/Octokit/Octokit-Portable.csproj index 74f211dfd1..36e840bd46 100644 --- a/Octokit/Octokit-Portable.csproj +++ b/Octokit/Octokit-Portable.csproj @@ -398,6 +398,7 @@ + diff --git a/Octokit/Octokit-netcore45.csproj b/Octokit/Octokit-netcore45.csproj index 944bb68159..17d8bae84e 100644 --- a/Octokit/Octokit-netcore45.csproj +++ b/Octokit/Octokit-netcore45.csproj @@ -402,6 +402,7 @@ + diff --git a/Octokit/Octokit.csproj b/Octokit/Octokit.csproj index 60a9607f7c..760b5bfe67 100644 --- a/Octokit/Octokit.csproj +++ b/Octokit/Octokit.csproj @@ -85,6 +85,7 @@ +