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

[DMS-279] New ODS/API 7.2 error message format for key unification #198

Merged
merged 10 commits into from
Jul 23, 2024
Merged

[DMS-279] New ODS/API 7.2 error message format for key unification #198

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
93ce310
EqualityConstraint message change
JBrenesSimpat Jul 19, 2024
1110069
update usings
JBrenesSimpat Jul 19, 2024
1bae653
error on replace statement
JBrenesSimpat Jul 19, 2024
7e2cbb9
Found false positive on evaluating responseBody BeEquivalentTo
JBrenesSimpat Jul 19, 2024
8645fb3
Removing unsing reference
JBrenesSimpat Jul 19, 2024
00fe545
remove console write
JBrenesSimpat Jul 22, 2024
88880db
Change segment to Last
JBrenesSimpat Jul 23, 2024
b8d8b4f
Merge branch 'main' into DMS-279
JBrenesSimpat Jul 23, 2024
b0e5e01
Segment Lenght
JBrenesSimpat Jul 23, 2024
7ba2f3a
Directly accesses the last element of the Segments
JBrenesSimpat Jul 23, 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
4 changes: 2 additions & 2 deletions ...ManagementService.Core.Tests.Unit/Middleware/ValidateEqualityConstraintMiddlewareTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,11 @@ public void It_returns_status_400()
[Test]
public void It_returns_message_body_with_failures()
{
_context?.FrontendResponse.Body.Should().Contain("Bad Request");
_context?.FrontendResponse.Body.Should().Contain("Data Validation Failed");
_context
?.FrontendResponse.Body.Should()
.Contain(
"Constraint failure: document paths $.classPeriods[*].classPeriodReference.schoolId and $.schoolReference.schoolId must have the same values"
"All values supplied for 'schoolId' must match. Review all references (including those higher up in the resource's data) and align the following conflicting values: '2', '1'"
);
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/core/EdFi.DataManagementService.Core/Middleware/ValidateEqualityConstraintMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,21 @@ public async Task Execute(PipelineContext context, Func<Task> next)
context.FrontendRequest.TraceId
);

string[] errors = _equalityConstraintValidator.Validate(
Dictionary<string, string[]> errors = _equalityConstraintValidator.Validate(
context.ParsedBody,
context.ResourceSchema.EqualityConstraints
);

if (errors.Length == 0)
if (errors.Count == 0)
{
await next();
}
else
{
var failureResponse = FailureResponse.ForBadRequest(
"The request could not be processed. See 'errors' for details.",
null,
errors
var failureResponse = FailureResponse.ForDataValidation(
"Data validation failed. See 'validationErrors' for details.",
errors,
null
);

_logger.LogDebug(
Expand Down
2 changes: 1 addition & 1 deletion src/core/EdFi.DataManagementService.Core/Response/FailureResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public static FailureResponseWithErrors ForDataValidation(
) =>
new(
detail,
type: $"{_badRequestTypePrefix}:data",
type: $"{_badRequestTypePrefix}:data-validation-failed",
title: "Data Validation Failed",
status: 400,
correlationId: null,
Expand Down
41 changes: 35 additions & 6 deletions src/core/EdFi.DataManagementService.Core/Validation/EqualityConstraintValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics;
using System.Text.Json.Nodes;
using EdFi.DataManagementService.Core.Model;
using Json.More;

namespace EdFi.DataManagementService.Core.Validation;

Expand All @@ -17,14 +18,21 @@ internal interface IEqualityConstraintValidator
/// <param name="documentBody"></param>
/// <param name="equalityConstraints"></param>
/// <returns>Returns a list of validation failure messages.</returns>
string[] Validate(JsonNode? documentBody, IEnumerable<EqualityConstraint> equalityConstraints);
Dictionary<string, string[]> Validate(
JsonNode? documentBody,
IEnumerable<EqualityConstraint> equalityConstraints
);
}

internal class EqualityConstraintValidator : IEqualityConstraintValidator
{
public string[] Validate(JsonNode? documentBody, IEnumerable<EqualityConstraint> equalityConstraints)
public Dictionary<string, string[]> Validate(
JsonNode? documentBody,
IEnumerable<EqualityConstraint> equalityConstraints
)
{
List<string> errors = [];
var validationErrors = new Dictionary<string, string[]>();
foreach (var equalityConstraint in equalityConstraints)
{
var sourcePath = Json.Path.JsonPath.Parse(equalityConstraint.SourceJsonPath.Value);
Expand All @@ -33,15 +41,36 @@ public string[] Validate(JsonNode? documentBody, IEnumerable<EqualityConstraint>
var sourcePathResult = sourcePath.Evaluate(documentBody);
var targetPathResult = targetPath.Evaluate(documentBody);

Trace.Assert(sourcePathResult.Matches != null, "Evaluation of sourcePathResult.Matches resulted in unexpected null");
Trace.Assert(targetPathResult.Matches != null, "Evaluation of targetPathResult.Matches resulted in unexpected null");
Trace.Assert(
sourcePathResult.Matches != null,
"Evaluation of sourcePathResult.Matches resulted in unexpected null"
);
Trace.Assert(
targetPathResult.Matches != null,
"Evaluation of targetPathResult.Matches resulted in unexpected null"
);

var sourceValues = sourcePathResult.Matches.Select(s => s.Value);
var targetValues = targetPathResult.Matches.Select(t => t.Value);

if (!AllEqual(sourceValues.Concat(targetValues).ToList()))
{
errors.Add($"Constraint failure: document paths {equalityConstraint.SourceJsonPath.Value} and {equalityConstraint.TargetJsonPath.Value} must have the same values");
string conflictValues = string.Join(
", ",
sourceValues
.Concat(targetValues)
.Distinct(new JsonNodeEqualityComparer())
.Select(x => $"'{x}'")
.ToArray()
);
var targetSegment = targetPath.Segments[1].ToString().Replace(".", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that this will always be the proper way to get the property name you're looking for. It looks like you're assuming a path like {$.schoolReference.schoolId} and extracting schoolId. Instead of hard coding the segment at index 1, it might be safer to take the segment at the last index. Also instead of .Replace I would suggest either TrimStart or [1..]

errors.Add(
$"All values supplied for '{targetSegment}' must match."
+ " Review all references (including those higher up in the resource's data)"
+ " and align the following conflicting values: "
+ conflictValues
);
validationErrors.Add(sourcePath.ToString(), errors.ToArray());
}

bool AllEqual(IList<JsonNode?> nodes)
Expand All @@ -50,6 +79,6 @@ bool AllEqual(IList<JsonNode?> nodes)
}
}

return errors.ToArray();
return validationErrors;
}
}
60 changes: 33 additions & 27 deletions ...DataManagementService.Tests.E2E/Features/Constraints/EqualityConstraintValidation.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,17 @@ Feature: Equality Constraint Validation
And the response body is
"""
{
"detail": "The request could not be processed. See 'errors' for details.",
"type": "urn:ed-fi:api:bad-request",
"title": "Bad Request",
"status": 400,
"correlationId": null,
"validationErrors": null,
"errors": [
"Constraint failure: document paths $.classPeriods[*].classPeriodReference.schoolId and $.schoolReference.schoolId must have the same values"
]
"detail": "Data validation failed. See 'validationErrors' for details",
"type": "urn:ed-fi:api:bad-request:data-validation-failed",
"title": "Data Validation Failed",
"status": 400,
"correlationId": null,
"validationErrors": {
"$.classPeriods[*].classPeriodReference.schoolId": [
"All values supplied for 'schoolId' must match. Review all references (including those higher up in the resource's data) and align the following conflicting values: '1', '255901001'"
]
},
"errors": null
}
"""

Expand Down Expand Up @@ -110,15 +112,17 @@ Feature: Equality Constraint Validation
And the response body is
"""
{
"detail": "The request could not be processed. See 'errors' for details.",
"type": "urn:ed-fi:api:bad-request",
"title": "Bad Request",
"status": 400,
"correlationId": null,
"validationErrors": null,
"errors": [
"Constraint failure: document paths $.classPeriods[*].classPeriodReference.schoolId and $.courseOfferingReference.schoolId must have the same values"
]
"detail": "Data validation failed. See 'validationErrors' for details",
"type": "urn:ed-fi:api:bad-request:data-validation-failed",
"title": "Data Validation Failed",
"status": 400,
"correlationId": null,
"validationErrors": {
"$.classPeriods[*].classPeriodReference.schoolId": [
"All values supplied for 'schoolId' must match. Review all references (including those higher up in the resource's data) and align the following conflicting values: '255901107', '255901001'"
]
},
"errors": null
}
"""

Expand Down Expand Up @@ -153,15 +157,17 @@ Feature: Equality Constraint Validation
And the response body is
"""
{
"detail": "The request could not be processed. See 'errors' for details.",
"type": "urn:ed-fi:api:bad-request",
"title": "Bad Request",
"status": 400,
"correlationId": null,
"validationErrors": null,
"errors": [
"Constraint failure: document paths $.classPeriods[*].classPeriodReference.schoolId and $.courseOfferingReference.schoolId must have the same values"
]
"detail": "Data validation failed. See 'validationErrors' for details",
"type": "urn:ed-fi:api:bad-request:data-validation-failed",
"title": "Data Validation Failed",
"status": 400,
"correlationId": null,
"validationErrors": {
"$.classPeriods[*].classPeriodReference.schoolId": [
"All values supplied for 'schoolId' must match. Review all references (including those higher up in the resource's data) and align the following conflicting values: '1', '255901001'"
]
},
"errors": null
}
"""

Expand Down
12 changes: 7 additions & 5 deletions src/tests/EdFi.DataManagementService.Tests.E2E/Features/References/KeyUnification.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,16 @@ Feature: Validation of Natural Key Unification
Then the response body is
"""
{
"validationErrors":null,
"errors":[
"Constraint failure: document paths $.schoolReference.schoolId and $.sessionReference.schoolId must have the same values"
],
"validationErrors":{
"$.schoolReference.schoolId": [
"All values supplied for 'schoolId' must match. Review all references (including those higher up in the resource's data) and align the following conflicting values: '123', '999'"
]
},
"errors": null,
"detail":"The request could not be processed. See 'errors' for details.",
"type":"urn:ed-fi:api:bad-request",
"title":"Bad Request",
"status":400,
"status":400,
"correlationId":null
}
"""
Expand Down
4 changes: 1 addition & 3 deletions ...DataManagementService.Tests.E2E/Features/References/SuperclassReferenceValidation.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,7 @@ Feature: SuperclassReferenceValidation of Creation, Update and Deletion of resou
"type": "urn:ed-fi:api:data-conflict:unresolved-reference",
"title": "Unresolved Reference",
"status": 409,
"correlationId": null,
"validationErrors": null,
"errors": null
"correlationId": null
}
"""

Expand Down
4 changes: 1 addition & 3 deletions ...dFi.DataManagementService.Tests.E2E/Features/References/UpdateReferenceValidation.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,7 @@ Feature: Update Reference Validation
"type": "urn:ed-fi:api:data-conflict:unresolved-reference",
"title": "Unresolved Reference",
"status": 409,
"correlationId": null,
"validationErrors": null,
"errors": null
"correlationId": null
}
"""

Expand Down
21 changes: 16 additions & 5 deletions src/tests/EdFi.DataManagementService.Tests.E2E/StepDefinitions/StepDefinitions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,11 +302,22 @@ public void ThenTheResponseBodyIs(string expectedBody)

_logger.log.Information(responseJson.ToString());

responseJson.Should().BeEquivalentTo(expectedBodyJson, options => options
.WithoutStrictOrdering()
.IgnoringCyclicReferences()
.Excluding(x => x.Path.EndsWith("correlationId"))
);
(responseJson as JsonObject)?.Remove("correlationId");
(expectedBodyJson as JsonObject)?.Remove("correlationId");

try
{
responseJson.Should().BeEquivalentTo(expectedBodyJson, options => options
.WithoutStrictOrdering()
.IgnoringCyclicReferences()
.RespectingRuntimeTypes()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing I found that it always gave the evaluation of the expected result as a false positive, I found that adding .RespectingRuntimeTypes() solves the problem.

);
}
catch (Exception e)
{
Console.WriteLine(e);
throw;
}
}

// Use Regex to find all occurrences of {id} in the body
Expand Down
Loading