diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 7b5e12ce3..14ceb9605 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -29,6 +29,7 @@ + diff --git a/src/GraphQL.EntityFramework/ConnectionConverter.cs b/src/GraphQL.EntityFramework/ConnectionConverter.cs index 2bbc5d043..9c9c1e16c 100644 --- a/src/GraphQL.EntityFramework/ConnectionConverter.cs +++ b/src/GraphQL.EntityFramework/ConnectionConverter.cs @@ -22,6 +22,68 @@ internal static bool HasOrderingInExpressionTree(Expression expression) return false; } + /// + /// The query with its ordering removed, for an ordering that replaces it. Only the query's + /// own chain is stripped, and only above any Skip or Take, since below one the ordering + /// decides which rows are included. + /// + internal static IQueryable WithoutOrdering(IQueryable queryable) => + Strip(queryable, IsOrdering); + + /// + /// The query to count, without the operators that only shape the entities returned, which a + /// count does not return: ordering, includes, split queries, and tracking. + /// + static IQueryable ForCount(IQueryable queryable) => + Strip(queryable, _ => IsOrdering(_) || _ is + "Include" or "ThenInclude" or + "AsSplitQuery" or "AsSingleQuery" or + "AsNoTracking" or "AsNoTrackingWithIdentityResolution" or "AsTracking"); + + static bool IsOrdering(string methodName) => + methodName is "OrderBy" or "OrderByDescending" or "ThenBy" or "ThenByDescending"; + + static IQueryable Strip(IQueryable queryable, Func isStripped) + { + var expression = Strip(queryable.Expression, isStripped); + if (expression == queryable.Expression) + { + return queryable; + } + + return queryable.Provider.CreateQuery(expression); + } + + static Expression Strip(Expression expression, Func isStripped) + { + // Queryable operators, and extensions such as Include and AsNoTracking, take the query + // they build on as their first argument + if (expression is not MethodCallExpression { Object: null, Arguments.Count: > 0 } methodCall || + !typeof(IQueryable).IsAssignableFrom(methodCall.Arguments[0].Type)) + { + return expression; + } + + var methodName = methodCall.Method.Name; + if (isStripped(methodName)) + { + return Strip(methodCall.Arguments[0], isStripped); + } + + if (methodName is "Skip" or "Take" or "SkipLast" or "TakeLast" or "SkipWhile" or "TakeWhile") + { + return expression; + } + + var source = Strip(methodCall.Arguments[0], isStripped); + if (source == methodCall.Arguments[0]) + { + return expression; + } + + return methodCall.Update(methodCall.Object, [source, .. methodCall.Arguments.Skip(1)]); + } + public static Connection ApplyConnectionContext(List list, int? first, string? afterString, int? last, string? beforeString) where T : class { @@ -170,7 +232,7 @@ static async Task> LoadPage( List rows; if (NeedsCount(context, last, before)) { - count = await queryable.CountAsync(cancel); + count = await ForCount(queryable).CountAsync(cancel); cancel.ThrowIfCancellationRequested(); int take; (skip, take) = Window(first, after, last, before, count.Value); diff --git a/src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs b/src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs index 029e0328b..512cfe81d 100644 --- a/src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs +++ b/src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs @@ -65,6 +65,9 @@ public static IQueryable ApplyGraphQlArguments( return (queryable, false); } + // The argument replaces any ordering the resolver applied, which EF would discard anyway + queryable = ConnectionConverter.WithoutOrdering(queryable); + var orderBy = orderBys.First(); var property = PropertyCache.GetProperty(orderBy.Path) .Lambda; diff --git a/src/Tests/ConnectionConverter/ConnectionConverterTests.cs b/src/Tests/ConnectionConverter/ConnectionConverterTests.cs index 0449010bb..53a022aac 100644 --- a/src/Tests/ConnectionConverter/ConnectionConverterTests.cs +++ b/src/Tests/ConnectionConverter/ConnectionConverterTests.cs @@ -8,7 +8,7 @@ static ConnectionConverterTests() => dbContext.AddRange(list.Select(_ => new Entity {Property = _})); await dbContext.SaveChangesAsync(); }, - constructInstance: builder => new(builder.Options)); + constructInstance: builder => new(builder.ThrowOnAntiPatterns().Options)); static List list = ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j"]; diff --git a/src/Tests/DependencyResolutionTests/DependencyTests.cs b/src/Tests/DependencyResolutionTests/DependencyTests.cs index 567b77f34..b78686795 100644 --- a/src/Tests/DependencyResolutionTests/DependencyTests.cs +++ b/src/Tests/DependencyResolutionTests/DependencyTests.cs @@ -3,7 +3,7 @@ static SqlInstance sqlInstance; static DependencyTests() => - sqlInstance = new(builder => new(builder.Options)); + sqlInstance = new(builder => new(builder.ThrowOnAntiPatterns().Options)); static string query = """ { diff --git a/src/Tests/IntegrationTests/IntegrationTests.Connection_without_first_or_last_returns_everything.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Connection_without_first_or_last_returns_everything.verified.txt index f9e464487..b597c3a1a 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Connection_without_first_or_last_returns_everything.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Connection_without_first_or_last_returns_everything.verified.txt @@ -37,9 +37,6 @@ Text: select COUNT(*) from ChildEntities as c - left outer join - ParentEntities as p - on c.ParentId = p.Id }, { Text: @@ -47,9 +44,6 @@ select c.Id, c.ParentId, c.Property from ChildEntities as c - left outer join - ParentEntities as p - on c.ParentId = p.Id order by c.Property offset @p rows fetch next @p1 rows only, Parameters: { diff --git a/src/Tests/IntegrationTests/IntegrationTests.OrderByArgumentReplacesResolverOrdering.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.OrderByArgumentReplacesResolverOrdering.verified.txt new file mode 100644 index 000000000..b148e0c68 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.OrderByArgumentReplacesResolverOrdering.verified.txt @@ -0,0 +1,21 @@ +{ + target: { + Data: { + orderedParentEntities: [ + { + property: Value1 + }, + { + property: Value2 + } + ] + } + }, + sql: { + Text: +select p.Id, + p.Property +from ParentEntities as p +order by p.Property + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.SchemaPrint.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.SchemaPrint.verified.txt index a7094a822..1fe538585 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.SchemaPrint.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.SchemaPrint.verified.txt @@ -9,6 +9,7 @@ namedEntities(id: ID, ids: [ID!], where: NamedIdEntityWhere, orderBy: [NamedIdEntityOrderBy!], skip: Int, take: Int): [NamedId!]! misNamed(id: ID, ids: [ID!], where: WithMisNamedQueryParentEntityWhere, orderBy: [WithMisNamedQueryParentEntityOrderBy!], skip: Int, take: Int): [WithMisNamedQueryParent!]! parentEntities(id: ID, ids: [ID!], where: ParentEntityWhere, orderBy: [ParentEntityOrderBy!], skip: Int, take: Int): [Parent!]! + orderedParentEntities(id: ID, ids: [ID!], where: ParentEntityWhere, orderBy: [ParentEntityOrderBy!], skip: Int, take: Int): [Parent!]! childEntities(id: ID, ids: [ID!], where: ChildEntityWhere, orderBy: [ChildEntityOrderBy!], skip: Int, take: Int): [Child!]! dateEntities(id: ID, ids: [ID!], where: DateEntityWhere, orderBy: [DateEntityOrderBy!], skip: Int, take: Int): [DateEntity!]! enumEntities(id: ID, ids: [ID!], where: EnumEntityWhere, orderBy: [EnumEntityOrderBy!], skip: Int, take: Int): [EnumEntity!]! diff --git a/src/Tests/IntegrationTests/IntegrationTests.cs b/src/Tests/IntegrationTests/IntegrationTests.cs index c95fc7da1..971505348 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.cs +++ b/src/Tests/IntegrationTests/IntegrationTests.cs @@ -21,6 +21,7 @@ from ParentEntities _.Ignore( CoreEventId.ShadowForeignKeyPropertyCreated, CoreEventId.CollectionWithoutComparer)); + builder.ThrowOnAntiPatterns(); return new(builder.Options); }); @@ -546,6 +547,34 @@ public async Task Take() await RunQuery(database, query, null, null, false, [entity1, entity2]); } + // The orderBy argument replaces the ordering the resolver applied, rather than being added + // after it, where EF discards the resolver's ordering + [Fact] + public async Task OrderByArgumentReplacesResolverOrdering() + { + var query = + """ + { + orderedParentEntities (orderBy: {property: ascending}) + { + property + } + } + """; + + var entity1 = new ParentEntity + { + Property = "Value1" + }; + var entity2 = new ParentEntity + { + Property = "Value2" + }; + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, null, false, [entity1, entity2]); + } + [Fact] public async Task TakeNoOrder() { diff --git a/src/Tests/IntegrationTests/Query.cs b/src/Tests/IntegrationTests/Query.cs index b9bf98dc3..3cd8c3245 100644 --- a/src/Tests/IntegrationTests/Query.cs +++ b/src/Tests/IntegrationTests/Query.cs @@ -52,6 +52,10 @@ public Query(IEfGraphQLService efGraphQlService) name: "parentEntities", resolve: _ => _.DbContext.ParentEntities); + AddQueryField( + name: "orderedParentEntities", + resolve: _ => _.DbContext.ParentEntities.OrderByDescending(_ => _.Property)); + AddQueryField( name: "childEntities", resolve: _ => _.DbContext.ChildEntities); diff --git a/src/Tests/Mapping/MappingTests.cs b/src/Tests/Mapping/MappingTests.cs index 8ffe61394..c5376e610 100644 --- a/src/Tests/Mapping/MappingTests.cs +++ b/src/Tests/Mapping/MappingTests.cs @@ -15,6 +15,7 @@ static MappingTests() CoreEventId.ShadowForeignKeyPropertyCreated, CoreEventId.RowLimitingOperationWithoutOrderByWarning, CoreEventId.CollectionWithoutComparer)); + builder.ThrowOnAntiPatterns(); return new(builder.Options); }); } diff --git a/src/Tests/MultiContextTests/MultiContextTests.cs b/src/Tests/MultiContextTests/MultiContextTests.cs index c5a6dff50..53bffc4b6 100644 --- a/src/Tests/MultiContextTests/MultiContextTests.cs +++ b/src/Tests/MultiContextTests/MultiContextTests.cs @@ -3,9 +3,9 @@ [Fact] public async Task Run() { - var sqlInstance1 = new SqlInstance(constructInstance: builder => new(builder.Options)); + var sqlInstance1 = new SqlInstance(constructInstance: builder => new(builder.ThrowOnAntiPatterns().Options)); - var sqlInstance2 = new SqlInstance(constructInstance: builder => new(builder.Options)); + var sqlInstance2 = new SqlInstance(constructInstance: builder => new(builder.ThrowOnAntiPatterns().Options)); var query = """ diff --git a/src/Tests/SelfReferencingWhereTests.cs b/src/Tests/SelfReferencingWhereTests.cs index f9394b371..31091ff40 100644 --- a/src/Tests/SelfReferencingWhereTests.cs +++ b/src/Tests/SelfReferencingWhereTests.cs @@ -29,7 +29,7 @@ public class SelfReferencingWhereTests data.AddRange(root, other); await data.SaveChangesAsync(); }, - constructInstance: builder => new(builder.Options)); + constructInstance: builder => new(builder.ThrowOnAntiPatterns().Options)); [Fact] public async Task List_path_into_same_type() diff --git a/src/Tests/Tests.csproj b/src/Tests/Tests.csproj index d2705fc4a..00d9c4578 100644 --- a/src/Tests/Tests.csproj +++ b/src/Tests/Tests.csproj @@ -13,6 +13,7 @@ +