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

Expression of type 'System.Object' cannot be used for parameter of type 'Microsoft.EntityFrameworkCore.Metadata.IEntityType' #9551

Closed
dannythomas13 opened this issue Aug 24, 2017 · 20 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. regression type-bug
Milestone

Comments

@dannythomas13
Copy link

dannythomas13 commented Aug 24, 2017

Since updating to v2 I now get the following exception from one of my unit tests:

System.ArgumentException : Expression of type 'System.Object' cannot be used for parameter of type 'Microsoft.EntityFrameworkCore.Metadata.IEntityType' of method 'Void StartTracking(System.Object, Microsoft.EntityFrameworkCore.Metadata.IEntityType)'
Parameter name: arg1

In order to get the test to pass I have had to change:

var projects = this.context.Projects
                        .Include(p => p.Windfarm)
                        .Include(p => p.Turbines)
                        .Where(p => p.Windfarm.ClientId == clientId && p.IsActive)
                        .GroupBy(g => g.WindfarmId)
.Select(s => s.OrderByDescending(o => o.DateStarted).FirstOrDefault())
                        .ToList();

to the following:

var activeProjects = this.context.Projects
                        .Include(p => p.Windfarm)
                        .Include(p => p.Turbines)
                        .Where(p => p.Windfarm.ClientId == clientId && p.IsActive)
                        .GroupBy(g => g.WindfarmId).ToList();

            var projects = activeProjects.Select(s => s.OrderByDescending(o => o.DateStarted).FirstOrDefault())
                        .ToList();
@smitpatel
Copy link
Contributor

smitpatel commented Aug 24, 2017

Repro

    public class Program
    {
        public static void Main(string[] args)
        {
            using (var db = new MyContext())
            {
                // Recreate database
                db.Database.EnsureDeleted();
                db.Database.EnsureCreated();

                // Seed database
                db.AddRange(
                    new Blog
                    {
                        Posts = new List<Post>
                        {
                            new Post(),
                            new Post(),
                            new Post()
                        }
                    },
                    new Blog
                    {
                        Posts = new List<Post>
                        {
                            new Post(),
                            new Post(),
                            new Post()
                        }
                    }
                    );


                db.SaveChanges();
            }

            using (var db = new MyContext())
            {
                // Run queries
                var query = db.Set<Post>()
                    .Include(p => p.Blog)
                    .GroupBy(p => EF.Property<int?>(p, "BlogId"))
                    .Select(g => g.OrderBy(e => e.Id).FirstOrDefault()).ToList();
            }

            Console.WriteLine("Program finished.");
        }
    }


    public class MyContext : DbContext
    {
        // Declare DBSets
        public DbSet<Blog> Blogs { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            // Select 1 provider
            optionsBuilder
                .UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=_ModelApp;Trusted_Connection=True;Connect Timeout=5;ConnectRetryCount=0")
                //.UseSqlite("filename=_modelApp.db")
                //.UseInMemoryDatabase(databaseName: "_modelApp")
                .EnableSensitiveDataLogging()
                .UseLoggerFactory(new LoggerFactory().AddConsole());
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            // Configure model
        }
    }

    public class Blog
    {
        public int Id { get; set; }
        public List<Post> Posts { get; set; }
    }

    public class Post
    {
        public int Id { get; set; }
        public Blog Blog { get; set; }
    }

@smitpatel
Copy link
Contributor

Include with GroupBy and Select with single result operator.
This is regression from 1.1.2
@anpete

@smitpatel
Copy link
Contributor

@maumar this bug!

@ajcvickers ajcvickers added this to the 2.0.1 milestone Aug 25, 2017
@Cowlephant
Copy link

Just when I thought I might have gotten through with migrating to 2.0.. .more Entity Framework regressions.

Back to 1.x I go I suppose.

@dannythomas13
Copy link
Author

@twilliamsgsnetx i'm also disappointed that I have had to retest my entire code base because this has affected a few places but I wouldn't be so hasty as to roll back, especially if you've already done the hard part of migrating. I would suggest simply using ef to do the selects but for any grouping operations do that after you have called toList

@maumar
Copy link
Contributor

maumar commented Aug 29, 2017

Problem is that when we apply Include on the selector, need to special case group by (apply include on element selector, rather than on qsre directly) - this logic is located in QueryModelExtensions.GetOutputExpression

However, for more complex scenarios the logic is not correct, as we only look at the top level query model and top level expression type to determine whether we are dealing with GroupBy query. In the problematic case group by is "hidden" in a subquery.

Workaround: add ToList() after GroupBy call, there is no perf hit, since (in 2.0) everything after Groupby is performed on the client anyway

maumar added a commit that referenced this issue Aug 31, 2017
…parameter of type 'Microsoft.EntityFrameworkCore.Metadata.IEntityType'

Problem was that we made some assumptions when compiling include, that do not hold true for complex queries involving group by and other operations that follow.

Once we build _Include(...) expression and try to substitute a qsre with it, we have compensation logic in case of groupby - trying to apply the substitution on the ElementSelector of the groupby. This logic is located in QueryModelExtensions.GetOutputExpression.
However, we assume that the outermost query model is the one that contains the groupby result operator. For the queries in the bug, this is not the case - GroupBy result operator is a subquery and therefore our old logic was not detecting it correctly, causing an error.

Fix is to find a correct query model (based on the qsre used in the Include result operator) and only after the correct QM (which can be in a subquery) is obtained, perform the compensation logic in QueryModelExtensions.GetOutputExpression.

Additional fix is needed when compiling collection navigation includes. We manually craft the QM that fetches child elements, and also assume that the outermost parent QM is the one that needs to be joined to. Similar approach is used here - find the "real" QM referenced in the include and build the collection-fetching QM based on that instead.
maumar added a commit that referenced this issue Sep 1, 2017
…parameter of type 'Microsoft.EntityFrameworkCore.Metadata.IEntityType'

Problem was that we made some assumptions when compiling include, that do not hold true for complex queries involving group by and other operations that follow.

Once we build _Include(...) expression and try to substitute a qsre with it, we have compensation logic in case of groupby - trying to apply the substitution on the ElementSelector of the groupby. This logic is located in QueryModelExtensions.GetOutputExpression.
However, we assume that the outermost query model is the one that contains the groupby result operator. For the queries in the bug, this is not the case - GroupBy result operator is a subquery and therefore our old logic was not detecting it correctly, causing an error.

Fix is to find a correct query model (based on the qsre used in the Include result operator) and only after the correct QM (which can be in a subquery) is obtained, perform the compensation logic in QueryModelExtensions.GetOutputExpression.

Additional fix is needed when compiling collection navigation includes. We manually craft the QM that fetches child elements, and also assume that the outermost parent QM is the one that needs to be joined to. Similar approach is used here - find the "real" QM referenced in the include and build the collection-fetching QM based on that instead.
maumar added a commit that referenced this issue Sep 1, 2017
…parameter of type 'Microsoft.EntityFrameworkCore.Metadata.IEntityType'

Problem was that we made some assumptions when compiling include, that do not hold true for complex queries involving group by and other operations that follow.

Once we build _Include(...) expression and try to substitute a qsre with it, we have compensation logic in case of groupby - trying to apply the substitution on the ElementSelector of the groupby. This logic is located in QueryModelExtensions.GetOutputExpression.
However, we assume that the outermost query model is the one that contains the groupby result operator. For the queries in the bug, this is not the case - GroupBy result operator is a subquery and therefore our old logic was not detecting it correctly, causing an error.

Fix is to find a correct query model (based on the qsre used in the Include result operator) and only after the correct QM (which can be in a subquery) is obtained, perform the compensation logic in QueryModelExtensions.GetOutputExpression.

Additional fix is needed when compiling collection navigation includes. We manually craft the QM that fetches child elements, and also assume that the outermost parent QM is the one that needs to be joined to. Similar approach is used here - find the "real" QM referenced in the include and build the collection-fetching QM based on that instead.
maumar added a commit that referenced this issue Sep 1, 2017
…parameter of type 'Microsoft.EntityFrameworkCore.Metadata.IEntityType'

Problem was that we made some assumptions when compiling include, that do not hold true for complex queries involving group by and other operations that follow.

Once we build _Include(...) expression and try to substitute a qsre with it, we have compensation logic in case of groupby - trying to apply the substitution on the ElementSelector of the groupby. This logic is located in QueryModelExtensions.GetOutputExpression.
However, we assume that the outermost query model is the one that contains the groupby result operator. For the queries in the bug, this is not the case - GroupBy result operator is a subquery and therefore our old logic was not detecting it correctly, causing an error.

Fix is to find a correct query model (based on the qsre used in the Include result operator) and only after the correct QM (which can be in a subquery) is obtained, perform the compensation logic in QueryModelExtensions.GetOutputExpression.

Additional fix is needed when compiling collection navigation includes. We manually craft the QM that fetches child elements, and also assume that the outermost parent QM is the one that needs to be joined to. Similar approach is used here - find the "real" QM referenced in the include and build the collection-fetching QM based on that instead.
@grokky1
Copy link

grokky1 commented Sep 4, 2017

Please consider a patch release - on myget, or an alpha on nuget?

There are a number of blocking bugs preventing us from migrating to 2.0. A prerelease/patch would help tremendously for now.

maumar added a commit that referenced this issue Sep 6, 2017
…parameter of type 'Microsoft.EntityFrameworkCore.Metadata.IEntityType'

Problem was that we made some assumptions when compiling include, that do not hold true for complex queries involving group by and other operations that follow.

Once we build _Include(...) expression and try to substitute a qsre with it, we have compensation logic in case of groupby - trying to apply the substitution on the ElementSelector of the groupby. This logic is located in QueryModelExtensions.GetOutputExpression.
However, we assume that the outermost query model is the one that contains the groupby result operator. For the queries in the bug, this is not the case - GroupBy result operator is a subquery and therefore our old logic was not detecting it correctly, causing an error.

Fix is to find a correct query model (based on the qsre used in the Include result operator) and only after the correct QM (which can be in a subquery) is obtained, perform the compensation logic in QueryModelExtensions.GetOutputExpression.

Additional fix is needed when compiling collection navigation includes. We manually craft the QM that fetches child elements, and also assume that the outermost parent QM is the one that needs to be joined to. Similar approach is used here - find the "real" QM referenced in the include and build the collection-fetching QM based on that instead.
@ajcvickers
Copy link
Member

Hi @dannythomas13, @twilliamsgsnetx, @grokky1. We are gathering information on the use of EF Core pre-release builds. You reported this issue shortly after the release of 2.0.0 RTM. It would be really helpful if you could let us know:

  • Did you consider testing your code against the pre-release builds?
  • Was there anything blocking you from using pre-release builds?
  • What do you think could make it easier for you to use pre-release builds in the future?

Thanks in advance for any feedback. Hopefully this will help us to increase the value of pre-release builds going forward.

@grokky1
Copy link

grokky1 commented Sep 13, 2017

@ajcvickers Hey thanks for looking into this! We upgraded to netcoreapp2.0 a few days after release. We managed to upgrade both ASP.NET Core and EF Core (after dealing with various breaking changes). But then various tests started failing! After much investigating, we realised it wasn't our code, but the new bits (BTW all our problems have something to do with Include() and ThenInclude()).

After ensuring it wasn't because of breaking changes, and realsing that the workarounds were too brittle (and reluctance to change our working code), we decided to just cut our losses and go back to 1.1.

My thoughts:

  • If there were patches for various bugs, we'd have definitely used them and given feedback. We spent over a week doing the upgrade, so it was difficult for us to admit defeat and roll back! Would have loved to actually finish the job. Though we'd wait for a stable before pushing our new code into production, we could at least move forward.
  • Like others have mentioned, when you folks release a patch/hotfix/whatever, please ensure they cover the issues only, and don't introduce new features at the same time, or change any existing features. Otherwise it's not really a hotfix and our tests may start failing for other reasons.

Hope to see 2.0.1 soon! 😄

--
UPDATE
To build on second point: A hotfix release plan like they do with Node.js would be great - e.g.

  1. a "stable with hotfixes" only nuget (e.g. 2.0.1, 2.0.2, 2.0.3), AND
  2. a "current with new features" bleeding edge with everything nuget (2.0.0, 2.1.0, 2.2.0)

So the "stable with hotfixes" would never have new features, only hotfixes.

@smitpatel smitpatel assigned smitpatel and unassigned maumar Sep 14, 2017
@ajcvickers
Copy link
Member

@grokky1 Thanks for the feedback. Could you provide some more information on how the EF Core releases are different from what you are suggesting? (It seems what you describe is what is happening, so I am probably missing something.) For example, was there a patch release that included changes you considered to be features? Or is it more that these releases don't come out fast enough? Thanks again for your thoughts.

@grokky1
Copy link

grokky1 commented Sep 16, 2017

@ajcvickers Maybe I'm the one who is confused... 😆 I remember trying one of the myget releases and it fixed something but then something else broke (I guess that's to be expected as it's a alpha/beta after all). I was looking in these forums and noticed others complain about the same thing. Maybe we were all mistaken!

So I'm glad you say the release plan works that way. But there should be more docs around this matter.

Please correct me if I'm wrong:

  • There is a package (on nuget) for EFCore stable
  • There are packages (on nuget) for prereleases, basically betas
  • There are packages on myget - I assume these are alphas / nightlies / whatever - basically the latest builds from your team, including new features

How do I get my hands on a hotfix without new features - which nuget do I use?

@ajcvickers
Copy link
Member

@smitpatel What's the status of this issue?

@ajcvickers
Copy link
Member

@grokky1

  • NuGet
    • RTM Patch releases (e.g. 1.1.0 -> 1.1.1) containing important bug fixes and no new features.
    • RTM Point releases (e.g. 1.0.x -> 1.1.0) containing bug fixes and new non-breaking features
    • RTM Major releases (e.g. 1.x.x -> 2.0.0) containing bug fixes, new features, and possibly breaking changes
    • Pre-releases in each of these categories, although we haven't yet done any pre-releases on NuGet for patch releases due to logistical issues on our side.
  • MyGet
    • Nightly builds, C.I. builds, etc. (That is, not releases)

@Eilon
Copy link
Member

Eilon commented Sep 18, 2017

This patch bug is approved for the 2.0.x patch. Please send a PR to the feature/2.0.1 branch and get it reviewed and merged. When we have the rel/2.0.1 branches ready please port the commit to that branch.

@grokky1
Copy link

grokky1 commented Sep 19, 2017

@ajcvickers Thanks for that breakdown! I'm sure it's documented somewhere but I didn't know for sure until now.

2.0.1 is coming... excited!

anpete added a commit that referenced this issue Sep 20, 2017
…ed for parameter of type 'Microsoft.EntityFrameworkCore.Metadata.IEntityType'

- Modify QuerySourceTracingExpressionVisitor so that it can return the "deepest" GroupBy QueryModel if one exists.
- Update the Include compiler to target the correct QueryModel when GroupBy is in play.
anpete added a commit that referenced this issue Sep 21, 2017
@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 Oct 4, 2017
smitpatel added a commit that referenced this issue Oct 4, 2017
This reverts commit 9b3a514.
@anpete anpete closed this as completed in 265e1e1 Oct 5, 2017
@smitpatel smitpatel reopened this Oct 5, 2017
@smitpatel smitpatel removed their assignment Oct 9, 2017
@anpete anpete closed this as completed Oct 11, 2017
@Eilon
Copy link
Member

Eilon commented Oct 23, 2017

Hi, we have a public test feed that you can use to try out the ASP.NET/EF Core 2.0.3 patch!

To try out the pre-release patch, please refer to the following guide:

We are looking for feedback on this patch. We'd like to know if you have any issues with this patch by updating your apps and libraries to the latest packages and seeing if it fixes the issues you've had, or if it introduces any new issues. If you have any issues or questions, please reply on this issue to let us know as soon as possible.

Thanks,
Eilon

@paillave
Copy link

paillave commented Jan 23, 2020

I am experiencing this bug again on EF Core 3.1. The same code worked on EF Core 2.1
the following returns some duplicate Security entities

            var tmp = await _dbContext.Set<Security>()
                .AsNoTracking()
                .Include(i => (i as RegularSecurity).Classifications)
                .Include(i => (i as ShareClass).SubFund).ThenInclude(i => i.Classifications)                 .Include(i => (i as ShareClass).SubFund).ThenInclude(i => i.Issuer)
                .Include(i => (i as ShareClass).SubFund).ThenInclude(i => i.Sicav)
                .Include(i => (i as RegularSecurity).Issuer)
                .Where(s => securityIds.Contains(s.Id))
                .AsNoTracking()
                .ToListAsync();

@maumar
Copy link
Contributor

maumar commented Jan 23, 2020

@paillave can you provide full code listing? (i.e. entities and dbcontext)

@maumar
Copy link
Contributor

maumar commented Jan 23, 2020

@paillave ideally, file a new issue with the info and reference this one.

@gterdem
Copy link

gterdem commented Aug 11, 2020

@maumar I came across similar issue with @paillave, i don't know if he solved it or not.
Created a new issue at dotnet/EntityFramework.Docs#4099 since it seems different than the topic.

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. regression type-bug
Projects
None yet
Development

No branches or pull requests

10 participants