diff --git a/RELEASENOTES.md b/RELEASENOTES.md index e69de29bb..c408e37e8 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -0,0 +1 @@ +- BBS2GH: When a Bitbucket Server API request fails with a 5xx response, the response body is now included in the standard log output (alongside the generic error message) to make troubleshooting easier without needing to inspect the verbose log. diff --git a/src/Octoshift/Services/BbsClient.cs b/src/Octoshift/Services/BbsClient.cs index 1da355c06..63588d595 100644 --- a/src/Octoshift/Services/BbsClient.cs +++ b/src/Octoshift/Services/BbsClient.cs @@ -85,7 +85,7 @@ private async Task SendAsync(HttpMethod httpMethod, string url, object b } using var payload = body?.ToJson().ToStringContent(); - var response = httpMethod.ToString() switch + using var response = httpMethod.ToString() switch { "GET" => await _httpClient.GetAsync(url), "DELETE" => await _httpClient.DeleteAsync(url), @@ -97,6 +97,16 @@ private async Task SendAsync(HttpMethod httpMethod, string url, object b var content = await response.Content.ReadAsStringAsync(); _log.LogVerbose($"RESPONSE ({response.StatusCode}): {content}"); + if (!response.IsSuccessStatusCode && (int)response.StatusCode >= 500) + { + var serverErrorException = new HttpRequestException( + $"Response status code does not indicate success: {(int)response.StatusCode} ({response.ReasonPhrase}).", + null, + response.StatusCode); + serverErrorException.Data["ResponseBody"] = content; + throw serverErrorException; + } + response.EnsureSuccessStatusCode(); return content; diff --git a/src/Octoshift/Services/OctoLogger.cs b/src/Octoshift/Services/OctoLogger.cs index b737b845e..1b59b7e60 100644 --- a/src/Octoshift/Services/OctoLogger.cs +++ b/src/Octoshift/Services/OctoLogger.cs @@ -146,7 +146,18 @@ public virtual void LogError(Exception ex) lock (_mutex) { var verboseMessage = ex is HttpRequestException httpEx ? $"[HTTP ERROR {(int?)httpEx.StatusCode}] {ex}" : ex.ToString(); - var logMessage = Verbose ? verboseMessage : ex is OctoshiftCliException ? ex.Message : GENERIC_ERROR_MESSAGE; + var responseBody = ex.Data["ResponseBody"] as string; + if (responseBody is not null) + { + verboseMessage = $"{verboseMessage}{Environment.NewLine}Response: {responseBody}"; + } + var logMessage = Verbose + ? verboseMessage + : ex is OctoshiftCliException + ? ex.Message + : responseBody is not null + ? $"{GENERIC_ERROR_MESSAGE}{Environment.NewLine}Response: {responseBody}" + : GENERIC_ERROR_MESSAGE; var output = Redact(FormatMessage(logMessage, LogLevel.ERROR)); diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/BbsClientTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/BbsClientTests.cs index 491e529bb..566a750dc 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/BbsClientTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/BbsClientTests.cs @@ -442,19 +442,28 @@ public async Task GetAllAsync_Throws_HttpRequestException_On_Non_Success_Respons const string url = "http://example.com/resource"; var firstResponseContent = new { isLastPage = false, nextPageStart = 2, values = Array.Empty() }.ToJson(); - using var firstResponse = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(firstResponseContent) }; - - using var secondResponse = new HttpResponseMessage(HttpStatusCode.InternalServerError); var handlerMock = new Mock(); // first request const string firstRequestUrl = $"{url}?start=0&limit=100"; - MockHttpHandler(req => req.Method == HttpMethod.Get && req.RequestUri!.ToString() == firstRequestUrl, firstResponse, handlerMock); + handlerMock + .Protected() + .Setup>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get && req.RequestUri!.ToString() == firstRequestUrl), + ItExpr.IsAny()) + .ReturnsAsync(() => new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(firstResponseContent) }); - // second request + // second request - return a fresh response each time so retries don't reuse a disposed instance const string secondRequestUrl = $"{url}?start=2&limit=100"; - MockHttpHandler(req => req.Method == HttpMethod.Get && req.RequestUri!.ToString() == secondRequestUrl, secondResponse, handlerMock); + handlerMock + .Protected() + .Setup>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get && req.RequestUri!.ToString() == secondRequestUrl), + ItExpr.IsAny()) + .ReturnsAsync(() => new HttpResponseMessage(HttpStatusCode.InternalServerError)); using var httpClient = new HttpClient(handlerMock.Object); var bbsClient = new BbsClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy); @@ -466,6 +475,34 @@ await bbsClient .ThrowExactlyAsync(); } + [Fact] + public async Task SendAsync_Includes_Response_Body_In_Exception_Data_On_5xx() + { + // Arrange + const string url = "http://example.com/resource"; + const string responseBody = "{\"errors\":[{\"message\":\"Could not start migration job\"}]}"; + + using var response = new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) + { + Content = new StringContent(responseBody) + }; + + var handlerMock = MockHttpHandler(req => req.Method == HttpMethod.Post, response); + + using var httpClient = new HttpClient(handlerMock.Object); + var bbsClient = new BbsClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy); + + // Act + var thrown = await bbsClient + .Invoking(async x => await x.PostAsync(url, _rawRequestBody)) + .Should() + .ThrowExactlyAsync(); + + // Assert + thrown.Which.StatusCode.Should().Be(HttpStatusCode.ServiceUnavailable); + thrown.Which.Data["ResponseBody"].Should().Be(responseBody); + } + [Fact] public async Task GetAllAsync_Overrides_Pagination_Query_Params_In_Request_Url() { diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/OctoLoggerTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/OctoLoggerTests.cs index 1b8a3b0bd..fd7c01265 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/OctoLoggerTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/OctoLoggerTests.cs @@ -240,6 +240,30 @@ public void LogError_With_Exception_Should_Write_To_Console_Error() _consoleError.Should().NotBeNull(); } + [Fact] + public void LogError_For_Exception_With_ResponseBody_Should_Include_Response_Body_In_Non_Verbose_Mode() + { + // Arrange + const string genericErrorMessage = "An unexpected error happened. Please see the logs for details."; + const string responseBody = "{\"errors\":[{\"message\":\"Could not start migration job\"}]}"; + var httpException = new HttpRequestException("Response status code does not indicate success: 503 (Service Unavailable).", null, HttpStatusCode.ServiceUnavailable); + httpException.Data["ResponseBody"] = responseBody; + + // Act + _octoLogger.LogError(httpException); + + // Assert + _consoleOutput.Should().BeNull(); + + _consoleError.Should().Contain(genericErrorMessage); + _consoleError.Should().Contain($"Response: {responseBody}"); + + _logOutput.Should().Contain(genericErrorMessage); + _logOutput.Should().Contain($"Response: {responseBody}"); + + _verboseLogOutput.Should().Contain(responseBody); + } + [Fact] public void LogInformation_Should_Write_To_Console_Out() {