-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Discussion on lazy-loading of navigation properties #3797
Comments
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. |
We will be implementing explicit loading, but it's not there yet. Lazy loading is still undecided... so feedback is good - thank you.
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. |
+1 for lazy load |
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. |
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 |
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).
|
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. |
@AndiRudi that is true... but that is also true of explicit loading since explicit loading is about calling an EF API to load data. |
@rowanmiller is there a way to prevent that behavior?
E.g. Ask EF not to 'fix-up' the navigation properties automatically? |
@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? |
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 |
@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. |
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:
Then, after adding that data, I want to query my DbSet and return only a Parent record - without Includ'ing Children.
How can I ensure that the Children won't be attached?
|
I would downvote this if possible, lazy loading is a source of problem, and hides some db work. Explicit is better than implicit IMHO! |
@dneimke you should use a separate instance of your context for the seeding. |
@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:
Lazy:
I can't see any difference here, but I see the following improvements
|
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. |
@AndiRudi my opinion about lazy loading is different, here are my concerns
|
@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). |
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. |
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.
|
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 |
👍 +1 Or SomeDbContext.SomeDbSet.Where(xxx).LazyLoad(); |
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. |
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 |
Part of #10042, #10509, #3797 It would be good to make this work in a future release, but this involves running a no-tracking query with fixup where the root entity is already materialized. For now, this will throw so we can make it work later without it being a breaking change. Lazy-loading behaviors for non-tracked entities: * Proxies: * No-op if entity was explicitly detached * Throw if it was queried as no-tracking * Lazy-loading entities with loader service property: * No-op if entity was explicitly detached * Throw if it was queried as no-tracking * Lazy-loading entities without service property: * Throw even if entity was explicitly detached, but entity can set loader to null, or a service property can be defined * Throw if it was queried as no-tracking
Glad to see basic support for lazy loading coming in 2.1. However, is there going to be a story for doing the lazy loading asynchronously? The API may not be as sleek as Are there any other options that could help here? I could also think of making the navigation property accessible as |
@dasMulli We don't intend to add support for async navigation properties at this time, although this is something we have discussed and may revisit in the future. That being said, I will investigate adding async support to the infrastructure such that async loading could be triggered from your entity. |
Add in lazy loading for many to *, and have a good way to handle many to many in controller, not just w linq query in dbcontext file. |
Microsoft.EntityFrameworkCore.Proxies Is already available as nuget or do I have to download it manually from github? |
@Magic73 Based on the fact that a nuget.org search returns no results, I'm going to say you can't get it from NuGet. Additionally, to use EFCore proxies, you need EFCore 2.1, which is not out on nuget. However, EFCore 2.1 and Proxies (2.1) are available on dotnet.myget.org/feed/aspnetcore-dev (proxies) if you want prerelease versions. |
@ajcvickers,
plus
that's how I usually do it, and I think that's a good approach. IMHO. |
As opposite, I think lazy-loading is a good (maybe best) practice for ORMs. Let me explain why; In DDD, Aggregate roots are considered as transaction units. If we want to make a change on an aggregate root, we normally get it as a whole (including child entities) from data source, make the change and save it as a whole (even if the change is made on a child entity). We are sure that all child collections are available. NoSQL databases, like MongoDb perfectly works like that by default. However, for relational databases, we need to make joins to get an entity as a whole. If most of the cases we will not work with the child entities, making joins (to get the whole entity with childs pre-loaded) will be unnecessary and not so natural for relational databases. Lazy loading is the best mimic for truely implementing aggregate roots in relational databases. In this way, when a code works on an aggregate root it does not care if child collections/properties are loaded. They are just loaded on demand by the infrastructure (ORM). I don't want to call
The only problem with lazy loading is when you query a list of entities from database, work on child properties but forgot to include child properties on the query (this leads to 1+N querying problem). This is obviously a programmer mistake. DDD does not care about such querying. When you query a list from database, you should specifically care about performance, create necessary indexes and carefully write your query. (BTW, I adready implemented EnsureCollectionLoadedAsync/EnsurePropertyLoadedAsync with repository/uow pattern for ABP framework. But I don't like it and waiting for lazy loading). |
I agree that there is only problem, but this problem is Huge problem. Programmers do make mistakes. |
I like that you really have to opt-in: https://docs.microsoft.com/en-us/ef/core/querying/related-data#lazy-loading |
I'm glad there is an option to manually wire it up using async calls, though I'd appreciate if there was some "more natural" async way (ideally utilizing |
@alexk8 If you don't like it, then don't use it |
Does this new implementation of Lazy Loading support the retrieval of a children of children?
I am using EF Core 2.1 Lazy Loading. When I retrieve Parent1, Child1 and Child2 are loaded, however, Child1A and Child1B are null and remain null even when directly accessed via Parent1.Child1.Child1A and Parent1.Child1.Child1B. I find this odd, especially since Child1A should just be a reference back to Parent1 (which has already been loaded). |
@nicbavetta Based on the info given it seems this should work. Please file a new issue with a runnable project/solution or complete code listing that demonstrates the issue. |
Does the current (2.1) approach work with non-navigational properties? I have a field in my model which is a large text, and I would like it to be loaded only if I reference it. Can it be done just making the property virtual? What if I called Include().ToList() to avoid n+1 queries? Would that force the loading of the property? Would it even make sense to use Include() with that property? |
Hi, we're injecting Anyway, I'm currently dealing with the following scenario, and I'd love to hear your thoughts on the best solution.
The issue with ignoring
Any thoughts on what the best way to solve this is? I understand the EF Core predicament: I'm assuming that after the context has been disposed, any knowledge of which navigation properties have been loaded is lost. Any assistance/guidance would be appreciated. |
@optiks I have been thinking about something similar to this to make the experience better when using proxies--see #12780. The idea there is to store the IsLoaded state for each navigation as part of the proxy, so that it travels with the entity instance. You could probably do something similar here--for example: public ICollection<Post> Posts
{
get
{
if (!_postsIsLoaded)
{
LazyLoader.Load(this, ref _posts);
_postsIsLoaded = true;
}
return _posts;
}
set => _posts = value;
} The problem is getting the flag set when EF eagerly loads the collection. When I implement #12780 I will try to do so in a way that there is a hook that can be used both by proxies and non-proxies to make this easier. Until then, I can't think of a way to get EF to set the flag for you on an eager load, but maybe it could be done manually in your code after executing the query. |
@ajvickers Thanks. We ended up suppressing the warning as the shortest path forward, but we’ll definitely get back to addressing this soon. I’d imagine, if the loaded state needs to be stored somewhere, on the entity itself is probably the best place. So, if the entity implemented an interface such as From my experience it’s pretty common for entities to inherit from a common base class, so it’d only need to be done once. It’s definitely some extra overhead, but if developers are already dealing with injecting the lazy loader and invoking, it doesn’t seem unreasonable. Just setting the state flag in the property getter won’t be sufficient for the case where EF Core is working directly with fields and eager loading. |
@ajcvickers If we wanted to handle this on our side, what would be the best way of identifying which references/collections were eager loaded? i.e. if we wanted to set a flag representing that a reference/collection was loaded. Using the context.ChangeTracker.Tracked += (s, e) =>
{
var isLoaded = e.Entry.Collections.Single(x => x.Metadata.Name == nameof(Employee.Devices)).IsLoaded; // this is false
};
var q = context.Set<Employee>().Include(a => a.Devices).ToList(); We can intercept the e: After playing around a bit, there's extra complexity to deal with regarding creating new entities/updating collections. I think I'll leave this one for the EF Core team to solve ;). |
Hey all, this is the only thread I have been a part of EF Core so thought of posting here, even if it means "resurrecting" it (not sure if that's possible on a closed issue but still). 😁 I wanted to give a huge shout of appreciation to the team here for all their great work. It really shows. I have been digging into 3.1 now for the first time and set up the identity SDK on Blazor with relatively zero friction. EFCore "feels" really solid and I would put it there around EF6 -- right where I left it! 😆 What made me think of this thread was this nice little tidbit I read here: Looks like the lazy magic is all in there now. Just the fact it was put into its own assembly is the cherry on the cake to me as far as an architectural impression. I'm sure there are others with their own impressions/opinions on this, but for me, this is a really impressive achievement and wanted to share my appreciation. So, congratulations and thank you for your "persistence" (groan 😏) and dedication to this project! It's a great time to develop on the .NET Core platform in part because of your efforts here. 🎉 |
@Mike-EEE Thanks so much for posting this. It really does help boost my morale and I think I can say the same for the rest of the team. 😄 I was pretty happy with the way lazy loading turned out. It's not tightly coupled to proxies, which is important for future plans on re-writing entities at compile time (with Rosyln) as an alternative to proxies. Also, we are successfully using Castle proxies from the community rather than rolling our own implementation, which I think is a great example of working with (as opposed to against or in ignorance of) the existing .NET ecosystem. |
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
The cancel method then needs to cancel each booking on the course so it would do something like this
The booking in turn would refund the transaction
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.
The text was updated successfully, but these errors were encountered: