Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
- 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.
- #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.
- 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.
- 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).
- 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).
- 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.
- 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.
Expand Down
13 changes: 13 additions & 0 deletions src/EntityGraphQL/Compiler/GqlNodes/CompileContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,19 @@ public void AddConstant(IField? fromField, ParameterExpression parameterExpressi

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

/// <summary>
/// The context a field with extensions was compiled against (bound to its field parameter). A paging field's
/// edges/items field rebuilds the collection from the original field expression and needs the same context. The
/// grandparent node's NextFieldContext is not it when the paging field is on an object selected from a schema
/// expression, e.g. the `node` of another connection (p_ConnectionEdge.Node, not the edge being selected). Stored
/// here as the node belongs to the cached document shared across requests.
/// </summary>
private readonly Dictionary<IGraphQLNode, Expression> fieldContexts = [];

internal void SetFieldContext(IGraphQLNode node, Expression context) => fieldContexts[node] = context;

internal Expression? GetFieldContext(IGraphQLNode node) => fieldContexts.TryGetValue(node, out var context) ? context : null;

/// <summary>
/// Carry the first pass's state into the context used for the second (services) pass.
/// </summary>
Expand Down
4 changes: 4 additions & 0 deletions src/EntityGraphQL/Schema/Field.cs
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,10 @@ ParameterReplacer replacer
)
{
Expression? expression = fieldExpression;
// extensions are given the field's own expression as Context. Record the context it is bound to below so an
// extension compiling a child field can rebuild from it - see CompileContext.GetFieldContext
if (fieldContext != null && fieldNode != null && !contextChanged && Extensions.Count > 0)
compileContext.SetFieldContext(fieldNode, fieldContext);
// don't store parameterReplacer as a class field as GetExpression is called in compiling - i.e. across threads
(var result, var argumentParam) = PrepareFieldExpression(args, expression!, replacer, expression, fieldNode, docParam, docVariables, contextChanged, withoutServiceFields, compileContext);
if (result == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@ namespace EntityGraphQL.Schema.FieldExtensions;
public class ConnectionEdgeExtension : BaseFieldExtension
{
private readonly Type listType;
private readonly ParameterExpression firstSelectParam;

/// <param name="listType">The element type of the paged collection</param>
public ConnectionEdgeExtension(Type listType)
{
this.listType = listType;
firstSelectParam = Expression.Parameter(listType, "edgeNode");
}

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

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

// we now know the fields they want to select so we rebuild the base expression
// remove the above Select(new ConnectionEdge<T>(), ...)
baseExpression = ((MethodCallExpression)baseExpression).Arguments[0];
var edgeSelect = (MethodCallExpression)baseExpression;
var firstSelectParam = ((LambdaExpression)(edgeSelect.Arguments[1] is UnaryExpression { NodeType: ExpressionType.Quote } quote ? quote.Operand : edgeSelect.Arguments[1])).Parameters[0];
baseExpression = edgeSelect.Arguments[0];
var isQueryable = baseExpression.Type.IsGenericTypeQueryable();
// remove null check as it is not required
var nodeField = selectionExpressions.First(f => f.Key.SchemaName == "node").Value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ FieldExtensionExpressionContext context
}
else
{
newItemsExp = parameterReplacer.Replace(offsetPagingExtension.OriginalFieldExpression!, originalFieldParam, grandparentContext!);
var parentContext = compileContext.GetFieldContext(fieldNode.ParentNode) ?? grandparentContext!;
newItemsExp = parameterReplacer.Replace(offsetPagingExtension.OriginalFieldExpression!, originalFieldParam, parentContext);
}

// other extensions defined on the original field need to run on the collection
Expand Down
125 changes: 125 additions & 0 deletions src/tests/EntityGraphQL.EF.Tests/PagingTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,131 @@ public void TestRootConnectionPagingLastBeforePastTheEnd()
Assert.Equal(new[] { 1, 2 }.Select(ConnectionHelper.SerializeCursor), ((IEnumerable<dynamic>)movies.edges).Select(e => (string)e.cursor));
}

// a nested connection on the node of a root one
[Fact]
public void TestNestedConnectionPagingInConnectionNode()
{
var schema = SchemaBuilder.FromObject<TestDbContext>();
using var factory = new TestDbContextFactory();
var data = factory.CreateContext();
AddActorsAndMovies(data);
data.SaveChanges();

schema.Query().ReplaceField("movies", db => db.Movies.OrderBy(m => m.Id), "Movies").UseConnectionPaging();
schema.Type<Movie>().ReplaceField("actors", m => m.Actors.OrderBy(a => a.Id), "Actors").UseConnectionPaging();

var result = schema.ExecuteRequestWithContext(
new QueryRequest { Query = @"{ movies(last: 2) { edges { cursor node { name actors(last: 2) { totalCount edges { cursor node { name } } } } } } }" },
data,
null,
null
);
Assert.Null(result.Errors);
var edges = ((IEnumerable<dynamic>)((dynamic)result.Data!["movies"]!).edges).ToList();
Assert.Equal(new[] { "M2", "M3" }, edges.Select(e => (string)e.node.name));
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(2), ConnectionHelper.SerializeCursor(3) }, edges.Select(e => (string)e.cursor));
// M2's actors are A, B and M3's A, B, C
Assert.Equal(2, edges[0].node.actors.totalCount);
Assert.Equal(new[] { "A", "B" }, ((IEnumerable<dynamic>)edges[0].node.actors.edges).Select(e => (string)e.node.name));
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(1), ConnectionHelper.SerializeCursor(2) }, ((IEnumerable<dynamic>)edges[0].node.actors.edges).Select(e => (string)e.cursor));
Assert.Equal(3, edges[1].node.actors.totalCount);
Assert.Equal(new[] { "B", "C" }, ((IEnumerable<dynamic>)edges[1].node.actors.edges).Select(e => (string)e.node.name));
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(2), ConnectionHelper.SerializeCursor(3) }, ((IEnumerable<dynamic>)edges[1].node.actors.edges).Select(e => (string)e.cursor));
}

// The same connection field at two levels (actor.movies ... actors.node.movies). EF needs SQL APPLY to page a nested
// collection inside another paged nested collection, which SQLite does not support (SQL Server and Postgres do), so this
// runs in memory and checks the expression EF would be given: the edges field is shared, and its Select must not declare
// the same parameter in nested lambdas - EF can't rewrite that ("When called from 'VisitLambda', rewriting a node...")
[Fact]
public void TestNestedConnectionPagingSameFieldAtTwoLevels()
{
var schema = SchemaBuilder.FromObject<TestDbContext>();
using var factory = new TestDbContextFactory();
var data = factory.CreateContext();
var actors = AddActorsAndMovies(data);

schema.Query().ReplaceField("actor", new { id = 0 }, (db, args) => actors.FirstOrDefault(a => a.Id == args.id), "Actor");
schema.Type<Actor>().ReplaceField("movies", a => a.Movies.OrderBy(m => m.Id), "Movies").UseConnectionPaging();
schema.Type<Movie>().ReplaceField("actors", m => m.Actors.OrderBy(a => a.Id), "Actors").UseConnectionPaging();

var expressions = new List<System.Linq.Expressions.Expression>();
var result = schema.ExecuteRequestWithContext(
new QueryRequest
{
Query =
$@"{{ actor(id: 1) {{
movies(last: 2) {{ totalCount edges {{ cursor node {{ name
actors(first: 1, after: ""{ConnectionHelper.SerializeCursor(1)}"") {{ totalCount pageInfo {{ hasNextPage }} edges {{ cursor node {{ name
movies(last: 1) {{ edges {{ cursor node {{ name }} }} }}
}} }} }}
}} }} }}
}} }}",
},
data,
null,
null,
new ExecutionOptions
{
BeforeExecuting = (e, _) =>
{
expressions.Add(e);
return e;
},
}
);
Assert.Null(result.Errors);
Assert.All(expressions, e => new NoShadowedParameters().Visit(e));

dynamic movies = ((dynamic)result.Data!["actor"]!).movies;
Assert.Equal(3, movies.totalCount);
Assert.Equal(new[] { "M2", "M3" }, ((IEnumerable<dynamic>)movies.edges).Select(e => (string)e.node.name));
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(2), ConnectionHelper.SerializeCursor(3) }, ((IEnumerable<dynamic>)movies.edges).Select(e => (string)e.cursor));
// M2's actors are A, B - after A
dynamic m2Actors = movies.edges[0].node.actors;
Assert.Equal(2, m2Actors.totalCount);
Assert.False(m2Actors.pageInfo.hasNextPage);
Assert.Equal("B", m2Actors.edges[0].node.name);
Assert.Equal(ConnectionHelper.SerializeCursor(2), m2Actors.edges[0].cursor);
// B's movies are M2, M3 - last 1
Assert.Equal("M3", m2Actors.edges[0].node.movies.edges[0].node.name);
Assert.Equal(ConnectionHelper.SerializeCursor(2), m2Actors.edges[0].node.movies.edges[0].cursor);
// M3's actors are A, B, C - after A, first 1
dynamic m3Actors = movies.edges[1].node.actors;
Assert.Equal(3, m3Actors.totalCount);
Assert.True(m3Actors.pageInfo.hasNextPage);
Assert.Equal("B", m3Actors.edges[0].node.name);
Assert.Equal(ConnectionHelper.SerializeCursor(2), m3Actors.edges[0].cursor);
}

private sealed class NoShadowedParameters : System.Linq.Expressions.ExpressionVisitor
{
private readonly HashSet<System.Linq.Expressions.ParameterExpression> inScope = [];

protected override System.Linq.Expressions.Expression VisitLambda<T>(System.Linq.Expressions.Expression<T> node)
{
foreach (var p in node.Parameters)
Assert.True(inScope.Add(p), $"Parameter '{p.Name}' is declared by a lambda nested in another declaring it");
base.VisitLambda(node);
foreach (var p in node.Parameters)
inScope.Remove(p);
return node;
}
}

private static List<Actor> AddActorsAndMovies(TestDbContext data)
{
var m1 = new Movie("M1") { Id = 1, Released = new DateTime(2020, 1, 1) };
var m2 = new Movie("M2") { Id = 2, Released = new DateTime(2021, 1, 1) };
var m3 = new Movie("M3") { Id = 3, Released = new DateTime(2022, 1, 1) };
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] }];
m1.Actors = [actors[0]];
m2.Actors = [actors[0], actors[1]];
m3.Actors = [actors[0], actors[1], actors[2]];
data.Actors.AddRange(actors);
return actors;
}

private static TestDbContext CreateActorWithMovies(TestDbContextFactory factory)
{
var data = factory.CreateContext();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,52 @@ private class ProjectWrapper
public Project? Project { get; set; }
}

// paging on the node of another connection. The inner paging rebuilds its collection from the context its field is
// compiled against - the edge being selected - not the schema's p_ConnectionEdge.Node ("unbound variable" before)
[Fact]
public void TestPagingInConnectionNode()
{
var schema = SchemaBuilder.FromObject<TestDataContext>();
var data = new TestDataContext();
FillProjectData(data);

schema.Query().ReplaceField("projects", ctx => ctx.Projects.OrderBy(p => p.Id), "Projects").UseConnectionPaging();
schema.Type<Project>().ReplaceField("tasks", p => p.Tasks.OrderBy(t => t.Id), "Tasks").UseConnectionPaging();
schema.Type<Project>().AddField("pagedTasks", p => p.Tasks.OrderBy(t => t.Id), "Tasks").UseOffsetPaging();

var result = schema.ExecuteRequestWithContext(
new QueryRequest
{
Query =
$@"{{
projects(first: 1) {{
edges {{ cursor node {{
name
tasks(first: 2, after: ""{ConnectionHelper.SerializeCursor(1)}"") {{ totalCount edges {{ cursor node {{ name }} }} }}
pagedTasks(skip: 3, take: 5) {{ totalItems hasNextPage items {{ name }} }}
}} }}
}}
}}",
},
data,
null,
null
);
Assert.Null(result.Errors);

dynamic project = ((dynamic)result.Data!["projects"]!).edges[0];
Assert.Equal(ConnectionHelper.SerializeCursor(1), project.cursor);
Assert.Equal("Project 1", project.node.name);
dynamic tasks = project.node.tasks;
Assert.Equal(5, tasks.totalCount);
Assert.Equal(new[] { "Task 2", "Task 3" }, ((IEnumerable<dynamic>)tasks.edges).Select(e => (string)e.node.name));
Assert.Equal(new[] { ConnectionHelper.SerializeCursor(2), ConnectionHelper.SerializeCursor(3) }, ((IEnumerable<dynamic>)tasks.edges).Select(e => (string)e.cursor));
dynamic pagedTasks = project.node.pagedTasks;
Assert.Equal(5, pagedTasks.totalItems);
Assert.False(pagedTasks.hasNextPage);
Assert.Equal(new[] { "Task 4", "Task 5" }, ((IEnumerable<dynamic>)pagedTasks.items).Select(i => (string)i.name));
}

private static void FillProjectData(TestDataContext data)
{
data.Projects =
Expand Down