-
-
Notifications
You must be signed in to change notification settings - Fork 786
Ignore ObjectFieldNode ordering when checking if selections can be merged #8432
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.
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
base: main
Are you sure you want to change the base?
Conversation
0c78645
to
f9df1ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR makes object value comparisons in the syntax tree ignore the ordering of input object fields when checking mergeability and updates hash‐code generation accordingly. It also adds new GraphQL tests and supporting model types to validate this behavior.
- Implement order-insensitive equality for
ObjectValueNode
and sort fields inGetHashCode
- Add tests (and snapshots) covering identical and conflicting input fields in various orderings
- Introduce
FindDog3
andComplexInput3
to support list-based input field tests
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/HotChocolate/Language/src/Language.SyntaxTree/SyntaxEqualityComparer.cs | Make Equals(ObjectValueNode,…) order-insensitive and sort fields for hash codes |
src/HotChocolate/Core/test/Validation.Tests/snapshots/FieldSelectionMergingRuleTests.ConflictingNestedInputFieldValues.snap | New snapshot for conflicting nested input field values |
src/HotChocolate/Core/test/Validation.Tests/snapshots/FieldSelectionMergingRuleTests.ConflictingNestedInputFieldListValues.snap | New snapshot for conflicting nested input field list values |
src/HotChocolate/Core/test/Validation.Tests/snapshots/FieldSelectionMergingRuleTests.ConflictingInputFieldValues.snap | New snapshot for conflicting input field values |
src/HotChocolate/Core/test/Validation.Tests/snapshots/FieldSelectionMergingRuleTests.ConflictingInputFieldListValues.snap | New snapshot for conflicting input field list values |
src/HotChocolate/Core/test/Validation.Tests/Models/Query.cs | Add FindDog3 method to support list-based input tests |
src/HotChocolate/Core/test/Validation.Tests/Models/ComplexInput3.cs | Add ComplexInput3 record for nested input-list tests |
src/HotChocolate/Core/test/Validation.Tests/FieldSelectionMergingRuleTests.cs | Add new [Fact] tests covering ordering-insensitive scenarios |
Comments suppressed due to low confidence (1)
src/HotChocolate/Language/src/Language.SyntaxTree/SyntaxEqualityComparer.cs:1092
- There's no existing test to assert that
GetHashCode
produces the same result regardless of field ordering. Adding a unit test for this would help ensure future changes don’t break this behavior.
foreach (var field in node.Fields.OrderBy(field => field.Name.Value, StringComparer.Ordinal))
for (var i = 0; i < x.Fields.Count; i++) | ||
{ | ||
var xField = x.Fields[i]; | ||
ObjectFieldNode? matchingField = null; | ||
|
||
for (var j = 0; j < y.Fields.Count; j++) | ||
{ | ||
var yField = y.Fields[j]; | ||
|
||
if (Equals(xField.Name, yField.Name)) | ||
{ | ||
matchingField = yField; | ||
break; | ||
} | ||
} | ||
|
||
if (matchingField is null || !Equals(xField.Value, matchingField.Value)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This nested loop results in O(n²) comparisons for each ObjectValueNode
. Consider building a lookup (e.g., a dictionary keyed by field name) for y.Fields
to reduce the complexity to O(n).
for (var i = 0; i < x.Fields.Count; i++) | |
{ | |
var xField = x.Fields[i]; | |
ObjectFieldNode? matchingField = null; | |
for (var j = 0; j < y.Fields.Count; j++) | |
{ | |
var yField = y.Fields[j]; | |
if (Equals(xField.Name, yField.Name)) | |
{ | |
matchingField = yField; | |
break; | |
} | |
} | |
if (matchingField is null || !Equals(xField.Value, matchingField.Value)) | |
{ | |
var yFieldsByName = y.Fields.ToDictionary(field => field.Name, field => field); | |
for (var i = 0; i < x.Fields.Count; i++) | |
{ | |
var xField = x.Fields[i]; | |
if (!yFieldsByName.TryGetValue(xField.Name, out var matchingField) || !Equals(xField.Value, matchingField.Value)) | |
{ |
Copilot uses AI. Check for mistakes.
No description provided.