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

Post the response from the endpoint when log file posting succeed or failed #2771

Merged
merged 2 commits into from
Jan 29, 2024
Merged

Post the response from the endpoint when log file posting succeed or failed #2771

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24c6674
Log server message
shaopeng-gh Jan 29, 2024
462ca80
fix comments
shaopeng-gh Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/Sarif/Core/SarifLog.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public static SarifLog Load(Stream source, bool deferred = false)
/// <param name="filePath"></param>
/// <param name="fileSystem"></param>
/// <param name="httpClient"></param>
/// <returns>If the SarifLog has been posted.</returns>
/// <returns>If the SarifLog has been posted successfully.</returns>
public static async Task<bool> Post(Uri postUri,
string filePath,
IFileSystem fileSystem,
Expand All @@ -101,8 +101,20 @@ public static SarifLog Load(Stream source, bool deferred = false)
return false;
}

await Post(postUri, new MemoryStream(fileBytes), httpClient);
Console.WriteLine($"Posted log file successfully to: {postUri}");
HttpResponseMessage response = await Post(postUri, new MemoryStream(fileBytes), httpClient);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

copied the code section,
but made some changes to the message, please review.
I used generic form: "Endpoint provided message"

string responseText = response.Content.ReadAsStringAsync().Result;
Copy link
Collaborator

@scottoneil-ms scottoneil-ms Jan 29, 2024 •
edited by shaopeng-gh

Choose a reason for hiding this comment

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

This method is actually async, so you can await this Task instead of doing .Result. (And that is better.) #Resolved


try
{
response.EnsureSuccessStatusCode();
}
catch (Exception)
{
Console.WriteLine($"Error Posting log file to: {postUri}. Endpoint provided message: {responseText}");
return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems better to check response.IsSuccessStatusCode. #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is, as opposed to doing a try/catch here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And I also think status code is useful to user, I think we should add it to the output as well

Console.WriteLine($"Posted log file successfully to: {postUri}. Endpoint provided message: {responseText}");
return true;
Copy link
Collaborator

@scottoneil-ms scottoneil-ms Jan 29, 2024 •
edited by shaopeng-gh

Choose a reason for hiding this comment

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

I'm not sure what the format of the responseText is going to be and what it contains. Is it possible to run this locally and provide a sample in the PR description? #Resolved

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 is generic,
the server that takes the SARIF file is to be set by the user,
and that server could be with any design by the user.
It can returns anything, e.g. correlation id.

}

Expand Down
13 changes: 12 additions & 1 deletion src/Test.UnitTests.Sarif/Core/SarifLogTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ public async Task SarifLog_PostFile_WithInvalidParameters_ShouldThrowException()
}

[Fact]
public async Task SarifLog_PostFile_ShouldPostTests()
public async Task SarifLog_PostFile_PostTests()
{
using var assertionScope = new AssertionScope();

Expand Down Expand Up @@ -318,6 +318,17 @@ public async Task SarifLog_PostFile_ShouldPostTests()
fileSystem.Object,
httpClient: new HttpClient(httpMock));
logPosted.Should().BeFalse("with warning level ToolExecutionNotifications");

steam = (MemoryStream)CreateSarifLogStreamWithToolExecutionNotifications(FailureLevel.Error);
fileSystem
.Setup(f => f.FileReadAllBytes(It.IsAny<string>()))
.Returns(steam.ToArray());
httpMock.Mock(HttpMockHelper.CreateBadRequestResponse());
logPosted = await SarifLog.Post(postUri: postUri,
filePath,
fileSystem.Object,
httpClient: new HttpClient(httpMock));
logPosted.Should().BeTrue("with error level ToolExecutionNotifications, but the server returns BadRequest");
}

[Fact]
Expand Down