Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ private sealed class ProjectionMemberRemappingExpressionVisitor(
Dictionary<ProjectionMember, ProjectionMember> projectionMemberMappings)
: ExpressionVisitor
{
// #30915: tracks New/MemberInit nodes rebuilt by this visitor (old instance → new instance). Used by the caller
// to re-key any _nonEntityNullabilityMarkers entries whose key was one of the rebuilt nodes, so that a previously-
// recorded marker key does not go stale when an outer-shaper remap creates a fresh node instance.
public Dictionary<Expression, Expression> RebuiltNodes { get; }
= new Dictionary<Expression, Expression>(ReferenceEqualityComparer.Instance);
Comment on lines +57 to +61

protected override Expression VisitExtension(Expression expression)
{
if (expression is ProjectionBindingExpression projectionBindingExpression)
Expand All @@ -70,6 +76,28 @@ protected override Expression VisitExtension(Expression expression)

return base.VisitExtension(expression);
}

protected override Expression VisitNew(NewExpression node)
{
var visited = (NewExpression)base.VisitNew(node);
if (!ReferenceEquals(visited, node))
{
RebuiltNodes[node] = visited;
}

return visited;
}

protected override Expression VisitMemberInit(MemberInitExpression node)
{
var visited = (MemberInitExpression)base.VisitMemberInit(node);
if (!ReferenceEquals(visited, node))
{
RebuiltNodes[node] = visited;
}

return visited;
}
}

private sealed class ProjectionMemberToIndexConvertingExpressionVisitor(
Expand Down
31 changes: 30 additions & 1 deletion src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3054,7 +3054,36 @@ private Expression AddJoin(
projectionMapping[remappedProjectionMember] = MakeNullable(expression, outerNullable);
}

outerShaper = new ProjectionMemberRemappingExpressionVisitor(this, mapping).Visit(outerShaper);
var outerRemapper = new ProjectionMemberRemappingExpressionVisitor(this, mapping);
outerShaper = outerRemapper.Visit(outerShaper);

// #30915: the outer remap above rebuilds any New/MemberInit node whose projection bindings changed; a
// node previously recorded as a nullability-marker key is now a stale instance. Re-key each such marker
// onto its rebuilt node, re-binding the marker value through the same remap so it still resolves.
// Only this (outer client-eval == false) branch is covered; the other AddJoin branches that remap the
// outer shaper are intentionally not re-keyed (no reachable repro). If one is ever hit with a live prior
// marker, the fail-safe holds: the gate simply does not fire and behavior falls back to the prior throw
// rather than producing an incorrect result. Tracked with the #30915 follow-ups.
if (_nonEntityNullabilityMarkers is not null)
{
foreach (var oldNode in _nonEntityNullabilityMarkers.Keys.ToList())
{
// Re-key only when the key node was rebuilt AND the marker binding still resolves through this
// remap. The marker column is not referenced by the shaper tree, so its projection member could in
// principle have been pruned from _projectionMapping while the key node survived; in that case
// skip the re-key rather than letting outerRemapper.Visit hit the throwing indexer
// (ProjectionMemberRemappingExpressionVisitor.VisitExtension). Skipping preserves the fail-safe:
// the gate does not fire and behavior degrades to the prior throw, never a KeyNotFoundException.
if (outerRemapper.RebuiltNodes.TryGetValue(oldNode, out var newNode)
&& _nonEntityNullabilityMarkers[oldNode] is ProjectionBindingExpression { ProjectionMember: { } markerMember }
&& mapping.ContainsKey(markerMember))
{
var reboundBinding = outerRemapper.Visit(_nonEntityNullabilityMarkers[oldNode]);
RemapNonEntityNullabilityMarker(oldNode, newNode, reboundBinding);
}
}
}

mapping.Clear();

foreach (var (projectionMember, expression) in innerSelect._projectionMapping)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -738,9 +738,25 @@ from countInfo in g.DefaultIfEmpty()
.Join(context.Statuses, e => e.PickupStatusId, s2 => s2.PickupStatusId, (e, s2) => new { s2.PickupStatusId, e.countInfo })
.OrderBy(e => e.PickupStatusId);

var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => query.ToListAsync());
Assert.Contains("Nullable object must have a value", ex.Message);
// #30915 TODO: currently throws on base; flip to assert results if/when fixed.
var result = await query.ToListAsync();

Assert.Equal(3, result.Count);

// status 1 -> matched, Count 2
Assert.Equal(1, result[0].PickupStatusId);
Assert.NotNull(result[0].countInfo);
Assert.Equal(1, result[0].countInfo.pickupStatusId);
Assert.Equal(2, result[0].countInfo.Count);

// status 2 -> no match on the left join; whole non-entity object is null even after the second join
Assert.Equal(2, result[1].PickupStatusId);
Assert.Null(result[1].countInfo);

// status 3 -> matched, Count 1
Assert.Equal(3, result[2].PickupStatusId);
Assert.NotNull(result[2].countInfo);
Assert.Equal(3, result[2].countInfo.pickupStatusId);
Assert.Equal(1, result[2].countInfo.Count);
}

[Fact] // plain inner with no aggregate -> no pushdown
Expand Down Expand Up @@ -1211,12 +1227,119 @@ public virtual async Task Two_left_joined_nonentity_objects_second_marker_orphan
.LeftJoin(categories, e => e.PickupStatusId, c => c.pickupStatusId, (e, second) => new { e.PickupStatusId, e.first, second })
.OrderBy(e => e.PickupStatusId);

// Two sequential non-entity nullable joins: the FIRST object's marker must pass through the
// SECOND join's outer-shaper remap. It currently does not (Blocker-1).
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => query.ToListAsync());
Assert.Contains("Nullable object must have a value", ex.Message);
// #30915 TODO: Blocker-1 (graph-anchored keying); when fixed, flip to assert results and
// verify two distinct marker aliases.
var result = await query.ToListAsync();

Assert.Equal(3, result.Count);

// status 1 -> both joins matched, Count 2
Assert.Equal(1, result[0].PickupStatusId);
Assert.NotNull(result[0].first);
Assert.Equal(1, result[0].first.pickupStatusId);
Assert.Equal(2, result[0].first.Count);
Assert.NotNull(result[0].second);
Assert.Equal(1, result[0].second.pickupStatusId);
Assert.Equal(2, result[0].second.Count);

// status 2 -> neither join matched; both whole non-entity objects are null.
// The first object's marker passes through the second join's outer-shaper remap,
// and the second object carries its own marker — two distinct marker aliases in SQL.
Assert.Equal(2, result[1].PickupStatusId);
Assert.Null(result[1].first);
Assert.Null(result[1].second);

// status 3 -> both joins matched, Count 1
Assert.Equal(3, result[2].PickupStatusId);
Assert.NotNull(result[2].first);
Assert.Equal(3, result[2].first.pickupStatusId);
Assert.Equal(1, result[2].first.Count);
Assert.NotNull(result[2].second);
Assert.Equal(3, result[2].second.pickupStatusId);
Assert.Equal(1, result[2].second.Count);
}

[Fact]
public virtual async Task Three_sequential_joins_marker_survives_two_remaps()
{
// The marker, recorded on the non-entity inner-shaper node from the first DefaultIfEmpty, must
// survive TWO subsequent AddJoin outer-shaper remaps (re-key composes): one when the second
// .Join rebuilds the outer shaper and again when the third .Join does the same.
var contextFactory = await InitializeNonSharedTest<Context30915>(seed: Seed30915);
using var context = contextFactory.CreateDbContext();

var categories = context.Requests
.GroupBy(r => r.PickupStatusId, (k, els) => new { pickupStatusId = k, Count = els.Count() });

var query = (from s in context.Statuses
join c in categories on s.PickupStatusId equals c.pickupStatusId into g
from countInfo in g.DefaultIfEmpty()
select new { s.PickupStatusId, countInfo })
.Join(context.Statuses, e => e.PickupStatusId, s2 => s2.PickupStatusId, (e, s2) => new { s2.PickupStatusId, e.countInfo })
.Join(context.Statuses, e => e.PickupStatusId, s3 => s3.PickupStatusId, (e, s3) => new { s3.PickupStatusId, e.countInfo })
.OrderBy(e => e.PickupStatusId);

var result = await query.ToListAsync();

Assert.Equal(3, result.Count);

// status 1 -> matched, Count 2
Assert.Equal(1, result[0].PickupStatusId);
Assert.NotNull(result[0].countInfo);
Assert.Equal(1, result[0].countInfo.pickupStatusId);
Assert.Equal(2, result[0].countInfo.Count);

// status 2 -> no match on the left join; whole non-entity object is null even after TWO subsequent joins
Assert.Equal(2, result[1].PickupStatusId);
Assert.Null(result[1].countInfo);

// status 3 -> matched, Count 1
Assert.Equal(3, result[2].PickupStatusId);
Assert.NotNull(result[2].countInfo);
Assert.Equal(3, result[2].countInfo.pickupStatusId);
Assert.Equal(1, result[2].countInfo.Count);
}

[Fact]
public virtual async Task Marker_object_nested_in_outer_wrapper_across_second_join()
{
// The marker-carrying countInfo is nested inside an outer anonymous wrapper object constructed
// by the second .Join. RebuiltNodes must capture the nested rebuild so the marker still gates
// countInfo correctly after the join rebuilds the wrapping node.
var contextFactory = await InitializeNonSharedTest<Context30915>(seed: Seed30915);
using var context = contextFactory.CreateDbContext();

var categories = context.Requests
.GroupBy(r => r.PickupStatusId, (k, els) => new { pickupStatusId = k, Count = els.Count() });

var query = (from s in context.Statuses
join c in categories on s.PickupStatusId equals c.pickupStatusId into g
from countInfo in g.DefaultIfEmpty()
select new { s.PickupStatusId, countInfo })
.Join(context.Statuses, e => e.PickupStatusId, s2 => s2.PickupStatusId, (e, s2) => new { s2.PickupStatusId, wrapper = new { e.countInfo } })
.OrderBy(e => e.PickupStatusId);

var result = await query.ToListAsync();

Assert.Equal(3, result.Count);

// status 1 -> matched, wrapper is always non-null (constructed on match side), countInfo inside is non-null
Assert.Equal(1, result[0].PickupStatusId);
Assert.NotNull(result[0].wrapper);
Assert.NotNull(result[0].wrapper.countInfo);
Assert.Equal(1, result[0].wrapper.countInfo.pickupStatusId);
Assert.Equal(2, result[0].wrapper.countInfo.Count);

// status 2 -> no match on the left join; wrapper itself is non-null (it is always constructed),
// but wrapper.countInfo is null because the marker gates the inner non-entity object to null
Assert.Equal(2, result[1].PickupStatusId);
Assert.NotNull(result[1].wrapper);
Assert.Null(result[1].wrapper.countInfo);

// status 3 -> matched, wrapper is non-null, countInfo inside is non-null
Assert.Equal(3, result[2].PickupStatusId);
Assert.NotNull(result[2].wrapper);
Assert.NotNull(result[2].wrapper.countInfo);
Assert.Equal(3, result[2].wrapper.countInfo.pickupStatusId);
Assert.Equal(1, result[2].wrapper.countInfo.Count);
}

#endregion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3123,5 +3123,82 @@ ORDER BY [s0].[PickupStatusId]
""");
}

public override async Task Second_join_after_then_whole_object()
{
await base.Second_join_after_then_whole_object();

AssertSql(
"""
SELECT [s0].[PickupStatusId], [r0].[pickupStatusId], [r0].[Count], [r0].[marker]
FROM [Statuses] AS [s]
LEFT JOIN (
SELECT [r].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker]
FROM [Requests] AS [r]
GROUP BY [r].[PickupStatusId]
) AS [r0] ON [s].[PickupStatusId] = [r0].[pickupStatusId]
INNER JOIN [Statuses] AS [s0] ON [s].[PickupStatusId] = [s0].[PickupStatusId]
ORDER BY [s0].[PickupStatusId]
""");
}

public override async Task Two_left_joined_nonentity_objects_second_marker_orphaned()
{
await base.Two_left_joined_nonentity_objects_second_marker_orphaned();

AssertSql(
"""
SELECT [s].[PickupStatusId], [r0].[pickupStatusId], [r0].[Count], [r0].[marker], [r2].[pickupStatusId], [r2].[Count], [r2].[marker]
FROM [Statuses] AS [s]
LEFT JOIN (
SELECT [r].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker]
FROM [Requests] AS [r]
GROUP BY [r].[PickupStatusId]
) AS [r0] ON [s].[PickupStatusId] = [r0].[pickupStatusId]
LEFT JOIN (
SELECT [r1].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker]
FROM [Requests] AS [r1]
GROUP BY [r1].[PickupStatusId]
) AS [r2] ON [s].[PickupStatusId] = [r2].[pickupStatusId]
ORDER BY [s].[PickupStatusId]
""");
}

public override async Task Three_sequential_joins_marker_survives_two_remaps()
{
await base.Three_sequential_joins_marker_survives_two_remaps();

AssertSql(
"""
SELECT [s1].[PickupStatusId], [r0].[pickupStatusId], [r0].[Count], [r0].[marker]
FROM [Statuses] AS [s]
LEFT JOIN (
SELECT [r].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker]
FROM [Requests] AS [r]
GROUP BY [r].[PickupStatusId]
) AS [r0] ON [s].[PickupStatusId] = [r0].[pickupStatusId]
INNER JOIN [Statuses] AS [s0] ON [s].[PickupStatusId] = [s0].[PickupStatusId]
INNER JOIN [Statuses] AS [s1] ON [s0].[PickupStatusId] = [s1].[PickupStatusId]
ORDER BY [s1].[PickupStatusId]
""");
}

public override async Task Marker_object_nested_in_outer_wrapper_across_second_join()
{
await base.Marker_object_nested_in_outer_wrapper_across_second_join();

AssertSql(
"""
SELECT [s0].[PickupStatusId], [r0].[pickupStatusId], [r0].[Count], [r0].[marker]
FROM [Statuses] AS [s]
LEFT JOIN (
SELECT [r].[PickupStatusId] AS [pickupStatusId], COUNT(*) AS [Count], 1 AS [marker]
FROM [Requests] AS [r]
GROUP BY [r].[PickupStatusId]
) AS [r0] ON [s].[PickupStatusId] = [r0].[pickupStatusId]
INNER JOIN [Statuses] AS [s0] ON [s].[PickupStatusId] = [s0].[PickupStatusId]
ORDER BY [s0].[PickupStatusId]
""");
}

#endregion
}
Loading
Loading