From d186bd887006461012c6431fbd8d563557312e14 Mon Sep 17 00:00:00 2001 From: jericho Date: Sat, 14 Oct 2023 14:49:29 -0400 Subject: [PATCH 1/6] (GH-2802) Add unit test to demonstrate the problem This unit test currently fails which demonstrates the problem and it should pass when I am done with the PR --- .../Clients/RepositoryContentsClientTests.cs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/Octokit.Tests/Clients/RepositoryContentsClientTests.cs b/Octokit.Tests/Clients/RepositoryContentsClientTests.cs index 1d6046aa83..8dec410043 100644 --- a/Octokit.Tests/Clients/RepositoryContentsClientTests.cs +++ b/Octokit.Tests/Clients/RepositoryContentsClientTests.cs @@ -4,6 +4,8 @@ using NSubstitute; using Xunit; using System.Collections.Generic; +using System.Net; +using Octokit.Internal; namespace Octokit.Tests.Clients { @@ -882,6 +884,27 @@ public async Task EnsuresNonNullArguments() await Assert.ThrowsAsync(() => client.GetArchive(1, ArchiveFormat.Tarball, "ref", TimeSpan.Zero)); } + + [Fact] + public async Task ReturnsExpectedContent() + { + var headers = new Dictionary(); + var response = TestSetup.CreateResponse(HttpStatusCode.OK, new byte[] { 1, 2, 3, 4 }, headers); + var responseTask = Task.FromResult>(new ApiResponse(response)); + + var connection = Substitute.For(); + connection.Get(Arg.Is(u => u.ToString() == "repos/org/repo/tarball/"), null) + .Returns(responseTask); + + var apiConnection = Substitute.For(); + apiConnection.Connection.Returns(connection); + + var client = new RepositoryContentsClient(apiConnection); + + var actual = await client.GetArchive("org", "repo"); + + Assert.Equal(new byte[] { 1, 2, 3, 4 }, actual); + } } } } \ No newline at end of file From f9e82fc848fefbc2c18f8ff7f3b1944ad951dae8 Mon Sep 17 00:00:00 2001 From: jericho Date: Sat, 14 Oct 2023 15:06:41 -0400 Subject: [PATCH 2/6] (GH-2802) Add a GetRaw method that accepts a timeout I debated adding an optional parameter for the timeout to the existing GetRaw method (which would be my personal preference) but it would be a breaking change and I doubt the Octokit team would be interested in such a change --- Octokit/Http/Connection.cs | 16 +++++++++++++++- Octokit/Http/IConnection.cs | 12 +++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/Octokit/Http/Connection.cs b/Octokit/Http/Connection.cs index c0fc81f4ee..b6fa2d7435 100644 --- a/Octokit/Http/Connection.cs +++ b/Octokit/Http/Connection.cs @@ -244,7 +244,21 @@ public Task> GetRaw(Uri uri, IDictionary pa } /// - public Task> GetRawStream(Uri uri, IDictionary parameters) + public Task> GetRaw(Uri uri, IDictionary parameters, TimeSpan timeout) + { + Ensure.ArgumentNotNull(uri, nameof(uri)); + + return GetRaw(new Request + { + Method = HttpMethod.Get, + BaseAddress = BaseAddress, + Endpoint = uri.ApplyParameters(parameters), + Timeout = timeout + }); + } + + /// + public Task> GetRawStream(Uri uri, IDictionary parameters) { Ensure.ArgumentNotNull(uri, nameof(uri)); diff --git a/Octokit/Http/IConnection.cs b/Octokit/Http/IConnection.cs index 8301b64c0d..836f53651d 100644 --- a/Octokit/Http/IConnection.cs +++ b/Octokit/Http/IConnection.cs @@ -36,9 +36,19 @@ public interface IConnection : IApiInfoProvider /// /// URI endpoint to send request to /// Querystring parameters for the request + /// The Timeout value /// representing the received HTTP response /// The property will be null if the points to a directory instead of a file - Task> GetRawStream(Uri uri, IDictionary parameters); + Task> GetRaw(Uri uri, IDictionary parameters, TimeSpan timeout); + + /// + /// Performs an asynchronous HTTP GET request that expects a containing raw data. + /// + /// URI endpoint to send request to + /// Querystring parameters for the request + /// representing the received HTTP response + /// The property will be null if the points to a directory instead of a file + Task> GetRawStream(Uri uri, IDictionary parameters); /// /// Performs an asynchronous HTTP GET request. From b21d04d0086f89a741efd452f4d5d353a722270a Mon Sep 17 00:00:00 2001 From: jericho Date: Sat, 14 Oct 2023 15:09:11 -0400 Subject: [PATCH 3/6] (GH-2802) Invoke `GetRaw` rather than `Get>byte[]>` when retrieving a repo's archive content. This ensure stream content are handled properly and solves the problem described in GitHub issue 2802 --- Octokit/Clients/RepositoryContentsClient.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Octokit/Clients/RepositoryContentsClient.cs b/Octokit/Clients/RepositoryContentsClient.cs index 200cc473a0..409acd9559 100644 --- a/Octokit/Clients/RepositoryContentsClient.cs +++ b/Octokit/Clients/RepositoryContentsClient.cs @@ -395,7 +395,7 @@ public async Task GetArchive(string owner, string name, ArchiveFormat ar var endpoint = ApiUrls.RepositoryArchiveLink(owner, name, archiveFormat, reference); - var response = await Connection.Get(endpoint, timeout).ConfigureAwait(false); + var response = await Connection.GetRaw(endpoint, null, timeout).ConfigureAwait(false); return response.Body; } @@ -416,7 +416,7 @@ public async Task GetArchive(long repositoryId, ArchiveFormat archiveFor var endpoint = ApiUrls.RepositoryArchiveLink(repositoryId, archiveFormat, reference); - var response = await Connection.Get(endpoint, timeout).ConfigureAwait(false); + var response = await Connection.GetRaw(endpoint, null, timeout).ConfigureAwait(false); return response.Body; } From 67612fcb69d7b7bfdf2a0bbcc8cb33fadc4c2a24 Mon Sep 17 00:00:00 2001 From: jericho Date: Sat, 14 Oct 2023 15:27:44 -0400 Subject: [PATCH 4/6] (GH-2802) Fix unit tests that got broken due to my recent change to the GetArchive method --- .../Clients/RepositoryContentsClientTests.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Octokit.Tests/Clients/RepositoryContentsClientTests.cs b/Octokit.Tests/Clients/RepositoryContentsClientTests.cs index 8dec410043..c07e33e162 100644 --- a/Octokit.Tests/Clients/RepositoryContentsClientTests.cs +++ b/Octokit.Tests/Clients/RepositoryContentsClientTests.cs @@ -750,7 +750,7 @@ public async Task RequestsCorrectUrl1() const string expectedUri = "repos/org/repo/tarball/"; var expectedTimeSpan = TimeSpan.FromMinutes(60); - connection.Connection.Received().Get(Arg.Is(uri => uri.ToString() == expectedUri), Arg.Is(span => span == expectedTimeSpan)); + connection.Connection.Received().GetRaw(Arg.Is(uri => uri.ToString() == expectedUri), null, Arg.Is(span => span == expectedTimeSpan)); } [Fact] @@ -764,7 +764,7 @@ public async Task RequestsCorrectUrl1WithRepositoryId() const string expectedUri = "repositories/1/tarball/"; var expectedTimeSpan = TimeSpan.FromMinutes(60); - connection.Connection.Received().Get(Arg.Is(uri => uri.ToString() == expectedUri), Arg.Is(span => span == expectedTimeSpan)); + connection.Connection.Received().GetRaw(Arg.Is(uri => uri.ToString() == expectedUri), null, Arg.Is(span => span == expectedTimeSpan)); } [Fact] @@ -778,7 +778,7 @@ public async Task RequestsCorrectUrl2() const string expectedUri = "repos/org/repo/zipball/"; var expectedTimeSpan = TimeSpan.FromMinutes(60); - connection.Connection.Received().Get(Arg.Is(uri => uri.ToString() == expectedUri), Arg.Is(span => span == expectedTimeSpan)); + connection.Connection.Received().GetRaw(Arg.Is(uri => uri.ToString() == expectedUri), null, Arg.Is(span => span == expectedTimeSpan)); } [Fact] @@ -792,7 +792,7 @@ public async Task RequestsCorrectUrl2WithRepositoryId() const string expectedUri = "repositories/1/zipball/"; var expectedTimeSpan = TimeSpan.FromMinutes(60); - connection.Connection.Received().Get(Arg.Is(uri => uri.ToString() == expectedUri), Arg.Is(span => span == expectedTimeSpan)); + connection.Connection.Received().GetRaw(Arg.Is(uri => uri.ToString() == expectedUri), null, Arg.Is(span => span == expectedTimeSpan)); } [Fact] @@ -806,7 +806,7 @@ public async Task RequestsCorrectUrl3() const string expectedUri = "repos/org/repo/zipball/ref"; var expectedTimeSpan = TimeSpan.FromMinutes(60); - connection.Connection.Received().Get(Arg.Is(uri => uri.ToString() == expectedUri), Arg.Is(span => span == expectedTimeSpan)); + connection.Connection.Received().GetRaw(Arg.Is(uri => uri.ToString() == expectedUri), null, Arg.Is(span => span == expectedTimeSpan)); } [Fact] @@ -820,7 +820,7 @@ public async Task RequestsCorrectUrl3WithRepositoryId() const string expectedUri = "repositories/1/zipball/ref"; var expectedTimeSpan = TimeSpan.FromMinutes(60); - connection.Connection.Received().Get(Arg.Is(uri => uri.ToString() == expectedUri), Arg.Is(span => span == expectedTimeSpan)); + connection.Connection.Received().GetRaw(Arg.Is(uri => uri.ToString() == expectedUri), null, Arg.Is(span => span == expectedTimeSpan)); } [Fact] @@ -834,7 +834,7 @@ public async Task RequestsCorrectUrl4() const string expectedUri = "repos/org/repo/zipball/ref"; var expectedTimeSpan = TimeSpan.FromMinutes(60); - connection.Connection.Received().Get(Arg.Is(uri => uri.ToString() == expectedUri), Arg.Is(span => span == expectedTimeSpan)); + connection.Connection.Received().GetRaw(Arg.Is(uri => uri.ToString() == expectedUri), null, Arg.Is(span => span == expectedTimeSpan)); } [Fact] @@ -848,7 +848,7 @@ public async Task RequestsCorrectUrl4WithRepositoryId() const string expectedUri = "repositories/1/zipball/ref"; var expectedTimeSpan = TimeSpan.FromMinutes(60); - connection.Connection.Received().Get(Arg.Is(uri => uri.ToString() == expectedUri), Arg.Is(span => span == expectedTimeSpan)); + connection.Connection.Received().GetRaw(Arg.Is(uri => uri.ToString() == expectedUri), null, Arg.Is(span => span == expectedTimeSpan)); } [Fact] @@ -893,7 +893,7 @@ public async Task ReturnsExpectedContent() var responseTask = Task.FromResult>(new ApiResponse(response)); var connection = Substitute.For(); - connection.Get(Arg.Is(u => u.ToString() == "repos/org/repo/tarball/"), null) + connection.GetRaw(Arg.Is(u => u.ToString() == "repos/org/repo/tarball/"), null, TimeSpan.FromMinutes(60)) .Returns(responseTask); var apiConnection = Substitute.For(); From 97f111f883893a1a7645adf92cdd8fb34ae4f992 Mon Sep 17 00:00:00 2001 From: jericho Date: Sat, 14 Oct 2023 15:31:14 -0400 Subject: [PATCH 5/6] (GH-2802) Fix formatting --- Octokit/Http/Connection.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Octokit/Http/Connection.cs b/Octokit/Http/Connection.cs index b6fa2d7435..ec622c43ae 100644 --- a/Octokit/Http/Connection.cs +++ b/Octokit/Http/Connection.cs @@ -242,9 +242,9 @@ public Task> GetRaw(Uri uri, IDictionary pa Endpoint = uri.ApplyParameters(parameters) }); } - + /// - public Task> GetRaw(Uri uri, IDictionary parameters, TimeSpan timeout) + public Task> GetRaw(Uri uri, IDictionary parameters, TimeSpan timeout) { Ensure.ArgumentNotNull(uri, nameof(uri)); From e227191d71ad651ca5f0b917bc484135eda30b01 Mon Sep 17 00:00:00 2001 From: jericho Date: Sat, 14 Oct 2023 15:35:57 -0400 Subject: [PATCH 6/6] (GH-2802) Fix more formatting --- Octokit/Http/Connection.cs | 2 +- Octokit/Http/IConnection.cs | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Octokit/Http/Connection.cs b/Octokit/Http/Connection.cs index ec622c43ae..3225bf88db 100644 --- a/Octokit/Http/Connection.cs +++ b/Octokit/Http/Connection.cs @@ -258,7 +258,7 @@ public Task> GetRaw(Uri uri, IDictionary pa } /// - public Task> GetRawStream(Uri uri, IDictionary parameters) + public Task> GetRawStream(Uri uri, IDictionary parameters) { Ensure.ArgumentNotNull(uri, nameof(uri)); diff --git a/Octokit/Http/IConnection.cs b/Octokit/Http/IConnection.cs index 836f53651d..c9f8daa848 100644 --- a/Octokit/Http/IConnection.cs +++ b/Octokit/Http/IConnection.cs @@ -30,7 +30,7 @@ public interface IConnection : IApiInfoProvider /// representing the received HTTP response /// The property will be null if the points to a directory instead of a file Task> GetRaw(Uri uri, IDictionary parameters); - + /// /// Performs an asynchronous HTTP GET request that expects a containing raw data. /// @@ -42,13 +42,13 @@ public interface IConnection : IApiInfoProvider Task> GetRaw(Uri uri, IDictionary parameters, TimeSpan timeout); /// - /// Performs an asynchronous HTTP GET request that expects a containing raw data. - /// - /// URI endpoint to send request to - /// Querystring parameters for the request - /// representing the received HTTP response - /// The property will be null if the points to a directory instead of a file - Task> GetRawStream(Uri uri, IDictionary parameters); + /// Performs an asynchronous HTTP GET request that expects a containing raw data. + /// + /// URI endpoint to send request to + /// Querystring parameters for the request + /// representing the received HTTP response + /// The property will be null if the points to a directory instead of a file + Task> GetRawStream(Uri uri, IDictionary parameters); /// /// Performs an asynchronous HTTP GET request.