Skip to content

Consume URI in PUT response for GitHub-owned storage for GEI #1371

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 5, 2025
Merged

Consume URI in PUT response for GitHub-owned storage for GEI #1371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ce9bbc
Consume URI in PUT response for GitHub-owned storage for GEI.
synthead Jun 26, 2025
df49d6c
Merge branch 'main' into consume-gei-uri-in-multipart-response
synthead Jun 26, 2025
ff927dd
Merge branch 'main' into consume-gei-uri-in-multipart-response
begonaguereca Jul 7, 2025
5b3d7f8
Merge branch 'main' into consume-gei-uri-in-multipart-response
synthead Aug 4, 2025
3dca950
Merge branch 'main' into consume-gei-uri-in-multipart-response
synthead Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/Octoshift/Services/ArchiveUploader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;
using System.Web;
using Newtonsoft.Json.Linq;
using OctoshiftCLI.Extensions;

Expand Down Expand Up @@ -64,11 +63,8 @@ private async Task<string> UploadMultipart(Stream archiveContent, string archive
{
// 1. Start the upload
var startHeaders = await StartUpload(uploadUrl, archiveName, archiveContent.Length);

var nextUrl = GetNextUrl(startHeaders);

var guid = HttpUtility.ParseQueryString(nextUrl.Query)["guid"];

// 2. Upload parts
int bytesRead;
var partsRead = 0;
Expand All @@ -80,9 +76,9 @@ private async Task<string> UploadMultipart(Stream archiveContent, string archive
}

// 3. Complete the upload
await CompleteUpload(nextUrl.ToString());
var geiUri = await CompleteUpload(nextUrl.ToString());

return $"gei://archive/{guid}";
return geiUri.ToString();
Copy link
Preview

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable geiUri is a Uri object, but calling ToString() on it is redundant since the method return type is string. This should return geiUri.ToString() or change the return type to Uri and return geiUri directly.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion produces this error:

error CS0029: Cannot implicitly convert type 'System.Uri' to 'string' [src/Octoshift/Octoshift.csproj]

}
catch (Exception ex)
{
Expand Down Expand Up @@ -132,12 +128,16 @@ private async Task<Uri> UploadPart(byte[] body, int bytesRead, string nextUrl, i
}
}

private async Task CompleteUpload(string lastUrl)
private async Task<Uri> CompleteUpload(string lastUrl)
{
try
{
await _retryPolicy.Retry(async () => await _client.PutAsync(lastUrl, ""));
var response = await _retryPolicy.Retry(async () => await _client.PutAsync(lastUrl, ""));
var responseData = JObject.Parse(response);

_log.LogInformation("Finished uploading archive");

return new Uri((string)responseData["uri"]);
Copy link
Preview

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cast to (string) could throw an exception if the 'uri' field is null or not a string. Consider using responseData["uri"]?.ToString() or add null checking to provide a more helpful error message if the expected field is missing.

Suggested change
return new Uri((string)responseData["uri"]);
var uriString = responseData["uri"]?.ToString();
if (string.IsNullOrWhiteSpace(uriString))
{
throw new OctoshiftCliException("The response from the server did not contain a valid 'uri' field.");
}
return new Uri(uriString);

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With our API, it's not possible for a 201 to be returned from uploads and not have a URI in the response.

}
catch (Exception ex)
{
Expand Down
64 changes: 53 additions & 11 deletions src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Threading.Tasks;
using FluentAssertions;
using Moq;
using Newtonsoft.Json.Linq;
using OctoshiftCLI.Extensions;
using OctoshiftCLI.Services;
using Xunit;
Expand Down Expand Up @@ -52,9 +53,20 @@ public async Task Upload_Should_Upload_All_Chunks_When_Stream_Exceeds_Limit()
const string archiveName = "test-archive";
const string baseUrl = "https://uploads.github.com";
const string guid = "c9dbd27b-f190-4fe4-979f-d0b7c9b0fcb3";
const string geiUri = $"gei://archive/{guid}";

var startUploadBody = new { content_type = "application/octet-stream", name = archiveName, size = contentSize };

var completeUploadResponse = new JObject
{
["guid"] = guid,
["node_id"] = "global-relay-id",
["name"] = archiveName,
["size"] = largeContent.Length,
["uri"] = geiUri,
["created_at"] = "2025-06-23T17:13:02.818-07:00"
};
Comment on lines +60 to +68
Copy link
Preview

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The completeUploadResponse initialization is duplicated across multiple tests. Consider extracting it into a common helper or fixture to reduce repetition.

Suggested change
var completeUploadResponse = new JObject
{
["guid"] = guid,
["node_id"] = "global-relay-id",
["name"] = archiveName,
["size"] = largeContent.Length,
["uri"] = geiUri,
["created_at"] = "2025-06-23T17:13:02.818-07:00"
};
var completeUploadResponse = CreateCompleteUploadResponse(guid, archiveName, largeContent.Length, geiUri);

Copilot uses AI. Check for mistakes.


const string initialUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads";
const string firstUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads?part_number=1&guid={guid}";
const string secondUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads?part_number=2&guid={guid}";
Expand Down Expand Up @@ -83,13 +95,13 @@ public async Task Upload_Should_Upload_All_Chunks_When_Stream_Exceeds_Limit()
// Mocking the final PUT request to complete the multipart upload
_githubClientMock
.Setup(m => m.PutAsync($"{baseUrl}{lastUrl}", "", null))
.ReturnsAsync(string.Empty);
.ReturnsAsync(completeUploadResponse.ToString());

// act
var result = await _archiveUploader.Upload(archiveContent, archiveName, orgDatabaseId);

// assert
result.Should().Be($"gei://archive/{guid}");
result.Should().Be(geiUri);

_githubClientMock.Verify(m => m.PostWithFullResponseAsync(It.IsAny<string>(), It.IsAny<object>(), null), Times.Once);
_githubClientMock.Verify(m => m.PatchWithFullResponseAsync(It.IsAny<string>(), It.IsAny<object>(), null), Times.Exactly(3));
Expand All @@ -109,10 +121,20 @@ public async Task Upload_Should_Retry_Failed_Upload_Part_Patch_Requests()
const string archiveName = "test-archive";
const string baseUrl = "https://uploads.github.com";
const string guid = "c9dbd27b-f190-4fe4-979f-d0b7c9b0fcb3";
const string expectedResult = $"gei://archive/{guid}";
const string geiUri = $"gei://archive/{guid}";

var startUploadBody = new { content_type = "application/octet-stream", name = archiveName, size = largeContent.Length };

var completeUploadResponse = new JObject
{
["guid"] = guid,
["node_id"] = "global-relay-id",
["name"] = archiveName,
["size"] = largeContent.Length,
["uri"] = geiUri,
["created_at"] = "2025-06-23T17:13:02.818-07:00"
};

const string initialUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads";
const string firstUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads?part_number=1&guid={guid}";
const string secondUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads?part_number=2&guid={guid}";
Expand All @@ -139,13 +161,13 @@ public async Task Upload_Should_Retry_Failed_Upload_Part_Patch_Requests()
// Mocking the final PUT request to complete the multipart upload
_githubClientMock
.Setup(m => m.PutAsync($"{baseUrl}{lastUrl}", "", null))
.ReturnsAsync(string.Empty);
.ReturnsAsync(completeUploadResponse.ToString());

// act
var result = await _archiveUploader.Upload(archiveContent, archiveName, orgDatabaseId);

// assert
Assert.Equal(expectedResult, result);
Assert.Equal(geiUri, result);

_githubClientMock.Verify(m => m.PostWithFullResponseAsync(It.IsAny<string>(), It.IsAny<object>(), null), Times.Once);
_githubClientMock.Verify(m => m.PatchWithFullResponseAsync(It.IsAny<string>(), It.IsAny<object>(), null), Times.Exactly(4)); // 2 retries + 2 success
Expand All @@ -165,10 +187,20 @@ public async Task Upload_Should_Retry_Failed_Start_Upload_Post_Request()
const string archiveName = "test-archive";
const string baseUrl = "https://uploads.github.com";
const string guid = "c9dbd27b-f190-4fe4-979f-d0b7c9b0fcb3";
const string expectedResult = $"gei://archive/{guid}";
const string geiUri = $"gei://archive/{guid}";

var startUploadBody = new { content_type = "application/octet-stream", name = archiveName, size = largeContent.Length };

var completeUploadResponse = new JObject
{
["guid"] = guid,
["node_id"] = "global-relay-id",
["name"] = archiveName,
["size"] = largeContent.Length,
["uri"] = geiUri,
["created_at"] = "2025-06-23T17:13:02.818-07:00"
};

const string initialUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads";
const string firstUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads?part_number=1&guid={guid}";
const string secondUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads?part_number=2&guid={guid}";
Expand All @@ -195,13 +227,13 @@ public async Task Upload_Should_Retry_Failed_Start_Upload_Post_Request()
// Mocking the final PUT request to complete the multipart upload
_githubClientMock
.Setup(m => m.PutAsync($"{baseUrl}{lastUrl}", "", null))
.ReturnsAsync(string.Empty);
.ReturnsAsync(completeUploadResponse.ToString());

// act
var result = await _archiveUploader.Upload(archiveContent, archiveName, orgDatabaseId);

// assert
Assert.Equal(expectedResult, result);
Assert.Equal(geiUri, result);

_githubClientMock.Verify(m => m.PostWithFullResponseAsync(It.IsAny<string>(), It.IsAny<object>(), null), Times.Exactly(3)); // 2 retries + 1 success
_githubClientMock.Verify(m => m.PatchWithFullResponseAsync(It.IsAny<string>(), It.IsAny<object>(), null), Times.Exactly(2));
Expand All @@ -221,10 +253,20 @@ public async Task Upload_Should_Retry_Failed_Complete_Upload_Put_Request()
const string archiveName = "test-archive";
const string baseUrl = "https://uploads.github.com";
const string guid = "c9dbd27b-f190-4fe4-979f-d0b7c9b0fcb3";
const string expectedResult = $"gei://archive/{guid}";
const string geiUri = $"gei://archive/{guid}";

var startUploadBody = new { content_type = "application/octet-stream", name = archiveName, size = largeContent.Length };

var completeUploadResponse = new JObject
{
["guid"] = guid,
["node_id"] = "global-relay-id",
["name"] = archiveName,
["size"] = largeContent.Length,
["uri"] = geiUri,
["created_at"] = "2025-06-23T17:13:02.818-07:00"
};

const string initialUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads";
const string firstUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads?part_number=1&guid={guid}";
const string secondUploadUrl = $"/organizations/{orgDatabaseId}/gei/archive/blobs/uploads?part_number=2&guid={guid}";
Expand All @@ -251,13 +293,13 @@ public async Task Upload_Should_Retry_Failed_Complete_Upload_Put_Request()
.SetupSequence(m => m.PutAsync($"{baseUrl}{lastUrl}", "", null))
.ThrowsAsync(new TimeoutException("The operation was canceled."))
.ThrowsAsync(new TimeoutException("The operation was canceled."))
.ReturnsAsync(string.Empty);
.ReturnsAsync(completeUploadResponse.ToString());

// act
var result = await _archiveUploader.Upload(archiveContent, archiveName, orgDatabaseId);

// assert
Assert.Equal(expectedResult, result);
Assert.Equal(geiUri, result);

_githubClientMock.Verify(m => m.PostWithFullResponseAsync(It.IsAny<string>(), It.IsAny<object>(), null), Times.Once);
_githubClientMock.Verify(m => m.PatchWithFullResponseAsync(It.IsAny<string>(), It.IsAny<object>(), null), Times.Exactly(2));
Expand Down
Loading