Project abstract types instead of falling back to includes

src/Directory.Build.props

@@ -2,7 +2,7 @@
 <Project>
   <PropertyGroup>
     <NoWarn>CS1591;NU5104;CS1573;CS9107;NU1608;NU1109</NoWarn>
-    <Version>35.4.0</Version>
+    <Version>35.5.0</Version>
     <LangVersion>preview</LangVersion>
     <AssemblyVersion>1.0.0</AssemblyVersion>
     <PackageTags>EntityFrameworkCore, EntityFramework, GraphQL</PackageTags>

src/GraphQL.EntityFramework/ConnectionConverter.cs

@@ -32,14 +32,33 @@ internal static IQueryable<T> WithoutOrdering<T>(IQueryable<T> queryable) =>
 
     /// <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.
+    /// count does not return: the select projection, ordering, includes, split queries, and
+    /// tracking.
     /// </summary>
     static IQueryable<T> ForCount<T>(IQueryable<T> queryable) =>
-        Strip(queryable, _ => IsOrdering(_) || _ is
+        Strip(WithoutProjection(queryable), _ => IsOrdering(_) || _ is
             "Include" or "ThenInclude" or
             "AsSplitQuery" or "AsSingleQuery" or
             "AsNoTracking" or "AsNoTrackingWithIdentityResolution" or "AsTracking");
 
+    /// <summary>
+    /// The select projection is the last operator applied, and creates the same type it reads, so
+    /// there are as many rows without it. EF drops it from the count it translates, but only after
+    /// the whole projection, and every collection in it, has been compiled as part of the query.
+    /// A select that changes the type, or one further down, belongs to the resolver and is kept.
+    /// </summary>
+    static IQueryable<T> WithoutProjection<T>(IQueryable<T> queryable)
+    {
+        if (queryable.Expression is MethodCallExpression { Method.Name: "Select", Object: null, Arguments.Count: 2 } select &&
+            select.Method.DeclaringType == typeof(Queryable) &&
+            typeof(IQueryable<T>).IsAssignableFrom(select.Arguments[0].Type))
+        {
+            return queryable.Provider.CreateQuery<T>(select.Arguments[0]);
+        }
+
+        return queryable;
+    }
+
     static bool IsOrdering(string methodName) =>
         methodName is "OrderBy" or "OrderByDescending" or "ThenBy" or "ThenByDescending";
 

src/GraphQL.EntityFramework/ForeignKeyExtractor.cs

@@ -30,6 +30,19 @@ static IReadOnlySet<string> GetForeignKeys(IEntityType entity)
             }
         }
 
+        // A query for this type also returns its derived types, each projected as itself, so the
+        // foreign keys they declare are loaded too. A name is only bound on the types that have it.
+        foreach (var derived in entity.GetDerivedTypes())
+        {
+            foreach (var foreignKey in derived.GetDeclaredForeignKeys())
+            {
+                foreach (var property in foreignKey.Properties)
+                {
+                    foreignKeyNames.Add(property.Name);
+                }
+            }
+        }
+
         // Include TPH discriminator property so projected entities maintain correct type identity.
         // Without this, projected entities get the default discriminator value (e.g. enum value 0)
         // instead of the actual value, causing downstream code that switches on the discriminator to fail.

src/GraphQL.EntityFramework/SelectProjection/HoistedNavigation.cs

@@ -0,0 +1,64 @@
+/// <summary>
+/// Sets a navigation on an entity the projection has already created. A type with derived types is
+/// projected as a chain of type tests, each creating its own type, and a navigation that loads a
+/// collection is bound once to the result of that chain rather than inside every branch of it.
+/// There is no expression for assigning a property of an existing instance that EF accepts in a
+/// projection, so it is a call EF evaluates on the client, as it does the member inits.
+/// </summary>
+static class HoistedNavigation
+{
+    static MethodInfo setMethod = typeof(HoistedNavigation)
+        .GetMethod(nameof(Set), BindingFlags.Static | BindingFlags.NonPublic)!;
+
+    static ConcurrentDictionary<PropertyInfo, MethodInfo> setMethods = new();
+
+    public static Expression Assign(Expression entity, Type entityType, PropertyInfo property, Expression value)
+    {
+        var method = setMethods.GetOrAdd(
+            property,
+            _ => setMethod.MakeGenericMethod(_.ReflectedType!, _.PropertyType));
+
+        if (entity.Type != entityType)
+        {
+            entity = Expression.Convert(entity, entityType);
+        }
+
+        if (value.Type != property.PropertyType)
+        {
+            value = Expression.Convert(value, property.PropertyType);
+        }
+
+        // The name is a constant, so the expression, and with it EF's compiled query cache entry,
+        // depends only on which navigations were requested
+        return Expression.Call(null, method, entity, value, Expression.Constant(property.Name));
+    }
+
+    static TEntity? Set<TEntity, TValue>(TEntity? entity, TValue value, string name)
+        where TEntity : class
+    {
+        if (entity is not null)
+        {
+            Setter<TEntity, TValue>.Get(name)(entity, value);
+        }
+
+        return entity;
+    }
+
+    static class Setter<TEntity, TValue>
+        where TEntity : class
+    {
+        static ConcurrentDictionary<string, Action<TEntity, TValue>> setters = new();
+
+        public static Action<TEntity, TValue> Get(string name) =>
+            setters.GetOrAdd(
+                name,
+                _ =>
+                {
+                    var property = typeof(TEntity).GetProperty(_, BindingFlags.Instance | BindingFlags.Public)!;
+                    var entity = Expression.Parameter(typeof(TEntity));
+                    var value = Expression.Parameter(typeof(TValue));
+                    var assign = Expression.Assign(Expression.Property(entity, property), value);
+                    return Expression.Lambda<Action<TEntity, TValue>>(assign, entity, value).Compile();
+                });
+    }
+}

src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs

@@ -63,7 +63,7 @@ record PropertyMetadata(Type EntityType, PropertyInfo Property, bool CanWrite, b
     record EntityTypeMetadata(
         ParameterExpression Parameter,
         IReadOnlyDictionary<string, PropertyMetadata> Properties,
-        NewExpression NewInstance,
+        NewExpression? NewInstance,
         MethodInfo SelectMethod,
         MethodInfo ToListMethod,
         ConstantExpression NullConstant);
@@ -122,11 +122,6 @@ public static bool TryBuild<TEntity>(
         argumentFields = state.ArgumentFields;
         var entityType = typeof(TEntity);
 
-        if (entityType.IsAbstract)
-        {
-            return false;
-        }
-
         var parameter = GetEntityMetadata(entityType).Parameter;
         if (!TryBuildEntityInit(parameter, entityType, projection, state, null, out var body))
         {
@@ -143,6 +138,13 @@ public static bool TryBuild<TEntity>(
     /// matching type, most derived first. False when the entity, or one of the navigations under
     /// it that has no writable property to fall back to, cannot be projected.
     /// </summary>
+    /// <remarks>
+    /// An abstract type cannot be created by a member init, but no row is ever of that type
+    /// itself: each is one of its concrete derived types, and the chain of type tests creates
+    /// those. So the chain ends in null where a concrete type's ends in its own member init. An
+    /// abstract type used to be loaded whole through includes, every column of it and of every
+    /// entity under it, whatever was selected.
+    /// </remarks>
     static bool TryBuildEntityInit(
         Expression source,
         Type entityType,
@@ -152,15 +154,37 @@ static bool TryBuildEntityInit(
         [NotNullWhen(true)] out Expression? expression)
     {
         expression = null;
+        state.DerivedTypes.TryGetValue(entityType, out var derived);
 
-        if (!TryBuildMemberInit(source, entityType, projection, state, path, out var memberInit))
+        List<(PropertyMetadata Metadata, NavigationProjectionInfo Navigation)>? hoisted = null;
+        if (derived is not null)
         {
-            return false;
+            projection = Hoist(entityType, projection, out hoisted);
+        }
+
+        Expression result;
+        if (entityType.IsAbstract)
+        {
+            if (!HasConcrete(derived))
+            {
+                return false;
+            }
+
+            result = GetEntityMetadata(entityType).NullConstant;
+        }
+        else
+        {
+            if (!TryBuildMemberInit(source, entityType, projection, state, path, out var memberInit))
+            {
+                return false;
+            }
+
+            result = memberInit;
         }
 
-        expression = memberInit;
-        if (!state.DerivedTypes.TryGetValue(entityType, out var derived))
+        if (derived is null)
         {
+            expression = result;
             return true;
         }
 
@@ -179,15 +203,90 @@ static bool TryBuildEntityInit(
                 return false;
             }
 
-            expression = Expression.Condition(
+            result = Expression.Condition(
                 Expression.TypeIs(source, derivedType),
                 Expression.Convert(derivedInit, entityType),
-                expression);
+                result);
         }
 
+        if (hoisted is not null)
+        {
+            foreach (var (metadata, navigation) in hoisted)
+            {
+                var binding = BuildNavigationBinding(source, metadata, navigation, state, path);
+                if (binding is not null)
+                {
+                    result = HoistedNavigation.Assign(result, entityType, metadata.Property, binding.Expression);
+                }
+            }
+        }
+
+        expression = result;
         return true;
     }
 
+    /// <summary>
+    /// Takes the navigations out of the projection of a type with derived types. Each type test
+    /// in the chain creates its own member init, so a navigation bound in them was repeated once
+    /// per concrete type. EF does not recognise a repeated collection subquery as the same one,
+    /// so it joined, or split out, the collection once for every type. A reference navigation is
+    /// joined once regardless, but everything under it was repeated in the expression, and a
+    /// hierarchy under a hierarchy multiplied. These are instead bound once, to whichever type
+    /// the chain created.
+    /// </summary>
+    static FieldProjectionInfo Hoist(
+        Type entityType,
+        FieldProjectionInfo projection,
+        out List<(PropertyMetadata Metadata, NavigationProjectionInfo Navigation)>? hoisted)
+    {
+        hoisted = null;
+        if (projection.Navigations is null)
+        {
+            return projection;
+        }
+
+        var properties = GetEntityMetadata(entityType).Properties;
+        Dictionary<string, NavigationProjectionInfo>? kept = null;
+        foreach (var (name, navigation) in projection.Navigations)
+        {
+            if (!properties.TryGetValue(name, out var metadata) ||
+                !metadata.CanWrite)
+            {
+                continue;
+            }
+
+            hoisted ??= [];
+            hoisted.Add((metadata, navigation));
+            kept ??= new(projection.Navigations);
+            kept.Remove(name);
+        }
+
+        if (kept is null)
+        {
+            return projection;
+        }
+
+        return projection with { Navigations = kept };
+    }
+
+    static bool HasConcrete(IReadOnlyList<Type>? types)
+    {
+        if (types is null)
+        {
+            return false;
+        }
+
+        foreach (var type in types)
+        {
+            if (!type.IsAbstract)
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     /// <summary>
     /// The fields selected through fragments on a derived type are recorded against the base
     /// projection: scalars by name, which resolve against the derived type's properties, and
@@ -218,7 +317,7 @@ static bool TryBuildMemberInit(
             return false;
         }
 
-        memberInit = Expression.MemberInit(GetEntityMetadata(entityType).NewInstance, bindings);
+        memberInit = Expression.MemberInit(GetEntityMetadata(entityType).NewInstance!, bindings);
         return true;
     }
 
@@ -300,22 +399,11 @@ static bool TryBuildBindings(
                     continue;
                 }
 
-                var navAccess = Expression.Property(source, metadata.Property);
-                var navPath = path is null ? metadata.Property.Name : $"{path}.{metadata.Property.Name}";
-                if (!TryBuildNavigationBinding(navAccess, navProjection, state, navPath, out var binding))
+                var binding = BuildNavigationBinding(source, metadata, navProjection, state, path);
+                if (binding is not null)
                 {
-                    if (!metadata.CanWrite)
-                    {
-                        continue;
-                    }
-
-                    // Can't project navigation (e.g. read-only properties on target entity)
-                    // Fall back to including the full navigation entity
-                    binding = BuildFullNavigationBinding(navAccess, navProjection, state);
-                    state.AddIncludePaths(navPath, navProjection.Projection);
+                    bindings.Add(binding);
                 }
-
-                bindings.Add(binding);
             }
         }
 
@@ -324,6 +412,35 @@ static bool TryBuildBindings(
         return true;
     }
 
+    /// <summary>
+    /// The binding of a navigation: projected where it can be, otherwise bound whole, with what
+    /// was requested under it loaded through includes. Null when it can be neither.
+    /// </summary>
+    static MemberAssignment? BuildNavigationBinding(
+        Expression source,
+        PropertyMetadata metadata,
+        NavigationProjectionInfo navProjection,
+        BuildState state,
+        string? path)
+    {
+        var navAccess = Expression.Property(source, metadata.Property);
+        var navPath = path is null ? metadata.Property.Name : $"{path}.{metadata.Property.Name}";
+        if (TryBuildNavigationBinding(navAccess, navProjection, state, navPath, out var binding))
+        {
+            return binding;
+        }
+
+        if (!metadata.CanWrite)
+        {
+            return null;
+        }
+
+        // Can't project navigation (e.g. read-only properties on target entity)
+        // Fall back to including the full navigation entity
+        state.AddIncludePaths(navPath, navProjection.Projection);
+        return BuildFullNavigationBinding(navAccess, navProjection, state);
+    }
+
     static MemberAssignment Bind(Expression source, PropertyMetadata metadata) =>
         Expression.Bind(metadata.Property, Expression.Property(source, metadata.Property));
 
@@ -346,9 +463,9 @@ static bool TryBuildNavigationBinding(
         binding = null;
         var navType = navProjection.EntityType;
 
-        // An abstract type cannot be created by a member init, and a navigation wanted whole has
-        // no field list to build one from. Both fall back to binding the navigation itself.
-        if (navType.IsAbstract || navProjection.IsWhole)
+        // A navigation wanted whole has no field list to build a member init from, so it falls
+        // back to binding the navigation itself.
+        if (navProjection.IsWhole)
         {
             return false;
         }
@@ -554,7 +671,8 @@ static EntityTypeMetadata GetEntityMetadata(Type type) =>
                 dictionary[property.Name] = new(type, property, canWrite, isAutoProperty);
             }
 
-            var newInstance = Expression.New(type);
+            // Never created, so never asked for: see TryBuildEntityInit
+            var newInstance = type.IsAbstract ? null : Expression.New(type);
             var selectMethod = SelectExpressionBuilder.selectMethod.MakeGenericMethod(type, type);
             var toListMethod = SelectExpressionBuilder.toListMethod.MakeGenericMethod(type);
             var nullConstant = Expression.Constant(null, type);