Update pattern for scaffolding column default constraints

[christopher-landress]

When using database first, creating a table (MSSQL) with a a bit field that has a default value produces strange results.

MSSQL Script:

CREATE TABLE [dbo].[Account] (
	[Id] INT NOT NULL IDENTITY(1,1),
	[EmailAddress] VARCHAR(256) NOT NULL,
	[PasswordSalt] BINARY(512) NOT NULL,
	[PasswordHash] BINARY(512) NOT NULL,
	[Verified] BIT NOT NULL CONSTRAINT DF_Account_Verified DEFAULT (0),
	[UtcDateCreated] DATETIME NOT NULL CONSTRAINT DF_Account_UtcDateCreated DEFAULT (GETUTCDATE()),
	[Active] BIT NOT NULL CONSTRAINT DF_Account_Active DEFAULT (1),
	CONSTRAINT PK_Account_Id PRIMARY KEY ([Id]),
	CONSTRAINT UQ_Account_EmailAddress UNIQUE ([EmailAddress])
);

Scaffolding command:

Scaffold-DbContext "Server=172.16.1.140;Database=DocStudio;User Id=<uid>;Password=<pass>;" Microsoft.EntityFrameworkCore.SqlServer -OutputDir Models -f

Generated model :

using System;
using System.Collections.Generic;

namespace Namespace.Models
{
    public partial class Account
    {
        public int Id { get; set; }
        public string EmailAddress { get; set; }
        public byte[] PasswordSalt { get; set; }
        public byte[] PasswordHash { get; set; }
        public bool Verified { get; set; }
        public DateTime UtcDateCreated { get; set; }
        public bool? Active { get; set; }
    }
}

and finally, the fluent OnModelCreating method:

protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Account>(entity =>
            {
                entity.HasIndex(e => e.EmailAddress)
                    .HasName("UQ_Account_EmailAddress")
                    .IsUnique();

                entity.Property(e => e.Active)
                    .IsRequired()
                    .HasDefaultValueSql("((1))");

                entity.Property(e => e.EmailAddress)
                    .IsRequired()
                    .HasMaxLength(256)
                    .IsUnicode(false);

                entity.Property(e => e.PasswordHash)
                    .IsRequired()
                    .HasMaxLength(512);

                entity.Property(e => e.PasswordSalt)
                    .IsRequired()
                    .HasMaxLength(512);

                entity.Property(e => e.UtcDateCreated)
                    .HasColumnType("datetime")
                    .HasDefaultValueSql("(getutcdate())");
            });
        }

Several issues here. Note the difference between Verified and Active. Despite being identical in their definition they produce different property types in the model.

The lack of a default value for Verified. There should be a definition like so:

                entity.Property(e => e.Verified)
                    .IsRequired()
                    .HasDefaultValueSql("((0))");

Missing a primary key definition.

Finally a warning for Active, but not for Verified:

The column 'dbo.Account.Active' would normally be mapped to a non-nullable bool property, but it has a default constraint. Such a column is mapped to a nullable bool property to allow a difference between setting the property to false and invoking the default constraint.

[olavivaino]

Struggling with the same issue. Its really not desirable to have "bool" in the DB and "bool?" in the code.
I'm wondering what is the reasoning behind this:

The column 'dbo.Account.Active' would normally be mapped to a non-nullable bool property, but it has a default constraint. Such a column is mapped to a nullable bool property to allow a difference between setting the property to false and invoking the default constraint.

[christopher-landress]

The most

Struggling with the same issue. Its really not desirable to have "bool" in the DB and "bool?" in the code.
I'm wondering what is the reasoning behind this:

The column 'dbo.Account.Active' would normally be mapped to a non-nullable bool property, but it has a default constraint. Such a column is mapped to a nullable bool property to allow a difference between setting the property to false and invoking the default constraint.

It seems to be completely indiscriminate too. Verified was defined in the model correctly but is missing the default value in OnModelCreating whereas Active was defined incorrectly in the model but correctly in OnModelCreating. It also got the unique index, but ignored the primary key completely. Makes it impossible to use scaffolding in a build task without constantly fudging with the model. M(sometimes)VC.

[ajcvickers]

@christopher-landress @olavivaino With regard to the boolean default values, can you give some details on you wish these default values to be used from EF? Should they be included in the model, and, if so, why?

@christopher-landress If a primary key is being ignored, can you post the details on the key that is being ignored--this sounds like a bug.

[christopher-landress]

The default value should be whatever the constraint specifies. In the above case Verified should be 0 and Active should be 1... specified by :

CONSTRAINT DF_Account_Verified DEFAULT (0)

            entity.Property(e => e.Verified)
                .IsRequired()
                .HasDefaultValueSql("((0))");

and

CONSTRAINT DF_Account_Active DEFAULT (1)

            entity.Property(e => e.Active)
                .IsRequired()
                .HasDefaultValueSql("((1))");

If NOT NULL & a default constraint are defined for a bit field, it should never be specified with a Boolean? type. I would argue that Boolean? should only be specified when the value could be null, but even then if there is a default it shouldn't be a nullable type.

As far as the primary key is concerned, I would have expected to see the following:

This one appears to be present :

                entity.HasIndex(e => e.EmailAddress)
                    .HasName("UQ_Account_EmailAddress")
                    .IsUnique();

I expected to see the following but it was missing :

                entity.HasKey(e => e.Id)
                .HasName("PK_Account_Id");

This happens with the release version of the tools as well as the pre-release. Very simple to test with the above sql script.

[ajcvickers]

@christopher-landress Assume that the reverse engineering is doing what you expect, such that you have two bool properties in your model, both with HasDefaultValue (or HasDefaultValueSql) set. Can you describe how you will then use these properties? What will the default value do? Why is it there?

(I'm not asking to be mean, but because how to deal with column defaults depends on what they are used for, so I'm trying to understand what your expectation is once the default gets into the model.)

[christopher-landress]

The way I see it, and maybe I am over/under thinking it, is the advantage of having default values for future use. Neither of which are initially important to the insert. Verified would be updated to true once the email address is validated, and Active would work as a soft delete for the record. This could absolutely be accomplished with different schema but I feel like that defeats the purpose of db first. I don't feel like it's unreasonable to set the initial state of the record without intervention, especially when those values aren't designed to be visible. Some of us like to stuff as much logic as we can into the database, assuming it can be used across multiple backends. What is the advantage of having to set Verified and Active explicitly before inserting when my schema is designed to set those values for me?

I also use PostgreSQL, which supports schema inheritance. Having a base table that supports keys, timestamps, and soft deletes is very helpful if I can assume the values that get set are standardized.

Perhaps I am missing your point. Maybe it's a disconnect between developers who like to stuff as much logic into the database as possible and those who don't. I'm having a hard time believing other people aren't having this same issue.

[ajcvickers]

@christopher-landress Consider this entity type:

public class Foo
{
    public int Id { get ;set; }
    public bool IsValid { get ;set; }
}

where IsValid has a column default of true and this is mapped correctly in the EF model.

After inserting the the following entity instance, what would you expect the IsValid column value to be set to?

context.Add(new Foo { IsValid = false });
context.SaveChanges();

Likewise, for this:

context.Add(new Foo { IsValid = true });
context.SaveChanges();

And for this one:

context.Add(new Foo());
context.SaveChanges();

[christopher-landress]

I understand the limitation of the model's type.

public class Foo
{
    public int Id { get ;set; }
    public bool IsValid { get ;set; } = true; //ugly kludge we use
}

Is there no way to fix this in OnModelCreating without using a nullable type?

[ajcvickers]

@christopher-landress We're open to ideas.

[ajcvickers]

Note for triage: thought about this some more last night and while this doesn't work now maybe we could make it work:

public class Blog
{
    public int Id { get; set; }

    private bool? _isValid;

    public bool IsValid
    {
        get => _isValid ?? false;
        set => _isValid = value;
    }
}

modelBuilder
    .Entity<Blog>()
    .Property(e => e.IsValid)
    .HasDefaultValue(true)
    .UsePropertyAccessMode(PropertyAccessMode.Field);

The idea is that the update pipeline reads from the backing field and so can tell whether or not an explicit value has been set. But the entity type still has a non-nullable property so application code doesn't need to be changed to handle this.

[ajcvickers]

Update following further discussion in triage. When we are able to parse the value of the default constraint, then we will use it to generate full client-side defaults. For example:

    public bool IsValid { get; set; } = true;

    public int Lives { get; set; } = 3;

These types will always insert some specific value, but if the value is not set explicitly, then it will match the value that would have been generated by the constraint. (Note that we effectively do this already when the constraint default is the same as the CLR default.)

For cases where the constraint cannot be parsed, we will use the pattern from the previous comment. This will allow the store default to be used, although with more complex entity classes.

[olavivaino]

I would prefer not to have default value in the model at all. In DB default value is mandatory for not null columns when adding column to table with existing DB.
In the code its preferred to excplicitly value properties or if theres sound default logic in place then value them in the constructor or setter. When some default value logic is "hidden" in the db model code then this makes code more difficult to read and its easier to overlook it. Until its too late.

The main issue is that datatype in the model and in the DB are not matching anymore.
At least add option for scaffolding to turn on/off default values in the scaffolded model. And make default off (wink)

[ajcvickers]

@olavivaino Yes, that is a valid option, which is why I was asking how the defaults were going to be used in the model. I think we recognize the need for both of these cases, although it is not clear which should be the default.

[ajcvickers]

See further discussion here: #15070

• For scaffolding:
  • We currently only do C for bools with a default of true. We could consider doing B here, but this could be punted to post 3.0.
  • For other cases, just keep doing what we are doing.

[francisrath]

Sorry for hijacking this issue, but I think it's the most relevant place.

As I understand the discussion, EF Core should already properly map a non-null bool with a database default of (1) to true, not a nullable bool?

I have an existing database (created with EF 6 Code First a few years ago) that I am now accessing from a new EF Core 3.1.3 based client. I'd like to have a clean scaffold, so that I can re-run when I do future migrations in the EF 6 code base.

Relevant part of table definiton:

CREATE TABLE [dbo].[Customers](
	[Id] [int] IDENTITY(1,1) NOT NULL,
	[Options_HasMusicLicense] [bit] NOT NULL,
);

ALTER TABLE [dbo].[Customers] ADD  DEFAULT ((1)) FOR [Options_HasMusicLicense]

Command to scaffold:
ef dbcontext scaffold --context MyContext --data-annotations "(connection string)" Microsoft.EntityFrameworkCore.SqlServer --force

Resulting in:
The column 'dbo.Customers.Options_HasMusicLicense' would normally be mapped to a non-nullable bool property, but it has a default constraint. Such a column is mapped to a nullable bool property to allow a difference between setting the property to false and invoking the default constraint.

Generated class:

public partial class Customer
    {
        [Key]
        public int Id { get; set; }
        [Required]
        [Column("Options_HasMusicLicense")]
        public bool? OptionsHasMusicLicense { get; set; }
    }

And in my model builder:

        modelBuilder.Entity<Customer>(entity =>
        {
            entity.Property(e => e.OptionsHasMusicLicense).HasDefaultValueSql("((1))");
        });

I'd like the property on Customer to be

        [Column("Options_HasMusicLicense")]
        public bool OptionsHasMusicLicense { get; set; } = true;

What am I doing wrong? 🤔

[ajcvickers]

As I understand the discussion, EF Core should already properly map a non-null bool with a database default of (1) to true, not a nullable bool?

This has not been implemented yet.

[ErikEJ]

@ajcvickers I have been thinking about this in relation to ErikEJ/EFCorePowerTools#348 - would it be an option to do the below? (In CSharpEntityTypeGenerator) - or do you prefer to wait for this? #13613 (comment)

        private void GenerateRequiredAttribute(IProperty property)
        {
            if (!property.IsNullable
                && property.GetDefaultValue() != null
                && property.GetDefaultValueSql() != null
                && property.ClrType.IsNullableType()
                && !property.IsPrimaryKey())
            {
                _sb.AppendLine(new AttributeWriter(nameof(RequiredAttribute)).ToString());
            }
        }

[mhkolk]

Just to add another case which uses the Postgres boolean type

is_deleted boolean NOT NULL DEFAULT false

then the scaffolded db context contains this code for the column

entity.Property(e => e.IsDeleted).HasColumnName("is_deleted");

The notion of this column having a default value is lost when scaffolding. When doing an initial migration from this kind of db context the notion of default value is lost there too obviously.

It's the same if I scaffold using --data-annotations.

[mhkolk]

I have manually configured defaults in the model and now there is a warning of

The 'bool' property 'IsDeleted' on entity type 'Asset' is configured with a database-generated default. This default will always be used for inserts when the property has the value 'false', since this is the CLR default for the 'bool' type. Consider using the nullable 'bool?' type instead, so that the default will only be used for inserts when the property value is 'null'.

which doesn't make a lot of sense because it is obvious that false value will always be used when true is not the value of the property and that is completely ok and fine and expected for a boolean value.

I don't understand why the obsession with nullable boolean and the need to display warnings here.

[kingmotley]

I believe the confusion comes in because some may want the database default value to hold special meaning in the EF model object.

I personally really don't have any expectation of that sort. The default value is for migrations so the database knows what value to use when I add the new Boolean column and for 3rd party non-EF software to use. I just expect EF to be able to create/migrate the default value constraint and otherwise ignore it. I don't try to conflate the database default value to having any effect on the C#/EF property at all. If I "new" that class, it won't necessarily initialize to the value of the database default value.

In SQL if you have a bit (same for int/varchar), if you do the equivalent:
INSERT INTO MyTable(mycol) VALUES (0);

the database does not say oh, he's inserting the default value into mycol, but that column has a default value of 1, then he meant to insert a 1. Nor will it accept it if you specify NULL in that column since that column isn't nullable. However, if you do the equivalent:
INSERT INTO MyTable() VALUES ();
then since you have not specified THE COLUMN (which isn't possible in EF, or a model that has a property for every column), then it will use the database default value for that column.

I see these as two separate things. If you could do the following:
public class MyTable { public int Id {get;set;} public bool MyCol {get;set;}} and you _context.MyTables.Add(new MyTable()); then the value of MyCol is false, and it does not matter what the database default value is, it should get inserted as false. However, given the same table, and you also had this class: public class MyTableWithMissingProperty { public int Id {get;set;} } and you _context.MyTables.Add(new MyTableWithMissingProperty()); then yes, since that does not even have the MyCol property, EF should generate an insert statement without the column even mentioned, and the database would then insert the default value. EF currently does not accept a class that does not have all the properties (It can't be both a collection of MyTable and MyTableWithMissingProperty), so that just isn't possible today.

Of course, if I generate my model, and then through another tool (SSMS) add a bool column and assign a default value, and my code continues to use the old model, EF will work as I expect. It will insert rows into the table and use the default value.

I hope that doesn't make things even less clear.

[mhkolk]

EF Core 8 still issues this warning at runtime

The 'bool' property 'IsDeleted' on entity type 'Measurement' is configured with a database-generated default, but has no configured sentinel value. The database-generated default will always be used for inserts when the property has the value 'False', since this is the CLR default for the 'bool' type. Consider using a nullable type, using a nullable backing field, or setting the sentinel value for the property to ensure the database default is used when, and only when, appropriate. See https://aka.ms/efcore-docs-default-values for more information.

for the following property

public bool IsDeleted { get; set; }

As before I still don't understand why the warning - what it is that we should be warned about in this case - don't get it.

If those warnings weren't removed from the new EF version, is there at least an option to disable them?