From 1df56c4064035ea018bf95e64191ca6f38e60ee7 Mon Sep 17 00:00:00 2001 From: Michael Staib Date: Tue, 10 Dec 2024 16:33:52 +0100 Subject: [PATCH 1/6] Adds field requirements. --- .../Planning/OperationPlanner.cs | 21 +--- .../Fusion.Execution.Tests/FusionTestBase.cs | 7 ++ ...HotChocolate.Fusion.Execution.Tests.csproj | 3 + .../LookupRequirementsTests.cs | 111 ++++++++++++++++++ .../__resources__/fusion2.graphql | 41 +++++++ 5 files changed, 163 insertions(+), 20 deletions(-) create mode 100644 src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/LookupRequirementsTests.cs create mode 100644 src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__resources__/fusion2.graphql diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs index ebcd5dda618..3e9e6ee65f4 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs @@ -135,7 +135,7 @@ private bool TryPlanFieldSelection( // root fields, so we now the field will be resolvable on the // source schema. if (context.Parent is OperationPlanNode - || IsResolvable(fieldNode, field, context.Operation.SchemaName)) + || field.Sources.ContainsSchema(context.Operation.SchemaName)) { var fieldNamedType = field.Type.NamedType(); @@ -390,20 +390,6 @@ private static bool IsEntityPathResolvable(Stack entityPath, string sc return true; } - // this needs more meat - private bool IsResolvable( - FieldNode fieldNode, - CompositeOutputField field, - string schemaName) - => field.Sources.ContainsSchema(schemaName); - - // this needs more meat - private bool IsResolvable( - InlineFragmentNode inlineFragment, - CompositeComplexType typeCondition, - string schemaName) - => typeCondition.Sources.ContainsSchema(schemaName); - private static bool TryGetLookup( SelectionPlanNode selection, string schemaName, @@ -709,11 +695,6 @@ public record UnresolvedType( InlineFragmentNode InlineFragment, CompositeComplexType TypeCondition); - public class RequestPlanNode - { - public ICollection Operations { get; } = new List(); - } - private record struct LookupOperation( OperationPlanNode Operation, FieldPlanNode Field); diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/FusionTestBase.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/FusionTestBase.cs index 028b82a5936..6c51be936a7 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/FusionTestBase.cs +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/FusionTestBase.cs @@ -1,3 +1,4 @@ +using System.Diagnostics.CodeAnalysis; using HotChocolate.Fusion.Planning; using HotChocolate.Fusion.Planning.Nodes; using HotChocolate.Fusion.Types; @@ -14,6 +15,12 @@ protected static CompositeSchema CreateCompositeSchema() return CompositeSchemaBuilder.Create(compositeSchemaDoc); } + protected static CompositeSchema CreateCompositeSchema([StringSyntax("graphql")] string schema) + { + var compositeSchemaDoc = Utf8GraphQLParser.Parse(schema); + return CompositeSchemaBuilder.Create(compositeSchemaDoc); + } + protected static RootPlanNode PlanOperationAsync(CompositeSchema compositeSchema, string operation) { var doc = Utf8GraphQLParser.Parse(operation); diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/HotChocolate.Fusion.Execution.Tests.csproj b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/HotChocolate.Fusion.Execution.Tests.csproj index 33db404e773..b9c2c54e37f 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/HotChocolate.Fusion.Execution.Tests.csproj +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/HotChocolate.Fusion.Execution.Tests.csproj @@ -9,6 +9,9 @@ Always + + Always + diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/LookupRequirementsTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/LookupRequirementsTests.cs new file mode 100644 index 00000000000..5a9a7f12fb3 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/LookupRequirementsTests.cs @@ -0,0 +1,111 @@ +using HotChocolate.Fusion.Planning; + +namespace HotChocolate.Fusion; + +public class LookupRequirementsTests : FusionTestBase +{ + [Test] + public void Key_Has_Requirement_To_Schema_That_Is_Not_In_Context() + { + var schema = CreateCompositeSchema( + """ + type Query { + productById(id: ID!): Product + @fusion__field(schema: PRODUCTS) + } + + type Product + @fusion__type(schema: PRODUCTS) + @fusion__type(schema: SHIPPING) + @fusion__type(schema: REVIEWS) + @fusion__lookup( + schema: PRODUCTS + key: "{ id }" + field: "productById(id: ID!): Product" + map: ["id"] + ) + @fusion__lookup( + schema: SHIPPING + key: "{ id }" + field: "productById(id: ID!): Product" + map: ["id"] + ) + @fusion__lookup( + schema: REVIEWS + key: "{ id }" + field: "productById(id: ID!): Product" + map: ["id"] + ) { + id: ID! + @fusion__field(schema: PRODUCTS) + @fusion__field(schema: SHIPPING) + @fusion__field(schema: REVIEWS) + internalId: String! + @fusion__field(schema: SHIPPING) + internalSomeOther: String! + @fusion__field(schema: REVIEWS) + @fusion__requires( + schema: SHIPPING + field: "internalSomeOther(internalId: String!): String!" + map: ["internalId"] + ) + } + """); + + var plan = PlanOperationAsync( + schema, + """ + { + productById(id: 1) { + internalSomeOther + } + } + """); + + plan.ToYaml().MatchInlineSnapshot( + """ + nodes: + - id: 1 + schema: "PRODUCTS" + operation: >- + { + productById(id: 1) { + id + } + } + - id: 2 + schema: "SHIPPING + operation: >- + { + productById(id: 1) { + internalId + } + } + requirements: + - name: "__fusion_requirement_1" + dependsOn: "1" + selectionSet: "productById" + field: "id" + type: "ID!" + - id: 3 + schema: "SHIPPING" + operation: >- + query($__fusion_requirement_1: ID!, $__fusion_requirement_2: String!) { + productById(id: $__fusion_requirement_1) { + internalSomeOther(internalId: $__fusion_requirement_2) + } + } + requirements: + - name: "__fusion_requirement_1" + dependsOn: "1" + selectionSet: "productById" + field: "id" + type: "ID!" + - name: "__fusion_requirement_2" + dependsOn: "2" + selectionSet: "productById" + field: "internalId" + type: "String!" + """); + } +} diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__resources__/fusion2.graphql b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__resources__/fusion2.graphql new file mode 100644 index 00000000000..4616c08242f --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__resources__/fusion2.graphql @@ -0,0 +1,41 @@ +type Query { + productById(id: ID!): Product + @fusion__field(schema: PRODUCTS) +} + +type Product + @fusion__type(schema: PRODUCTS) + @fusion__type(schema: SHIPPING) + @fusion__type(schema: REVIEWS) + @fusion__lookup( + schema: PRODUCTS + key: "{ id }" + field: "productById(id: ID!): Product" + map: ["id"] + ) + @fusion__lookup( + schema: SHIPPING + key: "{ id }" + field: "productById(id: ID!): Product" + map: ["id"] + ) + @fusion__lookup( + schema: REVIEWS + key: "{ id }" + field: "productById(id: ID!): Product" + map: ["id"] + ) { + id: ID! + @fusion__field(schema: PRODUCTS) + @fusion__field(schema: SHIPPING) + @fusion__field(schema: REVIEWS) + internalId: String! + @fusion__field(schema: SHIPPING) + internalSomeOther: String! + @fusion__field(schema: SHIPPING) + @fusion__requires( + schema: SHIPPING + field: "internalSomeOther(internalId: String!): String!" + map: ["internalId"] + ) +} From 77c282f65a4a7a3d9ea9aba2ace639ff1dffe72b Mon Sep 17 00:00:00 2001 From: Michael Staib Date: Tue, 17 Dec 2024 13:05:18 +0100 Subject: [PATCH 2/6] merge --- .../Planning/OperationPlanner.cs | 94 ++++++++++++------- .../Completion/CompositeSchemaBuilder.cs | 15 ++- .../Directives/DirectiveParserException.cs | 17 +++- .../Types/FieldRequirements.cs | 9 +- .../src/Fusion.Execution/Types/Lookup.cs | 6 ++ .../Utilities/FieldSelectionMapUtilities.cs | 54 +++++++++++ .../Fusion.Execution.Tests/FusionTestBase.cs | 12 ++- .../LookupRequirementsTests.cs | 6 +- .../__resources__/fusion1.graphql | 2 +- 9 files changed, 168 insertions(+), 47 deletions(-) create mode 100644 src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Utilities/FieldSelectionMapUtilities.cs diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs index 7acf610401b..3531e062da9 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs @@ -30,7 +30,11 @@ public RequestPlanNode CreatePlan(DocumentNode document, string? operationName) schema.QueryType, operationDefinition.SelectionSet); - var context = new PlaningContext(operation, operation, ImmutableStack.Empty); + var context = new PlaningContext( + operation, + operation, + ImmutableStack.Empty); + if (TryPlanSelectionSet(context)) { TryMakeOperationConditional(operation, operation.Selections); @@ -54,7 +58,6 @@ private bool TryPlanSelectionSet( } List? unresolvedSelections = null; - // List? unresolvedTypes = null; var type = (CompositeComplexType)context.Parent.DeclaringType; var haveConditionalSelectionsBeenRemoved = false; @@ -205,8 +208,19 @@ private bool TryPlanFieldSelection( if (context.Parent is OperationPlanNode || field.Sources.ContainsSchema(context.Operation.SchemaName)) { + var source = field.Sources[context.Operation.SchemaName]; var fieldNamedType = field.Type.NamedType(); + if (source.Requirements is not null) + { + var unresolvedRequirements = ParseSelectionSet(type, source.Requirements.SelectionSet); + foreach (var requiredField in unresolvedRequirements) + { + // TODO : reintegrate + // trackRequiredField(new UnresolvedField(requiredField.FieldNode, requiredField.Field)); + } + } + // if the field has no selection set it must be a leaf type. // This also means that if this field is resolvable that we can // just include it and no further processing is required. @@ -224,7 +238,7 @@ private bool TryPlanFieldSelection( return true; } - // if this field as a selection set it must be a object, interface or union type, + // if this field as a selection set it must be an object, interface or union type, // otherwise the validation should have caught this. So, we just throw here if this // is not the case. if (fieldNamedType.Kind != TypeKind.Object @@ -321,7 +335,7 @@ private bool TryHandleUnresolvedSelections( continue; } - // note : this can lead to a operation explosions as fields could be unresolvable + // note : this can lead to an operation explosions as fields could be unresolvable // and would be spread out in the lower level call. We do that for now to test out the // overall concept and will backtrack later to the upper call. selections.Clear(); @@ -364,15 +378,14 @@ private bool TryHandleUnresolvedSelections( } } - var (lookupOperation, lookupField) = - CreateLookupOperation(schemaName, lookup, type, context.Parent, selections); - if (!TryPlanSelectionSet(context with { Operation = lookupOperation, Parent = lookupField }, true)) + var (lookupOp, lookupField) = CreateLookupOperation(schemaName, lookup, type, context.Parent, selections); + if (!TryPlanSelectionSet(context with { Operation = lookupOp, Parent = lookupField }, true)) { continue; } - schemasInContext.Add(schemaName, lookupOperation); - TryMakeOperationConditional(lookupOperation, lookupField.Selections); + schemasInContext.Add(schemaName, lookupOp); + TryMakeOperationConditional(lookupOp, lookupField.Selections); // we add the lookup operation to all the schemas that we have requirements with. foreach (var requiredSchema in fieldSchemaDependencies.Values.Distinct()) @@ -380,7 +393,7 @@ private bool TryHandleUnresolvedSelections( // Add child node is wrong ... this is a graph and the lookup operation has dependencies on // this operation. We should probably double link here. // maybe AddDependantNode()? - schemasInContext[requiredSchema].AddDependantOperation(lookupOperation); + schemasInContext[requiredSchema].AddDependantOperation(lookupOp); } // add requirements to the operation @@ -420,7 +433,7 @@ private bool TryHandleUnresolvedSelections( requiredField, argument.Type); - lookupOperation.AddRequirement(requirement); + lookupOp.AddRequirement(requirement); lookupField.AddArgument(new ArgumentAssignment(argument.Name, new VariableNode(requirementName))); } @@ -739,7 +752,7 @@ private static FieldNode CreateFieldNodeFromPath(FieldPath path) new SelectionSetNode([current])); } - return current!; + return current; } private void TryMakeOperationConditional( @@ -796,32 +809,51 @@ private static bool IsSelectionAlwaysSkipped(ISelectionNode selectionNode) var isSkipDirective = directive.Name.Value == "skip"; var isIncludedDirective = directive.Name.Value == "include"; - if (isSkipDirective || isIncludedDirective) + if (!isSkipDirective && !isIncludedDirective) { - var ifArgument = directive.Arguments.FirstOrDefault(a => a.Name.Value == "if"); + continue; + } - if (ifArgument is not null) - { - if (ifArgument.Value is BooleanValueNode booleanValueNode) - { - if (booleanValueNode.Value && isSkipDirective) - { - return true; - } + var ifArgument = directive.Arguments.FirstOrDefault(a => a.Name.Value == "if"); - if (!booleanValueNode.Value && isIncludedDirective) - { - return true; - } - } - } + if (ifArgument?.Value is not BooleanValueNode booleanValueNode) + { + continue; + } + + if ((isSkipDirective && booleanValueNode.Value) + || (isIncludedDirective && !booleanValueNode.Value)) + { + return true; } } return false; } - // TODO: Needs to be scoped on operation unless planner is transient + private static IReadOnlyList ParseSelectionSet( + CompositeComplexType type, + SelectionSetNode selectionSetNode) + { + var unresolvedFields = new List(); + + foreach (var selectionNode in selectionSetNode.Selections) + { + if (selectionNode is FieldNode fieldNode) + { + if (!type.Fields.TryGetField(fieldNode.Name.Value, out var field)) + { + throw new InvalidOperationException( + "There is an unknown field in the selection set."); + } + + unresolvedFields.Add(new UnresolvedField(fieldNode, field)); + } + } + + return unresolvedFields; + } + private string GetNextRequirementName() => $"__fusion_requirement_{++_lastRequirementId}"; @@ -834,10 +866,6 @@ public record UnresolvedField( FieldNode FieldNode, CompositeOutputField Field) : IUnresolvedSelection; - public record UnresolvedType( - InlineFragmentNode InlineFragment, - CompositeComplexType TypeCondition); - public record UnresolvedInlineFragment( IReadOnlyList Directives, CompositeComplexType TypeCondition, diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Completion/CompositeSchemaBuilder.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Completion/CompositeSchemaBuilder.cs index cfa4ca27d11..1a10571a108 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Completion/CompositeSchemaBuilder.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Completion/CompositeSchemaBuilder.cs @@ -2,6 +2,7 @@ using System.Collections.Immutable; using HotChocolate.Fusion.Types.Collections; using HotChocolate.Fusion.Types.Directives; +using HotChocolate.Fusion.Utilities; using HotChocolate.Language; namespace HotChocolate.Fusion.Types.Completion; @@ -295,21 +296,25 @@ private static SourceObjectFieldCollection BuildSourceObjectFieldCollection( if (requireDirective is not null) { - var arguments = ImmutableArray.CreateBuilder(); + var argumentsBuilder = ImmutableArray.CreateBuilder(); foreach (var argument in requireDirective.Field.Arguments) { - arguments.Add(new RequiredArgument(argument.Name.Value, argument.Type)); + argumentsBuilder.Add(new RequiredArgument(argument.Name.Value, argument.Type)); } - var fields = ImmutableArray.CreateBuilder(); + var fieldsBuilder = ImmutableArray.CreateBuilder(); foreach (var field in requireDirective.Map) { - fields.Add(FieldPath.Parse(field)); + fieldsBuilder.Add(FieldPath.Parse(field)); } - return new FieldRequirements(schemaName, arguments.ToImmutable(), fields.ToImmutable()); + var arguments = argumentsBuilder.ToImmutable(); + var fields = fieldsBuilder.ToImmutable(); + var selectionSet = fields.ToSelectionSetNode(); + + return new FieldRequirements(schemaName, arguments, fields, selectionSet); } return null; diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Directives/DirectiveParserException.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Directives/DirectiveParserException.cs index 2ce73c73002..a90ef46f19e 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Directives/DirectiveParserException.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Directives/DirectiveParserException.cs @@ -1,4 +1,17 @@ namespace HotChocolate.Fusion.Types.Directives; -internal class DirectiveParserException(string message) - : Exception(message); +internal class DirectiveParserException : Exception +{ + public DirectiveParserException() + { + } + + public DirectiveParserException(string message) : base(message) + { + } + + public DirectiveParserException(string? message, Exception? innerException) + : base(message, innerException) + { + } +} diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/FieldRequirements.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/FieldRequirements.cs index 9436847fc14..aa9763ac920 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/FieldRequirements.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/FieldRequirements.cs @@ -1,11 +1,13 @@ using System.Collections.Immutable; +using HotChocolate.Language; namespace HotChocolate.Fusion.Types; public sealed class FieldRequirements( string schemaName, ImmutableArray arguments, - ImmutableArray fields) + ImmutableArray fields, + SelectionSetNode selectionSet) { /// /// Gets the name of the source schema that has requirements. for a field. @@ -21,4 +23,9 @@ public sealed class FieldRequirements( /// Gets the paths to the field that are required. /// public ImmutableArray Fields { get; } = fields; + + /// + /// Gets the selection set that represents the field requirements. + /// + public SelectionSetNode SelectionSet { get; } = selectionSet; } diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Lookup.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Lookup.cs index 7dc72284ecc..751affe1601 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Lookup.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Types/Lookup.cs @@ -1,4 +1,5 @@ using System.Collections.Immutable; +using HotChocolate.Language; namespace HotChocolate.Fusion.Types; @@ -35,4 +36,9 @@ public Lookup( /// Gets the paths to the field that are required. /// public ImmutableArray Fields { get; } + + /// + /// Gets the complexity score of fulfilling the requirements. + /// + public int RequirementsCost { get; set; } } diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Utilities/FieldSelectionMapUtilities.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Utilities/FieldSelectionMapUtilities.cs new file mode 100644 index 00000000000..cea84c7101f --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Utilities/FieldSelectionMapUtilities.cs @@ -0,0 +1,54 @@ +using System.Collections.Immutable; +using HotChocolate.Fusion.Planning.Nodes; +using HotChocolate.Fusion.Types; +using HotChocolate.Language; + +namespace HotChocolate.Fusion.Utilities; + +internal static class FieldSelectionMapUtilities +{ + public static FieldPath CreateFieldPath(ImmutableStack path) + { + var current = FieldPath.Root; + + foreach (var segment in path.Reverse()) + { + if (segment is FieldPlanNode field) + { + current = current.Append(field.Field.Name); + } + } + + return current; + } + + public static FieldNode ToFieldNode(this FieldPath path) + { + var current = new FieldNode(path.Name); + + foreach (var segment in path.Skip(1)) + { + current = new FieldNode( + null, + new NameNode(segment.Name), + null, + Array.Empty(), + Array.Empty(), + new SelectionSetNode([current])); + } + + return current; + } + + public static SelectionSetNode ToSelectionSetNode(this ImmutableArray paths) + { + var selections = new ISelectionNode[paths.Length]; + + for (var i = 0; i < paths.Length; i++) + { + selections[i] = paths[i].ToFieldNode(); + } + + return new SelectionSetNode(selections); + } +} diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/FusionTestBase.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/FusionTestBase.cs index 9d80bf894d7..9c07c416973 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/FusionTestBase.cs +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/FusionTestBase.cs @@ -15,14 +15,22 @@ protected static CompositeSchema CreateCompositeSchema() return CompositeSchemaBuilder.Create(compositeSchemaDoc); } - protected static CompositeSchema CreateCompositeSchema([StringSyntax("graphql")] string schema) + protected static CompositeSchema CreateCompositeSchema( + [StringSyntax("graphql")] string schema) { var compositeSchemaDoc = Utf8GraphQLParser.Parse(schema); return CompositeSchemaBuilder.Create(compositeSchemaDoc); } - protected static RequestPlanNode PlanOperation(DocumentNode request, CompositeSchema compositeSchema) + protected static RequestPlanNode PlanOperation( + [StringSyntax("graphql")] string request, + CompositeSchema compositeSchema) + { + var document = Utf8GraphQLParser.Parse(request); + return PlanOperation(document, compositeSchema); + } + protected static RequestPlanNode PlanOperation(DocumentNode request, CompositeSchema compositeSchema) { var rewriter = new InlineFragmentOperationRewriter(compositeSchema); var rewritten = rewriter.RewriteDocument(request, null); diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/LookupRequirementsTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/LookupRequirementsTests.cs index 5a9a7f12fb3..b8eefab2eea 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/LookupRequirementsTests.cs +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/LookupRequirementsTests.cs @@ -52,15 +52,15 @@ type Product } """); - var plan = PlanOperationAsync( - schema, + var plan = PlanOperation( """ { productById(id: 1) { internalSomeOther } } - """); + """, + schema); plan.ToYaml().MatchInlineSnapshot( """ diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__resources__/fusion1.graphql b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__resources__/fusion1.graphql index b28a3b8736c..0680b84cb9a 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__resources__/fusion1.graphql +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__resources__/fusion1.graphql @@ -222,7 +222,7 @@ directive @fusion__inputField( directive @fusion__requires( schema: fusion__Schema! field: fusion__FieldDefinition! - map: [fusion__FieldSelectionMap!]! + map: [fusion__FieldSelectionMap]! ) repeatable on FIELD_DEFINITION directive @fusion__lookup( From 10afab570bc4db36e971a3dda63586383d651cc6 Mon Sep 17 00:00:00 2001 From: Michael Staib Date: Tue, 17 Dec 2024 19:52:24 +0100 Subject: [PATCH 3/6] Reworked field merging --- .../InlineFragmentOperationRewriter.cs | 252 +++++++++++++++++- .../Planning/MergeSelectionSetRewriter.cs | 28 ++ .../Planning/Nodes/SelectionPlanNode.cs | 13 + .../Planning/OperationPlanner.cs | 36 ++- .../GraphQLSnapshotValueFormatter.cs | 40 +++ .../ModuleInitializer.cs | 2 + ...> InlineFragmentOperationRewriterTests.cs} | 70 +++-- .../MergeSelectionSetRewriterTests.cs | 55 ++++ 8 files changed, 467 insertions(+), 29 deletions(-) create mode 100644 src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/MergeSelectionSetRewriter.cs create mode 100644 src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Formatters/GraphQLSnapshotValueFormatter.cs rename src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/{InlineFragmentOperationRewriterTests.txt => InlineFragmentOperationRewriterTests.cs} (76%) create mode 100644 src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/MergeSelectionSetRewriterTests.cs diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs index d10ecb8dac7..d3ac16623aa 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs @@ -1,3 +1,5 @@ +using System.Collections; +using System.Diagnostics.CodeAnalysis; using System.Collections.Immutable; using HotChocolate.Fusion.Types; using HotChocolate.Language; @@ -31,14 +33,14 @@ public DocumentNode RewriteDocument(DocumentNode document, string? operationName return new DocumentNode(ImmutableArray.Empty.Add(newOperation)); } - private void RewriteFields(SelectionSetNode selectionSet, Context context) + internal void RewriteFields(SelectionSetNode selectionSet, Context context) { foreach (var selection in selectionSet.Selections) { switch (selection) { case FieldNode field: - RewriteField(field, context); + context.AddField(field); break; case InlineFragmentNode inlineFragment: @@ -50,6 +52,23 @@ private void RewriteFields(SelectionSetNode selectionSet, Context context) break; } } + + foreach (var (_, fields) in context.Fields) + { + foreach (var field in fields.GroupBy(t => t, t => t, FieldComparer.Instance)) + { + var mergedField = field.Key; + + if (mergedField.SelectionSet is not null) + { + mergedField = mergedField.WithSelectionSet( + new SelectionSetNode( + field.SelectMany(t => t.SelectionSet!.Selections).ToList())); + } + + RewriteField(mergedField, context); + } + } } private void RewriteField(FieldNode fieldNode, Context context) @@ -224,10 +243,239 @@ public readonly ref struct Context( public HashSet Visited { get; } = new(SyntaxComparer.BySyntax); + public OrderedDictionary> Fields { get; } = new(StringComparer.Ordinal); + public FragmentDefinitionNode GetFragmentDefinition(string name) => fragments[name]; + public void AddField(FieldNode field) + { + if (!Fields.TryGetValue(field.Name.Value, out var fields)) + { + fields = []; + Fields.Add(field.Name.Value, fields); + } + + fields.Add(field); + } + public Context Branch(ICompositeNamedType type) => new(type, fragments); } + + private sealed class FieldComparer : IEqualityComparer + { + public bool Equals(FieldNode? x, FieldNode? y) + { + if (ReferenceEquals(x, y)) + { + return true; + } + + if (x is null) + { + return false; + } + + if (y is null) + { + return false; + } + + return Equals(x.Alias, y.Alias) + && x.Name.Equals(y.Name) + && Equals(x.Directives, y.Directives) + && Equals(x.Arguments, y.Arguments); + } + + private bool Equals(IReadOnlyList a, IReadOnlyList b) + { + if (a.Count == 0 && b.Count == 0) + { + return true; + } + + return a.SequenceEqual(b, SyntaxComparer.BySyntax); + } + + public int GetHashCode(FieldNode obj) + { + var hashCode = new HashCode(); + + if (obj.Alias is not null) + { + hashCode.Add(obj.Alias.Value); + } + + hashCode.Add(obj.Name.Value); + + for (var i = 0; i < obj.Directives.Count; i++) + { + hashCode.Add(SyntaxComparer.BySyntax.GetHashCode(obj.Directives[i])); + } + + for (var i = 0; i < obj.Arguments.Count; i++) + { + hashCode.Add(SyntaxComparer.BySyntax.GetHashCode(obj.Arguments[i])); + } + + return hashCode.ToHashCode(); + } + + public static FieldComparer Instance { get; } = new(); + } +} + +#if NET8_0 +public class OrderedDictionary + : IDictionary + , IReadOnlyDictionary + where TKey : notnull +{ + private readonly List> _order; + private readonly Dictionary _map; + + public OrderedDictionary(IEqualityComparer keyComparer) + { + _order = []; + _map = new Dictionary(keyComparer); + } + + public OrderedDictionary(IEnumerable> values) + { + if (values is null) + { + throw new ArgumentNullException(nameof(values)); + } + + _order = []; + _map = new Dictionary(); + + foreach (var item in values) + { + _map.Add(item.Key, item.Value); + _order.Add(item); + } + } + + private OrderedDictionary(OrderedDictionary source) + { + if (source is null) + { + throw new ArgumentNullException(nameof(source)); + } + + _order = [..source._order,]; + _map = new Dictionary(source._map); + } + + public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) + => _map.TryGetValue(key, out value); + + public TValue this[TKey key] + { + get + { + return _map[key]; + } + set + { + if (_map.ContainsKey(key)) + { + _map[key] = value; + _order[IndexOfKey(key)] = + new KeyValuePair(key, value); + } + else + { + Add(key, value); + } + } + } + + public ICollection Keys => _map.Keys; + + IEnumerable IReadOnlyDictionary.Keys => + Keys; + + public ICollection Values => _map.Values; + + IEnumerable IReadOnlyDictionary.Values => + Values; + + public int Count => _order.Count; + + public bool IsReadOnly => false; + + public void Add(TKey key, TValue value) + { + Add(new KeyValuePair(key, value)); + } + + public void Add(KeyValuePair item) + { + _map.Add(item.Key, item.Value); + _order.Add(item); + } + + public void Clear() + { + _map.Clear(); + _order.Clear(); + } + + public bool Contains(KeyValuePair item) + => _order.Contains(item); + + public bool ContainsKey(TKey key) + => _map.ContainsKey(key); + + public void CopyTo(KeyValuePair[] array, int arrayIndex) + => _order.CopyTo(array, arrayIndex); + + public bool Remove(TKey key) + { + if (_map.ContainsKey(key)) + { + _map.Remove(key); + _order.RemoveAt(IndexOfKey(key)); + return true; + } + return false; + } + + public bool Remove(KeyValuePair item) + { + var index = _order.IndexOf(item); + + if (index != -1) + { + _order.RemoveAt(index); + _map.Remove(item.Key); + return true; + } + + return false; + } + + private int IndexOfKey(TKey key) + { + for (var i = 0; i < _order.Count; i++) + { + if (key.Equals(_order[i].Key)) + { + return i; + } + } + return -1; + } + + public IEnumerator> GetEnumerator() + => _order.GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() + => _order.GetEnumerator(); + + public OrderedDictionary Clone() => new(this); } +#endif diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/MergeSelectionSetRewriter.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/MergeSelectionSetRewriter.cs new file mode 100644 index 00000000000..fc0fd6ed404 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/MergeSelectionSetRewriter.cs @@ -0,0 +1,28 @@ +using HotChocolate.Fusion.Types; +using HotChocolate.Language; + +namespace HotChocolate.Fusion.Planning; + +public class MergeSelectionSetRewriter(CompositeSchema schema) +{ + private readonly InlineFragmentOperationRewriter _rewriter = new(schema); + + public SelectionSetNode RewriteSelectionSets( + IReadOnlyList selectionSets, + ICompositeNamedType type) + { + var context = new InlineFragmentOperationRewriter.Context( + type, + new Dictionary()); + + var merged = new SelectionSetNode( + null, + selectionSets.SelectMany(t => t.Selections).ToList()); + + _rewriter.RewriteFields(merged, context); + + return new SelectionSetNode( + null, + context.Selections.ToImmutable()); + } +} diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/Nodes/SelectionPlanNode.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/Nodes/SelectionPlanNode.cs index 1837460805d..f26812dbfcc 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/Nodes/SelectionPlanNode.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/Nodes/SelectionPlanNode.cs @@ -10,6 +10,7 @@ public abstract class SelectionPlanNode : PlanNode { private List? _directives; private List? _selections; + private List? _requirements; private bool? _isConditional; private string? _skipVariable; private string? _includeVariable; @@ -69,6 +70,12 @@ public IReadOnlyList Directives public IReadOnlyList Selections => _selections ?? (IReadOnlyList)Array.Empty(); + /// + /// Gets the requirements that are needed to execute this selection. + /// + public IReadOnlyList RequirementNodes + => _requirements ?? (IReadOnlyList)Array.Empty(); + /// /// Defines if the selection is conditional. /// @@ -150,6 +157,12 @@ public void AddDirective(CompositeDirective directive) public bool RemoveDirective(CompositeDirective directive) => _directives?.Remove(directive) == true; + public void AddRequirementNode(SelectionSetNode selectionSet) + { + ArgumentNullException.ThrowIfNull(selectionSet); + (_requirements ??= []).Add(selectionSet); + } + private void InitializeConditions() { if (_isConditional.HasValue) diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs index 3531e062da9..e5973b0d199 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/OperationPlanner.cs @@ -173,7 +173,7 @@ private bool TryPlanInlineFragmentSelection( if (unresolvedSelections is { Count: > 0 }) { var unresolvedInlineFragment = - new UnresolvedInlineFragment(inlineFragmentNode.Directives, typeCondition, unresolvedSelections); + new UnresolvedInlineFragment(typeCondition, inlineFragmentNode.Directives, unresolvedSelections); trackUnresolvedSelection(unresolvedInlineFragment); } @@ -213,12 +213,7 @@ private bool TryPlanFieldSelection( if (source.Requirements is not null) { - var unresolvedRequirements = ParseSelectionSet(type, source.Requirements.SelectionSet); - foreach (var requiredField in unresolvedRequirements) - { - // TODO : reintegrate - // trackRequiredField(new UnresolvedField(requiredField.FieldNode, requiredField.Field)); - } + context.Parent.AddRequirementNode(source.Requirements.SelectionSet); } // if the field has no selection set it must be a leaf type. @@ -831,11 +826,11 @@ private static bool IsSelectionAlwaysSkipped(ISelectionNode selectionNode) return false; } - private static IReadOnlyList ParseSelectionSet( + private IReadOnlyList ParseRequirements( CompositeComplexType type, SelectionSetNode selectionSetNode) { - var unresolvedFields = new List(); + var unresolvedFields = new List(); foreach (var selectionNode in selectionSetNode.Selections) { @@ -849,6 +844,21 @@ private static IReadOnlyList ParseSelectionSet( unresolvedFields.Add(new UnresolvedField(fieldNode, field)); } + else if (selectionNode is InlineFragmentNode fragmentNode) + { + var typeCondition = type; + if (fragmentNode.TypeCondition?.Name is { Value : { } conditionTypeName } && + schema.TryGetType(conditionTypeName, out var typeConditionType)) + { + typeCondition = typeConditionType; + } + + unresolvedFields.Add( + new UnresolvedInlineFragment( + typeCondition, + fragmentNode.Directives, + ParseRequirements(typeCondition, fragmentNode.SelectionSet))); + } } return unresolvedFields; @@ -864,12 +874,14 @@ public interface IUnresolvedSelection; public record UnresolvedField( FieldNode FieldNode, - CompositeOutputField Field) : IUnresolvedSelection; + CompositeOutputField Field) + : IUnresolvedSelection; public record UnresolvedInlineFragment( - IReadOnlyList Directives, CompositeComplexType TypeCondition, - List UnresolvedSelections) : IUnresolvedSelection; + IReadOnlyList Directives, + IReadOnlyList UnresolvedSelections) + : IUnresolvedSelection; private record struct LookupOperation( OperationPlanNode Operation, diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Formatters/GraphQLSnapshotValueFormatter.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Formatters/GraphQLSnapshotValueFormatter.cs new file mode 100644 index 00000000000..18f7a3b5ad5 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Formatters/GraphQLSnapshotValueFormatter.cs @@ -0,0 +1,40 @@ +using System.Buffers; +using CookieCrumble.Formatters; +using HotChocolate.Language; +using HotChocolate.Language.Utilities; + +namespace CookieCrumble.HotChocolate.Formatters; + +internal sealed class GraphQLSnapshotValueFormatter : SnapshotValueFormatter +{ + protected override void Format(IBufferWriter snapshot, ISyntaxNode value) + { + var serialized = value.Print().AsSpan(); + var buffer = ArrayPool.Shared.Rent(serialized.Length); + var span = buffer.AsSpan()[..serialized.Length]; + var written = 0; + + for (var i = 0; i < serialized.Length; i++) + { + if (serialized[i] is not '\r') + { + span[written++] = serialized[i]; + } + } + + span = span[..written]; + snapshot.Append(span); + + ArrayPool.Shared.Return(buffer); + } + + protected override void FormatMarkdown(IBufferWriter snapshot, ISyntaxNode value) + { + snapshot.Append("```graphql"); + snapshot.AppendLine(); + Format(snapshot, value); + snapshot.AppendLine(); + snapshot.Append("```"); + snapshot.AppendLine(); + } +} diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/ModuleInitializer.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/ModuleInitializer.cs index d50f67d0c56..bf90891de8c 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/ModuleInitializer.cs +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/ModuleInitializer.cs @@ -1,4 +1,5 @@ using System.Runtime.CompilerServices; +using CookieCrumble.HotChocolate.Formatters; namespace HotChocolate.Fusion; @@ -8,5 +9,6 @@ internal static class ModuleInitializer public static void Initialize() { CookieCrumbleTUnit.Initialize(); + Snapshot.RegisterFormatter(new GraphQLSnapshotValueFormatter()); } } diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.txt b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs similarity index 76% rename from src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.txt rename to src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs index 07e96d0db34..d42d6860601 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.txt +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs @@ -1,13 +1,12 @@ -using HotChocolate.Fusion.Planning; using HotChocolate.Fusion.Types.Completion; using HotChocolate.Language; -namespace HotChocolate.Fusion.Execution.Planning; +namespace HotChocolate.Fusion.Planning; -public static class InlineFragmentOperationRewriterTests +public class InlineFragmentOperationRewriterTests { - [Fact] - public static void Inline_Into_ProductById_SelectionSet() + [Test] + public void Inline_Into_ProductById_SelectionSet() { // arrange var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); @@ -43,8 +42,8 @@ fragment Product on Product { """); } - [Fact] - public static void Inline_Into_ProductById_SelectionSet_2_Levels() + [Test] + public void Inline_Into_ProductById_SelectionSet_2_Levels() { // arrange var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); @@ -84,8 +83,8 @@ fragment Product2 on Product { """); } - [Fact] - public static void Inline_Inline_Fragment_Into_ProductById_SelectionSet_1() + [Test] + public void Inline_Inline_Fragment_Into_ProductById_SelectionSet_1() { // arrange var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); @@ -119,8 +118,8 @@ public static void Inline_Inline_Fragment_Into_ProductById_SelectionSet_1() """); } - [Fact] - public static void Inline_Into_ProductById_SelectionSet_3_Levels() + [Test] + public void Inline_Into_ProductById_SelectionSet_3_Levels() { // arrange var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); @@ -162,8 +161,8 @@ fragment Product2 on Product { """); } - [Fact] - public static void Do_Not_Inline_Inline_Fragment_Into_ProductById_SelectionSet() + [Test] + public void Do_Not_Inline_Inline_Fragment_Into_ProductById_SelectionSet() { // arrange var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); @@ -199,8 +198,8 @@ ... @include(if: true) { """); } - [Fact] - public static void Deduplicate_Fields() + [Test] + public void Deduplicate_Fields() { // arrange var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); @@ -237,4 +236,45 @@ fragment Product on Product { } """); } + + [Test] + public void Leafs_With_Different_Directives_Do_Not_Merge() + { + // arrange + var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); + var compositeSchema = CompositeSchemaBuilder.Create(compositeSchemaDoc); + + var doc = Utf8GraphQLParser.Parse( + """ + query($skip: Boolean!) { + productById(id: 1) { + ... Product + } + } + + fragment Product on Product { + id + name @include(if: $skip) + name @include(if: $skip) + name @skip(if: $skip) + name @skip(if: $skip) + } + """); + + // act + var rewriter = new InlineFragmentOperationRewriter(compositeSchema); + var rewritten = rewriter.RewriteDocument(doc, null); + + // assert + rewritten.MatchInlineSnapshot( + """ + query($skip: Boolean!) { + productById(id: 1) { + id + name @include(if: $skip) + name @skip(if: $skip) + } + } + """); + } } diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/MergeSelectionSetRewriterTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/MergeSelectionSetRewriterTests.cs new file mode 100644 index 00000000000..93c8844b868 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/MergeSelectionSetRewriterTests.cs @@ -0,0 +1,55 @@ +using HotChocolate.Fusion.Types; +using HotChocolate.Fusion.Types.Completion; +using HotChocolate.Language; + +namespace HotChocolate.Fusion.Planning; + +public class MergeSelectionSetRewriterTests +{ + [Test] + public void Merge_Two_SelectionSets() + { + // arrange + var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); + var compositeSchema = CompositeSchemaBuilder.Create(compositeSchemaDoc); + var productType = compositeSchema.GetType("Product"); + + var selectionSet1 = Utf8GraphQLParser.Syntax.ParseSelectionSet( + """ + { + id + name + reviews { + id + } + } + """); + + var selectionSet2 = Utf8GraphQLParser.Syntax.ParseSelectionSet( + """ + { + reviews { + body + } + name + } + """); + + // act + var rewriter = new MergeSelectionSetRewriter(compositeSchema); + var rewritten = rewriter.RewriteSelectionSets([selectionSet1, selectionSet2], productType); + + // assert + rewritten.MatchInlineSnapshot( + """ + { + id + name + reviews { + id + body + } + } + """); + } +} From 45bff27b40d7f456c983db2b84d25b1ae0221eb0 Mon Sep 17 00:00:00 2001 From: Michael Staib Date: Tue, 17 Dec 2024 22:25:08 +0100 Subject: [PATCH 4/6] updated snapshots --- ...t_Next_To_Same_Selection_With_Different_Sub_Selection.yaml | 4 ---- ...t_Next_To_Same_Selection_With_Different_Sub_Selection.yaml | 4 ---- 2 files changed, 8 deletions(-) diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/FragmentTests.Fragment_On_Root_Next_To_Same_Selection_With_Different_Sub_Selection.yaml b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/FragmentTests.Fragment_On_Root_Next_To_Same_Selection_With_Different_Sub_Selection.yaml index cd38e02de77..6581ea0a8c8 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/FragmentTests.Fragment_On_Root_Next_To_Same_Selection_With_Different_Sub_Selection.yaml +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/FragmentTests.Fragment_On_Root_Next_To_Same_Selection_With_Different_Sub_Selection.yaml @@ -3,8 +3,6 @@ request: query($slug: String!) { productBySlug(slug: $slug) { description - } - productBySlug(slug: $slug) { name } } @@ -15,8 +13,6 @@ nodes: query($slug: String!) { productBySlug(slug: $slug) { description - } - productBySlug(slug: $slug) { name } } diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/InlineFragmentTests.InlineFragment_On_Root_Next_To_Same_Selection_With_Different_Sub_Selection.yaml b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/InlineFragmentTests.InlineFragment_On_Root_Next_To_Same_Selection_With_Different_Sub_Selection.yaml index cd38e02de77..6581ea0a8c8 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/InlineFragmentTests.InlineFragment_On_Root_Next_To_Same_Selection_With_Different_Sub_Selection.yaml +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/InlineFragmentTests.InlineFragment_On_Root_Next_To_Same_Selection_With_Different_Sub_Selection.yaml @@ -3,8 +3,6 @@ request: query($slug: String!) { productBySlug(slug: $slug) { description - } - productBySlug(slug: $slug) { name } } @@ -15,8 +13,6 @@ nodes: query($slug: String!) { productBySlug(slug: $slug) { description - } - productBySlug(slug: $slug) { name } } From c8b957954fc1c814e76acc32ddbfa76dcf25c456 Mon Sep 17 00:00:00 2001 From: Michael Staib Date: Tue, 17 Dec 2024 22:53:11 +0100 Subject: [PATCH 5/6] Improved inlining --- .../InlineFragmentOperationRewriter.cs | 118 +++++++++++++----- .../Planning/MergeSelectionSetRewriter.cs | 3 +- .../InlineFragmentOperationRewriterTests.cs | 57 +++++++++ ...try_Selection_From_Different_Subgraph.yaml | 4 - 4 files changed, 146 insertions(+), 36 deletions(-) diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs index d3ac16623aa..a8d266fd83e 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs @@ -16,7 +16,8 @@ public DocumentNode RewriteDocument(DocumentNode document, string? operationName var fragmentLookup = CreateFragmentLookup(document); var context = new Context(operationType, fragmentLookup); - RewriteFields(operation.SelectionSet, context); + CollectSelections(operation.SelectionSet, context); + RewriteSelections(context); var newSelectionSet = new SelectionSetNode( null, @@ -33,7 +34,7 @@ public DocumentNode RewriteDocument(DocumentNode document, string? operationName return new DocumentNode(ImmutableArray.Empty.Add(newOperation)); } - internal void RewriteFields(SelectionSetNode selectionSet, Context context) + internal void CollectSelections(SelectionSetNode selectionSet, Context context) { foreach (var selection in selectionSet.Selections) { @@ -43,6 +44,30 @@ internal void RewriteFields(SelectionSetNode selectionSet, Context context) context.AddField(field); break; + case InlineFragmentNode inlineFragment: + CollectInlineFragment(inlineFragment, context); + break; + + case FragmentSpreadNode fragmentSpread: + CollectFragmentSpread(fragmentSpread, context); + break; + } + } + } + + internal void RewriteSelections(Context context) + { + var collectedSelections = context.Selections.ToImmutableArray(); + context.Selections.Clear(); + + foreach (var selection in collectedSelections) + { + switch (selection) + { + case FieldNode field: + MergeField(field.Name.Value, context); + break; + case InlineFragmentNode inlineFragment: RewriteInlineFragment(inlineFragment, context); break; @@ -53,9 +78,9 @@ internal void RewriteFields(SelectionSetNode selectionSet, Context context) } } - foreach (var (_, fields) in context.Fields) + void MergeField(string fieldName, Context ctx) { - foreach (var field in fields.GroupBy(t => t, t => t, FieldComparer.Instance)) + foreach (var field in ctx.Fields[fieldName].GroupBy(t => t, t => t, FieldComparer.Instance)) { var mergedField = field.Key; @@ -66,7 +91,7 @@ internal void RewriteFields(SelectionSetNode selectionSet, Context context) field.SelectMany(t => t.SelectionSet!.Selections).ToList())); } - RewriteField(mergedField, context); + RewriteField(mergedField, ctx); } } } @@ -87,7 +112,8 @@ private void RewriteField(FieldNode fieldNode, Context context) var field = ((CompositeComplexType)context.Type).Fields[fieldNode.Name.Value]; var fieldContext = context.Branch(field.Type.NamedType()); - RewriteFields(fieldNode.SelectionSet, fieldContext); + CollectSelections(fieldNode.SelectionSet, fieldContext); + RewriteSelections(fieldContext); var newSelectionSetNode = new SelectionSetNode( null, @@ -108,23 +134,29 @@ private void RewriteField(FieldNode fieldNode, Context context) } } - private void RewriteInlineFragment(InlineFragmentNode inlineFragment, Context context) + private void CollectInlineFragment(InlineFragmentNode inlineFragment, Context context) { - if ((inlineFragment.TypeCondition is null - || inlineFragment.TypeCondition.Name.Value.Equals(context.Type.Name, StringComparison.Ordinal)) + if ((inlineFragment.TypeCondition is null + || inlineFragment.TypeCondition.Name.Value.Equals(context.Type.Name, StringComparison.Ordinal)) && inlineFragment.Directives.Count == 0) { - RewriteFields(inlineFragment.SelectionSet, context); + CollectSelections(inlineFragment.SelectionSet, context); return; } + context.AddInlineFragment(inlineFragment); + } + + private void RewriteInlineFragment(InlineFragmentNode inlineFragment, Context context) + { var typeCondition = inlineFragment.TypeCondition is null ? context.Type : schema.GetType(inlineFragment.TypeCondition.Name.Value); var inlineFragmentContext = context.Branch(typeCondition); - RewriteFields(inlineFragment.SelectionSet, inlineFragmentContext); + CollectSelections(inlineFragment.SelectionSet, inlineFragmentContext); + RewriteSelections(inlineFragmentContext); var newSelectionSetNode = new SelectionSetNode( null, @@ -139,7 +171,7 @@ private void RewriteInlineFragment(InlineFragmentNode inlineFragment, Context co context.Selections.Add(newInlineFragment); } - private void InlineFragmentDefinition( + private void CollectFragmentSpread( FragmentSpreadNode fragmentSpread, Context context) { @@ -149,28 +181,37 @@ private void InlineFragmentDefinition( if (fragmentSpread.Directives.Count == 0 && typeCondition.IsAssignableFrom(context.Type)) { - RewriteFields(fragmentDefinition.SelectionSet, context); + CollectSelections(fragmentDefinition.SelectionSet, context); + return; } - else - { - var fragmentContext = context.Branch(typeCondition); - RewriteFields(fragmentDefinition.SelectionSet, fragmentContext); + context.AddFragmentSpread(fragmentSpread); + } - var selectionSet = new SelectionSetNode( - null, - fragmentContext.Selections.ToImmutable()); + private void InlineFragmentDefinition( + FragmentSpreadNode fragmentSpread, + Context context) + { + var fragmentDefinition = context.GetFragmentDefinition(fragmentSpread.Name.Value); + var typeCondition = schema.GetType(fragmentDefinition.TypeCondition.Name.Value); + var fragmentContext = context.Branch(typeCondition); - var inlineFragment = new InlineFragmentNode( - null, - new NamedTypeNode(typeCondition.Name), - RewriteDirectives(fragmentSpread.Directives), - selectionSet); + CollectSelections(fragmentDefinition.SelectionSet, fragmentContext); + RewriteSelections(fragmentContext); - if (context.Visited.Add(inlineFragment)) - { - context.Selections.Add(inlineFragment); - } + var selectionSet = new SelectionSetNode( + null, + fragmentContext.Selections.ToImmutable()); + + var inlineFragment = new InlineFragmentNode( + null, + new NamedTypeNode(typeCondition.Name), + RewriteDirectives(fragmentSpread.Directives), + selectionSet); + + if (context.Visited.Add(inlineFragment)) + { + context.Selections.Add(inlineFragment); } } @@ -194,6 +235,7 @@ private IReadOnlyList RewriteDirectives(IReadOnlyList RewriteArguments(IReadOnlyList { buffer[i] = arguments[i].WithLocation(null); } + return ImmutableArray.Create(buffer); } @@ -243,7 +286,7 @@ public readonly ref struct Context( public HashSet Visited { get; } = new(SyntaxComparer.BySyntax); - public OrderedDictionary> Fields { get; } = new(StringComparer.Ordinal); + public Dictionary> Fields { get; } = new(StringComparer.Ordinal); public FragmentDefinitionNode GetFragmentDefinition(string name) => fragments[name]; @@ -254,11 +297,22 @@ public void AddField(FieldNode field) { fields = []; Fields.Add(field.Name.Value, fields); + Selections.Add(field); } fields.Add(field); } + public void AddInlineFragment(InlineFragmentNode inlineFragment) + { + Selections.Add(inlineFragment); + } + + public void AddFragmentSpread(FragmentSpreadNode fragmentSpread) + { + Selections.Add(fragmentSpread); + } + public Context Branch(ICompositeNamedType type) => new(type, fragments); } @@ -329,7 +383,7 @@ public int GetHashCode(FieldNode obj) #if NET8_0 public class OrderedDictionary : IDictionary - , IReadOnlyDictionary + , IReadOnlyDictionary where TKey : notnull { private readonly List> _order; @@ -441,6 +495,7 @@ public bool Remove(TKey key) _order.RemoveAt(IndexOfKey(key)); return true; } + return false; } @@ -467,6 +522,7 @@ private int IndexOfKey(TKey key) return i; } } + return -1; } diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/MergeSelectionSetRewriter.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/MergeSelectionSetRewriter.cs index fc0fd6ed404..1ac68a652f0 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/MergeSelectionSetRewriter.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/MergeSelectionSetRewriter.cs @@ -19,7 +19,8 @@ public SelectionSetNode RewriteSelectionSets( null, selectionSets.SelectMany(t => t.Selections).ToList()); - _rewriter.RewriteFields(merged, context); + _rewriter.CollectSelections(merged, context); + _rewriter.RewriteSelections(context); return new SelectionSetNode( null, diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs index d42d6860601..628a9386847 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs @@ -172,6 +172,7 @@ public void Do_Not_Inline_Inline_Fragment_Into_ProductById_SelectionSet() """ { productById(id: 1) { + id ... @include(if: true) { id name @@ -189,6 +190,7 @@ ... @include(if: true) { """ { productById(id: 1) { + id ... @include(if: true) { id name @@ -277,4 +279,59 @@ name @skip(if: $skip) } """); } + + [Test] + public void Composites_Without_Directives_Are_Merged() + { + // arrange + var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); + var compositeSchema = CompositeSchemaBuilder.Create(compositeSchemaDoc); + + var doc = Utf8GraphQLParser.Parse( + """ + query($slug: String!) { + productBySlug(slug: $slug) { + ...ProductFragment1 + ...ProductFragment2 + } + } + + fragment ProductFragment1 on Product { + reviews { + nodes { + body + } + } + } + + fragment ProductFragment2 on Product { + reviews { + pageInfo { + hasNextPage + } + } + } + """); + + // act + var rewriter = new InlineFragmentOperationRewriter(compositeSchema); + var rewritten = rewriter.RewriteDocument(doc, null); + + // assert + rewritten.MatchInlineSnapshot( + """ + query($slug: String!) { + productBySlug(slug: $slug) { + reviews { + nodes { + body + } + pageInfo { + hasNextPage + } + } + } + } + """); + } } diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/FragmentTests.Two_Fragments_On_Sub_Selection_With_Different_But_Same_Entry_Selection_From_Different_Subgraph.yaml b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/FragmentTests.Two_Fragments_On_Sub_Selection_With_Different_But_Same_Entry_Selection_From_Different_Subgraph.yaml index f6c6193faa4..7bdb9df9e54 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/FragmentTests.Two_Fragments_On_Sub_Selection_With_Different_But_Same_Entry_Selection_From_Different_Subgraph.yaml +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/FragmentTests.Two_Fragments_On_Sub_Selection_With_Different_But_Same_Entry_Selection_From_Different_Subgraph.yaml @@ -6,8 +6,6 @@ request: nodes { body } - } - reviews { pageInfo { hasNextPage } @@ -32,8 +30,6 @@ nodes: nodes { body } - } - reviews { pageInfo { hasNextPage } From 20086d18e0e043599f0dda11f1bcab4e9093985b Mon Sep 17 00:00:00 2001 From: Michael Staib Date: Wed, 18 Dec 2024 14:43:28 +0100 Subject: [PATCH 6/6] Added test with aliases --- .../InlineFragmentOperationRewriter.cs | 165 +----------------- .../InlineFragmentTests.cs | 29 +++ .../InlineFragmentOperationRewriterTests.cs | 39 +++++ ...FragmentTests.Merge_Fields_With_Alias.yaml | 18 ++ 4 files changed, 94 insertions(+), 157 deletions(-) create mode 100644 src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/InlineFragmentTests.Merge_Fields_With_Alias.yaml diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs index a8d266fd83e..fceffd798fd 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Planning/InlineFragmentOperationRewriter.cs @@ -65,7 +65,7 @@ internal void RewriteSelections(Context context) switch (selection) { case FieldNode field: - MergeField(field.Name.Value, context); + MergeField(field.ResponseName(), context); break; case InlineFragmentNode inlineFragment: @@ -109,7 +109,7 @@ private void RewriteField(FieldNode fieldNode, Context context) } else { - var field = ((CompositeComplexType)context.Type).Fields[fieldNode.Name.Value]; + var field = ((CompositeComplexType)context.Type).Fields[fieldNode.ResponseName()]; var fieldContext = context.Branch(field.Type.NamedType()); CollectSelections(fieldNode.SelectionSet, fieldContext); @@ -293,10 +293,11 @@ public FragmentDefinitionNode GetFragmentDefinition(string name) public void AddField(FieldNode field) { - if (!Fields.TryGetValue(field.Name.Value, out var fields)) + var responseName = field.ResponseName(); + if (!Fields.TryGetValue(responseName, out var fields)) { fields = []; - Fields.Add(field.Name.Value, fields); + Fields.Add(responseName, fields); Selections.Add(field); } @@ -380,158 +381,8 @@ public int GetHashCode(FieldNode obj) } } -#if NET8_0 -public class OrderedDictionary - : IDictionary - , IReadOnlyDictionary - where TKey : notnull +file static class FileExtensions { - private readonly List> _order; - private readonly Dictionary _map; - - public OrderedDictionary(IEqualityComparer keyComparer) - { - _order = []; - _map = new Dictionary(keyComparer); - } - - public OrderedDictionary(IEnumerable> values) - { - if (values is null) - { - throw new ArgumentNullException(nameof(values)); - } - - _order = []; - _map = new Dictionary(); - - foreach (var item in values) - { - _map.Add(item.Key, item.Value); - _order.Add(item); - } - } - - private OrderedDictionary(OrderedDictionary source) - { - if (source is null) - { - throw new ArgumentNullException(nameof(source)); - } - - _order = [..source._order,]; - _map = new Dictionary(source._map); - } - - public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) - => _map.TryGetValue(key, out value); - - public TValue this[TKey key] - { - get - { - return _map[key]; - } - set - { - if (_map.ContainsKey(key)) - { - _map[key] = value; - _order[IndexOfKey(key)] = - new KeyValuePair(key, value); - } - else - { - Add(key, value); - } - } - } - - public ICollection Keys => _map.Keys; - - IEnumerable IReadOnlyDictionary.Keys => - Keys; - - public ICollection Values => _map.Values; - - IEnumerable IReadOnlyDictionary.Values => - Values; - - public int Count => _order.Count; - - public bool IsReadOnly => false; - - public void Add(TKey key, TValue value) - { - Add(new KeyValuePair(key, value)); - } - - public void Add(KeyValuePair item) - { - _map.Add(item.Key, item.Value); - _order.Add(item); - } - - public void Clear() - { - _map.Clear(); - _order.Clear(); - } - - public bool Contains(KeyValuePair item) - => _order.Contains(item); - - public bool ContainsKey(TKey key) - => _map.ContainsKey(key); - - public void CopyTo(KeyValuePair[] array, int arrayIndex) - => _order.CopyTo(array, arrayIndex); - - public bool Remove(TKey key) - { - if (_map.ContainsKey(key)) - { - _map.Remove(key); - _order.RemoveAt(IndexOfKey(key)); - return true; - } - - return false; - } - - public bool Remove(KeyValuePair item) - { - var index = _order.IndexOf(item); - - if (index != -1) - { - _order.RemoveAt(index); - _map.Remove(item.Key); - return true; - } - - return false; - } - - private int IndexOfKey(TKey key) - { - for (var i = 0; i < _order.Count; i++) - { - if (key.Equals(_order[i].Key)) - { - return i; - } - } - - return -1; - } - - public IEnumerator> GetEnumerator() - => _order.GetEnumerator(); - - IEnumerator IEnumerable.GetEnumerator() - => _order.GetEnumerator(); - - public OrderedDictionary Clone() => new(this); + public static string ResponseName(this FieldNode field) + => field.Alias?.Value ?? field.Name.Value; } -#endif diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/InlineFragmentTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/InlineFragmentTests.cs index 493d96b2a9b..54c0dc0cd24 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/InlineFragmentTests.cs +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/InlineFragmentTests.cs @@ -408,4 +408,33 @@ public void Two_Fragments_On_Sub_Selection_With_Different_Selection_From_Differe // assert plan.MatchSnapshot(); } + + [Test] + public void Merge_Fields_With_Alias() + { + // arrange + var compositeSchema = CreateCompositeSchema(); + + var request = Utf8GraphQLParser.Parse( + """ + query($slug: String!) { + productBySlug(slug: $slug) { + ... { + a: name + } + a: name + name + ... { + name + } + } + } + """); + + // act + var plan = PlanOperation(request, compositeSchema); + + // assert + plan.MatchSnapshot(); + } } diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs index 628a9386847..2bd91816753 100644 --- a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/Planning/InlineFragmentOperationRewriterTests.cs @@ -334,4 +334,43 @@ fragment ProductFragment2 on Product { } """); } + + [Test] + public void Merge_Fields_With_Aliases() + { + // arrange + var compositeSchemaDoc = Utf8GraphQLParser.Parse(FileResource.Open("fusion1.graphql")); + var compositeSchema = CompositeSchemaBuilder.Create(compositeSchemaDoc); + + var doc = Utf8GraphQLParser.Parse( + """ + query($slug: String!) { + productBySlug(slug: $slug) { + ... { + a: name + } + a: name + name + ... { + name + } + } + } + """); + + // act + var rewriter = new InlineFragmentOperationRewriter(compositeSchema); + var rewritten = rewriter.RewriteDocument(doc, null); + + // assert + rewritten.MatchInlineSnapshot( + """ + query($slug: String!) { + productBySlug(slug: $slug) { + a: name + name + } + } + """); + } } diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/InlineFragmentTests.Merge_Fields_With_Alias.yaml b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/InlineFragmentTests.Merge_Fields_With_Alias.yaml new file mode 100644 index 00000000000..1292362e70d --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Execution.Tests/__snapshots__/InlineFragmentTests.Merge_Fields_With_Alias.yaml @@ -0,0 +1,18 @@ +request: + - document: >- + query($slug: String!) { + productBySlug(slug: $slug) { + a: name + name + } + } +nodes: + - id: 1 + schema: "PRODUCTS" + operation: >- + query($slug: String!) { + productBySlug(slug: $slug) { + a: name + name + } + }