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

Wrong property assignment with Unions in InMemoryProvider #23360

Closed
wischi-chr opened this issue Nov 17, 2020 · 0 comments · Fixed by #24363
Closed

Wrong property assignment with Unions in InMemoryProvider #23360

wischi-chr opened this issue Nov 17, 2020 · 0 comments · Fixed by #24363
Assignees
Labels
area-in-memory closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@wischi-chr
Copy link

Description

There seems to be a bug in the way unions are handled by the in-memory provider. The expression used inside the first select seems to determine how the other source (provided as parameter to Union()) is interpreted/assigned which causes wrong assignments. Data from a specific "column" is assigned to an unrelated (wrong) property. This only seems to happen with the InMemoryProvider (tested it with SQL Server Provider as well and it worked as intended).

Provider and version information

EF Core version: 3.1.3 / 3.1.10 / 5.0.0 (these are the versions I tested and that showed the same behaviour)
Database provider: Microsoft.EntityFrameworkCore.InMemory
Target framework: .NET 3.1
Operating system: Win10
IDE: Visual Studio Enterprise 2019 16.7.7

Code (All-in-one file) that reproduces the issue

// nuget: Microsoft.EntityFrameworkCore.InMemory

using Microsoft.EntityFrameworkCore;
using System;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Threading.Tasks;

namespace EFCoreUnionBug
{
    class Program
    {
        static async Task Main()
        {
            await AddTestData();
            await TriggerBug();
        }

        static async Task TriggerBug()
        {
            using var context = new SampleContext();

            // Because the assigned properties are swapped the union that is executed (in-memory)
            // assigns the wrong value (different column) to the property.
            //
            // There are similar issues if a different subset of properties are assigned
            // inside the select statement. It looks like the first select determines
            // the columns that are queried/assigned for the second query.

            var userQuery = context.User
                .Select(u => new CommonSelectType()
                {
                    // 1. FirstName, 2. LastName
                    FirstName = u.Forename,
                    LastName = u.Surname,
                });

            var customerQuery = context.Customer
                .Select(c => new CommonSelectType()
                {
                    // 1. LastName, 2. FirstName
                    LastName = c.FamilyName,
                    FirstName = c.GivenName,
                });

            var result = await userQuery.Union(customerQuery).ToListAsync();

            Console.WriteLine($"result[0].FirstName = {result[0].FirstName}");
            Console.WriteLine($"result[0].LastName  = {result[0].LastName}");
            Console.WriteLine($"result[1].FirstName = {result[1].FirstName}");
            Console.WriteLine($"result[1].LastName  = {result[1].LastName}");

            // Output:
            // =======
            // result[0].FirstName = Peter
            // result[0].LastName  = Smith
            // result[1].FirstName = Doe     <--- wrong
            // result[1].LastName  = John    <--- 
        }

        static async Task AddTestData()
        {
            using var context = new SampleContext();

            context.User.Add(new User()
            {
                Forename = "Peter",
                Surname = "Smith",
            });

            context.Customer.Add(new Customer()
            {
                GivenName = "John",
                FamilyName = "Doe",
            });

            await context.SaveChangesAsync();
        }
    }

    public class CommonSelectType
    {
        public string FirstName { get; set; }
        public string LastName { get; set; }
    }

    public class SampleContext : DbContext
    {
        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseInMemoryDatabase(nameof(SampleContext));
        }

        public virtual DbSet<User> User { get; set; }
        public virtual DbSet<Customer> Customer { get; set; }
    }

    public class User
    {
        [Key]
        public int Key { get; set; }

        public string Forename { get; set; }
        public string Surname { get; set; }
    }

    public class Customer
    {
        [Key]
        public int Key { get; set; }

        public string GivenName { get; set; }
        public string FamilyName { get; set; }
    }
}
@ajcvickers ajcvickers added this to the 6.0.0 milestone Nov 17, 2020
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 9, 2021
@smitpatel smitpatel assigned smitpatel and unassigned maumar Mar 9, 2021
smitpatel added a commit that referenced this issue Mar 10, 2021
Implement left join as client method to reduce complexity
To resolve the indexing issue stemming from #23934
Now all the projections are applied immediately to reshape the value buffer so that all our future bindings are always read value expressions which refers to a proper index.
In order to do this, we apply select for entity type with hierarchy to avoid entity check conditional expression.
For adding projection (through ReplaceProjectionMapping),
- For client projection, we apply a select and re-generate client projection as read values
- For projection mapping, we iterate the mappings, we apply a select and re-generate the mapping as read values
- If projection mapping is empty then we add a dummy 1 so that it becomes non-range-variable
When applying projection, we generate a selector lambda to form a value buffer and replaced all the expressions to read from new value buffer. Overall this solves the issue of having complex expressions to map or pull. This also removed PushDownIntoSubquery method.

In order to avoid the issue of indexes changing when generating join due to iterating projection mappings, we now also have projectionMappingExpressions which remembers the all expressions inside projectionMapping (which are all read value as we generated before). So now we don't need to iterate the mapping and we use the existing expressions directly. This keeps existing indexes.

Resolves #13561
Resolves #17539
Resolves #18194
Resolves #18435
Resolves #19344
Resolves #19469
Resolves #19667
Resolves #19742
Resolves #19967
Resolves #20359
Resolves #21677
Resolves #23360
Resolves #17537
Resolves #18394
Resolves #23934
smitpatel added a commit that referenced this issue Mar 10, 2021
Implement left join as client method to reduce complexity
To resolve the indexing issue stemming from #23934
Now all the projections are applied immediately to reshape the value buffer so that all our future bindings are always read value expressions which refers to a proper index.
In order to do this, we apply select for entity type with hierarchy to avoid entity check conditional expression.
For adding projection (through ReplaceProjectionMapping),
- For client projection, we apply a select and re-generate client projection as read values
- For projection mapping, we iterate the mappings, we apply a select and re-generate the mapping as read values
- If projection mapping is empty then we add a dummy 1 so that it becomes non-range-variable
When applying projection, we generate a selector lambda to form a value buffer and replace all the expressions to read from new value buffer. Overall this solves the issue of having complex expressions to map or pull. This also removed PushDownIntoSubquery method.

In order to avoid the issue of indexes changing when generating join due to iterating projection mappings, we now also have projectionMappingExpressions which remembers the all expressions inside projectionMapping (which are all read value as we generated before). So now we don't need to iterate the mapping and we use the existing expressions directly. This keeps existing indexes.

Resolves #13561
Resolves #17539
Resolves #18194
Resolves #18435
Resolves #19344
Resolves #19469
Resolves #19667
Resolves #19742
Resolves #19967
Resolves #20359
Resolves #21677
Resolves #23360
Resolves #17537
Resolves #18394
Resolves #23934
smitpatel added a commit that referenced this issue Mar 10, 2021
Implement left join as client method to reduce complexity
To resolve the indexing issue stemming from #23934
Now all the projections are applied immediately to reshape the value buffer so that all our future bindings are always read value expressions which refers to a proper index.
In order to do this, we apply select for entity type with hierarchy to avoid entity check conditional expression.
For adding projection (through ReplaceProjectionMapping),
- For client projection, we apply a select and re-generate client projection as read values
- For projection mapping, we iterate the mappings, we apply a select and re-generate the mapping as read values
- If projection mapping is empty then we add a dummy 1 so that it becomes non-range-variable
When applying projection, we generate a selector lambda to form a value buffer and replace all the expressions to read from new value buffer. Overall this solves the issue of having complex expressions to map or pull. This also removed PushDownIntoSubquery method.

In order to avoid the issue of indexes changing when generating join due to iterating projection mappings, we now also have projectionMappingExpressions which remembers the all expressions inside projectionMapping (which are all read value as we generated before). So now we don't need to iterate the mapping and we use the existing expressions directly. This keeps existing indexes.

Resolves #13561
Resolves #17539
Resolves #18194
Resolves #18435
Resolves #19344
Resolves #19469
Resolves #19667
Resolves #19742
Resolves #19967
Resolves #20359
Resolves #21677
Resolves #23360
Resolves #17537
Resolves #18394
Resolves #23934
Resolves #17620
Resolves #18912
smitpatel added a commit that referenced this issue Mar 10, 2021
Implement left join as client method to reduce complexity
To resolve the indexing issue stemming from #23934
Now all the projections are applied immediately to reshape the value buffer so that all our future bindings are always read value expressions which refers to a proper index.
In order to do this, we apply select for entity type with hierarchy to avoid entity check conditional expression.
For adding projection (through ReplaceProjectionMapping),
- For client projection, we apply a select and re-generate client projection as read values
- For projection mapping, we iterate the mappings, we apply a select and re-generate the mapping as read values
- If projection mapping is empty then we add a dummy 1 so that it becomes non-range-variable
When applying projection, we generate a selector lambda to form a value buffer and replace all the expressions to read from new value buffer. Overall this solves the issue of having complex expressions to map or pull. This also removed PushDownIntoSubquery method.

In order to avoid the issue of indexes changing when generating join due to iterating projection mappings, we now also have projectionMappingExpressions which remembers the all expressions inside projectionMapping (which are all read value as we generated before). So now we don't need to iterate the mapping and we use the existing expressions directly. This keeps existing indexes.

Resolves #13561
Resolves #17539
Resolves #18194
Resolves #18435
Resolves #19344
Resolves #19469
Resolves #19667
Resolves #19742
Resolves #19967
Resolves #20359
Resolves #21677
Resolves #23360
Resolves #17537
Resolves #18394
Resolves #23934
smitpatel added a commit that referenced this issue Mar 10, 2021
Implement left join as client method to reduce complexity
To resolve the indexing issue stemming from #23934
Now all the projections are applied immediately to reshape the value buffer so that all our future bindings are always read value expressions which refers to a proper index.
In order to do this, we apply select for entity type with hierarchy to avoid entity check conditional expression.
For adding projection (through ReplaceProjectionMapping),
- For client projection, we apply a select and re-generate client projection as read values
- For projection mapping, we iterate the mappings, we apply a select and re-generate the mapping as read values
- If projection mapping is empty then we add a dummy 1 so that it becomes non-range-variable
When applying projection, we generate a selector lambda to form a value buffer and replace all the expressions to read from new value buffer. Overall this solves the issue of having complex expressions to map or pull. This also removed PushDownIntoSubquery method.

In order to avoid the issue of indexes changing when generating join due to iterating projection mappings, we now also have projectionMappingExpressions which remembers the all expressions inside projectionMapping (which are all read value as we generated before). So now we don't need to iterate the mapping and we use the existing expressions directly. This keeps existing indexes.

Resolves #13561
Resolves #17539
Resolves #18194
Resolves #18435
Resolves #19344
Resolves #19469
Resolves #19667
Resolves #19742
Resolves #19967
Resolves #20359
Resolves #21677
Resolves #23360
Resolves #17537
Resolves #18394
Resolves #23934

# Conflicts:
#	test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs
@ghost ghost closed this as completed in #24363 Mar 10, 2021
ghost pushed a commit that referenced this issue Mar 10, 2021
Implement left join as client method to reduce complexity
To resolve the indexing issue stemming from #23934
Now all the projections are applied immediately to reshape the value buffer so that all our future bindings are always read value expressions which refers to a proper index.
In order to do this, we apply select for entity type with hierarchy to avoid entity check conditional expression.
For adding projection (through ReplaceProjectionMapping),
- For client projection, we apply a select and re-generate client projection as read values
- For projection mapping, we iterate the mappings, we apply a select and re-generate the mapping as read values
- If projection mapping is empty then we add a dummy 1 so that it becomes non-range-variable
When applying projection, we generate a selector lambda to form a value buffer and replace all the expressions to read from new value buffer. Overall this solves the issue of having complex expressions to map or pull. This also removed PushDownIntoSubquery method.

In order to avoid the issue of indexes changing when generating join due to iterating projection mappings, we now also have projectionMappingExpressions which remembers the all expressions inside projectionMapping (which are all read value as we generated before). So now we don't need to iterate the mapping and we use the existing expressions directly. This keeps existing indexes.

Resolves #13561
Resolves #17539
Resolves #18194
Resolves #18435
Resolves #19344
Resolves #19469
Resolves #19667
Resolves #19742
Resolves #19967
Resolves #20359
Resolves #21677
Resolves #23360
Resolves #17537
Resolves #18394
Resolves #23934

# Conflicts:
#	test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview3 Mar 25, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview3, 6.0.0 Nov 8, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-in-memory closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants