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

Initializing reference navigation in constructor causes incorrect fixup #18007

Open
RickStrahl opened this issue Sep 24, 2019 · 28 comments

Comments

@RickStrahl
Copy link

commented Sep 24, 2019

I have a very simple sample application (my AlbumViewer Sample. It has Albums and Artists (and also Tracks).

In upgrading to the .NET Core 3.0 RTM bits today I noticed that the DB was no longer returning the related data properly.

Specifically I'm querying a list of Albums, which should return Artist records as part of the query results. But there's a problem with the Artist data with only the first resulting album getting the related Artist data - all subsequent Albums with the same entity get a null reference for the Artist:

image

The model is very simple:

    [DebuggerDisplay("{Title} {Artist.ArtistName}")]
    public class Album
    {           
        public int Id { get; set; }
        public int ArtistId { get; set; }        
        public string Title { get; set; }
        public string Description { get; set; }
        public int Year { get; set; }
        public string ImageUrl { get; set; }
        public string AmazonUrl { get; set; }
        public string SpotifyUrl { get; set; }

        public virtual Artist Artist { get; set; }
        public virtual IList<Track> Tracks { get; set; }

        public Album()
        {
            Artist = new Artist();
            Tracks = new List<Track>();
        }
    }

and this is a basic 1-1 relation ship.

The query runs:

 public async Task<List<Album>> GetAllAlbums(int page = 0, int pageSize = 15)
        {
            IQueryable<Album> albums = Context.Albums
                .Where(alb=> alb.ArtistId == 8)   // for debug only retrieve one artist set of records
                .Include(ctx => ctx.Tracks)
                .Include(ctx => ctx.Artist)
                .OrderBy(alb => alb.Title);

            if (page > 0)
            {
                albums = albums
                                .Skip((page - 1) * pageSize)
                                .Take(pageSize);
            }

            var list = await albums.ToListAsync();
            return list;
        }

In this query 3 records are returned each of which have the same artist. The first record correctly holds the the Artist data. Subsequent matches have the .Artist=null.

Note it appears that this affects any Album records where there are multiple albums per artist. The first match always has the Artist filled, but any records later in the resultset have the Artist empty.

EF Core version:
Database provider: 3.0 RTM
Target framework: .NET Core 3.0 RTM
Operating system: Windows 10
IDE: VS 2019 16.3

@RickStrahl RickStrahl added the type-bug label Sep 24, 2019
@RickStrahl

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

Some additional points about the results in this query:

To demonstrate more clearly I added another subquery to eek out all the entries that have an empty album, which results in 43 of 90 - basically all albums that have more than one album for a single artist. First one has Artist, subsequent ones do not.

image

There should never be any entries in that second list as there are not empty artists and no orphaned artists in the db.

So just to be sure there's not something wrong with my own assumptions I removed the EF Core 3.0 assemblies and rolled them back to the EF Core 2.2, then re-ran the same application.

image

As you can see 2.2 correctly returns the proper results with no empty Artist objects.

@divega

This comment has been minimized.

Copy link

commented Sep 24, 2019

@smitpatel, can you please do the initial investigation?

@smitpatel smitpatel self-assigned this Sep 24, 2019
@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

I am not able to reproduce this error
Repro code used

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

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

                // Seed database
                var artist = new Artist();
                db.Add(artist);

                db.AddRange(
                    new Album
                    {
                        Artist = artist,
                        Title = "Album1",
                        Tracks =
                        {
                            new Track(),
                            new Track(),
                            new Track(),
                        }
                    },
                    new Album
                    {
                        Artist = artist,
                        Title = "Album2",
                        Tracks =
                        {
                            new Track(),
                            new Track(),
                            new Track(),
                        }
                    },
                    new Album
                    {
                        Artist = artist,
                        Title = "Album3",
                        Tracks =
                        {
                            new Track(),
                            new Track(),
                            new Track(),
                        }
                    }
                    );

                db.SaveChanges();
            }

            using (var db = new MyContext())
            {
                // Run queries
                var result = db.Albums.Where(alb => alb.ArtistId == 1)
                    .Include(ctx => ctx.Tracks)
                    .Include(ctx => ctx.Artist)
                    .OrderBy(alb => alb.Artist)
                    .ToList();

                Console.WriteLine(result.Where(a => a.Artist == null).Count());
            }
            Console.WriteLine("Program finished.");
        }
    }


    public class MyContext : DbContext
    {
        private static ILoggerFactory ContextLoggerFactory
            => LoggerFactory.Create(b =>
            {
                b
                .AddConsole()
                .AddFilter("", LogLevel.Debug);
            });

        // Declare DBSets
        public DbSet<Album> Albums { 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")
                //.UseCosmos("https://localhost:8081", @"C2y6yDjf5/R+ob0N8A7Cgv30VRDJIWEHLM+4QDU5DE2nQ9nDuVTqobD4b8mGGyPMbIZnqyMsEcaGQy67XIw/Jw==", "_ModelApp")
                .EnableSensitiveDataLogging()
                .UseLoggerFactory(ContextLoggerFactory);
        }

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

    public class Album
    {
        public int Id { get; set; }
        public int ArtistId { get; set; }
        public string Title { get; set; }
        public string Description { get; set; }
        public int Year { get; set; }
        public string ImageUrl { get; set; }
        public string AmazonUrl { get; set; }
        public string SpotifyUrl { get; set; }

        public virtual Artist Artist { get; set; }
        public virtual IList<Track> Tracks { get; set; }

        public Album()
        {
            Artist = new Artist();
            Tracks = new List<Track>();
        }
    }

    public class Artist
    {
        public int Id { get; set; }
    }
    public class Track
    {
        public int Id { get; set; }
        public int AlbumId { get; set; }
    }
}
@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

Further, the other 2 screenshots in 2nd comment should throw Null Ref is artist is not loaded when comparing album.Artist.Id == 0.

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

True but there are no orphaned entities so Artist should never be null - that code is there to demonstrate the issue - it's not in the actual code. Also I'm not sure that it should through since the constructor is in fact creating the instance.

Again note that this works fine in 2.2.

I'll take a look later if I can simplify this down to a simpler repo but my suggestion is maybe look at the repo and run the ASP.NET application and hit /api/albums then look at the data (or step in). The data autoloads in SqLite from a JSON file so you should be able to see the same thing I'm running here.

I don't doubt that this may not fail in a no data or memory db driver scenario, but with either SQL Server or SQLite in the above scenario it definitely fails.

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

I used SqlServer as database & with adding seed data as evident in repro code above.

@rellis-of-rhindleton

This comment has been minimized.

Copy link

commented Sep 25, 2019

Forked the AlbumViewer repo to see if I could spot anything. Checked out the EF 3.0 commit. Using the SqlLite option, I'm seeing similar behavior. For artist id 12 (Accept), I'm getting three albums. The first has everything right; the second has an instance of Artist with none of its properties set.

locals

Additionally - in GetAlbumsForArtist, although the query includes Artist, none of the returned albums have the right data. Again it looks like they all have a new instance of Artist with default property values.

With this query code, breakpoint on the return statement:

public async Task<List<Album>> GetAlbumsForArtist(int artistId)
{
	var albums = await Context.Albums
		.Include(a => a.Tracks)
		.Include(a => a.Artist)
		.Where(a => a.ArtistId == artistId)
		.ToListAsync();

	return albums;
}

This is the value of albums:

locals2

I don't have any deeper insights here, just figured I'd see if I could reproduce the problem.

(Thanks for the list @RickStrahl ... now I have some new albums to listen to. 🥇 )

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Sep 25, 2019

@rellis-of-rhindleton Thanks for confirming that it's not a machine specific thing.

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Sep 25, 2019

@smitpatel - I've created a simpler test project that you can test with here:

https://github.com/RickStrahl-Content/EfCore30QueryBug

This gives you the data set I'm working with which is un-interesting but consistently demonstrates the failing behavior. Give that a shot and let me know what you find.

@alexwiese

This comment has been minimized.

Copy link

commented Sep 25, 2019

@RickStrahl @smitpatel
I've run Rick's repro on my machine and it fails with both SQL and SQLLite.

X EfCoreQueryWithSqLiteDataBug [687ms]
  Error Message:
   Assert.IsTrue failed. There are 38 uninitialized Artist records
  Stack Trace:
     at EFCoreQueryBug.EfCoreQueryBug.EfCoreQueryWithSqLiteDataBug() in C:\git-other\EfCore30QueryBug\EfCoreQueryBug.cs:line 33


  X EfCoreQueryWithSqlDataBug [256ms]
  Error Message:
   Assert.IsTrue failed. There are 38 uninitialized Artist records
  Stack Trace:
     at EFCoreQueryBug.EfCoreQueryBug.EfCoreQueryWithSqlDataBug() in C:\git-other\EfCore30QueryBug\EfCoreQueryBug.cs:line 54

Interestingly if I comment out the line that sets the Artist property in the Album constructor it works fine.
This leads me to believe EF is getting tripped up by the presence of the "default" Artist somehow.

        public Album()
        {
           // Artist = new Artist();
            Tracks = new List<Track>();
        }

Further to this setting the ID of the "default" Artist to a different value such as -1 or 1 it also works (except the results are wrong, it seems the first instance has the Artist set correctly, but each subsequent instance having that ID will not be set correctly).

Artist = new Artist { Id = 1 };

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

There is possibly issue in seeding. Did you check data on server side?

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

Or fixup is not running properly since Album is not null.

@alexwiese

This comment has been minimized.

Copy link

commented Sep 25, 2019

@smitpatel
The data is fine, if I comment out that line that sets the "default" Artist then the data loads correctly.
There is not issue with the data in the database.

Ie. this works fine

        {
           // Artist = new Artist();
            Tracks = new List<Track>();
        }

This causes the issue where the non-first Album having that ArtistId does not have the Artist set

        public Album()
        {
            Artist = new Artist();
            Tracks = new List<Track>();
        }

Should be easier to narrow down the issue by looking at the behaviour with that line commented/uncommented.

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

My theory is statemanager is not working correctly.
Step by step

  • Album is materialized and state manager starts tracking it.
  • Artist is materialized and state manager starts tracking it. This also causes fix up with initial album.
  • Album 2 is materialized and state manager starts tracking it. Since artist is not null & Artist Id is already tracking, it probably assumed that Artist got replaced (navigation changed which would propagate to FK property).

In EF Core 2.2, query also did fix-up but in 3.0 we decided to make it centralized and let state manager always do the fix-up so behavior could arise.

@rellis-of-rhindleton

This comment has been minimized.

Copy link

commented Sep 25, 2019

Is behavior defined somewhere for what to do if a newly materialized entity already has a nav prop set? It seems like a different situation than, say, a default collection property. Not suggesting it should change from 2.x, just looking for insight.

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Sep 25, 2019

@smitpatel So are you saying this is by design and as Alex suggests the Artist child should be left at null?

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

@RickStrahl - It seems that something is happening in state manager rather than query pipeline itself. Now that we know what is the root cause here (initializing reference navigation), I can debug this in EF Core codebase to see what state manager does (as noted above, still just a theory).

Though there are some obvious issue with initializing reference navigation and it could be very well by design that you cannot initialize reference navigation

  • Adding Album would add default artist, rather than throwing that related artist does not exist.
  • Forgetting to add include would never throw, rather give incorrect result.
  • Lazy loading may not work since the navigation is already non-null

Is there a particular reason for initializing reference navigation? That is certainly something which we don't recommend.

@rellis-of-rhindleton

This comment has been minimized.

Copy link

commented Sep 25, 2019

From a domain design standpoint, it seems like it'd be more intuitive to either leave the property null or require a non-null instance in the constructor parameters.

But without knowing what the framework expects it gets left to personal tastes. There isn't much guidance on this as far as I can find. If it's not recommended then maybe a page should be added to the docs about best practices for constructors/initialization, and Entity Framework's expectations? Materialization of entities is kind of a black box in EF, unless I'm missing something recent.

@jonsagara

This comment has been minimized.

Copy link

commented Sep 25, 2019

I always write my entity collections to be initialized so that I never have to worry about the collection being null:

public List<Thing> Things { get; private set; } = new List<Thing>();
@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

To keep the discussion in correct direction, collection navigations which are initialized are still supported and works correctly. Collection navigations can get away without having a setter too.

For reference navigation, you require a setter property.
I would agree with @rellis-of-rhindleton on the fact that if you are initializing something in ctor, then it is more intuitive for it to be either some collection initialization or initialize based on parameters of ctor.
We would be looking at some docs. We don't have so far because this is first time, we saw that it started causing issue. I believe it stems from the fact that state manager always had this behavior but query itself was doing identity resolution which was hiding it. And query behavior turned out to be different (but worked for the case). In past, Inserting just Album still adds empty Artist to database.

From customer perspective, at least as a work-around, don't initialize reference navigations to some value in constructor which would be used by EF.

We will discuss in team, what is the most useful user experience here. According to discussion, we would add documentation on recommendation or update the state manager to work in this case.

@RickStrahl

This comment has been minimized.

Copy link
Author

commented Sep 25, 2019

The more I think about I see that the object initialization shouldn't be necessary, but it is more consistent since you can initialize a collection. When you're talking about object initialization in the terms you do above you are talking about an implementation detail in the framework that is interfering with normal object behavior you would expect.

The use case used to be that you can create a new Album and have an artist that you can start punching values into. EF 6 was smart enough to figure out that the Artist was a new Artist and automatically added it. From a domain design perspective I never want to see a null instance on an object. If I new up a new Album then by default the Artist is an unassigned empty object which may later be filled with data assumed to be a new record. If the entity is loaded I would expect it to get overwritten with the value from the Db.

From my domain perspective I never want Artist to ever be null.

The old EF6 behavior is the behavior I would like to see actually, but that hasn't ever worked in EF Core where you had to explicitly add anyway.

Again for the most part I think this is a consistency issue and if I understand the problem correctly it shouldn't be expensive to check whether the attached property is tracked or not. After all this worked in in 2.2 since that returns the correct results so at the very least this is a breaking change.

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

Reference navigations which does not have setters are not mapped by default whereas collection navigations without setters are mapped. There is already inconsistency between how reference vs collection navigations are treated. So consistency argument is very weak.
Setting the Artist to null is still possible because you have a setter available. Moreover, a collection initialized still adds whole entity to collection. Whereas reference is assigned the object rather than individual property. Initializing artist and punching values in is not ORM would assign reference navigation (to preserve semantics of materialization of Artist itself.

It's an unexpected breaking change for us because we did not expect or see user initialize reference navigation. We look into either reverting this breaking change or document it.

@rellis-of-rhindleton

This comment has been minimized.

Copy link

commented Sep 25, 2019

@smitpatel - for my own understanding, and to make sure we are hearing you right, you're saying that EF ideally deals in whole entities; getting into pieces (properties) of entities is not something the team wants to mess with...?

However you handle the breaking change, it'd be great if this process ends with something written up that explains what the framework expects and how it handles materialization.

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

That is wrong interpretation of what I said. If you want EF to assign a reference navigation or add something to collection navigation as part of Include, EF will materialize entity instance from server side using materializer and will assign (or add) entity instance. Of course you are free to do whatever if you assign yourself. EF does not use pattern where it assigns album.Artist.ArtistName = "something", when it is asked to Include Artist on album instance it materialized. If you manually write that in your query, we will do that but EF is not going to be tracking that Album or Artist.

@divega divega assigned ajcvickers and unassigned smitpatel Sep 26, 2019
@divega divega added this to the 3.1.0 milestone Sep 26, 2019
@divega

This comment has been minimized.

Copy link

commented Sep 26, 2019

EF Triage: The change in behavior affected a scenario for which we haven't intentionally designed, in which one of the model classes initializes one of its reference navigation properties to a new object in its constructor.

We haven't seen this pattern commonly applied before and we don't expect it to become much more common. Although the ability to specify reference navigation properties as non-nullable may provide incentive to set them to a new object, at least we already have recommendations for dealing with this in the documentation: https://docs.microsoft.com/ef/core/miscellaneous/nullable-reference-types#non-nullable-properties-and-initialization.

We aren't 100% in agreement re the pattern being something we should explicitly discourage in our documentation, or whether it actually has practical applications.

However, we tend to agree that we should come up with a design that contemplates the pattern and once we decide what the behavior should be, add the corresponding tests and documentation.

Since we don't expect the impact to be as significant as suggested in the original report, we are not planning to rush a fix into a 3.0 patch, and we'd rather have @ajcvickers take a look at it from the holistic perspective of the change tracker behavior.

We are assigning the issue to him to investigate in 3.1 and come up with a recommendation.

@smitpatel smitpatel changed the title Related Child Records not properly populating even with Include() Initializing reference navigation in constructor causes incorrect fixup Sep 27, 2019
@jinweijie

This comment has been minimized.

Copy link

commented Sep 30, 2019

Hello,

just for your information, the Entity Developer's template to generate the POCO entities is written with constructor instantiation with children tables. So it will break a lot applications after upgrade to ef core 3.0. For me, it's not a problem which can be solved with just documentation.

Thanks.

@ajcvickers

This comment has been minimized.

Copy link
Member

commented Oct 12, 2019

I have now had time to investigate this more completely. First, EF Core was never intentionally designed to work this way--the behavior in 2.2 was caused by a bug whereby an existing entity instance was replaced by a new instance, with the first instance simply discarded. Note that this is different from populating an instance that was already created.

Second, the same code running against EF6 does not have the same behavior as EF Core 2.2. EF6 instead behaves more like EF Core 3.0--that is, the existing instances are retained and not populated. Any data in the database is discarded. So the pattern of pre-initializing reference navigation properties did not work in EF6.

Third, I don't really buy that an uninitialized entity instance is an improvement for a domain model than a null reference. In fact, I would say it is worse. A null reference is clearly an unambiguously not initialized. That being said, populating an instance that already exists could be useful if the navigation property is explicitly configured to indicate that it has been set but the entity it references is known to be uninitialized. However, the value here seems quite low.

So my conclusions are:

  • The 2.2 behavior is wrong and was caused by a bug. Discarding entity instances and replacing them with new instances is not correct.
  • EF6 behavior agrees with this
  • The 3.0 behavior is better than the 2.2 behavior (and closer to the EF6 behavior) but there is some data dependency on the behavior that should be investigated.
  • A better behavior may be to explicitly throw in a situation where already existing instance has a different key value to the one coming from the database, although this is probably not without perf impact
  • An way to configure the requested behavior may be worthwhile, but seems a long way off given the resources available and the extent of the existing backlog.

We will re-discuss in triage.

@ajcvickers ajcvickers removed this from the 3.1.0 milestone Oct 12, 2019
@RickStrahl

This comment has been minimized.

Copy link
Author

commented Oct 13, 2019

Fair enough I guess. I agree wit the point that it probably should throw or somehow indicate that binding was not performed.

Closing since this is obviously beating a dead horse.

I will say this in closing though:

I don't get why all the fuss about this being NOT supported. It should be trivial to tell if an instance exist and is an entity ref or a plain POCO object. If you're running a query or otherwise populating that instance, it gets overwritten as would be expected. Likewise on an update it wouldn't get updated because it's not been added. Maybe there's some intricacy I don't see here, but this seems a no-brainer to not have this fail in the way described above, regardless of what you THINK is YOUR preference for doing your domain modeling.

But... whatever, now that I know, I (probably) won't make this mistake again, but I'm sure I won't be the last person that's running into this.

@RickStrahl RickStrahl closed this Oct 13, 2019
@ajcvickers ajcvickers reopened this Oct 13, 2019
@ajcvickers ajcvickers added this to the Backlog milestone Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.