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

Create check constraints for columns that are only conditionally nullable #20931

Open
Tracked by #22946 ...
pekspro opened this issue May 12, 2020 · 7 comments
Open
Tracked by #22946 ...

Comments

@pekspro
Copy link

pekspro commented May 12, 2020

I'm having an Order object, with StreetAddress as an owned entity.

public class Order
{
    public int Id { get; set; }
    public StreetAddress ShippingAddress { get; set; }
}

public class StreetAddress
{
    public int PostCode { get; set; }
    public string City { get; set; }
}

In OnModelCreating I have configured both PostCode and City to be required. But despite that I could add an Order without City and save it successfully into the database (I looks correct in the database too). But if I then read back that order ShippingAddress will be null.

Honestly, I am not sure what to expect in this scenario, but this look weird. If I cannot use required fields on owned entities, I will expect an exception if I try to do that. If it is supported, I should get an exception when I try to save properties with missing values.

Steps to reproduce

You could clone: https://github.com/pekspro/OwnedEntitiesTest

Or just run this code:

using Microsoft.EntityFrameworkCore;
using System;
using System.Collections.Generic;
using System.Linq;

namespace EFModeling.OwnedEntities
{

    public class Order
    {
        public int Id { get; set; }
        public StreetAddress ShippingAddress { get; set; }
    }

    public class StreetAddress
    {
        public int PostCode { get; set; }
        public string City { get; set; }
    }

    public class OwnedEntityContext : DbContext
    {
        public DbSet<Order> Orders { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
            => optionsBuilder
                .UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=EFOwnedEntity;Trusted_Connection=True;ConnectRetryCount=0");

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Order>().OwnsOne(
                o => o.ShippingAddress,
                sa =>
                {
                    // NOTE: IsRequired() is used here
                    sa.Property(p => p.PostCode).HasColumnName("PostCode").IsRequired();
                    sa.Property(p => p.City).HasColumnName("ShipsToCity").IsRequired();
                });
        }
    }

    public static class Program
    {
        static void Main(string[] args)
        {
            using (var context = new OwnedEntityContext())
            {
                context.Database.EnsureDeleted();
                context.Database.EnsureCreated();

                context.Add(new Order
                {
                    ShippingAddress = new StreetAddress()
                    {
                        City = null,
                        PostCode = 12345
                    }
                });

                // Why does this works when City i required?
                context.SaveChanges();
            }

            using (var context = new OwnedEntityContext())
            {
                var orders = context.Orders.Include(a => a.ShippingAddress).ToList();

                foreach(var order in orders)
                {
                    // One order is printed here, but ShippingAddress is null even if we should get a value for PostCode.
                    Console.WriteLine($"OrderID: {order.Id} PostCode: {order.ShippingAddress?.PostCode} City: {order?.ShippingAddress?.City}");
                }
            }
        }
    }
}

Further technical details

EF Core version:3.1.3
Target framework: NET Core 3.1

Just for fun I tried with EF Core version 5.0.0-preview.3.20181.2 but it did behave the same.

@ajcvickers
Copy link
Member

@AndriySvyryd @smitpatel I assume the columns are nullable here because of optional owned types?

@pekspro EF Core doesn't do any validation that the entities match the model you define, beyond that which is required to generate correct SQL. This is because doing so is considerable extra overhead in the general case which duplicates validation which must usually be done at higher levels anyway.

@smitpatel
Copy link
Member

From query perspective, ShippingAddress is null because one of the required column has null value.
How SaveChanges allowed that to be saved could be a bug.

@pekspro
Copy link
Author

pekspro commented May 13, 2020

Thanks for a fast response, @ajcvickers and @smitpatel. The fact that EF Core does not validate on save explains a lot. But I am a bit puzzled about:

From query perspective, ShippingAddress is null because one of the required column has null value.

Does this mean that there is validation when data is read from the database? That is a bit unexpected.

Also, ShippingAddress itself is optional now in my model as I understand it. Is it possible to make this a required property? I would like to avoid that PostCode and City is nullable in the database.

@smitpatel
Copy link
Member

See #12100

@ajcvickers ajcvickers changed the title Owned entities with required properties Create check constraints for columns that are only conditionally nullable May 19, 2020
@ajcvickers
Copy link
Member

We discussed the overall situation where a property is required in the model but the column in the database cannot be nullable. The most common case of this is for TPH, but it may also be applicable to table sharing scenarios. Even though the column can't be nullable, we know it can only be null in certain conditions--for example, when the discriminator column has certain values. We could generate check constraints in the database for this.

@pekspro
Copy link
Author

pekspro commented May 21, 2020

Thanks for you are thinking about this, @ajcvickers . In my real-life scenario where I run into this, I changed my strategy to use base classes and interfaces instead. It was good enough for me :-)

@AndriySvyryd
Copy link
Member

Related to #2595

@bricelam bricelam removed their assignment Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment