From 8635bfc88b59ef6673cec6abed1d5e00b5834384 Mon Sep 17 00:00:00 2001 From: ndejaco2 Date: Fri, 1 Sep 2023 09:56:28 -0700 Subject: [PATCH 1/2] Adding failing test for fusion query planner --- .../test/Core.Tests/RequestPlannerTests.cs | 35 ++++++++ .../Fusion/test/Shared/Authors/Author.cs | 16 ++++ .../Fusion/test/Shared/Authors/AuthorQuery.cs | 20 +++++ .../test/Shared/Authors/AuthorRepository.cs | 22 +++++ .../Fusion/test/Shared/Authors/Book.cs | 15 ++++ .../Fusion/test/Shared/Books/Author.cs | 14 ++++ .../Fusion/test/Shared/Books/Book.cs | 17 ++++ .../Fusion/test/Shared/Books/BookQuery.cs | 18 ++++ .../test/Shared/Books/BookRepository.cs | 25 ++++++ .../Fusion/test/Shared/DemoProject.cs | 83 +++++++++++++++++++ 10 files changed, 265 insertions(+) create mode 100644 src/HotChocolate/Fusion/test/Shared/Authors/Author.cs create mode 100644 src/HotChocolate/Fusion/test/Shared/Authors/AuthorQuery.cs create mode 100644 src/HotChocolate/Fusion/test/Shared/Authors/AuthorRepository.cs create mode 100644 src/HotChocolate/Fusion/test/Shared/Authors/Book.cs create mode 100644 src/HotChocolate/Fusion/test/Shared/Books/Author.cs create mode 100644 src/HotChocolate/Fusion/test/Shared/Books/Book.cs create mode 100644 src/HotChocolate/Fusion/test/Shared/Books/BookQuery.cs create mode 100644 src/HotChocolate/Fusion/test/Shared/Books/BookRepository.cs diff --git a/src/HotChocolate/Fusion/test/Core.Tests/RequestPlannerTests.cs b/src/HotChocolate/Fusion/test/Core.Tests/RequestPlannerTests.cs index 0c1b78d48f8..046a920ef9e 100644 --- a/src/HotChocolate/Fusion/test/Core.Tests/RequestPlannerTests.cs +++ b/src/HotChocolate/Fusion/test/Core.Tests/RequestPlannerTests.cs @@ -1057,6 +1057,41 @@ query TopProducts { await snapshot.MatchAsync(); } + [Fact(Skip = "This test currently fails during query planning")] + public async Task Query_Plan_27_Selection_Set_Empty() + { + // arrange + using var demoProject = await DemoProject.CreateAsync(); + + var fusionGraph = await FusionGraphComposer.ComposeAsync( + new[] + { + demoProject.Authors.ToConfiguration(), + demoProject.Books.ToConfiguration() + }); + + // act + var result = await CreateQueryPlanAsync( + fusionGraph, + """ + query Query { + authorById(id: "1") { + id, + name, + bio, + books { + id + author { + books { + id + } + } + } + } + } + """); + } + private static async Task<(DocumentNode UserRequest, Execution.Nodes.QueryPlan QueryPlan)> CreateQueryPlanAsync( Skimmed.Schema fusionGraph, [StringSyntax("graphql")] string query) diff --git a/src/HotChocolate/Fusion/test/Shared/Authors/Author.cs b/src/HotChocolate/Fusion/test/Shared/Authors/Author.cs new file mode 100644 index 00000000000..7784618dbac --- /dev/null +++ b/src/HotChocolate/Fusion/test/Shared/Authors/Author.cs @@ -0,0 +1,16 @@ +namespace HotChocolate.Fusion.Shared.Authors; + +public class Author +{ + public string Id { get; set;} + + public string Name { get; set; } + + public string Bio { get; set;} + + public Author(string id, string name, string bio) { + this.Id = id; + this.Name = name; + this.Bio = bio; + } +} \ No newline at end of file diff --git a/src/HotChocolate/Fusion/test/Shared/Authors/AuthorQuery.cs b/src/HotChocolate/Fusion/test/Shared/Authors/AuthorQuery.cs new file mode 100644 index 00000000000..21aef4c52e9 --- /dev/null +++ b/src/HotChocolate/Fusion/test/Shared/Authors/AuthorQuery.cs @@ -0,0 +1,20 @@ +namespace HotChocolate.Fusion.Shared.Authors; + +[GraphQLName("Query")] +public sealed class AuthorQuery +{ + public Author? AuthorById( + string id, + [Service] AuthorRepository repository) + => repository.GetAuthorById(id); + + public IEnumerable Authors(int limit, [Service] AuthorRepository repository) + => repository.GetAuthors(limit); + + public Book BookByAuthorId( + string authorId, + [Service] AuthorRepository repository) { + Author author = repository.GetAuthorById(authorId); + return new Book(authorId, author); + } +} \ No newline at end of file diff --git a/src/HotChocolate/Fusion/test/Shared/Authors/AuthorRepository.cs b/src/HotChocolate/Fusion/test/Shared/Authors/AuthorRepository.cs new file mode 100644 index 00000000000..dd70fd219d1 --- /dev/null +++ b/src/HotChocolate/Fusion/test/Shared/Authors/AuthorRepository.cs @@ -0,0 +1,22 @@ +namespace HotChocolate.Fusion.Shared.Authors; + +public sealed class AuthorRepository +{ + private readonly Dictionary _authors; + + public AuthorRepository() + { + _authors = new[] + { + new Author("1", "First author", "The first author") + }.ToDictionary(t => t.Id); + } + + public IEnumerable GetAuthors(int limit) + => _authors.Values.OrderBy(t => t.Id).Take(limit); + + public Author? GetAuthorById(string id) + => _authors.TryGetValue(id, out var author) + ? author + : null; +} diff --git a/src/HotChocolate/Fusion/test/Shared/Authors/Book.cs b/src/HotChocolate/Fusion/test/Shared/Authors/Book.cs new file mode 100644 index 00000000000..397c8474530 --- /dev/null +++ b/src/HotChocolate/Fusion/test/Shared/Authors/Book.cs @@ -0,0 +1,15 @@ + +namespace HotChocolate.Fusion.Shared.Authors; + +public class Book +{ + + public string AuthorId { get; set;} + + public Author Author {get; set; } + + public Book(string authorId, Author author) { + this.AuthorId = authorId; + this.Author = author; + } +} diff --git a/src/HotChocolate/Fusion/test/Shared/Books/Author.cs b/src/HotChocolate/Fusion/test/Shared/Books/Author.cs new file mode 100644 index 00000000000..6b43527ca38 --- /dev/null +++ b/src/HotChocolate/Fusion/test/Shared/Books/Author.cs @@ -0,0 +1,14 @@ +namespace HotChocolate.Fusion.Shared.Books; + + +public class Author +{ + public string Id { get; set;} + + public IEnumerable Books { get; set; } + + public Author(string id, IEnumerable books) { + this.Id = id; + this.Books = books; + } +} \ No newline at end of file diff --git a/src/HotChocolate/Fusion/test/Shared/Books/Book.cs b/src/HotChocolate/Fusion/test/Shared/Books/Book.cs new file mode 100644 index 00000000000..b65fe18f072 --- /dev/null +++ b/src/HotChocolate/Fusion/test/Shared/Books/Book.cs @@ -0,0 +1,17 @@ +namespace HotChocolate.Fusion.Shared.Books; + +public class Book +{ + public string Id { get; set;} + + public string AuthorId { get; set;} + + public string Title {get; set; } + + + public Book(string id, string authorId, string title) { + this.Id = id; + this.AuthorId = authorId; + this.Title = title; + } +} diff --git a/src/HotChocolate/Fusion/test/Shared/Books/BookQuery.cs b/src/HotChocolate/Fusion/test/Shared/Books/BookQuery.cs new file mode 100644 index 00000000000..502535bea3a --- /dev/null +++ b/src/HotChocolate/Fusion/test/Shared/Books/BookQuery.cs @@ -0,0 +1,18 @@ +namespace HotChocolate.Fusion.Shared.Books; + +[GraphQLName("Query")] +public sealed class BookQuery +{ + public Book? BookById( + string id, + [Service] BookRepository repository) + => repository.GetBookById(id); + + public IEnumerable Books(int limit, [Service] BookRepository repository) + => repository.GetBooks(limit); + + public Author authorById( + string id, + [Service] BookRepository repository) + => new Author(id, repository.GetBooksByAuthorId(id)); +} \ No newline at end of file diff --git a/src/HotChocolate/Fusion/test/Shared/Books/BookRepository.cs b/src/HotChocolate/Fusion/test/Shared/Books/BookRepository.cs new file mode 100644 index 00000000000..b228f6b15fb --- /dev/null +++ b/src/HotChocolate/Fusion/test/Shared/Books/BookRepository.cs @@ -0,0 +1,25 @@ +namespace HotChocolate.Fusion.Shared.Books; + +public sealed class BookRepository +{ + private readonly Dictionary _books; + + public BookRepository() + { + _books = new[] + { + new Book("1", "1", "The first book") + }.ToDictionary(t => t.Id); + } + + public IEnumerable GetBooks(int limit) + => _books.Values.OrderBy(t => t.Id).Take(limit); + + public Book? GetBookById(string id) + => _books.TryGetValue(id, out var book) + ? book + : null; + + public IEnumerable GetBooksByAuthorId(string authorId) + => _books.Values.Where(b => b.AuthorId.Equals(authorId)); +} diff --git a/src/HotChocolate/Fusion/test/Shared/DemoProject.cs b/src/HotChocolate/Fusion/test/Shared/DemoProject.cs index d1994d33bd8..b404db066d6 100644 --- a/src/HotChocolate/Fusion/test/Shared/DemoProject.cs +++ b/src/HotChocolate/Fusion/test/Shared/DemoProject.cs @@ -6,6 +6,8 @@ using HotChocolate.Fusion.Shared.Products; using HotChocolate.Fusion.Shared.Reviews; using HotChocolate.Fusion.Shared.Shipping; +using HotChocolate.Fusion.Shared.Books; +using HotChocolate.Fusion.Shared.Authors; using HotChocolate.Transport.Http; using HotChocolate.Types.Descriptors; using HotChocolate.Utilities.Introspection; @@ -29,6 +31,8 @@ private DemoProject( DemoSubgraph shipping, DemoSubgraph appointment, DemoSubgraph patient1, + DemoSubgraph books, + DemoSubgraph authors, IHttpClientFactory clientFactory, IWebSocketConnectionFactory webSocketConnectionFactory) { @@ -40,6 +44,8 @@ private DemoProject( Shipping = shipping; Appointment = appointment; Patient1 = patient1; + Books = books; + Authors = authors; HttpClientFactory = clientFactory; WebSocketConnectionFactory = webSocketConnectionFactory; } @@ -62,6 +68,10 @@ private DemoProject( public DemoSubgraph Patient1 { get; } + public DemoSubgraph Books { get; } + + public DemoSubgraph Authors { get; } + public static async Task CreateAsync(CancellationToken ct = default) { var disposables = new List(); @@ -215,6 +225,41 @@ public static async Task CreateAsync(CancellationToken ct = default .DownloadSchemaAsync(patient1Client, ct) .ConfigureAwait(false); + var books = testServerFactory.Create( + s => s + .AddRouting() + .AddGraphQLServer() + .AddQueryType() + .AddConvention(_ => new DefaultNamingConventions()), + c => c + .UseRouting() + .UseEndpoints(endpoints => endpoints.MapGraphQL())); + disposables.Add(books); + + var booksClient = books.CreateClient(); + booksClient.BaseAddress = new Uri("http://localhost:5000/graphql"); + var booksSchema = await introspection + .DownloadSchemaAsync(booksClient, ct) + .ConfigureAwait(false); + + var authors = testServerFactory.Create( + s => s + .AddRouting() + .AddGraphQLServer() + .AddQueryType() + .AddConvention(_ => new DefaultNamingConventions()), + c => c + .UseRouting() + .UseEndpoints(endpoints => endpoints.MapGraphQL())); + disposables.Add(authors); + + var authorsClient = authors.CreateClient(); + authorsClient.BaseAddress = new Uri("http://localhost:5000/graphql"); + var authorsSchema = await introspection + .DownloadSchemaAsync(authorsClient, ct) + .ConfigureAwait(false); + + var httpClients = new Dictionary> { { @@ -287,6 +332,26 @@ public static async Task CreateAsync(CancellationToken ct = default return httpClient; } }, + { + "Books", () => + { + // ReSharper disable once AccessToDisposedClosure + var httpClient = books.CreateClient(); + httpClient.BaseAddress = new Uri("http://localhost:5000/graphql"); + httpClient.DefaultRequestHeaders.AddGraphQLPreflight(); + return httpClient; + } + }, + { + "Authors", () => + { + // ReSharper disable once AccessToDisposedClosure + var httpClient = books.CreateClient(); + httpClient.BaseAddress = new Uri("http://localhost:5000/graphql"); + httpClient.DefaultRequestHeaders.AddGraphQLPreflight(); + return httpClient; + } + }, }; var webSocketClients = new Dictionary> @@ -312,6 +377,12 @@ public static async Task CreateAsync(CancellationToken ct = default { "Patient1", () => new MockWebSocketConnection(patient1.CreateWebSocketClient()) }, + { + "Books", () => new MockWebSocketConnection(books.CreateWebSocketClient()) + }, + { + "Authors", () => new MockWebSocketConnection(authors.CreateWebSocketClient()) + }, }; return new DemoProject( @@ -358,6 +429,18 @@ public static async Task CreateAsync(CancellationToken ct = default new Uri("ws://localhost:5000/graphql"), patient1Schema, patient1), + new DemoSubgraph( + "Books", + booksClient.BaseAddress, + new Uri("ws://localhost:5000/graphql"), + booksSchema, + books), + new DemoSubgraph( + "Authors", + authorsClient.BaseAddress, + new Uri("ws://localhost:5000/graphql"), + authorsSchema, + authors), new MockHttpClientFactory(httpClients), new MockWebSocketConnectionFactory(webSocketClients)); } From d5964b7f12d0e29a754369b877f3b6526444e974 Mon Sep 17 00:00:00 2001 From: ndejaco2 Date: Fri, 1 Sep 2023 15:33:27 -0700 Subject: [PATCH 2/2] Add potential fix for failing test and update snapshots --- .../Pipeline/RequirementsPlannerMiddleware.cs | 6 +- .../test/Core.Tests/RequestPlannerTests.cs | 9 +- ...de_Single_Fragment_Multiple_Subgraphs.snap | 2 +- ...iple_Require_Steps_From_Same_Subgraph.snap | 121 ++++++++++++++++++ 4 files changed, 133 insertions(+), 5 deletions(-) create mode 100644 src/HotChocolate/Fusion/test/Core.Tests/__snapshots__/RequestPlannerTests.Query_Plan_27_Multiple_Require_Steps_From_Same_Subgraph.snap diff --git a/src/HotChocolate/Fusion/src/Core/Planning/Pipeline/RequirementsPlannerMiddleware.cs b/src/HotChocolate/Fusion/src/Core/Planning/Pipeline/RequirementsPlannerMiddleware.cs index 1d2adcf42ba..e3a5a012625 100644 --- a/src/HotChocolate/Fusion/src/Core/Planning/Pipeline/RequirementsPlannerMiddleware.cs +++ b/src/HotChocolate/Fusion/src/Core/Planning/Pipeline/RequirementsPlannerMiddleware.cs @@ -64,7 +64,9 @@ currentStep.ParentSelection is { } parent && roots.Add(siblingExecutionStep.SubgraphName); } - schemas.TryAdd(siblingExecutionStep.SubgraphName, siblingExecutionStep); + // Tracks the most recent execution step (by query plan step order) targeting a given subgraph + // Replacing a previous execution step if necessary. + schemas[siblingExecutionStep.SubgraphName] = siblingExecutionStep; } // clean and fill requires set @@ -94,7 +96,7 @@ currentStep.ParentSelection is { } parent && // already depends on. var variables = OrderByUsage(step.SelectionSetTypeMetadata.Variables, currentStep); - // if we have root steps as siblings we will prfer to fulfill the requirements + // if we have root steps as siblings we will prefer to fulfill the requirements // from these steps. if (roots.Count > 0 && requires.Count > 0) { diff --git a/src/HotChocolate/Fusion/test/Core.Tests/RequestPlannerTests.cs b/src/HotChocolate/Fusion/test/Core.Tests/RequestPlannerTests.cs index 046a920ef9e..22554e06758 100644 --- a/src/HotChocolate/Fusion/test/Core.Tests/RequestPlannerTests.cs +++ b/src/HotChocolate/Fusion/test/Core.Tests/RequestPlannerTests.cs @@ -1057,8 +1057,8 @@ query TopProducts { await snapshot.MatchAsync(); } - [Fact(Skip = "This test currently fails during query planning")] - public async Task Query_Plan_27_Selection_Set_Empty() + [Fact] + public async Task Query_Plan_27_Multiple_Require_Steps_From_Same_Subgraph() { // arrange using var demoProject = await DemoProject.CreateAsync(); @@ -1090,6 +1090,11 @@ query Query { } } """); + + var snapshot = new Snapshot(); + snapshot.Add(result.UserRequest, nameof(result.UserRequest)); + snapshot.Add(result.QueryPlan, nameof(result.QueryPlan)); + await snapshot.MatchAsync(); } private static async Task<(DocumentNode UserRequest, Execution.Nodes.QueryPlan QueryPlan)> CreateQueryPlanAsync( diff --git a/src/HotChocolate/Fusion/test/Core.Tests/__snapshots__/RequestPlannerTests.Query_Plan_18_Node_Single_Fragment_Multiple_Subgraphs.snap b/src/HotChocolate/Fusion/test/Core.Tests/__snapshots__/RequestPlannerTests.Query_Plan_18_Node_Single_Fragment_Multiple_Subgraphs.snap index 44e884de690..b00ed7620cd 100644 --- a/src/HotChocolate/Fusion/test/Core.Tests/__snapshots__/RequestPlannerTests.Query_Plan_18_Node_Single_Fragment_Multiple_Subgraphs.snap +++ b/src/HotChocolate/Fusion/test/Core.Tests/__snapshots__/RequestPlannerTests.Query_Plan_18_Node_Single_Fragment_Multiple_Subgraphs.snap @@ -50,7 +50,7 @@ QueryPlan "node": { "type": "Resolve", "subgraph": "Reviews2", - "document": "query FetchNode_2($id: ID!) { node(id: $id) { ... on User { reviews { body } __typename } } }", + "document": "query FetchNode_2($id: ID!) { node(id: $id) { ... on User { reviews { body } __fusion_exports__2: id __typename } } }", "selectionSetId": 0, "forwardedVariables": [ { diff --git a/src/HotChocolate/Fusion/test/Core.Tests/__snapshots__/RequestPlannerTests.Query_Plan_27_Multiple_Require_Steps_From_Same_Subgraph.snap b/src/HotChocolate/Fusion/test/Core.Tests/__snapshots__/RequestPlannerTests.Query_Plan_27_Multiple_Require_Steps_From_Same_Subgraph.snap new file mode 100644 index 00000000000..98769d7a26b --- /dev/null +++ b/src/HotChocolate/Fusion/test/Core.Tests/__snapshots__/RequestPlannerTests.Query_Plan_27_Multiple_Require_Steps_From_Same_Subgraph.snap @@ -0,0 +1,121 @@ +UserRequest +--------------- +query Query { + authorById(id: "1") { + id + name + bio + books { + id + author { + books { + id + } + } + } + } +} +--------------- + +QueryPlan +--------------- +{ + "document": "query Query { authorById(id: \u00221\u0022) { id name bio books { id author { books { id } } } } }", + "operation": "Query", + "rootNode": { + "type": "Sequence", + "nodes": [ + { + "type": "Resolve", + "subgraph": "Authors", + "document": "query Query_1 { authorById(id: \u00221\u0022) { id name bio __fusion_exports__1: id } }", + "selectionSetId": 0, + "provides": [ + { + "variable": "__fusion_exports__1" + }, + { + "variable": "__fusion_exports__2" + } + ] + }, + { + "type": "Compose", + "selectionSetIds": [ + 0 + ] + }, + { + "type": "Parallel", + "nodes": [ + { + "type": "Resolve", + "subgraph": "Books", + "document": "query Query_2($__fusion_exports__1: String!) { authorById(id: $__fusion_exports__1) { books { id } } }", + "selectionSetId": 1, + "path": [ + "authorById" + ], + "requires": [ + { + "variable": "__fusion_exports__1" + } + ] + }, + { + "type": "Resolve", + "subgraph": "Authors", + "document": "query Query_3($__fusion_exports__2: String!) { bookByAuthorId(authorId: $__fusion_exports__2) { author { __fusion_exports__3: id } } }", + "selectionSetId": 2, + "path": [ + "bookByAuthorId" + ], + "requires": [ + { + "variable": "__fusion_exports__2" + } + ], + "provides": [ + { + "variable": "__fusion_exports__3" + } + ] + } + ] + }, + { + "type": "Compose", + "selectionSetIds": [ + 1, + 2 + ] + }, + { + "type": "Resolve", + "subgraph": "Books", + "document": "query Query_4($__fusion_exports__3: String!) { authorById(id: $__fusion_exports__3) { books { id } } }", + "selectionSetId": 3, + "path": [ + "authorById" + ], + "requires": [ + { + "variable": "__fusion_exports__3" + } + ] + }, + { + "type": "Compose", + "selectionSetIds": [ + 3 + ] + } + ] + }, + "state": { + "__fusion_exports__1": "Author_id", + "__fusion_exports__2": "Book_authorId", + "__fusion_exports__3": "Author_id" + } +} +---------------