Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: RepositoryContentsClient.GetArchive does not return the expected binary content #2802

Closed
1 task done
Jericho opened this issue Oct 14, 2023 · 0 comments · Fixed by #2803
Closed
1 task done
Labels
Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented

Comments

@Jericho
Copy link
Contributor

Jericho commented Oct 14, 2023

What happened?

This commit which was published in Octokit v8.1.1 introduced a change in HttpClientAdapter.cs to return binary content as a stream rather than an array of byte. However, the code in RepositoryContentsClient.GetArchive still expects byte[] and was not adjusted to handle a stream.

This problem was not caught in unit testing because none of the tests in RepositoryContentsClientTests validate that GetArchive returns the expected array of bytes. I wrote the following unit test to demonstrate the problem. This unit test fails because the response does not contain the expected bytes:

[Fact]
public async Task ReturnsExpectedContent()
{
	var headers = new Dictionary<string, string>();
	var response = TestSetup.CreateResponse(HttpStatusCode.OK, new byte[] { 1, 2, 3, 4 }, headers);
	var responseTask = Task.FromResult<IApiResponse<byte[]>>(new ApiResponse<byte[]>(response));

	var connection = Substitute.For<IConnection>();
	connection.Get<byte[]>(Arg.Is<Uri>(u => u.ToString() == "repos/org/repo/tarball/"), null)
		.Returns(responseTask);

	var apiConnection = Substitute.For<IApiConnection>();
	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); // <-- this assertion fails, thereby demonstrating that GetArchive does not return the expected array of bytes
}

I will submit a PR to resolve this problem and ensure that the unit test presented above completes successfully.

Versions

This problem can be observed in Octokit 8.1.1.

I verified 7.1.0, 7.2.0, 8.0.0 and 8.0.1 and they work fine. Meaning: they return the expected array of bytes.

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@Jericho Jericho added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Oct 14, 2023
kfcampbell pushed a commit that referenced this issue Oct 16, 2023
…d binary content (#2803)

* (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

* (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

* (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

* (GH-2802) Fix unit tests that got broken due to my recent change to the GetArchive method

* (GH-2802) Fix formatting

* (GH-2802) Fix more formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant