efcore: Initializing reference navigation in constructor causes incorrect fixup

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.

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.

I’m a colleague of the person who wrote #19892, considered a duplicate of this issue. I’ve tried to carefully read through this issue, and this part of an earlier comment caught my attention:

• The 2.2 behavior is wrong and was caused by a bug.
• The 3.0 behavior is better than the 2.2 behavior
• A way to configure the requested behavior may be worthwhile, but seems a long way off given the resources available

I appreciate that sentiment. But consider our perspective, where #19892 only contains a minimal repro, our actual application is of course much larger. Our domain led us to heavy use of this pattern:

public class HumanBody {
  public Head Head { get; set; }
  public Torso Torso { get; set; }

  // This might be tricky as an "owned" entity. In the DDD-sense it might be
  // but to keep it in the same table in e.g. Sql Server it might require some
  // serialize-as-json convertor.... (with its own problems)
  public IList<Limb> Limbs { get; set; }

  public HumanBody() {
    Head = new Head();
    Torso = new Torso();
    Limbs = new List<Limb>();
  }
}

I chose this generally-understandable example domain of a human body, because it’s analogous (code wise) to our application in that:

• It makes sense to encapsulate things like info on the “Head” in classes
• It makes sense that these properties are mapped as Owned entities
• It makes sense to have a convention to always have those properties set, via the constructor, so that programmers in our code base can always be sure the navigation properties are non-null
• To a degree, it could make sense that an app developer expects owned properties to be properly saved and change tracked when the aggregate root is mutated/saved

Now, with new C# features, marking the properties as non-nullable might be feasible, but we’re early adopters of .NET Core here (since 1.x), so getting there might be a ways off.

From an application developer’s point of view, the 2.2 behavior made sense. I don’t think it’s unreasonable that we felt our pattern as above was working properly, instead of “by accident because of a bug in 2.2 that we were abusing”.

Now imagine the above pattern in a decently sized application. While the EF Core team considers the 2.2 behavior a bug, for our use case it was a feature. Either way, this breaking change in behavior is disastrous, blocking us from migrating to 3.x

---

Again, I appreciate the point of view from the EF Core team (and appreciate the hard work for such a fine library!), but it does leave us with a problem.

I really hope others with the same issue see my comment, and express whether they 👍 or 👎 . And I hope that could possibly be a good motivation for a feature toggle for this “2.2 behavior”, or failing that perhaps some explicit documentation of how to solve the underlying issue I tried to describe with above pattern.

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.

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 @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 };

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.

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.

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.

@jeroenheijmans I think you are making very valid points. To me, the distinction comes down to how the child object fits into the domain/data model.

If we consider simple properties of intrinsic types, then it’s clear they should behave as you are suggesting. For example, consider this:

public class HumanBody
{
  public int Id { get; set; }
  public string Name { get; set; }

  public HumanBody()
  {
    Name = "John Doe";
  }
}

If I query for this, then EF will first construct the object, and then set the Name property to the value from the database*. So “John Doe” really is just a default.

Now, let’s say that I have something a bit more complex than an intrinsic type, but still something that in DDD terminology should be treated as value object:

public class PersonalInfo
{
  public string FirstName { get; set; }
  public string LastName { get; set; }
  public long SSN { get; set; }
}

public class HumanBody
{
  public int Id { get; set; }
  public PersonalInfo Info { get; set; }

  public HumanBody()
  {
    PersonalInfo = new PersonalInfo
    {
      FirstName = "John",
      LastName = "Doe",
      SSN = -1;
    }
  }
}

In this case I think it should still work like the intrinsic type. That is, the value set in the constructor is a default only, and it should be replaced with whatever was in the database when materializing a query. (I’ll come back to what this means in EF shortly.)

But now let’s say I have something which is itself an entity type with identity. (That is, it’s specifically not a value object.) For example:

public class Hat
{
  public int HatId { get; set; }
  public string Color { get; set; }
  public string Style { get; set; }
}

public class HumanBody
{
  public int Id { get; set; }
  public Hat Info { get; set; }

  public HumanBody()
  {
    Hat = new Hat
    {
      Id = 0,
      Style = "Fedora"
    }
  }
}

Consider that in the general case, an entity like Hat can be re-parented from one human body to another. It might also be able to exist without any parent at all. It has its own identity.

So this could mean two things:

• The Hat set in the constructor is just a default and should be replaced, like with the other cases above
• Or, the Hat represents an actual Hat entity with its own identity that is referenced by the parent entity.

The intention in EF Core (as in EF6) was to treat it as the latter. That is, if you have an instance of an entity type then it is treated as an entity with its own identity, not a value object. (The bug meant we weren’t doing this correctly.) Otherwise, what if this Hat wasn’t set in the constructor and isn’t a default? Does this mean if the identity is different from what is in the database? What about other property values? And as you get into what to do with graphs it gets more ambiguous.

So what does this mean for EF Core? Fundamentally, it means using entities in place of value objects is a leaky abstraction that doesn’t work too well when you get down to the details. This in turn means that owned entity types (which are still entity types even though the key can be in shadow state) should not be used when the semantics of value objects are needed.

Instead, value objects should be mapped as normal properties with value converters to transform to and from the database. This works well when the value object can be converted or serialized into a single column, but we are missing the feature to split this over multiple columns: #13947

Anyway, I hope this explains a bit about the way we’re thinking about these scenarios. It’s certainly not a cut-and-dry decision. I hope and expect we will continue to learn in this area.

---

Footnotes

* Note that EF can also use the constructor for cases like this. For example, given this:

public class HumanBody
{
  public int Id { get; set; }
  public string Name { get; set; }

  public HumanBody(string name)
  {
    Name ??= "John Doe";
  }
}

EF will construct the object by calling the constructor and passing in the name from the database. So in this case there is no second step where EF calls the property setter.

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.

@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.

@smitpatel, can you please do the initial investigation?

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.

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.

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