Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug when Take clause is used in a query and also applied to the child collections #19763

Closed
gathogojr opened this issue Jan 31, 2020 · 13 comments
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-5.0 type-bug
Milestone

Comments

@gathogojr
Copy link

An issue was reported under the OData/WebApi project where the user complained of a breaking change after upgrading from ASP.NET Core 2.2/EF Core 2.2.6 to ASP.NET Core 3.1/EF Core 3.1.1
Link to the issue OData/WebApi#2026

Basically, the user had this Url comprising a resource path and some query options and calls to the OData service started failing (throwing an exception) after the upgrade:
/odata/Employers?$expand=Employees($expand=Department;$top=3)&$top=1

The issue was triaged by the OData Team and after further investigation it was discovered it was caused by a possible regression bug in EF Core. The user had also suspected the same. I have gone further and reproduced it in a simple Console project stripped off OData bells and whistles. This was necessary to rule out the possibility that ODL was generating an invalid LINQ expression

Steps to reproduce

  1. Create a .NET Core (Console) project targeting .NET Core 3.1 framework and the following packages:

    • Microsoft.EntityFrameworkCore 3.1.1
    • Microsoft.EntityFrameworkCore.Tools 3.1.1
    • Microsoft.EntityFrameworkCore.SqlServer 3.1.1
    • Microsoft.Extensions.Logging.Console 3.1.1
  2. Add the following entity classes:

    Department class:

    public class Department
    {
        [Key]
        public int Id { get; set; }
    
        public string Name { get; set; }
    }
    

    Employee class:

    public class Employee
    {
        [Key]
        public int Id { get; set; }
        public string Name { get; set; }
        public int EmployerId { get; set; }
        public virtual Employer Employer { get; set; }
        public int DepartmentId { get; set; }
        public virtual Department Department { get; set; }
    }
    

    Employer class:

    {
        [Key]
        public int Id { get; set; }
        public string Name { get; set; }
        public virtual IEnumerable<Employee> Employees { get; set; }
    }
    
  3. Use fluent API to configure an entity model

    public class AppDbContext : DbContext
    {
        private static readonly ILoggerFactory _loggerFactory = LoggerFactory.Create(builder => {
        builder.AddConsole();
        });
    
        public DbSet<Department> Departments { get; set; }
        public DbSet<Employer> Employers { get; set; }
        public DbSet<Employee> Employees { get; set; }
    
        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            base.OnConfiguring(optionsBuilder);
    
            if (!optionsBuilder.IsConfigured)
            {
                optionsBuilder
                    .UseLoggerFactory(_loggerFactory)
                    .UseSqlServer(@"Data Source = (LocalDb)\MSSQLLocalDB; Integrated Security = True; Persist Security Info = True; Database = Repro04EmpDb");
            }
        }
    }
    
  4. Generate and run migration files to trigger creation of the database and database objects. Optionally, populate the database tables with sample data

  5. Compose and execute the following LINQ query against the database context

    var query = db.Employers.Take(1).Select(d => new Employer
    {
        Id = d.Id,
        Name = d.Name,
        Employees = d.Employees.Take(3).Select(e => new Employee
        {
            Id = e.Id,
            Name = e.Name,
            EmployerId = e.EmployerId,
            DepartmentId = e.DepartmentId,
            Department = e.Department
        })
    });
    
    var result = query.ToList(); // Trigger execution
    

Expected Result:
A list comprising at most 1 item of type Employer with a child collection of type Employee with at most 3 items

Actual Result:
Exception message:

{"Invalid column name 'DepartmentId'.\r\nInvalid column name 'EmployerId'.\r\nInvalid column name 'DepartmentId'."}

Exception stacktrace:

" at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action1 wrapCloseInAction)\r\n at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action1 wrapCloseInAction)\r\n at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)\r\n at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)\r\n at Microsoft.Data.SqlClient.SqlDataReader.TryConsumeMetaData()\r\n at Microsoft.Data.SqlClient.SqlDataReader.get_MetaData()\r\n at Microsoft.Data.SqlClient.SqlCommand.FinishExecuteReader(SqlDataReader ds, RunBehavior runBehavior, String resetOptionsString, Boolean isInternal, Boolean forDescribeParameterEncryption, Boolean shouldCacheForAlwaysEncrypted)\r\n at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, Boolean isAsync, Int32 timeout, Task& task, Boolean asyncWrite, Boolean inRetry, SqlDataReader ds, Boolean describeParameterEncryptionRequest)\r\n at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, TaskCompletionSource1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String method)\r\n at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method)\r\n at Microsoft.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior)\r\n at Microsoft.Data.SqlClient.SqlCommand.ExecuteDbDataReader(CommandBehavior behavior)\r\n at System.Data.Common.DbCommand.ExecuteReader()\r\n at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReader(RelationalCommandParameterObject parameterObject)\r\n at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable1.Enumerator.InitializeReader(DbContext _, Boolean result)\r\n at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func3 operation, Func3 verifySucceeded)\r\n at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable1.Enumerator.MoveNext()\r\n at System.Collections.Generic.List1..ctor(IEnumerable1 collection)\r\n at System.Linq.Enumerable.ToList[TSource](IEnumerable1 source)\r\n at ..."

Logging the generated SQL query confirms the user's observation that EmployerId and DepartmentId are referencing the wrong aliases.

  • [t].[EmployerId] should be [t0].[EmployerId]
  • [t].[DepartmentId] should be [t0].[DepartmentId] (two locations)

Query

SELECT [t].[Id], [t].[Name], [t1].[c], [t1].[Id], [t1].[DepartmentId], [t1].[EmployerId], [t1].[Name], [t1].[c0], [t1].[c1], [t1].[c2], [t1].[Id0], [t1].[Name0], [t1].[c3]
FROM (
    SELECT TOP(@__TypedProperty_4) [e].[Id], [e].[Name]
    FROM [Employers] AS [e]
    ORDER BY [e].[Id]
) AS [t]
OUTER APPLY (
    SELECT @__TypedProperty_2 AS [c], [t].[Id], [t].[DepartmentId], [t].[EmployerId], [t].[Name], CAST(1 AS bit) AS [c0], N'Department' AS [c1], @__TypedProperty_3 AS [c2], [d].[Id] AS [Id0], [d].[Name] AS [Name0], CAST(0 AS bit) AS [c3]
    FROM (
        SELECT TOP(@__TypedProperty_1) [e0].[Id], [e0].[DepartmentId], [e0].[EmployerId], [e0].[Name]
        FROM [Employees] AS [e0]
        WHERE [t].[Id] = [e0].[EmployerId]
        ORDER BY [e0].[Id]
    ) AS [t0]
    INNER JOIN [Departments] AS [d] ON [t].[DepartmentId] = [d].[Id]
) AS [t1]
ORDER BY [t].[Id], [t1].[Id], [t1].[Id0]

Note that the same LINQ query runs successfully against EF Core 2.2 - 2 SQL statements are generated, one to fetch the list of parent objects and the other to fetch the child collections for each returned parent object

SELECT TOP(@__TypedProperty_4) [$it].[Id], [$it].[Name]
FROM [Employers] AS [$it]
ORDER BY [$it].[Id]
SELECT TOP(@__TypedProperty_1) [$it0].[Id], [$it0].[DepartmentId], [$it0].[EmployerId], [$it0].[Name], [$it.Department].[Id], [$it.Department].[Name]
FROM [Employees] AS [$it0]
INNER JOIN [Departments] AS [$it.Department] ON [$it0].[DepartmentId] = [$it.Department].[Id]
WHERE @_outer_Id = [$it0].[EmployerId]
ORDER BY [$it0].[Id]

Here is a link to a GitHub repo reproducing the issue as described above (EF Core 3.1.1)
https://github.com/gathogojr/ODataWebApiIssue2026Repro04_EFCore311

Here's a link to a GitHub repo with a similar Console project against EF Core 2.2.6
https://github.com/gathogojr/ODataWebApiIssue2026Repro03_EFCore226

Further technical details

EF Core version: 2.2.6
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.1
Operating system: Windows 10 Enterprise
IDE: Visual Studio 2019 16.4.3

@ajcvickers
Copy link
Member

@maumar to take a look.

@maumar
Copy link
Contributor

maumar commented Feb 1, 2020

repros on current master.

query we generate now looks as follows:

SELECT [t].[Id], [t].[Name], [t1].[Id], [t1].[Name], [t1].[EmployerId], [t1].[DepartmentId], [t1].[Id0], [t1].[Name0]
FROM (
    SELECT TOP(@__p_0) [e].[Id], [e].[Name]
    FROM [Employers] AS [e]
) AS [t]
OUTER APPLY (
    SELECT [t].[Id], [t].[Name], [t].[EmployerId], [t].[DepartmentId], [d].[Id] AS [Id0], [d].[Name] AS [Name0]
    FROM (
        SELECT TOP(3) [e0].[Id], [e0].[DepartmentId], [e0].[EmployerId], [e0].[Name]
        FROM [Employees] AS [e0]
        WHERE [t].[Id] = [e0].[EmployerId]
    ) AS [t0]
    INNER JOIN [Departments] AS [d] ON [t].[DepartmentId] = [d].[Id]
) AS [t1]
ORDER BY [t].[Id], [t1].[Id], [t1].[Id0]

@joakimriedel
Copy link
Contributor

Perhaps same root cause as #17809 , but this repro is much lighter.

@gathogojr
Copy link
Author

gathogojr commented Feb 3, 2020

@maumar @ajcvickers If you take a keen look at the query you posted above DepartmentId and EmployerId are not returned in the subquery aliased t - they're returned in the subquery aliased t0. That is the fix that would be needed to fix the body of the subquery aliased t1, every place where you have [t].[EmployerId] and [t].[DepartmentId]

@smitpatel
Copy link
Contributor

Same root case as #17337

@smitpatel
Copy link
Contributor

In Complex navs.

+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual Task Take_Select_collection_Take(bool async)
+        {
+            return AssertQuery(
+                async,
+                ss => ss.Set<Level1>().OrderBy(l1 => l1.Id).Take(1)
+                    .Select(l1 => new
+                    {
+                        Id = l1.Id,
+                        Name = l1.Name,
+                        Level2s = l1.OneToMany_Required1.OrderBy(l2 => l2.Id).Take(3)
+                            .Select(l2 => new
+                            {
+                                Id = l2.Id,
+                                Name = l2.Name,
+                                Level1Id = EF.Property<int>(l2, "OneToMany_Required_Inverse2Id"),
+                                Level2Id = l2.Level1_Required_Id,
+                                Level2 = l2.OneToOne_Required_FK_Inverse2
+                            })
+                    }));
+        }

@gathogojr
Copy link
Author

@maumar @smitpatel @ajcvickers Re-tested with Microsoft.EntityFrameworkCore 3.1.4 and Microsoft.EntityFrameworkCore 5.0.0-preview.4.20220.10 and the issue still exists.

The query below looks straightforward to me so I'm wondering if there's an alternative way to re-write it in a way that it results into a valid SQL statement

var query = db.Employers.Take(1).Select(d => new Employer
{
    Id = d.Id,
    Name = d.Name,
    Employees = d.Employees.Take(3).Select(e => new Employee
    {
        Id = e.Id,
        Name = e.Name,
        EmployerId = e.EmployerId,
        DepartmentId = e.DepartmentId,
        Department = e.Department
    })
});

@ajcvickers
Copy link
Member

@gathogojr This issue is not yet fixed, hence the issue is still "Open". It is in the 5.0.0 milestone, which means we plan to fix it for the EF Core 5.0 release.

@ajcvickers ajcvickers modified the milestones: 5.0.0, Backlog Jun 9, 2020
@smitpatel smitpatel removed their assignment Aug 27, 2020
@BenjaminAbt
Copy link

Hi @ajcvickers I see this issue has been punted. Is there any workaround we can use?

var query = from blog in _dbContext.Blogs
            where blog.Id == 1
            let latestPosts = (from post in blog.Posts
                               orderby post.PostedOn descending
                               select post).Skip(skipPosts).Take(takePosts)
            select latestPosts;

Error also occures when skip is < 1 even there is no Skip / Take on parent level

@ajcvickers
Copy link
Member

@smitpatel @maumar Any workarounds here?

@BenjaminAbt
Copy link

BenjaminAbt commented Sep 22, 2020

Looks like the workaround is just

var query = from blog in _dbContext.Blogs
            where blog.Id == 1
            let latestPosts = (from post in _dbContext.BlogPosts
                               where post.BlogId == blog.Id
                               orderby post.PostedOn descending
                               select post).Skip(skipPosts).Take(takePosts)
            select latestPosts;

So use the DbSet instead of the child collection

@ajcvickers
Copy link
Member

ajcvickers commented Nov 13, 2020

Note from triage: this issue is blocked by #17337. That issue is committed to be fixed in 6.0, after which this issue should also be fixed/verified fixed for 6.0. (Do not punt from 6.0.)

@snebjorn
Copy link

I encountered something that seems related to this when querying with OData/WebApi

GET /odata/blogs?$expand=Models.RssBlog/Feeds($expand=Bar;$top=0)&$top=10

Blogs is using the table-per-hierarchy strategy.

The above will throw Microsoft.Data.SqlClient.SqlException (0x80131904): The multi-part identifier "t.XXX" could not be bound.

I observed the same issue with $take and $orderby

However these are fine.

GET /odata/blogs?$expand=Models.RssBlog/Feeds($expand=Bar)&$top=10
GET /odata/blogs?$expand=Models.RssBlog/Feeds($expand=Bar;$top=10)

I'll try to provide a demo project in the coming days

@smitpatel smitpatel added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed blocked labels Mar 24, 2021
smitpatel added a commit that referenced this issue Mar 24, 2021
- Don't apply Include on entities with Include already applied
- Update table references when pushing down select into left for set operation
- Update identifiers after applying set operation if the projection removed exiting identifiers
- Update SQL references in pending collection during push down

Fix for the repro in #17337
Resolves #18738
Resolves #19763
Resolves #19947
Resolves #20813
Resolves #21026
Resolves #22222
Resolves #23676
Resolves #23720
smitpatel added a commit that referenced this issue Mar 24, 2021
- Don't apply Include on entities with Include already applied
- Update table references when pushing down select into left for set operation
- Update identifiers after applying set operation if the projection removed exiting identifiers
- Update SQL references in pending collection during push down

Fix for the repro in #17337
Resolves #18738
Resolves #19763
Resolves #19947
Resolves #20813
Resolves #21026
Resolves #22222
Resolves #23676
Resolves #23720
Resolves #24216
smitpatel added a commit that referenced this issue Mar 24, 2021
- Don't apply Include on entities with Include already applied
- Update table references when pushing down select into left for set operation
- Update identifiers after applying set operation if the projection removed exiting identifiers
- Update SQL references in pending collection during push down

Fix for the repro in #17337
Resolves #18738
Resolves #19763
Resolves #19947
Resolves #20813
Resolves #21026
Resolves #22222
Resolves #23676
Resolves #23720
Resolves #24216
smitpatel added a commit that referenced this issue Mar 25, 2021
- Don't apply Include on entities with Include already applied
- Update table references when pushing down select into left for set operation
- Update identifiers after applying set operation if the projection removed exiting identifiers
- Update SQL references in pending collection during push down

Fix for the repro in #17337
Resolves #18738
Resolves #19763
Resolves #19947
Resolves #20813
Resolves #21026
Resolves #22222
Resolves #23676
Resolves #23720
Resolves #24216
smitpatel added a commit that referenced this issue Mar 25, 2021
- Don't apply Include on entities with Include already applied
- Update table references when pushing down select into left for set operation
- Update identifiers after applying set operation if the projection removed exiting identifiers
- Update SQL references in pending collection during push down

Fix for the repro in #17337
Resolves #18738
Resolves #19763
Resolves #19947
Resolves #20813
Resolves #21026
Resolves #22222
Resolves #23676
Resolves #23720
Resolves #24216
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview4 Mar 25, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview4, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-5.0 type-bug
Projects
None yet
Development

No branches or pull requests

7 participants