Skip to content

Commit 4af6ce4

Browse files
lukemurrayclaude
andcommitted
Fix paging on the node of another connection
{ movies(first: 2) { edges { node { actors(first: 1) { ... } } } } } with UseConnectionPaging (or UseOffsetPaging) on the inner field failed with "unbound variable: p_ConnectionEdge`1", in memory and with EF. - The inner edges/items field rebuilt its collection from the grandparent node's NextFieldContext. For `node` that is the schema expression p_ConnectionEdge.Node, not the edge being selected. Field.GetExpression now records the context a field with extensions is bound to on the (per-request) CompileContext, and the edges/items extensions use it. - The edges field (and its ConnectionEdgeExtension) is shared by every connection of a type, and its Select parameter was a field on the extension. The same connection at two levels declared it in nested lambdas, which EF can not rewrite ("When called from 'VisitLambda'..."). Create it per use. EF still needs SQL APPLY to page a nested collection inside another paged nested collection, which SQLite does not support - the two-level case is tested in memory with a check that no lambda shadows a parameter. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 9847158 commit 4af6ce4

7 files changed

Lines changed: 200 additions & 5 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
- Fixed `UseOffsetPaging()` on a field that is not on the root query type when its collection comes from EF - e.g. `schema.Type<Actor>().ReplaceField("movies", new { }, (a, _) => a.Movies.OrderBy(m => m.Released)).UseOffsetPaging()` - failing with `The LINQ expression 'p_Movie => new Dynamic_items...' could not be translated`. The collection is part of the parent's projection, so EF has to translate the paging, and `items` was paged with EntityGraphQL's own `Skip`/`Take(int?)` helpers, which EF does not know. A collection that is not an `IQueryable` is now paged with `System.Linq`'s `Skip`/`Take` (a null `skip` is `0`, a null `take` is the rest of the collection - `int.MaxValue - skip`, as EF pages a nested collection with `row <= skip + take`, which would overflow an `int` on SQL Server and Postgres). `hasNextPage` on such a field is translated too: EF evaluated EntityGraphQL's `PageHasNext` helper in memory, loading the whole collection for every parent, and it is now an `EXISTS` query. Paging on root query fields was not affected.
1616
- #574 - Fixed `UseConnectionPaging()` on a field that is not on the root query type when its collection comes from EF - e.g. `schema.Type<Actor>().ReplaceField("movies", a => a.Movies.OrderByDescending(m => m.Released)).UseConnectionPaging()` - failing with `The LINQ expression 'edgeNode => ...' could not be translated`. As with offset paging above, the edges are now paged with `System.Linq`'s `Skip`/`Take`. Cursors were assigned by `ConnectionHelper.ApplyCursors` while enumerating the page, which EF can not translate inside a parent's projection, so for such a collection they are now set on the edges after EF materializes them. `last` (without `before`) on such a collection pages from the end of each parent's own collection - it relied on one total count shared by the whole request - and is taken as `Reverse().Take(last)` so EF can translate it. With `after` too, the rows up to `after` are then dropped from that page. `last` with or without `before` on such a collection needs it to be ordered (e.g. `a.Movies.OrderBy(m => m.Released)`) when it comes from EF, which can not `Reverse()` an unordered collection - it fails with EF's `could not be translated` error. An in-memory collection does not need to be ordered. `pageInfo { hasNextPage }` alone is an `EXISTS` query rather than loading the whole collection. Paging on root query fields is unchanged.
1717
- Fixed a connection field paging a collection of the same type at the root (`IQueryable`) and on a parent object sharing one `ConnectionEdgeExtension`, which used whether the first configured field was `IQueryable` for both (`No generic method 'Skip' on type 'QueryableExtensions'`). It is now decided per use.
18+
- Fixed paging on the `node` of another connection - e.g. `movies(first: 2) { edges { node { actors(first: 1) { ... } } } }` with `UseConnectionPaging()` (or `UseOffsetPaging()`) on both - failing with `unbound variable: p_ConnectionEdge`1`, in memory and with EF. The inner paging's edges/items rebuilt its collection from the schema's `node` expression rather than the edge being selected. Also, as the edges field is shared by every connection of a type, the same connection field at two levels (`actor.movies ... actors.node.movies`) declared the same parameter in nested lambdas, which EF can not rewrite. Note that EF needs SQL `APPLY` to page a nested collection inside another paged nested collection, which SQLite does not support (SQL Server and Postgres do).
1819
- Fixed connection paging `pageInfo.hasPreviousPage` being `false` for `last` without `before` when exactly one item comes before the page (e.g. `last: 2` of 3 items).
1920
- Fixed connection paging with `last` and a `before` cursor past the end of the collection - e.g. `last: 3, before: <cursor 5>` of 3 items - returning too few items (`2` and `3` rather than all 3), as the page was taken as ending at the cursor. It is now the last items of the collection, with matching cursors and `pageInfo`. On a nested collection the cursor is shared by every parent and can be past the end of only some of them: each parent's page is chosen from its own count, at the cost of also selecting the last `last` items of each parent alongside the page.
2021
- Fixed connection paging `pageInfo.hasNextPage` being `false` for `before` when the cursor is the last item (e.g. `last: 3, before: <cursor 6>` of 6 items), although the item at the cursor comes after the page.

‎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
Original file line numberDiff line numberDiff line change
@@ -787,6 +787,52 @@ private class ProjectWrapper
787787
public Project? Project { get; set; }
788788
}
789789

790+
// paging on the node of another connection. The inner paging rebuilds its collection from the context its field is
791+
// compiled against - the edge being selected - not the schema's p_ConnectionEdge.Node ("unbound variable" before)
792+
[Fact]
793+
public void TestPagingInConnectionNode()
794+
{
795+
var schema = SchemaBuilder.FromObject<TestDataContext>();
796+
var data = new TestDataContext();
797+
FillProjectData(data);
798+
799+
schema.Query().ReplaceField("projects", ctx => ctx.Projects.OrderBy(p => p.Id), "Projects").UseConnectionPaging();
800+
schema.Type<Project>().ReplaceField("tasks", p => p.Tasks.OrderBy(t => t.Id), "Tasks").UseConnectionPaging();
801+
schema.Type<Project>().AddField("pagedTasks", p => p.Tasks.OrderBy(t => t.Id), "Tasks").UseOffsetPaging();
802+
803+
var result = schema.ExecuteRequestWithContext(
804+
new QueryRequest
805+
{
806+
Query =
807+
$@"{{
808+
projects(first: 1) {{
809+
edges {{ cursor node {{
810+
name
811+
tasks(first: 2, after: ""{ConnectionHelper.SerializeCursor(1)}"") {{ totalCount edges {{ cursor node {{ name }} }} }}
812+
pagedTasks(skip: 3, take: 5) {{ totalItems hasNextPage items {{ name }} }}
813+
}} }}
814+
}}
815+
}}",
816+
},
817+
data,
818+
null,
819+
null
820+
);
821+
Assert.Null(result.Errors);
822+
823+
dynamic project = ((dynamic)result.Data!["projects"]!).edges[0];
824+
Assert.Equal(ConnectionHelper.SerializeCursor(1), project.cursor);
825+
Assert.Equal("Project 1", project.node.name);
826+
dynamic tasks = project.node.tasks;
827+
Assert.Equal(5, tasks.totalCount);
828+
Assert.Equal(new[] { "Task 2", "Task 3" }, ((IEnumerable<dynamic>)tasks.edges).Select(e => (string)e.node.name));
829+
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(2), ConnectionHelper.SerializeCursor(3) }, ((IEnumerable<dynamic>)tasks.edges).Select(e => (string)e.cursor));
830+
dynamic pagedTasks = project.node.pagedTasks;
831+
Assert.Equal(5, pagedTasks.totalItems);
832+
Assert.False(pagedTasks.hasNextPage);
833+
Assert.Equal(new[] { "Task 4", "Task 5" }, ((IEnumerable<dynamic>)pagedTasks.items).Select(i => (string)i.name));
834+
}
835+
790836
private static void FillProjectData(TestDataContext data)
791837
{
792838
data.Projects =

0 commit comments

Comments
 (0)