Enable configuring required 1-to-1 dependents

[AndriySvyryd]

For normal and ownership relationships, see #9005

This would affect:

• Table splitting queries
• Potentially client-side cascade deletes
• Inserts

The API would be on the NavigationBuilder. To be called after the ownership or relationship configuration:

• .Navigation(o => o.Details).IsRequired()

Consider obsoleting .IsRequired() on the relationship
Consider warning if the constraint is not enforceable

[ajcvickers]

Moving this to the backlog for now. Note that adding semantics for a principal with required dependents is a departure for EF Core and can cause a lot of confusion when this cannot consistently be enforced by EF. However, this might not be an issue if it is constrained to within an aggregate (#1985) since the entire aggregate graph must be loaded by definition. So we should consider what this metadata really means and how it is exposed.

[ToddThomson]

I am using an Owned Entity as an aggregate. The aggregate has properties that are required ( using [Required] annotation ). After reading through the many issues which concern migrations not setting the owned property field to ( nullable: false ), I simply cannot understand why this is the case.

What is the reason why properties marked with [Required] in an aggregate are not picking up the (nullable: false) arg?

Is there a workaround other than manually modifying the migration?

There must be something I am missing here...

[AndriySvyryd]

@ToddThomson Currently all dependents, including owned types are optional (e.g. Person.Adress can be null). Therefore when mapped to the same table as the principal entity type all non-shared columns have to be nullable. But the properties on the owned type are still required (e.g. you can't save changes with Person.Adress.City being null)

[ToddThomson]

@AndriySvyryd My gut feel is that the team has got this wrong. However, there must be good reason for the behavior - I will look into it.

What was jarring to me was the fact that EF Core took the decision to make my aggregate class properties nullable when I had expressly marked (some of) them as required.

Also, when trying to react, by setting the aggregate as required there was no change in behavior.

It seems that having a "all dependents are optional" is a bit harsh when there is configuration that could give choice.

Anyway, the manual changes to the migration file are easy so I am not halted!

[ZombieProtectionAgency]

I have been working on a custom TemporalDBContext and I was using owned types to add and configure the required fields as well as a couple other tracking fields that all models will share. Since this change I would have to hack together a solution by tweaking the column operation. which feels really wrong.

Am I misusing owned types?

[ajcvickers]

@AndriySvyryd @roji Anything left to follow up on here? Do docs need updating?

[Davilink]

Is that change will be back ported to EFCore 3.1 ?

[ajcvickers]

@Davilink This was a significant change that is not appropriate for a patch release of 3.1. See the release planning process for more information.

[Davilink]

So we would need to migrate our project to .net 5, just to be able to use EF Core 5 that will fix the bug ?

[ErikEJ]

@Davilink No, EF Core 5 works just fine with .NET Core 3.1 😎

[Davilink]

In the doc https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-5.0/whatsnew#required-11-dependents, it show that we need to add the IsRequired on the prop of the OwnEntity and on the navigation Property, is it required to do that if the project use Nullable reference ?

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Person>(b =>
    {
        b.OwnsOne(e => e.HomeAddress,
            b =>
            {
                b.Property(e => e.Line1).IsRequired(); <----
                b.Property(e => e.City).IsRequired();  <----
                b.Property(e => e.Region).IsRequired();  <----
                b.Property(e => e.Postcode).IsRequired();  <----
            });
        b.Navigation(e => e.HomeAddress).IsRequired();  <----

        b.OwnsOne(e => e.WorkAddress);
    });
}

Is EF Core detect that is the project doesn't use Nullable Reference, we have to add these statement, but if the project do use the Nullable reference, it is optionnal to add these statement ?
See example below (project use nullable reference), none of my properties are nullable, will i have to use the IsRequired() statement (in the fluent api or data annotation) or will EFCore will understand right away.

// ProjectEvent.cs
namespace Test.Entities.Events
{
    public class ProjectEvent : BaseEntity
    {
        public Instant Date { get; set; }

        public virtual Translated Event { get; set; } = default!;

        public Guid ProjectId { get; set; }

        public virtual Project Project { get; set; } = default!;
    }
}

// Translated.cs
namespace Test.Entities
{
    public class Translated
    {
        public string En { get; set; } = default!;

        public string Fr { get; set; } = default!;
    }
}

[AndriySvyryd]

@Davilink If you use non-nullable reference types no additional configuration is necessary.

[Davilink]

So only this is necessary that is what you say:

#region ProjectEvent
modelBuilder.Entity<ProjectEvent>()
    .Property(pe => pe.Date)
    .HasConversion(InstantConverter); // Needed because we use NodaTime

modelBuilder.Entity<ProjectEvent>()
    .OwnsOne(pe => pe.Event);
#endregion

[AndriySvyryd]

@Davilink Yes

[pantonis]

I am still experiencing this issue on EF Core 5.0.2. Owned properties are created as null in the database whereas I define them as .IsRequired()

modelBuilder.Entity<ImageExt>(x =>
{
	x.OwnsOne(p => p.Position, a =>
	{
		a.Property(pp => pp.RowIndex).IsRequired().HasColumnName("RowIndex");
		a.Property(pp => pp.ColumnIndex).IsRequired().HasColumnName("ColumnIndex");
	});
});


public class ImageExt : Entity<long>
{
	public string Name { get; set; }

	public PositionExt Position { get; set; }
}

public class PositionExt  : ValueObject
{
	public int RowIndex { get; private set; }
	public int ColumnIndex { get; private set; }

	protected PositionExt()
	{
	}

	private PositionExt(int rowIndex, int columnIndex)
	{
		RowIndex = rowIndex;
		ColumnIndex = columnIndex;
	}


	public static Result<PositionExt> Create(int rowIndex, int columnIndex)
	{
		if (rowIndex < 1)
			return Result.Failure<PositionExt>("Row number must be greater than 0.");

		if (columnIndex < 1)
			return Result.Failure<PositionExt>("Column number must be greater than 0.");

		return Result.Success(new PositionExt(rowIndex, columnIndex));
	}

	protected override IEnumerable<object> GetEqualityComponents()
	{
		yield return RowIndex;
		yield return ColumnIndex;
	}
}

[AndriySvyryd]

@pantonis You need to configure the navigation as required after x.OwnsOne(p => p.Position,...:

x.Navigation(p => p.Position).IsRequired();

[pantonis]

@AndriySvyryd by adding navigation it would create a new table right? What I need is to keep RowIndex and ColumnIndex in the same table ImageExt

[AndriySvyryd]

@pantonis No, to map it to a different table you'd need to call ToTable:

	x.OwnsOne(p => p.Position, a =>
	{
		a.ToTable("DifferentTable");
	});

[pantonis]

Then why do we need Navigation for the same table? btw I tried to add it and it complains for a foreign key to that navigation property

…

On Thu, 4 Feb 2021, 20:50 Andriy Svyryd, ***@***.***> wrote: @pantonis <https://github.com/pantonis> No, to map it to a different table you'd need to call ToTable: x.OwnsOne(p => p.Position, a => { a.ToTable("DifferentTable"); }); — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12100 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZEO3WQVUMJTHKU3N2UNGDS5LT7LANCNFSM4FBAGKRQ> .

[AndriySvyryd]

Then why do we need Navigation for the same table? btw I tried to add it
and it complains for a foreign key to that navigation property

You need to call it after x.OwnsOne(p => p.Position,... to mark the navigation to the owned type as required, meaning that it should always be populated with an instance.

[pantonis]

Thanks that did the trick. Consider updating the first comment to include this.

[arielmoraes]

We upgraded from 3.1 to 5 and the behavior has changed yet again, we have an example using nested Owned Types as follows:

public class A 
{
   public B ValueObject { get; set; }
}

public class B
{
   public C AnotherValueObject { get; set; }

   public decimal? NullableProp { get; set; }
}

public class C
{
   public decimal Number { get; set }
}

B is an Owned Type of A
C is an Owned Type of B

if NullableProp of B is null and the Number of C is not then ValueObject of A will be null.

I've read all the breaking changes from 3.1 to 5 but couldn't find anything related to that.

Am I missing something?

EDIT

Forgot to mention, for that example we had Lazy Loading enabled.

[ajcvickers]

@arielmoraes You'll need to mark the navigation as required, as shown in #12100 (comment) above.

For a more in-depth discussion see https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-6.0/whatsnew#changes-to-owned-optional-dependent-handling.

[arielmoraes]

@ajcvickers indeed that fixes the problem and before commenting here we used that as a workaround, but was that a breaking change of EF 5 or a bug?

[ajcvickers]

@arielmoraes I don't remember off the top of my head which changes were made in which release.

[AndriySvyryd]

Might be an unintended breaking change, it was only working in 3.1 as an accident, this scenario wasn't supported.

[arielmoraes]

Usually, my code breaks by accident, I'd love if it worked instead hahaha.

[jon-peel]

@pantonis You need to configure the navigation as required after x.OwnsOne(p => p.Position,...:

x.Navigation(p => p.Position).IsRequired();

Thank you, this should really be in the documentation at https://learn.microsoft.com/en-us/ef/core/modeling/owned-entities

[AndriySvyryd]

Thank you, this should really be in the documentation at learn.microsoft.com/en-us/ef/core/modeling/owned-entities

There's currently a highlighted paragraph that says:

The owned entity type can be marked as required, see Required one-to-one dependents for more information.

[jon-peel]

It is there... thank you... That one is on me.