Skip to content

Commit 730471d

Browse files
smitpatelsmitpatel
committed
Query: Implement First/Single/Last OrDefault throwing behavior
Subquery produces top(1) for Single/SingleOrDefault Single/First/SingleOrDefault in subquery is treated as if FirstOrDefault Last/LastOrDefault throws when there is no ordering on SelectExpression to reverse Resolves #15559
1 parent d0b21ed commit 730471d

16 files changed

+387
-121
lines changed

Diff for: ‎.gitignore

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
QueryBaseline.cs
1+
QueryBaseline.txt
22

33
## Ignore Visual Studio temporary files, build results, and
44
## files generated by popular Visual Studio add-ons.

Diff for: ‎src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

+10-2
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ public class RelationalQueryableMethodTranslatingExpressionVisitor : QueryableMe
2525
private readonly RelationalProjectionBindingExpressionVisitor _projectionBindingExpressionVisitor;
2626
private readonly IModel _model;
2727
private readonly ISqlExpressionFactory _sqlExpressionFactory;
28+
private readonly bool _subquery;
2829

2930
public RelationalQueryableMethodTranslatingExpressionVisitor(
3031
QueryableMethodTranslatingExpressionVisitorDependencies dependencies,
@@ -40,6 +41,7 @@ public RelationalQueryableMethodTranslatingExpressionVisitor(
4041
_projectionBindingExpressionVisitor = new RelationalProjectionBindingExpressionVisitor(this, _sqlTranslator);
4142
_model = model;
4243
_sqlExpressionFactory = sqlExpressionFactory;
44+
_subquery = false;
4345
}
4446

4547
protected virtual RelationalQueryableMethodTranslatingExpressionVisitorDependencies RelationalDependencies { get; }
@@ -54,6 +56,7 @@ protected RelationalQueryableMethodTranslatingExpressionVisitor(
5456
_weakEntityExpandingExpressionVisitor = parentVisitor._weakEntityExpandingExpressionVisitor;
5557
_projectionBindingExpressionVisitor = new RelationalProjectionBindingExpressionVisitor(this, _sqlTranslator);
5658
_sqlExpressionFactory = parentVisitor._sqlExpressionFactory;
59+
_subquery = true;
5760
}
5861

5962
protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
@@ -555,12 +558,17 @@ private SqlBinaryExpression CreateJoinPredicate(
555558
protected override ShapedQueryExpression TranslateLastOrDefault(
556559
ShapedQueryExpression source, LambdaExpression predicate, Type returnType, bool returnDefault)
557560
{
561+
var selectExpression = (SelectExpression)source.QueryExpression;
562+
if (selectExpression.Orderings.Count == 0)
563+
{
564+
return null;
565+
}
566+
558567
if (predicate != null)
559568
{
560569
source = TranslateWhere(source, predicate);
561570
}
562571

563-
var selectExpression = (SelectExpression)source.QueryExpression;
564572
selectExpression.ReverseOrderings();
565573
selectExpression.ApplyLimit(TranslateExpression(Expression.Constant(1)));
566574

@@ -849,7 +857,7 @@ protected override ShapedQueryExpression TranslateSingleOrDefault(ShapedQueryExp
849857
}
850858

851859
var selectExpression = (SelectExpression)source.QueryExpression;
852-
selectExpression.ApplyLimit(TranslateExpression(Expression.Constant(2)));
860+
selectExpression.ApplyLimit(TranslateExpression(Expression.Constant(_subquery ? 1 : 2)));
853861

854862
if (source.ShaperExpression.Type != returnType)
855863
{

Diff for: ‎test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs

+42
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,12 @@ FROM root c
347347
WHERE (c[""Discriminator""] = ""Employee"")");
348348
}
349349

350+
[ConditionalTheory(Skip = "Issue #17246")]
351+
public override Task Where_query_composition_entity_equality_one_element_Single(bool isAsync)
352+
{
353+
return base.Where_query_composition_entity_equality_one_element_Single(isAsync);
354+
}
355+
350356
[ConditionalTheory(Skip = "Issue#17246")]
351357
public override async Task Where_query_composition_entity_equality_one_element_FirstOrDefault(bool isAsync)
352358
{
@@ -358,6 +364,12 @@ FROM root c
358364
WHERE (c[""Discriminator""] = ""Employee"")");
359365
}
360366

367+
[ConditionalTheory(Skip = "Issue #17246")]
368+
public override Task Where_query_composition_entity_equality_one_element_First(bool isAsync)
369+
{
370+
return base.Where_query_composition_entity_equality_one_element_First(isAsync);
371+
}
372+
361373
[ConditionalTheory(Skip = "Issue #17246")]
362374
public override async Task Where_query_composition_entity_equality_no_elements_SingleOrDefault(bool isAsync)
363375
{
@@ -369,6 +381,12 @@ FROM root c
369381
WHERE (c[""Discriminator""] = ""Employee"")");
370382
}
371383

384+
[ConditionalTheory(Skip = "Issue #17246")]
385+
public override Task Where_query_composition_entity_equality_no_elements_Single(bool isAsync)
386+
{
387+
return base.Where_query_composition_entity_equality_no_elements_Single(isAsync);
388+
}
389+
372390
[ConditionalTheory(Skip = "Issue#17246")]
373391
public override async Task Where_query_composition_entity_equality_no_elements_FirstOrDefault(bool isAsync)
374392
{
@@ -380,6 +398,24 @@ FROM root c
380398
WHERE (c[""Discriminator""] = ""Employee"")");
381399
}
382400

401+
[ConditionalTheory(Skip = "Issue #17246")]
402+
public override Task Where_query_composition_entity_equality_no_elements_First(bool isAsync)
403+
{
404+
return base.Where_query_composition_entity_equality_no_elements_First(isAsync);
405+
}
406+
407+
[ConditionalTheory(Skip = "Issue #17246")]
408+
public override Task Where_query_composition_entity_equality_multiple_elements_SingleOrDefault(bool isAsync)
409+
{
410+
return base.Where_query_composition_entity_equality_multiple_elements_SingleOrDefault(isAsync);
411+
}
412+
413+
[ConditionalTheory(Skip = "Issue #17246")]
414+
public override Task Where_query_composition_entity_equality_multiple_elements_Single(bool isAsync)
415+
{
416+
return base.Where_query_composition_entity_equality_multiple_elements_Single(isAsync);
417+
}
418+
383419
[ConditionalTheory(Skip = "Issue#17246")]
384420
public override async Task Where_query_composition_entity_equality_multiple_elements_FirstOrDefault(bool isAsync)
385421
{
@@ -391,6 +427,12 @@ FROM root c
391427
WHERE (c[""Discriminator""] = ""Employee"")");
392428
}
393429

430+
[ConditionalTheory(Skip = "Issue #17246")]
431+
public override Task Where_query_composition_entity_equality_multiple_elements_First(bool isAsync)
432+
{
433+
return base.Where_query_composition_entity_equality_multiple_elements_First(isAsync);
434+
}
435+
394436
[ConditionalTheory(Skip = "Issue #17246")]
395437
public override async Task Where_query_composition2(bool isAsync)
396438
{

Diff for: ‎test/EFCore.InMemory.FunctionalTests/Query/IncludeInMemoryTest.cs

+6
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,11 @@ public IncludeInMemoryTest(IncludeInMemoryFixture fixture, ITestOutputHelper tes
1818
public override void Include_collection_with_client_filter(bool useString)
1919
{
2020
}
21+
22+
[ConditionalTheory(Skip = "Issue #16963")]
23+
public override void Include_collection_with_last_no_orderby(bool useString)
24+
{
25+
base.Include_collection_with_last_no_orderby(useString);
26+
}
2127
}
2228
}

Diff for: ‎test/EFCore.InMemory.FunctionalTests/Query/SimpleQueryInMemoryTest.cs

+42
Original file line numberDiff line numberDiff line change
@@ -381,5 +381,47 @@ public override Task Projection_when_client_evald_subquery(bool isAsync)
381381

382382
[ConditionalTheory(Skip = "Issue #16963")]
383383
public override Task Select_bool_closure_with_order_by_property_with_cast_to_nullable(bool isAsync) => null;
384+
385+
[ConditionalTheory(Skip = "Issue #16963")]
386+
public override Task Where_query_composition_entity_equality_one_element_Single(bool isAsync)
387+
{
388+
return base.Where_query_composition_entity_equality_one_element_Single(isAsync);
389+
}
390+
391+
[ConditionalTheory(Skip = "Issue #16963")]
392+
public override Task Where_query_composition_entity_equality_one_element_First(bool isAsync)
393+
{
394+
return base.Where_query_composition_entity_equality_one_element_First(isAsync);
395+
}
396+
397+
[ConditionalTheory(Skip = "Issue #16963")]
398+
public override Task Where_query_composition_entity_equality_no_elements_Single(bool isAsync)
399+
{
400+
return base.Where_query_composition_entity_equality_no_elements_Single(isAsync);
401+
}
402+
403+
[ConditionalTheory(Skip = "Issue #16963")]
404+
public override Task Where_query_composition_entity_equality_no_elements_First(bool isAsync)
405+
{
406+
return base.Where_query_composition_entity_equality_no_elements_First(isAsync);
407+
}
408+
409+
[ConditionalTheory(Skip = "Issue #16963")]
410+
public override Task Where_query_composition_entity_equality_multiple_elements_SingleOrDefault(bool isAsync)
411+
{
412+
return base.Where_query_composition_entity_equality_multiple_elements_SingleOrDefault(isAsync);
413+
}
414+
415+
[ConditionalTheory(Skip = "Issue #16963")]
416+
public override Task Where_query_composition_entity_equality_multiple_elements_Single(bool isAsync)
417+
{
418+
return base.Where_query_composition_entity_equality_multiple_elements_Single(isAsync);
419+
}
420+
421+
[ConditionalTheory(Skip = "Issue #16963")]
422+
public override Task Last_when_no_order_by(bool isAsync)
423+
{
424+
return base.Last_when_no_order_by(isAsync);
425+
}
384426
}
385427
}

Diff for: ‎test/EFCore.Relational.Specification.Tests/TransactionTestBase.cs

+8-8
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ public virtual void SaveChanges_can_be_used_with_no_transaction()
5151
});
5252

5353

54-
context.Entry(context.Set<TransactionCustomer>().Last()).State = EntityState.Added;
54+
context.Entry(context.Set<TransactionCustomer>().OrderBy(c => c.Id).Last()).State = EntityState.Added;
5555

5656
Assert.Throws<DbUpdateException>(() => context.SaveChanges());
5757

@@ -176,7 +176,7 @@ public virtual async Task SaveChanges_uses_enlisted_transaction(bool async, bool
176176
Name = "Bobble"
177177
});
178178

179-
context.Entry(context.Set<TransactionCustomer>().Last()).State = EntityState.Added;
179+
context.Entry(context.Set<TransactionCustomer>().OrderBy(c => c.Id).Last()).State = EntityState.Added;
180180

181181
if (async)
182182
{
@@ -250,7 +250,7 @@ public virtual async Task SaveChanges_uses_enlisted_transaction_after_connection
250250
Name = "Bobble"
251251
});
252252

253-
context.Entry(context.Set<TransactionCustomer>().Last()).State = EntityState.Added;
253+
context.Entry(context.Set<TransactionCustomer>().OrderBy(c => c.Id).Last()).State = EntityState.Added;
254254

255255
context.Database.AutoTransactionsEnabled = true;
256256
}
@@ -302,7 +302,7 @@ public virtual async Task SaveChanges_uses_enlisted_transaction_connectionString
302302
Name = "Bobble"
303303
});
304304

305-
context.Entry(context.Set<TransactionCustomer>().Last()).State = EntityState.Added;
305+
context.Entry(context.Set<TransactionCustomer>().OrderBy(c => c.Id).Last()).State = EntityState.Added;
306306

307307
if (async)
308308
{
@@ -347,7 +347,7 @@ public virtual async Task SaveChanges_uses_ambient_transaction(bool async, bool
347347
Name = "Bobble"
348348
});
349349

350-
context.Entry(context.Set<TransactionCustomer>().Last()).State = EntityState.Added;
350+
context.Entry(context.Set<TransactionCustomer>().OrderBy(c => c.Id).Last()).State = EntityState.Added;
351351

352352
if (async)
353353
{
@@ -421,7 +421,7 @@ public virtual async Task SaveChanges_uses_ambient_transaction_with_connectionSt
421421
Name = "Bobble"
422422
});
423423

424-
context.Entry(context.Set<TransactionCustomer>().Last()).State = EntityState.Added;
424+
context.Entry(context.Set<TransactionCustomer>().OrderBy(c => c.Id).Last()).State = EntityState.Added;
425425

426426
if (async)
427427
{
@@ -464,7 +464,7 @@ public virtual void SaveChanges_throws_for_suppressed_ambient_transactions(bool
464464
Name = "Bobble"
465465
});
466466

467-
context.Entry(context.Set<TransactionCustomer>().Last()).State = EntityState.Added;
467+
context.Entry(context.Set<TransactionCustomer>().OrderBy(c => c.Id).Last()).State = EntityState.Added;
468468

469469
using (new TransactionScope(TransactionScopeOption.Suppress))
470470
{
@@ -501,7 +501,7 @@ public virtual void SaveChanges_uses_enlisted_transaction_after_ambient_transact
501501
Name = "Bobble"
502502
});
503503

504-
context.Entry(context.Set<TransactionCustomer>().Last()).State = EntityState.Added;
504+
context.Entry(context.Set<TransactionCustomer>().OrderBy(c => c.Id).Last()).State = EntityState.Added;
505505
}
506506

507507
using (var transaction = new CommittableTransaction(TimeSpan.FromMinutes(10)))

Diff for: ‎test/EFCore.Specification.Tests/ConcurrencyDetectorTestBase.cs

+2-2
Original file line numberDiff line numberDiff line change
@@ -98,15 +98,15 @@ public virtual Task Last_logs_concurrent_access_nonasync()
9898
return ConcurrencyDetectorTest(
9999
c =>
100100
{
101-
var result = c.Products.Last();
101+
var result = c.Products.OrderBy(p => p.ProductID).Last();
102102
return Task.FromResult(false);
103103
});
104104
}
105105

106106
[ConditionalFact]
107107
public virtual Task Last_logs_concurrent_access_async()
108108
{
109-
return ConcurrencyDetectorTest(c => c.Products.LastAsync());
109+
return ConcurrencyDetectorTest(c => c.Products.OrderBy(p => p.ProductID).LastAsync());
110110
}
111111

112112
[ConditionalFact]

Diff for: ‎test/EFCore.Specification.Tests/Query/IncludeTestBase.cs

+9-13
Original file line numberDiff line numberDiff line change
@@ -371,25 +371,19 @@ var customer
371371
}
372372
}
373373

374-
[ConditionalTheory(Skip = "Issue#15559")]
374+
[ConditionalTheory]
375375
[InlineData(false)]
376376
[InlineData(true)]
377377
public virtual void Include_collection_with_last_no_orderby(bool useString)
378378
{
379379
using (var context = CreateContext())
380380
{
381-
var customer
382-
= useString
383-
? context.Set<Customer>()
384-
.Include("Orders")
385-
.Last()
386-
: context.Set<Customer>()
387-
.Include(c => c.Orders)
388-
.Last();
389-
390-
Assert.NotNull(customer);
391-
Assert.Equal(7, customer.Orders.Count);
392-
Assert.Equal(8, context.ChangeTracker.Entries().Count());
381+
Assert.Equal(
382+
CoreStrings.TranslationFailed(@"Last<Customer>(Select<Customer, Customer>( source: DbSet<Customer>, selector: (c) => IncludeExpression( c, MaterializeCollectionNavigation(Navigation: Customer.Orders (<Orders>k__BackingField, List<Order>) Collection ToDependent Order Inverse: Customer PropertyAccessMode.Field, Where<Order>( source: DbSet<Order>, predicate: (o) => Property<string>(c, ""CustomerID"") == Property<string>(o, ""CustomerID""))), Orders)))"),
383+
RemoveNewLines(Assert.Throws<InvalidOperationException>(
384+
() => useString
385+
? context.Set<Customer>().Include("Orders").Last()
386+
: context.Set<Customer>().Include(c => c.Orders).Last()).Message));
393387
}
394388
}
395389

@@ -4327,6 +4321,8 @@ private static void CheckIsLoaded(
43274321
}
43284322
}
43294323

4324+
private string RemoveNewLines(string message) => message.Replace("\n", "").Replace("\r", "");
4325+
43304326
protected virtual void ClearLog()
43314327
{
43324328
}

Diff for: ‎test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.ResultOperators.cs

+11-6
Original file line numberDiff line numberDiff line change
@@ -1084,12 +1084,16 @@ public virtual Task Last(bool isAsync)
10841084

10851085
[ConditionalTheory]
10861086
[MemberData(nameof(IsAsyncData))]
1087-
public virtual Task Last_when_no_order_by(bool isAsync)
1087+
public virtual async Task Last_when_no_order_by(bool isAsync)
10881088
{
1089-
return AssertLast<Customer>(
1090-
isAsync,
1091-
cs => cs.Where(c => c.CustomerID == "ALFKI"),
1092-
entryCount: 1);
1089+
Assert.Equal(
1090+
CoreStrings.TranslationFailed(@"Last<object>(Select<Customer, object>( source: Where<Customer>( source: DbSet<Customer>, predicate: (c) => c.CustomerID == ""ALFKI""), selector: (c) => (object)c))"),
1091+
RemoveNewLines(
1092+
(await Assert.ThrowsAsync<InvalidOperationException>(
1093+
() => AssertLast<Customer>(
1094+
isAsync,
1095+
cs => cs.Where(c => c.CustomerID == "ALFKI"),
1096+
entryCount: 1))).Message));
10931097
}
10941098

10951099
[ConditionalTheory]
@@ -1492,7 +1496,8 @@ public virtual async Task Contains_with_local_anonymous_type_array_closure(bool
14921496
o => ids.Contains(
14931497
new
14941498
{
1495-
Id1 = o.OrderID, Id2 = o.ProductID
1499+
Id1 = o.OrderID,
1500+
Id2 = o.ProductID
14961501
})), entryCount: 1))).Message));
14971502
}
14981503

0 commit comments

Comments
 (0)