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

Projections With Full Types Not Fixing Up Navigations on Sqlite and SQL Server #7131

Closed
julielerman opened this issue Nov 25, 2016 · 31 comments
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

@julielerman
Copy link

julielerman commented Nov 25, 2016

I've searched for a related issue as well as blog posts/docs and am not finding anything. I know it's out there somewhere but have to stop looking!

Also related to #4007

Steps to reproduce

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

namespace ProjectionProblem
{
  public class Parent
  {
    public Parent()
    {
      Children = new List<Child>();
    }

    public int Id { get; set; }
    public string Description { get; set; }
    public List<Child> Children { get; set; }
  }

  public class Child
  {
    public int Id { get; set; }
    public string Description { get; set; }
  }

  public class MyContext : DbContext
  {
    public DbSet<Parent> Parents { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
      optionsBuilder.UseSqlite("Data source=ProjectionProblem.db");
    }
  }

  public class Program
  {
    private static void Main(string[] args)
    {
      using (var context = new MyContext())
      {
        context.Database.EnsureDeleted();
        context.Database.EnsureCreated();
        var parent = new Parent {Description = "Parent1"};
        parent.Children.Add(new Child {Description = "Child1"});
        parent.Children.Add(new Child {Description = "Child2"});
        context.Parents.Add(parent);
        context.SaveChanges();
      }
      using (var context2 = new MyContext())
      {
        var newtype = context2.Parents.Select(p => new {Parent = p, p.Children}).ToList();
      }
    }
  }
}

The issue

Navigations not being fixed up in results. newtype.Children has the child objects, but newtype.Parent.Children is empty.

Further technical details

EF Core version: 1.1
Operating system:
Visual Studio version: vs2015 but really not applicable

Other details about my project setup:

@julielerman
Copy link
Author

Just to add to this...other projections tests are also quite messy. Again, this is probably noted somewhere that I'm just not finding ...not mentioned in the roadmap or in this repo but that could be affected by how I'm searching.

Filtered projections .. e.g.
var newtype = context3.Parents.Select(p =>
new { Parent = p, Children=p.Children.Where(c=>c.Description=="Child2") }).ToList();

The queries are strange. Non of them have any reference to the children.
The results are strange. What's in the Children property is a ParameterInjector

Are we supposed to be avoiding projections for the time being? Besides for reasons of the N+1 issues?
So, suggest other strategies ala multiple queries with fixup, or explicit loads, and of course log & test to see what's best? (or wait :) )
Thanks

@maumar
Copy link
Contributor

maumar commented Nov 25, 2016

Try:

var newtype = context2.Parents.Select(p => new {Parent = p, p.Children.ToList()}).ToList();
}

Otherwise the child collections won't be automatically enumerated. Perhaps that's the problem.

@julielerman
Copy link
Author

julielerman commented Nov 26, 2016

Thanks for the suggestion but no you can't even do that ...p.Children is not a queryable. And it is enumerated in that it is getting recognized and there is a query for it and they are being returned, just not fixed up.

And although I know EF Core is not EF6, that would be a very strange change from how LINQ has always worked.

@maumar
Copy link
Contributor

maumar commented Nov 27, 2016

It's worth pointing out that the query plan will contain all the queries necessary to execute a given Linq statement, regardless of whether those queries actually going to be enumerated or not. Best way to ensure that the queries actually run is to look at Profiler in SSMS

@julielerman
Copy link
Author

julielerman commented Nov 27, 2016 via email

@tonkajuris
Copy link

This seems to work with the Include statement using sqlite:
var types2 = context2.Parents.Include(x => x.Children).Select(p => new { Parent = p, p.Children }).ToList();

SQL Server returns the child items without requiring the include:
var types = context2.Parents.Select(p => new { Parent = p, p.Children }).ToList();

@maumar
Copy link
Contributor

maumar commented Nov 27, 2016

Ahh missed the part about Sqlite. Looks like a bug then. I know for a fact that it works fine on SqlServer that's why I made all those suggestions :)

@maumar maumar self-assigned this Nov 27, 2016
@maumar maumar added this to the 1.2.0 milestone Nov 27, 2016
@maumar maumar changed the title Projections With Full Types Not Fixing Up Navigations Projections With Full Types Not Fixing Up Navigations on Sqlite Nov 27, 2016
@julielerman
Copy link
Author

julielerman commented Nov 27, 2016 via email

@maumar
Copy link
Contributor

maumar commented Nov 27, 2016

I see, the fixup should be happening for sure. I will look into it.

@julielerman
Copy link
Author

Thanks!

Here's a repo with logging using SQLite that tests (with console, not tests):
https://github.com/julielerman/EF110ProjectionProblem/

fixup from separate queries (fixup works)
fixup from projection (fixup doesn't work)
attempt to filter children on projection (no query for children!)
filter when using load (works as expected)

Should I open a separate issue for the second problem I raised? Where the filtered projection on children

var newtype = context3.Parents.Select(p =>
new { Parent = p, Children=p.Children.Where(c=>c.Description=="Child2") }).ToList();

didn't even send a query for the children to the database? Again, workarounds exist, but I would expect this to work the way it's always worked in EF.

@tonkajuris
Copy link

tonkajuris commented Nov 27, 2016

I was able to verify SQL Server profiler was returning a query. Haven't found a good way to profile SQLLite.
I've used Explain and Timer with selects without success.
var newtype = context3.Parents.Select(p => new { Parent = p, Children=p.Children.Where(c=>c.Description=="Child2") }).ToList();
I was able to profile the following:
exec sp_executesql N'SELECT [e].[Id], [e].[Description], [e].[ParentId] FROM [Children] AS [e] WHERE ([e].[ParentId] = @__get_Item_0) AND ([e].[Description] = N''Child2'')',N'@__get_Item_0 int',@__get_Item_0=1

@julielerman
Copy link
Author

julielerman commented Nov 27, 2016

Whoa .. okay I'm using Contains in my filter in Sql Server but == in the sample I built up to isolate that uses SQLite. Going to look for the 101st time at my log ...mmm maybe not at my log. I'll use sql server with sqlprofiler.

@julielerman
Copy link
Author

No difference on my end. Model may be different. Note (above) that I do NOT have a FK property in the child. This does not impede the fixup when I do separate queries. It may be impeding the fixup in the first projection (p, p.Children) and it may be impeding the QUERY in the 2nd projection . I'll modify my model. (Though if that's the case, it's still a bug ..fixup works in some cases but not others) BRB!

@julielerman
Copy link
Author

Still no difference with the explicit FK in child - no fixups, no additional query. So now i'm really confused. You are running my repo? Are you using a nightly build? I'm using 1.1.0 (released). my sqlserver tests are in a completely different project/model. I will change the repo to use SQL Server and profile that, but on the other project, my log and sqlprofiler are matching

@julielerman
Copy link
Author

Still no luck with the filtered projection. Only thing coming through profiler/log is
SELECT [p].[Id], [p].[Description]
FROM [Parents] AS [p]

I cannot see what we could be doing differently. Are you sure you're not seeing the output from the query using "context4"?

@maumar
Copy link
Contributor

maumar commented Nov 27, 2016

Currently the filtered case shouldn't automatically yield the results for child chollection, only if you add ToList or enumerate the collection explicitly

@maumar
Copy link
Contributor

maumar commented Nov 27, 2016

It is a known issue although I don't think we have a separate item for it. I just added some additional context to #4007 that was suggested by Diego in another thread, which might be how we untlimately tackle the problem

@julielerman julielerman changed the title Projections With Full Types Not Fixing Up Navigations on Sqlite Projections With Full Types Not Fixing Up Navigations on Sqlite and SQL Server Nov 27, 2016
@julielerman
Copy link
Author

@maumar wrt the fixup issue, I wanted to mention that the child objects are not being tracked when the results are materialized which is why the fixup isn't happening.

@maumar
Copy link
Contributor

maumar commented Nov 27, 2016

I see, we do some special handling of the result collections (wrapping them in a method IIRC) this might be what's preventing the entities to be tracked

@julielerman
Copy link
Author

@maumar yes, the ToList workaround does trigger the query ...still no fixup though. ;) But that just goes back to teh same problem as this issue is about. It's notable only because I need to make sure EF6 & earlier users are aware since its different behavior. Telling them "it's not Ef6, don't expect it to work the same" doesn't work very well. So I 'm just trying to point out what to watch out for. :)

@danobri danobri mentioned this issue Mar 11, 2017
16 tasks
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview1, 2.0.0 Apr 19, 2017
maumar added a commit that referenced this issue May 23, 2017
…on Sqlite and SQL Server

Problem was that entities created by MaterializeCollectionNavigation were not being tracked - TrackEntities only works on "naked" entities.

Fix is to add delegate argument to MaterializeCollectionNavigation method that would track the elements of the collection if the query is supposed to be tracked. We already have a similar logic for the Include.
maumar added a commit that referenced this issue May 23, 2017
…on Sqlite and SQL Server

Problem was that entities created by MaterializeCollectionNavigation were not being tracked - TrackEntities only works on "naked" entities.

Fix is to add delegate argument to MaterializeCollectionNavigation method that would track the elements of the collection if the query is supposed to be tracked. We already have a similar logic for the Include.
maumar added a commit that referenced this issue May 23, 2017
…on Sqlite and SQL Server

Problem was that entities created by MaterializeCollectionNavigation were not being tracked - TrackEntities only works on "naked" entities.

Fix is to add delegate argument to MaterializeCollectionNavigation method that would track the elements of the collection if the query is supposed to be tracked. We already have a similar logic for the Include.
@maumar
Copy link
Contributor

maumar commented Jun 6, 2017

This is partially addressed by #8584 - if the collection navigation is not composed on, we re-use include pipeline which handles tracking. If the collection is filtered however, we still use the old rewrite and results are not being tracked.

@maumar
Copy link
Contributor

maumar commented Jun 26, 2017

assigning to @anpete since he is reworking this entire area

@maumar maumar assigned anpete and unassigned maumar Jun 26, 2017
@anpete
Copy link
Contributor

anpete commented Jun 26, 2017

@maumar We should be good here now, right?
@julielerman Are you able to give this a try on the latest nightly or Preview1?

@julielerman
Copy link
Author

@anpete I'll run through my various scenarios and report back

@anpete
Copy link
Contributor

anpete commented Jun 27, 2017

Thanks!

@julielerman
Copy link
Author

julielerman commented Jun 27, 2017

My integration tests using SQL Server LocalDb (without the supporting classes, but hopefully that's enough) are failing with preview 1. Will check with nightly next. I've seeded my db so I know that there are 3 quotes total in there.

  [TestMethod] //PASSES
       public void EagerLoadViaProjectionRetrievesRelatedData()
       {
           using (var context = new SamuraiContext())
           {
               var samuraiList = context.Samurais
             .Select(s => new { Samurai = s, Quotes = s.Quotes })
             .ToList();
               Assert.AreEqual(3, samuraiList.Sum(s => s.Quotes.Count()));
       
           }
        }

       [TestMethod]  //FAILS Expected <3>, Actual <0>
       public void EagerLoadViaProjectionTracksRelatedData()
       {
           using (var context = new SamuraiContext())
           {
               context.Samurais
               .Select(s => new { Samurai = s, Quotes = s.Quotes })
               .ToList();
               Assert.AreEqual(3, context.ChangeTracker.Entries<Quote>().Count());
           }
       }
       [TestMethod]  FAILS: Expected <2>, Actual <1>
       public void EagerLoadViaProjectionTracksRelatedDataWhenModified()
       {
           using (var context = new SamuraiContext())
           {
               var samuraiGraph = context.Samurais
             .Select(s => new { Samurai = s, Quotes = s.Quotes })
             .First();
               samuraiGraph.Samurai.Name = "make name different";
               samuraiGraph.Quotes[0].Text = "make quote different";

               Assert.AreEqual(2, context.ChangeTracker.Entries().Count(e => e.State == EntityState.Modified));
           }
       }

@julielerman
Copy link
Author

julielerman commented Jun 27, 2017

[Update: these problems were due to confusion over SDKs etc. A clean VM with VS2017preview3 and new dotnet core run time and SDK solved the problem]
Status:
Using EF Core v2.0.0-preview3-25895
Also explicitly installed NETStandard.Library.NETFramework.2.0.0-preview3-25415-01
And had to install tuples library :) which is v4.4.0-preview3-25415-01
Still getting errors about tuple library missing. I see a new version of vs2017 preview just got pushed up a few hours ago. Will try that. I'm avoiding using .net core for this because that adds more layers of problems to the nightly builds. (I learned the hard way)
-aha! .NET 4.7 wasn't even installed

@julielerman
Copy link
Author

SUCCESS!!

Finally got things sorted out so that I can run my tests against the nightly EF Core 2.0 build. And my tests for projections are passing:

image

There are two things I am checking for in my tests:

  1. Related data retrieved via projection is being tracked. Earlier, it was getting retrieved and in memory but unknown by the change tracker.
  2. Filtering related data in a projection is working as expected. Earlier, the filter was ignored. In my test, I'm querying for samurais with their quotes but ONLY those quotes with a particular word in it. Of the 3 total quotes, only 2 have that word. The filter is now only retrieving those quotes.

THANK YOU!! This is an important fix.

I will go now back and do this with EF Core 2 Preview 2

@julielerman
Copy link
Author

Preview 2 in netstandard20 project tested via an MSTest netcoreapp2.0 project (just CLI + VSCode for this ...so much simpler :) )
All tests are passing

image

@anpete
Copy link
Contributor

anpete commented Jun 28, 2017

Thanks for taking the time to try this out!

@anpete
Copy link
Contributor

anpete commented Jun 28, 2017

Closing as fixed in 2.0

@anpete anpete closed this as completed Jun 28, 2017
@anpete anpete added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 28, 2017
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

No branches or pull requests

5 participants