From 934ee257c9a7f9b625a416819daa3ddd876e8888 Mon Sep 17 00:00:00 2001 From: maumar Date: Sat, 10 Feb 2024 15:20:34 -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. --- .../RelationalPropertyExtensions.cs | 9 ++- .../Migrations/MigrationsTestBase.cs | 67 +++++++++++++++++++ .../Migrations/MigrationsSqlServerTest.cs | 19 ++++++ .../Migrations/MigrationsSqliteTest.cs | 18 +++++ 4 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs b/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs index 0b0d0b7f410..929fb4f470e 100644 --- a/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs +++ b/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs @@ -20,6 +20,9 @@ public static class RelationalPropertyExtensions private static readonly bool UseOldBehavior32763 = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue32763", out var enabled32763) && enabled32763; + private static readonly bool UseOldBehavior33004 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue33004", out var enabled33004) && enabled33004; + private static readonly MethodInfo GetFieldValueMethod = typeof(DbDataReader).GetRuntimeMethod(nameof(DbDataReader.GetFieldValue), new[] { typeof(int) })!; @@ -1191,8 +1194,12 @@ public static bool IsColumnNullable(this IReadOnlyProperty property, in StoreObj return sharedTableRootProperty.IsColumnNullable(storeObject); } + var declaringEntityType = UseOldBehavior33004 + ? property.DeclaringType + : property.DeclaringType.ContainingEntityType; + return property.IsNullable - || (property.DeclaringType is IReadOnlyEntityType entityType + || (declaringEntityType 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 7ce11ea2355..eef96d80c95 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; @@ -2141,6 +2142,72 @@ public virtual Task Create_table_with_optional_primitive_collection() Assert.Single(customersTable.PrimaryKey!.Columns)); }); + [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 fd327ecba19..f8c618f88ca 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs @@ -9328,6 +9328,25 @@ CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]) """); } + 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 f0179b94b70..1dfe9c2d5a1 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs @@ -1702,6 +1702,24 @@ await Test( """); } + 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 +); +"""); + } + public override Task Create_sequence() => AssertNotSupportedAsync(base.Create_sequence, SqliteStrings.SequencesNotSupported);