Throw on EF anti-patterns in tests and fix the ones found

src/Directory.Packages.props

@@ -29,6 +29,7 @@
     <PackageVersion Include="Sbom" Version="0.3.1" />
     <PackageVersion Include="System.Text.Encodings.Web" Version="10.0.12" />
     <PackageVersion Include="Verify.DiffPlex" Version="3.3.1" />
+    <PackageVersion Include="Verify.EntityFramework" Version="16.0.0-beta.4" />
     <PackageVersion Include="Verify.GraphQL" Version="0.1.1" />
     <PackageVersion Include="Verify.SqlServer" Version="12.2.0" />
     <PackageVersion Include="Verify.XunitV3" Version="33.1.5" />

src/GraphQL.EntityFramework/ConnectionConverter.cs

@@ -22,6 +22,68 @@ internal static bool HasOrderingInExpressionTree(Expression expression)
         return false;
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    internal static IQueryable<T> WithoutOrdering<T>(IQueryable<T> queryable) =>
+        Strip(queryable, IsOrdering);
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    static IQueryable<T> ForCount<T>(IQueryable<T> 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<T> Strip<T>(IQueryable<T> queryable, Func<string, bool> isStripped)
+    {
+        var expression = Strip(queryable.Expression, isStripped);
+        if (expression == queryable.Expression)
+        {
+            return queryable;
+        }
+
+        return queryable.Provider.CreateQuery<T>(expression);
+    }
+
+    static Expression Strip(Expression expression, Func<string, bool> 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<T> ApplyConnectionContext<T>(List<T> list, int? first, string? afterString, int? last, string? beforeString)
         where T : class
     {
@@ -170,7 +232,7 @@ static async Task<Page<TItem>> LoadPage<TSource, TItem>(
         List<TItem> 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);

src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs

@@ -65,6 +65,9 @@ public static IQueryable<TItem> ApplyGraphQlArguments<TItem>(
             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<TItem>.GetProperty(orderBy.Path)
             .Lambda;

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<string> list = ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j"];
 

src/Tests/DependencyResolutionTests/DependencyTests.cs

@@ -3,7 +3,7 @@
     static SqlInstance<DependencyDbContext> sqlInstance;
 
     static DependencyTests() =>
-        sqlInstance = new(builder => new(builder.Options));
+        sqlInstance = new(builder => new(builder.ThrowOnAntiPatterns().Options));
 
     static string query = """
         {

src/Tests/IntegrationTests/IntegrationTests.Connection_without_first_or_last_returns_everything.verified.txt

@@ -37,19 +37,13 @@
       Text:
 select COUNT(*)
 from   ChildEntities as c
-       left outer join
-       ParentEntities as p
-       on c.ParentId = p.Id
     },
     {
       Text:
 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: {

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
+  }
+}

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!]!

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()
     {

src/Tests/IntegrationTests/Query.cs

@@ -52,6 +52,10 @@ public Query(IEfGraphQLService<IntegrationDbContext> efGraphQlService)
             name: "parentEntities",
             resolve: _ => _.DbContext.ParentEntities);
 
+        AddQueryField(
+            name: "orderedParentEntities",
+            resolve: _ => _.DbContext.ParentEntities.OrderByDescending(_ => _.Property));
+
         AddQueryField(
             name: "childEntities",
             resolve: _ => _.DbContext.ChildEntities);

src/Tests/Mapping/MappingTests.cs

@@ -15,6 +15,7 @@ static MappingTests()
                     CoreEventId.ShadowForeignKeyPropertyCreated,
                     CoreEventId.RowLimitingOperationWithoutOrderByWarning,
                     CoreEventId.CollectionWithoutComparer));
+            builder.ThrowOnAntiPatterns();
             return new(builder.Options);
         });
     }

src/Tests/MultiContextTests/MultiContextTests.cs

@@ -3,9 +3,9 @@
     [Fact]
     public async Task Run()
     {
-        var sqlInstance1 = new SqlInstance<DbContext1>(constructInstance: builder => new(builder.Options));
+        var sqlInstance1 = new SqlInstance<DbContext1>(constructInstance: builder => new(builder.ThrowOnAntiPatterns().Options));
 
-        var sqlInstance2 = new SqlInstance<DbContext2>(constructInstance: builder => new(builder.Options));
+        var sqlInstance2 = new SqlInstance<DbContext2>(constructInstance: builder => new(builder.ThrowOnAntiPatterns().Options));
 
         var query =
             """

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()

src/Tests/Tests.csproj

@@ -13,6 +13,7 @@
     <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" />
     <PackageReference Include="Verify.DiffPlex" />
     <PackageReference Include="EfOrderBy" />
+    <PackageReference Include="Verify.EntityFramework" />
     <PackageReference Include="Verify.GraphQL" />
     <PackageReference Include="Verify.SqlServer" />
     <PackageReference Include="xunit.v3" />