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

Calling static method in select and applying ToList produces wrong results. #7983

Closed
dmitryshunkov opened this issue Mar 24, 2017 · 10 comments · Fixed by #17142
Closed

Calling static method in select and applying ToList produces wrong results. #7983

dmitryshunkov opened this issue Mar 24, 2017 · 10 comments · Fixed by #17142
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@dmitryshunkov
Copy link

dmitryshunkov commented Mar 24, 2017

Inconsistent behavior when calling extension method for projection (f.e. for mapping to DTO) with and without ToList. Looks like closure behavior. If call ToList on Select with static extension method call it produces duplicate values.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.EntityFrameworkCore;

namespace TestEF
{
    public class BloggingContext : DbContext
    {
        public DbSet<Blog> Blogs { get; set; }
        public DbSet<Post> Posts { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
            => optionsBuilder.UseInMemoryDatabase();

        //=> optionsBuilder.UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=WonkyVB;Trusted_Connection=True;");
    }

    public class Blog
    {
        public int Id { get; set; }
        public string Title { get; set; }

        public ICollection<Post> Posts { get; set; }
    }

    public class Post
    {
        public int Id { get; set; }
        public string Title { get; set; }

        public int? BlogId { get; set; }
        public Blog Blog { get; set; }
    }

    public class PostDTO
    {
        public string Title { get; set; }
    }

    public static class Mappings
    {
        public static PostDTO From(this PostDTO dto, Post post)
        {
            dto.Title = post.Title;
            return dto;
        }
    }

    public class Program
    {
        public static void Main()
        {
            using (var context = new BloggingContext())
            {
                context.Database.EnsureDeleted();
                context.Database.EnsureCreated();

                context.Add(new Blog
                {
                    Posts = new List<Post> { new Post { Title = "First" }, new Post { Title = "Second" }, new Post { Title = "Third" } }
                });
                context.SaveChanges();
            }

            using (var context = new BloggingContext())
            {
                var list = context.Posts.Select(p => new PostDTO().From(p)).ToList(); // Produces: Third Third Third
                //var list = context.Posts.Select(p => new PostDTO().From(p)); // Produces: First Second Third
                foreach (var item in list) Console.WriteLine(item.Title);
            }
        }
    }
}

Further technical details

EF Core version: 1.1.1
Database Provider: (Microsoft.EntityFrameworkCore.InMemory, Microsoft.EntityFrameworkCore.SqlServer)
Operating system: Windows 10
IDE: (e.g. Visual Studio 2017)

@smitpatel
Copy link
Member

smitpatel commented Apr 11, 2017

I investigated into this and this is bug in ReLinq's QueryParser.

When we get the expression tree (after ParameterExtraction) the LambdaExpression in Select has NewExpression for PostDTO which converts to ConstantExpression of PostDTO in QueryModel.SelectClause after ReLinq parses the query.

Generated QueryModel.ToString

{from Post p in value(Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryable`1[Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests.QueryBugsTest+Post]) select value(Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests.QueryBugsTest+PostDTO).From([p])}

Point to mention that this does not happen if the new has any property assignment or anonymous type.

@MichaelKetting

@smitpatel
Copy link
Member

@dmitryshunkov - As a workaround you can move defining new PostDTO from select clause. Effectively write query like this

var list = db.Posts.Select(p => From(p)).ToList();

public static PostDTO From(Post post)
{
    var dto = new PostDTO { Title = post.Title };
    return dto;
}

@maumar
Copy link
Contributor

maumar commented Apr 13, 2017

@smitpatel to file a relinq bug on this

@MichaelKetting
Copy link
Contributor

@smitpatel Yes, an issue would be much approciated for bookkeeping.

PS: Sorry, I thought that I already responded here but looks like it got lost in the inbox queue. I try to do a repro for this in the re-linq tests over the weekend but the next couple of weeks will be crazy for me so I can't make promises and I'll apologize in advance for any delays :)

@smitpatel
Copy link
Member

Filed issue on Re-Linq https://www.re-motion.org/jira/browse/RMLNQ-112

@MichaelKetting
Copy link
Contributor

@smitpatel Just for bookkeeping: Thanks for the repro, it was a great tool for reminding me this can (and is meant to) be solved in EntityFramework using exisitng re-linq parsing infrastructure (IEvaluatableExpressionFilter). Please check the RMLNQ-112 issue for details.

@ajcvickers
Copy link
Member

Closing this for now since the pattern is not super common and there is no easy solution that would not negatively impact other scenarios. We would consider re-opening this based on feedback from others hitting it.

@smitpatel
Copy link
Member

smitpatel commented Nov 27, 2018

Re-opening this. Since we are removing ReLinq, we should look into supporting this kind of scenario. While this particular pattern may not be common, it is just a small subset of large scenario, in which user can call new class() which does not depend on range variable.

@smitpatel smitpatel self-assigned this Nov 27, 2018
@ajcvickers ajcvickers added this to the 3.0.0 milestone Nov 28, 2018
@smitpatel
Copy link
Member

This may or may not work in 3.0 easily since even if Relinq is removed, our funcletizer may try to compute the value out for the initialization.

@divega
Copy link
Contributor

divega commented Jun 20, 2019

@smitpatel to provide workaround.

@smitpatel smitpatel removed their assignment Jun 24, 2019
@smitpatel smitpatel modified the milestones: Backlog, 3.0.0 Aug 13, 2019
@smitpatel smitpatel self-assigned this Aug 13, 2019
@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 Aug 13, 2019
smitpatel added a commit that referenced this issue Aug 14, 2019
…ression tree

This puts some of the processing (evaluating the expression to the corresponding constant) inside translation pipeline.
- When applying EntityEquality, assumption here is that property is always going to be mapped on server side so we can generate constant.
- When translating newExpression. If the generated constant can be mapped, it would work. (like new Datetime()) else translation would null out.

Resolves #15712
Resolves #17048
Resolves #7983
smitpatel added a commit that referenced this issue Aug 14, 2019
…ression tree

This puts some of the processing (evaluating the expression to the corresponding constant) inside translation pipeline.
- When applying EntityEquality, assumption here is that property is always going to be mapped on server side so we can generate constant.
- When translating newExpression. If the generated constant can be mapped, it would work. (like new Datetime()) else translation would null out.

Resolves #15712
Resolves #17048
Resolves #7983
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview9 Aug 21, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview9, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants