From 3658b71d437f96c3c73485324ac682a60856af69 Mon Sep 17 00:00:00 2001 From: maumar Date: Wed, 5 Aug 2020 18:23:55 -0700 Subject: [PATCH] Fix to #15873 - Query: Identifying columns in the case of distinct Added validation step for AddCollectionJoin which checks that if subquery contains Distinct or GroupBy, the projection contains all identifying columns needed to correctly bucket the results during materialization. Also making sure that identifying columns can be correctly propagated during pushdown and joining - if they are not we mark them as such (by removing identifying columns altogether), so that we can throw exception when these columns are actually needed. Fixes #15873 --- .../Properties/RelationalStrings.Designer.cs | 12 ++- .../Properties/RelationalStrings.resx | 7 +- .../Query/SqlExpressions/SelectExpression.cs | 72 ++++++++++++++--- .../Query/NorthwindSelectQueryCosmosTest.cs | 6 -- .../Query/OwnedQueryCosmosTest.cs | 16 ++++ .../Query/OwnedQueryInMemoryTest.cs | 8 ++ .../GearsOfWarQueryRelationalTestBase.cs | 41 ++++++++++ ...NorthwindGroupByQueryRelationalTestBase.cs | 81 +++++++++++++++++++ ...dKeylessEntitiesQueryRelationalTestBase.cs | 2 +- .../Query/UdfDbFunctionTestBase.cs | 23 +++--- .../Query/NorthwindGroupByQueryTestBase.cs | 63 +++++++++------ .../Query/NorthwindSelectQueryTestBase.cs | 2 +- .../Query/OwnedQueryTestBase.cs | 19 +++++ ...omplexNavigationsWeakQuerySqlServerTest.cs | 11 +++ .../NorthwindGroupByQuerySqlServerTest.cs | 47 +++++++++-- ...orthwindMiscellaneousQuerySqlServerTest.cs | 27 +++++++ .../NorthwindSelectQuerySqlServerTest.cs | 23 ++---- .../Query/OwnedQuerySqlServerTest.cs | 12 +++ .../Query/UdfDbFunctionSqlServerTests.cs | 16 ++-- .../ComplexNavigationsWeakQuerySqliteTest.cs | 10 +++ .../Query/NorthwindGroupByQuerySqliteTest.cs | 5 ++ .../Query/NorthwindSelectQuerySqliteTest.cs | 6 +- 22 files changed, 413 insertions(+), 96 deletions(-) diff --git a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs index 0ac8ffbb476..07d33dd45a8 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs +++ b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs @@ -974,10 +974,10 @@ public static string SequenceContainsNoElements => GetString("SequenceContainsNoElements"); /// - /// Projecting collection correlated with keyless entity is not supported. + /// Not enough information to uniquely identify outer element in correlated collection scenario. This can happen when trying to correlate on keyless entity or when using 'Distinct' or 'GroupBy' operations without projecting all of the key columns. /// - public static string ProjectingCollectionOnKeylessEntityNotSupported - => GetString("ProjectingCollectionOnKeylessEntityNotSupported"); + public static string InsufficientInformationToIdentifyOuterElementOfCollectionJoin + => GetString("InsufficientInformationToIdentifyOuterElementOfCollectionJoin"); /// /// Cannot use view '{view}' for entity type '{entityType}' since it is being used for entity type '{otherEntityType}', there is a relationship between their primary keys in which '{entityType}' is the dependent and '{entityType}' has a base entity type mapped to a different view. Either map '{otherEntityType}' to a different view or invert the relationship between '{entityType}' and '{otherEntityType}'. @@ -1039,6 +1039,12 @@ public static string InvalidDerivedTypeInEntityProjection([CanBeNull] object der public static string MissingOrderingInSqlExpression => GetString("MissingOrderingInSqlExpression"); + /// + /// Collection subquery that uses 'Distinct' or 'Group By' operations must project key columns of all of it's tables. Missing column: {column}. Either add column(s) to the projection or rewrite query to not use 'GroupBy'/'Distinct' operation. + /// + public static string MissingIdentifyingProjectionInDistinctGroupBySubquery([CanBeNull] object column) + => string.Format(GetString("MissingIdentifyingProjectionInDistinctGroupBySubquery", nameof(column)), column); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/EFCore.Relational/Properties/RelationalStrings.resx b/src/EFCore.Relational/Properties/RelationalStrings.resx index 938a565331b..7a7687dd4fd 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.resx +++ b/src/EFCore.Relational/Properties/RelationalStrings.resx @@ -700,8 +700,8 @@ Sequence contains no elements. - - Projecting collection correlated with keyless entity is not supported. + + Not enough information to uniquely identify outer element in correlated collection scenario. This can happen when trying to correlate on keyless entity or when using 'Distinct' or 'GroupBy' operations without projecting all of the key columns. An error occurred while the batch executor was rolling back the transaction to a savepoint, after an exception occured. @@ -735,4 +735,7 @@ Reverse could not be translated to the server because there is no ordering on the server side. + + Collection subquery that uses 'Distinct' or 'Group By' operations must project key columns of all of it's tables. Missing column: {column}. Either add column(s) to the projection or rewrite query to not use 'GroupBy'/'Distinct' operation. + \ No newline at end of file diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index 8276f228a56..44d4f98fb18 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -1070,7 +1070,7 @@ public IDictionary PushdownIntoSubquery() var identifiers = _identifier.ToList(); _identifier.Clear(); - // TODO: See issue#15873 + foreach (var identifier in identifiers) { if (projectionMap.TryGetValue(identifier.Column, out var outerColumn)) @@ -1078,16 +1078,24 @@ public IDictionary PushdownIntoSubquery() _identifier.Add((outerColumn, identifier.Comparer)); } else if (!IsDistinct - && GroupBy.Count == 0) + && GroupBy.Count == 0 + || (GroupBy.Contains(identifier.Column))) { outerColumn = subquery.GenerateOuterColumn(identifier.Column); _identifier.Add((outerColumn, identifier.Comparer)); } + else + { + // if we can't propagate any identifier - clear them all instead + // when adding collection join we detect this and throw appropriate exception + _identifier.Clear(); + break; + } } var childIdentifiers = _childIdentifiers.ToList(); _childIdentifiers.Clear(); - // TODO: See issue#15873 + foreach (var identifier in childIdentifiers) { if (projectionMap.TryGetValue(identifier.Column, out var outerColumn)) @@ -1095,11 +1103,19 @@ public IDictionary PushdownIntoSubquery() _childIdentifiers.Add((outerColumn, identifier.Comparer)); } else if (!IsDistinct - && GroupBy.Count == 0) + && GroupBy.Count == 0 + || (GroupBy.Contains(identifier.Column))) { outerColumn = subquery.GenerateOuterColumn(identifier.Column); _childIdentifiers.Add((outerColumn, identifier.Comparer)); } + else + { + // if we can't propagate any identifier - clear them all instead + // when adding collection join we detect this and throw appropriate exception + _childIdentifiers.Clear(); + break; + } } var pendingCollections = _pendingCollections.ToList(); @@ -1328,10 +1344,16 @@ public Expression ApplyCollectionJoin( var innerSelectExpression = _pendingCollections[collectionIndex]; _pendingCollections[collectionIndex] = null; + if (_identifier.Count == 0) + { + throw new InvalidOperationException(RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin); + } + if (splitQuery) { var parentIdentifier = GetIdentifierAccessor(_identifier).Item1; innerSelectExpression.ApplyProjection(); + ValidateIdentifyingProjection(innerSelectExpression); for (var i = 0; i < _identifier.Count; i++) { @@ -1421,16 +1443,14 @@ public Expression ApplyCollectionJoin( else { var parentIdentifierList = _identifier.Except(_childIdentifiers).ToList(); - if (parentIdentifierList.Count == 0) - { - throw new InvalidOperationException(RelationalStrings.ProjectingCollectionOnKeylessEntityNotSupported); - } var (parentIdentifier, parentIdentifierValueComparers) = GetIdentifierAccessor(parentIdentifierList); var (outerIdentifier, outerIdentifierValueComparers) = GetIdentifierAccessor(_identifier); var innerClientEval = innerSelectExpression.Projection.Count > 0; innerSelectExpression.ApplyProjection(); + ValidateIdentifyingProjection(innerSelectExpression); + if (collectionIndex == 0) { foreach (var identifier in parentIdentifierList) @@ -1547,6 +1567,24 @@ public Expression ApplyCollectionJoin( return result; } + + static void ValidateIdentifyingProjection(SelectExpression selectExpression) + { + if (selectExpression.IsDistinct + || selectExpression.GroupBy.Count > 0) + { + var innerSelectProjectionExpressions = selectExpression._projection.Select(p => p.Expression).ToList(); + foreach (var innerSelectIdentifier in selectExpression._identifier) + { + if (!innerSelectProjectionExpressions.Contains(innerSelectIdentifier.Column) + && (selectExpression.GroupBy.Count == 0 + || !selectExpression.GroupBy.Contains(innerSelectIdentifier.Column))) + + throw new InvalidOperationException(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery( + innerSelectIdentifier.Column.Table.Alias + "." + innerSelectIdentifier.Column.Name)); + } + } + } } private sealed class EntityShaperNullableMarkingExpressionVisitor : ExpressionVisitor @@ -2173,14 +2211,24 @@ private void AddJoin( .Remap(joinPredicate); } - if (joinType == JoinType.LeftJoin - || joinType == JoinType.OuterApply) + // if the subquery that is joined to can't be uniquely identified + // then the entire join should also not be marked as non-identifiable + if (innerSelectExpression._identifier.Count == 0 + || _identifier.Count == 0) { - _identifier.AddRange(innerSelectExpression._identifier.Select(e => (e.Column.MakeNullable(), e.Comparer))); + _identifier.Clear(); } else { - _identifier.AddRange(innerSelectExpression._identifier); + if (joinType == JoinType.LeftJoin + || joinType == JoinType.OuterApply) + { + _identifier.AddRange(innerSelectExpression._identifier.Select(e => (e.Column.MakeNullable(), e.Comparer))); + } + else + { + _identifier.AddRange(innerSelectExpression._identifier); + } } var innerTable = innerSelectExpression.Tables.Single(); diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs index 38440d195b3..bdba375731a 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs @@ -1139,12 +1139,6 @@ public override Task Projecting_multiple_collection_with_same_constant_works(boo return base.Projecting_multiple_collection_with_same_constant_works(async); } - [ConditionalTheory(Skip = "Issue#17246")] - public override Task Projecting_after_navigation_and_distinct_works_correctly(bool async) - { - return base.Projecting_after_navigation_and_distinct_works_correctly(async); - } - public override Task Reverse_without_explicit_ordering_throws(bool async) { return AssertTranslationFailedWithDetails( diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs index 28525393c32..d8f07a68355 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs @@ -474,6 +474,22 @@ public override async Task Ordering_by_identifying_projection(bool async) AssertSql(" "); } + [ConditionalTheory(Skip = "issue #17246")] + public override async Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool isAsync) + { + await base.Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(isAsync); + + AssertSql(" "); + } + + [ConditionalTheory(Skip = "issue #17246")] + public override async Task Projecting_after_navigation_and_distinct_throws(bool isAsync) + { + await base.Projecting_after_navigation_and_distinct_throws(isAsync); + + AssertSql(" "); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); diff --git a/test/EFCore.InMemory.FunctionalTests/Query/OwnedQueryInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/Query/OwnedQueryInMemoryTest.cs index 9ea51a6a553..08e5828b507 100644 --- a/test/EFCore.InMemory.FunctionalTests/Query/OwnedQueryInMemoryTest.cs +++ b/test/EFCore.InMemory.FunctionalTests/Query/OwnedQueryInMemoryTest.cs @@ -1,7 +1,9 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Threading.Tasks; using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; using Xunit.Abstractions; namespace Microsoft.EntityFrameworkCore.Query @@ -14,6 +16,12 @@ public OwnedQueryInMemoryTest(OwnedQueryInMemoryFixture fixture, ITestOutputHelp //TestLoggerFactory.TestOutputHelper = testOutputHelper; } + [ConditionalTheory(Skip = "issue #19742")] + public override Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool async) + { + return base.Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(async); + } + public class OwnedQueryInMemoryFixture : OwnedQueryFixtureBase { protected override ITestStoreFactory TestStoreFactory => InMemoryTestStoreFactory.Instance; diff --git a/test/EFCore.Relational.Specification.Tests/Query/GearsOfWarQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/GearsOfWarQueryRelationalTestBase.cs index bc861a5a56d..a6c21e74559 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/GearsOfWarQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/GearsOfWarQueryRelationalTestBase.cs @@ -1,7 +1,13 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.TestModels.GearsOfWarModel; using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; namespace Microsoft.EntityFrameworkCore.Query { @@ -13,6 +19,41 @@ protected GearsOfWarQueryRelationalTestBase(TFixture fixture) { } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Correlated_collection_with_Distinct_missing_indentifying_columns_in_projection(bool async) + { + var message = (await Assert.ThrowsAsync( + () => AssertQuery( + async, + ss => ss.Set() + .OrderBy(g => g.Nickname) + .Select(g => g.Weapons.SelectMany(x => x.Owner.AssignedCity.BornGears) + .Select(x => (bool?)x.HasSoulPatch).Distinct().ToList())))).Message; + + Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("w.Id"), message); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Correlated_collection_with_GroupBy_missing_indentifying_columns_in_projection(bool async) + { + var message = (await Assert.ThrowsAsync( + () => AssertQuery( + async, + ss => ss.Set() + .Select(m => new + { + m.Id, + grouping = m.ParticipatingSquads + .Select(ps => ps.SquadId) + .GroupBy(s => s) + .Select(g => new { g.Key, Count = g.Count() }) + })))).Message; + + Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("s.MissionId"), message); + } + protected virtual bool CanExecuteQueryString => false; protected override QueryAsserter CreateQueryAsserter(TFixture fixture) diff --git a/test/EFCore.Relational.Specification.Tests/Query/NorthwindGroupByQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/NorthwindGroupByQueryRelationalTestBase.cs index 5181fcbfb97..8190e2a8048 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/NorthwindGroupByQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/NorthwindGroupByQueryRelationalTestBase.cs @@ -1,7 +1,13 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.TestModels.Northwind; using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; namespace Microsoft.EntityFrameworkCore.Query { @@ -13,6 +19,81 @@ protected NorthwindGroupByQueryRelationalTestBase(TFixture fixture) { } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Select_uncorrelated_collection_with_groupby_when_outer_is_distinct(bool async) + { + var message = (await Assert.ThrowsAsync( + () => AssertQuery( + async, + ss => ss.Set() + .Where(c => c.CustomerID.StartsWith("A")) + .Select(c => c.Customer.City) + .Distinct() + .Select(c => new + { + c1 = ss.Set().GroupBy(p => p.ProductID).Select(g => g.Key).ToArray(), + c2 = ss.Set().GroupBy(p => p.ProductID).Select(g => g.Count()).ToArray() + }), + assertOrder: true, + elementAsserter: (e, a) => + { + AssertCollection(e.c1, a.c1); + AssertCollection(e.c2, a.c2); + }))).Message; + + Assert.Equal(RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin, message); + } + + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Complex_query_with_groupBy_in_subquery4(bool async) + { + var message = (await Assert.ThrowsAsync( + () => AssertQuery( + async, + ss => ss.Set() + .Select( + c => new + { + Key = c.CustomerID, + Subquery = c.Orders + .Select(o => new { First = o.OrderID, Second = o.Customer.City + o.CustomerID }) + .GroupBy(x => x.Second) + .Select(g => new { Sum = g.Sum(x => x.First), Count = g.Count(x => x.Second.StartsWith("Lon")) }).ToList() + }), + elementSorter: e => e.Key, + elementAsserter: (e, a) => + { + Assert.Equal(e.Key, a.Key); + AssertCollection(e.Subquery, a.Subquery); + }))).Message; + + Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("o.OrderID"), message); + } + + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Select_nested_collection_with_distinct(bool async) + { + var message = (await Assert.ThrowsAsync( + () => AssertQuery( + async, + ss => ss.Set() + .OrderBy(c => c.CustomerID) + .Where(c => c.CustomerID.StartsWith("A")) + .Select( + c => c.Orders.Any() + ? c.Orders.Select(o => o.CustomerID).Distinct().ToArray() + : Array.Empty()), + assertOrder: true, + elementAsserter: (e, a) => AssertCollection(e, a)))).Message; + + Assert.Equal(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery("o.OrderID"), message); + } + protected virtual bool CanExecuteQueryString => false; protected override QueryAsserter CreateQueryAsserter(TFixture fixture) diff --git a/test/EFCore.Relational.Specification.Tests/Query/NorthwindKeylessEntitiesQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/NorthwindKeylessEntitiesQueryRelationalTestBase.cs index ba4297bf27b..1700ca2b692 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/NorthwindKeylessEntitiesQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/NorthwindKeylessEntitiesQueryRelationalTestBase.cs @@ -35,7 +35,7 @@ public virtual async Task Projecting_collection_correlated_with_keyless_entity_t OrderDetailIds = ss.Set().Where(c => c.City == cq.City).ToList() }).OrderBy(x => x.City).Take(2)))).Message; - Assert.Equal(RelationalStrings.ProjectingCollectionOnKeylessEntityNotSupported, message); + Assert.Equal(RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin, message); } protected override QueryAsserter CreateQueryAsserter(TFixture fixture) diff --git a/test/EFCore.Relational.Specification.Tests/Query/UdfDbFunctionTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/UdfDbFunctionTestBase.cs index 8928fc86b6a..cac01fd0dd8 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/UdfDbFunctionTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/UdfDbFunctionTestBase.cs @@ -1318,17 +1318,22 @@ public virtual void Scalar_Nested_Function_UDF_BCL_Instance() #region TableValuedFunction - [ConditionalFact(Skip = "Issue#15873")] + [ConditionalFact] public virtual void QF_Anonymous_Collection_No_PK_Throws() { using (var context = CreateContext()) { - var query = from c in context.Customers - select new { c.Id, products = context.GetTopSellingProductsForCustomer(c.Id).ToList() }; - - //Assert.Contains( - // RelationalStrings.DbFunctionProjectedCollectionMustHavePK("GetTopSellingProductsForCustomer"), - // Assert.Throws(() => query.ToList()).Message); + var query = (from c in context.Customers + select new + { + c.Id, + products = context.GetTopSellingProductsForCustomer(c.Id).ToList(), + orders = context.Orders.Where(o => o.CustomerId == c.Id).ToList() + }); + + Assert.Equal( + RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin, + Assert.Throws(() => query.ToList()).Message); } } @@ -1572,8 +1577,8 @@ public virtual void QF_Select_Correlated_Subquery_In_Anonymous_MultipleCollectio select new { c.Id, - Prods = context.GetTopTwoSellingProducts().Where(p => p.AmountSold == 249).Select(p => p.ProductId).ToList(), - Addresses = c.Addresses.Where(a => a.State == "NY").ToList() + Addresses = c.Addresses.Where(a => a.State == "NY").ToList(), + Prods = context.GetTopTwoSellingProducts().Where(p => p.AmountSold == 249).Select(p => p.ProductId).ToList() }).ToList(); Assert.Equal(4, results.Count); diff --git a/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs index a074cb63c87..c1a53a089b0 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs @@ -2007,7 +2007,7 @@ public virtual Task Distinct_GroupBy_OrderBy_key(bool async) assertOrder: true); } - [ConditionalTheory(Skip = "Issue #15873")] + [ConditionalTheory(Skip = "Issue #21965")] [MemberData(nameof(IsAsyncData))] public virtual Task Select_nested_collection_with_groupby(bool async) { @@ -2020,6 +2020,42 @@ public virtual Task Select_nested_collection_with_groupby(bool async) : Array.Empty())); } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Select_uncorrelated_collection_with_groupby_works(bool async) + { + return AssertQuery( + async, + ss => ss.Set() + .OrderBy(c => c.CustomerID) + .Where(c => c.CustomerID.StartsWith("A")) + .Select(c => ss.Set().GroupBy(o => o.OrderID).Select(g => g.Key).ToArray()), + assertOrder: true, + elementAsserter: (e, a) => AssertCollection(e, a)); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Select_uncorrelated_collection_with_groupby_multiple_collections_work(bool async) + { + return AssertQuery( + async, + ss => ss.Set() + .Where(c => c.CustomerID.StartsWith("A")) + .Select(c => c.Customer.City) + .Select(c => new + { + c1 = ss.Set().GroupBy(p => p.ProductID).Select(g => g.Key).ToArray(), + c2 = ss.Set().GroupBy(p => p.ProductID).Select(g => g.Count()).ToArray() + }), + assertOrder: true, + elementAsserter: (e, a) => + { + AssertCollection(e.c1, a.c1); + AssertCollection(e.c2, a.c2); + }); + } + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Select_GroupBy_All(bool async) @@ -2906,31 +2942,6 @@ public virtual Task Complex_query_with_groupBy_in_subquery3(bool async) }); } - // also 15279 - [ConditionalTheory(Skip = "issue #15873")] - [MemberData(nameof(IsAsyncData))] - public virtual Task Complex_query_with_groupBy_in_subquery4(bool async) - { - return AssertQuery( - async, - ss => ss.Set() - .Select( - c => new - { - Key = c.CustomerID, - Subquery = c.Orders - .Select(o => new { First = o.OrderID, Second = o.Customer.City + o.CustomerID }) - .GroupBy(x => x.Second) - .Select(g => new { Sum = g.Sum(x => x.First), Count = g.Count(x => x.Second.StartsWith("Lon")) }).ToList() - }), - elementSorter: e => e.Key, - elementAsserter: (e, a) => - { - Assert.Equal(e.Key, a.Key); - AssertCollection(e.Subquery, a.Subquery); - }); - } - [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task GroupBy_scalar_subquery(bool async) diff --git a/test/EFCore.Specification.Tests/Query/NorthwindSelectQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindSelectQueryTestBase.cs index c7ca084f79a..3d254abc722 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindSelectQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindSelectQueryTestBase.cs @@ -1794,7 +1794,7 @@ public virtual Task Projecting_multiple_collection_with_same_constant_works(bool [ConditionalTheory] [MemberData(nameof(IsAsyncData))] - public virtual Task Projecting_after_navigation_and_distinct_works_correctly(bool async) + public virtual Task Projecting_after_navigation_and_distinct_throws(bool async) { var filteredOrderIds = new[] { 10248, 10249, 10250 }; diff --git a/test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs index af9b927597e..fa886cfca0e 100644 --- a/test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs @@ -833,6 +833,25 @@ public virtual Task Ordering_by_identifying_projection(bool async) assertOrder: true); } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool async) + { + return AssertQuery( + async, + ss => ss.Set().OrderBy(f => f.Id).Select(f => f.Barton.Throned.Value).Select(t => new + { + t, + Planets = ss.Set().Where(p => p.Id != t).ToList() + }), + assertOrder: true, + elementAsserter: (e, a) => + { + AssertEqual(e.t, a.t); + AssertCollection(e.Planets, a.Planets); + }); + } + protected virtual DbContext CreateContext() => Fixture.CreateContext(); public abstract class OwnedQueryFixtureBase : SharedStoreFixtureBase, IQueryFixtureBase diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsWeakQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsWeakQuerySqlServerTest.cs index 74d581b896f..2d00ce0a609 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsWeakQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsWeakQuerySqlServerTest.cs @@ -1,7 +1,10 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Xunit; using Xunit.Abstractions; namespace Microsoft.EntityFrameworkCore.Query @@ -161,6 +164,14 @@ WHERE [l2].[OneToMany_Required_Inverse3Id] IS NOT NULL AND [l2].[Level2_Required ORDER BY [l].[Id], [t].[Id], [t].[Id0], [t1].[Id], [t1].[Id0], [t1].[Id00]"); } + public override async Task SelectMany_with_navigation_and_Distinct(bool async) + { + var message = (await Assert.ThrowsAsync( + () => base.SelectMany_with_navigation_and_Distinct(async))).Message; + + Assert.Equal(RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin, message); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs index bcc3c59854b..4215451594f 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs @@ -4,6 +4,7 @@ using System; using System.Linq; using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.TestUtilities; using Xunit; using Xunit.Abstractions; @@ -1508,6 +1509,44 @@ FROM [Orders] AS [o1] ORDER BY [o1].[OrderID]"); } + + public override async Task Select_uncorrelated_collection_with_groupby_works(bool async) + { + await base.Select_uncorrelated_collection_with_groupby_works(async); + + AssertSql( + @"SELECT [c].[CustomerID], [t].[OrderID] +FROM [Customers] AS [c] +OUTER APPLY ( + SELECT [o].[OrderID] + FROM [Orders] AS [o] + GROUP BY [o].[OrderID] +) AS [t] +WHERE [c].[CustomerID] LIKE N'A%' +ORDER BY [c].[CustomerID], [t].[OrderID]"); + } + + public override async Task Select_uncorrelated_collection_with_groupby_multiple_collections_work(bool async) + { + await base.Select_uncorrelated_collection_with_groupby_multiple_collections_work(async); + + AssertSql( + @"SELECT [o].[OrderID], [t].[ProductID], [t0].[c], [t0].[ProductID] +FROM [Orders] AS [o] +OUTER APPLY ( + SELECT [p].[ProductID] + FROM [Products] AS [p] + GROUP BY [p].[ProductID] +) AS [t] +OUTER APPLY ( + SELECT COUNT(*) AS [c], [p0].[ProductID] + FROM [Products] AS [p0] + GROUP BY [p0].[ProductID] +) AS [t0] +WHERE [o].[CustomerID] IS NOT NULL AND ([o].[CustomerID] LIKE N'A%') +ORDER BY [o].[OrderID], [t].[ProductID], [t0].[ProductID]"); + } + public override async Task Select_GroupBy_All(bool async) { await base.Select_GroupBy_All(async); @@ -2397,14 +2436,6 @@ FROM [Orders] AS [o] GROUP BY [o].[OrderID]"); } - public override async Task Complex_query_with_groupBy_in_subquery4(bool async) - { - await base.Complex_query_with_groupBy_in_subquery4(async); - - AssertSql( - @""); - } - public override async Task Group_by_with_arithmetic_operation_inside_aggregate(bool async) { await base.Group_by_with_arithmetic_operation_inside_aggregate(async); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs index f65843d21b7..851fcdca02c 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs @@ -5260,6 +5260,33 @@ FROM [Orders] AS [o] FROM [Customers] AS [c]"); } + public override async Task SelectMany_correlated_subquery_hard(bool async) + { + await base.SelectMany_correlated_subquery_hard(async); + + AssertSql( + @"@__p_0='91' + +SELECT [t0].[City] AS [c1], [t1].[City], [t1].[City0] AS [c1] +FROM ( + SELECT DISTINCT [t].[City] + FROM ( + SELECT TOP(@__p_0) [c].[City], [c].[CustomerID] + FROM [Customers] AS [c] + ) AS [t] +) AS [t0] +CROSS APPLY ( + SELECT TOP(9) [e].[City], [t0].[City] AS [City0], [e].[EmployeeID] + FROM [Employees] AS [e] + WHERE ([t0].[City] = [e].[City]) OR ([t0].[City] IS NULL AND [e].[City] IS NULL) +) AS [t1] +CROSS APPLY ( + SELECT TOP(9) [t0].[City], [e0].[EmployeeID] + FROM [Employees] AS [e0] + WHERE ([t1].[City] = [e0].[City]) OR ([t1].[City] IS NULL AND [e0].[City] IS NULL) +) AS [t2]"); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs index 72e187ddbab..907d655151e 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs @@ -1,11 +1,11 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System.Linq; +using System; using System.Threading.Tasks; using Microsoft.EntityFrameworkCore.Diagnostics; -using Microsoft.EntityFrameworkCore.TestModels.Northwind; using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; using Xunit.Abstractions; namespace Microsoft.EntityFrameworkCore.Query @@ -1457,23 +1457,12 @@ FROM [Customers] AS [c] ORDER BY [c].[CustomerID], [o].[OrderID], [o0].[OrderID]"); } - public override async Task Projecting_after_navigation_and_distinct_works_correctly(bool async) + public override async Task Projecting_after_navigation_and_distinct_throws(bool async) { - await base.Projecting_after_navigation_and_distinct_works_correctly(async); + var message = (await Assert.ThrowsAsync( + () => base.Projecting_after_navigation_and_distinct_throws(async))).Message; - AssertSql( - @"SELECT [t].[CustomerID], [t0].[CustomerID], [t0].[OrderID], [t0].[OrderDate] -FROM ( - SELECT DISTINCT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] - FROM [Orders] AS [o] - LEFT JOIN [Customers] AS [c] ON [o].[CustomerID] = [c].[CustomerID] -) AS [t] -OUTER APPLY ( - SELECT [t].[CustomerID], [o0].[OrderID], [o0].[OrderDate] - FROM [Orders] AS [o0] - WHERE [o0].[OrderID] IN (10248, 10249, 10250) AND (([t].[CustomerID] = [o0].[CustomerID]) OR ([t].[CustomerID] IS NULL AND [o0].[CustomerID] IS NULL)) -) AS [t0] -ORDER BY [t].[CustomerID], [t0].[OrderID]"); + Assert.Equal(RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin, message); } public override Task Reverse_without_explicit_ordering_throws(bool async) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs index 8ac619c670c..621a1c69677 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs @@ -1067,6 +1067,18 @@ WHERE [o20].[LeafAAddress_LeafType] IS NOT NULL ORDER BY [o].[Id], [t].[Id], [t].[Id0], [t3].[Id], [t3].[Id0], [t8].[Id], [t8].[Id0], [t8].[Id00], [t12].[Id], [t12].[Id0], [t17].[Id], [t17].[Id0], [t17].[Id00], [t19].[Id], [t19].[Id0], [o22].[ClientId], [o22].[Id]"); } + public override async Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool async) + { + await base.Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(async); + + AssertSql( + @"SELECT [b].[Throned_Value], [f].[Id], [b].[Id], [p].[Id], [p].[StarId] +FROM [Fink] AS [f] +LEFT JOIN [Barton] AS [b] ON [f].[BartonId] = [b].[Id] +LEFT JOIN [Planet] AS [p] ON ([b].[Throned_Value] <> [p].[Id]) OR [b].[Throned_Value] IS NULL +ORDER BY [f].[Id], [b].[Id], [p].[Id]"); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/UdfDbFunctionSqlServerTests.cs b/test/EFCore.SqlServer.FunctionalTests/Query/UdfDbFunctionSqlServerTests.cs index 22118fd8d2c..a5864ba8578 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/UdfDbFunctionSqlServerTests.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/UdfDbFunctionSqlServerTests.cs @@ -610,19 +610,19 @@ public override void QF_Select_Correlated_Subquery_In_Anonymous_MultipleCollecti base.QF_Select_Correlated_Subquery_In_Anonymous_MultipleCollections(); AssertSql( - @"SELECT [c].[Id], [t].[ProductId], [t0].[Id], [t0].[City], [t0].[CustomerId], [t0].[State], [t0].[Street] + @"SELECT [c].[Id], [t].[Id], [t].[City], [t].[CustomerId], [t].[State], [t].[Street], [t0].[ProductId] FROM [Customers] AS [c] -OUTER APPLY ( - SELECT [g].[ProductId] - FROM [dbo].[GetTopTwoSellingProducts]() AS [g] - WHERE [g].[AmountSold] = 249 -) AS [t] LEFT JOIN ( SELECT [a].[Id], [a].[City], [a].[CustomerId], [a].[State], [a].[Street] FROM [Addresses] AS [a] WHERE [a].[State] = N'NY' -) AS [t0] ON [c].[Id] = [t0].[CustomerId] -ORDER BY [c].[Id], [t0].[Id]"); +) AS [t] ON [c].[Id] = [t].[CustomerId] +OUTER APPLY ( + SELECT [g].[ProductId] + FROM [dbo].[GetTopTwoSellingProducts]() AS [g] + WHERE [g].[AmountSold] = 249 +) AS [t0] +ORDER BY [c].[Id], [t].[Id]"); } public override void QF_Select_NonCorrelated_Subquery_In_Anonymous() diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsWeakQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsWeakQuerySqliteTest.cs index 489aeeb5ea0..634754df3fe 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsWeakQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/ComplexNavigationsWeakQuerySqliteTest.cs @@ -1,7 +1,9 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore.Diagnostics; using Xunit; using Xunit.Abstractions; @@ -14,6 +16,14 @@ public ComplexNavigationsWeakQuerySqliteTest(ComplexNavigationsWeakQuerySqliteFi { } + public override async Task SelectMany_with_navigation_and_Distinct(bool async) + { + var message = (await Assert.ThrowsAsync( + () => base.SelectMany_with_navigation_and_Distinct(async))).Message; + + Assert.Equal(RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin, message); + } + // Sqlite does not support cross/outer apply public override Task Filtered_include_after_different_filtered_include_different_level(bool async) => null; public override Task Filtered_include_and_non_filtered_include_followed_by_then_include_on_same_navigation(bool async) => null; diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/NorthwindGroupByQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/NorthwindGroupByQuerySqliteTest.cs index cdbba5b44a8..813124138a5 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/NorthwindGroupByQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/NorthwindGroupByQuerySqliteTest.cs @@ -3,6 +3,7 @@ using System; using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.TestUtilities; using Xunit; using Xunit.Abstractions; @@ -18,5 +19,9 @@ public NorthwindGroupByQuerySqliteTest(NorthwindQuerySqliteFixture null; + public override Task Select_uncorrelated_collection_with_groupby_works(bool async) => null; } } diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/NorthwindSelectQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/NorthwindSelectQuerySqliteTest.cs index ee29a7e0af1..485055bce51 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/NorthwindSelectQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/NorthwindSelectQuerySqliteTest.cs @@ -186,11 +186,11 @@ public override async Task SelectMany_whose_selector_references_outer_source(boo (await Assert.ThrowsAsync( () => base.SelectMany_whose_selector_references_outer_source(async))).Message); - public override async Task Projecting_after_navigation_and_distinct_works_correctly(bool async) + public override async Task Projecting_after_navigation_and_distinct_throws(bool async) => Assert.Equal( - SqliteStrings.ApplyNotSupported, + RelationalStrings.InsufficientInformationToIdentifyOuterElementOfCollectionJoin, (await Assert.ThrowsAsync( - () => base.Projecting_after_navigation_and_distinct_works_correctly(async))).Message); + () => base.Projecting_after_navigation_and_distinct_throws(async))).Message); [ConditionalTheory(Skip = "Issue#17324")] public override Task Project_single_element_from_collection_with_OrderBy_over_navigation_Take_and_FirstOrDefault_2(bool async)