From 8e166b6d8bb4642445d5eada34acdae525047783 Mon Sep 17 00:00:00 2001 From: maumar Date: Fri, 9 Feb 2024 22:41:21 -0800 Subject: [PATCH] Fix to #33004 - Unfulfillable nullable contraints are set for complex types with TPH entities When figuring out nullability of columns representing a given property, if property is declared on derived entity in TPH, we make that column nullable. For complex type properties we should be doing the same (and we did), but declaring type of that property is the complex type itself. Instead we should look at the ContainingEntityType rather than just DeclaringType. Fixes #33004 --- .../RelationalPropertyExtensions.cs | 2 +- .../Migrations/MigrationsTestBase.cs | 68 +++++++++++++++++++ .../Migrations/MigrationsSqlServerTest.cs | 20 ++++++ .../Migrations/MigrationsSqliteTest.cs | 18 +++++ 4 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs b/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs index 4f286884283..2dba46961cf 100644 --- a/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs +++ b/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs @@ -1188,7 +1188,7 @@ public static bool IsColumnNullable(this IReadOnlyProperty property, in StoreObj } return property.IsNullable - || (property.DeclaringType is IReadOnlyEntityType entityType + || (property.DeclaringType.ContainingEntityType is IReadOnlyEntityType entityType && ((entityType.BaseType != null && entityType.GetMappingStrategy() == RelationalAnnotationNames.TphMappingStrategy) || IsOptionalSharingDependent(entityType, storeObject, 0))); diff --git a/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs b/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs index 8a4069c6b07..0b53a762564 100644 --- a/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.ComponentModel.DataAnnotations; using Microsoft.EntityFrameworkCore.Metadata.Internal; using Microsoft.EntityFrameworkCore.Migrations.Internal; using Microsoft.EntityFrameworkCore.Scaffolding.Metadata; @@ -2790,6 +2791,73 @@ await Test( """); } + [ConditionalFact] + public virtual Task Create_table_with_complex_type_with_required_properties_on_derived_entity_in_TPH() + => Test( + builder => { }, + builder => + { + builder.Entity( + "Contact", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.HasKey("Id"); + e.Property("Name"); + e.ToTable("Contacts"); + }); + + builder.Entity( + "Supplier", e => + { + e.HasBaseType("Contact"); + e.Property("Number"); + e.ComplexProperty("MyComplex", ct => + { + ct.ComplexProperty("MyNestedComplex").IsRequired(); + }); + }); + }, + model => + { + var contactsTable = Assert.Single(model.Tables.Where(t => t.Name == "Contacts")); + Assert.Collection( + contactsTable.Columns, + c => Assert.Equal("Id", c.Name), + c => Assert.Equal("Discriminator", c.Name), + c => Assert.Equal("Name", c.Name), + c => Assert.Equal("Number", c.Name), + c => + { + Assert.Equal("MyComplex_Prop", c.Name); + Assert.Equal(true, c.IsNullable); + }, + c => + { + Assert.Equal("MyComplex_MyNestedComplex_Bar", c.Name); + Assert.Equal(true, c.IsNullable); + }, + c => + { + Assert.Equal("MyComplex_MyNestedComplex_Foo", c.Name); + Assert.Equal(true, c.IsNullable); + }); + }); + + protected class MyComplex + { + [Required] + public string Prop { get; set; } + + [Required] + public MyNestedComplex Nested { get; set; } + } + + public class MyNestedComplex + { + public int Foo { get; set; } + public DateTime Bar { get; set; } + } + protected class Person { public int Id { get; set; } diff --git a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs index 34acaa8999f..0009dd61e9f 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs @@ -11101,6 +11101,26 @@ CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]) """); } + [ConditionalFact] + public override async Task Create_table_with_complex_type_with_required_properties_on_derived_entity_in_TPH() + { + await base.Create_table_with_complex_type_with_required_properties_on_derived_entity_in_TPH(); + + AssertSql( +""" +CREATE TABLE [Contacts] ( + [Id] int NOT NULL IDENTITY, + [Discriminator] nvarchar(8) NOT NULL, + [Name] nvarchar(max) NULL, + [Number] int NULL, + [MyComplex_Prop] nvarchar(max) NULL, + [MyComplex_MyNestedComplex_Bar] datetime2 NULL, + [MyComplex_MyNestedComplex_Foo] int NULL, + CONSTRAINT [PK_Contacts] PRIMARY KEY ([Id]) +); +"""); + } + protected override string NonDefaultCollation => _nonDefaultCollation ??= GetDatabaseCollation() == "German_PhoneBook_CI_AS" ? "French_CI_AS" diff --git a/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs index 7e4f0ba454e..2c065ae207f 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs @@ -2081,6 +2081,24 @@ public override Task Rename_sequence() public override Task Move_sequence() => AssertNotSupportedAsync(base.Move_sequence, SqliteStrings.SequencesNotSupported); + public override async Task Create_table_with_complex_type_with_required_properties_on_derived_entity_in_TPH() + { + await base.Create_table_with_complex_type_with_required_properties_on_derived_entity_in_TPH(); + + AssertSql( +""" +CREATE TABLE "Contacts" ( + "Id" INTEGER NOT NULL CONSTRAINT "PK_Contacts" PRIMARY KEY AUTOINCREMENT, + "Discriminator" TEXT NOT NULL, + "Name" TEXT NULL, + "Number" INTEGER NULL, + "MyComplex_Prop" TEXT NULL, + "MyComplex_MyNestedComplex_Bar" TEXT NULL, + "MyComplex_MyNestedComplex_Foo" INTEGER NULL +); +"""); + } + // SQLite does not support schemas protected override bool AssertSchemaNames => false;