diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 0ba60f39..dfbcc17c 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -2,7 +2,7 @@ CS1591;NU5104;CS1573;CS9107;NU1608;NU1109 - 34.1.6 + 34.1.7 preview 1.0.0 EntityFrameworkCore, EntityFramework, GraphQL diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs index ddf49a9f..cbd91391 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs @@ -166,7 +166,7 @@ FieldType BuildFirstField( // Get filter-required fields early so we can add filter-required navigations via Include var allFilterFields = fieldContext.Filters?.GetAllRequiredFilterProperties(); - query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context); + query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); query = query.ApplyGraphQlArguments(context, names, false, omitQueryArguments); // Apply column projection based on requested GraphQL fields diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs index 16f23071..33397f33 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs @@ -112,7 +112,7 @@ FieldType BuildQueryField( // Get filter-required fields early so we can add filter-required navigations via Include var allFilterFields = fieldContext.Filters?.GetAllRequiredFilterProperties(); - query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context); + query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); if (!omitQueryArguments) { query = query.ApplyGraphQlArguments(context, names, true, omitQueryArguments); diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs index 595830af..28ff9c8e 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs @@ -158,7 +158,7 @@ ConnectionBuilder AddQueryableConnection( // Get filter-required fields early so we can add filter-required navigations via Include var allFilterFields = fieldContext.Filters?.GetAllRequiredFilterProperties(); - query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context); + query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); query = query.ApplyGraphQlArguments(context, names, true, omitQueryArguments); // Apply column projection based on requested GraphQL fields diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs index f6fe30be..fcc2bee1 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs @@ -166,7 +166,7 @@ FieldType BuildSingleField( // Get filter-required fields early so we can add filter-required navigations via Include var allFilterFields = fieldContext.Filters?.GetAllRequiredFilterProperties(); - query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context); + query = includeAppender.AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); query = query.ApplyGraphQlArguments(context, names, false, omitQueryArguments); // Apply column projection based on requested GraphQL fields diff --git a/src/GraphQL.EntityFramework/IncludeAppender.cs b/src/GraphQL.EntityFramework/IncludeAppender.cs index 581ab779..68c4f121 100644 --- a/src/GraphQL.EntityFramework/IncludeAppender.cs +++ b/src/GraphQL.EntityFramework/IncludeAppender.cs @@ -25,20 +25,94 @@ public IQueryable AddIncludesWithFilters( IResolveFieldContext context, IReadOnlyDictionary>? allFilterFields) where TItem : class => - AddIncludesWithFiltersAndDetectNavigations(query, context); + AddIncludesWithFiltersAndDetectNavigations(query, context, allFilterFields); internal IQueryable AddIncludesWithFiltersAndDetectNavigations( IQueryable query, - IResolveFieldContext context) + IResolveFieldContext context, + IReadOnlyDictionary>? allFilterFields = null) where TItem : class { // Add includes from GraphQL query query = AddIncludes(query, context); - // 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. + // Add includes for filter-required navigations. + // While projection handles most cases, abstract navigation types cannot be projected + // (can't do "new AbstractType { ... }"). In those cases, Include is needed as fallback. + if (allFilterFields is { Count: > 0 }) + { + query = AddFilterNavigationIncludes(query, allFilterFields, typeof(TItem)); + } + + return query; + } + + IQueryable AddFilterNavigationIncludes( + IQueryable query, + IReadOnlyDictionary> allFilterFields, + Type entityType) + where TItem : class + { + // Get filter fields for this entity type and its base types + var relevantFilterFields = new List(); + foreach (var (filterType, filterFields) in allFilterFields) + { + if (filterType.IsAssignableFrom(entityType)) + { + relevantFilterFields.AddRange(filterFields); + } + } + + if (relevantFilterFields.Count == 0) + { + return query; + } + + // Get navigation metadata for this entity type + if (!navigations.TryGetValue(entityType, out var navigationProperties)) + { + return query; + } + + // Extract unique navigation names from filter fields (paths like "TravelRequest.Status" -> "TravelRequest") + var navigationNames = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var field in relevantFilterFields) + { + if (field.Contains('.')) + { + var navName = field[..field.IndexOf('.')]; + navigationNames.Add(navName); + } + } + + // Add Include for each filter-required navigation that has an abstract type + // (Non-abstract navigations can be handled by projection) + foreach (var navName in navigationNames) + { + // Find navigation in metadata (case-insensitive) + Navigation? navMetadata = null; + string? actualNavName = null; + foreach (var (key, value) in navigationProperties) + { + if (string.Equals(key, navName, StringComparison.OrdinalIgnoreCase)) + { + navMetadata = value; + actualNavName = key; + break; + } + } + + if (navMetadata == null || actualNavName == null) + { + continue; + } + + // Only add Include for abstract types - concrete types can use projection + if (navMetadata.Type.IsAbstract) + { + query = query.Include(navMetadata.Name); + } + } return query; } diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_combining_abstract_and_concrete_navigations.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_combining_abstract_and_concrete_navigations.verified.txt new file mode 100644 index 00000000..ad0c7806 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_combining_abstract_and_concrete_navigations.verified.txt @@ -0,0 +1,29 @@ +{ + target: +{ + "data": { + "derivedChildEntities": [ + { + "id": "Guid_1", + "property": "Child1" + } + ] + } +}, + sql: { + Text: +select d.Id, + d.ParentId, + d.Property, + d.TypedParentId, + b.Id, + b.Discriminator, + b.Property, + b.Status +from DerivedChildEntities as d + left outer join + BaseEntities as b + on d.ParentId = b.Id +order by d.Property + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_with_abstract_navigation_should_correctly_exclude_non_matching.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_abstract_navigation_should_correctly_exclude_non_matching.verified.txt new file mode 100644 index 00000000..fdfead17 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_abstract_navigation_should_correctly_exclude_non_matching.verified.txt @@ -0,0 +1,29 @@ +{ + target: +{ + "data": { + "derivedChildEntities": [ + { + "id": "Guid_1", + "property": "Child1-Approved" + } + ] + } +}, + sql: { + Text: +select d.Id, + d.ParentId, + d.Property, + d.TypedParentId, + b.Id, + b.Discriminator, + b.Property, + b.Status +from DerivedChildEntities as d + left outer join + BaseEntities as b + on d.ParentId = b.Id +order by d.Property + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_with_abstract_navigation_should_use_include_fallback.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_abstract_navigation_should_use_include_fallback.verified.txt new file mode 100644 index 00000000..ad0c7806 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_abstract_navigation_should_use_include_fallback.verified.txt @@ -0,0 +1,29 @@ +{ + target: +{ + "data": { + "derivedChildEntities": [ + { + "id": "Guid_1", + "property": "Child1" + } + ] + } +}, + sql: { + Text: +select d.Id, + d.ParentId, + d.Property, + d.TypedParentId, + b.Id, + b.Discriminator, + b.Property, + b.Status +from DerivedChildEntities as d + left outer join + BaseEntities as b + on d.ParentId = b.Id +order by d.Property + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_with_multiple_properties_from_abstract_navigation.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_multiple_properties_from_abstract_navigation.verified.txt new file mode 100644 index 00000000..ad0c7806 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_multiple_properties_from_abstract_navigation.verified.txt @@ -0,0 +1,29 @@ +{ + target: +{ + "data": { + "derivedChildEntities": [ + { + "id": "Guid_1", + "property": "Child1" + } + ] + } +}, + sql: { + Text: +select d.Id, + d.ParentId, + d.Property, + d.TypedParentId, + b.Id, + b.Discriminator, + b.Property, + b.Status +from DerivedChildEntities as d + left outer join + BaseEntities as b + on d.ParentId = b.Id +order by d.Property + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_with_null_abstract_navigation_should_handle_gracefully.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_null_abstract_navigation_should_handle_gracefully.verified.txt new file mode 100644 index 00000000..14683746 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_null_abstract_navigation_should_handle_gracefully.verified.txt @@ -0,0 +1,29 @@ +{ + target: +{ + "data": { + "derivedChildEntities": [ + { + "id": "Guid_1", + "property": "ChildWithParent" + } + ] + } +}, + sql: { + Text: +select d.Id, + d.ParentId, + d.Property, + d.TypedParentId, + b.Id, + b.Discriminator, + b.Property, + b.Status +from DerivedChildEntities as d + left outer join + BaseEntities as b + on d.ParentId = b.Id +order by d.Property + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests_abstract_navigation_filter_include.cs b/src/Tests/IntegrationTests/IntegrationTests_abstract_navigation_filter_include.cs new file mode 100644 index 00000000..1d15edd2 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests_abstract_navigation_filter_include.cs @@ -0,0 +1,333 @@ +/// +/// Tests for the fix where filter projections accessing properties through abstract navigation types +/// should use Include as a fallback when projection can't be built (because you can't do "new AbstractType { ... }"). +/// +/// Bug scenario eg: +/// - Aircraft has TravelRequest navigation (TravelRequest extends abstract BaseRequest) +/// - Filter projection: _ => new RequestInfo(_.TravelRequest.GroupOwnerId, _.TravelRequest.HighestStatusAchieved, ...) +/// - SelectExpressionBuilder can't build projection for abstract TravelRequest type +/// - Without Include fallback, TravelRequest is never loaded → properties are always null +/// - Fix: Add Include for filter-required abstract navigations as fallback +/// +public partial class IntegrationTests +{ + /// + /// Tests that filter projections accessing properties through abstract navigation types + /// correctly load the navigation data via Include fallback. + /// + /// Setup: + /// - DerivedChildEntity has Parent navigation of type BaseEntity (abstract) + /// - Filter projection accesses Parent.Status (like HighestStatusAchieved in MinistersApi) + /// - Filter checks if Parent.Status is a specific value + /// + /// Expected: Entity is found because Include loads Parent, allowing filter to evaluate correctly + /// Bug (before fix): Parent.Status is null because projection fails and no Include fallback + /// + [Fact] + public async Task Filter_with_abstract_navigation_should_use_include_fallback() + { + var query = + """ + { + derivedChildEntities + { + id + property + } + } + """; + + var parentId = Guid.NewGuid(); + var childId = Guid.NewGuid(); + + // Create parent with specific Status value + var parent = new DerivedEntity + { + Id = parentId, + Property = "Parent1", + Status = "Approved" // This is the value we'll filter on + }; + + // Create child that references the parent via abstract BaseEntity navigation + var child = new DerivedChildEntity + { + Id = childId, + Property = "Child1", + ParentId = parentId, + Parent = parent + }; + + var filters = new Filters(); + + // Filter projection accesses Status through abstract Parent navigation + // This mirrors the MinistersApi scenario: + // _.TravelRequest != null ? _.TravelRequest.HighestStatusAchieved : null + filters.For().Add( + projection: c => new AbstractNavStatusProjection( + c.Parent != null ? c.Parent.Status : null, + c.Id), + filter: (_, _, _, proj) => proj.ParentStatus == "Approved"); + + await using var database = await sqlInstance.Build(); + + // Before fix: Child would NOT be returned because Parent.Status is null (Include not added) + // After fix: Child IS returned because Include loads Parent, making Status available + await RunQuery(database, query, null, filters, false, [parent, child]); + } + + /// + /// Tests that filter correctly EXCLUDES entities when abstract navigation property + /// doesn't match the filter criteria. + /// + /// This verifies the Include fallback is working - if Status wasn't loaded, + /// all entities would be excluded (Status would be null, never matching "Approved"). + /// + [Fact] + public async Task Filter_with_abstract_navigation_should_correctly_exclude_non_matching() + { + var query = + """ + { + derivedChildEntities + { + id + property + } + } + """; + + var parent1Id = Guid.NewGuid(); + var parent2Id = Guid.NewGuid(); + var child1Id = Guid.NewGuid(); + var child2Id = Guid.NewGuid(); + + // Parent with Status that MATCHES filter + var parent1 = new DerivedEntity + { + Id = parent1Id, + Property = "Parent1", + Status = "Approved" + }; + + // Parent with Status that does NOT match filter + var parent2 = new DerivedEntity + { + Id = parent2Id, + Property = "Parent2", + Status = "Draft" // This should be excluded + }; + + var child1 = new DerivedChildEntity + { + Id = child1Id, + Property = "Child1-Approved", + ParentId = parent1Id, + Parent = parent1 + }; + + var child2 = new DerivedChildEntity + { + Id = child2Id, + Property = "Child2-Draft", + ParentId = parent2Id, + Parent = parent2 + }; + + var filters = new Filters(); + + // Filter should only allow entities where Parent.Status == "Approved" + filters.For().Add( + projection: c => new AbstractNavStatusProjection( + c.Parent != null ? c.Parent.Status : null, + c.Id), + filter: (_, _, _, proj) => proj.ParentStatus == "Approved"); + + await using var database = await sqlInstance.Build(); + + // Only child1 should be returned (parent has Status "Approved") + // child2 should be filtered out (parent has Status "Draft") + // Verified via snapshot - should only contain Child1-Approved + await RunQuery(database, query, null, filters, false, [parent1, parent2, child1, child2]); + } + + /// + /// Tests that filter projection with multiple properties from abstract navigation works. + /// This matches the MinistersApi scenario more closely where both GroupOwnerId and + /// HighestStatusAchieved are accessed from the abstract TravelRequest navigation. + /// + [Fact] + public async Task Filter_with_multiple_properties_from_abstract_navigation() + { + var query = + """ + { + derivedChildEntities + { + id + property + } + } + """; + + var parentId = Guid.NewGuid(); + var childId = Guid.NewGuid(); + + var parent = new DerivedEntity + { + Id = parentId, + Property = "TargetProperty", + Status = "Approved" + }; + + var child = new DerivedChildEntity + { + Id = childId, + Property = "Child1", + ParentId = parentId, + Parent = parent + }; + + var filters = new Filters(); + + // Access multiple properties from abstract Parent navigation + // (like accessing both GroupOwnerId and HighestStatusAchieved in MinistersApi) + filters.For().Add( + projection: c => new AbstractNavMultiPropertyProjection( + c.Parent != null ? c.Parent.Property : null, + c.Parent != null ? c.Parent.Status : null, + c.Id), + filter: (_, _, _, proj) => + proj is { ParentProperty: "TargetProperty", ParentStatus: "Approved" }); + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, filters, false, [parent, child]); + } + + /// + /// Tests that null parent navigation is handled correctly. + /// The filter should handle cases where the navigation is null. + /// + [Fact] + public async Task Filter_with_null_abstract_navigation_should_handle_gracefully() + { + var query = + """ + { + derivedChildEntities + { + id + property + } + } + """; + + var childWithParentId = Guid.NewGuid(); + var childWithoutParentId = Guid.NewGuid(); + var parentId = Guid.NewGuid(); + + var parent = new DerivedEntity + { + Id = parentId, + Property = "Parent1", + Status = "Approved" + }; + + var childWithParent = new DerivedChildEntity + { + Id = childWithParentId, + Property = "ChildWithParent", + ParentId = parentId, + Parent = parent + }; + + // Child with null Parent - should be excluded by filter (null status != "Approved") + var childWithoutParent = new DerivedChildEntity + { + Id = childWithoutParentId, + Property = "ChildWithoutParent", + ParentId = null, + Parent = null + }; + + var filters = new Filters(); + + filters.For().Add( + projection: c => new AbstractNavStatusProjection( + c.Parent != null ? c.Parent.Status : null, + c.Id), + filter: (_, _, _, proj) => proj.ParentStatus == "Approved"); + + await using var database = await sqlInstance.Build(); + + // Only childWithParent should be returned + // Verified via snapshot - childWithoutParent should be filtered out (null Parent) + await RunQuery(database, query, null, filters, false, [parent, childWithParent, childWithoutParent]); + } + + /// + /// Tests combination of abstract navigation filter with concrete navigation filter. + /// DerivedChildEntity has both: + /// - Parent (abstract BaseEntity) + /// - TypedParent (concrete DerivedWithNavigationEntity) + /// + [Fact] + public async Task Filter_combining_abstract_and_concrete_navigations() + { + var query = + """ + { + derivedChildEntities + { + id + property + } + } + """; + + var abstractParentId = Guid.NewGuid(); + var concreteParentId = Guid.NewGuid(); + var childId = Guid.NewGuid(); + + // Abstract parent (DerivedEntity extends BaseEntity) + var abstractParent = new DerivedEntity + { + Id = abstractParentId, + Property = "AbstractParent", + Status = "Approved" + }; + + // Concrete parent (DerivedWithNavigationEntity is concrete) + var concreteParent = new DerivedWithNavigationEntity + { + Id = concreteParentId, + Property = "ConcreteParent" + }; + + var child = new DerivedChildEntity + { + Id = childId, + Property = "Child1", + ParentId = abstractParentId, + Parent = abstractParent, + TypedParentId = concreteParentId, + TypedParent = concreteParent + }; + + var filters = new Filters(); + + // Filter on abstract navigation (should use Include fallback) + filters.For().Add( + projection: c => new AbstractNavStatusProjection( + c.Parent != null ? c.Parent.Status : null, + c.Id), + filter: (_, _, _, proj) => proj.ParentStatus == "Approved"); + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, filters, false, [abstractParent, concreteParent, child]); + } + + // Projection record types for the tests + // ReSharper disable NotAccessedPositionalProperty.Local + record AbstractNavStatusProjection(string? ParentStatus, Guid ChildId); + record AbstractNavMultiPropertyProjection(string? ParentProperty, string? ParentStatus, Guid ChildId); +}