Merge pull request #577 from EntityGraphQL/fix/connection-in-connecti… · EntityGraphQL/EntityGraphQL@9795c5a · GitHub
Skip to content

Commit 9795c5a

Browse files
authored
Merge pull request #577 from EntityGraphQL/fix/connection-in-connection-node
Fix paging on the node of another connection
2 parents 59e5f3b + c631041 commit 9795c5a

7 files changed

Lines changed: 200 additions & 5 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions

‎src/EntityGraphQL/Compiler/GqlNodes/CompileContext.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,19 @@ public void AddConstant(IField? fromField, ParameterExpression parameterExpressi
146146

147147
public bool WasFirstPassMaterialized(IGraphQLNode node) => firstPassMaterializedNodes.Contains(node);
148148

149+
/// <summary>
150+
/// The context a field with extensions was compiled against (bound to its field parameter). A paging field's
151+
/// edges/items field rebuilds the collection from the original field expression and needs the same context. The
152+
/// grandparent node's NextFieldContext is not it when the paging field is on an object selected from a schema
153+
/// expression, e.g. the `node` of another connection (p_ConnectionEdge.Node, not the edge being selected). Stored
154+
/// here as the node belongs to the cached document shared across requests.
155+
/// </summary>
156+
private readonly Dictionary<IGraphQLNode, Expression> fieldContexts = [];
157+
158+
internal void SetFieldContext(IGraphQLNode node, Expression context) => fieldContexts[node] = context;
159+
160+
internal Expression? GetFieldContext(IGraphQLNode node) => fieldContexts.TryGetValue(node, out var context) ? context : null;
161+
149162
/// <summary>
150163
/// Carry the first pass's state into the context used for the second (services) pass.
151164
/// </summary>

‎src/EntityGraphQL/Schema/Field.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,10 @@ ParameterReplacer replacer
238238
)
239239
{
240240
Expression? expression = fieldExpression;
241+
// extensions are given the field's own expression as Context. Record the context it is bound to below so an
242+
// extension compiling a child field can rebuild from it - see CompileContext.GetFieldContext
243+
if (fieldContext != null && fieldNode != null && !contextChanged && Extensions.Count > 0)
244+
compileContext.SetFieldContext(fieldNode, fieldContext);
241245
// don't store parameterReplacer as a class field as GetExpression is called in compiling - i.e. across threads
242246
(var result, var argumentParam) = PrepareFieldExpression(args, expression!, replacer, expression, fieldNode, docParam, docVariables, contextChanged, withoutServiceFields, compileContext);
243247
if (result == null)

‎src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionEdgeExtension.cs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,11 @@ namespace EntityGraphQL.Schema.FieldExtensions;
1111
public class ConnectionEdgeExtension : BaseFieldExtension
1212
{
1313
private readonly Type listType;
14-
private readonly ParameterExpression firstSelectParam;
1514

1615
/// <param name="listType">The element type of the paged collection</param>
1716
public ConnectionEdgeExtension(Type listType)
1817
{
1918
this.listType = listType;
20-
firstSelectParam = Expression.Parameter(listType, "edgeNode");
2119
}
2220

2321
/// <param name="listType">The element type of the paged collection</param>
@@ -98,7 +96,8 @@ FieldExtensionExpressionContext context
9896
}
9997
else
10098
{
101-
expression = parameterReplacer.Replace(pagingExtension.OriginalFieldExpression!, parentField.FieldParam!, grandparentContext!);
99+
var parentContext = compileContext.GetFieldContext(fieldNode.ParentNode) ?? grandparentContext!;
100+
expression = parameterReplacer.Replace(pagingExtension.OriginalFieldExpression!, parentField.FieldParam!, parentContext);
102101
}
103102

104103
// expression here is the adjusted Connection<T>(). This field (edges) is where we deal with the list again - field.Resolve
@@ -232,6 +231,10 @@ ref edgeExpression
232231
// First we select the edge node as the full object
233232
// we later change this to a anonymous object to not have the full table returned from EF
234233
// This happens later as we don't know what the query has selected yet
234+
// A parameter per use, not per extension: the edges field is shared by every connection of listType, so the same
235+
// field below its own node (actor.movies.node.actors.node.movies) would otherwise declare the same parameter in
236+
// nested lambdas, which EF can not rewrite
237+
var firstSelectParam = Expression.Parameter(listType, "edgeNode");
235238
expression = Expression.Call(
236239
isQueryable ? typeof(Queryable) : typeof(Enumerable),
237240
nameof(Enumerable.Select),
@@ -269,7 +272,9 @@ FieldExtensionSelectionContext context
269272

270273
// we now know the fields they want to select so we rebuild the base expression
271274
// remove the above Select(new ConnectionEdge<T>(), ...)
272-
baseExpression = ((MethodCallExpression)baseExpression).Arguments[0];
275+
var edgeSelect = (MethodCallExpression)baseExpression;
276+
var firstSelectParam = ((LambdaExpression)(edgeSelect.Arguments[1] is UnaryExpression { NodeType: ExpressionType.Quote } quote ? quote.Operand : edgeSelect.Arguments[1])).Parameters[0];
277+
baseExpression = edgeSelect.Arguments[0];
273278
var isQueryable = baseExpression.Type.IsGenericTypeQueryable();
274279
// remove null check as it is not required
275280
var nodeField = selectionExpressions.First(f => f.Key.SchemaName == "node").Value;

‎src/EntityGraphQL/Schema/FieldExtensions/OffsetPaging/OffsetPagingItemsExtension.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,8 @@ FieldExtensionExpressionContext context
8484
}
8585
else
8686
{
87-
newItemsExp = parameterReplacer.Replace(offsetPagingExtension.OriginalFieldExpression!, originalFieldParam, grandparentContext!);
87+
var parentContext = compileContext.GetFieldContext(fieldNode.ParentNode) ?? grandparentContext!;
88+
newItemsExp = parameterReplacer.Replace(offsetPagingExtension.OriginalFieldExpression!, originalFieldParam, parentContext);
8889
}
8990

9091
// other extensions defined on the original field need to run on the collection

‎src/tests/EntityGraphQL.EF.Tests/PagingTests.cs‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,131 @@ public void TestRootConnectionPagingLastBeforePastTheEnd()
549549
Assert.Equal(new[] { 1, 2 }.Select(ConnectionHelper.SerializeCursor), ((IEnumerable<dynamic>)movies.edges).Select(e => (string)e.cursor));
550550
}
551551

552+
// a nested connection on the node of a root one
553+
[Fact]
554+
public void TestNestedConnectionPagingInConnectionNode()
555+
{
556+
var schema = SchemaBuilder.FromObject<TestDbContext>();
557+
using var factory = new TestDbContextFactory();
558+
var data = factory.CreateContext();
559+
AddActorsAndMovies(data);
560+
data.SaveChanges();
561+
562+
schema.Query().ReplaceField("movies", db => db.Movies.OrderBy(m => m.Id), "Movies").UseConnectionPaging();
563+
schema.Type<Movie>().ReplaceField("actors", m => m.Actors.OrderBy(a => a.Id), "Actors").UseConnectionPaging();
564+
565+
var result = schema.ExecuteRequestWithContext(
566+
new QueryRequest { Query = @"{ movies(last: 2) { edges { cursor node { name actors(last: 2) { totalCount edges { cursor node { name } } } } } } }" },
567+
data,
568+
null,
569+
null
570+
);
571+
Assert.Null(result.Errors);
572+
var edges = ((IEnumerable<dynamic>)((dynamic)result.Data!["movies"]!).edges).ToList();
573+
Assert.Equal(new[] { "M2", "M3" }, edges.Select(e => (string)e.node.name));
574+
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(2), ConnectionHelper.SerializeCursor(3) }, edges.Select(e => (string)e.cursor));
575+
// M2's actors are A, B and M3's A, B, C
576+
Assert.Equal(2, edges[0].node.actors.totalCount);
577+
Assert.Equal(new[] { "A", "B" }, ((IEnumerable<dynamic>)edges[0].node.actors.edges).Select(e => (string)e.node.name));
578+
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(1), ConnectionHelper.SerializeCursor(2) }, ((IEnumerable<dynamic>)edges[0].node.actors.edges).Select(e => (string)e.cursor));
579+
Assert.Equal(3, edges[1].node.actors.totalCount);
580+
Assert.Equal(new[] { "B", "C" }, ((IEnumerable<dynamic>)edges[1].node.actors.edges).Select(e => (string)e.node.name));
581+
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(2), ConnectionHelper.SerializeCursor(3) }, ((IEnumerable<dynamic>)edges[1].node.actors.edges).Select(e => (string)e.cursor));
582+
}
583+
584+
// The same connection field at two levels (actor.movies ... actors.node.movies). EF needs SQL APPLY to page a nested
585+
// collection inside another paged nested collection, which SQLite does not support (SQL Server and Postgres do), so this
586+
// runs in memory and checks the expression EF would be given: the edges field is shared, and its Select must not declare
587+
// the same parameter in nested lambdas - EF can't rewrite that ("When called from 'VisitLambda', rewriting a node...")
588+
[Fact]
589+
public void TestNestedConnectionPagingSameFieldAtTwoLevels()
590+
{
591+
var schema = SchemaBuilder.FromObject<TestDbContext>();
592+
using var factory = new TestDbContextFactory();
593+
var data = factory.CreateContext();
594+
var actors = AddActorsAndMovies(data);
595+
596+
schema.Query().ReplaceField("actor", new { id = 0 }, (db, args) => actors.FirstOrDefault(a => a.Id == args.id), "Actor");
597+
schema.Type<Actor>().ReplaceField("movies", a => a.Movies.OrderBy(m => m.Id), "Movies").UseConnectionPaging();
598+
schema.Type<Movie>().ReplaceField("actors", m => m.Actors.OrderBy(a => a.Id), "Actors").UseConnectionPaging();
599+
600+
var expressions = new List<System.Linq.Expressions.Expression>();
601+
var result = schema.ExecuteRequestWithContext(
602+
new QueryRequest
603+
{
604+
Query =
605+
$@"{{ actor(id: 1) {{
606+
movies(last: 2) {{ totalCount edges {{ cursor node {{ name
607+
actors(first: 1, after: ""{ConnectionHelper.SerializeCursor(1)}"") {{ totalCount pageInfo {{ hasNextPage }} edges {{ cursor node {{ name
608+
movies(last: 1) {{ edges {{ cursor node {{ name }} }} }}
609+
}} }} }}
610+
}} }} }}
611+
}} }}",
612+
},
613+
data,
614+
null,
615+
null,
616+
new ExecutionOptions
617+
{
618+
BeforeExecuting = (e, _) =>
619+
{
620+
expressions.Add(e);
621+
return e;
622+
},
623+
}
624+
);
625+
Assert.Null(result.Errors);
626+
Assert.All(expressions, e => new NoShadowedParameters().Visit(e));
627+
628+
dynamic movies = ((dynamic)result.Data!["actor"]!).movies;
629+
Assert.Equal(3, movies.totalCount);
630+
Assert.Equal(new[] { "M2", "M3" }, ((IEnumerable<dynamic>)movies.edges).Select(e => (string)e.node.name));
631+
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(2), ConnectionHelper.SerializeCursor(3) }, ((IEnumerable<dynamic>)movies.edges).Select(e => (string)e.cursor));
632+
// M2's actors are A, B - after A
633+
dynamic m2Actors = movies.edges[0].node.actors;
634+
Assert.Equal(2, m2Actors.totalCount);
635+
Assert.False(m2Actors.pageInfo.hasNextPage);
636+
Assert.Equal("B", m2Actors.edges[0].node.name);
637+
Assert.Equal(ConnectionHelper.SerializeCursor(2), m2Actors.edges[0].cursor);
638+
// B's movies are M2, M3 - last 1
639+
Assert.Equal("M3", m2Actors.edges[0].node.movies.edges[0].node.name);
640+
Assert.Equal(ConnectionHelper.SerializeCursor(2), m2Actors.edges[0].node.movies.edges[0].cursor);
641+
// M3's actors are A, B, C - after A, first 1
642+
dynamic m3Actors = movies.edges[1].node.actors;
643+
Assert.Equal(3, m3Actors.totalCount);
644+
Assert.True(m3Actors.pageInfo.hasNextPage);
645+
Assert.Equal("B", m3Actors.edges[0].node.name);
646+
Assert.Equal(ConnectionHelper.SerializeCursor(2), m3Actors.edges[0].cursor);
647+
}
648+
649+
private sealed class NoShadowedParameters : System.Linq.Expressions.ExpressionVisitor
650+
{
651+
private readonly HashSet<System.Linq.Expressions.ParameterExpression> inScope = [];
652+
653+
protected override System.Linq.Expressions.Expression VisitLambda<T>(System.Linq.Expressions.Expression<T> node)
654+
{
655+
foreach (var p in node.Parameters)
656+
Assert.True(inScope.Add(p), $"Parameter '{p.Name}' is declared by a lambda nested in another declaring it");
657+
base.VisitLambda(node);
658+
foreach (var p in node.Parameters)
659+
inScope.Remove(p);
660+
return node;
661+
}
662+
}
663+
664+
private static List<Actor> AddActorsAndMovies(TestDbContext data)
665+
{
666+
var m1 = new Movie("M1") { Id = 1, Released = new DateTime(2020, 1, 1) };
667+
var m2 = new Movie("M2") { Id = 2, Released = new DateTime(2021, 1, 1) };
668+
var m3 = new Movie("M3") { Id = 3, Released = new DateTime(2022, 1, 1) };
669+
List<Actor> actors = [new Actor("A") { Id = 1, Movies = [m1, m2, m3] }, new Actor("B") { Id = 2, Movies = [m2, m3] }, new Actor("C") { Id = 3, Movies = [m3] }];
670+
m1.Actors = [actors[0]];
671+
m2.Actors = [actors[0], actors[1]];
672+
m3.Actors = [actors[0], actors[1], actors[2]];
673+
data.Actors.AddRange(actors);
674+
return actors;
675+
}
676+
552677
private static TestDbContext CreateActorWithMovies(TestDbContextFactory factory)
553678
{
554679
var data = factory.CreateContext();

‎src/tests/EntityGraphQL.Tests/ConnectionPaging/ConnectionPagingTests.cs‎

Lines changed: 46 additions & 0 deletions

0 commit comments

Comments
 (0)