Skip to content

Commit

Permalink
[ODS-5985] Correlation IDs in the API error messages (add server-gene…
Browse files Browse the repository at this point in the history
…rated correlationIds) (#834)

* Updated behavior to include a generated server-side GUID value for the correlationId if one is not supplied by the client.

* Fixed unit test around the server-assigned GUID-based correlation id.
  • Loading branch information
gmcelhanon authored Sep 28, 2023
1 parent ee53db4 commit 191f026
Show file tree
Hide file tree
Showing 16 changed files with 65 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using EdFi.Ods.Api.Infrastructure.Pipelines.Put;
using EdFi.Ods.Common;
using EdFi.Ods.Common.Configuration;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Context;
using EdFi.Ods.Common.Exceptions;
using EdFi.Ods.Common.Infrastructure.Pipelines.Delete;
Expand Down Expand Up @@ -145,14 +146,14 @@ private IActionResult CreateActionResultFromException(
// See RFC 5789 - Conflicting modification (with "If-Match" header)
restError.Code = StatusCodes.Status412PreconditionFailed;
restError.Message = "Resource was modified by another consumer.";
restError.CorrelationId = (string) _logContextAccessor.GetValue("CorrelationId");
restError.CorrelationId = (string) _logContextAccessor.GetValue(CorrelationConstants.LogContextKey);
}

return string.IsNullOrWhiteSpace(restError.Message)
? (IActionResult)StatusCode(restError.Code ?? default)
: StatusCode(
restError.Code ?? default,
ErrorTranslator.GetErrorMessage(restError.Message, (string)_logContextAccessor.GetValue("CorrelationId")));
ErrorTranslator.GetErrorMessage(restError.Message, (string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}

protected abstract void MapAll(TGetByExampleRequest request, TEntityInterface specification);
Expand Down Expand Up @@ -185,7 +186,7 @@ public virtual async Task<IActionResult> GetAll(
return BadRequest(
ErrorTranslator.GetErrorMessage(
$"Limit must be omitted or set to a value between 0 and {_defaultPageLimitSize}.",
(string)_logContextAccessor.GetValue("CorrelationId")));
(string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}

var internalRequestAsResource = new TResourceModel();
Expand Down Expand Up @@ -319,7 +320,7 @@ public virtual async Task<IActionResult> Post([FromBody] TPostRequest request)
return BadRequest(
ErrorTranslator.GetErrorMessage(
"Resource identifiers cannot be assigned by the client.",
(string)_logContextAccessor.GetValue("CorrelationId")));
(string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}

// Read the If-Match header and populate the resource DTO with an etag value.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Collections.Generic;
using System.Net;
using EdFi.Ods.Api.Models;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Logging;

namespace EdFi.Ods.Api.ExceptionHandling
Expand Down Expand Up @@ -37,7 +38,7 @@ public RESTError GetRestErrorFromException(Exception exception)
if (translator.TryTranslateMessage(exception, out error))
{
// Assign the correlation error (if it exists)
error.CorrelationId = (string)_logContextAccessor.GetValue("CorrelationId");
error.CorrelationId = (string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey);

return error;
}
Expand All @@ -50,7 +51,7 @@ public RESTError GetRestErrorFromException(Exception exception)
Code = (int)HttpStatusCode.InternalServerError,
Type = "Internal Server Error",
Message = "An unexpected error occurred on the server.",
CorrelationId = (string)_logContextAccessor.GetValue("CorrelationId")
CorrelationId = (string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)
};

return response;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Threading.Tasks;
using EdFi.Ods.Api.Models;
using EdFi.Ods.Common.Configuration;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Context;
using EdFi.Ods.Common.Logging;
using EdFi.Ods.Common.Models;
Expand Down Expand Up @@ -157,7 +158,7 @@ void SetForbiddenResponse()
var error = new RESTError()
{
Message = errorMessage,
CorrelationId = (string) _logContextAccessor.GetValue("CorrelationId")
CorrelationId = (string) _logContextAccessor.GetValue(CorrelationConstants.LogContextKey)
};

context.Result = new ObjectResult(error) { StatusCode = StatusCodes.Status403Forbidden };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0.
// See the LICENSE and NOTICES files in the project root for more information.

using System;
using System.Threading.Tasks;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Logging;
using Microsoft.AspNetCore.Http;

Expand All @@ -25,16 +27,20 @@ public RequestCorrelationMiddleware(ILogContextAccessor logContextAccessor)
public async Task InvokeAsync(HttpContext context, RequestDelegate next)
{
// Capture correlation from the header, if present
if (context.Request.Headers.TryGetValue("correlation-id", out var headerValues))
if (context.Request.Headers.TryGetValue(CorrelationConstants.HttpHeader, out var headerValues))
{
_logContextAccessor.SetValue("CorrelationId", headerValues[0]);
_logContextAccessor.SetValue(CorrelationConstants.LogContextKey, headerValues[0]);
}
else
{
// Capture correlation from the query string, if present
if (context.Request.Query.TryGetValue("correlationId", out var queryStringValues))
if (context.Request.Query.TryGetValue(CorrelationConstants.QueryString, out var queryStringValues))
{
_logContextAccessor.SetValue("CorrelationId", queryStringValues[0]);
_logContextAccessor.SetValue(CorrelationConstants.LogContextKey, queryStringValues[0]);
}
else
{
_logContextAccessor.SetValue(CorrelationConstants.LogContextKey, Guid.NewGuid().ToString());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// See the LICENSE and NOTICES files in the project root for more information.

using EdFi.Ods.Api.ExceptionHandling;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Logging;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Options;
Expand All @@ -24,6 +25,6 @@ public void Configure(ApiBehaviorOptions options)
options.InvalidModelStateResponseFactory = actionContext =>
new BadRequestObjectResult(
ErrorTranslator.GetErrorMessage(actionContext.ModelState,
(string)_logContextAccessor.GetValue("CorrelationId")));
(string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}
}
13 changes: 13 additions & 0 deletions Application/EdFi.Ods.Common/Constants/CorrelationConstants.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// SPDX-License-Identifier: Apache-2.0
// Licensed to the Ed-Fi Alliance under one or more agreements.
// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0.
// See the LICENSE and NOTICES files in the project root for more information.

namespace EdFi.Ods.Common.Constants;

public static class CorrelationConstants
{
public const string LogContextKey = "CorrelationId";
public const string HttpHeader = "correlation-id";
public const string QueryString = "correlationId";
}
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public async Task<IActionResult> Get(string schema, string resource, [FromQuery]
return BadRequest(
ErrorTranslator.GetErrorMessage(
string.Join(" ", parameterMessages),
(string)_logContextAccessor.GetValue("CorrelationId")));
(string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}

// Set authorization context (should this be moved?)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public async Task<IActionResult> Get(
return BadRequest(
ErrorTranslator.GetErrorMessage(
string.Join(" ", parameterMessages),
(string) _logContextAccessor.GetValue("CorrelationId")));
(string) _logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}

var queryParameters = new QueryParameters(urlQueryParametersRequest);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Net.Http;
using System.Threading.Tasks;
using EdFi.Common.Extensions;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Context;
using EdFi.Ods.Common.Logging;
using EdFi.Ods.Features.ChangeQueries.ActionResults;
Expand Down Expand Up @@ -41,7 +42,7 @@ public Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecuti
if (!context.HttpContext.Request.Method.EqualsIgnoreCase(HttpMethod.Get.ToString())
&& !context.HttpContext.Request.Method.EqualsIgnoreCase(HttpMethod.Options.ToString()))
{
context.Result = new SnapshotsAreReadOnlyResult((string) _logContextAccessor.GetValue("CorrelationId"));
context.Result = new SnapshotsAreReadOnlyResult((string) _logContextAccessor.GetValue(CorrelationConstants.LogContextKey));
return Task.CompletedTask;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public IActionResult Get()
string message = e.Message.Replace($"{Environment.NewLine} is used by ", " -> ")
.Replace(Environment.NewLine, " ");

return BadRequest(ErrorTranslator.GetErrorMessage(message, (string) _logContextAccessor.GetValue("CorrelationId")));
return BadRequest(ErrorTranslator.GetErrorMessage(message, (string) _logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public virtual IActionResult Get()
return BadRequest(
ErrorTranslator.GetErrorMessage(
"Limit must be omitted or set to a value between 1 and 100.",
(string)_logContextAccessor.GetValue("CorrelationId")));
(string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}
}

Expand Down Expand Up @@ -265,7 +265,7 @@ IDictionary<string, CompositeSpecificationParameter> GetCompositeSpecificationPa
restError.Code ?? default,
ErrorTranslator.GetErrorMessage(
restError.Message,
(string)_logContextAccessor.GetValue("CorrelationId")));
(string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}

return Ok(json);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ private async Task<IActionResult> GetTokenInformation(TokenInfoRequest tokenInfo
if (tokenInfoRequest == null || tokenInfoRequest.Token == null ||
!Guid.TryParse(tokenInfoRequest.Token, out Guid accessToken))
{
return BadRequest(ErrorTranslator.GetErrorMessage("Invalid token", (string) _logContextAccessor.GetValue("CorrelationId")));
return BadRequest(ErrorTranslator.GetErrorMessage("Invalid token", (string) _logContextAccessor.GetValue(CorrelationConstants.LogContextKey)));
}

var oAuthTokenClientDetails = await _apiClientDetailsProvider.GetApiClientDetailsForTokenAsync(accessToken.ToString("N"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Linq;
using System.Threading.Tasks;
using EdFi.Ods.Api.Models;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Context;
using EdFi.Ods.Common.Logging;
using EdFi.Ods.Common.Metadata.Profiles;
Expand Down Expand Up @@ -195,7 +196,7 @@ async Task WriteResponse(HttpResponse httpResponse, int statusCode, string heade
new RESTError()
{
Message = string.Format(messageFormat, headerName),
CorrelationId = (string)_logContextAccessor.GetValue("CorrelationId")
CorrelationId = (string)_logContextAccessor.GetValue(CorrelationConstants.LogContextKey)
}, Formatting.Indented, _jsonSerializerSettings);

httpResponse.Headers.ContentLength = json.Length;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Net;
using EdFi.Ods.Api.ExceptionHandling;
using EdFi.Ods.Api.Models;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Logging;
using FakeItEasy;
using NUnit.Framework;
Expand Down Expand Up @@ -35,7 +36,7 @@ public void GetRestErrorFromException_ShouldUseTranslator_WhenExceptionIsTransla
};

var logContextAccessor = A.Fake<ILogContextAccessor>();
A.CallTo(() => logContextAccessor.GetValue("CorrelationId")).Returns("12345");
A.CallTo(() => logContextAccessor.GetValue(CorrelationConstants.LogContextKey)).Returns("12345");

var exception = new Exception("Test exception");
var restErrorProvider = new RESTErrorProvider(translators, logContextAccessor);
Expand All @@ -57,7 +58,7 @@ public void GetRestErrorFromException_ShouldUseDefaultError_WhenExceptionIsNotTr
var translators = new List<IExceptionTranslator> { new FakeExceptionTranslator(false, null) };

var logContextAccessor = A.Fake<ILogContextAccessor>();
A.CallTo(() => logContextAccessor.GetValue("CorrelationId")).Returns("54321");
A.CallTo(() => logContextAccessor.GetValue(CorrelationConstants.LogContextKey)).Returns("54321");

var exception = new Exception("Test exception");
var restErrorProvider = new RESTErrorProvider(translators, logContextAccessor);
Expand All @@ -79,7 +80,7 @@ public void GetRestErrorFromException_ShouldUseDefaultErrorWithNoCorrelationId_W
var translators = new List<IExceptionTranslator> { new FakeExceptionTranslator(false, null) };

var logContextAccessor = A.Fake<ILogContextAccessor>();
A.CallTo(() => logContextAccessor.GetValue("CorrelationId")).Returns(null);
A.CallTo(() => logContextAccessor.GetValue(CorrelationConstants.LogContextKey)).Returns(null);

var exception = new Exception("Test exception");
var restErrorProvider = new RESTErrorProvider(translators, logContextAccessor);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0.
// See the LICENSE and NOTICES files in the project root for more information.

using System;
using System.Threading.Tasks;
using EdFi.Ods.Api.Middleware;
using EdFi.Ods.Common.Constants;
using EdFi.Ods.Common.Logging;
using FakeItEasy;
using Microsoft.AspNetCore.Http;
Expand All @@ -24,14 +26,14 @@ public async Task InvokeAsync_WithHeader_CapturesCorrelationId()
var logContextWriter = A.Fake<ILogContextAccessor>();

var middleware = new RequestCorrelationMiddleware(logContextWriter);
context.Request.Headers["correlation-id"] = "123456";
context.Request.Headers[CorrelationConstants.HttpHeader] = "123456";

// Act
await middleware.InvokeAsync(context, nextMiddleware);

// Assert
A.CallTo(() => nextMiddleware(context)).MustHaveHappenedOnceExactly();
A.CallTo(() => logContextWriter.SetValue("CorrelationId", "123456")).MustHaveHappenedOnceExactly();
A.CallTo(() => logContextWriter.SetValue(CorrelationConstants.LogContextKey, "123456")).MustHaveHappenedOnceExactly();
}

[Test]
Expand All @@ -50,7 +52,7 @@ public async Task InvokeAsync_WithQueryString_CapturesCorrelationId()

// Assert
A.CallTo(() => nextMiddleware(context)).MustHaveHappenedOnceExactly();
A.CallTo(() => logContextWriter.SetValue("CorrelationId", "7890")).MustHaveHappenedOnceExactly();
A.CallTo(() => logContextWriter.SetValue(CorrelationConstants.LogContextKey, "7890")).MustHaveHappenedOnceExactly();
}

[Test]
Expand All @@ -62,19 +64,19 @@ public async Task InvokeAsync_WithHeaderAndQueryString_PrefersHeader()
var logContextWriter = A.Fake<ILogContextAccessor>();

var middleware = new RequestCorrelationMiddleware(logContextWriter);
context.Request.Headers["correlation-id"] = "123456";
context.Request.Headers[CorrelationConstants.HttpHeader] = "123456";
context.Request.QueryString = new QueryString("?correlationId=7890");

// Act
await middleware.InvokeAsync(context, nextMiddleware);

// Assert
A.CallTo(() => nextMiddleware(context)).MustHaveHappenedOnceExactly();
A.CallTo(() => logContextWriter.SetValue("CorrelationId", "123456")).MustHaveHappenedOnceExactly();
A.CallTo(() => logContextWriter.SetValue(CorrelationConstants.LogContextKey, "123456")).MustHaveHappenedOnceExactly();
}

[Test]
public async Task InvokeAsync_WithoutCorrelationId_DoesNotSetProperty()
public async Task InvokeAsync_WithoutCorrelationId_SetsPropertyToANewGuid()
{
// Arrange
var context = new DefaultHttpContext();
Expand All @@ -88,6 +90,8 @@ public async Task InvokeAsync_WithoutCorrelationId_DoesNotSetProperty()

// Assert
A.CallTo(() => nextMiddleware(context)).MustHaveHappenedOnceExactly();
A.CallTo(() => logContextWriter.SetValue(A<string>.Ignored, A<object>.Ignored)).MustNotHaveHappened();
A.CallTo(() => logContextWriter.SetValue(CorrelationConstants.LogContextKey, A<object>.That.Matches(x => IsGuid(x)))).MustHaveHappenedOnceExactly();
}

private static bool IsGuid(object x) => Guid.TryParse(x as string, out var ignored);
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@
"\r",
"const response = pm.response.json();\r",
"\r",
"pm.test(\"Should not include a correlation id in response\", () => {\r",
" pm.expect(response).to.deep.equal({\r",
"pm.test(\"Should include a server-generated correlation id in the error response\", () => {\r",
" pm.expect(response).to.deep.include({\r",
" \"message\": \"Unexpected character encountered while parsing value: }. Path '', line 0, position 0.\"\r",
" });\r",
"\r",
" pm.expect(response.correlationId).to.exist;\r",
"});\r",
""
],
Expand Down Expand Up @@ -5837,7 +5839,7 @@
],
"body": {
"mode": "raw",
"raw": "{\r\n \"studentUniqueId\": \"{{supplied:{{scenarioId}}:studentUniqueId}}\",\r\n \"birthDate\":\"{{supplied:{{scenarioId}}:birthDate}}\",\r\n \"firstName\": \"{{supplied:{{scenarioId}}:firstName}}\",\r\n \"lastSurname\": \"{{supplied:{{scenarioId}}:lastSurname}}\"\r\n}"
"raw": "{\r\n \"studentUniqueId\": \"{{supplied:{{scenarioId}}:studentUniqueId}}\",\r\n \"birthDate\": \"{{supplied:{{scenarioId}}:birthDate}}\",\r\n \"firstName\": \"{{supplied:{{scenarioId}}:firstName}}\",\r\n \"lastSurname\": \"{{supplied:{{scenarioId}}:lastSurname}}\"\r\n}"
},
"url": {
"raw": "{{ApiBaseUrl}}/data/v3/ed-fi/students",
Expand Down

0 comments on commit 191f026

Please sign in to comment.