Skip to content

Commit 35cd115

Browse files
rojiroji
authored
Stop moving pending selector after DefaultIfEmpty in nav expansion (#36241)
Fixes #36208
1 parent 2c27fcd commit 35cd115

File tree

11 files changed

+141
-121
lines changed

11 files changed

+141
-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
@@ -2399,20 +2399,25 @@ [new ProjectionExpression(nullSqlExpression, "empty")],
23992399
_tables.Add(dummySelectExpression);
24002400
_tables.Add(joinTable);
24012401

2402+
// Go over all projected columns and make them nullable; for non-nullable value types, add a SQL COALESCE as well.
24022403
var projectionMapping = new Dictionary<ProjectionMember, Expression>();
2403-
foreach (var projection in _projectionMapping)
2404+
foreach (var (projectionMember, projection) in _projectionMapping)
24042405
{
2405-
var projectionToAdd = projection.Value;
2406-
if (projectionToAdd is StructuralTypeProjectionExpression typeProjection)
2406+
var newProjection = projection switch
24072407
{
2408-
projectionToAdd = typeProjection.MakeNullable();
2409-
}
2410-
else if (projectionToAdd is ColumnExpression column)
2408+
StructuralTypeProjectionExpression p => p.MakeNullable(),
2409+
ColumnExpression column => column.MakeNullable(),
2410+
var p => p
2411+
};
2412+
2413+
if (newProjection is SqlExpression { Type: var type } newSqlProjection && !type.IsNullableType())
24112414
{
2412-
projectionToAdd = column.MakeNullable();
2415+
newProjection = sqlExpressionFactory.Coalesce(
2416+
newSqlProjection,
2417+
sqlExpressionFactory.Constant(type.GetDefaultValue(), type));
24132418
}
24142419

2415-
projectionMapping[projection.Key] = projectionToAdd;
2420+
projectionMapping[projectionMember] = newProjection;
24162421
}
24172422

24182423
// 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
@@ -2566,26 +2566,26 @@ public override async Task Contains_over_entityType_should_materialize_when_comp
25662566
AssertSql();
25672567
}
25682568

2569-
public override async Task Average_after_default_if_empty_does_not_throw(bool async)
2569+
public override async Task Average_after_DefaultIfEmpty_does_not_throw(bool async)
25702570
{
25712571
// Contains over subquery. Issue #17246.
2572-
await AssertTranslationFailed(() => base.Average_after_default_if_empty_does_not_throw(async));
2572+
await AssertTranslationFailed(() => base.Average_after_DefaultIfEmpty_does_not_throw(async));
25732573

25742574
AssertSql();
25752575
}
25762576

2577-
public override async Task Max_after_default_if_empty_does_not_throw(bool async)
2577+
public override async Task Max_after_DefaultIfEmpty_does_not_throw(bool async)
25782578
{
25792579
// Contains over subquery. Issue #17246.
2580-
await AssertTranslationFailed(() => base.Max_after_default_if_empty_does_not_throw(async));
2580+
await AssertTranslationFailed(() => base.Max_after_DefaultIfEmpty_does_not_throw(async));
25812581

25822582
AssertSql();
25832583
}
25842584

2585-
public override async Task Min_after_default_if_empty_does_not_throw(bool async)
2585+
public override async Task Min_after_DefaultIfEmpty_does_not_throw(bool async)
25862586
{
25872587
// Contains over subquery. Issue #17246.
2588-
await AssertTranslationFailed(() => base.Min_after_default_if_empty_does_not_throw(async));
2588+
await AssertTranslationFailed(() => base.Min_after_DefaultIfEmpty_does_not_throw(async));
25892589

25902590
AssertSql();
25912591
}

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

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -202,56 +202,64 @@ 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_preceded_by_constant_Select(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_preceded_by_constant_Select(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_positive(bool async)
238238
{
239-
await base.Default_if_empty_top_level_arg(async);
239+
// Cosmos client evaluation. Issue #17246.
240+
await AssertTranslationFailed(() => base.DefaultIfEmpty_top_level_positive(async));
241+
242+
AssertSql();
243+
}
244+
245+
public override async Task DefaultIfEmpty_top_level_arg(bool async)
246+
{
247+
await base.DefaultIfEmpty_top_level_arg(async);
240248

241249
AssertSql();
242250
}
243251

244-
public override async Task Default_if_empty_top_level_arg_followed_by_projecting_constant(bool async)
252+
public override async Task DefaultIfEmpty_top_level_arg_followed_by_projecting_constant(bool async)
245253
{
246-
await base.Default_if_empty_top_level_arg_followed_by_projecting_constant(async);
254+
await base.DefaultIfEmpty_top_level_arg_followed_by_projecting_constant(async);
247255

248256
AssertSql();
249257
}
250258

251-
public override async Task Default_if_empty_top_level_projection(bool async)
259+
public override async Task DefaultIfEmpty_top_level_projection(bool async)
252260
{
253261
// Cosmos client evaluation. Issue #17246.
254-
await AssertTranslationFailed(() => base.Default_if_empty_top_level_projection(async));
262+
await AssertTranslationFailed(() => base.DefaultIfEmpty_top_level_projection(async));
255263

256264
AssertSql();
257265
}

‎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
@@ -1893,21 +1893,21 @@ public virtual Task Min_over_default_returns_default(bool async)
18931893

18941894
[ConditionalTheory]
18951895
[MemberData(nameof(IsAsyncData))]
1896-
public virtual Task Average_after_default_if_empty_does_not_throw(bool async)
1896+
public virtual Task Average_after_DefaultIfEmpty_does_not_throw(bool async)
18971897
=> AssertAverage(
18981898
async,
18991899
ss => ss.Set<Order>().Where(o => o.OrderID == 10243).Select(o => o.OrderID).DefaultIfEmpty());
19001900

19011901
[ConditionalTheory]
19021902
[MemberData(nameof(IsAsyncData))]
1903-
public virtual Task Max_after_default_if_empty_does_not_throw(bool async)
1903+
public virtual Task Max_after_DefaultIfEmpty_does_not_throw(bool async)
19041904
=> AssertMax(
19051905
async,
19061906
ss => ss.Set<Order>().Where(o => o.OrderID == 10243).Select(o => o.OrderID).DefaultIfEmpty());
19071907

19081908
[ConditionalTheory]
19091909
[MemberData(nameof(IsAsyncData))]
1910-
public virtual Task Min_after_default_if_empty_does_not_throw(bool async)
1910+
public virtual Task Min_after_DefaultIfEmpty_does_not_throw(bool async)
19111911
=> AssertMin(
19121912
async,
19131913
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)