Skip to content

Commit 67e5859

Browse files
rojiroji
committed
Stop moving pending selector after DefaultIfEmpty in nav expansion
Fixes #36208
1 parent 43ca495 commit 67e5859

File tree

11 files changed

+133
-121
lines changed

11 files changed

+133
-121
lines changed

‎src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -464,24 +464,24 @@ public virtual void ApplyDefaultIfEmpty()
464464
throw new InvalidOperationException(InMemoryStrings.DefaultIfEmptyAppliedAfterProjection);
465465
}
466466

467-
var projectionMapping = new Dictionary<ProjectionMember, Expression>();
468-
foreach (var (projectionMember, expression) in _projectionMapping)
469-
{
470-
projectionMapping[projectionMember] = expression is EntityProjectionExpression entityProjectionExpression
471-
? MakeEntityProjectionNullable(entityProjectionExpression)
472-
: MakeReadValueNullable(expression);
473-
}
474-
475-
_projectionMapping = projectionMapping;
476-
var projectionMappingExpressions = _projectionMappingExpressions.Select(e => MakeReadValueNullable(e)).ToList();
477-
_projectionMappingExpressions.Clear();
478-
_projectionMappingExpressions.AddRange(projectionMappingExpressions);
479-
_groupingParameter = null;
480-
481467
ServerQueryExpression = Call(
482468
EnumerableMethods.DefaultIfEmptyWithArgument.MakeGenericMethod(typeof(ValueBuffer)),
483469
ServerQueryExpression,
484470
Constant(new ValueBuffer(Enumerable.Repeat((object?)null, _projectionMappingExpressions.Count).ToArray())));
471+
472+
ReplaceProjection(_projectionMapping.ToDictionary(
473+
kv => kv.Key,
474+
kv => kv.Value switch
475+
{
476+
EntityProjectionExpression p => MakeEntityProjectionNullable(p),
477+
478+
var p when !p.Type.IsNullableType()
479+
=> Coalesce(
480+
MakeReadValueNullable(p),
481+
p.Type.GetDefaultValueConstant()),
482+
483+
var p => p
484+
}));
485485
}
486486

487487
/// <summary>

‎src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2378,20 +2378,25 @@ [new ProjectionExpression(nullSqlExpression, "empty")],
23782378
_tables.Add(dummySelectExpression);
23792379
_tables.Add(joinTable);
23802380

2381+
// Go over all projected columns and make them nullable; for non-nullable value types, add a SQL COALESCE as well.
23812382
var projectionMapping = new Dictionary<ProjectionMember, Expression>();
2382-
foreach (var projection in _projectionMapping)
2383+
foreach (var (projectionMember, projection) in _projectionMapping)
23832384
{
2384-
var projectionToAdd = projection.Value;
2385-
if (projectionToAdd is StructuralTypeProjectionExpression typeProjection)
2385+
var newProjection = projection switch
23862386
{
2387-
projectionToAdd = typeProjection.MakeNullable();
2388-
}
2389-
else if (projectionToAdd is ColumnExpression column)
2387+
StructuralTypeProjectionExpression p => p.MakeNullable(),
2388+
ColumnExpression column => column.MakeNullable(),
2389+
var p => p
2390+
};
2391+
2392+
if (newProjection is SqlExpression { Type: var type } newSqlProjection && !type.IsNullableType())
23902393
{
2391-
projectionToAdd = column.MakeNullable();
2394+
newProjection = sqlExpressionFactory.Coalesce(
2395+
newSqlProjection,
2396+
sqlExpressionFactory.Constant(type.GetDefaultValue(), type));
23922397
}
23932398

2394-
projectionMapping[projection.Key] = projectionToAdd;
2399+
projectionMapping[projectionMember] = newProjection;
23952400
}
23962401

23972402
// ChildIdentifiers shouldn't be required to be updated since during translation they should be empty.

‎src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -966,23 +966,21 @@ private Expression ProcessContains(NavigationExpansionExpression source, Express
966966

967967
private NavigationExpansionExpression ProcessDefaultIfEmpty(NavigationExpansionExpression source)
968968
{
969-
source.UpdateSource(
970-
Expression.Call(
971-
QueryableMethods.DefaultIfEmptyWithoutArgument.MakeGenericMethod(source.SourceElementType),
972-
source.Source));
969+
// Apply any pending selector, since previous Selects can't be moved after DefaultIfEmpty (change of meaning, see #36208).
970+
// Mark the entity(s) being selected as optional.
971+
source = (NavigationExpansionExpression)_pendingSelectorExpandingExpressionVisitor.Visit(source);
972+
var newStructure = SnapshotExpression(source.PendingSelector);
973+
newStructure = _entityReferenceOptionalMarkingExpressionVisitor.Visit(newStructure);
974+
var queryable = Reduce(source);
973975

974-
var pendingSelector = source.PendingSelector;
975-
_entityReferenceOptionalMarkingExpressionVisitor.Visit(pendingSelector);
976-
if (!pendingSelector.Type.IsNullableType())
977-
{
978-
pendingSelector = Expression.Coalesce(
979-
Expression.Convert(pendingSelector, pendingSelector.Type.MakeNullable()),
980-
pendingSelector.Type.GetDefaultValueConstant());
981-
}
976+
var result = Expression.Call(
977+
QueryableMethods.DefaultIfEmptyWithoutArgument.MakeGenericMethod(queryable.Type.GetSequenceType()),
978+
queryable);
982979

983-
source.ApplySelector(pendingSelector);
980+
var navigationTree = new NavigationTreeExpression(newStructure);
981+
var parameterName = GetParameterName("e");
984982

985-
return source;
983+
return new NavigationExpansionExpression(result, navigationTree, navigationTree, parameterName);
986984
}
987985

988986
private NavigationExpansionExpression ProcessDistinct(NavigationExpansionExpression source, MethodInfo genericMethod)

‎src/Shared/SharedTypeExtensions.cs

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,6 @@ public static MethodInfo GetGenericMethod(
394394

395395
private static readonly Dictionary<Type, object> CommonTypeDictionary = new()
396396
{
397-
#pragma warning disable IDE0034 // Simplify 'default' expression - default causes default(object)
398397
{ typeof(int), default(int) },
399398
{ typeof(Guid), default(Guid) },
400399
{ typeof(DateOnly), default(DateOnly) },
@@ -412,25 +411,15 @@ public static MethodInfo GetGenericMethod(
412411
{ typeof(ushort), default(ushort) },
413412
{ typeof(ulong), default(ulong) },
414413
{ typeof(sbyte), default(sbyte) }
415-
#pragma warning restore IDE0034 // Simplify 'default' expression
416414
};
417415

418416
public static object? GetDefaultValue(
419417
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
420418
this Type type)
421-
{
422-
if (!type.IsValueType)
423-
{
424-
return null;
425-
}
419+
=> type.IsNullableType() ? null : RuntimeHelpers.GetUninitializedObject(type);
426420

427-
// A bit of perf code to avoid calling Activator.CreateInstance for common types and
428-
// to avoid boxing on every call. This is about 50% faster than just calling CreateInstance
429-
// for all value types.
430-
return CommonTypeDictionary.TryGetValue(type, out var value)
431-
? value
432-
: Activator.CreateInstance(type);
433-
}
421+
public static ConstantExpression GetDefaultValueConstant(this Type type)
422+
=> Expression.Constant(type.GetDefaultValue(), type);
434423

435424
[RequiresUnreferencedCode("Gets all types from the given assembly - unsafe for trimming")]
436425
public static IEnumerable<TypeInfo> GetConstructibleTypes(
@@ -648,7 +637,4 @@ public static IEnumerable<string> GetNamespaces(this Type type)
648637
}
649638
}
650639
}
651-
652-
public static ConstantExpression GetDefaultValueConstant(this Type type)
653-
=> Expression.Constant(type.IsValueType ? RuntimeHelpers.GetUninitializedObject(type) : null, type);
654640
}

‎test/EFCore.Cosmos.FunctionalTests/Query/NorthwindAggregateOperatorsQueryCosmosTest.cs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2573,26 +2573,26 @@ public override async Task Contains_over_entityType_should_materialize_when_comp
25732573
AssertSql();
25742574
}
25752575

2576-
public override async Task Average_after_default_if_empty_does_not_throw(bool async)
2576+
public override async Task Average_after_DefaultIfEmpty_does_not_throw(bool async)
25772577
{
25782578
// Contains over subquery. Issue #17246.
2579-
await AssertTranslationFailed(() => base.Average_after_default_if_empty_does_not_throw(async));
2579+
await AssertTranslationFailed(() => base.Average_after_DefaultIfEmpty_does_not_throw(async));
25802580

25812581
AssertSql();
25822582
}
25832583

2584-
public override async Task Max_after_default_if_empty_does_not_throw(bool async)
2584+
public override async Task Max_after_DefaultIfEmpty_does_not_throw(bool async)
25852585
{
25862586
// Contains over subquery. Issue #17246.
2587-
await AssertTranslationFailed(() => base.Max_after_default_if_empty_does_not_throw(async));
2587+
await AssertTranslationFailed(() => base.Max_after_DefaultIfEmpty_does_not_throw(async));
25882588

25892589
AssertSql();
25902590
}
25912591

2592-
public override async Task Min_after_default_if_empty_does_not_throw(bool async)
2592+
public override async Task Min_after_DefaultIfEmpty_does_not_throw(bool async)
25932593
{
25942594
// Contains over subquery. Issue #17246.
2595-
await AssertTranslationFailed(() => base.Min_after_default_if_empty_does_not_throw(async));
2595+
await AssertTranslationFailed(() => base.Min_after_DefaultIfEmpty_does_not_throw(async));
25962596

25972597
AssertSql();
25982598
}

‎test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -202,56 +202,56 @@ public override async Task Queryable_reprojection(bool async)
202202
AssertSql();
203203
}
204204

205-
public override async Task Default_if_empty_top_level(bool async)
205+
public override async Task DefaultIfEmpty_top_level(bool async)
206206
{
207207
// Cosmos client evaluation. Issue #17246.
208-
await AssertTranslationFailed(() => base.Default_if_empty_top_level(async));
208+
await AssertTranslationFailed(() => base.DefaultIfEmpty_top_level(async));
209209

210210
AssertSql();
211211
}
212212

213-
public override async Task Join_with_default_if_empty_on_both_sources(bool async)
213+
public override async Task Join_with_DefaultIfEmpty_on_both_sources(bool async)
214214
{
215215
// Cosmos client evaluation. Issue #17246.
216-
await AssertTranslationFailed(() => base.Join_with_default_if_empty_on_both_sources(async));
216+
await AssertTranslationFailed(() => base.Join_with_DefaultIfEmpty_on_both_sources(async));
217217

218218
AssertSql();
219219
}
220220

221-
public override async Task Default_if_empty_top_level_followed_by_projecting_constant(bool async)
221+
public override async Task DefaultIfEmpty_top_level_followed_by_constant_Select(bool async)
222222
{
223223
// Cosmos client evaluation. Issue #17246.
224-
await AssertTranslationFailed(() => base.Default_if_empty_top_level_followed_by_projecting_constant(async));
224+
await AssertTranslationFailed(() => base.DefaultIfEmpty_top_level_followed_by_constant_Select(async));
225225

226226
AssertSql();
227227
}
228228

229-
public override async Task Default_if_empty_top_level_positive(bool async)
229+
public override async Task DefaultIfEmpty_top_level_positive(bool async)
230230
{
231231
// Cosmos client evaluation. Issue #17246.
232-
await AssertTranslationFailed(() => base.Default_if_empty_top_level_positive(async));
232+
await AssertTranslationFailed(() => base.DefaultIfEmpty_top_level_positive(async));
233233

234234
AssertSql();
235235
}
236236

237-
public override async Task Default_if_empty_top_level_arg(bool async)
237+
public override async Task DefaultIfEmpty_top_level_arg(bool async)
238238
{
239-
await base.Default_if_empty_top_level_arg(async);
239+
await base.DefaultIfEmpty_top_level_arg(async);
240240

241241
AssertSql();
242242
}
243243

244-
public override async Task Default_if_empty_top_level_arg_followed_by_projecting_constant(bool async)
244+
public override async Task DefaultIfEmpty_top_level_arg_followed_by_projecting_constant(bool async)
245245
{
246-
await base.Default_if_empty_top_level_arg_followed_by_projecting_constant(async);
246+
await base.DefaultIfEmpty_top_level_arg_followed_by_projecting_constant(async);
247247

248248
AssertSql();
249249
}
250250

251-
public override async Task Default_if_empty_top_level_projection(bool async)
251+
public override async Task DefaultIfEmpty_top_level_projection(bool async)
252252
{
253253
// Cosmos client evaluation. Issue #17246.
254-
await AssertTranslationFailed(() => base.Default_if_empty_top_level_projection(async));
254+
await AssertTranslationFailed(() => base.DefaultIfEmpty_top_level_projection(async));
255255

256256
AssertSql();
257257
}

‎test/EFCore.InMemory.FunctionalTests/Query/NorthwindSelectQueryInMemoryTest.cs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,4 @@ public override Task
1414
() => base
1515
.SelectMany_with_collection_being_correlated_subquery_which_references_non_mapped_properties_from_inner_and_outer_entity(
1616
async));
17-
18-
public override async Task SelectMany_correlated_with_outer_3(bool async)
19-
// DefaultIfEmpty. Issue #17536.
20-
=> await Assert.ThrowsAsync<EqualException>(() => base.SelectMany_correlated_with_outer_3(async));
2117
}

‎test/EFCore.Specification.Tests/Query/NorthwindAggregateOperatorsQueryTestBase.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1899,21 +1899,21 @@ public virtual Task Min_over_default_returns_default(bool async)
18991899

19001900
[ConditionalTheory]
19011901
[MemberData(nameof(IsAsyncData))]
1902-
public virtual Task Average_after_default_if_empty_does_not_throw(bool async)
1902+
public virtual Task Average_after_DefaultIfEmpty_does_not_throw(bool async)
19031903
=> AssertAverage(
19041904
async,
19051905
ss => ss.Set<Order>().Where(o => o.OrderID == 10243).Select(o => o.OrderID).DefaultIfEmpty());
19061906

19071907
[ConditionalTheory]
19081908
[MemberData(nameof(IsAsyncData))]
1909-
public virtual Task Max_after_default_if_empty_does_not_throw(bool async)
1909+
public virtual Task Max_after_DefaultIfEmpty_does_not_throw(bool async)
19101910
=> AssertMax(
19111911
async,
19121912
ss => ss.Set<Order>().Where(o => o.OrderID == 10243).Select(o => o.OrderID).DefaultIfEmpty());
19131913

19141914
[ConditionalTheory]
19151915
[MemberData(nameof(IsAsyncData))]
1916-
public virtual Task Min_after_default_if_empty_does_not_throw(bool async)
1916+
public virtual Task Min_after_DefaultIfEmpty_does_not_throw(bool async)
19171917
=> AssertMin(
19181918
async,
19191919
ss => ss.Set<Order>().Where(o => o.OrderID == 10243).Select(o => o.OrderID).DefaultIfEmpty());

‎test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2232,60 +2232,69 @@ protected class Foo
22322232

22332233
[ConditionalTheory]
22342234
[MemberData(nameof(IsAsyncData))]
2235-
public virtual Task Default_if_empty_top_level(bool async)
2235+
public virtual Task DefaultIfEmpty_top_level(bool async)
22362236
=> AssertQuery(
22372237
async,
2238-
ss => from e in ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty()
2239-
select e);
2238+
ss => ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty());
22402239

22412240
[ConditionalTheory]
22422241
[MemberData(nameof(IsAsyncData))]
2243-
public virtual Task Join_with_default_if_empty_on_both_sources(bool async)
2242+
public virtual Task Join_with_DefaultIfEmpty_on_both_sources(bool async)
22442243
=> AssertQuery(
22452244
async,
2246-
ss => (from e in ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty()
2247-
select e).Join(
2248-
from e in ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty()
2249-
select e, o => o, i => i, (o, i) => o),
2245+
ss => ss.Set<Employee>()
2246+
.Where(c => c.EmployeeID == NonExistentID)
2247+
.DefaultIfEmpty()
2248+
.Join(
2249+
ss.Set<Employee>()
2250+
.Where(c => c.EmployeeID == NonExistentID)
2251+
.DefaultIfEmpty(),
2252+
o => o, i => i, (o, i) => o),
22502253
assertEmpty: true);
22512254

22522255
[ConditionalTheory]
22532256
[MemberData(nameof(IsAsyncData))]
2254-
public virtual Task Default_if_empty_top_level_followed_by_projecting_constant(bool async)
2257+
public virtual Task DefaultIfEmpty_top_level_followed_by_constant_Select(bool async)
22552258
=> AssertQuery(
22562259
async,
2257-
ss => from e in ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty()
2258-
select "Foo");
2260+
ss => ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty().Select(_ => "Foo"));
2261+
2262+
[ConditionalTheory] // #36208
2263+
[MemberData(nameof(IsAsyncData))]
2264+
public virtual Task DefaultIfEmpty_top_level_preceded_by_constant_Select(bool async)
2265+
=> AssertQuery(
2266+
async,
2267+
ss => ss.Set<Employee>().Where(e => e.EmployeeID == NonExistentID).Select(_ => "Foo").DefaultIfEmpty());
22592268

22602269
[ConditionalTheory]
22612270
[MemberData(nameof(IsAsyncData))]
2262-
public virtual Task Default_if_empty_top_level_arg(bool async)
2271+
public virtual Task DefaultIfEmpty_top_level_arg(bool async)
22632272
=> AssertTranslationFailed(
22642273
() => AssertQuery(
22652274
async,
2266-
ss => from e in ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty(new Employee())
2267-
select e));
2275+
ss => ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty(new Employee())));
22682276

22692277
[ConditionalTheory]
22702278
[MemberData(nameof(IsAsyncData))]
2271-
public virtual Task Default_if_empty_top_level_arg_followed_by_projecting_constant(bool async)
2279+
public virtual Task DefaultIfEmpty_top_level_arg_followed_by_projecting_constant(bool async)
22722280
=> AssertTranslationFailed(
22732281
() => AssertQueryScalar(
22742282
async,
2275-
ss => from e in ss.Set<Employee>().Where(c => c.EmployeeID == NonExistentID).DefaultIfEmpty(new Employee())
2276-
select 42));
2283+
ss => ss.Set<Employee>()
2284+
.Where(c => c.EmployeeID == NonExistentID)
2285+
.DefaultIfEmpty(new Employee())
2286+
.Select(_ => 42)));
22772287

22782288
[ConditionalTheory]
22792289
[MemberData(nameof(IsAsyncData))]
2280-
public virtual Task Default_if_empty_top_level_positive(bool async)
2290+
public virtual Task DefaultIfEmpty_top_level_positive(bool async)
22812291
=> AssertQuery(
22822292
async,
2283-
ss => from e in ss.Set<Employee>().Where(c => c.EmployeeID > 0).DefaultIfEmpty()
2284-
select e);
2293+
ss => ss.Set<Employee>().Where(c => c.EmployeeID > 0).DefaultIfEmpty());
22852294

22862295
[ConditionalTheory]
22872296
[MemberData(nameof(IsAsyncData))]
2288-
public virtual Task Default_if_empty_top_level_projection(bool async)
2297+
public virtual Task DefaultIfEmpty_top_level_projection(bool async)
22892298
=> AssertQueryScalar(
22902299
async,
22912300
ss => from e in ss.Set<Employee>().Where(e => e.EmployeeID == NonExistentID).Select(e => e.EmployeeID).DefaultIfEmpty()

0 commit comments

Comments
 (0)