From 2ba594f6c81f3120099c7d63182d9cea5cf2fa2d Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 2 Oct 2026 17:33:59 +1000 Subject: [PATCH] Skip null checks on required navigations and AsNoTracking before Select Verify.EntityFramework 16.0.0-beta.7 flags both patterns: - a null check on a required navigation, which EF translates to an always false comparison - AsNoTracking followed by Select, which has no effect Required-ness comes from the EF model. AsNoTracking now applies only when ApplyProjection adds no Select. --- .../Filters/FilterEntry.cs | 4 ++-- .../GraphApi/EfGraphQLService_First.cs | 7 +------ .../GraphApi/EfGraphQLService_Queryable.cs | 7 +------ .../EfGraphQLService_QueryableConnection.cs | 7 +------ .../GraphApi/EfGraphQLService_Single.cs | 7 +------ .../IncludeAppender.cs | 21 +++++++++++++------ src/GraphQL.EntityFramework/Navigation.cs | 3 ++- .../NavigationReader.cs | 10 ++++++++- .../NavigationProjectionInfo.cs | 7 ++++++- .../SelectExpressionBuilder.cs | 8 +++++++ ...vigation_denies_when_no_match.verified.txt | 3 +-- ...ssing_navigation_uses_include.verified.txt | 3 +-- ...d_navigation_selections_merge.verified.txt | 3 +-- ..._should_not_select_all_fields.verified.txt | 3 +-- ...ntegrationTests.Many_children.verified.txt | 3 +-- .../IntegrationTests.Owned.verified.txt | 3 +-- ...d_filter_accessing_navigation.verified.txt | 1 - ...s_to_navigation_of_field_type.verified.txt | 3 +-- .../IntegrationTests.Where_owned.verified.txt | 3 +-- 19 files changed, 54 insertions(+), 52 deletions(-) diff --git a/src/GraphQL.EntityFramework/Filters/FilterEntry.cs b/src/GraphQL.EntityFramework/Filters/FilterEntry.cs index 2653f290a..9b60017a6 100644 --- a/src/GraphQL.EntityFramework/Filters/FilterEntry.cs +++ b/src/GraphQL.EntityFramework/Filters/FilterEntry.cs @@ -109,7 +109,7 @@ public FieldProjectionInfo AddRequirements( var navMetadata = FindNavigation(navigationProperties, navName)!; mergedNavigations[navName] = mergedNavigations.TryGetValue(navName, out var existingNav) ? existingNav with { IsWhole = true } - : new(navMetadata.Type, navMetadata.IsCollection, new([], null, null, null), true); + : new(navMetadata.Type, navMetadata.IsCollection, new([], null, null, null), true, IsRequired: navMetadata.IsRequired); } } @@ -135,7 +135,7 @@ public FieldProjectionInfo AddRequirements( } else { - mergedNavigations[navName] = new(navMetadata.Type, navMetadata.IsCollection, new(requiredProps, null, null, null)); + mergedNavigations[navName] = new(navMetadata.Type, navMetadata.IsCollection, new(requiredProps, null, null, null), IsRequired: navMetadata.IsRequired); } } } diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs index f49ea5b09..01c5af1e7 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_First.cs @@ -159,14 +159,9 @@ FieldType BuildFirstField( return ReturnNullable(); } - if (disableTracking) - { - query = query.AsNoTracking(); - } - query = query.ApplyGraphQlArguments(context, names, false, omitQueryArguments); - query = includeAppender.ApplyProjection(context, fieldContext.Filters, query); + query = includeAppender.ApplyProjection(context, fieldContext.Filters, query, disableTracking); QueryLogger.Write(query); diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs index b0168ee02..7155ad1ef 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Queryable.cs @@ -105,17 +105,12 @@ FieldType BuildQueryField( return Array.Empty(); } - if (disableTracking) - { - query = query.AsNoTracking(); - } - if (!omitQueryArguments) { query = query.ApplyGraphQlArguments(context, names, true, omitQueryArguments); } - query = includeAppender.ApplyProjection(context, fieldContext.Filters, query); + query = includeAppender.ApplyProjection(context, fieldContext.Filters, query, disableTracking); QueryLogger.Write(query); diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs index 360128e53..074ca0d15 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_QueryableConnection.cs @@ -78,14 +78,9 @@ ConnectionBuilder BuildQueryConnection( return Empty(context); } - if (disableTracking) - { - query = query.AsNoTracking(); - } - query = query.ApplyGraphQlArguments(context, names, true, omitQueryArguments); - query = includeAppender.ApplyProjection(context, fieldContext.Filters, query); + query = includeAppender.ApplyProjection(context, fieldContext.Filters, query, disableTracking); try { diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs index 11c12438c..c8f7b519a 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Single.cs @@ -159,14 +159,9 @@ FieldType BuildSingleField( return ReturnNullable(); } - if (disableTracking) - { - query = query.AsNoTracking(); - } - query = query.ApplyGraphQlArguments(context, names, false, omitQueryArguments); - query = includeAppender.ApplyProjection(context, fieldContext.Filters, query); + query = includeAppender.ApplyProjection(context, fieldContext.Filters, query, disableTracking); QueryLogger.Write(query); diff --git a/src/GraphQL.EntityFramework/IncludeAppender.cs b/src/GraphQL.EntityFramework/IncludeAppender.cs index 4c16eb357..d5ab10ff9 100644 --- a/src/GraphQL.EntityFramework/IncludeAppender.cs +++ b/src/GraphQL.EntityFramework/IncludeAppender.cs @@ -9,17 +9,20 @@ /// where the entity can be projected, otherwise includes. A projected navigation whose type /// cannot be projected is bound whole, and the navigations under it are then loaded through /// includes alongside the select, since EF applies includes to the entities in a projection. + /// applies AsNoTracking only when no select is added, since + /// a select creates new instances that EF does not track, so AsNoTracking would do nothing. /// public IQueryable ApplyProjection( IResolveFieldContext context, Filters? filters, - IQueryable query) + IQueryable query, + bool disableTracking = false) where TDbContext : DbContext where TItem : class { if (context.SubFields is null) { - return query; + return ApplyTracking(query, disableTracking); } var type = typeof(TItem); @@ -43,7 +46,7 @@ public IQueryable ApplyProjection( if (!SelectExpressionBuilder.TryBuild(projection, keyNames, derivedTypes, out var expression, out var includePaths, out var argumentFields)) { - return AddIncludesFromProjection(query, projection); + return ApplyTracking(AddIncludesFromProjection(query, projection), disableTracking); } foreach (var includePath in includePaths) @@ -56,6 +59,10 @@ public IQueryable ApplyProjection( return query.Select(expression); } + static IQueryable ApplyTracking(IQueryable query, bool disableTracking) + where TItem : class => + disableTracking ? query.AsNoTracking() : query; + /// /// Whether the projection loads a collection at any depth, the only case where query splitting /// changes anything. @@ -432,7 +439,8 @@ void ProcessSelectionSet( new( navType, navigation.IsCollection, - GetNestedProjection(field.SelectionSet, navGraphType, navType, nestedNavProps, nestedKeys, nestedFks, context))); + GetNestedProjection(field.SelectionSet, navGraphType, navType, nestedNavProps, nestedKeys, nestedFks, context), + IsRequired: navigation.IsRequired)); } return result; @@ -686,7 +694,7 @@ void ProcessProjectionExpression( nestedProjection = new(nestedScalarFields, nestedKeys ?? [], nestedFks ?? new HashSet(), []); } - AddNavigation(navProjections, navigation.Name, new(navType, navigation.IsCollection, nestedProjection, isWhole, arguments)); + AddNavigation(navProjections, navigation.Name, new(navType, navigation.IsCollection, nestedProjection, isWhole, arguments, navigation.IsRequired)); } } @@ -891,7 +899,8 @@ void ProcessNavigationOrScalar( new( navType, navigation.IsCollection, - GetNestedProjection(field.SelectionSet, GetComplexGraphType(fieldType), navType, nestedNavProps, nestedKeys, nestedFks, context))); + GetNestedProjection(field.SelectionSet, GetComplexGraphType(fieldType), navType, nestedNavProps, nestedKeys, nestedFks, context), + IsRequired: navigation.IsRequired)); } /// diff --git a/src/GraphQL.EntityFramework/Navigation.cs b/src/GraphQL.EntityFramework/Navigation.cs index 3af8c4947..559c244ff 100644 --- a/src/GraphQL.EntityFramework/Navigation.cs +++ b/src/GraphQL.EntityFramework/Navigation.cs @@ -7,5 +7,6 @@ public record Navigation Type Type, bool IsNullable, bool IsCollection, - string? InverseName = null + string? InverseName = null, + bool IsRequired = false ); \ No newline at end of file diff --git a/src/GraphQL.EntityFramework/NavigationReader.cs b/src/GraphQL.EntityFramework/NavigationReader.cs index d6ca06210..61836eb0b 100644 --- a/src/GraphQL.EntityFramework/NavigationReader.cs +++ b/src/GraphQL.EntityFramework/NavigationReader.cs @@ -20,11 +20,19 @@ static IReadOnlyDictionary GetNavigations(IEntityType entity _ => { var (itemType, isCollection) = GetNavigationType(_); - return new Navigation(_.Name, itemType, _.PropertyInfo!.IsNullable(), isCollection, _.Inverse?.Name); + return new Navigation(_.Name, itemType, _.PropertyInfo!.IsNullable(), isCollection, _.Inverse?.Name, IsRequired(_)); }) .ToDictionary(_ => _.Name.ToLowerInvariant(), StringComparer.OrdinalIgnoreCase); } + // EF never materializes a required reference navigation as null, so the select projection + // can skip the null check on it + static bool IsRequired(INavigationBase navigation) => + navigation is INavigation { IsCollection: false } reference && + (reference.IsOnDependent + ? reference.ForeignKey.IsRequired + : reference.ForeignKey.IsRequiredDependent); + static (Type itemType, bool isCollection) GetNavigationType(INavigationBase navigation) { var navigationType = navigation.ClrType; diff --git a/src/GraphQL.EntityFramework/SelectProjection/NavigationProjectionInfo.cs b/src/GraphQL.EntityFramework/SelectProjection/NavigationProjectionInfo.cs index 87064e01d..03d4bd357 100644 --- a/src/GraphQL.EntityFramework/SelectProjection/NavigationProjectionInfo.cs +++ b/src/GraphQL.EntityFramework/SelectProjection/NavigationProjectionInfo.cs @@ -3,6 +3,10 @@ /// itself rather than properties of it. The select projection then binds the navigation whole /// instead of building a member init from . /// +/// +/// The navigation is required in the EF model, so is never null. The select projection then +/// skips the null check, which EF would otherwise translate to an always false comparison. +/// /// /// The field's ids, where and orderBy, to apply inside the collection subquery. Null when the /// navigation was selected more than once, since one loaded collection cannot satisfy two sets @@ -13,7 +17,8 @@ record NavigationProjectionInfo( bool IsCollection, FieldProjectionInfo Projection, bool IsWhole = false, - NavigationArguments? Arguments = null) + NavigationArguments? Arguments = null, + bool IsRequired = false) { public NavigationProjectionInfo Merge(NavigationProjectionInfo other) => this with diff --git a/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs b/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs index 4fe1d1abf..e0f37594a 100644 --- a/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs +++ b/src/GraphQL.EntityFramework/SelectProjection/SelectExpressionBuilder.cs @@ -381,6 +381,14 @@ static bool TryBuildNavigationBinding( return false; } + // A required navigation is never null, and EF translates a null check on one to an always + // false comparison, so it is only checked when optional + if (navProjection.IsRequired) + { + binding = Expression.Bind(navAccess.Member, init); + return true; + } + // source.Parent == null ? null : new Parent { ... } var conditional = Expression.Condition( Expression.Equal(navAccess, navMetadata.NullConstant), diff --git a/src/Tests/IntegrationTests/IntegrationTests.AddSingleField_with_filter_accessing_navigation_denies_when_no_match.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.AddSingleField_with_filter_accessing_navigation_denies_when_no_match.verified.txt index a40dd21c7..365cf9772 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.AddSingleField_with_filter_accessing_navigation_denies_when_no_match.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.AddSingleField_with_filter_accessing_navigation_denies_when_no_match.verified.txt @@ -6,8 +6,7 @@ }, sql: { Text: -select top (2) cast (0 as bit), - case when f0.Discriminator = N'FilterDerivedEntity' then cast (1 as bit) else cast (0 as bit) end, +select top (2) case when f0.Discriminator = N'FilterDerivedEntity' then cast (1 as bit) else cast (0 as bit) end, f0.CommonProperty, f.BaseEntityId, f.Id, diff --git a/src/Tests/IntegrationTests/IntegrationTests.AddSingleField_with_filter_accessing_navigation_uses_include.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.AddSingleField_with_filter_accessing_navigation_uses_include.verified.txt index 6ad60c86e..62b4a6a0f 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.AddSingleField_with_filter_accessing_navigation_uses_include.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.AddSingleField_with_filter_accessing_navigation_uses_include.verified.txt @@ -9,8 +9,7 @@ }, sql: { Text: -select top (2) cast (0 as bit), - case when f0.Discriminator = N'FilterDerivedEntity' then cast (1 as bit) else cast (0 as bit) end, +select top (2) case when f0.Discriminator = N'FilterDerivedEntity' then cast (1 as bit) else cast (0 as bit) end, f0.CommonProperty, f.BaseEntityId, f.Id, diff --git a/src/Tests/IntegrationTests/IntegrationTests.Aliased_navigation_selections_merge.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Aliased_navigation_selections_merge.verified.txt index 427b017fd..82f3e92d4 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Aliased_navigation_selections_merge.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Aliased_navigation_selections_merge.verified.txt @@ -16,8 +16,7 @@ }, sql: { Text: -select cast (0 as bit), - d.Id, +select d.Id, d.IsActive, d.Name, e.DepartmentId, 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 index 7965c72c8..b91346520 100644 --- 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 @@ -8,8 +8,7 @@ }, sql: { Text: -select top (2) cast (0 as bit), - case when f0.Discriminator = N'FilterDerivedEntity' then cast (1 as bit) else cast (0 as bit) end, +select top (2) case when f0.Discriminator = N'FilterDerivedEntity' then cast (1 as bit) else cast (0 as bit) end, f0.CommonProperty, f0.Id, f.BaseEntityId, diff --git a/src/Tests/IntegrationTests/IntegrationTests.Many_children.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Many_children.verified.txt index 17b24378a..56a2f85ae 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Many_children.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Many_children.verified.txt @@ -12,8 +12,7 @@ }, sql: { Text: -select case when c.Id is null then cast (1 as bit) else cast (0 as bit) end, - c.Id, +select c.Id, c.ParentId, c0.Id, c0.ParentId, diff --git a/src/Tests/IntegrationTests/IntegrationTests.Owned.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Owned.verified.txt index b718e3160..686317505 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Owned.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Owned.verified.txt @@ -11,8 +11,7 @@ }, sql: { Text: -select top (2) cast (0 as bit), - o.Child1_Property, +select top (2) o.Child1_Property as Property, o.Id, o.Property from OwnedParents as o diff --git a/src/Tests/IntegrationTests/IntegrationTests.Query_with_select_projection_and_filter_accessing_navigation.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Query_with_select_projection_and_filter_accessing_navigation.verified.txt index ac3675bea..e7242f192 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Query_with_select_projection_and_filter_accessing_navigation.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Query_with_select_projection_and_filter_accessing_navigation.verified.txt @@ -11,7 +11,6 @@ sql: { Text: select i0.Id, - case when i1.Id is null then cast (1 as bit) else cast (0 as bit) end, i1.Id from IncludeNonQueryableBs as i inner join diff --git a/src/Tests/IntegrationTests/IntegrationTests.Selection_applies_to_navigation_of_field_type.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Selection_applies_to_navigation_of_field_type.verified.txt index 0e9871b12..5219f089f 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Selection_applies_to_navigation_of_field_type.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Selection_applies_to_navigation_of_field_type.verified.txt @@ -15,8 +15,7 @@ }, sql: { Text: -select case when c.Id is null then cast (1 as bit) else cast (0 as bit) end, - c.Id, +select c.Id, case when w0.Id is null then cast (1 as bit) else cast (0 as bit) end, w0.Id, c.ParentId, diff --git a/src/Tests/IntegrationTests/IntegrationTests.Where_owned.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Where_owned.verified.txt index 5a0c55ad1..3e185da7f 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Where_owned.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Where_owned.verified.txt @@ -11,8 +11,7 @@ }, sql: { Text: -select top (2) cast (0 as bit), - o.Child1_Property, +select top (2) o.Child1_Property as Property, o.Id, o.Property from OwnedParents as o