Validate requiredness (nullability) in the in-memory database

[vnvizitiu]

Description

I created a model that has a name property which I tried setting as required from both the ModelBuilder and using annotations.
When inspecting the tracked changes it confirms that the property IsNullable == false, out of this 2 scenarios arise:

• When using entity framework in memory, then the change is saved in memory with the field being null.
• When using a connection string for localdb then the following NullReferenceException appears

Exception message: Object reference not set to an instance of an object.
Stack trace:       at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware.System.IObserver<System.Collections.Generic.KeyValuePair<System.String,System.Object>>.OnNext(KeyValuePair`2 keyValuePair)
   at System.Diagnostics.DiagnosticListener.Write(String name, Object value)
   at Microsoft.EntityFrameworkCore.Internal.CoreLoggerExtensions.SaveChangesFailed(IDiagnosticsLogger`1 diagnostics, DbContext context, Exception exception)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges()
   at EComm.Front.Data.DbInitializer.Initialize(ApplicationDbContext context) in F:\TFS\Mine\E-Comm\EComm.Front\Data\DbInitializer.cs:line 11

This was confirmed through an NUnit test (for the in-memory database) and in an ASP.NET Core 2 application (for the exception version)

Steps to reproduce

The following code is from the Unit Test

[TestFixture]
public class DbInitializeTests
{
        [Test]
        public void WhenInitializingTheDb_ItShouldStoreTheItems()
        {
            //arrange
            var dbOptions = new DbContextOptionsBuilder<ApplicationDbContext>();
            dbOptions.UseInMemoryDatabase("testdb");
            var dbContext = new ApplicationDbContext(dbOptions.Options);

            //act
            dbContext.Categories.Add(new Category());

            dbContext.SaveChanges();

            //assert
            Assert.That(dbContext.Categories, Is.Not.Empty);
            Assert.That(dbContext.Categories.First().Name, Is.Not.Null.Or.Empty); // fails here, as it the save went ok up to here.
        }
}

    public class Category : Entity<Guid>
    {
        public string Name { get; set; }

        public string NameInUrlFormat => Name.ToLowerInvariant().Replace(" ", "-");
}

public abstract class Entity<T>
{
     public T Id { get; set; }
}

public class ApplicationDbContext : IdentityDbContext<ApplicationUser>
{
        public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options)
            : base(options)
        {
        }

        public DbSet<Category> Categories { get; set; }

        protected override void OnModelCreating(ModelBuilder builder)
        {
            base.OnModelCreating(builder);
            // Customize the ASP.NET Identity model and override the defaults if needed.
            // For example, you can rename the ASP.NET Identity table names and more.
            // Add your customizations after calling base.OnModelCreating(builder);

            builder.Entity<Category>(
                typeBuilder => 
                    typeBuilder.Property(category => category.Name)
                    .IsRequired()
                    .IsUnicode()
                    .HasMaxLength(100));
        }
 }

Expected Results

The SaveChanges() method should have thrown an exception since the model requirements were not fulfiled

Further technical details

EF Core version: 2.0.1
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10 Pro
IDE: Visual Studio 2017 15.5.2

Mentions

This issue was also mentioned in the documentation found here in the first and last comments.

[ajcvickers]

@vnvizitiu Several points on this:

• EF Core doesn't do any validation of entities beyond what is needed for internal consistency. Validation is something that could be done in EF, but experience shows that it is not something that is useful to many developers because it usually cannot replace either client-side validation or database validation and there are also other places where validation can be done more effectively.
• For the in-memory case, the in-memory database does not currently validate nullability (i.e. requiredness) when saving property values. This was previously reported in Validating entities using data annotations or fluent api (In Memory) #7228 and it was decided not to change this, but I'm leaving this issue open so we can discuss again in triage.
• For the SQL Server case, SQL Server is throwing an exception as expected, but it looks like you are hitting the issue reported in Insert entity in non existing table throws NullException #9599

[vnvizitiu]

Thanks for the quick reply.

I looked through the mentioned issues. It was my understanding that the in-memory implementation was a way to help out with unit testing, personally, I kinda see it as an issue if the test behaves one way and the production behaves a different way.

Also, I did not test this yet (but I will eventually when the domain grows), but if nullability is not checked, does this mean foreign key constraints do not apply as well?

On a side note, please also consider the confusion with the documentation since this was also brought up there.

[ajcvickers]

@vnvizitiu The in-memory database is not a relational database and so it does not always have the semantics of a relational database, as described here: https://docs.microsoft.com/en-us/ef/core/miscellaneous/testing/ and here: https://docs.microsoft.com/en-us/ef/core/miscellaneous/testing/in-memory. If you need to test with relational semantics, then you may be better off using SQLite in-memory: https://docs.microsoft.com/en-us/ef/core/miscellaneous/testing/sqlite, but even then SQLite will likely behave differently than your production database server. In general, what you use for testing depends on how much your tests depend on actual database behavior. The only way to ensure the same behavior is to use the same database engine.

[vnvizitiu]

Thanks for the info, sorry I couldn't respond earlier, was away

[lioobayoyo]

I'm also interested in EF In Memory DB handling correctly the requiredness of fields like string, for unit (or not-so-unit)- testing purposes.

I don't really understand what "up-for-grabs" tag means. Does it indicate that the enhancement will be grabbed in some near-future next version ?

[ajcvickers]

@lioobayoyo "up-for-grabs" means that we believe the fix for this issue is straightforward enough that somebody without deep knowledge of EF Core internals could likely create a pull request for it. In other words, it's a good first-time issue for somebody wanting to contribute to EF Core.

[Saibamen]

Hello Arthur.
Any update? Maybe it is the right time to look on this issue again while we are waiting for Entity Framework Core 5?

[ajcvickers]

@Saibamen I'm not sure what you are asking. Can you elaborate?

[Saibamen]

Ok, let me write why we need in-memory testing.

We do a database migration from EF6 to EF Core 5.0 and we want to write in-memory tests to check if our Context is compatible with old one (like checking if Validation Errors are correct - trying to add too long string and checking if Exception from HasMaxLength(50) is thrown). We can't do this because of this issue.

We tried SQLite, but we failed at the DB configuration, because we have duplicated indexes (we can't change that, because we need to be compatible with old DB) - issue #14548

[fagnercarvalho]

Hey @ajcvickers, can I take this one?

I would add a ValidateNullability method inside the Update and Create methods in the InMemoryTable class and then follow a logic similar to this one for each property:

public override int SaveChanges()
{
	var entries = ChangeTracker.Entries()
				   .Where(e => e.State == EntityState.Modified || e.State == EntityState.Added)
				   .ToList();

	foreach (var entry in entries)
	{
		var properties = entry.Metadata.GetProperties();

		foreach(var property in properties)
		{
			var value = property.PropertyInfo.GetValue(entry.Entity);
			if (!property.IsNullable && value == null) 
			{
				throw new DbUpdateException("Required properties are missing");
			}
		}
	}

	return base.SaveChanges();
}

For the exception message I would take an approach similar to how concurrency conflicts are handled today in the InMemoryTable class like this:

Required property 'Name' is missing for instance of entity type '<Entity>' with the key value '{Id: 1}'.

Required properties {Name, Test} are missing for instance of entity type '<Entity>' with the key value '{Id: 1}'.

[ajcvickers]

@vnvizitiu Sorry for the slow response--I've been out for a bit. Yes, feel free to take this on. The approach you suggest seems reasonable. However, this will be a breaking change, so we may want to:

• Allow the new behavior to be disabled
• Consider whether or not the new behavior should be disabled by default

@dotnet/efteam Any thoughts on the breaking nature of this?

[roji]

Considering that the main use of InMemory is for testing code that otherwise uses relational databases, I'd vote for enabling by default but possibly providing an opt-out knob.

[fagnercarvalho]

Thank you, this makes sense @ajcvickers and @roji! I will create a PR in the next few days.