dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.55k stars 3.13k forks source link

Discussion on lazy-loading of navigation properties #3797

Closed AndiRudi closed 1 year ago

AndiRudi commented 8 years ago

Note:

With regard to feedback, I think it is worth reiterating some comments made a few months ago. We read and consider all feedback (and try to make the right choices based on it) no matter how it is delivered; that is, whether it is provided politely and with respect or not. Yelling at us or others with different opinions at best makes you feel good and us/them feel bad. It doesn’t achieve anything concrete. We are doing our best to prioritize features and implement them in the best way we can. We really appreciate your feedback and it makes a big difference on shaping the product. We personally appreciate it more when feedback is delivered in a respectful way, but please don’t stop providing constructive feedback.


Original issue:

Hi,

i was wondering if I am the only one that thinks EF Core is useless without Lazy Loading? Or do I do something wrong? Lets just consider a simple scenario when a course provider cancels a course on a course booking plattform written in MVC.

The user calls courses/cancel/1. The action would get the course and call cancel method like here

Course course = context.Courses.SingleOrDefault(c => c.Id = 1);
course.Cancel();

The cancel method then needs to cancel each booking on the course so it would do something like this

foreach(Booking booking in this.Bookings)
{
    booking.Cancel();
}

The booking in turn would refund the transaction

foreach(Transaction transaction in this.Transactions)
{
    transaction.Refund();
}

So for this to work, lazy loading is needed?! (The solution would be to eager load all the data on the controllers action method. But I do not know what will be needed from the controller?)

I'd appreciate any information on that.

jemiller0 commented 8 years ago

I think you can do something like the following to eagerly load everything.

Course course = context.Courses.Include(c => c.Bookings).ThenInclude(b => b.Transactions).SingleOrDefault(c => c.Id = 1);

Otherwise, I'm assuming EF 7 has a way of explicitly loading properties like the following. However, I just upgraded to EF 7 today and haven't done that before. I think they changed the API around some. I'm assuming there has to be an equivalent though.

https://msdn.microsoft.com/en-us/data/jj574232.aspx#explicit

Also, as far as I know this site is for submitting bug reports, not questions.

rowanmiller commented 8 years ago

We will be implementing explicit loading, but it's not there yet. Lazy loading is still undecided... so feedback is good - thank you.

Also, as far as I know this site is for submitting bug reports, not questions.

I think this can be considered a feature request. Lazy loading is still on the list of things we are considering... but it looks like we don't have a dedicated issue tracking it, so we'll use this one.

yukozh commented 8 years ago

+1 for lazy load

dneimke commented 8 years ago

I'm not saying that Lazy Loading isn't important, but the example provided is not a good example of "why". Such logic should live in a business logic layer which would orchestrate all of the actions required to fulfil the transaction.

jemiller0 commented 8 years ago

It should at least have explicit loading. EF 7 doesn’t even have that at this point. In the example given, you are forced to load data that you don’t need since it was nested collections. I guess that’s no worse than it would be with lazy loading. I think you could be more selective with explicit. I guess the only way to load properties is using Include() ThenInclude() at this point. I find it surprising that the release won’t even have explicit loading. I guess it will still be workable. Lazy loading can get you into a lot of trouble though if you don’t pay attention to what’s happening. It can cause performance problems.

Jon

rowanmiller commented 8 years ago

It's not as tidy as proper explicit loading support, but you can run a second LINQ query to load related data (EF will fixup all the navigation properties for you).

Course course = context.Courses.SingleOrDefault(c => c.Id = 1);

// sometime later, load the related data
context.Bookings.Where(b => b.CourseId == course.CourseId).ToList();
AndiRudi commented 8 years ago

I already thought about this approach. But the problem is, that I do not have access to the context inside a model. Or has this changed in EF Core? I would need a way to inject the context or some other scope into the model to have the same context.

rowanmiller commented 8 years ago

@AndiRudi that is true... but that is also true of explicit loading since explicit loading is about calling an EF API to load data.

dneimke commented 8 years ago

@rowanmiller is there a way to prevent that behavior?

Course course = context.Courses.SingleOrDefault(c => c.Id = 1);

// sometime later, load the related data
context.Bookings.Where(b => b.CourseId == course.CourseId).ToList();

E.g. Ask EF not to 'fix-up' the navigation properties automatically?

rowanmiller commented 8 years ago

@dneimke sure, if you want to execute a query without all the change tracking, fixup to existing entities that are already tracked, etc., then you can use the AsNoTracking() method on the query.

Out of interest, what is the scenario where you don't want that to happen?

dneimke commented 8 years ago

Running tests against an InMemoryProvider. I want to ensure that I can execute multiple test cases from a Single serviceProvider instance without having the results dirtied by side effects

rowanmiller commented 8 years ago

@dneimke That would work. Possibly better to register the context as scoped and then have a scope per test? That way you get a clean context per test.

dneimke commented 8 years ago

True. Another example is that I want to write a test console app. To begin with I want to Ensure my Db exists and to seed it with data:

// Called from Main
db.Database.EnsureDeleted();
db.Database.EnsureCreated();

void SeedData()
{
    // Add some parent records using Add or AddRange

    // Add some child records using Add or AddRange
}

RunQuery();   // see below

Then, after adding that data, I want to query my DbSet and return only a Parent record - without Includ'ing Children.

var parent = db.Parent.SingleOrDefault(...);

How can I ensure that the Children won't be attached?

int? count = parent.Children?.Count;     // should be null
ilmax commented 8 years ago

I would downvote this if possible, lazy loading is a source of problem, and hides some db work. Explicit is better than implicit IMHO! I prefer EF Core never implement this.

rowanmiller commented 8 years ago

@dneimke you should use a separate instance of your context for the seeding.

AndiRudi commented 8 years ago

@ilmax I don't think that easying the writing of code has anything to do with hiding. If you understand lazy loading, you don't have issues. Let's just go into the example of @rowanmiller above.

Explicit:

Course course = context.Courses.SingleOrDefault(c => c.Id = 1);
//This translates more or less to SELECT * FROM COURSE WHERE Id = 1

// Sometime later, load the related data
context.Bookings.Where(b => b.CourseId == course.CourseId).ToList();
//This translates more or less to SELECT * FROM Bookings WHERE CourseId = ...

Lazy:

Course course = context.Courses.SingleOrDefault(c => c.Id = 1);
//This translates more or less to SELECT * FROM COURSE WHERE Id = 1

// Sometime later, load the related data
course.Bookings.ToList();
//This translates more or less to SELECT * FROM Bookings WHERE CourseId = ...

I can't see any difference here, but I see the following improvements

  1. The data context used by the second call automatically is the same than used by the first one. This is important for the Unit-Of-Work. In the explicit call a developer could even create another data context (ok, thats not directly true because correct injection would take care of this, but it feels better)
  2. I don't want to query for the context all the time. This is boilerplate code.
  3. It is easier to read and more ddd than.
jemiller0 commented 8 years ago

I think a lot of people get burned by lazy loading. I can understand why some people don't like it. I think it's a nice feature to have, but, I can understand why people don't like it. Namely, because when you develop an app, you need to watch the SQL logs and make sure the ORM isn't kicking off a lot of N+1 queries and the like. I haven't seen much of that with EF, but, had issues with this with EclipseLink on another platform. As long as there is a way to get everything you need loaded in an efficient manner, then, I would probably be all right with not having lazy loading. The new way that EF loads collections looks cool to me. I'm hopeful that this is more efficient than N+1 queries.

ilmax commented 8 years ago

@AndiRudi my opinion about lazy loading is different, here are my concerns

  1. it's not simplify but hiding (at least IMHO) because a non EF expert cannot see something EF related in course.Bookings.ToList() and explicit is better than implicit.
  2. Requires that your model matches some EF requirements (e.g. virtual navigation properties) and I prefer avoid.
  3. It's super easy to fall in the SELECT N+1 trap
AndiRudi commented 8 years ago

@ilmax I see... but then we don't need the navigation properties (.Bookings) at all? Then I always have to query the database with context.Bookings to make sure I have the correct data and using .Bookings would always be kind of "I dont know whats in there"....

The only case it would make sense is when using .Include(). And .Include() only makes sense when you write everything in one method because you'll never know what data is needed later (exception you write everything in one method).

jemiller0 commented 8 years ago

OK, you convinced me. I think lazy loading should be there as well. I'm surprised that even explicit loading isn't there. I think that tells you where EF 7 is. It is a rewrite and important features are still missing. EF is my preferred ORM for .NET, but, it has always lagged behind other ORMs such as NHibernate. Microsoft was late to the game to begin with. In some respects, they still aren't to the point where NHibernate was years ago (although in other respects they are ahead). Unfortunately, RedHat bought JBoss and they jettisoned NHibernate. Last time I checked, the new developers couldn't seem to even figure out how to put a link to the documentation on their website. So, I will stick with EF, even if it isn't perfect. I have a feeling we will be waiting for quite some time though before we see lazy loading.

AndiRudi commented 8 years ago

Today i tried the new C# Interactive window and I think this is another +1 for Lazy loading. Maybe not as important, but quite cool.

Microsoft (R) Roslyn C# Compiler version 1.1.0.51109
Loading context from 'CSharpInteractive.rsp'.
Type "#help" for more information.
> #r "C:\coursika\Stories\2302-UpdatePurchaseOrder\Coursika.Core\bin\Debug\Coursika.Core.dll"
. using Coursika;
. var db = new CourseContext("Data Source=localhost;Initial Catalog=CoursikaProd;Integrated Security=SSPI;MultipleActiveResultSets=True");
. var course = db.Courses.FirstOrDefault();
. Console.WriteLine(course.Name.ToString());
. course.Bookings.Where(c => c.BookingStatus == Coursika.Models.BookingStatus.Booked).ToList().ForEach(c => Console.WriteLine(c.BookingStatus));
. 
Kurs 17 Pilates am Vormittag
Booked
Booked
Booked
Booked
Booked
Booked
> 
heraclesaraujo commented 8 years ago

I'd vote for this to be something separated, like a plugin.

use dependency injection for creating a lazy loaded proxy

app.UseEntityFramework().UseSqlServer().UseLazyLoading()

or

some kind of explicit mapping for the entity HasMany(model=>model.Collection).LazyLoad();

yukozh commented 8 years ago

I'd vote for this to be something separated, like a plugin.

use dependency injection for creating a lazy loaded proxy

app.UseEntityFramework().UseSqlServer().UseLazyLoading()

or

some kind of explicit mapping for the entity HasMany(model=>model.Collection).LazyLoad();

:+1: +1

Or SomeDbContext.SomeDbSet.Where(xxx).LazyLoad();

Spongman commented 8 years ago

please consider un-removing lazy-loading. I don't have an issue with navigation properties 'hiding' database calls, but I do have an issue where the context itself is not necessarily available in scope. the problem with explicit loading is that all the code now needs to be context-aware which means you need to thread the context instance throughout your code. a significant part of our code navigates our entity graph via the navigation properties and entity extension methods, and the context is just not in scope. also we have found .Include() to be woefully inadequate due to the exponential size of the result set for non-trivial graph fetches.

alaatm commented 8 years ago

I hope lazy-loading can make it to RTM. You can have it off by default and have API to explicitly enable it so that users are aware of the implications. At the end of day, lazy-loading has many advantages specially in the case where a DbContext instance shouldn't be exposed in certain places, e.g. model classes.

Boglen commented 8 years ago

Or something like

Db.Parent.Single(p => p.Id == 1).WithLazy(p => p.ChildsThatIDontKnowUseOrNot);

Grinderofl commented 8 years ago

HasMany(model=>model.Collection).LazyLoad(); would work the best. This way, each individual property can be configured. There could also be a convention which enables Lazy Loading on properties by default if needed.

farlop commented 8 years ago

+1 for Lazy Load. Maybe the best approach could be leaving it enabled by default (as current EF6) and add some mechanism to turn it off globally and use an extension method to enable it where the developer decides (on a single query or on a single relationship).

TrabacchinLuigi commented 8 years ago

I rarely use lazy loading, it only gave me headaches (how many of us end up hitting the whole database after a serialization?), i personally prefer eagerly loading, i won't change anything, i really like how good the new change tracker is, just use the same context if you want something related, or use a new one for unrelated stuff. +1 for eagerly loading, makes everything easier to understand

aemarkov commented 8 years ago

I facing same problem and tried to solve it using Include, but VS says, that this method is undefined.

divega commented 8 years ago

@Garrus007 Include() and ThenInclude() are extension methods for IQueryable<T> defined in the Microsoft.Data.Entity namespace. Are you missing a using statement with that namespace in your code?

aemarkov commented 8 years ago

@divega Oh, greath thanks.

I tired solving progblem like this

Course course = context.Courses.SingleOrDefault(c => c.Id = 1);

// sometime later, load the related data
context.Bookings.Where(b => b.CourseId == course.CourseId).ToList();

and using Include() helps.

Grinderofl commented 8 years ago

Lazy Loading is an integral pattern of enterprise application architecture. NHibernate + Fluent NHibernate is a good example of a well-structured and built lazy loading, where one specifies the behaviour from entity to entity. What is happening here is a requirement of heavy coupling of the ORM with business rules and optimization, and the necessity for business layer to know too much about the logic of the data source, with Include() statements. That does not domain driven design fit.

I'd be willing to give a go with implementing it, if there is a proposed mechanism for that as well as the best way to do that? I would imagine adding the feature to Relational and derivatives with perhaps extension methods akin to ReferenceCollectionBuilder.LazyLoad(LazyLoadingOptions) => LazyLoadingOptions = enum{ None, Select, Join } ?

@rowanmiller Thoughts?

strich commented 8 years ago

Seems like this feature barely needs another +1, however whilst this is being sorted out can we please put specific mention of the lack of this feature and some example Include() code in the EF Core and ASP.NET Core docs? I just spent the last 2 days trying to figure out why my project wasn't working with one-to-many relationship and almost gave up on EF entirely.

The Blog example here specifically could benefit from an additional step showing the preferred manual method of using the one-to-many relationship between the Blog and its Posts.

iancooper commented 8 years ago

I'm never sure about Lazy Loading. It seems to cause a lot of ORM heartbreak. It suffers from two main problems. First is is unbounded - if your child collection has 10000 items that Lazy Load will cause you problems; second if it does get called, eve if we want the children loaded it results in Select N+1 problems.

In many cases it is easier to write peformant code by ensuring that the path the code takes for a request is known, and the shape of the object graph you need to load to satisfy that request is known, so that you can eager load instead.

I certainly wouldn't make this the default if the it is supported. I would force you to switch it on.

jemiller0 commented 8 years ago

Personally, I wonder about the .Include() way of doing things. I guess the way the queries are handled in EF 7 is more efficient than in 6, but, in some cases it seems like if EF did N+1 queries with a cache, it would be faster. I've done some testing with JPA and EclipseLink in Java and the queries seemed to execute faster even though they were doing N+1 queries. The code that I tested was a list page in a web app with paging and 100 results. I found that EclipseLink was smart enough that if a value was already in the cache, it wouldn't execute the sub-query again. I always thought N+1 was something to be avoided at all costs, but, the giant queries that at least EF 6 generates seem a lot worse. I've also noticed things in EF 6 where if you are doing a projection that only includes a few fields if you use dot notation to traverse through properties more than a level deep, it will do nested queries and include all table columns instead of only the columns that you're including the projection. I noticed this when using the MySQL provider. I don't know if it's an issue with the SQL Server provider. I also wonder about the provider model. I don't understand why different vendors need to make a driver specifically for EF as opposed to just a standard ADO.NET driver. I guess it makes more sense now that EF 7 can query non-relational data stores. But, in the case of SQL data stores, it seems like there should just be different dialects like N/Hibernate has. Maybe EF is the same, I don't know, but, I wonder how much effort it takes for a vendor to implement a driver. I really wish all this got sorted out because it seems that EF is over a decade behind other ORMs in some respects. I'm planning on testing NHibernate some today because the SQL queries EF 6 is generating for some of the projections I have is just way too out of hand. I want to upgrade to EF 7, but, last time I tried it, there were still major problems with it, like it using INNER JOINs for everything when it should be using LEFT JOINs.

anpete commented 8 years ago

@iancooper Exactly.

What I'm getting from this thread is that there are occasions when N+1 is acceptable because the productivity benefits outweigh any potential perf implications.

I agree that making it opt-in could be a good compromise.

rariancom commented 8 years ago

+1 for eagerly Loading Lazy Loading off by default, should be included, otherwise people might stick to EF6

yukozh commented 8 years ago

Yeah, I hope both eager and lazy are included in ef7

frbar commented 8 years ago

+1 for lazy-loading, disabled by default. Everyone agrees that with great power comes great responsibility...

nefremov commented 8 years ago

I suppose that lazy-loading cannot be implemented in current approach because EF7 doesn't use proxies at all. I'm ready to implement my own lazy-loading proxies. It is possible because you have provided me a hook to intercept object materialization, namely IEntityMaterializer.CreateEntity(...). I have successfully injected my custom very simple inheritors of SqlServerDbContextOptionsBuilder, SqlServerModelSource, ModelBuilder and Model so that my MyModel.AddEntityType() returns my own MyEntityType : EntityType, IEntityMaterializer required to intercept materialization. But I need access to current DbContext inside IEntityMaterializer.CreateEntity(...) to build lazy-loading proxy. Now DbContext is available in the callers of IEntityMaterializer.CreateEntity(...), e.g. UnbufferedEntityShaper.Shape(...) via queryContext.StateManager.Context. So to use DbContext during entity materialization I have to 1) use my own interface IMyEntityMaterializer instead of IEntityMaterializer 2) inject inheritor of EntityMaterializerSource with modified CreateMaterializeExpression(...) that uses my IMyEntityMaterializer to build materializationExpression 3) inject inheritor of MaterializerFactory with modified CreateMaterializer(...) to close materializationExpression with DbContext parameter into LambdaExpression materializer. 4) inject inheritor of UnbufferedEntityShaper with modified Shape(...) to pass DbContext to materializer invocation (which internally calls IMyEntityMaterializer.CreateEntity) 5) make the same changes to other shapers

Please, consider to change IEntityMaterializer interface and classes mentioned above to provide DbContext for object materialization. EF itself doesn't use IEntityMaterializer so these changes won't break anything. But these very small changes allows us to make third-party lazy-loading library on top of EF core.

rcollina commented 8 years ago

I agree with @iancooper .

Eager loading forces the developer to acknowledge the data access pattern, making it explicit. In my opinion, lazy loading is deceivingly handy to have.

nefremov commented 8 years ago

Returning to my comment, the changes for explicit passing DbContext to IEntityMaterializer turn out wider than it seemed before. To support context-aware proxies the struct EntityLoadInfo should be rewritten to grab DbContext and Materializer delegate type should be changed everywhere from Func<ValueBuffer, object> to Func<ValueBuffer, DbContext, object> or to Func<DbContext, ValueBuffer, object>.

Besides, I wrote a small demo for lazy loading on top of EF https://github.com/nefremov/LazyEntityFramework To minimize injections DbContext is passed to IEntityMaterializer inside ValueBuffer (ugly!!!). Also explicit (instead of dynamically generated) proxies are used for clearance. I've tested just a couple of common scenarios but I suppose that suggested approach may work.

jemiller0 commented 8 years ago

So, if lazy loading proxies aren't supported, I'm assuming change tracking proxies aren't supported either? Is support for that planned in the future? I fear that without change tracking proxies, update performance will be poor. From what I remember, performance was significantly worse in EF 6 without them compared to the pre-DbContext days when collections were using the EF specific collection classes. From what I remember, using change tracking proxies improved performance, but, it still wasn't as fast as EF 4/5. I think performance is going to be horrible using the snapshot approach. Also, I think it uses a lot more memory from what I remember. I think this will make EF unusable for anything except very simple use where you have a small number of entities in the context. I was hopeful that with the batch update support, things would be improved. I'm not so sure about that, if doing things without proxies is the plan.

SergeySagan commented 8 years ago

Certainly need Lazy Loading... how do you propose doing DDD without it?

Angeland commented 8 years ago

So lazy loading is non existant in EF7 to prevent bad practice and over-fetching of unneeded data? Well, its not a good reason, the tools should not limit the developer to shoot of his foot straight off. Who uses EF without lazy loading anyways?

bdelaney commented 8 years ago

@strich thanks for your comment about including information like this in an example. I found myself up the same creek. I thought my model was completely wrong when linq was not working the way I expected it to. I even resorted to using Linqpad and running things directly on my SQL database. Where of course, all the usual Linq syntax worked. Plus it took me over a day to track down the "new improved" way of creating many-to-many relationships. That had me drooling and thinking I was going mad. But now with over 40 hrs of work into the new EF 7 way, I'm still struggling with navigation properties in my many-to-many.

A much more comprehensive example than the simple blogging tutorial is necessary. Otherwise any of us creating complex models are going to be very frustrated.

DoddsDonnelly commented 8 years ago

+++++ 1 for lazy loading

PeterLindgren-se commented 8 years ago

Whether lazy loading default is on or off doesn't matter much, the important thing IMHO is to be able to specify it on a per-query basis – there are occasions when you only need the base table data, and other occasions when you need a whole tree loaded. For complex queries with many and deep dependent objects, specifying .Include() for each defeats the idea behind an ORM, and makes the query difficult to read and understand for other humans. Also, when extending the model you have to make sure you .include() the new dependent objects everywhere, a tedious process, not to mention all tests and test cases.

jemiller0 commented 8 years ago

I have a feeling lazy loading will get in there eventually. It's just not as high priority item as some other things, like the fact that until a recent patch, it was using inner joins on everything including nullable fields. I think a lot of this has to do with the fact that EF is being completely rewritten. The same thing is true with .NET Core, and ASP.NET Core. I'm less worried about this than what is going on with ASP.NET. For example, no Web Forms and I don't see Microsoft ever doing anything about that. I don't know why it didn't occur to me recently, but, I think the whole reason Microsoft is making things cross platform is because of phone.