Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unfulfillable nullable contraints are set for complex types with TPH entities #33004

Closed
Wasserwecken opened this issue Feb 5, 2024 · 6 comments · Fixed by #33052
Closed

Unfulfillable nullable contraints are set for complex types with TPH entities #33004

Wasserwecken opened this issue Feb 5, 2024 · 6 comments · Fixed by #33052
Assignees
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Milestone

Comments

@Wasserwecken
Copy link

Wasserwecken commented Feb 5, 2024

Usually, if a field has the Required attribute it is mapped as nullable: false.
But if this field is declared in a TPH entity, that condition may not be possible.

Unfortunately, required-flaged fields within complex types are set as NOT NULL anyway,
which prevents persisting entities that do not implement those fields.

image

Core for reproduction

Migrations are required (Add-Migration Init -c CustomContext5)

using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Diagnostics;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;

namespace ConsoleApp5
{
    internal static class Program5
    {
        public static void Test(string[] args)
        {
            var supplier = new Supplier() { Name = "Supplier", Extra = new() { Field = "Field" } };
            var customer = new Customer() { Name = "Customer" };

            using (var ctx = new CustomContext5())
            {
                ctx.Database.Migrate();
                ctx.Add(supplier);
                ctx.SaveChanges();

                ctx.Add(customer);
                ctx.SaveChanges();
            }

            Console.WriteLine("Done!");
            Console.ReadLine();
        }

        public class CustomContext5 : DbContext
        {
            public DbSet<Contact> Contacts { get; set; }
            public DbSet<Supplier> Suppliers { get; set; }
            public DbSet<Customer> Customers { get; set; }

            protected override void OnConfiguring(DbContextOptionsBuilder builder)
            {
                builder.LogTo(Console.WriteLine, new[] { RelationalEventId.CommandExecuting }).EnableSensitiveDataLogging();
                builder.UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=TestDb;Trusted_Connection=True");
            }

            protected override void OnModelCreating(ModelBuilder builder)
            {
                builder.Entity<Contact>().HasDiscriminator(e => e.Discriminator);
            }
        }

        public abstract class Contact
        {
            public int Id { get; set; }
            public string? Discriminator { get; set; }
            public string? Name { get; set; }
        }

        public class Supplier : Contact
        {
            [Required]
            public string? AnotherExtra { get; set; }
            public required Extra Extra { get; set; }
        }

        public class Customer : Contact
        {
        }

        [ComplexType]
        public class Extra
        {
            [Required]
            public string? Field { get; set; }
        }
    }
}

Include provider and version information

EF Core version: 8.0.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer

@Wasserwecken
Copy link
Author

Wasserwecken commented Feb 5, 2024

Minor note: This also affects atomic field types such as int and bool that are not flagged with Required.

@ajcvickers
Copy link
Member

Note for team: still repros on latest daily. Patch candidate.

@maumar
Copy link
Contributor

maumar commented Feb 10, 2024

problem is in RelationalPropertyExtensions.IsColumnNullable - there we try to compute nullability of a column that the given property is mapped to. For inheritance scenario we look at declaring type and check if it's part of TPH. However for property of a complex type, declaring type is that complex type so we don't do the proper check. We should find the parent non-complex type instead

maumar added a commit that referenced this issue Feb 10, 2024
… 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
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Feb 10, 2024
@roji
Copy link
Member

roji commented Feb 10, 2024

Should we consider patching this (bug in new feature, extremely low risk)?

maumar added a commit that referenced this issue Feb 10, 2024
… types with TPH entities (#33052)

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
@maumar
Copy link
Contributor

maumar commented Feb 10, 2024

reopening for patch

@maumar maumar reopened this Feb 10, 2024
maumar added a commit that referenced this issue Feb 10, 2024
… 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.
@ajcvickers ajcvickers modified the milestones: 8.0.x, 8.0.4 Feb 15, 2024
maumar added a commit that referenced this issue Mar 5, 2024
… 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.
maumar added a commit that referenced this issue Mar 5, 2024
… types with TPH entities (#33054)

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.
@maumar
Copy link
Contributor

maumar commented Mar 5, 2024

merged to release/8.0, closing

@maumar maumar closed this as completed Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Projects
None yet
4 participants