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.

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

Jump to bottom

PART 3: Add UploadArchiveToGithubStorageWithMultiPart method #1271

Merged
merged 8 commits into from
Oct 8, 2024
Merged

PART 3: Add UploadArchiveToGithubStorageWithMultiPart method #1271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aab0557
Add UploadArchiveToGithubStorageWithMultiPart method
begonaguereca Sep 25, 2024
1fca83d
Make changes to SendAsync command
begonaguereca Oct 8, 2024
f81e02a
remove backslashed
begonaguereca Oct 8, 2024
2c8de4a
Escape string
begonaguereca Oct 8, 2024
619e6da
Merge branch 'main' into github-api-upload
begonaguereca Oct 8, 2024
bf5eef2
Fixing unit tests
begonaguereca Oct 8, 2024
60e9ed1
alphabetical order
begonaguereca Oct 8, 2024
cd88309
clean up
begonaguereca Oct 8, 2024
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
28 changes: 28 additions & 0 deletions src/Octoshift/Services/GithubApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Http;
Expand Down Expand Up @@ -1071,6 +1072,33 @@ mutation abortRepositoryMigration(
}
}

public virtual async Task<string> UploadArchiveToGithubStorage(string orgDatabaseId, bool isMultipart, string archiveName, Stream archiveContent)
{
using var httpContent = new StreamContent(archiveContent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I would call this variable streamContent for clarity as both stream and multipart from data are technically HttpContent"

Suggested change
using var httpContent = new StreamContent(archiveContent);
using var streamContent = new StreamContent(archiveContent);

string response;

if (isMultipart)
{
var url = $"https://uploads.github.com/organizations/{orgDatabaseId.EscapeDataString()}/gei/archive/blobs/uploads";

using var content = new MultipartFormDataContent
{
{ httpContent, "archive", archiveName }
};
Comment on lines +1084 to +1087
Copy link
Collaborator

Choose a reason for hiding this comment

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

Initializing a using variable with an object initializer might lead to problems if an exception is thrown during the initialization and the using variable may never get disposed so here it's better to add the content with the Add() method:

Suggested change
using var content = new MultipartFormDataContent
{
{ httpContent, "archive", archiveName }
};
using var content = new MultipartFormDataContent();
content.Add(streamContent, "archive", archiveName);


response = await _client.PostAsync(url, content);
begonaguereca marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
var url = $"https://uploads.github.com/organizations/{orgDatabaseId.EscapeDataString()}/gei/archive?name={archiveName.EscapeDataString()}";

response = await _client.PostAsync(url, httpContent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

renaming httpContent to streamContent:

Suggested change
response = await _client.PostAsync(url, httpContent);
response = await _client.PostAsync(url, streamContent);

}

var data = JObject.Parse(response);
return "gei://archive/" + (string)data["archiveId"];
}

private static object GetMannequinsPayload(string orgId)
{
var query = "query($id: ID!, $first: Int, $after: String)";
Expand Down
9 changes: 7 additions & 2 deletions src/Octoshift/Services/GithubClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,14 @@ public virtual async Task<string> PatchAsync(string url, object body, Dictionary

if (body != null)
{
_log.LogVerbose($"HTTP BODY: {body.ToJson()}");
_log.LogVerbose(body is MultipartFormDataContent or StreamContent ? "HTTP BODY: BLOB" : $"HTTP BODY: {body.ToJson()}");

request.Content = body.ToJson().ToStringContent();
request.Content = body switch
{
MultipartFormDataContent multipartFormDataContent => multipartFormDataContent,
StreamContent streamContent => streamContent,
_ => body.ToJson().ToStringContent()
};
Comment on lines +165 to +172
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking this can be simplified as we now either support a HttpContent or a JSON object (it cannot let me add a code suggestion):

            if (body is HttpContent httpContent)
            {
                _log.LogVerbose("HTTP BODY: BLOB");
                request.Content = httpContent;
            }
            else
            {
                var jsonBody = body.ToJson();
                _log.LogVerbose($"HTTP BODY: {jsonBody}");
                request.Content = jsonBody.ToStringContent();
            }

}

using var response = await _httpClient.SendAsync(request);
Expand Down
58 changes: 57 additions & 1 deletion src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Http;
Expand Down Expand Up @@ -3462,6 +3463,62 @@ await _githubApi.Invoking(api => api.AbortMigration(migrationId))
.WithMessage(expectedErrorMessage);
}

[Fact]
public async Task UploadArchiveToGithubStorage()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public async Task UploadArchiveToGithubStorage()
public async Task UploadArchiveToGithubStorage_Should_Upload_Stream_Content()

{
//Arange
const string org = "1234";
const bool isMultipart = false;
const string archiveName = "archiveName";

// Using a MemoryStream as a valid stream implementation
using var archiveContent = new MemoryStream(new byte[] { 1, 2, 3 });
var expectedArchiveId = "123456";
var jsonResponse = $"{{ \"archiveId\": \"{expectedArchiveId}\" }}";

_githubClientMock
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<HttpContent>(), null))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make sure that PostAsync receives a StreamContent:

Suggested change
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<HttpContent>(), null))
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<StreamContent>(), null))

.ReturnsAsync(jsonResponse);

var expectedStringResponse = "gei://archive/" + expectedArchiveId;

// Act
var actualStringResponse = await _githubApi.UploadArchiveToGithubStorage(org, isMultipart, archiveName, archiveContent);

// Assert
expectedStringResponse.Should().Be(actualStringResponse);

}


[Fact]
public async Task UploadArchiveToGithubStorageWithMultiPart()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use snaked cased test names throughout the project:

Suggested change
public async Task UploadArchiveToGithubStorageWithMultiPart()
public async Task UploadArchiveToGithubStorage_Should_Upload_Multipart_Content()

{
//Arange
const string org = "123455";
const bool isMultipart = true;
const string archiveName = "archiveName";

// Using a MemoryStream as a valid stream implementation
using var archiveContent = new MemoryStream(new byte[] { 1, 2, 3 });

var expectedArchiveId = "123456";
var jsonResponse = $"{{ \"archiveId\": \"{expectedArchiveId}\" }}";

_githubClientMock
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<HttpContent>(), null))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<HttpContent>(), null))
.Setup(m => m.PostAsync(It.IsAny<string>(), It.IsAny<MultipartFormDataContent>(), null))

.ReturnsAsync(jsonResponse);

var expectedStringResponse = "gei://archive/" + expectedArchiveId;

// Act
var actualStringResponse = await _githubApi.UploadArchiveToGithubStorage(org, isMultipart, archiveName, archiveContent);

// Assert
expectedStringResponse.Should().Be(actualStringResponse);

}

private string Compact(string source) =>
source
.Replace("\r", "")
Expand All @@ -3471,5 +3528,4 @@ private string Compact(string source) =>
.Replace("\\n", "")
.Replace("\\t", "")
.Replace(" ", "");

}
Loading