Efcore: Re-consider how default values are reverse engineered

[ajcvickers]

Feedback on 2.0 (e.g. #9507 #9617) has made me think about what the real intent is behind having columns with default values. Specifically, I think it is likely wrong to assume that the EF model should have a store-generated property just because there is some default on the column. It may well be the cause the the application will always supply a value and so, when using EF, the default is never used. The default may instead be there because:

• It was needed when adding a new column (either by Migrations or manually)
• It is there for legacy reasons--old/non-EF app made use of it in hand-crafted SQL, but EF never will
• It's just part of the way all columns are created by the DBA or similar--the column gets a default regardless of whether it is needed or not

Going beyond the most general case, if the CLR default would generate the same value as the database default, then having EF do store generation is a no-op anyway. If an app was relying on this for bools in 1.1, and we now generate nullable bools, everything will still work, but the app needs to be updated to use nullable bools.

[ajcvickers]

Triage: we will update reverse engineering to not make the property store-generated if the default value from the column matches the default value of the CLR property.

[JuanIrigoyen]

This change break with the model, if you need to use the default value in a entity, the logical is set these in the constructor of the class or in the property how default value. The problem is when the database modify the record after insert with a trigger, calculate fields, another procedure or function of transact-Sql that EF don´t know. For example if your default value is a function in sql server how default([dbo].[NEWDOC_Order]()) EF never to know this. In this case the default value will be lost and you always must to read the records inserted before return it to EF, but if EF know the function, numeric or string values, getdate() or other basic functions these can be set in the constructor or in the properties. If I have a bool field and in my model the field refer to bool?, the validations an others functions are different. In numeric fields If I make a sum, I would be forced to change the function to not read the null values. If I have a default value in a table with not null in my model must be the same because I must to change my validations and programs for this cause. I´m sure this behavior is not correct.

In my opinion the use of default values ​​should be something like this:

public partial class Customer
{
    public Customer()
    {
        Date = DateTime.Now;    // Default Value "Getdate()"
        Total = 100;            // Default Value 100
    }

    public string CustomerId { get; set; }
    public string Name { get; set; }

    [DefaultValue("Getdate()")]
    public DateTime Date { get; set; }

    [DefaultValue(100)]
    public decimal Total { get; set; }
}

// Or using default values in properties

public partial class Customer
{
    public Customer()
    {

    }

    public int CustomerId { get; set; }

    public string Name { get; set; }

    public DateTime Date { get; set; } = System.DateTime.Now;       // Default Value "Getdate()"

    public decimal Total { get; set; } = 100;                       // Default Value 100
}

[ryanelian]

Can you guys just hotfix this nullable bool scaffold behavior instead of shipping on 2.1.0? Maybe ship a 2.0.1? Just add a quick flag when invoking the command line scaffold tool or something perhaps, please?

Currently my workaround is to downgrade the separate project containing the DbContext class + entity classes + the EF Core + its dotnet ef CLI tool to 1.1.2, re-scaffold (using a handy PowerShell script) then re-upgrade to 2.0.0, but it's quite a stupid thing to do...

I mean, 2.1.0 / Q1 2018 is a pretty long wait...

EDIT: ErikEJ's Toolbox is very cool. Love it. But I still prefer my black and white console: .\scaffold-dbcontext.ps1 and be done without a single mouse click...

[ErikEJ]

@ryanelian you can use my SQLite Toolbox to generate the v1 scaffolding, without installing anything in your solution 😀 - also works for SQL Server

[ajcvickers]

Triage: we plan to make the following change in 2.0.1: for non-nullable bool columns in SQL Server that have a column default of false, we will scaffold a non-nullable bool property without any store generation flags.

Post 2.0.1 we will do what is described in the triage comment above.

We will not change the behavior where the default in the database is not the CLR default. Specifically, for bool columns that have a default of true, we will still scaffold a nullable bool. This is because non-nullable bool properties with a store-generated value of true are not useful--the value in the database will always be true--see #8400 and #7163 for more details.

[JuanIrigoyen]

I can´t understand. 'non-nullable bool properties with a store-generated value of true are not useful'. If you want to insert same fields and you have only two values (only true or false) without null values. I have this configuration in more than hundred forms. For me it´s the same problem. In the database you have 'bool not null default value(1)' and in the model you have bool?.... Please at least set a parameter in the scaffolding for control this behavior.

In the photo you can see when I insert a record I have Monday, Tuesday, Wednesday, etc, Same fields have true and others with false how default values. In the database all fields are bool not nullable with 1 or 0 how default value. All use the same control based in a checkedit control that don´t let null values.

[ajcvickers]

@JuanIrigoyen This is the scenario I am talking about. Entity with a non-nullable bool:
```C#
public class Fooo
{
public int Id { get; set; }
public bool Flag { get; set; }
}

Configured in EF to have a store-generated default of true:
```C#
modelBuilder
     .Entity<Fooo>()
     .Property(e => e.Flag)
     .HasDefaultValueSql("(1)");
 ```
With this configuration, Flag will always be saved as true; never false, and hence is not useful. For example:
```C#
using (var context = new TestContext())
{
    context.Database.EnsureDeleted();
    context.Database.EnsureCreated();

    context.Add(new Fooo { Flag = false });
    context.SaveChanges();
}

using (var context = new TestContext())
{
    Debug.Assert(context.Fooos.First().Flag);
}

This behavior hasn't changed from EF Core 1.0--there's lots of discussion in the linked issues that talk about this. The most complete is probably in #7089. Essentially, the issue is that the sentinel to tell EF to use use a store-generated value is the same as the value 'false'. The ways to make this work are:

• Stop having EF make use of the store generated value--e.g. by removing the HasDefaultValue call.
• Make the property nullable so that the sentinel ('null') is different from the valid value 'false'.

[JuanIrigoyen]

ok, I understand, thank you for the explanation, In this case I prefer the first option, because a bool? value is different from bool and it is important that the model is similar to the table in the database. In this case the default value is deleted for the value set and it´s not necessary to call HasDefaultValue method.

If you set the property nullable we need to change all controls because even the bool nullable in a forms is represented differently. bool? can show three states.

[ryanelian]

This is because non-nullable bool properties with a store-generated value of true are not useful--the value in the database will always be true--

I can't understand.

This is my table:

CREATE TABLE Employee
(
    EmployeeId VARCHAR(64) CONSTRAINT PK_Employee PRIMARY KEY,
    Username VARCHAR(256) CONSTRAINT UQ_Employee_Username UNIQUE NOT NULL,
    Password VARCHAR(256),
    PasswordUpdatedAt DATETIME2,
    Name VARCHAR(256) NOT NULL,
    Email VARCHAR(256),
    IsEnabled BIT NOT NULL DEFAULT 1,
)

This is from EF Core 1.1.2 scaffold (navigation properties and some stuffs are omitted for brevity):

public partial class Employee
{
    [Column(TypeName = "varchar(64)")]
    public string EmployeeId { get; set; }
    [Required]
    [Column(TypeName = "varchar(256)")]
    public string Username { get; set; }
    [Column(TypeName = "varchar(256)")]
    public string Password { get; set; }
    public DateTime? PasswordUpdatedAt { get; set; }
    [Required]
    [Column(TypeName = "varchar(256)")]
    public string Name { get; set; }
    [Column(TypeName = "varchar(256)")]
    public string Email { get; set; }
    public bool IsEnabled { get; set; }
}

public partial class MyDbContext : DbContext
{
    public virtual DbSet<Employee> Employee { get; set; }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Employee>(entity =>
        {
            entity.HasIndex(e => e.Username)
                .HasName("UQ_Employee_Username")
                .IsUnique();

            entity.Property(e => e.IsEnabled).HasDefaultValueSql("1");
        });
    }
}

This is the C# code for setting IsEnabled to false on rows already created (update operation). Nothing fancy:

e.IsEnabled = form.IsEnabled;
await DB.SaveChangesAsync();

The field is very clearly set to false when I view the data directly using SQL Server Management Studio. (Forgive me for not showing the other data, they're abit confidential):

Flag will always be saved as true; never false, and hence is not useful.

Please help me understand this phenomenon.
(Sorry if I misunderstood the previously shown scenario / your example above.)

[ajcvickers]

@ryanelian It can be updated to false, but never inserted as false.

[ryanelian]

Ahh. That explains it.

Perhaps to maintain compatibility with database context created with version 1 scaffold, BIT NOT NULL DEFAULT X properties should be scaffolded with Auto-Property Initializers as an interim / temporary / emergency solution?

public partial class Employee
{
    public bool IsEnabled { get; set; } = true; // or false
}

i.e. For now, no need to implement that for each and every data type. Just make an compatibility flag to scaffold boolean property like that, please?

That way you won't break compat for 2.0.0 and 1.X.X users...

[ajcvickers]

Hi @ryanelian. We are gathering information on the use of EF Core pre-release builds. You reported this issue shortly after the release of 2.0.0 RTM. It would be really helpful if you could let us know:

• Did you consider testing your code against the pre-release builds?
• Was there anything blocking you from using pre-release builds?
• What do you think could make it easier for you to use pre-release builds in the future?

Thanks in advance for any feedback. Hopefully this will help us to increase the value of pre-release builds going forward.

[ryanelian]

Did you consider testing your code against the pre-release builds?

Yes

Was there anything blocking you from using pre-release builds?

When I installed 2.0.0 preview1 SDK on my machine, I was not able to compile my 1.1.X project from Visual Studio 2017 Update 2 even when global.json exists for unknown reason. (AKA I cannot work.)

So I stopped playing with pre-release builds...

What do you think could make it easier for you to use pre-release builds in the future?

Don't break Visual Studio 😕

[smitpatel]

Impact: Refer to first post.
Risk: Low. Only in SqlServer, Columns with type bit and default value of (0) will not get default value constraint and will scaffold non-nullable bool which would match with 1.1 behavior.

[ajcvickers]

Created #9826 for the 2.0.1 part of this; marking this issue for re-triage

[ajcvickers]

Triage: for 2.1, we will do other simple types (e.g. ints) like we did in 2.0.1, and also look at what we might do for other providers. We may also choose to do more (such as a flag to turn off generating store-generated properties for columns with default constraints), but that probably requires more discussion

[ajcvickers]

@ryanelian Thanks for the feedback; much appreciated.

[ryanelian]

You're welcome.

Also, +1 to completely turning off default values for generated properties using a flag...

Off-topic but related, is there a way to disable the warnings when using 1.1 generated entities with EF Core 2?

2017-09-02 02:56:01.522 +07:00 [Warning] The 'bool' property '"IsEnabled"' on entity type '"Employee"' is configured with a database-generated default. This default will always be used 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 when the property value is 'null'.

This is distracting me from the actual warning logs in the application...

[ajcvickers]

@ryanelian Yes, for example:
C# optionsBuilder .ConfigureWarnings(wc => wc.Ignore(RelationalEventId.BoolWithDefaultWarning)) .UseSqlServer(...);

[RandyBuchholz]

Thinking forward on this, what are the long term implications? C# is moving towards not null reference types. So later on I'll have my strings defaulting to not nullable. Databases like Oracle use strings for bools. So if we follow this approach, will EF make these strings nullable too is they have a default of True?

Overall I think the team is doing a great job on EF, but it seems that sometimes the decisions are based on a Code-First dictate. In a db heavy shop that uses DBA's to develop their databases, when a developer comes to them and says EF is going to ignore your mandatory column with a default of True, (but by the way, you can use default is False) and change it's type, shops my rethink the use of EF.

[cgountanis]

Personally I would love to just use the model classes and simple annotations. Right now I get this bool default value warning and I am forced to use the modelBuilder for simple default values on table creation within SQL Server. I see no reason to set the value on the class property as it does nothing for IntialCreate so I was forced to use a config for every model, which is OK, I like the control but since we are looking into the warning why not fix it all, aye? I would like to use one or the other, right now I am using both e.g. [Required, StringLength(100)] and IEntityTypeConfiguration.

public void Configure(EntityTypeBuilder<SystemUser> builder)
{
    builder.HasKey(o => o.Id);
    builder.Property(o => o.Id).HasDefaultValueSql("newsequentialid()");
    builder.Property(o => o.Created).HasDefaultValueSql("getutcdate()");
    builder.Property(o => o.Disabled).HasDefaultValue(false);
 }

[cgountanis]

This does work but seems like a workaround.

// Add DbContext SSOContext using connection string from settings.
services.AddDbContext<SSOContext>(options =>
{
    options.UseSqlServer(_configuration.GetConnectionString("DefaultConnection"));
    options.ConfigureWarnings(wc => wc.Ignore(RelationalEventId.BoolWithDefaultWarning));
});

[dapug]

I'm not seeing the value in a nullable bool when the DB requires not-null. It's _going_ to result in true or false in the DB in the end, which means the app also has no _third_ choice, even if the dev is too lazy to set the value. The default value in the DB is pointless IMO, as long as the entity doesn't allow null, which is great (i.e. should not need to set a default value on the DB end, because the entity should be forced to ALWAYS provide one value or another... which ironically is completely opposite of what nullable bool accomplishes). Not interested in changing my app to handle this. Based on this and other threads, if it can't be decided which way to go, then just add a flag to let us enforce what to generate, giving us three options:

Use bool
Use bool?
Use auto-property for default value (public bool IsEnabled { get; set; } = true;)

[ajcvickers]

Note from triage: post 2.1 we should consider adding a flag that will cause reverse engineering to ignore column defaults when defining the EF model. However, some thought needs to go into how this would interact with update-from-database. Leaving it up to @bricelam to decide how to track this--using an existing issue or creating a new one.

[bricelam]

Filed #10881 (Option to ignore DEFAULT constraints)

[bricelam]

Notes from implementing this:

1. Some tools remove DEFAULT constraints by setting them to NULL. This doesn't remove the constraint, but effectively disables it.
2. Adding NOT NULL columns requires specifying a DEFAULT (typically 0). We do this in Migrations.

Both these classes will now be ignored by Reverse Engineering. 🎉

[ErikEJ]

@bricelam Are the default values generated by scaffolding used by EF anywhere?
Or are they there for roundtrippabilty

[bricelam]

By reverse engineering .HasDefaultValueSql("something") EF will not specify a value in the INSERT statement when sentinel value (CLR default by convention) is present (causing the default value to be generated), and after inserting it will propagate the generated value back into the entity.

[bricelam]

So the actual value only really matters for round-tripping, but since we have to put something...

[ErikEJ]

OK, so it is used by the Update pipeline - thanks! I will add them, then:
https://github.com/ErikEJ/SqlCeToolbox/blob/master/src/GUI/ReverseEngineer20/ReverseEngineer/SqlServerDacpacDatabaseModelFactory.cs#L263

And what about sequences in the Model?

[bricelam]

Sequences, constraint names, non-unique indexes, and index filters aren't currently used by the runtime and are mostly there for round-tripping.

[johnkwaters]

I ran in to this today too. I was wanting to set a default value with my code first DB generation, so that the column would end up with XXX bit NOT NULL DEFAULT 0
The reason I do this is if you are inserting manually (not through EF), you don't want to have to specify the default values. But if there is no default, you cannot omit them in your insert. Hence, a useful default.
Your line of reasoning seems flawed in that it centers on EF being the ONLY access path to the DB to consider. But what about my case where you want to generate a schema that is friendly to other access patterns, without a bunch of warnings?

[cgountanis]

You can either wait for the version that it is fixed or use the ignore when creating the connection in startup. Pretty sure there was an example above.

[cgountanis]

I thought the newer versions were going to fix the need for BoolWithDefaultWarning on bool properties within a model.

[ajcvickers]

@cgountanis That warning should not be generated when reverse engineering with the 2.1 tools. Can you please create a new issue describing specifically when you are seeing this? Please include the column definition and the generated code.

[cgountanis]

@ajcvickers https://github.com/aspnet/EntityFrameworkCore/issues/12198