Enable configuring required 1-to-1 dependents
AndriySvyryd opened this issue · 49 comments
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
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.
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...
@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)
@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!
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?
If the dependent is mapped to same table in table splitting and does not have any column other than PKs (which are shared), make sure dependent is required else throw.
So what is the status of this ticket? Is it going to be fixed/changed? I expect the marked owned entity property as IsRequired() to be nullable: false in the generated EFCore migration.
@XardasLord It's currently in the 5.0 plan. However, the fixes to the optional owned entities queries didn't explicitly require this after all, so it's not clear that the priority is as high as it was. Is there something that doesn't work with the nullable columns, or is it only the database schema that isn't as you want it?
@ajcvickers It is only about the generated database schema.
I understand the complexity of the issue, however in my opinion it is quite confusing that its ok to add IsRequired() to a property in the configuration, but the generated migration won't include this - so the column will be nullable.
If someone who uses this, doesnt bother with checking the migration then it will create a wrong database schema, because many cases a nullable column could crate unexpected behaviours...
so in my case it has quite high priority :)
Is there something that doesn't work with the nullable columns
I had commented further up, I am using owned types for auditing fields. Period columns cannot be nullable. Without non-nullable owned type properties I will have to copy + paste my 3 audit columns (and their 3 attributes each) to all 30 of my models and all future models.
Did I understand correctly that required owned types will be available in 5.0.0-rc1?
Hi there,
I have been testing with EF Core 5 RC1 and there's one problem with this update.
I have the following class where you can see by comment which properties now map to nullable DB columns with EF Core 5 RC1:
[Owned]
public class ReadWriteProperty<TData>
{
// Constructors omitted.
public TData Data { get; } // Maps to non-nullable DB column even when TData is nullable (e.g. string?); not great.
public DateTimeOffset Modified { get; } // Maps to non-nullable DB column; great
public string? ModifiedBy { get; } // Maps to nullable DB column; great
}The mapping of generic property Data to a non-nullable DB column is I believe incorrect. A recent discussion with @roji here concluded the nullable status of generic type parameters is indeterminate so shouldn't a property of generic parameter type map to a nullable DB column by default? Then we can override this with IsRequired in the Fluent if necessary?
Currently all my ReadWriteProperty<string?> Data properties are mapping to non-nullable strings which is actually a problem as some of these represent optional foreign keys so "" is not a workable substitute for NULL....
Many thanks for the update and hopefully this can be fixed.
/cc @roji @AndriySvyryd This is potentially another NRT issue in model building.
Just compiling the above I get
error CS8618: Non-nullable property 'Data' must contain a non-null value when exiting constructor. Consider declaring the property as nullable.
It seems that the compiler assumes TData is non-nullable
Just to add, as these are { get; } properties I'm specifying them like so:
builder.OwnsOne(
navigationExpression: p => p.MyProperty,
buildAction: od =>
{
od.Property(e => e.Data);
od.Property(e => e.Modified);
od.Property(e => e.ModifiedBy);
});
builder.Navigation(p => p.MyProperty).IsRequired(); // added after reading RC1 blog post but can be omitted.I also noticed as an experiment (it's not a setting I want) .Metadata.SetAfterSaveBehavior(PropertySaveBehavior.Throw) doesn't seem to work on these properties so maybe there is something about the model building.....
Just compiling the above I get
@AndriySvyryd , I omitted the constructors from my extract. Here is the full class:
[Owned]
public class ReadWriteProperty<TData>
{
/// <summary>
/// Constructor intended for use by EF Core only.
/// </summary>
/// <param name="data"></param>
/// <param name="modified"></param>
/// <param name="modifiedBy"></param>
private protected ReadWriteProperty(TData data, DateTimeOffset modified, string? modifiedBy)
{
Data = data;
Modified = modified;
ModifiedBy = modifiedBy;
}
public ReadWriteProperty(TData data, ITimeProvider timeProvider, string? modifiedBy)
{
Data = data;
Modified = timeProvider.GetUtcNow();
ModifiedBy = modifiedBy;
}
public TData Data { get; }
public DateTimeOffset Modified { get; }
public string? ModifiedBy { get; }
}@markm77 My point was that the compiler marks TData as non-nullable even prior to it being assigned a concrete type.
@AndriySvyryd I don't believe generic parameters are assumed non-nullable by default without e.g. where TData : non-null. Did you add where TData : class? Note I am using C# 8 and nulllable. I am happily using ReadWriteProperty<string?> etc.
@markm77 Make Data nullable to change the default nullability.
[Owned]
public class ReadWriteProperty<TData>
{
// Constructors omitted.
public TData? Data { get; set; } // Maps to non-nullable DB column even when TData is nullable (e.g. string?); not great.
public DateTimeOffset Modified { get; set; } // Maps to non-nullable DB column; great
public string? ModifiedBy { get; set; } // Maps to nullable DB column; great
}@AndriySvyryd First thanks for taking the time to respond to this, appreciated!
I've reproduced the compiler warning you got with a parameterless constructor (CS8618). I realise this warning refers to a problem with non-nullable Data but believe this is because at this point the compiler must assume TData could be either nullable or non-nullable and there is a problem with this constructor in the non-nullable case. This warning therefore must be generated.
For the same reason, the solution you suggested (replacing TData with TData?) without further changes generates CS8627 because TData might be nullable.
It is true that I probably could create two generic classes as a workaround (one for nullable TData, the other for non-nullable TData) but the whole point of this class was to handle all cases. It currently supports for example both
public ReadWriteProperty<bool> IsDeleted { get; set; } = null!;and
public ReadWriteProperty<string?> MyForeignKey { get; set; } = null!;which is very useful.
In any case, I think EF Core should probably default to assuming nullable where nullability is not known (as I believe it isn't here). That allows for nullability to be fixed if necessary with IsRequired in the Fluent. Or alternatively perhaps you could introduce IsNotRequired into the Fluent to allow a situation like this to be handled?
Many thanks,
Mark
In any case, I think EF Core should probably default to assuming nullable where nullability is not known (as I believe it isn't here). That allows for nullability to be fixed if necessary with IsRequired in the Fluent. Or alternatively perhaps you could introduce IsNotRequired into the Fluent to allow a situation like this to be handled?
The compiler assumes that it is non-nullable, so that's what we follow. You can call IsRequired(false) for a particular property.
The compiler assumes that it is non-nullable
With the greatest of respect, I believe this is wrong which is demonstrable by replacing TData with TData? and observing the error: [CS8627] A nullable type parameter must be known to be a value type or non-nullable reference type. Consider adding a 'class', 'struct', or type constraint.
You can call IsRequired(false) for a particular property.
Thanks so much (!) for pointing this out; gave me a way to deal with this and fix a lot of issues I was having including with my unit tests.
I simply wanted a solution or workaround so am satisfied now; have a great day!
@AndriySvyryd @roji Anything left to follow up on here? Do docs need updating?
Is that change will be back ported to EFCore 3.1 ?
@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.
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 ?
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!;
}
}@Davilink If you use non-nullable reference types no additional configuration is necessary.
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@Davilink Yes
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;
}
}
@pantonis You need to configure the navigation as required after x.OwnsOne(p => p.Position,...:
x.Navigation(p => p.Position).IsRequired();@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
@pantonis No, to map it to a different table you'd need to call ToTable:
x.OwnsOne(p => p.Position, a =>
{
a.ToTable("DifferentTable");
});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.
Thanks that did the trick. Consider updating the first comment to include this.
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.
@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.
@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?
@arielmoraes I don't remember off the top of my head which changes were made in which release.
Might be an unintended breaking change, it was only working in 3.1 as an accident, this scenario wasn't supported.
Usually, my code breaks by accident, I'd love if it worked instead hahaha.
@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
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.
It is there... thank you... That one is on me.