diff --git a/docs/analyzers/GQLEF007.md b/docs/analyzers/GQLEF007.md new file mode 100644 index 00000000..1b96314d --- /dev/null +++ b/docs/analyzers/GQLEF007.md @@ -0,0 +1,143 @@ +# GQLEF007: Identity projection with abstract navigation access + +## Cause + +A filter uses identity projection (`_ => _` or 4-parameter syntax) but the filter lambda accesses properties through an abstract navigation type. + +## Problem + +Abstract types cannot be instantiated in SQL projections. When Entity Framework Core encounters a projection that would require instantiating an abstract class, it cannot generate the SQL expression. Instead, EF Core falls back to using `Include()` which loads **all columns** from the related table. + +This creates a significant performance problem: +- The filter only needs 1-2 fields from the navigation +- But EF Core loads all 30+ fields because of the Include fallback +- Query performance degrades and memory usage increases unnecessarily + +## Rule Details + +This diagnostic is reported when: +1. A filter uses identity projection (explicit `_ => _` or implicit via 4-parameter syntax) +2. The filter lambda accesses a property through a navigation property +3. That navigation property's type is abstract (has the `abstract` keyword) + +### Examples of violations + +```csharp +// ❌ Identity projection with abstract navigation access +public abstract class BaseEntity +{ + public string Property { get; set; } +} + +public class Child +{ + public Guid Id { get; set; } + public BaseEntity Parent { get; set; } +} + +// This triggers GQLEF007: +filters.For().Add( + projection: _ => _, // Identity projection + filter: (_, _, _, c) => c.Parent.Property == "test"); + +// This also triggers GQLEF007 (4-parameter syntax uses identity projection): +filters.For().Add( + filter: (_, _, _, c) => c.Parent.Property == "test"); +``` + +### Examples of correct code + +```csharp +// ✅ Explicit projection extracting required properties +filters.For().Add( + projection: c => new { c.Id, ParentProperty = c.Parent.Property }, + filter: (_, _, _, proj) => proj.ParentProperty == "test"); + +// ✅ Concrete (non-abstract) navigation is fine with identity projection +public class ConcreteParent // Not abstract +{ + public string Property { get; set; } +} + +public class Child +{ + public ConcreteParent Parent { get; set; } +} + +filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.Parent.Property == "test"); // OK - ConcreteParent is not abstract +``` + +## Solution + +Convert the identity projection to an explicit projection that extracts only the required properties from the abstract navigation. + +### Manual Fix + +1. Identify all properties accessed through the abstract navigation +2. Create an explicit projection that extracts those properties with flattened names +3. Update the filter to use the projected property names +4. Rename the filter parameter to `proj` for clarity + +**Before:** +```csharp +filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.Parent.Property == "test"); +``` + +**After:** +```csharp +filters.For().Add( + projection: c => new { c.Id, ParentProperty = c.Parent.Property }, + filter: (_, _, _, proj) => proj.ParentProperty == "test"); +``` + +### Automatic Fix + +The code fixer can automatically perform this transformation: +1. Place cursor on the diagnostic +2. Press `Ctrl+.` (or `Cmd+.` on Mac) +3. Select "Convert to explicit projection" + +The fixer will: +- Extract all accessed navigation properties +- Create an anonymous type projection with flattened property names +- Update the filter lambda to use the new property names +- Rename the filter parameter from entity name to `proj` + +## Performance Impact + +Using explicit projections instead of identity projections with abstract navigations can significantly improve performance: + +**Identity Projection (Inefficient):** +```sql +-- Loads all 34 columns from BaseRequest table +SELECT a.*, br.* +FROM Accommodation a +LEFT JOIN BaseRequest br ON a.TravelRequestId = br.Id +``` + +**Explicit Projection (Efficient):** +```sql +-- Loads only required columns +SELECT a.Id, br.GroupOwnerId, br.HighestStatusAchieved +FROM Accommodation a +LEFT JOIN BaseRequest br ON a.TravelRequestId = br.Id +``` + +## When to Suppress + +**Never.** This diagnostic indicates a real performance issue that should be fixed. There is no valid scenario where loading all columns from an abstract navigation is preferred over loading only the required columns. + +## Related Rules + +- **GQLEF004** - Suggests using 4-parameter filter syntax for identity projections with key-only access +- **GQLEF005** - Prevents accessing non-key properties with 4-parameter filter syntax +- **GQLEF006** - Prevents accessing non-key properties with explicit identity projection + +## See Also + +- [Filter Projections Documentation](/docs/filters.md#abstract-type-navigations) +- [Understanding EF Core Projections](https://learn.microsoft.com/en-us/ef/core/querying/how-query-works) diff --git a/docs/filters.md b/docs/filters.md index da4579eb..cc0306ec 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -258,7 +258,7 @@ EfGraphQLConventions.RegisterInContainer( Common nullable patterns: * **Has value check**: `quantity.HasValue && quantity.Value > 0` -* **Null check**: `!quantity.HasValue` +* **Null check**: `?quantity.HasValue` * **Exact match**: `isApproved == true` (not null or false) @@ -522,12 +522,12 @@ The simplified API is syntactic sugar for the identity projection pattern: ```csharp // Simplified API filters.For().Add( - filter: (_, _, _, a) => a.Id != Guid.Empty); + filter: (_, _, _, a) => a.Id ?= Guid.Empty); // Equivalent full API filters.For().Add( projection: _ => _, // Identity projection - filter: (_, _, _, a) => a.Id != Guid.Empty); + filter: (_, _, _, a) => a.Id ?= Guid.Empty); ``` ### Analyzer Support @@ -556,3 +556,96 @@ filters.For().Add( ``` The simplified API makes intent clearer and reduces boilerplate while maintaining the same runtime behavior + +## Abstract Type Navigations + +When working with filters that access properties through abstract navigation properties, special care must be taken to avoid performance issues. + +### The Problem + +Abstract types cannot be instantiated in SQL projections. When EF Core encounters an abstract navigation in a projection, it falls back to using `Include()` which loads **all columns** from the navigation table, even when only one or two fields are required. + +### Example + +```csharp +// Given: +public abstract class BaseRequest // Abstract class with many fields +{ + public Guid Id { get; set; } + public Guid GroupOwnerId { get; set; } + public RequestStatus HighestStatusAchieved { get; set; } + // ... 30+ more columns ... +} + +public class Accommodation +{ + public Guid Id { get; set; } + public Guid TravelRequestId { get; set; } + public BaseRequest? TravelRequest { get; set; } // Navigation to abstract type +} + +// ❌ INEFFICIENT - Loads all 34 columns from BaseRequest: +filters.For().Add( + projection: _ => _, // Identity projection + filter: (_, _, _, a) => a.TravelRequest?.GroupOwnerId == groupId); + +// ✅ EFFICIENT - Only loads Id, GroupOwnerId, HighestStatusAchieved: +filters.For().Add( + projection: a => new { + a.Id, + RequestOwnerId = a.TravelRequest?.GroupOwnerId, + RequestStatus = a.TravelRequest?.HighestStatusAchieved + }, + filter: (_, _, _, proj) => proj.RequestOwnerId == groupId); +``` + +### Detection and Prevention + +The library provides both compile-time and runtime detection: + +**Compile-Time (Analyzer GQLEF007)** + +The analyzer detects when filters use identity projections with abstract navigation access: + +```csharp +// This will show GQLEF007 error in the IDE: +filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.AbstractParent?.Property == "value"); +``` + +The code fixer can automatically convert this to an explicit projection. + +**Runtime (Exception)** + +If the analyzer is bypassed, runtime validation will throw an exception when the filter is registered: + +```csharp +// Throws InvalidOperationException: +// "Filter for 'Child' uses identity projection '_ => _' to access properties +// of abstract navigation 'Parent' (BaseEntity). This forces Include() to load +// all columns from BaseEntity. Extract only the required properties..." +``` + +### Best Practices + +1. **Always use explicit projections** when accessing abstract navigations +2. **Extract only required properties** from the abstract navigation +3. **Flatten navigation properties** in the projection (e.g., `ParentProperty` instead of nested access) +4. **Update the filter** to use the flattened property names + +### Concrete Navigations + +This issue only affects **abstract** navigation types. Concrete navigation types work fine with identity projections: + +```csharp +// ✅ WORKS - ConcreteParent is not abstract: +filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.ConcreteParent?.Property == "value"); +``` + +### See Also + +* [GQLEF007 Diagnostic Documentation](/docs/analyzers/GQLEF007.md) +* [Identity Projection Filters](#simplified-filter-api) diff --git a/docs/mdsource/filters.source.md b/docs/mdsource/filters.source.md index ee91cd1c..42d5cdbb 100644 --- a/docs/mdsource/filters.source.md +++ b/docs/mdsource/filters.source.md @@ -86,7 +86,7 @@ snippet: nullable-value-type-projections Common nullable patterns: * **Has value check**: `quantity.HasValue && quantity.Value > 0` -* **Null check**: `!quantity.HasValue` +* **Null check**: `?quantity.HasValue` * **Exact match**: `isApproved == true` (not null or false) @@ -201,12 +201,12 @@ The simplified API is syntactic sugar for the identity projection pattern: ```csharp // Simplified API filters.For().Add( - filter: (_, _, _, a) => a.Id != Guid.Empty); + filter: (_, _, _, a) => a.Id ?= Guid.Empty); // Equivalent full API filters.For().Add( projection: _ => _, // Identity projection - filter: (_, _, _, a) => a.Id != Guid.Empty); + filter: (_, _, _, a) => a.Id ?= Guid.Empty); ``` ### Analyzer Support @@ -235,3 +235,96 @@ filters.For().Add( ``` The simplified API makes intent clearer and reduces boilerplate while maintaining the same runtime behavior + +## Abstract Type Navigations + +When working with filters that access properties through abstract navigation properties, special care must be taken to avoid performance issues. + +### The Problem + +Abstract types cannot be instantiated in SQL projections. When EF Core encounters an abstract navigation in a projection, it falls back to using `Include()` which loads **all columns** from the navigation table, even when only one or two fields are required. + +### Example + +```csharp +// Given: +public abstract class BaseRequest // Abstract class with many fields +{ + public Guid Id { get; set; } + public Guid GroupOwnerId { get; set; } + public RequestStatus HighestStatusAchieved { get; set; } + // ... 30+ more columns ... +} + +public class Accommodation +{ + public Guid Id { get; set; } + public Guid TravelRequestId { get; set; } + public BaseRequest? TravelRequest { get; set; } // Navigation to abstract type +} + +// ❌ INEFFICIENT - Loads all 34 columns from BaseRequest: +filters.For().Add( + projection: _ => _, // Identity projection + filter: (_, _, _, a) => a.TravelRequest?.GroupOwnerId == groupId); + +// ✅ EFFICIENT - Only loads Id, GroupOwnerId, HighestStatusAchieved: +filters.For().Add( + projection: a => new { + a.Id, + RequestOwnerId = a.TravelRequest?.GroupOwnerId, + RequestStatus = a.TravelRequest?.HighestStatusAchieved + }, + filter: (_, _, _, proj) => proj.RequestOwnerId == groupId); +``` + +### Detection and Prevention + +The library provides both compile-time and runtime detection: + +**Compile-Time (Analyzer GQLEF007)** + +The analyzer detects when filters use identity projections with abstract navigation access: + +```csharp +// This will show GQLEF007 error in the IDE: +filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.AbstractParent?.Property == "value"); +``` + +The code fixer can automatically convert this to an explicit projection. + +**Runtime (Exception)** + +If the analyzer is bypassed, runtime validation will throw an exception when the filter is registered: + +```csharp +// Throws InvalidOperationException: +// "Filter for 'Child' uses identity projection '_ => _' to access properties +// of abstract navigation 'Parent' (BaseEntity). This forces Include() to load +// all columns from BaseEntity. Extract only the required properties..." +``` + +### Best Practices + +1. **Always use explicit projections** when accessing abstract navigations +2. **Extract only required properties** from the abstract navigation +3. **Flatten navigation properties** in the projection (e.g., `ParentProperty` instead of nested access) +4. **Update the filter** to use the flattened property names + +### Concrete Navigations + +This issue only affects **abstract** navigation types. Concrete navigation types work fine with identity projections: + +```csharp +// ✅ WORKS - ConcreteParent is not abstract: +filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.ConcreteParent?.Property == "value"); +``` + +### See Also + +* [GQLEF007 Diagnostic Documentation](/docs/analyzers/GQLEF007.md) +* [Identity Projection Filters](#simplified-filter-api) diff --git a/src/Directory.Build.props b/src/Directory.Build.props index bbf01b70..467fcb88 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -2,7 +2,7 @@ CS1591;NU5104;CS1573;CS9107;NU1608;NU1109 - 34.1.4 + 34.1.5 preview 1.0.0 EntityFrameworkCore, EntityFramework, GraphQL diff --git a/src/GraphQL.EntityFramework.Analyzers.Tests/AbstractNavigationProjectionCodeFixProviderTests.ConvertsIdentityProjectionToExplicitProjection.verified.txt b/src/GraphQL.EntityFramework.Analyzers.Tests/AbstractNavigationProjectionCodeFixProviderTests.ConvertsIdentityProjectionToExplicitProjection.verified.txt new file mode 100644 index 00000000..81d1ef94 --- /dev/null +++ b/src/GraphQL.EntityFramework.Analyzers.Tests/AbstractNavigationProjectionCodeFixProviderTests.ConvertsIdentityProjectionToExplicitProjection.verified.txt @@ -0,0 +1,27 @@ +using GraphQL.EntityFramework; +using Microsoft.EntityFrameworkCore; +using System; + +public abstract class BaseEntity +{ + public Guid Id { get; set; } + public string Property { get; set; } = ""; +} + +public class ChildEntity +{ + public Guid Id { get; set; } + public BaseEntity? Parent { get; set; } +} + +public class TestDbContext : DbContext { } + +public class TestClass +{ + public void ConfigureFilters(Filters filters) + { + filters.For().Add( + projection: c => new { c.Id, ParentProperty = c.Parent.Property }, + filter: (_, _, _, proj) => proj.ParentProperty == "test"); + } +} \ No newline at end of file diff --git a/src/GraphQL.EntityFramework.Analyzers.Tests/AbstractNavigationProjectionCodeFixProviderTests.cs b/src/GraphQL.EntityFramework.Analyzers.Tests/AbstractNavigationProjectionCodeFixProviderTests.cs new file mode 100644 index 00000000..f50f0b42 --- /dev/null +++ b/src/GraphQL.EntityFramework.Analyzers.Tests/AbstractNavigationProjectionCodeFixProviderTests.cs @@ -0,0 +1,162 @@ +public class AbstractNavigationProjectionCodeFixProviderTests +{ + [Fact] + public async Task ConvertsIdentityProjectionToExplicitProjection() + { + var source = """ + using GraphQL.EntityFramework; + using Microsoft.EntityFrameworkCore; + using System; + + public abstract class BaseEntity + { + public Guid Id { get; set; } + public string Property { get; set; } = ""; + } + + public class ChildEntity + { + public Guid Id { get; set; } + public BaseEntity? Parent { get; set; } + } + + public class TestDbContext : DbContext { } + + public class TestClass + { + public void ConfigureFilters(Filters filters) + { + filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.Parent!.Property == "test"); + } + } + """; + + var result = await ApplyCodeFixAsync(source); + await Verify(result); + } + + static async Task ApplyCodeFixAsync(string source) + { + var document = CreateDocument(source); + var compilation = await document.Project.GetCompilationAsync(); + if (compilation == null) + { + throw new("Compilation failed"); + } + + // Get diagnostics from analyzer + var analyzer = new FilterIdentityProjectionAnalyzer(); + var compilationWithAnalyzers = compilation.WithAnalyzers([analyzer]); + var allDiagnostics = await compilationWithAnalyzers.GetAllDiagnosticsAsync(); + + var diagnostic = allDiagnostics.FirstOrDefault(_ => _.Id == "GQLEF007"); + if (diagnostic == null) + { + throw new("No GQLEF007 diagnostic found"); + } + + // Apply code fix + var codeFixProvider = new AbstractNavigationProjectionCodeFixProvider(); + var actions = new List(); + + var context = new CodeFixContext( + document, + diagnostic, + (action, _) => actions.Add(action), + default); + + await codeFixProvider.RegisterCodeFixesAsync(context); + + if (actions.Count == 0) + { + throw new($"No code fixes were registered. Diagnostic: {diagnostic}"); + } + + var operations = await actions[0].GetOperationsAsync(default); + if (operations.IsEmpty) + { + throw new("Code action returned no operations"); + } + + var operation = operations.OfType().FirstOrDefault(); + if (operation == null) + { + throw new("No ApplyChangesOperation found"); + } + + var changedSolution = operation.ChangedSolution; + var changedDocument = changedSolution.GetDocument(document.Id); + if (changedDocument == null) + { + throw new("Failed to get changed document"); + } + + var root = await changedDocument.GetSyntaxRootAsync(); + return root?.ToFullString() ?? ""; + } + + static Document CreateDocument(string source) + { + var projectId = ProjectId.CreateNewId(); + var documentId = DocumentId.CreateNewId(projectId); + + var references = new List + { + MetadataReference.CreateFromFile(typeof(object).Assembly.Location), + MetadataReference.CreateFromFile(typeof(DbContext).Assembly.Location), + MetadataReference.CreateFromFile(typeof(Filters<>).Assembly.Location), + }; + + var requiredAssemblies = new[] + { + typeof(Guid).Assembly, + Assembly.Load("System.Runtime"), + typeof(IQueryable<>).Assembly, + Assembly.Load("System.Security.Claims"), + }; + + foreach (var assembly in requiredAssemblies) + { + if (!string.IsNullOrEmpty(assembly.Location)) + { + references.Add(MetadataReference.CreateFromFile(assembly.Location)); + } + } + + foreach (var assembly in AppDomain.CurrentDomain.GetAssemblies()) + { + if (!assembly.IsDynamic && + !string.IsNullOrEmpty(assembly.Location) && + references.All(_ => _.Display != assembly.Location)) + { + var name = assembly.GetName().Name ?? ""; + if ((!name.StartsWith("System.") && + !name.StartsWith("Microsoft.")) || + name.Contains("xunit", StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + try + { + references.Add(MetadataReference.CreateFromFile(assembly.Location)); + } + catch + { + // Ignore assemblies that can't be referenced + } + } + } + + var solution = new AdhocWorkspace() + .CurrentSolution + .AddProject(projectId, "TestProject", "TestProject", LanguageNames.CSharp) + .WithProjectCompilationOptions(projectId, new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)) + .AddMetadataReferences(projectId, references) + .AddDocument(documentId, "TestDocument.cs", source); + + return solution.GetDocument(documentId)!; + } +} diff --git a/src/GraphQL.EntityFramework.Analyzers.Tests/FilterIdentityProjectionAnalyzerTests.cs b/src/GraphQL.EntityFramework.Analyzers.Tests/FilterIdentityProjectionAnalyzerTests.cs index 0d3ca3a0..3524dfcd 100644 --- a/src/GraphQL.EntityFramework.Analyzers.Tests/FilterIdentityProjectionAnalyzerTests.cs +++ b/src/GraphQL.EntityFramework.Analyzers.Tests/FilterIdentityProjectionAnalyzerTests.cs @@ -602,9 +602,206 @@ static async Task GetDiagnosticsAsync(string source) throw new($"Compilation errors:\n{errorMessages}"); } - // Filter to only GQLEF004, GQLEF005, and GQLEF006 diagnostics + // Filter to only GQLEF004, GQLEF005, GQLEF006, and GQLEF007 diagnostics return allDiagnostics - .Where(_ => _.Id == "GQLEF004" || _.Id == "GQLEF005" || _.Id == "GQLEF006") + .Where(_ => _.Id == "GQLEF004" || _.Id == "GQLEF005" || _.Id == "GQLEF006" || _.Id == "GQLEF007") .ToArray(); } + + // GQLEF007 Tests - Identity projection with abstract navigation access + + [Fact] + public async Task GQLEF007_Reports_IdentityProjection_WithAbstractNavigationAccess() + { + var source = """ + using GraphQL.EntityFramework; + using Microsoft.EntityFrameworkCore; + using System; + + public abstract class BaseEntity + { + public Guid Id { get; set; } + public string Property { get; set; } = ""; + } + + public class DerivedEntity : BaseEntity { } + + public class ChildEntity + { + public Guid Id { get; set; } + public Guid? ParentId { get; set; } + public BaseEntity? Parent { get; set; } + } + + public class TestDbContext : DbContext { } + + public class TestClass + { + public void ConfigureFilters(Filters filters) + { + filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.Parent!.Property == "test"); + } + } + """; + + var diagnostics = await GetDiagnosticsAsync(source); + Assert.Single(diagnostics); + Assert.Equal("GQLEF007", diagnostics[0].Id); + Assert.Contains("Parent", diagnostics[0].GetMessage()); + } + + [Fact] + public async Task GQLEF007_Reports_FourParamFilter_WithAbstractNavigationAccess() + { + var source = """ + using GraphQL.EntityFramework; + using Microsoft.EntityFrameworkCore; + using System; + + public abstract class BaseEntity + { + public Guid Id { get; set; } + public string Status { get; set; } = ""; + } + + public class ChildEntity + { + public Guid Id { get; set; } + public BaseEntity? Parent { get; set; } + } + + public class TestDbContext : DbContext { } + + public class TestClass + { + public void ConfigureFilters(Filters filters) + { + filters.For().Add( + filter: (_, _, _, c) => c.Parent!.Status == "Active"); + } + } + """; + + var diagnostics = await GetDiagnosticsAsync(source); + Assert.Single(diagnostics); + Assert.Equal("GQLEF007", diagnostics[0].Id); + } + + [Fact] + public async Task GQLEF007_NoReport_ConcreteNavigationAccess() + { + var source = """ + using GraphQL.EntityFramework; + using Microsoft.EntityFrameworkCore; + using System; + + public class ConcreteParent + { + public Guid Id { get; set; } + public string Property { get; set; } = ""; + } + + public class ChildEntity + { + public Guid Id { get; set; } + public ConcreteParent? Parent { get; set; } + } + + public class TestDbContext : DbContext { } + + public class TestClass + { + public void ConfigureFilters(Filters filters) + { + filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.Parent!.Property == "test"); + } + } + """; + + var diagnostics = await GetDiagnosticsAsync(source); + // Should report GQLEF006 (non-key property access) but not GQLEF007 + Assert.DoesNotContain(diagnostics, d => d.Id == "GQLEF007"); + } + + [Fact] + public async Task GQLEF007_NoReport_ExplicitProjection_WithAbstractNavigation() + { + var source = """ + using GraphQL.EntityFramework; + using Microsoft.EntityFrameworkCore; + using System; + + public abstract class BaseEntity + { + public Guid Id { get; set; } + public string Property { get; set; } = ""; + } + + public class ChildEntity + { + public Guid Id { get; set; } + public BaseEntity? Parent { get; set; } + } + + public class TestDbContext : DbContext { } + + public class TestClass + { + public void ConfigureFilters(Filters filters) + { + filters.For().Add( + projection: c => new { c.Id, ParentProp = c.Parent!.Property }, + filter: (_, _, _, proj) => proj.ParentProp == "test"); + } + } + """; + + var diagnostics = await GetDiagnosticsAsync(source); + // Explicit projection - analyzer doesn't catch this (runtime does) + Assert.Empty(diagnostics); + } + + [Fact] + public async Task GQLEF007_Reports_NestedAbstractNavigationAccess() + { + var source = """ + using GraphQL.EntityFramework; + using Microsoft.EntityFrameworkCore; + using System; + using System.Collections.Generic; + + public abstract class BaseEntity + { + public Guid Id { get; set; } + public List Children { get; set; } = new(); + } + + public class ChildEntity + { + public Guid Id { get; set; } + public BaseEntity? Parent { get; set; } + } + + public class TestDbContext : DbContext { } + + public class TestClass + { + public void ConfigureFilters(Filters filters) + { + filters.For().Add( + projection: _ => _, + filter: (_, _, _, c) => c.Parent!.Children.Count > 0); + } + } + """; + + var diagnostics = await GetDiagnosticsAsync(source); + Assert.Single(diagnostics); + Assert.Equal("GQLEF007", diagnostics[0].Id); + Assert.Contains("Parent", diagnostics[0].GetMessage()); + } } diff --git a/src/GraphQL.EntityFramework.Analyzers/AnalyzerReleases.Unshipped.md b/src/GraphQL.EntityFramework.Analyzers/AnalyzerReleases.Unshipped.md index 4c5e96d7..2ddac229 100644 --- a/src/GraphQL.EntityFramework.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/GraphQL.EntityFramework.Analyzers/AnalyzerReleases.Unshipped.md @@ -10,3 +10,4 @@ GQLEF003 | Usage | Error | Identity projection is not allowed in projection-base GQLEF004 | Usage | Info | Suggests using 4-parameter filter syntax instead of explicit identity projection GQLEF005 | Usage | Error | Prevents accessing non-key properties with 4-parameter filter syntax GQLEF006 | Usage | Error | Prevents accessing non-key properties with explicit identity projection +GQLEF007 | Usage | Error | Identity projection with abstract navigation access diff --git a/src/GraphQL.EntityFramework.Analyzers/DiagnosticDescriptors.cs b/src/GraphQL.EntityFramework.Analyzers/DiagnosticDescriptors.cs index bac59381..e0c24a13 100644 --- a/src/GraphQL.EntityFramework.Analyzers/DiagnosticDescriptors.cs +++ b/src/GraphQL.EntityFramework.Analyzers/DiagnosticDescriptors.cs @@ -49,4 +49,14 @@ public static class DiagnosticDescriptors isEnabledByDefault: true, description: "When using identity projection '_ => _', only primary keys and foreign keys are guaranteed to be loaded. Either project specific properties needed or use the simplified API if you only need key properties.", helpLinkUri: "https://github.com/SimonCropp/GraphQL.EntityFramework#identity-projection-filters"); + + public static readonly DiagnosticDescriptor GQLEF007 = new( + id: "GQLEF007", + title: "Identity projection with abstract navigation access", + messageFormat: "Filter for '{0}' uses identity projection but accesses abstract navigation '{1}'. This forces Include() to load all columns. Use explicit projection to load only required properties.", + category: "Usage", + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: "Abstract type navigations cannot use SQL projections and fall back to Include() which loads all columns. Use explicit projection to extract only required properties from abstract navigations.", + helpLinkUri: "https://github.com/SimonCropp/GraphQL.EntityFramework#abstract-navigation-projections"); } diff --git a/src/GraphQL.EntityFramework.Analyzers/FilterIdentityProjectionAnalyzer.cs b/src/GraphQL.EntityFramework.Analyzers/FilterIdentityProjectionAnalyzer.cs index bf32b23e..a1a9e311 100644 --- a/src/GraphQL.EntityFramework.Analyzers/FilterIdentityProjectionAnalyzer.cs +++ b/src/GraphQL.EntityFramework.Analyzers/FilterIdentityProjectionAnalyzer.cs @@ -7,7 +7,8 @@ public class FilterIdentityProjectionAnalyzer : DiagnosticAnalyzer [ DiagnosticDescriptors.GQLEF004, DiagnosticDescriptors.GQLEF005, - DiagnosticDescriptors.GQLEF006 + DiagnosticDescriptors.GQLEF006, + DiagnosticDescriptors.GQLEF007 ]; public override void Initialize(AnalysisContext context) @@ -45,6 +46,21 @@ static void AnalyzeInvocation(SyntaxNodeAnalysisContext context) } // Identity projection detected + // First check for abstract navigation access (GQLEF007) + var abstractNav = FindAbstractNavigationAccess(filterLambda, context.SemanticModel); + if (abstractNav != null) + { + // Error: Identity projection with abstract navigation access + var diagnostic = Diagnostic.Create( + DiagnosticDescriptors.GQLEF007, + invocation.GetLocation(), + entityType ?? "Entity", + abstractNav); + context.ReportDiagnostic(diagnostic); + return; + } + + // Then check for non-key property access (GQLEF006) var nonKeyProperty = FindNonKeyPropertyAccess(filterLambda, context.SemanticModel); if (nonKeyProperty != null) { @@ -75,7 +91,20 @@ static void AnalyzeInvocation(SyntaxNodeAnalysisContext context) if (is4ParamFilter) { - // Validate that filter only accesses PK/FK properties + // First check for abstract navigation access (GQLEF007) + var abstractNav = FindAbstractNavigationAccess(filterLambda, context.SemanticModel); + if (abstractNav != null) + { + var diagnostic = Diagnostic.Create( + DiagnosticDescriptors.GQLEF007, + invocation.GetLocation(), + entityType ?? "Entity", + abstractNav); + context.ReportDiagnostic(diagnostic); + return; + } + + // Then validate that filter only accesses PK/FK properties var nonKeyProperty = FindNonKeyPropertyAccess(filterLambda, context.SemanticModel); if (nonKeyProperty != null) { @@ -149,8 +178,8 @@ static bool IsFilterBuilderAdd( } // Find the projection and filter parameters - var projectionParameter = methodSymbol.Parameters.FirstOrDefault(p => p.Name == "projection"); - var filterParameter = methodSymbol.Parameters.FirstOrDefault(p => p.Name == "filter"); + var projectionParameter = methodSymbol.Parameters.FirstOrDefault(_ => _.Name == "projection"); + var filterParameter = methodSymbol.Parameters.FirstOrDefault(_ => _.Name == "filter"); if (projectionParameter == null && filterParameter == null) { @@ -409,4 +438,75 @@ static int GetParameterIndex(ImmutableArray parameters, IParam } return -1; } + + static string? FindAbstractNavigationAccess( + LambdaExpressionSyntax lambda, + SemanticModel semanticModel) + { + var body = lambda.Body; + + // Get the filter lambda parameter name (last parameter in the filter signature) + string? filterParameterName = null; + if (lambda is SimpleLambdaExpressionSyntax simpleLambda) + { + filterParameterName = simpleLambda.Parameter.Identifier.Text; + } + else if (lambda is ParenthesizedLambdaExpressionSyntax parenthesizedLambda) + { + var parameterCount = parenthesizedLambda.ParameterList.Parameters.Count; + if (parameterCount > 0) + { + filterParameterName = parenthesizedLambda.ParameterList.Parameters[parameterCount - 1].Identifier.Text; + } + } + + if (filterParameterName == null) + { + return null; + } + + // Find all member access expressions in the lambda + var memberAccesses = body.DescendantNodesAndSelf() + .OfType(); + + foreach (var memberAccess in memberAccesses) + { + // Check for navigation property access pattern: e.Parent.Property + // We want to detect when `e.Parent` is a navigation to an abstract type + if (memberAccess.Expression is MemberAccessExpressionSyntax { Expression: IdentifierNameSyntax identifier } nestedAccess && + identifier.Identifier.Text == filterParameterName) + { + // This is e.Parent.Property - check if Parent is abstract + if (semanticModel.GetSymbolInfo(nestedAccess).Symbol is IPropertySymbol { Type.IsAbstract: true } navigationSymbol) + { + return navigationSymbol.Name; + } + } + // Also check direct navigation access: e.Parent (without further property access) + else if (memberAccess.Expression is IdentifierNameSyntax directIdentifier && + directIdentifier.Identifier.Text == filterParameterName) + { + if (semanticModel.GetSymbolInfo(memberAccess).Symbol is not IPropertySymbol propertySymbol) + { + continue; + } + + var propType = propertySymbol.Type; + // Check if this is a reference type (not a primitive) and is abstract + // This indicates it's likely a navigation property to an abstract entity + if (propType is not { IsAbstract: true, TypeKind: TypeKind.Class }) + { + continue; + } + + // But make sure it's not a primitive key property or known scalar + if (!IsPrimaryKeyProperty(propertySymbol) && !IsForeignKeyProperty(propertySymbol)) + { + return propertySymbol.Name; + } + } + } + + return null; + } } diff --git a/src/GraphQL.EntityFramework.CodeFixes/AbstractNavigationProjectionCodeFixProvider.cs b/src/GraphQL.EntityFramework.CodeFixes/AbstractNavigationProjectionCodeFixProvider.cs new file mode 100644 index 00000000..9bbc3bd7 --- /dev/null +++ b/src/GraphQL.EntityFramework.CodeFixes/AbstractNavigationProjectionCodeFixProvider.cs @@ -0,0 +1,325 @@ +using Microsoft.CodeAnalysis.CSharp; + +namespace GraphQL.EntityFramework.CodeFixes; + +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AbstractNavigationProjectionCodeFixProvider))] +[Shared] +public class AbstractNavigationProjectionCodeFixProvider : CodeFixProvider +{ + const string title = "Convert to explicit projection"; + + public override ImmutableArray FixableDiagnosticIds { get; } = + [DiagnosticDescriptors.GQLEF007.Id]; + + public override FixAllProvider GetFixAllProvider() => + WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken); + if (root == null) + { + return; + } + + var diagnostic = context.Diagnostics[0]; + var diagnosticSpan = diagnostic.Location.SourceSpan; + + var node = root.FindNode(diagnosticSpan); + var invocation = node as InvocationExpressionSyntax ?? node.FirstAncestorOrSelf(); + + if (invocation == null) + { + return; + } + + context.RegisterCodeFix( + CodeAction.Create( + title: title, + createChangedDocument: c => ConvertToExplicitProjectionAsync(context.Document, invocation, c), + equivalenceKey: title), + diagnostic); + } + + static async Task ConvertToExplicitProjectionAsync( + Document document, + InvocationExpressionSyntax invocation, + Cancel cancel) + { + var root = await document.GetSyntaxRootAsync(cancel); + if (root == null) + { + return document; + } + + // Find projection and filter arguments + var arguments = invocation.ArgumentList.Arguments; + LambdaExpressionSyntax? filterLambda = null; + var projectionArgumentIndex = -1; + var filterArgumentIndex = -1; + + for (var i = 0; i < arguments.Count; i++) + { + var arg = arguments[i]; + if (arg.NameColon?.Name.Identifier.Text == "projection") + { + projectionArgumentIndex = i; + } + else if (arg.NameColon?.Name.Identifier.Text == "filter" && + arg.Expression is LambdaExpressionSyntax filtLambda) + { + filterLambda = filtLambda; + filterArgumentIndex = i; + } + } + + // If no explicit projection argument, this is 4-parameter syntax + var is4ParamSyntax = projectionArgumentIndex == -1; + + if (filterLambda == null) + { + return document; + } + + // Extract accessed properties from filter + var accessedProperties = ExtractAccessedProperties(filterLambda); + if (accessedProperties.Count == 0) + { + return document; + } + + // Get the entity parameter name from the filter (last parameter) + string entityParamName; + if (filterLambda is SimpleLambdaExpressionSyntax simpleLambda) + { + entityParamName = simpleLambda.Parameter.Identifier.Text; + } + else if (filterLambda is ParenthesizedLambdaExpressionSyntax parenthesizedLambda) + { + var paramCount = parenthesizedLambda.ParameterList.Parameters.Count; + entityParamName = paramCount > 0 + ? parenthesizedLambda.ParameterList.Parameters[paramCount - 1].Identifier.Text + : "entity"; + } + else + { + return document; + } + + // Build new projection lambda: e => new { e.Id, Prop1 = e.Nav.Prop1, ... } + var newProjectionLambda = BuildProjectionLambda(entityParamName, accessedProperties); + + // Build new filter lambda with renamed parameter: (_, _, _, proj) => proj.Prop1 == value + var newFilterLambda = BuildFilterLambda(filterLambda, accessedProperties); + + // Replace arguments + SyntaxNode newInvocation; + if (is4ParamSyntax) + { + // Add projection argument + var projectionArg = SyntaxFactory.Argument(newProjectionLambda) + .WithNameColon(SyntaxFactory.NameColon("projection")) + .WithLeadingTrivia(SyntaxFactory.Whitespace("\n ")); + + var filterArg = arguments[0].WithExpression(newFilterLambda); + + var newArguments = SyntaxFactory.SeparatedList([projectionArg, filterArg], + [SyntaxFactory.Token(SyntaxKind.CommaToken).WithTrailingTrivia(SyntaxFactory.Whitespace(" "))]); + + newInvocation = invocation.WithArgumentList( + invocation.ArgumentList.WithArguments(newArguments)); + } + else + { + // Replace projection and filter arguments + var newArguments = arguments; + + if (projectionArgumentIndex >= 0) + { + newArguments = newArguments.Replace( + newArguments[projectionArgumentIndex], + newArguments[projectionArgumentIndex].WithExpression(newProjectionLambda)); + } + + if (filterArgumentIndex >= 0) + { + newArguments = newArguments.Replace( + newArguments[filterArgumentIndex], + newArguments[filterArgumentIndex].WithExpression(newFilterLambda)); + } + + newInvocation = invocation.WithArgumentList( + invocation.ArgumentList.WithArguments(newArguments)); + } + + var newRoot = root.ReplaceNode(invocation, newInvocation); + return document.WithSyntaxRoot(newRoot); + } + + static List ExtractAccessedProperties(LambdaExpressionSyntax filterLambda) + { + var properties = new List(); + var body = filterLambda.Body; + + // Get filter parameter name + string? paramName = null; + if (filterLambda is SimpleLambdaExpressionSyntax simpleLambda) + { + paramName = simpleLambda.Parameter.Identifier.Text; + } + else if (filterLambda is ParenthesizedLambdaExpressionSyntax parenthesizedLambda) + { + var paramCount = parenthesizedLambda.ParameterList.Parameters.Count; + if (paramCount > 0) + { + paramName = parenthesizedLambda.ParameterList.Parameters[paramCount - 1].Identifier.Text; + } + } + + if (paramName == null) + { + return properties; + } + + // Find all member accesses + var memberAccesses = body.DescendantNodesAndSelf() + .OfType(); + + foreach (var memberAccess in memberAccesses) + { + // Look for patterns like: e.Parent.Property or e.Parent!.Property (with null-forgiving operator) + // The null-forgiving operator creates a PostfixUnaryExpressionSyntax wrapper + var innerExpression = memberAccess.Expression; + + // Unwrap null-forgiving operator if present: e.Parent! -> e.Parent + if (innerExpression is PostfixUnaryExpressionSyntax { RawKind: (int)SyntaxKind.SuppressNullableWarningExpression } postfix) + { + innerExpression = postfix.Operand; + } + + if (innerExpression is MemberAccessExpressionSyntax nestedAccess) + { + // Check if the root is our parameter (possibly with null-forgiving) + var rootExpr = nestedAccess.Expression; + if (rootExpr is PostfixUnaryExpressionSyntax { RawKind: (int)SyntaxKind.SuppressNullableWarningExpression } rootPostfix) + { + rootExpr = rootPostfix.Operand; + } + + if (rootExpr is IdentifierNameSyntax identifier && identifier.Identifier.Text == paramName) + { + // e.Parent.Property - extract as "Parent.Property" + var navName = nestedAccess.Name.Identifier.Text; + var propName = memberAccess.Name.Identifier.Text; + var fullPath = $"{navName}.{propName}"; + var flatName = $"{navName}{propName}"; + + if (!properties.Any(p => p.FullPath == fullPath)) + { + properties.Add(new(fullPath, flatName, memberAccess)); + } + } + } + } + + return properties; + } + + static LambdaExpressionSyntax BuildProjectionLambda( + string paramName, + List properties) + { + // Build: _ => new { _.Id, Prop1 = _.Nav.Prop1, ... } + var parameter = SyntaxFactory.Parameter(SyntaxFactory.Identifier(paramName)); + + List initializers = []; + + // Add Id property + initializers.Add( + SyntaxFactory.AnonymousObjectMemberDeclarator( + SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + SyntaxFactory.IdentifierName(paramName), + SyntaxFactory.IdentifierName("Id")))); + + // Add accessed properties with flattened names + foreach (var prop in properties) + { + var parts = prop.FullPath.Split('.'); + ExpressionSyntax expression = SyntaxFactory.IdentifierName(paramName); + + foreach (var part in parts) + { + expression = SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + expression, + SyntaxFactory.IdentifierName(part)); + } + + initializers.Add( + SyntaxFactory.AnonymousObjectMemberDeclarator( + SyntaxFactory.NameEquals(prop.FlatName), + expression)); + } + + var anonymousObject = SyntaxFactory.AnonymousObjectCreationExpression( + SyntaxFactory.SeparatedList(initializers)); + + return SyntaxFactory.SimpleLambdaExpression(parameter, anonymousObject); + } + + static LambdaExpressionSyntax BuildFilterLambda( + LambdaExpressionSyntax originalFilter, + List properties) + { + // Replace entity parameter references with proj and update property accesses + var newBody = originalFilter.Body; + + // Replace each property access with flattened name + foreach (var prop in properties) + { + // Replace e.Parent.Property with proj.ParentProperty + newBody = newBody.ReplaceNodes( + newBody.DescendantNodesAndSelf().Where(_ => _ == prop.OriginalAccess), + (_, _) => SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + SyntaxFactory.IdentifierName("proj"), + SyntaxFactory.IdentifierName(prop.FlatName))); + } + + // Build new parameter list with "proj" as last parameter + if (originalFilter is SimpleLambdaExpressionSyntax) + { + return SyntaxFactory.SimpleLambdaExpression( + SyntaxFactory.Parameter(SyntaxFactory.Identifier("proj")), + newBody); + } + + if (originalFilter is ParenthesizedLambdaExpressionSyntax parenthesizedLambda) + { + var parameters = parenthesizedLambda.ParameterList.Parameters; + var newParameters = parameters.RemoveAt(parameters.Count - 1) + .Add(SyntaxFactory.Parameter(SyntaxFactory.Identifier("proj"))); + + return SyntaxFactory.ParenthesizedLambdaExpression( + SyntaxFactory.ParameterList(newParameters), + newBody); + } + + return originalFilter; + } + + class PropertyAccess + { + public PropertyAccess(string fullPath, string flatName, SyntaxNode originalAccess) + { + FullPath = fullPath; + FlatName = flatName; + OriginalAccess = originalAccess; + } + + public string FullPath { get; } + public string FlatName { get; } + public SyntaxNode OriginalAccess { get; } + } +} diff --git a/src/GraphQL.EntityFramework/Filters/FilterEntry.cs b/src/GraphQL.EntityFramework/Filters/FilterEntry.cs index 1ba9f4ab..60ba19d5 100644 --- a/src/GraphQL.EntityFramework/Filters/FilterEntry.cs +++ b/src/GraphQL.EntityFramework/Filters/FilterEntry.cs @@ -20,6 +20,7 @@ public FilterEntry( var compiled = projection.Compile(); compiledProjection = entity => compiled((TEntity)entity); RequiredPropertyNames = ProjectionAnalyzer.ExtractRequiredProperties(projection); + ValidateProjectionCompatibility(projection, RequiredPropertyNames); } } @@ -36,4 +37,65 @@ public Task ShouldIncludeWithProjection( : default!; return filter(userContext, data, userPrincipal, projectedData); } + + static void ValidateProjectionCompatibility( + Expression> projection, + IReadOnlySet requiredPropertyNames) + { + // Extract navigation paths (e.g., "Parent.Property" -> navigation "Parent") + var navigationPaths = requiredPropertyNames + .Where(_ => _.Contains('.')) + .Select(_ => _.Split('.')[0]) + .Distinct() + .ToList(); + + if (navigationPaths.Count == 0) + { + return; + } + + var entityType = typeof(TEntity); + var isIdentity = IsIdentityProjection(projection); + + // Check each navigation for abstract types + foreach (var navPath in navigationPaths) + { + var navProperty = entityType.GetProperty(navPath); + if (navProperty == null) + { + continue; + } + + var navType = navProperty.PropertyType; + + // For collections, get the element type + if (navType.IsGenericType) + { + var genericDef = navType.GetGenericTypeDefinition(); + if (genericDef == typeof(ICollection<>) || + genericDef == typeof(IList<>) || + genericDef == typeof(IEnumerable<>) || + genericDef == typeof(List<>)) + { + navType = navType.GetGenericArguments()[0]; + } + } + + if (navType.IsAbstract) + { + var projectionType = isIdentity ? "identity projection '_ => _'" : "projection that accesses abstract navigation"; + throw new InvalidOperationException( + $"Filter for '{entityType.Name}' uses {projectionType} " + + $"to access properties of abstract navigation '{navPath}' ({navType.Name}). " + + $"This forces Include() to load all columns from {navType.Name}. " + + $"Abstract types cannot be projected. Extract only the required properties: " + + $"projection: e => new {{ e.Id, {navPath}Property = e.{navPath}.PropertyName }}, " + + $"filter: (_, _, _, proj) => proj.{navPath}Property == value"); + } + } + } + + // Detect: x => x (where body == parameter) + static bool IsIdentityProjection(Expression> projection) => + projection.Body == projection.Parameters[0]; } diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs index 9d718204..ddf49a9f 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs @@ -166,16 +166,16 @@ FieldType BuildFirstField( // Get filter-required fields early so we can add filter-required navigations via Include var allFilterFields = fieldContext.Filters?.GetAllRequiredFilterProperties(); - var (queryWithIncludes, hasAbstractFilterNavigations) = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); - query = queryWithIncludes; + query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context); query = query.ApplyGraphQlArguments(context, names, false, omitQueryArguments); // Apply column projection based on requested GraphQL fields // Skip projection for abstract types as they cannot be instantiated - // Skip projection if abstract filter navigations are present (projection fails on abstract types, EF would drop includes) - if (!typeof(TReturn).IsAbstract && !hasAbstractFilterNavigations) + if (!typeof(TReturn).IsAbstract) { - // Merge filter-required scalar fields into projection (navigations are handled via Include above) + // Try to build projection even with abstract filter navigations + // The projection system may handle them (e.g., TPH inheritance) + // If projection build fails, we fall back to Include (which was already added above) if (includeAppender.TryGetProjectionExpressionWithFilters(context, allFilterFields, out var selectExpr)) { query = query.Select(selectExpr); @@ -248,4 +248,4 @@ await fieldContext.Filters.ShouldInclude(context.UserContext, fieldContext.DbCon TReturn? ReturnNullable(IQueryable? query = null) => nullable ? null : throw new FirstEntityNotFoundException(query?.SafeToQueryString()); } -} \ No newline at end of file +} diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs index ce6753e5..16f23071 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs @@ -112,8 +112,7 @@ FieldType BuildQueryField( // Get filter-required fields early so we can add filter-required navigations via Include var allFilterFields = fieldContext.Filters?.GetAllRequiredFilterProperties(); - var (queryWithIncludes, hasAbstractFilterNavigations) = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); - query = queryWithIncludes; + query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context); if (!omitQueryArguments) { query = query.ApplyGraphQlArguments(context, names, true, omitQueryArguments); @@ -121,10 +120,11 @@ FieldType BuildQueryField( // Apply column projection based on requested GraphQL fields // Skip projection for abstract types as they cannot be instantiated - // Skip projection if abstract filter navigations are present (projection fails on abstract types, EF would drop includes) - if (!typeof(TReturn).IsAbstract && !hasAbstractFilterNavigations) + if (!typeof(TReturn).IsAbstract) { - // Merge filter-required scalar fields into projection (navigations are handled via Include above) + // Try to build projection even with abstract filter navigations + // The projection system may handle them (e.g., TPH inheritance) + // If projection build fails, we fall back to Include (which was already added above) if (includeAppender.TryGetProjectionExpressionWithFilters(context, allFilterFields, out var selectExpr)) { query = query.Select(selectExpr); @@ -198,4 +198,4 @@ static Type MakeListGraphType(Type? itemGraphType) keyNames.TryGetValue(typeof(TSource), out var names); return names; } -} \ No newline at end of file +} diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs index c00c83e2..595830af 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs @@ -158,16 +158,16 @@ ConnectionBuilder AddQueryableConnection( // Get filter-required fields early so we can add filter-required navigations via Include var allFilterFields = fieldContext.Filters?.GetAllRequiredFilterProperties(); - var (queryWithIncludes, hasAbstractFilterNavigations) = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); - query = queryWithIncludes; + query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context); query = query.ApplyGraphQlArguments(context, names, true, omitQueryArguments); // Apply column projection based on requested GraphQL fields // Skip projection for abstract types as they cannot be instantiated - // Skip projection if abstract filter navigations are present (projection fails on abstract types, EF would drop includes) - if (!typeof(TReturn).IsAbstract && !hasAbstractFilterNavigations) + if (!typeof(TReturn).IsAbstract) { - // Merge filter-required scalar fields into projection (navigations are handled via Include above) + // Try to build projection even with abstract filter navigations + // The projection system may handle them (e.g., TPH inheritance) + // If projection build fails, we fall back to Include (which was already added above) if (includeAppender.TryGetProjectionExpressionWithFilters(context, allFilterFields, out var selectExpr)) { query = query.Select(selectExpr); @@ -219,4 +219,4 @@ ConnectionBuilder AddQueryableConnection( field.AddWhereArgument(hasId); return builder; } -} \ No newline at end of file +} diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs index 50410c42..f6fe30be 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs @@ -166,16 +166,16 @@ FieldType BuildSingleField( // Get filter-required fields early so we can add filter-required navigations via Include var allFilterFields = fieldContext.Filters?.GetAllRequiredFilterProperties(); - var (queryWithIncludes, hasAbstractFilterNavigations) = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); - query = queryWithIncludes; + query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context); query = query.ApplyGraphQlArguments(context, names, false, omitQueryArguments); // Apply column projection based on requested GraphQL fields // Skip projection for abstract types as they cannot be instantiated - // Skip projection if abstract filter navigations are present (projection fails on abstract types, EF would drop includes) - if (!typeof(TReturn).IsAbstract && !hasAbstractFilterNavigations) + if (!typeof(TReturn).IsAbstract) { - // Merge filter-required scalar fields into projection (navigations are handled via Include above) + // Try to build projection even with abstract filter navigations + // The projection system may handle them (e.g., TPH inheritance) + // If projection build fails, we fall back to Include (which was already added above) if (includeAppender.TryGetProjectionExpressionWithFilters(context, allFilterFields, out var selectExpr)) { query = query.Select(selectExpr); @@ -248,4 +248,4 @@ await fieldContext.Filters.ShouldInclude(context.UserContext, fieldContext.DbCon TReturn? ReturnNullable(IQueryable? query = null) => nullable ? null : throw new SingleEntityNotFoundException(query?.SafeToQueryString()); } -} \ No newline at end of file +} diff --git a/src/GraphQL.EntityFramework/IncludeAppender.cs b/src/GraphQL.EntityFramework/IncludeAppender.cs index fafd1126..581ab779 100644 --- a/src/GraphQL.EntityFramework/IncludeAppender.cs +++ b/src/GraphQL.EntityFramework/IncludeAppender.cs @@ -24,86 +24,23 @@ public IQueryable AddIncludesWithFilters( IQueryable query, IResolveFieldContext context, IReadOnlyDictionary>? allFilterFields) - where TItem : class - { - var (resultQuery, _) = AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); - return resultQuery; - } + where TItem : class => + AddIncludesWithFiltersAndDetectNavigations(query, context); - internal (IQueryable query, bool hasAbstractFilterNavigations) AddIncludesWithFiltersAndDetectNavigations( + internal IQueryable AddIncludesWithFiltersAndDetectNavigations( IQueryable query, - IResolveFieldContext context, - IReadOnlyDictionary>? allFilterFields) + IResolveFieldContext context) where TItem : class { - // First add includes from GraphQL query + // Add includes from GraphQL query query = AddIncludes(query, context); - // Then add includes for filter-required navigations - var hasAbstractFilterNavigations = false; - if (allFilterFields is { Count: > 0 }) - { - var type = typeof(TItem); - if (navigations.TryGetValue(type, out var navigationProperties)) - { - (query, hasAbstractFilterNavigations) = AddFilterIncludes(query, allFilterFields, type, navigationProperties); - } - } - - return (query, hasAbstractFilterNavigations); - } - - static (IQueryable query, bool hasFilterNavigations) AddFilterIncludes( - IQueryable query, - IReadOnlyDictionary> allFilterFields, - Type entityType, - IReadOnlyDictionary navigationProperties) - where TItem : class - { - // Get filter fields for this entity type and its base types - var relevantFilterFields = new HashSet(StringComparer.OrdinalIgnoreCase); - foreach (var (filterType, filterFields) in allFilterFields) - { - if (filterType.IsAssignableFrom(entityType)) - { - foreach (var field in filterFields) - { - relevantFilterFields.Add(field); - } - } - } - - if (relevantFilterFields.Count == 0) - { - return (query, false); - } + // Note: Filter-required navigations are now handled entirely by the projection system + // in TryGetProjectionExpressionWithFilters, which builds projections that include + // filter-required fields. Abstract navigation access is prevented by FilterEntry validation. + // No additional includes are needed here. - // Extract navigation names from filter fields (e.g., "TravelRequest.GroupOwnerId" -> "TravelRequest") - var filterNavigations = new HashSet(StringComparer.OrdinalIgnoreCase); - foreach (var field in relevantFilterFields) - { - if (field.Contains('.')) - { - var navName = field.Split('.', 2)[0]; - filterNavigations.Add(navName); - } - } - - // Add Include for each filter-required navigation - var hasAbstractNavigations = false; - foreach (var navName in filterNavigations) - { - if (navigationProperties.TryGetValue(navName, out var navMetadata)) - { - query = query.Include(navName); - if (navMetadata.Type.IsAbstract) - { - hasAbstractNavigations = true; - } - } - } - - return (query, hasAbstractNavigations); + return query; } public FieldProjectionInfo? GetProjection(IResolveFieldContext context) @@ -273,17 +210,16 @@ FieldProjectionInfo MergeFilterFieldsIntoProjection( Projection = updatedProjection }; } - else if (!navType.IsAbstract) + else { - // Create navigation projection for filter-only navigations (unless abstract) - // Abstract navigations are handled via AddIncludesWithFilters() which uses EF Include + // Create navigation projection for filter-only navigations + // Note: For abstract types, we still create the projection here + // If SelectExpressionBuilder can't handle it, TryBuild will return false + // and the Include added by AddFilterIncludes will be used instead var navProjection = new FieldProjectionInfo(requiredProps, null, null, null); navProjection = MergeFilterFieldsIntoProjection(navProjection, allFilterFields, navType); mergedNavigations[navName] = new(navType, navMetadata.IsCollection, navProjection); } - // Note: Abstract filter-required navigations are handled via AddIncludesWithFilters() which uses EF Include. - // We skip creating projections for them to prevent SelectExpressionBuilder from failing, - // which would cause it to fall back to loading all entity properties. } // Recursively process existing navigations @@ -559,8 +495,7 @@ void ProcessProjectionExpression( // Add any specific fields from the projection expression foreach (var nestedPath in nestedPaths) { - if (!nestedPath.Contains('.') && - !nestedProjection.ScalarFields.Contains(nestedPath)) + if (!nestedPath.Contains('.')) { nestedProjection.ScalarFields.Add(nestedPath); } diff --git a/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs b/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs index a0604422..24656a05 100644 --- a/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs +++ b/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs @@ -142,6 +142,7 @@ public static bool TryBuild( var navType = navProjection.EntityType; // Can't create NewExpression for abstract types + // (EF Core can't compile "new AbstractType { ... }") if (navType.IsAbstract) { return null; @@ -160,7 +161,7 @@ public static bool TryBuild( var innerMemberInit = Expression.MemberInit(navMetadata.NewInstance, innerBindings); var innerLambda = Expression.Lambda(innerMemberInit, navParam); - // Build: x.Children.OrderBy(n => n.Key) to ensure deterministic ordering + // Build: x.Children.OrderBy(_ => _.Key) to ensure deterministic ordering Expression orderedCollection = navAccess; if (keyNames.TryGetValue(navType, out var keys) && keys.Count > 0) { @@ -190,6 +191,7 @@ public static bool TryBuild( var navType = navProjection.EntityType; // Can't create NewExpression for abstract types + // (EF Core can't compile "new AbstractType { ... }") if (navType.IsAbstract) { return null; @@ -333,7 +335,7 @@ static bool TryBuildNestedNavigationBinding( var innerMemberInit = Expression.MemberInit(navMetadata.NewInstance, innerBindings); var innerLambda = Expression.Lambda(innerMemberInit, navParam); - // .OrderBy(o => o.Key) to ensure deterministic ordering + // .OrderBy(_ => _.Key) to ensure deterministic ordering Expression orderedCollection = navAccess; if (keyNames.TryGetValue(navType, out var keys) && keys.Count > 0) { @@ -346,7 +348,7 @@ static bool TryBuildNestedNavigationBinding( } } - // .Select(n => new Child { ... }) + // .Select(_ => new Child { ... }) var selectCall = Expression.Call(null, navMetadata.SelectMethod, orderedCollection, innerLambda); // .ToList() diff --git a/src/Tests/IntegrationTests/Graphs/Filtering/FilterBaseEntity.cs b/src/Tests/IntegrationTests/Graphs/Filtering/FilterBaseEntity.cs new file mode 100644 index 00000000..093af5e1 --- /dev/null +++ b/src/Tests/IntegrationTests/Graphs/Filtering/FilterBaseEntity.cs @@ -0,0 +1,17 @@ +public class FilterBaseEntity +{ + public Guid Id { get; set; } = Guid.NewGuid(); + public string? CommonProperty { get; set; } + + // Many additional properties to simulate a "rich" entity + public string? Field1 { get; set; } + public string? Field2 { get; set; } + public string? Field3 { get; set; } + public int Field4 { get; set; } + public int Field5 { get; set; } + public DateTime? Field6 { get; set; } + public DateTime? Field7 { get; set; } + public bool Field8 { get; set; } + public bool Field9 { get; set; } + public Guid? Field10 { get; set; } +} diff --git a/src/Tests/IntegrationTests/Graphs/Filtering/FilterBaseGraphType.cs b/src/Tests/IntegrationTests/Graphs/Filtering/FilterBaseGraphType.cs new file mode 100644 index 00000000..995c6c32 --- /dev/null +++ b/src/Tests/IntegrationTests/Graphs/Filtering/FilterBaseGraphType.cs @@ -0,0 +1,7 @@ +public class FilterBaseGraphType : + EfObjectGraphType +{ + public FilterBaseGraphType(IEfGraphQLService graphQlService) : + base(graphQlService) => + AutoMap(); +} diff --git a/src/Tests/IntegrationTests/Graphs/Filtering/FilterDerivedEntity.cs b/src/Tests/IntegrationTests/Graphs/Filtering/FilterDerivedEntity.cs new file mode 100644 index 00000000..5cb9e356 --- /dev/null +++ b/src/Tests/IntegrationTests/Graphs/Filtering/FilterDerivedEntity.cs @@ -0,0 +1,5 @@ +public class FilterDerivedEntity : FilterBaseEntity +{ + public string? DerivedProperty { get; set; } + public IList References { get; set; } = []; +} diff --git a/src/Tests/IntegrationTests/Graphs/Filtering/FilterDerivedGraphType.cs b/src/Tests/IntegrationTests/Graphs/Filtering/FilterDerivedGraphType.cs new file mode 100644 index 00000000..62423392 --- /dev/null +++ b/src/Tests/IntegrationTests/Graphs/Filtering/FilterDerivedGraphType.cs @@ -0,0 +1,7 @@ +public class FilterDerivedGraphType : + EfObjectGraphType +{ + public FilterDerivedGraphType(IEfGraphQLService graphQlService) : + base(graphQlService) => + AutoMap(); +} diff --git a/src/Tests/IntegrationTests/Graphs/Filtering/FilterParentEntity.cs b/src/Tests/IntegrationTests/Graphs/Filtering/FilterParentEntity.cs index d0784eab..942d3b3c 100644 --- a/src/Tests/IntegrationTests/Graphs/Filtering/FilterParentEntity.cs +++ b/src/Tests/IntegrationTests/Graphs/Filtering/FilterParentEntity.cs @@ -2,5 +2,18 @@ { public Guid Id { get; set; } = Guid.NewGuid(); public string? Property { get; set; } + + // Additional properties to simulate a "rich" entity like TravelRequest + public string? Field1 { get; set; } + public string? Field2 { get; set; } + public string? Field3 { get; set; } + public int Field4 { get; set; } + public int Field5 { get; set; } + public DateTime? Field6 { get; set; } + public DateTime? Field7 { get; set; } + public bool Field8 { get; set; } + public bool Field9 { get; set; } + public Guid? Field10 { get; set; } + public IList Children { get; set; } = []; } \ No newline at end of file diff --git a/src/Tests/IntegrationTests/Graphs/Filtering/FilterReferenceEntity.cs b/src/Tests/IntegrationTests/Graphs/Filtering/FilterReferenceEntity.cs new file mode 100644 index 00000000..8ca02d6a --- /dev/null +++ b/src/Tests/IntegrationTests/Graphs/Filtering/FilterReferenceEntity.cs @@ -0,0 +1,9 @@ +public class FilterReferenceEntity +{ + public Guid Id { get; set; } = Guid.NewGuid(); + public string? Property { get; set; } + + // Navigation to base type (like Accommodation -> TravelRequest) + public Guid BaseEntityId { get; set; } + public FilterBaseEntity? BaseEntity { get; set; } +} diff --git a/src/Tests/IntegrationTests/Graphs/Filtering/FilterReferenceGraphType.cs b/src/Tests/IntegrationTests/Graphs/Filtering/FilterReferenceGraphType.cs new file mode 100644 index 00000000..7471d493 --- /dev/null +++ b/src/Tests/IntegrationTests/Graphs/Filtering/FilterReferenceGraphType.cs @@ -0,0 +1,7 @@ +public class FilterReferenceGraphType : + EfObjectGraphType +{ + public FilterReferenceGraphType(IEfGraphQLService graphQlService) : + base(graphQlService) => + AutoMap(); +} diff --git a/src/Tests/IntegrationTests/IntegrationDbContext.cs b/src/Tests/IntegrationTests/IntegrationDbContext.cs index 72769a8c..b369eb4b 100644 --- a/src/Tests/IntegrationTests/IntegrationDbContext.cs +++ b/src/Tests/IntegrationTests/IntegrationDbContext.cs @@ -40,6 +40,9 @@ protected override void OnConfiguring(DbContextOptionsBuilder builder) => public DbSet FieldBuilderProjectionParentEntities { get; set; } = null!; public DbSet Departments { get; set; } = null!; public DbSet Employees { get; set; } = null!; + public DbSet FilterBaseEntities { get; set; } = null!; + public DbSet FilterDerivedEntities { get; set; } = null!; + public DbSet FilterReferenceEntities { get; set; } = null!; protected override void OnModelCreating(ModelBuilder modelBuilder) { @@ -112,5 +115,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) .WithMany(_ => _.Employees) .HasForeignKey(_ => _.DepartmentId) .OnDelete(DeleteBehavior.Restrict); + + // Configure TPH inheritance for FilterBaseEntity -> FilterDerivedEntity + modelBuilder.Entity() + .OrderBy(_ => _.CommonProperty); + modelBuilder.Entity() + .HasBaseType(); + modelBuilder.Entity() + .OrderBy(_ => _.Property); } } diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_projection_should_not_select_all_navigation_fields.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_projection_should_not_select_all_navigation_fields.verified.txt new file mode 100644 index 00000000..69d7c350 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_projection_should_not_select_all_navigation_fields.verified.txt @@ -0,0 +1,21 @@ +{ + target: +{ + "data": { + "filterChildEntity": { + "id": "Guid_1" + } + } +}, + sql: { + Text: +select top (2) f.Id, + case when f0.Id is null then cast (1 as bit) else cast (0 as bit) end, + f0.Property +from FilterChildEntities as f + left outer join + FilterParentEntities as f0 + on f.ParentId = f0.Id +where f.Id = 'Guid_1' + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_projection_with_TPH_inheritance_should_not_select_all_fields.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_projection_with_TPH_inheritance_should_not_select_all_fields.verified.txt new file mode 100644 index 00000000..923918f4 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_projection_with_TPH_inheritance_should_not_select_all_fields.verified.txt @@ -0,0 +1,23 @@ +{ + target: +{ + "data": { + "filterReferenceEntity": { + "id": "Guid_1" + } + } +}, + sql: { + Text: +select top (2) f.Id, + f.BaseEntityId, + cast (0 as bit), + f0.CommonProperty, + f0.Id +from FilterReferenceEntities as f + inner join + FilterBaseEntities as f0 + on f.BaseEntityId = f0.Id +where f.Id = 'Guid_1' + } +} \ 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 2b18b883..cc70bd8a 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.SchemaPrint.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.SchemaPrint.verified.txt @@ -151,6 +151,10 @@ fieldBuilderProjectionParents(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [FieldBuilderProjectionParent!]! departments(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [Department!]! employees(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [Employee!]! + filterChildEntities(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [FilterChild!]! + filterChildEntity(id: ID, ids: [ID!], where: [WhereExpression!]): FilterChild! + filterReferenceEntities(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [FilterReference!]! + filterReferenceEntity(id: ID, ids: [ID!], where: [WhereExpression!]): FilterReference! } type CustomType { @@ -412,6 +416,16 @@ type FilterParent { orderBy: [OrderBy!], ids: [ID!]): FilterChildConnection! children(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [FilterChild!]! + field1: String + field10: ID + field2: String + field3: String + field4: Int! + field5: Int! + field6: DateTime + field7: DateTime + field8: Boolean! + field9: Boolean! id: ID! property: String } @@ -710,6 +724,28 @@ type Employee { name: String } +type FilterReference { + baseEntity: FilterBase + baseEntityId: ID! + id: ID! + property: String +} + +type FilterBase { + commonProperty: String + field1: String + field10: ID + field2: String + field3: String + field4: Int! + field5: Int! + field6: DateTime + field7: DateTime + field8: Boolean! + field9: Boolean! + id: ID! +} + type Mutation { parentEntityMutation(id: ID, ids: [ID!], where: [WhereExpression!]): Parent! } @@ -745,3 +781,20 @@ type DerivedWithNavigation implements BaseEntity { property: String status: String } + +type FilterDerived { + references(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [FilterReference!]! + commonProperty: String + derivedProperty: String + field1: String + field10: ID + field2: String + field3: String + field4: Int! + field5: Int! + field6: DateTime + field7: DateTime + field8: Boolean! + field9: Boolean! + id: ID! +} diff --git a/src/Tests/IntegrationTests/IntegrationTests_abstract_filter_validation.cs b/src/Tests/IntegrationTests/IntegrationTests_abstract_filter_validation.cs new file mode 100644 index 00000000..45c825bf --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests_abstract_filter_validation.cs @@ -0,0 +1,125 @@ +public partial class IntegrationTests +{ + /// + /// Test that explicit projection accessing properties through abstract navigation throws. + /// DerivedChildEntity.Parent is BaseEntity (abstract), so projecting Parent.Property + /// should throw because abstract types cannot be projected by EF Core. + /// + [Fact] + public void Explicit_projection_with_abstract_navigation_property_throws() + { + var filters = new Filters(); + + var exception = Assert.Throws(() => + { + // Projection accesses Parent.Property where Parent is BaseEntity (abstract) + filters.For().Add( + projection: c => c.Parent!.Property, + filter: (_, _, _, prop) => prop == "test"); + }); + + Assert.Contains("abstract navigation", exception.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("Parent", exception.Message); + } + + /// + /// Test that anonymous type projection with abstract navigation also throws. + /// Even with anonymous types, we cannot project from abstract navigations. + /// + [Fact] + public void Anonymous_type_projection_with_abstract_navigation_throws() + { + var filters = new Filters(); + + var exception = Assert.Throws(() => + { + // Even with anonymous type, projecting from abstract Parent throws + filters.For().Add( + projection: c => new { c.Id, ParentProperty = c.Parent!.Property }, + filter: (_, _, _, proj) => proj.ParentProperty == "test"); + }); + + Assert.Contains("abstract navigation", exception.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("Parent", exception.Message); + } + + /// + /// Test that explicit projection with concrete (non-abstract) navigation succeeds. + /// DerivedChildEntity.TypedParent is DerivedWithNavigationEntity (concrete), + /// so projection through it should be allowed. + /// + [Fact] + public void Explicit_projection_with_concrete_navigation_succeeds() + { + var filters = new Filters(); + + // DerivedWithNavigationEntity is concrete - should work fine + filters.For().Add( + projection: c => c.TypedParent!.Property, + filter: (_, _, _, prop) => prop == "test"); + + // Verify the filter was added successfully + var requiredProps = filters.GetRequiredFilterProperties(); + Assert.Contains("TypedParent.Property", requiredProps); + } + + /// + /// Test that identity projection without navigation access succeeds. + /// Identity projections are fine when the filter only accesses scalar properties. + /// Runtime validation doesn't analyze the filter, so this succeeds. + /// + [Fact] + public void Identity_projection_without_navigation_succeeds() + { + var filters = new Filters(); + + // Identity projection with filter that only accesses scalars + // (Runtime doesn't validate filter, so this is allowed) + filters.For().Add( + projection: c => c, + filter: (_, _, _, c) => c is { Property: "test", ParentId: not null }); + + // For identity projection, RequiredPropertyNames is empty + var requiredProps = filters.GetRequiredFilterProperties(); + Assert.Empty(requiredProps); + } + + /// + /// Test that the 4-parameter filter syntax (which uses identity projection internally) + /// does NOT throw at runtime. The analyzer will catch filter accesses at compile time. + /// Runtime validation only catches explicit projections through abstract navigations. + /// + [Fact] + public void Four_parameter_filter_with_abstract_navigation_allowed_at_runtime() + { + var filters = new Filters(); + + // Runtime doesn't analyze the filter body, so this is allowed + // (The analyzer will catch this at compile time) + filters.For().Add( + filter: (_, _, _, c) => c.ParentId != null); + + var requiredProps = filters.GetRequiredFilterProperties(); + Assert.Empty(requiredProps); // Identity projection extracts no properties + } + + /// + /// Test projecting from nested abstract navigation property. + /// + [Fact] + public void Projection_with_nested_abstract_navigation_property_throws() + { + var filters = new Filters(); + + var exception = Assert.Throws(() => + { + // Accessing nested property through abstract parent + filters.For().Add( + projection: c => c.Parent!.Status, + filter: (_, _, _, status) => status == "Active"); + }); + + Assert.Contains("abstract navigation", exception.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("Parent", exception.Message); + } +} diff --git a/src/Tests/IntegrationTests/IntegrationTests_named_type_filter_projection.cs b/src/Tests/IntegrationTests/IntegrationTests_named_type_filter_projection.cs index dfc8876f..0323b58c 100644 --- a/src/Tests/IntegrationTests/IntegrationTests_named_type_filter_projection.cs +++ b/src/Tests/IntegrationTests/IntegrationTests_named_type_filter_projection.cs @@ -102,7 +102,144 @@ public async Task Filter_projection_with_abstract_navigation_should_use_include( await RunQuery(database, query, null, filters, false, [parent, child1, child2]); } + /// + /// BUG TEST: Verifies that filter projections with navigation property access + /// should only SELECT the required fields from the navigation, not all fields. + /// + /// This reproduces the bug where EntityFilters cause Include() to be added, + /// which loads ALL columns from the navigation entity instead of just the + /// filter-required columns. + /// + /// Expected behavior: SQL should only select Parent.Property (required by filter) + /// and Parent.Id (primary key), not all the extra fields (Field1-Field10). + /// + /// Actual behavior (BUG): SQL includes ALL parent fields because Include() is used. + /// + [Fact] + public async Task Filter_projection_should_not_select_all_navigation_fields() + { + var query = + """ + { + filterChildEntity(id: "{id}") + { + id + } + } + """; + + var parent = new FilterParentEntity + { + Property = "AllowedParent", + Field1 = "Extra1", + Field2 = "Extra2", + Field3 = "Extra3", + Field4 = 100, + Field5 = 200, + Field6 = DateTime.UtcNow, + Field7 = DateTime.UtcNow, + Field8 = true, + Field9 = false, + Field10 = Guid.NewGuid() + }; + var child = new FilterChildEntity + { + Property = "Child1", + Parent = parent + }; + + query = query.Replace("{id}", child.Id.ToString()); + + var filters = new Filters(); + + // Filter only accesses Parent.Property - should NOT load all parent fields + filters.For().Add( + projection: c => new FilterChildProjection( + c.Parent != null ? c.Parent.Property : null, + c.Id), + filter: (_, _, _, projection) => projection.ParentProperty == "AllowedParent"); + + await using var database = await sqlInstance.Build(); + + // BUG: The SQL will include Parent.Field1, Parent.Field2, etc. even though + // only Parent.Property is needed by the filter + await RunQuery(database, query, null, filters, false, [parent, child]); + } + + /// + /// BUG TEST: Reproduces the bug where TPH inheritance + entity filters on base type + /// cause ALL columns to be selected from the navigation entity. + /// + /// This matches the MinistersApi scenario: + /// - FilterReferenceEntity (like Accommodation) has a navigation to FilterBaseEntity + /// - FilterBaseEntity (like BaseRequest) is an abstract base with TPH inheritance + /// - FilterDerivedEntity (like TravelRequest) inherits from FilterBaseEntity + /// - Both FilterReferenceEntity and FilterBaseEntity have entity filters + /// + /// Expected: SQL should only select BaseEntity.CommonProperty (required by filter) + /// Actual (BUG): SQL includes ALL fields (Field1-Field10) due to Include() being used + /// + [Fact] + public async Task Filter_projection_with_TPH_inheritance_should_not_select_all_fields() + { + var query = + """ + { + filterReferenceEntity(id: "{id}") + { + id + } + } + """; + + var derived = new FilterDerivedEntity + { + CommonProperty = "Allowed", + DerivedProperty = "Derived1", + Field1 = "Extra1", + Field2 = "Extra2", + Field3 = "Extra3", + Field4 = 100, + Field5 = 200, + Field6 = DateTime.UtcNow, + Field7 = DateTime.UtcNow, + Field8 = true, + Field9 = false, + Field10 = Guid.NewGuid() + }; + var reference = new FilterReferenceEntity + { + Property = "Reference1", + BaseEntity = derived + }; + + query = query.Replace("{id}", reference.Id.ToString()); + + var filters = new Filters(); + + // Filter on FilterReferenceEntity accesses BaseEntity.CommonProperty + filters.For().Add( + projection: r => new FilterReferenceProjection( + r.BaseEntity != null ? r.BaseEntity.CommonProperty : null, + r.Id), + filter: (_, _, _, proj) => proj.BaseCommonProperty == "Allowed"); + + // Filter on base type (like BaseRequest in MinistersApi) + filters.For().Add( + projection: b => new FilterBaseProjection(b.CommonProperty, b.Id), + filter: (_, _, _, proj) => proj.CommonProperty != null); + + await using var database = await sqlInstance.Build(); + + // BUG: The SQL will include a subquery selecting ALL fields from FilterBaseEntity + // (Field1-Field10) even though only CommonProperty is needed + await RunQuery(database, query, null, filters, false, [derived, reference]); + } + // ReSharper disable NotAccessedPositionalProperty.Local record ChildFilterProjection(Guid? ParentId, string? ParentProperty, Guid ChildId); record AbstractNavFilterProjection(Guid? ParentId, string? ParentProperty, Guid ChildId); + record FilterChildProjection(string? ParentProperty, Guid ChildId); + record FilterReferenceProjection(string? BaseCommonProperty, Guid Id); + record FilterBaseProjection(string? CommonProperty, Guid Id); } diff --git a/src/Tests/IntegrationTests/Query.cs b/src/Tests/IntegrationTests/Query.cs index 9957846e..2000c0e5 100644 --- a/src/Tests/IntegrationTests/Query.cs +++ b/src/Tests/IntegrationTests/Query.cs @@ -336,5 +336,21 @@ public Query(IEfGraphQLService efGraphQlService) AddQueryField( name: "employees", resolve: _ => _.DbContext.Employees); + + AddQueryField( + name: "filterChildEntities", + resolve: _ => _.DbContext.FilterChildEntities); + + AddSingleField( + name: "filterChildEntity", + resolve: _ => _.DbContext.FilterChildEntities); + + AddQueryField( + name: "filterReferenceEntities", + resolve: _ => _.DbContext.FilterReferenceEntities); + + AddSingleField( + name: "filterReferenceEntity", + resolve: _ => _.DbContext.FilterReferenceEntities); } } diff --git a/src/Tests/IntegrationTests/Schema.cs b/src/Tests/IntegrationTests/Schema.cs index 216da019..0988ca31 100644 --- a/src/Tests/IntegrationTests/Schema.cs +++ b/src/Tests/IntegrationTests/Schema.cs @@ -39,9 +39,13 @@ public Schema(IServiceProvider resolver) : RegisterTypeMapping(typeof(FieldBuilderProjectionParentEntity), typeof(FieldBuilderProjectionParentGraphType)); RegisterTypeMapping(typeof(DepartmentEntity), typeof(DepartmentGraphType)); RegisterTypeMapping(typeof(EmployeeEntity), typeof(EmployeeGraphType)); + RegisterTypeMapping(typeof(FilterReferenceEntity), typeof(FilterReferenceGraphType)); + RegisterTypeMapping(typeof(FilterBaseEntity), typeof(FilterBaseGraphType)); + RegisterTypeMapping(typeof(FilterDerivedEntity), typeof(FilterDerivedGraphType)); Query = (Query)resolver.GetService(typeof(Query))!; Mutation = (Mutation)resolver.GetService(typeof(Mutation))!; RegisterType(typeof(DerivedGraphType)); RegisterType(typeof(DerivedWithNavigationGraphType)); + RegisterType(typeof(FilterDerivedGraphType)); } } \ No newline at end of file diff --git a/src/Tests/UnitTests/FilterEntryValidationTests.cs b/src/Tests/UnitTests/FilterEntryValidationTests.cs new file mode 100644 index 00000000..9cf21089 --- /dev/null +++ b/src/Tests/UnitTests/FilterEntryValidationTests.cs @@ -0,0 +1,93 @@ +public class FilterEntryValidationTests +{ + class TestEntity + { + public Guid Id { get; set; } + public string? Property { get; set; } + public Guid? ParentId { get; set; } + public AbstractParent? Parent { get; set; } + public ConcreteParent? ConcreteParent { get; set; } + } + + abstract class AbstractParent + { + public Guid Id { get; set; } + public string? Property { get; set; } + } + + class ConcreteParent + { + public Guid Id { get; set; } + public string? Property { get; set; } + } + + class TestDbContext : DbContext; + + [Fact] + public void Explicit_projection_with_abstract_navigation_property_throws() + { + var filters = new Filters(); + + // This should throw - projection directly accesses abstract navigation property + var exception = Assert.Throws(() => + { + filters.For().Add( + projection: e => e.Parent!.Property, + filter: (_, _, _, prop) => prop == "test"); + }); + + Assert.Contains("abstract navigation", exception.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("Parent", exception.Message); + } + + [Fact] + public void Concrete_navigation_allows_identity_projection() + { + var filters = new Filters(); + + // Should not throw - concrete navigation + filters.For().Add( + projection: e => e, + filter: (_, _, _, e) => e.ConcreteParent!.Property == "test"); + } + + [Fact] + public void Anonymous_type_projection_with_abstract_navigation_throws() + { + var filters = new Filters(); + + // This should also throw - even with anonymous type, we're projecting from abstract navigation + var exception = Assert.Throws(() => + { + filters.For().Add( + projection: e => new { e.Id, ParentProp = e.Parent!.Property }, + filter: (_, _, _, proj) => proj.ParentProp == "test"); + }); + + Assert.Contains("abstract navigation", exception.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("Parent", exception.Message); + } + + [Fact] + public void Projection_without_abstract_navigation_succeeds() + { + var filters = new Filters(); + + // Should not throw - only accessing scalar properties + filters.For().Add( + projection: e => new { e.Id, e.Property }, + filter: (_, _, _, proj) => proj.Property == "test"); + } + + [Fact] + public void Identity_projection_without_navigation_succeeds() + { + var filters = new Filters(); + + // Should not throw - identity projection is fine if not accessing navigations + // (the analyzer will catch if the filter accesses navigations) + filters.For().Add( + projection: e => e, + filter: (_, _, _, e) => e.Property == "test"); + } +}